Messages in this thread Patch in this message | | | Date | Thu, 27 Jul 2023 14:06:11 +0800 | Subject | Re: [PATCH v4 09/20] KVM:x86: Add common code of CET MSR access | From | "Yang, Weijiang" <> |
| |
On 7/26/2023 9:46 PM, Chao Gao wrote: > On Wed, Jul 26, 2023 at 04:26:06PM +0800, Yang, Weijiang wrote: >>>> + /* >>>> + * This function cannot work without later CET MSR read/write >>>> + * emulation patch. >>> Probably you should consider merging the "later" patch into this one. >>> Then you can get rid of this comment and make this patch easier for >>> review ... >> Which later patch you mean? If you mean [13/20] KVM:VMX: Emulate read and >> write to CET MSRs, >> >> then I intentionally separate these two, this one is for CET MSR common >> checks and operations, >> >> the latter is specific to VMX, and add the above comments in case someone is > The problem of this organization is the handling of S_CET, SSP, INT_SSP_TABLE > MSR is incomplete in this patch. I think a better organization is to either > merge this patch and patch 13, or move all changes related to S_CET, SSP, > INT_SSP_TABLE into patch 13? e.g.,
Yes, I'm thinking of merging this patch with patch 13 to make it complete, thanks for
the suggestion!
> > case MSR_IA32_U_CET: > - case MSR_IA32_S_CET: > if (!kvm_cet_is_msr_accessible(vcpu, msr_info)) > return 1; > if ((!guest_can_use(vcpu, X86_FEATURE_SHSTK) && > (data & CET_SHSTK_MASK_BITS)) || > (!guest_can_use(vcpu, X86_FEATURE_IBT) && > (data & CET_IBT_MASK_BITS))) > return 1; > - if (msr == MSR_IA32_U_CET) > - kvm_set_xsave_msr(msr_info); > kvm_set_xsave_msr(msr_info); > break; > - case MSR_KVM_GUEST_SSP: > - case MSR_IA32_PL0_SSP ... MSR_IA32_INT_SSP_TAB: > case MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP: > if (!kvm_cet_is_msr_accessible(vcpu, msr_info)) > return 1; > if (is_noncanonical_address(data, vcpu)) > return 1; > if (!IS_ALIGNED(data, 4)) > return 1; > if (msr == MSR_IA32_PL0_SSP || msr == MSR_IA32_PL1_SSP || > msr == MSR_IA32_PL2_SSP) { > vcpu->arch.cet_s_ssp[msr - MSR_IA32_PL0_SSP] = data; > } else if (msr == MSR_IA32_PL3_SSP) { > kvm_set_xsave_msr(msr_info); > } > break; > > > > BTW, shouldn't bit2:0 of MSR_KVM_GUEST_SSP be 0? i.e., for MSR_KVM_GUEST_SSP, > the alignment check should be IS_ALIGNED(data, 8).
The check for GUEST_SSP should be consistent with that of PLx_SSPs, otherwise there would
be issues, maybe I need to change the alignment check as :
#ifdef CONFIG_X86_64
if (!IS_ALIGNED(data, 8)) return 1; #else if (!IS_ALIGNED(data, 4))
return 1; #endif
> >> bisecting >> >> the patches and happens to split at this patch, then it would faulted and >> take some actions. > I am not sure what kind of issue you are worrying about. In my understanding, > KVM hasn't advertised the support of IBT and SHSTK, so, > kvm_cpu_cap_has(X86_FEATURE_SHSTK/IBT) will always return false. and then > kvm_cet_is_msr_accessible() is guaranteed to return false. > > If there is any issue in your mind, you can fix it or reorganize your patches to > avoid the issue. To me, adding a comment and a warning is not a good solution.
I will reorganize the patches and merge the code in this patch to patch 13.
> >>>> int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) >>>> { >>>> u32 msr = msr_info->index; >>>> @@ -3982,6 +4023,35 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) >>>> vcpu->arch.guest_fpu.xfd_err = data; >>>> break; >>>> #endif >>>> +#define CET_IBT_MASK_BITS GENMASK_ULL(63, 2) >>> bit9:6 are reserved even if IBT is supported. >> Yes, as IBT is only available on Intel platforms, I move the handling of bit >> 9:6 to VMX related patch. > IIUC, bits 9:6 are not reserved for IBT. I don't get how IBT availability > affects the handling of bits 9:6.
I handle it in this way, when IBT is not available all bits 63:2 should be handled as reserved. When IBT is
available, additional checks for bits 9:6 should be enforced.
> >> Here's the common check in case IBT is not available. >> >>>> @@ -12131,6 +12217,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) >>>> >>>> vcpu->arch.cr3 = 0; >>>> kvm_register_mark_dirty(vcpu, VCPU_EXREG_CR3); >>>> + memset(vcpu->arch.cet_s_ssp, 0, sizeof(vcpu->arch.cet_s_ssp)); >>> ... this begs the question: where other MSRs are reset. I suppose >>> U_CET/PL3_SSP are handled when resetting guest FPU. But how about S_CET >>> and INT_SSP_TAB? there is no answer in this patch. >> I think the related guest VMCS fields(S_CET/INT_SSP_TAB/SSP) should be reset >> to 0 in vmx_vcpu_reset(), >> >> do you think so? > Yes, looks good.
| |