| Message ID | 20220512031803.3315890-14-xiaoyao.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDX QEMU support | expand |
> Validate TD attributes with tdx_caps that fixed-0 bits must be zero and > fixed-1 bits must be set. > -static void setup_td_guest_attributes(X86CPU *x86cpu) > +static int tdx_validate_attributes(TdxGuest *tdx) > +{ > + if (((tdx->attributes & tdx_caps->attrs_fixed0) | tdx_caps->attrs_fixed1) != > + tdx->attributes) { > + error_report("Invalid attributes 0x%lx for TDX VM (fixed0 0x%llx, fixed1 0x%llx)", > + tdx->attributes, tdx_caps->attrs_fixed0, tdx_caps->attrs_fixed1); > + return -EINVAL; > + } So, how is this supposed to work? Patch #2 introduces attributes as user-settable property. So do users have to manually figure and pass the correct value, so the check passes? Specifically the fixed1 check? I think 'attributes' should not be user-settable in the first place. Each feature-bit which is actually user-settable (and not already covered by another option like pmu) should be a separate attribute for tdx-object. Then the tdx code can create attributes from hardware capabilities and user settings. When user-settable options might not be available depending on hardware capabilities best practice is to create them as OnOffAuto properties. Auto == qemu can pick the value, typical behavior is to enable the feature if the hardware supports it. On == must enable, if it isn't possible throw an error and exit. Off == must disable, if it isn't possible throw an error and exit. take care, Gerd
On 5/23/2022 5:39 PM, Gerd Hoffmann wrote: >> Validate TD attributes with tdx_caps that fixed-0 bits must be zero and >> fixed-1 bits must be set. > >> -static void setup_td_guest_attributes(X86CPU *x86cpu) >> +static int tdx_validate_attributes(TdxGuest *tdx) >> +{ >> + if (((tdx->attributes & tdx_caps->attrs_fixed0) | tdx_caps->attrs_fixed1) != >> + tdx->attributes) { >> + error_report("Invalid attributes 0x%lx for TDX VM (fixed0 0x%llx, fixed1 0x%llx)", >> + tdx->attributes, tdx_caps->attrs_fixed0, tdx_caps->attrs_fixed1); >> + return -EINVAL; >> + } > > So, how is this supposed to work? Patch #2 introduces attributes as > user-settable property. So do users have to manually figure and pass > the correct value, so the check passes? Specifically the fixed1 check? > > I think 'attributes' should not be user-settable in the first place. > Each feature-bit which is actually user-settable (and not already > covered by another option like pmu) should be a separate attribute for > tdx-object. Then the tdx code can create attributes from hardware > capabilities and user settings. In patch #2, tdx-guest.attributes is defined as a field to hold a 64 bits value of attributes but it doesn't provide any getter/setter for it. So it's *not* user-settable. Did I miss something? (I'm not good at QEMU object) > When user-settable options might not be available depending on hardware > capabilities best practice is to create them as OnOffAuto properties. > > Auto == qemu can pick the value, typical behavior is to enable the > feature if the hardware supports it. > On == must enable, if it isn't possible throw an error and exit. > Off == must disable, if it isn't possible throw an error and exit. > > take care, > Gerd >
On Tue, May 24, 2022 at 12:19:51PM +0800, Xiaoyao Li wrote: > On 5/23/2022 5:39 PM, Gerd Hoffmann wrote: > > So, how is this supposed to work? Patch #2 introduces attributes as > > user-settable property. So do users have to manually figure and pass > > the correct value, so the check passes? Specifically the fixed1 check? > > > > I think 'attributes' should not be user-settable in the first place. > > Each feature-bit which is actually user-settable (and not already > > covered by another option like pmu) should be a separate attribute for > > tdx-object. Then the tdx code can create attributes from hardware > > capabilities and user settings. > > In patch #2, tdx-guest.attributes is defined as a field to hold a 64 bits > value of attributes but it doesn't provide any getter/setter for it. So it's > *not* user-settable. Ok. Why it is declared as object property in the first place then? take care, Gerd
On 5/24/2022 2:59 PM, Gerd Hoffmann wrote: > On Tue, May 24, 2022 at 12:19:51PM +0800, Xiaoyao Li wrote: >> On 5/23/2022 5:39 PM, Gerd Hoffmann wrote: >>> So, how is this supposed to work? Patch #2 introduces attributes as >>> user-settable property. So do users have to manually figure and pass >>> the correct value, so the check passes? Specifically the fixed1 check? >>> >>> I think 'attributes' should not be user-settable in the first place. >>> Each feature-bit which is actually user-settable (and not already >>> covered by another option like pmu) should be a separate attribute for >>> tdx-object. Then the tdx code can create attributes from hardware >>> capabilities and user settings. >> >> In patch #2, tdx-guest.attributes is defined as a field to hold a 64 bits >> value of attributes but it doesn't provide any getter/setter for it. So it's >> *not* user-settable. > > Ok. Why it is declared as object property in the first place then? Is there another way to define a member/field of object besides property? > take care, > Gerd >
On Tue, May 24, 2022 at 04:11:56PM +0800, Xiaoyao Li wrote: > On 5/24/2022 2:59 PM, Gerd Hoffmann wrote: > > On Tue, May 24, 2022 at 12:19:51PM +0800, Xiaoyao Li wrote: > > > On 5/23/2022 5:39 PM, Gerd Hoffmann wrote: > > > > So, how is this supposed to work? Patch #2 introduces attributes as > > > > user-settable property. So do users have to manually figure and pass > > > > the correct value, so the check passes? Specifically the fixed1 check? > > > > > > > > I think 'attributes' should not be user-settable in the first place. > > > > Each feature-bit which is actually user-settable (and not already > > > > covered by another option like pmu) should be a separate attribute for > > > > tdx-object. Then the tdx code can create attributes from hardware > > > > capabilities and user settings. > > > > > > In patch #2, tdx-guest.attributes is defined as a field to hold a 64 bits > > > value of attributes but it doesn't provide any getter/setter for it. So it's > > > *not* user-settable. > > > > Ok. Why it is declared as object property in the first place then? > > Is there another way to define a member/field of object besides property? Well, the C object struct is completely independent from the qapi struct. Typically qapi-generated structs are added as struct fields. Look at ui/input-linux.c for example. struct InputLinux holds all the object state. It has a GrabToggleKeys field, that is a qapi-generated enum (see qapi/common.json) and is user-configurable (there are getter and setter for it). So, you can have a private 'attributes' struct field in your tdx class, but the field doesn't have to be in the qapi struct for that. HTH, Gerd
On 5/24/2022 4:29 PM, Gerd Hoffmann wrote: > On Tue, May 24, 2022 at 04:11:56PM +0800, Xiaoyao Li wrote: >> On 5/24/2022 2:59 PM, Gerd Hoffmann wrote: >>> On Tue, May 24, 2022 at 12:19:51PM +0800, Xiaoyao Li wrote: >>>> On 5/23/2022 5:39 PM, Gerd Hoffmann wrote: >>>>> So, how is this supposed to work? Patch #2 introduces attributes as >>>>> user-settable property. So do users have to manually figure and pass >>>>> the correct value, so the check passes? Specifically the fixed1 check? >>>>> >>>>> I think 'attributes' should not be user-settable in the first place. >>>>> Each feature-bit which is actually user-settable (and not already >>>>> covered by another option like pmu) should be a separate attribute for >>>>> tdx-object. Then the tdx code can create attributes from hardware >>>>> capabilities and user settings. >>>> >>>> In patch #2, tdx-guest.attributes is defined as a field to hold a 64 bits >>>> value of attributes but it doesn't provide any getter/setter for it. So it's >>>> *not* user-settable. >>> >>> Ok. Why it is declared as object property in the first place then? >> >> Is there another way to define a member/field of object besides property? > > Well, the C object struct is completely independent from the qapi > struct. Typically qapi-generated structs are added as struct fields. > Look at ui/input-linux.c for example. > > struct InputLinux holds all the object state. It has a GrabToggleKeys > field, that is a qapi-generated enum (see qapi/common.json) and is > user-configurable (there are getter and setter for it). > > So, you can have a private 'attributes' struct field in your tdx class, > but the field doesn't have to be in the qapi struct for that. I see. Thanks for the explanation! I will remove the qom property definition in patch 2. > HTH, > Gerd >
diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c index e9c6e6fb396c..9f2cdf640b5c 100644 --- a/target/i386/kvm/tdx.c +++ b/target/i386/kvm/tdx.c @@ -31,6 +31,7 @@ (1ULL << KVM_FEATURE_PV_SCHED_YIELD) | \ (1ULL << KVM_FEATURE_MSI_EXT_DEST_ID)) +#define TDX_TD_ATTRIBUTES_DEBUG BIT_ULL(0) #define TDX_TD_ATTRIBUTES_PKS BIT_ULL(30) #define TDX_TD_ATTRIBUTES_PERFMON BIT_ULL(63) @@ -169,13 +170,32 @@ void tdx_get_supported_cpuid(uint32_t function, uint32_t index, int reg, } } -static void setup_td_guest_attributes(X86CPU *x86cpu) +static int tdx_validate_attributes(TdxGuest *tdx) +{ + if (((tdx->attributes & tdx_caps->attrs_fixed0) | tdx_caps->attrs_fixed1) != + tdx->attributes) { + error_report("Invalid attributes 0x%lx for TDX VM (fixed0 0x%llx, fixed1 0x%llx)", + tdx->attributes, tdx_caps->attrs_fixed0, tdx_caps->attrs_fixed1); + return -EINVAL; + } + + if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) { + error_report("Current QEMU doesn't support attributes.debug[bit 0] for TDX VM"); + return -EINVAL; + } + + return 0; +} + +static int setup_td_guest_attributes(X86CPU *x86cpu) { CPUX86State *env = &x86cpu->env; tdx_guest->attributes |= (env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_PKS) ? TDX_TD_ATTRIBUTES_PKS : 0; tdx_guest->attributes |= x86cpu->enable_pmu ? TDX_TD_ATTRIBUTES_PERFMON : 0; + + return tdx_validate_attributes(tdx_guest); } int tdx_pre_create_vcpu(CPUState *cpu) @@ -191,7 +211,10 @@ int tdx_pre_create_vcpu(CPUState *cpu) goto out; } - setup_td_guest_attributes(x86cpu); + r = setup_td_guest_attributes(x86cpu); + if (r) { + goto out; + } memset(&init_vm, 0, sizeof(init_vm)); init_vm.cpuid.nent = kvm_x86_arch_cpuid(env, init_vm.entries, 0);
Validate TD attributes with tdx_caps that fixed-0 bits must be zero and fixed-1 bits must be set. Besides, sanity check the attribute bits that have not been supported by QEMU yet. e.g., debug bit, it will be allowed in the future when debug TD support lands in QEMU. Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> --- target/i386/kvm/tdx.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)