Xen project Mailing List

On Thu, 20 Dec 2018 at 23:08, Stefano Stabellini <sstabellini@xxxxxxxxxx> 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@xxxxxxx>
> Reviewed-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
>
> ---
>
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

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