On 6/4/2018 3:20 PM, Konrad Rzeszutek Wilk wrote: >>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >>> index 26110c202b19..950ec50f77c3 100644 >>> --- a/arch/x86/kvm/svm.c >>> +++ b/arch/x86/kvm/svm.c >>> @@ -4115,7 +4115,8 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) >>> break; >>> case MSR_IA32_SPEC_CTRL: >>> if (!msr_info->host_initiated && >>> - !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBRS)) >>> + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBRS) && >>> + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_SSBD)) >> >> Shouldn't the IBRS/SSBD check be an "or" check? I don't think it's >> necessarily true that IBRS and SSBD have to both be set. Maybe something >> like: >> >> if (!msr_info->host_initiated && >> !(guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBRS) || >> guest_cpuid_has(vcpu, X86_FEATURE_AMD_SSBD)) >> >> Does that make sense? > > The '!' on each of the CPUID and '&&' make this the same. See: Doh! Yes, I don't know what I was thinking. Just the end of a long week I guess. > > > AMD_IBRS set | AMD_SSBD set | !AMD_IBRS && !AMD_SSBD | !(AMD_IBRS || AMD_SSBD) > 0 | 0 | 1 && 1 -> return 1 | !(0) -> 1 -> return 1 > 1 | 0 | 0 && 1, continue | !(1 || 0) -> continue > 1 | 1 | 0 && 0, continue | !(1 || 1) -> continue > 0 | 1 | 1 && 0, continue | !(0 || 1) -> continue > > Meaning we will return 1 if: > the host has not initiator it or, > the guest CPUID does not have AMD_IBRS flag or, > the guest CPUID does not have AMD SSBD flag > > I am fine modifying it the way you had in mind, but in the past the logic > was to use ! and &&, hence stuck to that. No reason to change, it's fine the way you have it. Thanks, Tom >> >>> return 1; >>> >>> msr_info->data = svm->spec_ctrl; >>> @@ -4217,11 +4218,12 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) >>> break; >>> case MSR_IA32_SPEC_CTRL: >>> if (!msr->host_initiated && >>> - !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBRS)) >>> + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBRS) && >>> + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_SSBD)) >> >> Same question as above. >> >> Thanks, >> Tom >> >>> return 1; >>> >>> /* The STIBP bit doesn't fault even if it's not advertised */ >>> - if (data & ~(SPEC_CTRL_IBRS | SPEC_CTRL_STIBP)) >>> + if (data & ~(SPEC_CTRL_IBRS | SPEC_CTRL_STIBP | SPEC_CTRL_SSBD)) >>> return 1; >>> >>> svm->spec_ctrl = data; >>>
