Re: [PATCH v3 06/19] xen/arm: mm: Avoid flushing the TLBs when mapping are inserted

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 02/04/2022 01:00, Stefano Stabellini wrote:
> On Mon, 21 Feb 2022, Julien Grall wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > Currently, the function xen_pt_update() will flush the TLBs even when
> > the mappings are inserted. This is a bit wasteful because we don't
> > allow mapping replacement. Even if we were, the flush would need to
> > happen earlier because mapping replacement should use Break-Before-Make
> > when updating the entry.
> >
> > A single call to xen_pt_update() can perform a single action. IOW, it
> > is not possible to, for instance, mix inserting and removing mappings.
> > Therefore, we can use `flags` to determine what action is performed.
> >
> > This change will be particularly help to limit the impact of switching
> > boot time mapping to use xen_pt_update().
> >
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > ---
> >      Changes in v2:
> >          - New patch
> > ---
> >   xen/arch/arm/mm.c | 17 ++++++++++++++---
> >   1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index fd16c1541ce2..7b4b9de8693e 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -1104,7 +1104,13 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, 
> > unsigned int level,
> >           /* We should be here with a valid MFN. */
> >           ASSERT(!mfn_eq(mfn, INVALID_MFN));
> >   
> > -        /* We don't allow replacing any valid entry. */
> > +        /*
> > +         * We don't allow replacing any valid entry.
> > +         *
> > +         * Note that the function xen_pt_update() relies on this
> > +         * assumption and will skip the TLB flush. The function will need
> > +         * to be updated if the check is relaxed.
> > +         */
> >           if ( lpae_is_valid(entry) )
> >           {
> >               if ( lpae_is_mapping(entry, level) )
> > @@ -1417,11 +1423,16 @@ static int xen_pt_update(unsigned long virt,
> >       }
> >   
> >       /*
> > -     * Flush the TLBs even in case of failure because we may have
> > +     * The TLBs flush can be safely skipped when a mapping is inserted
> > +     * as we don't allow mapping replacement (see xen_pt_check_entry()).
> > +     *
> > +     * For all the other cases, the TLBs will be flushed unconditionally
> > +     * even if the mapping has failed. This is because we may have
> >        * partially modified the PT. This will prevent any unexpected
> >        * behavior afterwards.
> >        */
> > -    flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
> > +    if ( !(flags & _PAGE_PRESENT) || mfn_eq(mfn, INVALID_MFN) )
> > +        flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
> I am trying to think of a care where the following wouldn't be enough
> but I cannot come up with one:
>
>     if ( mfn_eq(mfn, INVALID_MFN) )
>         flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
_PAGE_PRESENT is not set for two cases: when removing a page or 
populating page-tables for a region. Both of them will expect an 
INVALID_MFN (see the two asserts in xen_pt_check_entry()).
Therefore your solution should work. However, technically the 'mfn' is 
ignored in both situation (hence why this is an ASSERT() rather than a 
prod check).
Also, I feel it is better to flush more than less (missing a flush could 
have catastrophic result). So I chose to be explicit in which case the 
flush can be skipped.
Maybe it would be clearer if I write:

 !((flags & _PAGE_PRESENT) && !mfn_eq(mfn, INVALID_MFN))

Cheers,

--
Julien Grall