Re: [PATCH for-4.14 v4] x86/tlb: fix assisted flush usage

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Jun 30, 2020 at 02:13:36PM +0200, Jan Beulich wrote:
> On 26.06.2020 17:57, Roger Pau Monne wrote:
> > Commit e9aca9470ed86 introduced a regression when avoiding sending
> > IPIs for certain flush operations. Xen page fault handler
> > (spurious_page_fault) relies on blocking interrupts in order to
> > prevent handling TLB flush IPIs and thus preventing other CPUs from
> > removing page tables pages. Switching to assisted flushing avoided such
> > IPIs, and thus can result in pages belonging to the page tables being
> > removed (and possibly re-used) while __page_fault_type is being
> > executed.
> > 
> > Force some of the TLB flushes to use IPIs, thus avoiding the assisted
> > TLB flush. Those selected flushes are the page type change (when
> > switching from a page table type to a different one, ie: a page that
> > has been removed as a page table) and page allocation. This sadly has
> > a negative performance impact on the pvshim, as less assisted flushes
> > can be used. Note the flush in grant-table code is also switched to
> > use an IPI even when not strictly needed. This is done so that a
> > common arch_flush_tlb_mask can be introduced and always used in common
> > code.
> > 
> > Introduce a new flag (FLUSH_FORCE_IPI) and helper to force a TLB flush
> > using an IPI (flush_tlb_mask_sync, x86 only). Note that the flag is
> > only meaningfully defined when the hypervisor supports PV or shadow
> > paging mode, as otherwise hardware assisted paging domains are in
> > charge of their page tables and won't share page tables with Xen, thus
> > not influencing the result of page walks performed by the spurious
> > fault handler.
> > 
> > Just passing this new flag when calling flush_area_mask prevents the
> > usage of the assisted flush without any other side effects.
> > 
> > Note the flag is not defined on Arm.
> > 
> > Fixes: e9aca9470ed86 ('x86/tlb: use Xen L0 assisted TLB flush when available')
> > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> In principle
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> A few cosmetic remarks though:
> 
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2894,7 +2894,17 @@ static int _get_page_type(struct page_info *page, unsigned long type,
> >                        ((nx & PGT_type_mask) == PGT_writable_page)) )
> >                  {
> >                      perfc_incr(need_flush_tlb_flush);
> > -                    flush_tlb_mask(mask);
> > +                    if ( (x & PGT_type_mask) &&
> > +                         (x & PGT_type_mask) <= PGT_root_page_table )
> > +                        /*
> > +                         * If page was a page table make sure the flush is
> > +                         * performed using an IPI in order to avoid changing
> > +                         * the type of a page table page under the feet of
> > +                         * spurious_page_fault.
> > +                         */
> > +                        flush_tlb_mask_sync(mask);
> > +                    else
> > +                        flush_tlb_mask(mask);
> 
> Effectively this now is the only user of the new macro. I'd prefer
> avoiding its introduction (and hence avoiding the questionable
> "_sync" suffix), doing
> 
>     flush_mask(mask, FLUSH_TLB | (... ? FLUSH_FORCE_IPI : 0));

Right, maybe placing the '(x & PGT_type_mask) && (x & PGT_type_mask) <=
PGT_root_page_table' condition inside the parameter list of flush_mask
will make the code hard to read, so it might be worth to keep the
if?

> here and ...
> 
> > @@ -148,9 +158,24 @@ void flush_area_mask(const cpumask_t *, const void *va, unsigned int flags);
> >  /* Flush specified CPUs' TLBs */
> >  #define flush_tlb_mask(mask)                    \
> >      flush_mask(mask, FLUSH_TLB)
> > +/*
> > + * Flush specified CPUs' TLBs and force the usage of an IPI to do so. This is
> > + * required for certain operations that rely on page tables themselves not
> > + * being freed and reused when interrupts are blocked, as the flush IPI won't
> > + * be fulfilled until exiting from that critical region.
> > + */
> > +#define flush_tlb_mask_sync(mask)               \
> > +    flush_mask(mask, FLUSH_TLB | FLUSH_FORCE_IPI)
> >  #define flush_tlb_one_mask(mask,v)              \
> >      flush_area_mask(mask, (const void *)(v), FLUSH_TLB|FLUSH_ORDER(0))
> >  
> > +/*
> > + * Alias the common code TLB flush helper to the sync one in order to be on the
> > + * safe side. Note that not all calls from common code strictly require the
> > + * _sync variant.
> > + */
> > +#define arch_flush_tlb_mask flush_tlb_mask_sync
> 
> ...
> 
> #define arch_flush_tlb_mask(mask)               \
>     flush_mask(mask, FLUSH_TLB | FLUSH_FORCE_IPI)

Sure. Feel free to slightly adjust the comment, I think doing
s/Alias/Force/ would be enough.

> here. I'd be okay making these adjustments while committing, if
> you and others don't object.

That's fine, I leave to your judgment whether to use the ternary
operator in the _get_page_type case.

Roger.