On 7/27/2023 11:20 PM, Sean Christopherson wrote:
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.
It looks good to me! Thank you!
