Re: [Xen-devel] [PATCH v7 1/3] x86/tlb: introduce a flush HVM ASIDs flag

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

On Fri, Mar 20, 2020 at 10:36:49AM +0000, Julien Grall wrote:
> Hi,
> 
> On 20/03/2020 10:24, Roger Pau Monné wrote:
> > On Fri, Mar 20, 2020 at 10:08:33AM +0000, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 20/03/2020 10:00, Roger Pau Monné wrote:
> > > > On Fri, Mar 20, 2020 at 10:42:14AM +0100, Roger Pau Monné wrote:
> > > > > On Fri, Mar 20, 2020 at 09:12:16AM +0000, Julien Grall wrote:
> > > > > > Hi Roger,
> > > > > > 
> > > > > > On 20/03/2020 09:01, Roger Pau Monné wrote:
> > > > > > > On Fri, Mar 20, 2020 at 08:21:19AM +0100, Jan Beulich wrote:
> > > > > > > > On 19.03.2020 20:07, Julien Grall wrote:
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > On 19/03/2020 18:43, Roger Pau Monné wrote:
> > > > > > > > > > On Thu, Mar 19, 2020 at 06:07:44PM +0000, Julien Grall 
> > > > > > > > > > wrote:
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > On 19/03/2020 17:38, Roger Pau Monné wrote:
> > > > > > > > > > > > On Thu, Mar 19, 2020 at 04:21:23PM +0000, Julien Grall 
> > > > > > > > > > > > wrote:
> > > > > > > > > > > >      >> Why can't you keep flush_tlb_mask() here?
> > > > > > > > > > > > 
> > > > > > > > > > > > Because filtered_flush_tlb_mask is used in 
> > > > > > > > > > > > populate_physmap, and
> > > > > > > > > > > > changes to the phymap require an ASID flush on AMD 
> > > > > > > > > > > > hardware.
> > > > > > > > > > > 
> > > > > > > > > > > I am afraid this does not yet explain me why 
> > > > > > > > > > > flush_tlb_mask() could not be
> > > > > > > > > > > updated so it flush the ASID on AMD hardware.
> > > > > > > > > > 
> > > > > > > > > > Current behavior previous to this patch is to flush the 
> > > > > > > > > > ASIDs on
> > > > > > > > > > every TLB flush.
> > > > > > > > > > 
> > > > > > > > > > flush_tlb_mask is too widely used on x86 in places where 
> > > > > > > > > > there's no
> > > > > > > > > > need to flush the ASIDs. This prevents using assisted 
> > > > > > > > > > flushes (by L0)
> > > > > > > > > > when running nested, since those assisted flushes performed 
> > > > > > > > > > by L0
> > > > > > > > > > don't flush the L2 guests TLBs.
> > > > > > > > > > 
> > > > > > > > > > I could keep current behavior and leave flush_tlb_mask also 
> > > > > > > > > > flushing the
> > > > > > > > > > ASIDs, but that seems wrong as the function doesn't have 
> > > > > > > > > > anything in
> > > > > > > > > > it's name that suggests it also flushes the in-guest TLBs 
> > > > > > > > > > for HVM.
> > > > > > > > > 
> > > > > > > > > I agree the name is confusing, I had to look at the 
> > > > > > > > > implementation to understand what it does.
> > > > > > > > > 
> > > > > > > > > How about renaming (or introducing) the function to 
> > > > > > > > > flush_tlb_all_guests_mask() or 
> > > > > > > > > flush_tlb_all_guests_cpumask()) ?
> > > > > > > > 
> > > > > > > > And this would then flush _only_ guest TLBs?
> > > > > > > 
> > > > > > > No, I think from Julien's proposal (if I understood it correctly)
> > > > > > > flush_tlb_all_guests_mask would do what flush_tlb_mask currently 
> > > > > > > does
> > > > > > > previous to this patch (flush Xen's TLBs + HVM ASIDs).
> > > > > > 
> > > > > > It looks like there might be confusion on what "guest TLBs" means. 
> > > > > > In my
> > > > > > view this means any TLBs associated directly or indirectly with the 
> > > > > > guest.
> > > > > > On Arm, this would be nuke:
> > > > > >      - guest virtual address -> guest physical address TLB entry
> > > > > >      - guest physical address -> host physical address TLB entry
> > > > > >      - guest virtual address -> host physical address TLB entry
> > > > > 
> > > > > AFAICT ASID flush on AMD hardware will flush any of the above, while
> > > > > VPID flush on Intel will only flush the first item (guest linear to
> > > > 
> > > > Sorry, doing too many things at the same time. On Intel VPID flushes
> > > > will get rid of guest virtual to guest physical or host physical, but
> > > > not of guest physical to host physical, you need an EPT flush to
> > > > accomplish that.
> > > Are you suggesting that on x86, flush_tlb_mask() would not nuke the guest
> > > physical to host physical entries? If so, how is it meant to be safe?
> > 
> > You issue EPT flushes in that case when an EPT modification is
> > performed.
> 
> I am getting more and more confused with the goal of flush_tlb_mask() in
> common code.
> 
> Looking at the Arm code, the P2M code should already flush appropriatly the
> guest TLBs before giving back a page to the allocator.
> 
> So what are we trying to protect against with the call of flush_tlb_mask()?

So on x86 there are two completely different nested page table
technologies, NPT from AMD and EPT from Intel. EPT doesn't require a
VPID flush when modifying the nested page tables (it requires an EPT
flush), OTOH AMD NPT requires an ASID flush when modifying the tables,
and this seems to be implemented in common code for populate_physmap.

On x86 populate_physmap could also get rid of the flush, since the NPT
code already performs an ASID flush when modifying a nested page table
entry, but that's part of another patch.

Thanks, Roger.

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