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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jun 24, 2020 at 12:10:45PM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 23/06/2020 17:16, Roger Pau Monné wrote:
> > On Tue, Jun 23, 2020 at 04:46:29PM +0100, Julien Grall wrote:
> > > 
> > > 
> > > On 23/06/2020 16:15, Roger Pau Monné wrote:
> > > > On Tue, Jun 23, 2020 at 04:08:06PM +0100, Julien Grall wrote:
> > > > > Hi Roger,
> > > > > 
> > > > > On 23/06/2020 15:50, Roger Pau Monne wrote:
> > > > > > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> > > > > > index 9b62087be1..f360166f00 100644
> > > > > > --- a/xen/include/xen/mm.h
> > > > > > +++ b/xen/include/xen/mm.h
> > > > > > @@ -639,7 +639,8 @@ static inline void accumulate_tlbflush(bool 
> > > > > > *need_tlbflush,
> > > > > >         }
> > > > > >     }
> > > > > > -static inline void filtered_flush_tlb_mask(uint32_t 
> > > > > > tlbflush_timestamp)
> > > > > > +static inline void filtered_flush_tlb_mask(uint32_t 
> > > > > > tlbflush_timestamp,
> > > > > > +                                           bool sync)
> > > > > 
> > > > > I read the commit message and went through the code, but it is still 
> > > > > not
> > > > > clear what "sync" means in a non-architectural way.
> > > > > 
> > > > > As an Arm developper, I would assume this means we don't wait for the 
> > > > > TLB
> > > > > flush to complete. But I am sure this would result to some unexpected
> > > > > behavior.
> > > > 
> > > > No, when we return from filtered_flush_tlb_mask the flush has been
> > > > performed (either with sync or without), but I understand the
> > > > confusion given the parameter name.
> > > > 
> > > > > So can you explain on non-x86 words what this really mean?
> > > > 
> > > > sync (in this context) means to force the usage of an IPI (if built
> > > > with PV or shadow paging support) in order to perform the flush.
> > > 
> > > This is compare to?
> > 
> > Using assisted flushes, like you do on Arm, where you don't send an
> > IPI in order to achieve a TLB flush on a remote pCPU.
> 
> AFAICT, the assisted flushes only happen when running a nested Xen. Is that
> correct?

ATM yes, we don't have support for the newly introduced AMD INVLPGB
instruction yet, which provides such functionality on bare metal.

> > 
> > > > AFAICT on Arm you always avoid an IPI when performing a flush, and
> > > > that's fine because you don't have PV or shadow, and then you don't
> > > > require this.
> > > 
> > > Arm provides instructions to broadcast TLB flush, so by the time one of
> > > instruction is completed there is all the TLB entry associated to the
> > > translation doesn't exist.
> > > 
> > > I don't think using PV or shadow would change anything here in the way we 
> > > do
> > > the flush.
> > 
> > TBH, I'm not sure how this applies to Arm. There's no PV or shadow
> > implementation, so I have no idea whether this would apply or not.
> 
> Yes there is none. However, my point was that if we had to implement
> PV/shadow on Arm then an IPI would definitely be my last choice.

Right, this mostly depends on how you perform page table modifications
and whether you have to handle spurious faults like x86 does.

> > > > 
> > > > I could rename to force_ipi (or require_ipi) if that's better?
> > > 
> > > Hmmm, based on what I wrote above, I don't think this name would be more
> > > suitable. However, regardless the name, it is not clear to me when a 
> > > caller
> > > should use false or true.
> > > 
> > > Have you considered a rwlock to synchronize the two?
> > 
> > Yes, the performance drop is huge when I tried. I could try to refine,
> > but I think there's always going to be a performance drop, as you then
> > require mutual exclusion when modifying the page tables (you take the
> > lock in write mode). Right now modification of the page tables can be
> > done concurrently.
> 
> Fair enough. I will scratch that suggestion then. Thanks for the
> explanation!
> 
> So now getting back to filtered_flush_tlb(). AFAICT, the only two callers
> are in common code. The two are used for allocation purpose, so may I ask
> why they need to use different kind of flush?

Looking at it again, this is wrong. I've just realized that
populate_physmap will, depending on the situation, use the
MEMF_no_tlbflush flag, and so it needs to perform the flush by itself
(and that's why filtered_flush_tlb_mask is used).

I guess you will be fine with removing the sync parameter then, and on
x86 force filtered_flush_tlb_mask to always use physical IPIs in
order to perform the flush?

Thanks, Roger.