Re: [Xen-devel] [PATCH] xen/arm: p2m: Don't use default access permission when shattering a superpage

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 29 Jul 2016, Julien Grall wrote:
> The following message flood the console when memaccess is enabled on
> various platforms:
> 
> traps.c:2510:d1v0 HSR=0x9383004f pc=0xffff000008b7d4c4 gva=0xffff000008eeb8e0 
> gpa=0x0000004903f8e0
> 
> This is because a data abort from a guest was received due to a
> permission fault but memaccess thought there are no permission fault.
> 
> On ARM, memaccess permissions are stored in a radix tree because there
> are not enough available bits in the p2m entry to store the access
> restriction. When memaccess is restricting the access (i.e any other
> access than p2m_access_rwx), the access will be added in the radix tree
> using the GFN as a key. This will be done for all 4KB pages.
> 
> This means that memaccess has to shatter all the superpages in a given
> region to set the permission on a 4KB granularity. Currently, when a
> superpage is shattered, the new entries are using the value
> p2m->default_access which will restrict permission (because memaccess
> has been enabled). However the radix tree does not yet contain
> an entry for this GFN.
> 
> If a guest VCPU is running at the same time and trying to access the
> modified region, it will result to a stage-2 permission fault. As
> the radix tree does not yet contain an entry for the GFN, memaccess will
> deduce that the fault was not valid and a data abort will be injecting
> to the guest (and crash it).
> 
> Furthermore, the permission may be restricted outside of the requested
> region if it is only a subset of a 1GB/2MB superpage.
> 
> The two issues can be fixed by re-using the permission of the superpage
> entry and override the necessary fields. This is not a problem because
> memaccess cannot work on superpage.
> 
> Lastly, document the code which call mfn_to_p2m_entry when creating a
> the p2m entry for a table to explain that create the p2m entry to page table
> to explain that permission are ignored by the hardware (See D4.3.1 in ARM DDI
> 0487A.j). so the value of the parameter 'access' of mfn_to_p2m_entry does
> not matter.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
>     This patch needs to be backported up to Xen 4.6 (first release
>     which introduced memaccess on ARM). This may require few adjustement
>     because the code has changed a bit.
> 
>     Without it, the guest will randomly crash because the permission has
>     been overriden whilst shattering a superpage and before adding the access
>     permission in the radix tree.
> 
>     Also I am wondering if it would be better to pass p2m_access_rwx
>     to mfn_to_p2m_entry when creating a table entry. This would save
>     a couple of instructions.
> ---
>  xen/arch/arm/p2m.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 40a0b80..d60fbbf 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -434,7 +434,6 @@ static int p2m_create_table(struct p2m_domain *p2m, 
> lpae_t *entry,
>      p = __map_domain_page(page);
>      if ( splitting )
>      {
> -        p2m_type_t t = entry->p2m.type;
>          mfn_t mfn = _mfn(entry->p2m.base);
>          int i;
>  
> @@ -444,15 +443,20 @@ static int p2m_create_table(struct p2m_domain *p2m, 
> lpae_t *entry,
>           */
>           for ( i=0 ; i < LPAE_ENTRIES; i++ )
>           {
> -             pte = mfn_to_p2m_entry(mfn_add(mfn, i << (level_shift - 
> LPAE_SHIFT)),
> -                                    t, p2m->default_access);
> +             /*
> +              * Use the content of the superpage entry and override
> +              * the necessary fields. So the correct permissions are
> +              * kept.
> +              */
> +             pte = *entry;
> +             pte.p2m.base = mfn_x(mfn_add(mfn,
> +                                          i << (level_shift - LPAE_SHIFT)));
>  
>               /*
>                * First and second level super pages set p2m.table = 0, but
>                * third level entries set table = 1.
>                */
> -             if ( level_shift - LPAE_SHIFT )
> -                 pte.p2m.table = 0;
> +             pte.p2m.table = !(level_shift - LPAE_SHIFT);
>  
>               write_pte(&p[i], pte);
>           }
> @@ -467,6 +471,10 @@ static int p2m_create_table(struct p2m_domain *p2m, 
> lpae_t *entry,
>  
>      unmap_domain_page(p);
>  
> +    /*
> +     * The access value does not matter because the hardware will ignore
> +     * the permission fields for table entry.
> +     */
>      pte = mfn_to_p2m_entry(_mfn(page_to_mfn(page)), p2m_invalid,
>                             p2m->default_access);
>  
> -- 
> 1.9.1
> 

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