On Thu, Jul 27, 2023, Weijiang Yang wrote: > > On 7/27/2023 1:16 PM, Chao Gao wrote: > > > > > @@ -2402,6 +2417,31 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > > > > else > > > > > vmx->pt_desc.guest.addr_a[index / 2] = data; > > > > > break; > > > > > +#define VMX_CET_CONTROL_MASK (~GENMASK_ULL(9, 6)) > > > > bits9-6 are reserved for both intel and amd. Shouldn't this check be > > > > done in the common code? > > > My thinking is, on AMD platform, bit 63:2 is anyway reserved since it doesn't > > > support IBT, > > You can only say > > > > bits 5:2 and bits 63:10 are reserved since AMD doens't support IBT. > > > > bits 9:6 are reserved regardless of the support of IBT. > > > > > so the checks in common code for AMD is enough, when the execution flow comes > > > here, > > > > > > it should be vmx, and need this additional check. > > The checks against reserved bits are common for AMD and Intel: > > > > 1. if SHSTK is supported, bit1:0 are not reserved. > > 2. if IBT is supported, bit5:2 and bit63:10 are not reserved > > 3. bit9:6 are always reserved. > > > > There is nothing specific to Intel. +1 > So you want the code to be: > > +#define CET_IBT_MASK_BITS (GENMASK_ULL(5, 2) | GENMASK_ULL(63, > 10)) > > +#define CET_CTRL_RESERVED_BITS GENMASK(9, 6) > > +#define CET_SHSTK_MASK_BITSGENMASK(1, 0) > > +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)) || > > (data & CET_CTRL_RESERVED_BITS) ) > > ^^^^^^^^^^^^^^^^^^^^^^^^^ Yes, though I vote to separate each check, e.g. if (data & CET_CTRL_RESERVED_BITS) return 1; if (!guest_can_use(vcpu, X86_FEATURE_SHSTK) && (data & CET_SHSTK_MASK_BITS)) return 1; if (!guest_can_use(vcpu, X86_FEATURE_IBT) && (data & CET_IBT_MASK_BITS)) return 1; I would expect the code generation to be similar, if not outright identical, and IMO it's easier to quickly understand the flow if each check is a separate if-statement.
