Re: [PATCH 11/12] x86/mm: enable AMD translation cache extensions

[Rik van Riel <riel@surriel.com>]

On Fri, 2025-01-03 at 18:49 +0100, Jann Horn wrote:
> On Mon, Dec 30, 2024 at 6:53 PM Rik van Riel <riel@surriel.com> 
> > only those upper-level entries that lead to the target PTE in
> > the page table hierarchy, leaving unrelated upper-level entries
> > intact.
> 
> How does this patch interact with KVM SVM guests?
> In particular, will this patch cause TLB flushes performed by guest
> kernels to behave differently?
> 
That is a good question.

A Linux guest should be fine, since Linux already
flushes the parts of the TLB where page tables are
being freed.

I don't know whether this could potentially break
some non-Linux guests, though.


> [...]
> > diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> > index 454a370494d3..585d0731ca9f 100644
> > --- a/arch/x86/mm/tlb.c
> > +++ b/arch/x86/mm/tlb.c
> > @@ -477,7 +477,7 @@ static void broadcast_tlb_flush(struct
> > flush_tlb_info *info)
> >         if (info->stride_shift > PMD_SHIFT)
> >                 maxnr = 1;
> > 
> > -       if (info->end == TLB_FLUSH_ALL) {
> > +       if (info->end == TLB_FLUSH_ALL || info->freed_tables) {
> >                 invlpgb_flush_single_pcid(kern_pcid(asid));
> >                 /* Do any CPUs supporting INVLPGB need PTI? */
> >                 if (static_cpu_has(X86_FEATURE_PTI))
> > @@ -1110,7 +1110,7 @@ static void flush_tlb_func(void *info)
> >          *
> >          * The only question is whether to do a full or partial
> > flush.
> >          *
> > -        * We do a partial flush if requested and two extra
> > conditions
> > +        * We do a partial flush if requested and three extra
> > conditions
> >          * are met:
> >          *
> >          * 1. f->new_tlb_gen == local_tlb_gen + 1.  We have an
> > invariant that
> > @@ -1137,10 +1137,14 @@ static void flush_tlb_func(void *info)
> >          *    date.  By doing a full flush instead, we can increase
> >          *    local_tlb_gen all the way to mm_tlb_gen and we can
> > probably
> >          *    avoid another flush in the very near future.
> > +        *
> > +        * 3. No page tables were freed. If page tables were freed,
> > a full
> > +        *    flush ensures intermediate translations in the TLB
> > get flushed.
> >          */
> 
> Why is this necessary - do we ever issue TLB flushes that are
> intended
> to zap upper-level entries which are not covered by the specified
> address range?
> 
> When, for example, free_pmd_range() gets rid of a page table, it
> calls
> pmd_free_tlb(), which sets tlb->freed_tables and does
> tlb_flush_pud_range(tlb, address, PAGE_SIZE).
> 

I missed those calls.

It looks like this change is not needed.

Of course, the way pmd_free_tlb() operates, the
partial zaps done in that code will exceed the
(default 33 pages) value of tlb_single_page_flush_ceiling,
and the code in flush_tlb_mm_range() will already do
a full flush by default today.

I'll leave out these unnecessary changes in the next
version.

-- 
All Rights Reversed.