diff mbox series

[Xen-devel,MM-PART3,v2,12/12] xen/arm: mm: Remove set_pte_flags_on_range()

Message ID 20190514123125.29086-13-julien.grall@arm.com
State Superseded
Headers show
Series xen/arm: Provide a generic function to update Xen PT | expand

Commit Message

Julien Grall May 14, 2019, 12:31 p.m. UTC 
set_pte_flags_on_range() is yet another function that will open-code
update to a specific range in the Xen page-tables. It can be completely
dropped by using either modify_xen_mappings() or destroy_xen_mappings().

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

---
    Changes in v2:
        - Add missing newline in panic
        - Add Andrii's reviewed-by
---
 xen/arch/arm/mm.c | 58 ++++++++++---------------------------------------------
 1 file changed, 10 insertions(+), 48 deletions(-)

Comments

Stefano Stabellini June 12, 2019, 10:41 p.m. UTC | #1 
On Tue, 14 May 2019, Julien Grall wrote:
> set_pte_flags_on_range() is yet another function that will open-code
> update to a specific range in the Xen page-tables. It can be completely
> dropped by using either modify_xen_mappings() or destroy_xen_mappings().
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> 
> ---
>     Changes in v2:
>         - Add missing newline in panic
>         - Add Andrii's reviewed-by
> ---
>  xen/arch/arm/mm.c | 58 ++++++++++---------------------------------------------
>  1 file changed, 10 insertions(+), 48 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 23ca61e8f0..d74101bcd2 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1277,52 +1277,6 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
>      return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
>  }
>  
> -enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
> -static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
> -{
> -    lpae_t pte;
> -    int i;
> -
> -    ASSERT(is_kernel(p) && is_kernel(p + l));
> -
> -    /* Can only guard in page granularity */
> -    ASSERT(!((unsigned long) p & ~PAGE_MASK));
> -    ASSERT(!(l & ~PAGE_MASK));
> -
> -    for ( i = (p - _start) / PAGE_SIZE; 
> -          i < (p + l - _start) / PAGE_SIZE; 
> -          i++ )
> -    {
> -        pte = xen_xenmap[i];
> -        switch ( mg )
> -        {
> -        case mg_clear:
> -            pte.pt.valid = 0;
> -            break;
> -        case mg_ro:
> -            pte.pt.valid = 1;
> -            pte.pt.pxn = 1;
> -            pte.pt.xn = 1;
> -            pte.pt.ro = 1;
> -            break;
> -        case mg_rw:
> -            pte.pt.valid = 1;
> -            pte.pt.pxn = 1;

It shouldn't make any difference, but FYI we don't set pxn in
xen_pt_update.


> -            pte.pt.xn = 1;
> -            pte.pt.ro = 0;
> -            break;
> -        case mg_rx:
> -            pte.pt.valid = 1;
> -            pte.pt.pxn = 0;
> -            pte.pt.xn = 0;
> -            pte.pt.ro = 1;
> -            break;
> -        }
> -        write_pte(xen_xenmap + i, pte);
> -    }
> -    flush_xen_tlb_local();
> -}
> -
>  /* Release all __init and __initdata ranges to be reused */
>  void free_init_memory(void)
>  {
> @@ -1331,8 +1285,12 @@ void free_init_memory(void)
>      uint32_t insn;
>      unsigned int i, nr = len / sizeof(insn);
>      uint32_t *p;
> +    int rc;
>  
> -    set_pte_flags_on_range(__init_begin, len, mg_rw);
> +    rc = modify_xen_mappings((unsigned long)__init_begin,
> +                             (unsigned long)__init_end, PAGE_HYPERVISOR_RW);
> +    if ( rc )
> +        panic("Unable to map RW the init section (rc = %d)\n", rc);

Like for the previous patch, I wonder if we should replace ASSERTs with
panics: ASSERTs don't cause issues in non-debug builds. We don't really
have an "official policy" about this, but I have been going by the rule
of thumb that ASSERTs are really good to have while we need to be
careful with BUG_ON/panic because they might introduce instability (see
Linux policy not to have any.)


>  
>      /*
>       * From now on, init will not be used for execution anymore,
> @@ -1350,7 +1308,11 @@ void free_init_memory(void)
>      for ( i = 0; i < nr; i++ )
>          *(p + i) = insn;
>  
> -    set_pte_flags_on_range(__init_begin, len, mg_clear);
> +    rc = destroy_xen_mappings((unsigned long)__init_begin,
> +                              (unsigned long)__init_end);
> +    if ( rc )
> +        panic("Unable to remove the init section (rc = %d)\n", rc);
> +
>      init_domheap_pages(pa, pa + len);
>      printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
>  }
Julien Grall June 13, 2019, 8:51 a.m. UTC | #2 
Hi Stefano,

On 6/12/19 11:41 PM, Stefano Stabellini wrote:
> On Tue, 14 May 2019, Julien Grall wrote:
>> set_pte_flags_on_range() is yet another function that will open-code
>> update to a specific range in the Xen page-tables. It can be completely
>> dropped by using either modify_xen_mappings() or destroy_xen_mappings().
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>>
>> ---
>>      Changes in v2:
>>          - Add missing newline in panic
>>          - Add Andrii's reviewed-by
>> ---
>>   xen/arch/arm/mm.c | 58 ++++++++++---------------------------------------------
>>   1 file changed, 10 insertions(+), 48 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 23ca61e8f0..d74101bcd2 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c.
>> @@ -1277,52 +1277,6 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
>>       return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
>>   }
>>   
>> -enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
>> -static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
>> -{
>> -    lpae_t pte;
>> -    int i;
>> -
>> -    ASSERT(is_kernel(p) && is_kernel(p + l));
>> -
>> -    /* Can only guard in page granularity */
>> -    ASSERT(!((unsigned long) p & ~PAGE_MASK));
>> -    ASSERT(!(l & ~PAGE_MASK));
>> -
>> -    for ( i = (p - _start) / PAGE_SIZE;
>> -          i < (p + l - _start) / PAGE_SIZE;
>> -          i++ )
>> -    {
>> -        pte = xen_xenmap[i];
>> -        switch ( mg )
>> -        {
>> -        case mg_clear:
>> -            pte.pt.valid = 0;
>> -            break;
>> -        case mg_ro:
>> -            pte.pt.valid = 1;
>> -            pte.pt.pxn = 1;
>> -            pte.pt.xn = 1;
>> -            pte.pt.ro = 1;
>> -            break;
>> -        case mg_rw:
>> -            pte.pt.valid = 1;
>> -            pte.pt.pxn = 1;
> 
> It shouldn't make any difference, but FYI we don't set pxn in
> xen_pt_update.

Per D5.4.5 in DDI0487D.a, the PXN bit should be RES0 for any stage-1 
that supports only a single VA range.

The hypervisor stage-1 only supports a single VA range (we have only one 
TTBR), so this bit should be RES0. Any other value would be wrong and 
could lead to undefined behavior in the future.

So the current code was wrong. I will mention it in the commit message.

> 
> 
>> -            pte.pt.xn = 1;
>> -            pte.pt.ro = 0;
>> -            break;
>> -        case mg_rx:
>> -            pte.pt.valid = 1;
>> -            pte.pt.pxn = 0;
>> -            pte.pt.xn = 0;
>> -            pte.pt.ro = 1;
>> -            break;
>> -        }
>> -        write_pte(xen_xenmap + i, pte);
>> -    }
>> -    flush_xen_tlb_local();
>> -}
>> -
>>   /* Release all __init and __initdata ranges to be reused */
>>   void free_init_memory(void)
>>   {
>> @@ -1331,8 +1285,12 @@ void free_init_memory(void)
>>       uint32_t insn;
>>       unsigned int i, nr = len / sizeof(insn);
>>       uint32_t *p;
>> +    int rc;
>>   
>> -    set_pte_flags_on_range(__init_begin, len, mg_rw);
>> +    rc = modify_xen_mappings((unsigned long)__init_begin,
>> +                             (unsigned long)__init_end, PAGE_HYPERVISOR_RW);
>> +    if ( rc )
>> +        panic("Unable to map RW the init section (rc = %d)\n", rc);
> 
> Like for the previous patch, I wonder if we should replace ASSERTs with
> panics: ASSERTs don't cause issues in non-debug builds. We don't really
> have an "official policy" about this, but I have been going by the rule
> of thumb that ASSERTs are really good to have while we need to be
> careful with BUG_ON/panic because they might introduce instability (see
> Linux policy not to have any.)

We do have a policy docs/misc/xen-error-handling.txt. While I agree that 
we have to be careful with BUG_ON()/panic... you also have to take into 
account from where it is called.

In this case, replacing by an ASSERT here is going to make much worst.
This function is only called once at the end of the boot to remove any 
part of Xen that is not used anymore. If this were to fail, then this 
means that something goes really wrong and this is better to stop here 
rather than continuing with in an unstable state.

Cheers,
Stefano Stabellini June 13, 2019, 6:04 p.m. UTC | #3 
On Thu, 13 Jun 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 6/12/19 11:41 PM, Stefano Stabellini wrote:
> > On Tue, 14 May 2019, Julien Grall wrote:
> > > set_pte_flags_on_range() is yet another function that will open-code
> > > update to a specific range in the Xen page-tables. It can be completely
> > > dropped by using either modify_xen_mappings() or destroy_xen_mappings().
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> > > 
> > > ---
> > >      Changes in v2:
> > >          - Add missing newline in panic
> > >          - Add Andrii's reviewed-by
> > > ---
> > >   xen/arch/arm/mm.c | 58
> > > ++++++++++---------------------------------------------
> > >   1 file changed, 10 insertions(+), 48 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index 23ca61e8f0..d74101bcd2 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c.
> > > @@ -1277,52 +1277,6 @@ int modify_xen_mappings(unsigned long s, unsigned
> > > long e, unsigned int flags)
> > >       return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
> > >   }
> > >   -enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
> > > -static void set_pte_flags_on_range(const char *p, unsigned long l, enum
> > > mg mg)
> > > -{
> > > -    lpae_t pte;
> > > -    int i;
> > > -
> > > -    ASSERT(is_kernel(p) && is_kernel(p + l));
> > > -
> > > -    /* Can only guard in page granularity */
> > > -    ASSERT(!((unsigned long) p & ~PAGE_MASK));
> > > -    ASSERT(!(l & ~PAGE_MASK));
> > > -
> > > -    for ( i = (p - _start) / PAGE_SIZE;
> > > -          i < (p + l - _start) / PAGE_SIZE;
> > > -          i++ )
> > > -    {
> > > -        pte = xen_xenmap[i];
> > > -        switch ( mg )
> > > -        {
> > > -        case mg_clear:
> > > -            pte.pt.valid = 0;
> > > -            break;
> > > -        case mg_ro:
> > > -            pte.pt.valid = 1;
> > > -            pte.pt.pxn = 1;
> > > -            pte.pt.xn = 1;
> > > -            pte.pt.ro = 1;
> > > -            break;
> > > -        case mg_rw:
> > > -            pte.pt.valid = 1;
> > > -            pte.pt.pxn = 1;
> > 
> > It shouldn't make any difference, but FYI we don't set pxn in
> > xen_pt_update.
> 
> Per D5.4.5 in DDI0487D.a, the PXN bit should be RES0 for any stage-1 that
> supports only a single VA range.
> 
> The hypervisor stage-1 only supports a single VA range (we have only one
> TTBR), so this bit should be RES0. Any other value would be wrong and could
> lead to undefined behavior in the future.
> 
> So the current code was wrong. I will mention it in the commit message.
> 
> > 
> > 
> > > -            pte.pt.xn = 1;
> > > -            pte.pt.ro = 0;
> > > -            break;
> > > -        case mg_rx:
> > > -            pte.pt.valid = 1;
> > > -            pte.pt.pxn = 0;
> > > -            pte.pt.xn = 0;
> > > -            pte.pt.ro = 1;
> > > -            break;
> > > -        }
> > > -        write_pte(xen_xenmap + i, pte);
> > > -    }
> > > -    flush_xen_tlb_local();
> > > -}
> > > -
> > >   /* Release all __init and __initdata ranges to be reused */
> > >   void free_init_memory(void)
> > >   {
> > > @@ -1331,8 +1285,12 @@ void free_init_memory(void)
> > >       uint32_t insn;
> > >       unsigned int i, nr = len / sizeof(insn);
> > >       uint32_t *p;
> > > +    int rc;
> > >   -    set_pte_flags_on_range(__init_begin, len, mg_rw);
> > > +    rc = modify_xen_mappings((unsigned long)__init_begin,
> > > +                             (unsigned long)__init_end,
> > > PAGE_HYPERVISOR_RW);
> > > +    if ( rc )
> > > +        panic("Unable to map RW the init section (rc = %d)\n", rc);
> > 
> > Like for the previous patch, I wonder if we should replace ASSERTs with
> > panics: ASSERTs don't cause issues in non-debug builds. We don't really
> > have an "official policy" about this, but I have been going by the rule
> > of thumb that ASSERTs are really good to have while we need to be
> > careful with BUG_ON/panic because they might introduce instability (see
> > Linux policy not to have any.)
> 
> We do have a policy docs/misc/xen-error-handling.txt. While I agree that we
> have to be careful with BUG_ON()/panic... you also have to take into account
> from where it is called.
> 
> In this case, replacing by an ASSERT here is going to make much worst.
> This function is only called once at the end of the boot to remove any part of
> Xen that is not used anymore. If this were to fail, then this means that
> something goes really wrong and this is better to stop here rather than
> continuing with in an unstable state.

Yes, on second thought, a BUG_ON is fine here. The risk is only when the
BUG_ON could somehow be trigger by guest behavior, which is not the case
here.
Julien Grall June 13, 2019, 9:22 p.m. UTC | #4 
On 13/06/2019 19:04, Stefano Stabellini wrote:
> On Thu, 13 Jun 2019, Julien Grall wrote:

> 

> Yes, on second thought, a BUG_ON is fine here. The risk is only when the

> BUG_ON could somehow be trigger by guest behavior, which is not the case

> here.


Agreed. I think we are safe to use BUG_ON(...)/panic in any init 
function. Everywhere else, we should think twice before adding new one.

I will respin the patch as discussed.

Cheers,

> 


-- 
Julien Grall
diff mbox series

Patch

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 23ca61e8f0..d74101bcd2 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1277,52 +1277,6 @@  int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
     return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
 }
 
-enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
-static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
-{
-    lpae_t pte;
-    int i;
-
-    ASSERT(is_kernel(p) && is_kernel(p + l));
-
-    /* Can only guard in page granularity */
-    ASSERT(!((unsigned long) p & ~PAGE_MASK));
-    ASSERT(!(l & ~PAGE_MASK));
-
-    for ( i = (p - _start) / PAGE_SIZE; 
-          i < (p + l - _start) / PAGE_SIZE; 
-          i++ )
-    {
-        pte = xen_xenmap[i];
-        switch ( mg )
-        {
-        case mg_clear:
-            pte.pt.valid = 0;
-            break;
-        case mg_ro:
-            pte.pt.valid = 1;
-            pte.pt.pxn = 1;
-            pte.pt.xn = 1;
-            pte.pt.ro = 1;
-            break;
-        case mg_rw:
-            pte.pt.valid = 1;
-            pte.pt.pxn = 1;
-            pte.pt.xn = 1;
-            pte.pt.ro = 0;
-            break;
-        case mg_rx:
-            pte.pt.valid = 1;
-            pte.pt.pxn = 0;
-            pte.pt.xn = 0;
-            pte.pt.ro = 1;
-            break;
-        }
-        write_pte(xen_xenmap + i, pte);
-    }
-    flush_xen_tlb_local();
-}
-
 /* Release all __init and __initdata ranges to be reused */
 void free_init_memory(void)
 {
@@ -1331,8 +1285,12 @@  void free_init_memory(void)
     uint32_t insn;
     unsigned int i, nr = len / sizeof(insn);
     uint32_t *p;
+    int rc;
 
-    set_pte_flags_on_range(__init_begin, len, mg_rw);
+    rc = modify_xen_mappings((unsigned long)__init_begin,
+                             (unsigned long)__init_end, PAGE_HYPERVISOR_RW);
+    if ( rc )
+        panic("Unable to map RW the init section (rc = %d)\n", rc);
 
     /*
      * From now on, init will not be used for execution anymore,
@@ -1350,7 +1308,11 @@  void free_init_memory(void)
     for ( i = 0; i < nr; i++ )
         *(p + i) = insn;
 
-    set_pte_flags_on_range(__init_begin, len, mg_clear);
+    rc = destroy_xen_mappings((unsigned long)__init_begin,
+                              (unsigned long)__init_end);
+    if ( rc )
+        panic("Unable to remove the init section (rc = %d)\n", rc);
+
     init_domheap_pages(pa, pa + len);
     printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
 }