| Message ID | 20241203005921.1119116-2-kevinloughlin@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency | expand |
On 03/12/2024 12:59 am, Kevin Loughlin wrote: > diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h > index d4eb9e1d61b8..c040af2d8eff 100644 > --- a/arch/x86/include/asm/paravirt.h > +++ b/arch/x86/include/asm/paravirt.h > @@ -187,6 +187,13 @@ static __always_inline void wbinvd(void) > PVOP_ALT_VCALL0(cpu.wbinvd, "wbinvd", ALT_NOT_XEN); > } > > +extern noinstr void pv_native_wbnoinvd(void); > + > +static __always_inline void wbnoinvd(void) > +{ > + PVOP_ALT_VCALL0(cpu.wbnoinvd, "wbnoinvd", ALT_NOT_XEN); > +} Given this, ... > diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c > index fec381533555..a66b708d8a1e 100644 > --- a/arch/x86/kernel/paravirt.c > +++ b/arch/x86/kernel/paravirt.c > @@ -149,6 +154,7 @@ struct paravirt_patch_template pv_ops = { > .cpu.write_cr0 = native_write_cr0, > .cpu.write_cr4 = native_write_cr4, > .cpu.wbinvd = pv_native_wbinvd, > + .cpu.wbnoinvd = pv_native_wbnoinvd, > .cpu.read_msr = native_read_msr, > .cpu.write_msr = native_write_msr, > .cpu.read_msr_safe = native_read_msr_safe, this, and ... > diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c > index d6818c6cafda..a5c76a6f8976 100644 > --- a/arch/x86/xen/enlighten_pv.c > +++ b/arch/x86/xen/enlighten_pv.c > @@ -1162,6 +1162,7 @@ static const typeof(pv_ops) xen_cpu_ops __initconst = { > .write_cr4 = xen_write_cr4, > > .wbinvd = pv_native_wbinvd, > + .wbnoinvd = pv_native_wbnoinvd, > > .read_msr = xen_read_msr, > .write_msr = xen_write_msr, this, what is the point having a paravirt hook which is wired to native_wbnoinvd() in all cases? That just seems like overhead for overhead sake. ~Andrew
On Mon, Dec 2, 2024 at 5:28 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 03/12/2024 12:59 am, Kevin Loughlin wrote: > > diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h > > index d4eb9e1d61b8..c040af2d8eff 100644 > > --- a/arch/x86/include/asm/paravirt.h > > +++ b/arch/x86/include/asm/paravirt.h > > @@ -187,6 +187,13 @@ static __always_inline void wbinvd(void) > > PVOP_ALT_VCALL0(cpu.wbinvd, "wbinvd", ALT_NOT_XEN); > > } > > > > +extern noinstr void pv_native_wbnoinvd(void); > > + > > +static __always_inline void wbnoinvd(void) > > +{ > > + PVOP_ALT_VCALL0(cpu.wbnoinvd, "wbnoinvd", ALT_NOT_XEN); > > +} > > Given this, ... > > > diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c > > index fec381533555..a66b708d8a1e 100644 > > --- a/arch/x86/kernel/paravirt.c > > +++ b/arch/x86/kernel/paravirt.c > > @@ -149,6 +154,7 @@ struct paravirt_patch_template pv_ops = { > > .cpu.write_cr0 = native_write_cr0, > > .cpu.write_cr4 = native_write_cr4, > > .cpu.wbinvd = pv_native_wbinvd, > > + .cpu.wbnoinvd = pv_native_wbnoinvd, > > .cpu.read_msr = native_read_msr, > > .cpu.write_msr = native_write_msr, > > .cpu.read_msr_safe = native_read_msr_safe, > > this, and ... > > > diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c > > index d6818c6cafda..a5c76a6f8976 100644 > > --- a/arch/x86/xen/enlighten_pv.c > > +++ b/arch/x86/xen/enlighten_pv.c > > @@ -1162,6 +1162,7 @@ static const typeof(pv_ops) xen_cpu_ops __initconst = { > > .write_cr4 = xen_write_cr4, > > > > .wbinvd = pv_native_wbinvd, > > + .wbnoinvd = pv_native_wbnoinvd, > > > > .read_msr = xen_read_msr, > > .write_msr = xen_write_msr, > > this, what is the point having a paravirt hook which is wired to > native_wbnoinvd() in all cases? > > That just seems like overhead for overhead sake. I'm mirroring what's done for WBINVD here, which was changed to a paravirt hook in 10a099405fdf ("cpuidle, xenpv: Make more PARAVIRT_XXL noinstr clean") in order to avoid calls out to instrumented code as described in the commit message in more detail. I believe a hook is similarly required for WBNOINVD, but please let me know if you disagree. Thanks!
On 12/2/2024 5:44 PM, Kevin Loughlin wrote: > On Mon, Dec 2, 2024 at 5:28 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> >> On 03/12/2024 12:59 am, Kevin Loughlin wrote: >>> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h >>> index d4eb9e1d61b8..c040af2d8eff 100644 >>> --- a/arch/x86/include/asm/paravirt.h >>> +++ b/arch/x86/include/asm/paravirt.h >>> @@ -187,6 +187,13 @@ static __always_inline void wbinvd(void) >>> PVOP_ALT_VCALL0(cpu.wbinvd, "wbinvd", ALT_NOT_XEN); >>> } >>> >>> +extern noinstr void pv_native_wbnoinvd(void); >>> + >>> +static __always_inline void wbnoinvd(void) >>> +{ >>> + PVOP_ALT_VCALL0(cpu.wbnoinvd, "wbnoinvd", ALT_NOT_XEN); >>> +} >> >> Given this, ... >> >>> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c >>> index fec381533555..a66b708d8a1e 100644 >>> --- a/arch/x86/kernel/paravirt.c >>> +++ b/arch/x86/kernel/paravirt.c >>> @@ -149,6 +154,7 @@ struct paravirt_patch_template pv_ops = { >>> .cpu.write_cr0 = native_write_cr0, >>> .cpu.write_cr4 = native_write_cr4, >>> .cpu.wbinvd = pv_native_wbinvd, >>> + .cpu.wbnoinvd = pv_native_wbnoinvd, >>> .cpu.read_msr = native_read_msr, >>> .cpu.write_msr = native_write_msr, >>> .cpu.read_msr_safe = native_read_msr_safe, >> >> this, and ... >> >>> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c >>> index d6818c6cafda..a5c76a6f8976 100644 >>> --- a/arch/x86/xen/enlighten_pv.c >>> +++ b/arch/x86/xen/enlighten_pv.c >>> @@ -1162,6 +1162,7 @@ static const typeof(pv_ops) xen_cpu_ops __initconst = { >>> .write_cr4 = xen_write_cr4, >>> >>> .wbinvd = pv_native_wbinvd, >>> + .wbnoinvd = pv_native_wbnoinvd, >>> >>> .read_msr = xen_read_msr, >>> .write_msr = xen_write_msr, >> >> this, what is the point having a paravirt hook which is wired to >> native_wbnoinvd() in all cases? >> >> That just seems like overhead for overhead sake. > > I'm mirroring what's done for WBINVD here, which was changed to a > paravirt hook in 10a099405fdf ("cpuidle, xenpv: Make more PARAVIRT_XXL > noinstr clean") in order to avoid calls out to instrumented code as > described in the commit message in more detail. I believe a hook is > similarly required for WBNOINVD, but please let me know if you > disagree. Thanks! Then the question is why we need to add WBINVD/WBNOINVD to the paravirt hooks.
On 03.12.24 05:09, Xin Li wrote: > On 12/2/2024 5:44 PM, Kevin Loughlin wrote: >> On Mon, Dec 2, 2024 at 5:28 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>> >>> On 03/12/2024 12:59 am, Kevin Loughlin wrote: >>>> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h >>>> index d4eb9e1d61b8..c040af2d8eff 100644 >>>> --- a/arch/x86/include/asm/paravirt.h >>>> +++ b/arch/x86/include/asm/paravirt.h >>>> @@ -187,6 +187,13 @@ static __always_inline void wbinvd(void) >>>> PVOP_ALT_VCALL0(cpu.wbinvd, "wbinvd", ALT_NOT_XEN); >>>> } >>>> >>>> +extern noinstr void pv_native_wbnoinvd(void); >>>> + >>>> +static __always_inline void wbnoinvd(void) >>>> +{ >>>> + PVOP_ALT_VCALL0(cpu.wbnoinvd, "wbnoinvd", ALT_NOT_XEN); >>>> +} >>> >>> Given this, ... >>> >>>> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c >>>> index fec381533555..a66b708d8a1e 100644 >>>> --- a/arch/x86/kernel/paravirt.c >>>> +++ b/arch/x86/kernel/paravirt.c >>>> @@ -149,6 +154,7 @@ struct paravirt_patch_template pv_ops = { >>>> .cpu.write_cr0 = native_write_cr0, >>>> .cpu.write_cr4 = native_write_cr4, >>>> .cpu.wbinvd = pv_native_wbinvd, >>>> + .cpu.wbnoinvd = pv_native_wbnoinvd, >>>> .cpu.read_msr = native_read_msr, >>>> .cpu.write_msr = native_write_msr, >>>> .cpu.read_msr_safe = native_read_msr_safe, >>> >>> this, and ... >>> >>>> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c >>>> index d6818c6cafda..a5c76a6f8976 100644 >>>> --- a/arch/x86/xen/enlighten_pv.c >>>> +++ b/arch/x86/xen/enlighten_pv.c >>>> @@ -1162,6 +1162,7 @@ static const typeof(pv_ops) xen_cpu_ops __initconst = { >>>> .write_cr4 = xen_write_cr4, >>>> >>>> .wbinvd = pv_native_wbinvd, >>>> + .wbnoinvd = pv_native_wbnoinvd, >>>> >>>> .read_msr = xen_read_msr, >>>> .write_msr = xen_write_msr, >>> >>> this, what is the point having a paravirt hook which is wired to >>> native_wbnoinvd() in all cases? >>> >>> That just seems like overhead for overhead sake. >> >> I'm mirroring what's done for WBINVD here, which was changed to a >> paravirt hook in 10a099405fdf ("cpuidle, xenpv: Make more PARAVIRT_XXL >> noinstr clean") in order to avoid calls out to instrumented code as >> described in the commit message in more detail. I believe a hook is >> similarly required for WBNOINVD, but please let me know if you >> disagree. Thanks! > > Then the question is why we need to add WBINVD/WBNOINVD to the paravirt > hooks. > We don't. The wbinvd hook is a leftover from lguest times. I'll send a patch to remove it. Juergen P.S.: As the paravirt maintainer I would have preferred to be Cc-ed in the initial patch mail.
On 12/2/2024 10:47 PM, Juergen Gross wrote: > P.S.: As the paravirt maintainer I would have preferred to be Cc-ed in the > initial patch mail. Looks that Kevin didn't run './scripts/get_maintainer.pl'? Thanks! Xin
On Tue, Dec 3, 2024 at 12:11 AM Xin Li <xin@zytor.com> wrote: > > On 12/2/2024 10:47 PM, Juergen Gross wrote: > > P.S.: As the paravirt maintainer I would have preferred to be Cc-ed in the > > initial patch mail. > > Looks that Kevin didn't run './scripts/get_maintainer.pl'? Woops, my bad. I somehow ended up with the full maintainer list for patch 2/2 from the script but not this one (1/2). Apologies and thanks for the heads up. I saw Juergen's patch [0] ("x86/paravirt: remove the wbinvd hook") to remove the WBINVD hook, so I'll do the same for WBNOINVD in the next version (meaning I shouldn't need to update xenpv code anymore). [0] https://lore.kernel.org/lkml/20241203071550.26487-1-jgross@suse.com/ Thanks! Kevin
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h index d4eb9e1d61b8..c040af2d8eff 100644 --- a/arch/x86/include/asm/paravirt.h +++ b/arch/x86/include/asm/paravirt.h @@ -187,6 +187,13 @@ static __always_inline void wbinvd(void) PVOP_ALT_VCALL0(cpu.wbinvd, "wbinvd", ALT_NOT_XEN); } +extern noinstr void pv_native_wbnoinvd(void); + +static __always_inline void wbnoinvd(void) +{ + PVOP_ALT_VCALL0(cpu.wbnoinvd, "wbnoinvd", ALT_NOT_XEN); +} + static inline u64 paravirt_read_msr(unsigned msr) { return PVOP_CALL1(u64, cpu.read_msr, msr); diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h index 8d4fbe1be489..9a3f38ad1958 100644 --- a/arch/x86/include/asm/paravirt_types.h +++ b/arch/x86/include/asm/paravirt_types.h @@ -87,6 +87,7 @@ struct pv_cpu_ops { #endif void (*wbinvd)(void); + void (*wbnoinvd)(void); /* cpuid emulation, mostly so that caps bits can be disabled */ void (*cpuid)(unsigned int *eax, unsigned int *ebx, diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h index ca073f40698f..ecf93a243b83 100644 --- a/arch/x86/include/asm/smp.h +++ b/arch/x86/include/asm/smp.h @@ -112,6 +112,7 @@ void native_play_dead(void); void play_dead_common(void); void wbinvd_on_cpu(int cpu); int wbinvd_on_all_cpus(void); +int wbnoinvd_on_all_cpus(void); void smp_kick_mwait_play_dead(void); @@ -160,6 +161,12 @@ static inline int wbinvd_on_all_cpus(void) return 0; } +static inline int wbnoinvd_on_all_cpus(void) +{ + wbnoinvd(); + return 0; +} + static inline struct cpumask *cpu_llc_shared_mask(int cpu) { return (struct cpumask *)cpumask_of(0); diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h index aec6e2d3aa1d..c2d16ddcd79b 100644 --- a/arch/x86/include/asm/special_insns.h +++ b/arch/x86/include/asm/special_insns.h @@ -117,7 +117,12 @@ static inline void wrpkru(u32 pkru) static __always_inline void native_wbinvd(void) { - asm volatile("wbinvd": : :"memory"); + asm volatile("wbinvd" : : : "memory"); +} + +static __always_inline void native_wbnoinvd(void) +{ + asm volatile("wbnoinvd" : : : "memory"); } static inline unsigned long __read_cr4(void) @@ -173,6 +178,11 @@ static __always_inline void wbinvd(void) native_wbinvd(); } +static __always_inline void wbnoinvd(void) +{ + native_wbnoinvd(); +} + #endif /* CONFIG_PARAVIRT_XXL */ static __always_inline void clflush(volatile void *__p) diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c index fec381533555..a66b708d8a1e 100644 --- a/arch/x86/kernel/paravirt.c +++ b/arch/x86/kernel/paravirt.c @@ -121,6 +121,11 @@ noinstr void pv_native_wbinvd(void) native_wbinvd(); } +noinstr void pv_native_wbnoinvd(void) +{ + native_wbnoinvd(); +} + static noinstr void pv_native_safe_halt(void) { native_safe_halt(); @@ -149,6 +154,7 @@ struct paravirt_patch_template pv_ops = { .cpu.write_cr0 = native_write_cr0, .cpu.write_cr4 = native_write_cr4, .cpu.wbinvd = pv_native_wbinvd, + .cpu.wbnoinvd = pv_native_wbnoinvd, .cpu.read_msr = native_read_msr, .cpu.write_msr = native_write_msr, .cpu.read_msr_safe = native_read_msr_safe, diff --git a/arch/x86/lib/cache-smp.c b/arch/x86/lib/cache-smp.c index 7af743bd3b13..7ac5cca53031 100644 --- a/arch/x86/lib/cache-smp.c +++ b/arch/x86/lib/cache-smp.c @@ -20,3 +20,15 @@ int wbinvd_on_all_cpus(void) return 0; } EXPORT_SYMBOL(wbinvd_on_all_cpus); + +static void __wbnoinvd(void *dummy) +{ + wbnoinvd(); +} + +int wbnoinvd_on_all_cpus(void) +{ + on_each_cpu(__wbnoinvd, NULL, 1); + return 0; +} +EXPORT_SYMBOL(wbnoinvd_on_all_cpus); diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c index d6818c6cafda..a5c76a6f8976 100644 --- a/arch/x86/xen/enlighten_pv.c +++ b/arch/x86/xen/enlighten_pv.c @@ -1162,6 +1162,7 @@ static const typeof(pv_ops) xen_cpu_ops __initconst = { .write_cr4 = xen_write_cr4, .wbinvd = pv_native_wbinvd, + .wbnoinvd = pv_native_wbnoinvd, .read_msr = xen_read_msr, .write_msr = xen_write_msr,
In line with WBINVD usage, add WBONINVD helper functions, accounting for kernels built with and without CONFIG_PARAVIRT_XXL. Signed-off-by: Kevin Loughlin <kevinloughlin@google.com> --- arch/x86/include/asm/paravirt.h | 7 +++++++ arch/x86/include/asm/paravirt_types.h | 1 + arch/x86/include/asm/smp.h | 7 +++++++ arch/x86/include/asm/special_insns.h | 12 +++++++++++- arch/x86/kernel/paravirt.c | 6 ++++++ arch/x86/lib/cache-smp.c | 12 ++++++++++++ arch/x86/xen/enlighten_pv.c | 1 + 7 files changed, 45 insertions(+), 1 deletion(-)