1. Patchew
  2. linux
  3. [PATCH v3 0/3] x86/speculation: Disable IBRS when idle
  4. View patch

[PATCH v3 2/3] intel_idle: Sync up the SPEC_CTRL MSR value to x86_spec_ctrl_current

Waiman Long posted 3 patches 2 years, 7 months ago
There is a newer version of this series
[PATCH v3 2/3] intel_idle: Sync up the SPEC_CTRL MSR value to x86_spec_ctrl_current
Posted by Waiman Long 2 years, 7 months ago 
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 <longman@redhat.com>
---
 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
Re: [PATCH v3 2/3] intel_idle: Sync up the SPEC_CTRL MSR value to x86_spec_ctrl_current
Posted by Josh Poimboeuf 2 years, 7 months ago 
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 <longman@redhat.com>
> ---
>  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
Re: [PATCH v3 2/3] intel_idle: Sync up the SPEC_CTRL MSR value to x86_spec_ctrl_current
Posted by Waiman Long 2 years, 7 months ago 
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 <longman@redhat.com>
>> ---
>>   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
Re: [PATCH v3 2/3] intel_idle: Sync up the SPEC_CTRL MSR value to x86_spec_ctrl_current
Posted by Peter Zijlstra 2 years, 7 months ago 
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.
Re: [PATCH v3 2/3] intel_idle: Sync up the SPEC_CTRL MSR value to x86_spec_ctrl_current
Posted by Waiman Long 2 years, 7 months ago 
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
Re: [PATCH v3 2/3] intel_idle: Sync up the SPEC_CTRL MSR value to x86_spec_ctrl_current
Posted by Peter Zijlstra 2 years, 7 months ago 
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 <longman@redhat.com>
> > ---
> >  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.
Re: [PATCH v3 2/3] intel_idle: Sync up the SPEC_CTRL MSR value to x86_spec_ctrl_current
Posted by Peter Zijlstra 2 years, 7 months ago 
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.
Re: [PATCH v3 2/3] intel_idle: Sync up the SPEC_CTRL MSR value to x86_spec_ctrl_current
Posted by Waiman Long 2 years, 7 months ago 
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 <longman@redhat.com>
>>> ---
>>>   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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.