When intel_idle_ibrs() is called, it modifies the SPEC_CTRL MSR to
0 in order disable IBRS. However, the new MSR value isn't reflected
in x86_spec_ctrl_current which is at odd with the other code that
keep track of its state in that percpu variable. Fix that by updating
x86_spec_ctrl_current percpu value to always match the content of the
SPEC_CTRL MSR.
Signed-off-by: Waiman Long <[email protected]>
---
drivers/idle/intel_idle.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index aa2d19db2b1d..07fa23707b3c 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -181,13 +181,17 @@ static __cpuidle int intel_idle_ibrs(struct cpuidle_device *dev,
u64 spec_ctrl = spec_ctrl_current();
int ret;
- if (smt_active)
+ if (smt_active) {
+ __this_cpu_write(x86_spec_ctrl_current, 0);
native_wrmsrl(MSR_IA32_SPEC_CTRL, 0);
+ }
ret = __intel_idle(dev, drv, index);
- if (smt_active)
+ if (smt_active) {
native_wrmsrl(MSR_IA32_SPEC_CTRL, spec_ctrl);
+ __this_cpu_write(x86_spec_ctrl_current, spec_ctrl);
+ }
return ret;
}
--
2.31.1
On Wed, Jun 21, 2023 at 08:36:02PM -0400, Waiman Long wrote:
> When intel_idle_ibrs() is called, it modifies the SPEC_CTRL MSR to
> 0 in order disable IBRS. However, the new MSR value isn't reflected
> in x86_spec_ctrl_current which is at odd with the other code that
> keep track of its state in that percpu variable. Fix that by updating
> x86_spec_ctrl_current percpu value to always match the content of the
> SPEC_CTRL MSR.
Is this fixing an actual bug or is there some other reason for doing
this?
>
> Signed-off-by: Waiman Long <[email protected]>
> ---
> drivers/idle/intel_idle.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index aa2d19db2b1d..07fa23707b3c 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -181,13 +181,17 @@ static __cpuidle int intel_idle_ibrs(struct cpuidle_device *dev,
> u64 spec_ctrl = spec_ctrl_current();
> int ret;
>
> - if (smt_active)
> + if (smt_active) {
> + __this_cpu_write(x86_spec_ctrl_current, 0);
> native_wrmsrl(MSR_IA32_SPEC_CTRL, 0);
> + }
>
> ret = __intel_idle(dev, drv, index);
>
> - if (smt_active)
> + if (smt_active) {
> native_wrmsrl(MSR_IA32_SPEC_CTRL, spec_ctrl);
> + __this_cpu_write(x86_spec_ctrl_current, spec_ctrl);
> + }
More candidates for update_spec_ctrl()?
--
Josh
On Wed, Jun 21, 2023 at 10:46:33PM -0700, Josh Poimboeuf wrote:
> On Wed, Jun 21, 2023 at 08:36:02PM -0400, Waiman Long wrote:
> > When intel_idle_ibrs() is called, it modifies the SPEC_CTRL MSR to
> > 0 in order disable IBRS. However, the new MSR value isn't reflected
> > in x86_spec_ctrl_current which is at odd with the other code that
> > keep track of its state in that percpu variable. Fix that by updating
> > x86_spec_ctrl_current percpu value to always match the content of the
> > SPEC_CTRL MSR.
>
> Is this fixing an actual bug or is there some other reason for doing
> this?
>
> >
> > Signed-off-by: Waiman Long <[email protected]>
> > ---
> > drivers/idle/intel_idle.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> > index aa2d19db2b1d..07fa23707b3c 100644
> > --- a/drivers/idle/intel_idle.c
> > +++ b/drivers/idle/intel_idle.c
> > @@ -181,13 +181,17 @@ static __cpuidle int intel_idle_ibrs(struct cpuidle_device *dev,
> > u64 spec_ctrl = spec_ctrl_current();
> > int ret;
> >
> > - if (smt_active)
> > + if (smt_active) {
> > + __this_cpu_write(x86_spec_ctrl_current, 0);
> > native_wrmsrl(MSR_IA32_SPEC_CTRL, 0);
> > + }
> >
> > ret = __intel_idle(dev, drv, index);
> >
> > - if (smt_active)
> > + if (smt_active) {
> > native_wrmsrl(MSR_IA32_SPEC_CTRL, spec_ctrl);
> > + __this_cpu_write(x86_spec_ctrl_current, spec_ctrl);
> > + }
>
> More candidates for update_spec_ctrl()?
Both this and the play_dead case can't use update_spec_ctrl() because
RCU isn't there anymore and all that is noinstr. Additionally, both
sites rely on preemption being off already, where update_spec_ctrl()
can't do that.
That said, I suppose one could write it like so:
static __always_inline __update_spec_ctrl(u64 val)
{
__this_cpu_write(x86_spec_ctrl_current, val);
native_wrmsrl(MSR_IA32_SPEC_CTRL, val);
}
static void update_spec_ctrl(u64 val)
{
preempt_disable();
__update_spec_ctrl(val);
preempt_enable();
}
And then you can use __update_spec_ctrl(). But that would need a wee
audit of using native_wrmsrl() in all places, probably ok, IIRC Xen
wasn't using our IBRS stuff anyway.
On Wed, Jun 21, 2023 at 10:46:33PM -0700, Josh Poimboeuf wrote:
> On Wed, Jun 21, 2023 at 08:36:02PM -0400, Waiman Long wrote:
> > When intel_idle_ibrs() is called, it modifies the SPEC_CTRL MSR to
> > 0 in order disable IBRS. However, the new MSR value isn't reflected
> > in x86_spec_ctrl_current which is at odd with the other code that
> > keep track of its state in that percpu variable. Fix that by updating
> > x86_spec_ctrl_current percpu value to always match the content of the
> > SPEC_CTRL MSR.
>
> Is this fixing an actual bug or is there some other reason for doing
> this?
No actual bug, he did this for his debugfs file -- which is no longer
part of the series. With that on, you can observe the
x86_spec_ctrl_current value while idle.
Arguably that's not even inconsistent because we disable/enable the
thing with IRQs disabled, so nothing can observe the difference.
On 6/22/23 05:40, Peter Zijlstra wrote:
> On Wed, Jun 21, 2023 at 10:46:33PM -0700, Josh Poimboeuf wrote:
>> On Wed, Jun 21, 2023 at 08:36:02PM -0400, Waiman Long wrote:
>>> When intel_idle_ibrs() is called, it modifies the SPEC_CTRL MSR to
>>> 0 in order disable IBRS. However, the new MSR value isn't reflected
>>> in x86_spec_ctrl_current which is at odd with the other code that
>>> keep track of its state in that percpu variable. Fix that by updating
>>> x86_spec_ctrl_current percpu value to always match the content of the
>>> SPEC_CTRL MSR.
>> Is this fixing an actual bug or is there some other reason for doing
>> this?
> No actual bug, he did this for his debugfs file -- which is no longer
> part of the series. With that on, you can observe the
> x86_spec_ctrl_current value while idle.
Right. That is the main reason as I want the SPEC_CTRL MSRs value to be
observable by some external mean:-)
Regards,
Longman
On 6/22/23 01:46, Josh Poimboeuf wrote:
> On Wed, Jun 21, 2023 at 08:36:02PM -0400, Waiman Long wrote:
>> When intel_idle_ibrs() is called, it modifies the SPEC_CTRL MSR to
>> 0 in order disable IBRS. However, the new MSR value isn't reflected
>> in x86_spec_ctrl_current which is at odd with the other code that
>> keep track of its state in that percpu variable. Fix that by updating
>> x86_spec_ctrl_current percpu value to always match the content of the
>> SPEC_CTRL MSR.
> Is this fixing an actual bug or is there some other reason for doing
> this?
It is not a bug per se. It is mainly to make the per cpu variable more
up to date.
>
>> Signed-off-by: Waiman Long <[email protected]>
>> ---
>> drivers/idle/intel_idle.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
>> index aa2d19db2b1d..07fa23707b3c 100644
>> --- a/drivers/idle/intel_idle.c
>> +++ b/drivers/idle/intel_idle.c
>> @@ -181,13 +181,17 @@ static __cpuidle int intel_idle_ibrs(struct cpuidle_device *dev,
>> u64 spec_ctrl = spec_ctrl_current();
>> int ret;
>>
>> - if (smt_active)
>> + if (smt_active) {
>> + __this_cpu_write(x86_spec_ctrl_current, 0);
>> native_wrmsrl(MSR_IA32_SPEC_CTRL, 0);
>> + }
>>
>> ret = __intel_idle(dev, drv, index);
>>
>> - if (smt_active)
>> + if (smt_active) {
>> native_wrmsrl(MSR_IA32_SPEC_CTRL, spec_ctrl);
>> + __this_cpu_write(x86_spec_ctrl_current, spec_ctrl);
>> + }
> More candidates for update_spec_ctrl()?
I don't think we can use update_spec_ctrl() here simply because of the
noinstr requirement. See commit 9b461a6faae7 ("cpuidle, intel_idle: Fix
CPUIDLE_FLAG_IBRS").
Cheers,
Longman
On 6/22/23 05:38, Peter Zijlstra wrote:
> On Wed, Jun 21, 2023 at 10:46:33PM -0700, Josh Poimboeuf wrote:
>> On Wed, Jun 21, 2023 at 08:36:02PM -0400, Waiman Long wrote:
>>> When intel_idle_ibrs() is called, it modifies the SPEC_CTRL MSR to
>>> 0 in order disable IBRS. However, the new MSR value isn't reflected
>>> in x86_spec_ctrl_current which is at odd with the other code that
>>> keep track of its state in that percpu variable. Fix that by updating
>>> x86_spec_ctrl_current percpu value to always match the content of the
>>> SPEC_CTRL MSR.
>> Is this fixing an actual bug or is there some other reason for doing
>> this?
>>
>>> Signed-off-by: Waiman Long <[email protected]>
>>> ---
>>> drivers/idle/intel_idle.c | 8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
>>> index aa2d19db2b1d..07fa23707b3c 100644
>>> --- a/drivers/idle/intel_idle.c
>>> +++ b/drivers/idle/intel_idle.c
>>> @@ -181,13 +181,17 @@ static __cpuidle int intel_idle_ibrs(struct cpuidle_device *dev,
>>> u64 spec_ctrl = spec_ctrl_current();
>>> int ret;
>>>
>>> - if (smt_active)
>>> + if (smt_active) {
>>> + __this_cpu_write(x86_spec_ctrl_current, 0);
>>> native_wrmsrl(MSR_IA32_SPEC_CTRL, 0);
>>> + }
>>>
>>> ret = __intel_idle(dev, drv, index);
>>>
>>> - if (smt_active)
>>> + if (smt_active) {
>>> native_wrmsrl(MSR_IA32_SPEC_CTRL, spec_ctrl);
>>> + __this_cpu_write(x86_spec_ctrl_current, spec_ctrl);
>>> + }
>> More candidates for update_spec_ctrl()?
> Both this and the play_dead case can't use update_spec_ctrl() because
> RCU isn't there anymore and all that is noinstr. Additionally, both
> sites rely on preemption being off already, where update_spec_ctrl()
> can't do that.
>
> That said, I suppose one could write it like so:
>
> static __always_inline __update_spec_ctrl(u64 val)
> {
> __this_cpu_write(x86_spec_ctrl_current, val);
> native_wrmsrl(MSR_IA32_SPEC_CTRL, val);
> }
We can put this into asm/nospec-branch.h since x86_spec_ctrl_current is
defined there as well.
Cheers,
Longman
On Thu, Jun 22, 2023 at 11:38:28AM +0200, Peter Zijlstra wrote:
> Both this and the play_dead case can't use update_spec_ctrl() because
> RCU isn't there anymore and all that is noinstr. Additionally, both
> sites rely on preemption being off already, where update_spec_ctrl()
> can't do that.
Oh, I might be wrong about the preemption thing; it doesn't make sense
to do wrmsr with preemption on, so it could be simpler.
> That said, I suppose one could write it like so:
>
> static __always_inline __update_spec_ctrl(u64 val)
> {
> __this_cpu_write(x86_spec_ctrl_current, val);
> native_wrmsrl(MSR_IA32_SPEC_CTRL, val);
> }
>
> static void update_spec_ctrl(u64 val)
> {
> preempt_disable();
> __update_spec_ctrl(val);
> preempt_enable();
> }
>
> And then you can use __update_spec_ctrl(). But that would need a wee
> audit of using native_wrmsrl() in all places, probably ok, IIRC Xen
> wasn't using our IBRS stuff anyway.