lkml.org 
[lkml]   [2023]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH v4 09/20] KVM:x86: Add common code of CET MSR access
From

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.

\
 
 \ /
  Last update: 2023-07-27 08:07    [W:0.085 / U:0.676 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site