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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxx>
> > > Reviewed-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> > > 
> > > ---
> > >      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.

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