Re: [Xen-devel] [PATCH v3 2/5] xentrace: Memory/Page Mapping support for DOMID_XEN on ARM

[Julien Grall <julien.grall@xxxxxxx>]

(CC Stefano's new e-mail address)

Hello Benjamin,

On 04/04/16 19:48, Benjamin Sanda wrote:
>   xen/arch/arm/mm.c  |  3 ++-
>   xen/arch/arm/p2m.c | 35 +++++++++++++++++++++++++++++++----
>   2 files changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 81f9e2e..19d6c2c 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1099,7 +1099,8 @@ int xenmem_add_to_physmap_one(
>       {
>           struct domain *od;
>           p2m_type_t p2mt;
> -        od = rcu_lock_domain_by_any_id(foreign_domid);
> +        od = get_pg_owner(foreign_domid);
> +
Please also replace the call to rcu_unlock_domain by put_pg_owner to 
stay consistent.
>           if ( od == NULL )
>               return -ESRCH;
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index a2a9c4b..a99b670 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -227,11 +227,38 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr, 
> p2m_type_t *t)
>   {
>       paddr_t ret;
>       struct p2m_domain *p2m = &d->arch.p2m;
> +    struct page_info *page;
> +    unsigned long mfn;
>
> -    spin_lock(&p2m->lock);
> -    ret = __p2m_lookup(d, paddr, t);
> -    spin_unlock(&p2m->lock);
> -
> +    /*
> +    * DOMID_XEN is considered a PV guest on x86 (i.e MFN == GFN), but
> +    * on ARM there is no such concept. Thus requests to DOMID_XEN
> +    * on ARM use a MFN address directly and do not need translation
> +    * from PFN.
> +    */
The coding style for the comment should be:

/*
 * FOo
 * Bar
 */

> +    if(DOMID_XEN != d->domain_id)
if ( ... )

> +    {
> +        spin_lock(&p2m->lock);
> +        ret = __p2m_lookup(d, paddr, t);
> +        spin_unlock(&p2m->lock);
> +    }
> +    else
> +    {
> +        /* retrieve the page to determine read/write or read only mapping */
> +        mfn = paddr >> PAGE_SHIFT;
> +        if (mfn_valid(mfn))
> +        {
> +            page = mfn_to_page(mfn);
> +            *t = (page->u.inuse.type_info == PGT_writable_page ?
> +                                p2m_ram_rw : p2m_ram_ro);
Unfortunately, xenmem_add_to_physmap_one will ignore the return type and 
will always map using the type p2m_map_foreign. I would introduce
a new type p2m_map_foreign_ro to allow read-only foreign mapping.

I've looked at the x86 code (p2m_add_foreign) and I haven't been able to 
find how the page will be mapped read-only in the guest P2M. 
get_page_from_gfn will always return p2m_raw_rw for DOMID_XEN as it's a 
non translated domain.
Andrew and Jan, do you know how this is supposed to work when xentrace 
is used in a HVM domain? Does x86 Xen always mapped Read-Write the page?
> +        }
> +        else
> +        {
> +            *t = p2m_invalid;
> +        }
The brackets are not necessary for a single statement.

> +        ret = paddr;
> +    }
> +
>       return ret;
>   }
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel