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(-)
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,
--
2.47.0.338.g60cca15819-googOn 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
© 2016 - 2024 Red Hat, Inc.