Re: [PATCH v2 5/9] x86/spec-ctrl: Record the last write to MSR_SPEC_CTRL

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 31/01/2022 10:20, Jan Beulich wrote:
> On 28.01.2022 14:29, Andrew Cooper wrote:
>> In some cases, writes to MSR_SPEC_CTRL do not have interesting side effects,
>> and we should implement lazy context switching like we do with other MSRs.
>>
>> In the short term, this will be used by the SVM infrastructure, but I expect
>> to extend it to other contexts in due course.
>>
>> Introduce cpu_info.last_spec_ctrl for the purpose, and cache writes made from
>> the boot/resume paths.  The value can't live in regular per-cpu data when it
>> is eventually used for PV guests when XPTI might be active.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Technically
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

> But ...
>
>> --- a/xen/arch/x86/acpi/power.c
>> +++ b/xen/arch/x86/acpi/power.c
>> @@ -296,7 +296,10 @@ static int enter_state(u32 state)
>>      ci->spec_ctrl_flags |= (default_spec_ctrl_flags & SCF_ist_wrmsr);
>>  
>>      if ( boot_cpu_has(X86_FEATURE_IBRSB) )
>> +    {
>>          wrmsrl(MSR_SPEC_CTRL, default_xen_mcu_opt_ctrl);
>> +        ci->last_spec_ctrl = default_xen_mcu_opt_ctrl;
>> +    }
> ... wouldn't we better have a write_spec_ctrl() helper doing both,
> perhaps with the check-if-same logic added as well (overridable by
> a "force" boolean input, unless the cases where the write cannot be
> skipped can be entirely determined from previous and new values)?

I considered that, but decided against it.  The PV and IST caching logic
is likely to be in asm, meaning that the single piece of C check-if-same
logic is in vmentry_spec_ctrl().

i.e. I think this is the right way forward.  By the end of the PV work,
if there is obvious tidy-up to do, I will do so.

~Andrew