diff mbox

xen/arm: p2m: Don't use default access permission when shattering a superpage

Message ID 1469818399-30309-1-git-send-email-julien.grall@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Julien Grall July 29, 2016, 6:53 p.m. UTC
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@arm.com>

---
    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(-)

Comments

Tamas K Lengyel July 29, 2016, 11:21 p.m. UTC | #1
On Fri, Jul 29, 2016 at 12:53 PM, Julien Grall <julien.grall@arm.com> 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@arm.com>

Thanks for looking into this, it is very strange that this issue has
not surfaced before as mem_access was extensively tested on the
Arndale and the XU during the 4.6 merge. Maybe it just happened that
the superpage shattering path was never hit during the tests. It still
does work fine on my Cubietruck as of the latest staging..

Tamas
Julien Grall Aug. 1, 2016, 1:01 p.m. UTC | #2
On 30/07/16 00:21, Tamas K Lengyel wrote:
> On Fri, Jul 29, 2016 at 12:53 PM, Julien Grall <julien.grall@arm.com> 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@arm.com>
>
> Thanks for looking into this, it is very strange that this issue has
> not surfaced before as mem_access was extensively tested on the
> Arndale and the XU during the 4.6 merge. Maybe it just happened that
> the superpage shattering path was never hit during the tests. It still
> does work fine on my Cubietruck as of the latest staging..

Unless the memory is very fragmented, you will likely use 2MB superpages 
(you can find the number of 1GB, 2MB, 4KB mappings via the Xen console, 
key 'q'):

(XEN) p2m mappings for domain 0 (vmid 1):
(XEN)   1G mappings: 7 (shattered 0)
(XEN)   2M mappings: 196 (shattered 0)
(XEN)   4K mappings: 170

My setup was very basic, DOM0 has the same number of vCPUs as the number 
of pCPUs. A Linux guest [1] is started and then xen-access is called 
(see script [2]).

I have tested with various Linux guest: full distribution (Debian) and 
buildroot initramfs. The platform was Foundation model [3] and Juno-r2, 
although the processors are ARMv8 but I don't expect this to be a reason.

Regards,

[1]  42sh# cat guest.cfg
name="guest"
memory= "256"
kernel= "/root/Image"
extra= "console=hvc0 root=/dev/ram1"
ramdisk= "/root/initramfs"

[2] 42sh# cat init.sh
#! /bin/sh
mount -t proc proc  /proc
mount -t devtmpfs dev /dev

export HOME=/root

/etc/init.d/xencommons start
xl -vvv create /root/guest.cfg
sleep 2

echo "Calling memaccess"
/root/xen/tools/tests/xen-access/xen-access 1 write

exec /bin/bash

[3] http://www.arm.com/products/tools/models/foundation-model.php

>
> Tamas
>
Stefano Stabellini Aug. 4, 2016, 6:24 p.m. UTC | #3
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@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     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
>
diff mbox

Patch

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);