Re: [Xen-devel] [PATCH 3/6] xentrace: P2M lookup suport for ARM platform

[Julien Grall <julien.grall@xxxxxxx>]

Hello Benjamin,

Thank you for the patch.

On 16/03/16 20:51, Benjamin Sanda wrote:
> From: bensanda <ben.sanda@xxxxxxxxxxxxxxx>
>
> Modified p2m_lookup() to provide support for xentrace on the ARM platform. 
> Added check for DOMID_XEN which skips PFN to MFN translation. xentrace sends a 
> MFN dirrectly when requesting DOMID_XEN, so no translation is needed. Also sets 
> page memory type, p2m_type_t, to p2m_ram_rw to provide correct access.
A line in the commit message should not be longer than 72 characters.

> Signed-off-by: Benjamin Sanda <ben.sanda@xxxxxxxxxxxxxxx>
> ---
>   xen/arch/arm/p2m.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index a2a9c4b..2e7da43 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -228,10 +228,21 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr, 
> p2m_type_t *t)
>       paddr_t ret;
>       struct p2m_domain *p2m = &d->arch.p2m;
>
> -    spin_lock(&p2m->lock);
> -    ret = __p2m_lookup(d, paddr, t);
> -    spin_unlock(&p2m->lock);
> -
> +    /* Check for DOMID_XEN: If we are called with DOMID_XEN (from xentrace)
Multi-lines comment in Xen should be:

/*
 * Foo
 * Bar
 */

You can find the coding style in CODING_STYLE.

> +    then paddr is already a MFN and no translation is needed. We only set the
> +    page type as p2m_raw_rw and return the MFN directly */
DOMID_XEN is not specific to xentrace. It's a mechanism to share xenheap 
page to any guest.
xentrace is using directly an MFN because DOMID_XEN is considered as a 
PV guest on x86 (i.e MFN == GFN). And we don't have a such concept on ARM.
> +    if(DOMID_XEN != d->domain_id)

if ( d->domain_id != DOMID_XEN )

> +    {
> +        spin_lock(&p2m->lock);
> +        ret = __p2m_lookup(d, paddr, t);
> +        spin_unlock(&p2m->lock);
> +    }
> +    else
> +    {
> +        *t = p2m_ram_rw;
A DOMID_XEN page could be read only too. For instance, the meta-data of 
the trace buffer is read-only (see t_info), we don't want a domain to be 
able to overwrite them.
However, all the foreign page are mapped read-write. You will need to 
rework the code to map a foreign domain (see XENMAPSPACE_gmfn_foreign) 
to allow read-only foreign page (maybe by adding a new p2m_type_t?).
> +        ret = paddr;
> +    }
> +
>       return ret;
>   }
Regards,

--
Julien Grall

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