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

[Julien Grall <julien.grall@arm.com>]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel