Re: [PATCH v3 7/9] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 01/02/2022 11:47, Jan Beulich wrote:
> On 31.01.2022 16:36, Andrew Cooper wrote:
>> Hardware maintains both host and guest versions of MSR_SPEC_CTRL, but guests
>> run with the logical OR of both values.  Therefore, in principle we want to
>> clear Xen's value before entering the guest.  However, for migration
>> compatibility,
> I think you've explained this to me before, but I can't seem to put
> all of it together already now. Could expand on how a non-zero value
> behind a guest's back can help with migration compatibility? At the
> first glance I would be inclined to say only what the guest actually
> gets to see and use can affect its migration.

For VMs which see VIRT_SPEC_CTRL (compatibility with Fam15 thru Zen1),
writes of VIRT_SPEC_CTRL.SSBD (probably) need to use
SSBD-behind-the-guest's back.  I say probably, because I think this is
the least bad implementation option, but until we have working support,
it's still a guess.

For the ultra paranoid, a VM migrating in which can't see PSFD (e.g. for
compatibility with Zen2) should have PSFD set behind it's back.  Except
that SSBD also has an appropriate side effect so that existing "I'm a
piece of critical code" signals that have grown in various OSes continue
to do the safe thing on PSFD-capable hardware.  Given that we don't
activate SSBD by default, we shouldn't default disable PFSD behind an
unaware guest either.

That then leaves the meaning of spec-ctrl=ssbd,psfd because ssbd is
currently system wide (if enabled) on AMD.  This series changes that for
HVM guests, and it will change again shortly for PV guests, and this is
obviously the better default behaviour.  But we could have a system wide
option on top of guest support in most cases if someone sees a need.

>> and for performance reasons with SEV-SNP guests, we want the
>> ability to use a nonzero value behind the guest's back.

For completeness, for SEV-SNP, IBRS needs setting to avoid vmentry
issuing IBPB.  More specifically, the VMexit=>Entry path must not clear
IBRS, at which point hardware knows that nothing can have got into the
indirect predictor.


>>   Use vcpu_msrs to hold
>> this value, with the guest value in the VMCB.
>>
>> On the VMEntry path, adjusting MSR_SPEC_CTRL must be done after CLGI so as to
>> be atomic with respect to NMIs/etc.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Preferably with the above expansion and with one further style
> issue (see below) taken care of
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks

>
>> --- a/xen/arch/x86/hvm/svm/entry.S
>> +++ b/xen/arch/x86/hvm/svm/entry.S
>> @@ -55,11 +55,23 @@ __UNLIKELY_END(nsvm_hap)
>>          mov  %rsp, %rdi
>>          call svm_vmenter_helper
>>  
>> -        mov VCPU_arch_msrs(%rbx), %rax
>> -        mov VCPUMSR_spec_ctrl_raw(%rax), %eax
>> +        clgi
>>  
>>          /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
>> -        /* SPEC_CTRL_EXIT_TO_SVM   (nothing currently) */
>> +        /* SPEC_CTRL_EXIT_TO_SVM       Req: b=curr %rsp=regs/cpuinfo, Clob: 
>> acd */
>> +        .macro svm_vmentry_spec_ctrl
>> +            mov    VCPU_arch_msrs(%rbx), %rax
>> +            movzbl CPUINFO_last_spec_ctrl(%rsp), %edx
>> +            mov    VCPUMSR_spec_ctrl_raw(%rax), %eax
>> +            cmp    %edx, %eax
>> +            je 1f  /* Skip write if value is correct. */
> Wold you mind padding the insn operand properly, in line with all
> others nearby?

Oops yes.  Fixed.

~Andrew