| Message ID | 20241105062408.3533704-14-xiaoyao.li@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | QEMU TDX support | expand |
On Tue, Nov 05, 2024 at 01:23:21AM -0500, Xiaoyao Li wrote: > 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> > Acked-by: Gerd Hoffmann <kraxel@redhat.com> > > --- > Changes in v3: > - using error_setg() for error report; (Daniel) > --- > target/i386/kvm/tdx.c | 28 ++++++++++++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > > diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c > index 6cf81f788fe0..5a9ce2ada89d 100644 > --- a/target/i386/kvm/tdx.c > +++ b/target/i386/kvm/tdx.c > @@ -20,6 +20,7 @@ > #include "kvm_i386.h" > #include "tdx.h" > > +#define TDX_TD_ATTRIBUTES_DEBUG BIT_ULL(0) > #define TDX_TD_ATTRIBUTES_SEPT_VE_DISABLE BIT_ULL(28) > #define TDX_TD_ATTRIBUTES_PKS BIT_ULL(30) > #define TDX_TD_ATTRIBUTES_PERFMON BIT_ULL(63) > @@ -141,13 +142,33 @@ static int tdx_kvm_type(X86ConfidentialGuest *cg) > return KVM_X86_TDX_VM; > } > > -static void setup_td_guest_attributes(X86CPU *x86cpu) > +static int tdx_validate_attributes(TdxGuest *tdx, Error **errp) > +{ > + if ((tdx->attributes & ~tdx_caps->supported_attrs)) { > + error_setg(errp, "Invalid attributes 0x%lx for TDX VM " > + "(supported: 0x%llx)", > + tdx->attributes, tdx_caps->supported_attrs); > + return -1; Minor whitespace accident, with indentation too deep. > + } > + > + if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) { > + error_setg(errp, "Current QEMU doesn't support attributes.debug[bit 0] " > + "for TDX VM"); > + return -1; > + } > + > + return 0; > +} With regards, Daniel
On 11/5/2024 6:36 PM, Daniel P. Berrangé wrote: > On Tue, Nov 05, 2024 at 01:23:21AM -0500, Xiaoyao Li wrote: >> 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> >> Acked-by: Gerd Hoffmann <kraxel@redhat.com> >> >> --- >> Changes in v3: >> - using error_setg() for error report; (Daniel) >> --- >> target/i386/kvm/tdx.c | 28 ++++++++++++++++++++++++++-- >> 1 file changed, 26 insertions(+), 2 deletions(-) >> >> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c >> index 6cf81f788fe0..5a9ce2ada89d 100644 >> --- a/target/i386/kvm/tdx.c >> +++ b/target/i386/kvm/tdx.c >> @@ -20,6 +20,7 @@ >> #include "kvm_i386.h" >> #include "tdx.h" >> >> +#define TDX_TD_ATTRIBUTES_DEBUG BIT_ULL(0) >> #define TDX_TD_ATTRIBUTES_SEPT_VE_DISABLE BIT_ULL(28) >> #define TDX_TD_ATTRIBUTES_PKS BIT_ULL(30) >> #define TDX_TD_ATTRIBUTES_PERFMON BIT_ULL(63) >> @@ -141,13 +142,33 @@ static int tdx_kvm_type(X86ConfidentialGuest *cg) >> return KVM_X86_TDX_VM; >> } >> >> -static void setup_td_guest_attributes(X86CPU *x86cpu) >> +static int tdx_validate_attributes(TdxGuest *tdx, Error **errp) >> +{ >> + if ((tdx->attributes & ~tdx_caps->supported_attrs)) { >> + error_setg(errp, "Invalid attributes 0x%lx for TDX VM " >> + "(supported: 0x%llx)", >> + tdx->attributes, tdx_caps->supported_attrs); >> + return -1; > > Minor whitespace accident, with indentation too deep. Good catch! btw, how did you catch it? any tool like checkpatch.pl or just by your eyes? >> + } >> + >> + if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) { >> + error_setg(errp, "Current QEMU doesn't support attributes.debug[bit 0] " >> + "for TDX VM"); >> + return -1; >> + } >> + >> + return 0; >> +} > > With regards, > Daniel
On Tue, Nov 05, 2024 at 07:53:57PM +0800, Xiaoyao Li wrote: > On 11/5/2024 6:36 PM, Daniel P. Berrangé wrote: > > On Tue, Nov 05, 2024 at 01:23:21AM -0500, Xiaoyao Li wrote: > > > 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> > > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> > > > > > > --- > > > Changes in v3: > > > - using error_setg() for error report; (Daniel) > > > --- > > > target/i386/kvm/tdx.c | 28 ++++++++++++++++++++++++++-- > > > 1 file changed, 26 insertions(+), 2 deletions(-) > > > > > > diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c > > > index 6cf81f788fe0..5a9ce2ada89d 100644 > > > --- a/target/i386/kvm/tdx.c > > > +++ b/target/i386/kvm/tdx.c > > > @@ -20,6 +20,7 @@ > > > #include "kvm_i386.h" > > > #include "tdx.h" > > > +#define TDX_TD_ATTRIBUTES_DEBUG BIT_ULL(0) > > > #define TDX_TD_ATTRIBUTES_SEPT_VE_DISABLE BIT_ULL(28) > > > #define TDX_TD_ATTRIBUTES_PKS BIT_ULL(30) > > > #define TDX_TD_ATTRIBUTES_PERFMON BIT_ULL(63) > > > @@ -141,13 +142,33 @@ static int tdx_kvm_type(X86ConfidentialGuest *cg) > > > return KVM_X86_TDX_VM; > > > } > > > -static void setup_td_guest_attributes(X86CPU *x86cpu) > > > +static int tdx_validate_attributes(TdxGuest *tdx, Error **errp) > > > +{ > > > + if ((tdx->attributes & ~tdx_caps->supported_attrs)) { > > > + error_setg(errp, "Invalid attributes 0x%lx for TDX VM " > > > + "(supported: 0x%llx)", > > > + tdx->attributes, tdx_caps->supported_attrs); > > > + return -1; > > > > Minor whitespace accident, with indentation too deep. > > Good catch! > > btw, how did you catch it? any tool like checkpatch.pl or just by your eyes? Nah, I just notice the mis-alignment when reading the patches. > > > > + } > > > + > > > + if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) { > > > + error_setg(errp, "Current QEMU doesn't support attributes.debug[bit 0] " > > > + "for TDX VM"); > > > + return -1; > > > + } > > > + > > > + return 0; > > > +} With regards, Daniel
On Tue, 2024-11-05 at 01:23 -0500, Xiaoyao Li wrote: > -static void setup_td_guest_attributes(X86CPU *x86cpu) > +static int tdx_validate_attributes(TdxGuest *tdx, Error **errp) > +{ > + if ((tdx->attributes & ~tdx_caps->supported_attrs)) { > + error_setg(errp, "Invalid attributes 0x%lx for TDX VM " > + "(supported: 0x%llx)", > + tdx->attributes, tdx_caps->supported_attrs); > + return -1; > + } > + > + if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) { What is going on here? It doesn't look like debug attribute could be set in this series, so this is dead code I guess. If there is some concern that attributes that need extra qemu support could be set in QEMU somehow, it would be better to have a mask of qemu supported attributes and reject any not in the mask. > + error_setg(errp, "Current QEMU doesn't support attributes.debug[bit 0] " > + "for TDX VM"); > + return -1; > + } > + > + return 0; > +}
On 11/6/2024 4:56 AM, Edgecombe, Rick P wrote: > On Tue, 2024-11-05 at 01:23 -0500, Xiaoyao Li wrote: >> -static void setup_td_guest_attributes(X86CPU *x86cpu) >> +static int tdx_validate_attributes(TdxGuest *tdx, Error **errp) >> +{ >> + if ((tdx->attributes & ~tdx_caps->supported_attrs)) { >> + error_setg(errp, "Invalid attributes 0x%lx for TDX VM " >> + "(supported: 0x%llx)", >> + tdx->attributes, tdx_caps->supported_attrs); >> + return -1; >> + } >> + >> + if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) { > > What is going on here? It doesn't look like debug attribute could be set in this > series, so this is dead code I guess. If there is some concern that attributes > that need extra qemu support could be set in QEMU somehow, it would be better to > have a mask of qemu supported attributes and reject any not in the mask. Good catch and good idea! Will maintain a mask of supported attributes in QEMU. >> + error_setg(errp, "Current QEMU doesn't support attributes.debug[bit 0] " >> + "for TDX VM"); >> + return -1; >> + } >> + >> + return 0; >> +} >
diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c index 6cf81f788fe0..5a9ce2ada89d 100644 --- a/target/i386/kvm/tdx.c +++ b/target/i386/kvm/tdx.c @@ -20,6 +20,7 @@ #include "kvm_i386.h" #include "tdx.h" +#define TDX_TD_ATTRIBUTES_DEBUG BIT_ULL(0) #define TDX_TD_ATTRIBUTES_SEPT_VE_DISABLE BIT_ULL(28) #define TDX_TD_ATTRIBUTES_PKS BIT_ULL(30) #define TDX_TD_ATTRIBUTES_PERFMON BIT_ULL(63) @@ -141,13 +142,33 @@ static int tdx_kvm_type(X86ConfidentialGuest *cg) return KVM_X86_TDX_VM; } -static void setup_td_guest_attributes(X86CPU *x86cpu) +static int tdx_validate_attributes(TdxGuest *tdx, Error **errp) +{ + if ((tdx->attributes & ~tdx_caps->supported_attrs)) { + error_setg(errp, "Invalid attributes 0x%lx for TDX VM " + "(supported: 0x%llx)", + tdx->attributes, tdx_caps->supported_attrs); + return -1; + } + + if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) { + error_setg(errp, "Current QEMU doesn't support attributes.debug[bit 0] " + "for TDX VM"); + return -1; + } + + return 0; +} + +static int setup_td_guest_attributes(X86CPU *x86cpu, Error **errp) { 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, errp); } static int setup_td_xfam(X86CPU *x86cpu, Error **errp) @@ -211,7 +232,10 @@ int tdx_pre_create_vcpu(CPUState *cpu, Error **errp) init_vm = g_malloc0(sizeof(struct kvm_tdx_init_vm) + sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES); - setup_td_guest_attributes(x86cpu); + r = setup_td_guest_attributes(x86cpu, errp); + if (r) { + return r; + } r = setup_td_xfam(x86cpu, errp); if (r) {