diff mbox series

[Xen-devel,for-4.12,v2,4/8] xen/arm: Add support for read-only foreign mappings

Message ID 20181220192338.17526-5-julien.grall@arm.com
State New
Headers show
Series xen/arm: Add xentrace support | expand

Commit Message

Julien Grall Dec. 20, 2018, 7:23 p.m. UTC 
Current, foreign mappings can only be read-write. A follow-up patch will
extend foreign mapping for Xen backend memory (via XEN_DOMID), some of
that memory should only be read accessible for the mapping domain.

Introduce a new p2m_type to cater read-only foreign mappings. For now,
the decision between the two foreign mapping type is based on the type
of the guest page mapped.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

---

    Changes in v2:
        - Add Andrii's reviewed-by
---
 xen/arch/arm/mm.c         |  2 +-
 xen/arch/arm/p2m.c        |  1 +
 xen/include/asm-arm/p2m.h | 42 +++++++++++++++++++++++++++++++++++++++---
 3 files changed, 41 insertions(+), 4 deletions(-)

Comments

Stefano Stabellini Dec. 20, 2018, 11:07 p.m. UTC | #1 
On Thu, 20 Dec 2018, Julien Grall wrote:
> Current, foreign mappings can only be read-write. A follow-up patch will
> extend foreign mapping for Xen backend memory (via XEN_DOMID), some of
> that memory should only be read accessible for the mapping domain.
> 
> Introduce a new p2m_type to cater read-only foreign mappings. For now,
> the decision between the two foreign mapping type is based on the type
> of the guest page mapped.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> 
> ---
> 
>     Changes in v2:
>         - Add Andrii's reviewed-by
> ---
>  xen/arch/arm/mm.c         |  2 +-
>  xen/arch/arm/p2m.c        |  1 +
>  xen/include/asm-arm/p2m.h | 42 +++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 41 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 7193d83b44..58f7e54640 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1267,7 +1267,7 @@ int xenmem_add_to_physmap_one(
>          }
>  
>          mfn = page_to_mfn(page);
> -        t = p2m_map_foreign_rw;
> +        t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;

I know there is a p2m_is_ram check close by, but I think it would still
be better to do:

  if (p2mt == p2m_ram_rw)
    t = p2m_map_foreign_rw;
  else if (p2mt == p2m_ram_ro)
    t = p2m_map_foreign_ro;
  else
    error

to avoid cases where p2mt is something completely different
(p2m_mmio_direct_dev for instance) and t gets set to p2m_map_foreign_ro
by mistake.


>          rcu_unlock_domain(od);
>          break;
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index e0b84a9db5..dea04ef66f 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -477,6 +477,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
>          break;
>  
>      case p2m_iommu_map_ro:
> +    case p2m_map_foreign_ro:
>      case p2m_grant_map_ro:
>      case p2m_invalid:
>          e->p2m.xn = 1;
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index a1aef7b793..6f2728e2bb 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -116,6 +116,7 @@ typedef enum {
>      p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area non-cacheable */
>      p2m_mmio_direct_c,  /* Read/write mapping of genuine MMIO area cacheable */
>      p2m_map_foreign_rw, /* Read/write RAM pages from foreign domain */
> +    p2m_map_foreign_ro, /* Read-only RAM pages from foreign domain */
>      p2m_grant_map_rw,   /* Read/write grant mapping */
>      p2m_grant_map_ro,   /* Read-only grant mapping */
>      /* The types below are only used to decide the page attribute in the P2M */
> @@ -135,12 +136,16 @@ typedef enum {
>  #define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw) |  \
>                           p2m_to_mask(p2m_grant_map_ro))
>  
> +/* Foreign mappings types */
> +#define P2M_FOREIGN_TYPES (p2m_to_mask(p2m_map_foreign_rw) | \
> +                           p2m_to_mask(p2m_map_foreign_ro))
> +
>  /* Useful predicates */
>  #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES)
> -#define p2m_is_foreign(_t) (p2m_to_mask(_t) & p2m_to_mask(p2m_map_foreign_rw))
> +#define p2m_is_foreign(_t) (p2m_to_mask(_t) & P2M_FOREIGN_TYPES)
>  #define p2m_is_any_ram(_t) (p2m_to_mask(_t) &                   \
>                              (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \
> -                             p2m_to_mask(p2m_map_foreign_rw)))
> +                             P2M_FOREIGN_TYPES))
>  
>  /* All common type definitions should live ahead of this inclusion. */
>  #ifdef _XEN_P2M_COMMON_H
> @@ -295,7 +300,38 @@ struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn,
>  static inline struct page_info *get_page_from_gfn(
>      struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
>  {
> -    return p2m_get_page_from_gfn(d, _gfn(gfn), t);
> +    mfn_t mfn;
> +    p2m_type_t _t;
> +    struct page_info *page;
> +
> +    /*
> +     * Special case for DOMID_XEN as it is the only domain so far that is
> +     * not auto-translated.
> +     */
> +    if ( unlikely(d != dom_xen) )

Why unlikely?


> +        return p2m_get_page_from_gfn(d, _gfn(gfn), t);
> +
> +    if ( !t )
> +        t = &_t;
> +
> +    *t = p2m_invalid;
> +
> +    /*
> +     * DOMID_XEN see 1-1 RAM. The p2m_type is based on the type of the
                    ^sees

> +     * page.
> +     */
> +    mfn = _mfn(gfn);
> +    page = mfn_to_page(mfn);
> +
> +    if ( !mfn_valid(mfn) || !get_page(page, d) )
> +        return NULL;
> +
> +    if ( page->u.inuse.type_info & PGT_writable_page )
> +        *t = p2m_ram_rw;
> +    else
> +        *t = p2m_ram_ro;
> +
> +    return page;
>  }
>  
>  int get_page_type(struct page_info *page, unsigned long type);
> -- 
> 2.11.0
>
Julien Grall Dec. 20, 2018, 11:26 p.m. UTC | #2 
(Sorry for the formatting)

On Thu, 20 Dec 2018 at 23:08, Stefano Stabellini <sstabellini@kernel.org>
wrote:

> On Thu, 20 Dec 2018, Julien Grall wrote:

> > Current, foreign mappings can only be read-write. A follow-up patch will

> > extend foreign mapping for Xen backend memory (via XEN_DOMID), some of

> > that memory should only be read accessible for the mapping domain.

> >

> > Introduce a new p2m_type to cater read-only foreign mappings. For now,

> > the decision between the two foreign mapping type is based on the type

> > of the guest page mapped.

> >

> > Signed-off-by: Julien Grall <julien.grall@arm.com>

> > Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

> >

> > ---

> >

> >     Changes in v2:

> >         - Add Andrii's reviewed-by

> > ---

> >  xen/arch/arm/mm.c         |  2 +-

> >  xen/arch/arm/p2m.c        |  1 +

> >  xen/include/asm-arm/p2m.h | 42

> +++++++++++++++++++++++++++++++++++++++---

> >  3 files changed, 41 insertions(+), 4 deletions(-)

> >

> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c

> > index 7193d83b44..58f7e54640 100644

> > --- a/xen/arch/arm/mm.c

> > +++ b/xen/arch/arm/mm.c

> > @@ -1267,7 +1267,7 @@ int xenmem_add_to_physmap_one(

> >          }

> >

> >          mfn = page_to_mfn(page);

> > -        t = p2m_map_foreign_rw;

> > +        t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw :

> p2m_map_foreign_ro;

>

> I know there is a p2m_is_ram check close by, but I think it would still

> be better to do:

>

>   if (p2mt == p2m_ram_rw)

>     t = p2m_map_foreign_rw;

>   else if (p2mt == p2m_ram_ro)

>     t = p2m_map_foreign_ro;

>   else

>     error

>

> to avoid cases where p2mt is something completely different

> (p2m_mmio_direct_dev for instance) and t gets set to p2m_map_foreign_ro

> by mistake.



The case you suggest is impossible. You can only be here if you manage to
get a reference on the page (e.g p2m_foreign or p2m_ram).
The check above remove the foreign types. But if you ever get here there
are not much harm done as it would be read-only.

The extras 5 lines of code are just not worth it.


>

> >          rcu_unlock_domain(od);

> >          break;

> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c

> > index e0b84a9db5..dea04ef66f 100644

> > --- a/xen/arch/arm/p2m.c

> > +++ b/xen/arch/arm/p2m.c

> > @@ -477,6 +477,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t

> t, p2m_access_t a)

> >          break;

> >

> >      case p2m_iommu_map_ro:

> > +    case p2m_map_foreign_ro:

> >      case p2m_grant_map_ro:

> >      case p2m_invalid:

> >          e->p2m.xn = 1;

> > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h

> > index a1aef7b793..6f2728e2bb 100644

> > --- a/xen/include/asm-arm/p2m.h

> > +++ b/xen/include/asm-arm/p2m.h

> > @@ -116,6 +116,7 @@ typedef enum {

> >      p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area

> non-cacheable */

> >      p2m_mmio_direct_c,  /* Read/write mapping of genuine MMIO area

> cacheable */

> >      p2m_map_foreign_rw, /* Read/write RAM pages from foreign domain */

> > +    p2m_map_foreign_ro, /* Read-only RAM pages from foreign domain */

> >      p2m_grant_map_rw,   /* Read/write grant mapping */

> >      p2m_grant_map_ro,   /* Read-only grant mapping */

> >      /* The types below are only used to decide the page attribute in

> the P2M */

> > @@ -135,12 +136,16 @@ typedef enum {

> >  #define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw) |  \

> >                           p2m_to_mask(p2m_grant_map_ro))

> >

> > +/* Foreign mappings types */

> > +#define P2M_FOREIGN_TYPES (p2m_to_mask(p2m_map_foreign_rw) | \

> > +                           p2m_to_mask(p2m_map_foreign_ro))

> > +

> >  /* Useful predicates */

> >  #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES)

> > -#define p2m_is_foreign(_t) (p2m_to_mask(_t) &

> p2m_to_mask(p2m_map_foreign_rw))

> > +#define p2m_is_foreign(_t) (p2m_to_mask(_t) & P2M_FOREIGN_TYPES)

> >  #define p2m_is_any_ram(_t) (p2m_to_mask(_t) &                   \

> >                              (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \

> > -                             p2m_to_mask(p2m_map_foreign_rw)))

> > +                             P2M_FOREIGN_TYPES))

> >

> >  /* All common type definitions should live ahead of this inclusion. */

> >  #ifdef _XEN_P2M_COMMON_H

> > @@ -295,7 +300,38 @@ struct page_info *p2m_get_page_from_gfn(struct

> domain *d, gfn_t gfn,

> >  static inline struct page_info *get_page_from_gfn(

> >      struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)

> >  {

> > -    return p2m_get_page_from_gfn(d, _gfn(gfn), t);

> > +    mfn_t mfn;

> > +    p2m_type_t _t;

> > +    struct page_info *page;

> > +

> > +    /*

> > +     * Special case for DOMID_XEN as it is the only domain so far that

> is

> > +     * not auto-translated.

> > +     */

> > +    if ( unlikely(d != dom_xen) )

>

> Why unlikely?



I got the wrong way around. It should have been likely.


>

> > +        return p2m_get_page_from_gfn(d, _gfn(gfn), t);

> > +

> > +    if ( !t )

> > +        t = &_t;

> > +

> > +    *t = p2m_invalid;

> > +

> > +    /*

> > +     * DOMID_XEN see 1-1 RAM. The p2m_type is based on the type of the

>                     ^sees

>

> > +     * page.

> > +     */

> > +    mfn = _mfn(gfn);

> > +    page = mfn_to_page(mfn);

> > +

> > +    if ( !mfn_valid(mfn) || !get_page(page, d) )

> > +        return NULL;

> > +

> > +    if ( page->u.inuse.type_info & PGT_writable_page )

> > +        *t = p2m_ram_rw;

> > +    else

> > +        *t = p2m_ram_ro;

> > +

> > +    return page;

> >  }

> >

> >  int get_page_type(struct page_info *page, unsigned long type);

> > --

> > 2.11.0

> >

>

> _______________________________________________

> Xen-devel mailing list

> Xen-devel@lists.xenproject.org

> https://lists.xenproject.org/mailman/listinfo/xen-devel
<div><div dir="auto">(Sorry for the formatting)</div></div><div><br><div class="gmail_quote"><div dir="ltr">On Thu, 20 Dec 2018 at 23:08, Stefano Stabellini &lt;<a href="mailto:sstabellini@kernel.org">sstabellini@kernel.org</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Thu, 20 Dec 2018, Julien Grall wrote:<br>
&gt; Current, foreign mappings can only be read-write. A follow-up patch will<br>
&gt; extend foreign mapping for Xen backend memory (via XEN_DOMID), some of<br>
&gt; that memory should only be read accessible for the mapping domain.<br>
&gt; <br>
&gt; Introduce a new p2m_type to cater read-only foreign mappings. For now,<br>
&gt; the decision between the two foreign mapping type is based on the type<br>
&gt; of the guest page mapped.<br>
&gt; <br>
&gt; Signed-off-by: Julien Grall &lt;<a href="mailto:julien.grall@arm.com" target="_blank">julien.grall@arm.com</a>&gt;<br>
&gt; Reviewed-by: Andrii Anisov &lt;<a href="mailto:andrii_anisov@epam.com" target="_blank">andrii_anisov@epam.com</a>&gt;<br>
&gt; <br>
&gt; ---<br>
&gt; <br>
&gt;     Changes in v2:<br>
&gt;         - Add Andrii&#39;s reviewed-by<br>
&gt; ---<br>
&gt;  xen/arch/arm/mm.c         |  2 +-<br>
&gt;  xen/arch/arm/p2m.c        |  1 +<br>
&gt;  xen/include/asm-arm/p2m.h | 42 +++++++++++++++++++++++++++++++++++++++---<br>
&gt;  3 files changed, 41 insertions(+), 4 deletions(-)<br>
&gt; <br>
&gt; diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c<br>
&gt; index 7193d83b44..58f7e54640 100644<br>
&gt; --- a/xen/arch/arm/mm.c<br>
&gt; +++ b/xen/arch/arm/mm.c<br>
&gt; @@ -1267,7 +1267,7 @@ int xenmem_add_to_physmap_one(<br>
&gt;          }<br>
&gt;  <br>
&gt;          mfn = page_to_mfn(page);<br>
&gt; -        t = p2m_map_foreign_rw;<br>
&gt; +        t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;<br>
<br>
I know there is a p2m_is_ram check close by, but I think it would still<br>
be better to do:<br>
<br>
  if (p2mt == p2m_ram_rw)<br>
    t = p2m_map_foreign_rw;<br>
  else if (p2mt == p2m_ram_ro)<br>
    t = p2m_map_foreign_ro;<br>
  else<br>
    error<br>
<br>
to avoid cases where p2mt is something completely different<br>
(p2m_mmio_direct_dev for instance) and t gets set to p2m_map_foreign_ro<br>
by mistake.</blockquote><div dir="auto"><br></div><div dir="auto">The case you suggest is impossible. You can only be here if you manage to get a reference on the page (e.g p2m_foreign or p2m_ram).</div><div dir="auto">The check above remove the foreign types. But if you ever get here there are not much harm done as it would be read-only.</div><div dir="auto"><br></div><div dir="auto">The extras 5 lines of code are just not worth it.</div><div dir="auto"><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
&gt;          rcu_unlock_domain(od);<br>
&gt;          break;<br>
&gt; diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c<br>
&gt; index e0b84a9db5..dea04ef66f 100644<br>
&gt; --- a/xen/arch/arm/p2m.c<br>
&gt; +++ b/xen/arch/arm/p2m.c<br>
&gt; @@ -477,6 +477,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)<br>
&gt;          break;<br>
&gt;  <br>
&gt;      case p2m_iommu_map_ro:<br>
&gt; +    case p2m_map_foreign_ro:<br>
&gt;      case p2m_grant_map_ro:<br>
&gt;      case p2m_invalid:<br>
&gt;          e-&gt;p2m.xn = 1;<br>
&gt; diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h<br>
&gt; index a1aef7b793..6f2728e2bb 100644<br>
&gt; --- a/xen/include/asm-arm/p2m.h<br>
&gt; +++ b/xen/include/asm-arm/p2m.h<br>
&gt; @@ -116,6 +116,7 @@ typedef enum {<br>
&gt;      p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area non-cacheable */<br>
&gt;      p2m_mmio_direct_c,  /* Read/write mapping of genuine MMIO area cacheable */<br>
&gt;      p2m_map_foreign_rw, /* Read/write RAM pages from foreign domain */<br>
&gt; +    p2m_map_foreign_ro, /* Read-only RAM pages from foreign domain */<br>
&gt;      p2m_grant_map_rw,   /* Read/write grant mapping */<br>
&gt;      p2m_grant_map_ro,   /* Read-only grant mapping */<br>
&gt;      /* The types below are only used to decide the page attribute in the P2M */<br>
&gt; @@ -135,12 +136,16 @@ typedef enum {<br>
&gt;  #define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw) |  \<br>
&gt;                           p2m_to_mask(p2m_grant_map_ro))<br>
&gt;  <br>
&gt; +/* Foreign mappings types */<br>
&gt; +#define P2M_FOREIGN_TYPES (p2m_to_mask(p2m_map_foreign_rw) | \<br>
&gt; +                           p2m_to_mask(p2m_map_foreign_ro))<br>
&gt; +<br>
&gt;  /* Useful predicates */<br>
&gt;  #define p2m_is_ram(_t) (p2m_to_mask(_t) &amp; P2M_RAM_TYPES)<br>
&gt; -#define p2m_is_foreign(_t) (p2m_to_mask(_t) &amp; p2m_to_mask(p2m_map_foreign_rw))<br>
&gt; +#define p2m_is_foreign(_t) (p2m_to_mask(_t) &amp; P2M_FOREIGN_TYPES)<br>
&gt;  #define p2m_is_any_ram(_t) (p2m_to_mask(_t) &amp;                   \<br>
&gt;                              (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \<br>
&gt; -                             p2m_to_mask(p2m_map_foreign_rw)))<br>
&gt; +                             P2M_FOREIGN_TYPES))<br>
&gt;  <br>
&gt;  /* All common type definitions should live ahead of this inclusion. */<br>
&gt;  #ifdef _XEN_P2M_COMMON_H<br>
&gt; @@ -295,7 +300,38 @@ struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn,<br>
&gt;  static inline struct page_info *get_page_from_gfn(<br>
&gt;      struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)<br>
&gt;  {<br>
&gt; -    return p2m_get_page_from_gfn(d, _gfn(gfn), t);<br>
&gt; +    mfn_t mfn;<br>
&gt; +    p2m_type_t _t;<br>
&gt; +    struct page_info *page;<br>
&gt; +<br>
&gt; +    /*<br>
&gt; +     * Special case for DOMID_XEN as it is the only domain so far that is<br>
&gt; +     * not auto-translated.<br>
&gt; +     */<br>
&gt; +    if ( unlikely(d != dom_xen) )<br>
<br>
Why unlikely?</blockquote><div dir="auto"><br></div><div dir="auto">I got the wrong way around. It should have been likely.</div><div dir="auto"><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
&gt; +        return p2m_get_page_from_gfn(d, _gfn(gfn), t);<br>
&gt; +<br>
&gt; +    if ( !t )<br>
&gt; +        t = &amp;_t;<br>
&gt; +<br>
&gt; +    *t = p2m_invalid;<br>
&gt; +<br>
&gt; +    /*<br>
&gt; +     * DOMID_XEN see 1-1 RAM. The p2m_type is based on the type of the<br>
                    ^sees<br>
<br>
&gt; +     * page.<br>
&gt; +     */<br>
&gt; +    mfn = _mfn(gfn);<br>
&gt; +    page = mfn_to_page(mfn);<br>
&gt; +<br>
&gt; +    if ( !mfn_valid(mfn) || !get_page(page, d) )<br>
&gt; +        return NULL;<br>
&gt; +<br>
&gt; +    if ( page-&gt;u.inuse.type_info &amp; PGT_writable_page )<br>
&gt; +        *t = p2m_ram_rw;<br>
&gt; +    else<br>
&gt; +        *t = p2m_ram_ro;<br>
&gt; +<br>
&gt; +    return page;<br>
&gt;  }<br>
&gt;  <br>
&gt;  int get_page_type(struct page_info *page, unsigned long type);<br>
&gt; -- <br>
&gt; 2.11.0<br>
&gt; <br>
<br>
_______________________________________________<br>
Xen-devel mailing list<br>
<a href="mailto:Xen-devel@lists.xenproject.org" target="_blank">Xen-devel@lists.xenproject.org</a><br>
<a href="https://lists.xenproject.org/mailman/listinfo/xen-devel" rel="noreferrer" target="_blank">https://lists.xenproject.org/mailman/listinfo/xen-devel</a></blockquote></div></div>
Stefano Stabellini Dec. 20, 2018, 11:35 p.m. UTC | #3 
On Thu, 20 Dec 2018, Julien Grall wrote:
> (Sorry for the formatting)

> 

> On Thu, 20 Dec 2018 at 23:08, Stefano Stabellini <sstabellini@kernel.org> wrote:

>       On Thu, 20 Dec 2018, Julien Grall wrote:

>       > Current, foreign mappings can only be read-write. A follow-up patch will

>       > extend foreign mapping for Xen backend memory (via XEN_DOMID), some of

>       > that memory should only be read accessible for the mapping domain.

>       >

>       > Introduce a new p2m_type to cater read-only foreign mappings. For now,

>       > the decision between the two foreign mapping type is based on the type

>       > of the guest page mapped.

>       >

>       > Signed-off-by: Julien Grall <julien.grall@arm.com>

>       > Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

>       >

>       > ---

>       >

>       >     Changes in v2:

>       >         - Add Andrii's reviewed-by

>       > ---

>       >  xen/arch/arm/mm.c         |  2 +-

>       >  xen/arch/arm/p2m.c        |  1 +

>       >  xen/include/asm-arm/p2m.h | 42 +++++++++++++++++++++++++++++++++++++++---

>       >  3 files changed, 41 insertions(+), 4 deletions(-)

>       >

>       > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c

>       > index 7193d83b44..58f7e54640 100644

>       > --- a/xen/arch/arm/mm.c

>       > +++ b/xen/arch/arm/mm.c

>       > @@ -1267,7 +1267,7 @@ int xenmem_add_to_physmap_one(

>       >          }

>       > 

>       >          mfn = page_to_mfn(page);

>       > -        t = p2m_map_foreign_rw;

>       > +        t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;

> 

>       I know there is a p2m_is_ram check close by, but I think it would still

>       be better to do:

> 

>         if (p2mt == p2m_ram_rw)

>           t = p2m_map_foreign_rw;

>         else if (p2mt == p2m_ram_ro)

>           t = p2m_map_foreign_ro;

>         else

>           error

> 

>       to avoid cases where p2mt is something completely different

>       (p2m_mmio_direct_dev for instance) and t gets set to p2m_map_foreign_ro

>       by mistake.

> 

> 

> The case you suggest is impossible. You can only be here if you manage to get a reference on the page (e.g p2m_foreign or

> p2m_ram).

> The check above remove the foreign types. But if you ever get here there are not much harm done as it would be read-only.

> 

> The extras 5 lines of code are just not worth it.


I realize the case is impossible today, it was for clarity and for
future proof-ness. You can reduce line code count by combining it with
the p2m_is_ram check above:

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 49d7a76..01ae2cc 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1259,7 +1259,9 @@ int xenmem_add_to_physmap_one(
             return -EINVAL;
         }
 
-        if ( !p2m_is_ram(p2mt) )
+        if ( p2m_is_ram(p2mt) )
+            t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;
+        else
         {
             put_page(page);
             put_pg_owner(od);
@@ -1267,7 +1269,6 @@ int xenmem_add_to_physmap_one(
         }
 
         mfn = page_to_mfn(page);
-        t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;
 
         put_pg_owner(od);
         break;
Julien Grall Dec. 21, 2018, 10:45 a.m. UTC | #4 
Hi,

On 20/12/2018 23:35, Stefano Stabellini wrote:
> On Thu, 20 Dec 2018, Julien Grall wrote:
>> (Sorry for the formatting)
>>
>> On Thu, 20 Dec 2018 at 23:08, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>        On Thu, 20 Dec 2018, Julien Grall wrote:
>>        > Current, foreign mappings can only be read-write. A follow-up patch will
>>        > extend foreign mapping for Xen backend memory (via XEN_DOMID), some of
>>        > that memory should only be read accessible for the mapping domain.
>>        >
>>        > Introduce a new p2m_type to cater read-only foreign mappings. For now,
>>        > the decision between the two foreign mapping type is based on the type
>>        > of the guest page mapped.
>>        >
>>        > Signed-off-by: Julien Grall <julien.grall@arm.com>
>>        > Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>>        >
>>        > ---
>>        >
>>        >     Changes in v2:
>>        >         - Add Andrii's reviewed-by
>>        > ---
>>        >  xen/arch/arm/mm.c         |  2 +-
>>        >  xen/arch/arm/p2m.c        |  1 +
>>        >  xen/include/asm-arm/p2m.h | 42 +++++++++++++++++++++++++++++++++++++++---
>>        >  3 files changed, 41 insertions(+), 4 deletions(-)
>>        >
>>        > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>        > index 7193d83b44..58f7e54640 100644
>>        > --- a/xen/arch/arm/mm.c
>>        > +++ b/xen/arch/arm/mm.c
>>        > @@ -1267,7 +1267,7 @@ int xenmem_add_to_physmap_one(
>>        >          }
>>        >
>>        >          mfn = page_to_mfn(page);
>>        > -        t = p2m_map_foreign_rw;
>>        > +        t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;
>>
>>        I know there is a p2m_is_ram check close by, but I think it would still
>>        be better to do:
>>
>>          if (p2mt == p2m_ram_rw)
>>            t = p2m_map_foreign_rw;
>>          else if (p2mt == p2m_ram_ro)
>>            t = p2m_map_foreign_ro;
>>          else
>>            error
>>
>>        to avoid cases where p2mt is something completely different
>>        (p2m_mmio_direct_dev for instance) and t gets set to p2m_map_foreign_ro
>>        by mistake.
>>
>>
>> The case you suggest is impossible. You can only be here if you manage to get a reference on the page (e.g p2m_foreign or
>> p2m_ram).
>> The check above remove the foreign types. But if you ever get here there are not much harm done as it would be read-only.
>>
>> The extras 5 lines of code are just not worth it.
> 
> I realize the case is impossible today, it was for clarity and for
> future proof-ness. You can reduce line code count by combining it with
> the p2m_is_ram check above:
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 49d7a76..01ae2cc 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1259,7 +1259,9 @@ int xenmem_add_to_physmap_one(
>               return -EINVAL;
>           }
>   
> -        if ( !p2m_is_ram(p2mt) )
> +        if ( p2m_is_ram(p2mt) )
> +            t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;
> +        else
>           {
>               put_page(page);
>               put_pg_owner(od);
> @@ -1267,7 +1269,6 @@ int xenmem_add_to_physmap_one(
>           }
>   
>           mfn = page_to_mfn(page);
> -        t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;
>   
>           put_pg_owner(od);
>           break;
> 

That's a better solution. I will update the patch.

Cheers,
Julien Grall Dec. 21, 2018, 10:55 a.m. UTC | #5 
Hi,

On 20/12/2018 19:23, Julien Grall wrote:
> Current, foreign mappings can only be read-write. A follow-up patch will
> extend foreign mapping for Xen backend memory (via XEN_DOMID), some of
> that memory should only be read accessible for the mapping domain.
> 
> Introduce a new p2m_type to cater read-only foreign mappings. For now,
> the decision between the two foreign mapping type is based on the type
> of the guest page mapped.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> 
> ---
> 
>      Changes in v2:
>          - Add Andrii's reviewed-by
> ---
>   xen/arch/arm/mm.c         |  2 +-
>   xen/arch/arm/p2m.c        |  1 +
>   xen/include/asm-arm/p2m.h | 42 +++++++++++++++++++++++++++++++++++++++---
>   3 files changed, 41 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 7193d83b44..58f7e54640 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1267,7 +1267,7 @@ int xenmem_add_to_physmap_one(
>           }
>   
>           mfn = page_to_mfn(page);
> -        t = p2m_map_foreign_rw;
> +        t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;
>   
>           rcu_unlock_domain(od);
>           break;
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index e0b84a9db5..dea04ef66f 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -477,6 +477,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
>           break;
>   
>       case p2m_iommu_map_ro:
> +    case p2m_map_foreign_ro:
>       case p2m_grant_map_ro:
>       case p2m_invalid:
>           e->p2m.xn = 1;
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index a1aef7b793..6f2728e2bb 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -116,6 +116,7 @@ typedef enum {
>       p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area non-cacheable */
>       p2m_mmio_direct_c,  /* Read/write mapping of genuine MMIO area cacheable */
>       p2m_map_foreign_rw, /* Read/write RAM pages from foreign domain */
> +    p2m_map_foreign_ro, /* Read-only RAM pages from foreign domain */
>       p2m_grant_map_rw,   /* Read/write grant mapping */
>       p2m_grant_map_ro,   /* Read-only grant mapping */
>       /* The types below are only used to decide the page attribute in the P2M */
> @@ -135,12 +136,16 @@ typedef enum {
>   #define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw) |  \
>                            p2m_to_mask(p2m_grant_map_ro))
>   
> +/* Foreign mappings types */
> +#define P2M_FOREIGN_TYPES (p2m_to_mask(p2m_map_foreign_rw) | \
> +                           p2m_to_mask(p2m_map_foreign_ro))
> +
>   /* Useful predicates */
>   #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES)
> -#define p2m_is_foreign(_t) (p2m_to_mask(_t) & p2m_to_mask(p2m_map_foreign_rw))
> +#define p2m_is_foreign(_t) (p2m_to_mask(_t) & P2M_FOREIGN_TYPES)
>   #define p2m_is_any_ram(_t) (p2m_to_mask(_t) &                   \
>                               (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \
> -                             p2m_to_mask(p2m_map_foreign_rw)))
> +                             P2M_FOREIGN_TYPES))
>   
>   /* All common type definitions should live ahead of this inclusion. */
>   #ifdef _XEN_P2M_COMMON_H
> @@ -295,7 +300,38 @@ struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn,
>   static inline struct page_info *get_page_from_gfn(
>       struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
>   {

Something has gone wrong with this patch. The chunk below should be in a 
separate patch. I will split this patch in two.

> -    return p2m_get_page_from_gfn(d, _gfn(gfn), t);
> +    mfn_t mfn;
> +    p2m_type_t _t;
> +    struct page_info *page;
> +
> +    /*
> +     * Special case for DOMID_XEN as it is the only domain so far that is
> +     * not auto-translated.
> +     */
> +    if ( unlikely(d != dom_xen) )
> +        return p2m_get_page_from_gfn(d, _gfn(gfn), t);
> +
> +    if ( !t )
> +        t = &_t;
> +
> +    *t = p2m_invalid;
> +
> +    /*
> +     * DOMID_XEN see 1-1 RAM. The p2m_type is based on the type of the
> +     * page.
> +     */
> +    mfn = _mfn(gfn);
> +    page = mfn_to_page(mfn);
> +
> +    if ( !mfn_valid(mfn) || !get_page(page, d) )
> +        return NULL;
> +
> +    if ( page->u.inuse.type_info & PGT_writable_page )
> +        *t = p2m_ram_rw;
> +    else
> +        *t = p2m_ram_ro;
> +
> +    return page;
>   }
>   
>   int get_page_type(struct page_info *page, unsigned long type);
> 

Cheers,
diff mbox series

Patch

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 7193d83b44..58f7e54640 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1267,7 +1267,7 @@  int xenmem_add_to_physmap_one(
         }
 
         mfn = page_to_mfn(page);
-        t = p2m_map_foreign_rw;
+        t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;
 
         rcu_unlock_domain(od);
         break;
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index e0b84a9db5..dea04ef66f 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -477,6 +477,7 @@  static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
         break;
 
     case p2m_iommu_map_ro:
+    case p2m_map_foreign_ro:
     case p2m_grant_map_ro:
     case p2m_invalid:
         e->p2m.xn = 1;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index a1aef7b793..6f2728e2bb 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -116,6 +116,7 @@  typedef enum {
     p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area non-cacheable */
     p2m_mmio_direct_c,  /* Read/write mapping of genuine MMIO area cacheable */
     p2m_map_foreign_rw, /* Read/write RAM pages from foreign domain */
+    p2m_map_foreign_ro, /* Read-only RAM pages from foreign domain */
     p2m_grant_map_rw,   /* Read/write grant mapping */
     p2m_grant_map_ro,   /* Read-only grant mapping */
     /* The types below are only used to decide the page attribute in the P2M */
@@ -135,12 +136,16 @@  typedef enum {
 #define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw) |  \
                          p2m_to_mask(p2m_grant_map_ro))
 
+/* Foreign mappings types */
+#define P2M_FOREIGN_TYPES (p2m_to_mask(p2m_map_foreign_rw) | \
+                           p2m_to_mask(p2m_map_foreign_ro))
+
 /* Useful predicates */
 #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES)
-#define p2m_is_foreign(_t) (p2m_to_mask(_t) & p2m_to_mask(p2m_map_foreign_rw))
+#define p2m_is_foreign(_t) (p2m_to_mask(_t) & P2M_FOREIGN_TYPES)
 #define p2m_is_any_ram(_t) (p2m_to_mask(_t) &                   \
                             (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \
-                             p2m_to_mask(p2m_map_foreign_rw)))
+                             P2M_FOREIGN_TYPES))
 
 /* All common type definitions should live ahead of this inclusion. */
 #ifdef _XEN_P2M_COMMON_H
@@ -295,7 +300,38 @@  struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn,
 static inline struct page_info *get_page_from_gfn(
     struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
 {
-    return p2m_get_page_from_gfn(d, _gfn(gfn), t);
+    mfn_t mfn;
+    p2m_type_t _t;
+    struct page_info *page;
+
+    /*
+     * Special case for DOMID_XEN as it is the only domain so far that is
+     * not auto-translated.
+     */
+    if ( unlikely(d != dom_xen) )
+        return p2m_get_page_from_gfn(d, _gfn(gfn), t);
+
+    if ( !t )
+        t = &_t;
+
+    *t = p2m_invalid;
+
+    /*
+     * DOMID_XEN see 1-1 RAM. The p2m_type is based on the type of the
+     * page.
+     */
+    mfn = _mfn(gfn);
+    page = mfn_to_page(mfn);
+
+    if ( !mfn_valid(mfn) || !get_page(page, d) )
+        return NULL;
+
+    if ( page->u.inuse.type_info & PGT_writable_page )
+        *t = p2m_ram_rw;
+    else
+        *t = p2m_ram_ro;
+
+    return page;
 }
 
 int get_page_type(struct page_info *page, unsigned long type);