| Message ID | 20240605224409.2103109-1-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/i386: SEV: do not assume machine->cgs is SEV | expand |
On Thu, Jun 06, 2024 at 12:44:09AM +0200, Paolo Bonzini wrote: > Date: Thu, 6 Jun 2024 00:44:09 +0200 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: [PATCH] target/i386: SEV: do not assume machine->cgs is SEV > X-Mailer: git-send-email 2.45.1 > > There can be other confidential computing classes that are not derived > from sev-common. Avoid aborting when encountering them. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > target/i386/sev.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/target/i386/sev.c b/target/i386/sev.c > index 004c667ac14..97e15f8b7a9 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -1710,7 +1710,9 @@ void sev_es_set_reset_vector(CPUState *cpu) > { > X86CPU *x86; > CPUX86State *env; > - SevCommonState *sev_common = SEV_COMMON(MACHINE(qdev_get_machine())->cgs); > + ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs; > + SevCommonState *sev_common = SEV_COMMON( > + object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON)); SEV_COMMON(object_dynamic_cast()) looks to be twice cast, we can just force to do conversion with pointer type: (SevCommonState *) object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON) > /* Only update if we have valid reset information */ > if (!sev_common || !sev_common->reset_data_valid) { > -- > 2.45.1 > >
On 6/5/24 20:45, Zhao Liu wrote: >> @@ -1710,7 +1710,9 @@ void sev_es_set_reset_vector(CPUState *cpu) >> { >> X86CPU *x86; >> CPUX86State *env; >> - SevCommonState *sev_common = SEV_COMMON(MACHINE(qdev_get_machine())->cgs); >> + ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs; >> + SevCommonState *sev_common = SEV_COMMON( >> + object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON)); > > SEV_COMMON(object_dynamic_cast()) looks to be twice cast, we can just > force to do conversion with pointer type: > > (SevCommonState *) object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON) You don't need the explicit cast either, since C auto-converts from void*. sev_common = object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON); r~
Il gio 6 giu 2024, 05:52 Richard Henderson <richard.henderson@linaro.org> ha scritto: > > SEV_COMMON(object_dynamic_cast()) looks to be twice cast, we can just > > force to do conversion with pointer type: > > > > (SevCommonState *) object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON) > > You don't need the explicit cast either, since C auto-converts from void*. > > sev_common = object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON); > Doh, of course. Thanks to both! Paolo > > r~ > >
On 6/6/2024 6:44 AM, Paolo Bonzini wrote: > There can be other confidential computing classes that are not derived > from sev-common. Avoid aborting when encountering them. I hit it today when rebasing TDX patches to latest QEMU master, which has the SEV-SNP series merged. (I didn't get time to review it between it gets merged.) > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > target/i386/sev.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/target/i386/sev.c b/target/i386/sev.c > index 004c667ac14..97e15f8b7a9 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -1710,7 +1710,9 @@ void sev_es_set_reset_vector(CPUState *cpu) my approach is to guard with sev_enabled() when calling sev_es_set_reset_vector() in kvm_arch_reset_vcpu(), because calling sev* specific function in generic kvm code doesn't look reasonable to me. > { > X86CPU *x86; > CPUX86State *env; > - SevCommonState *sev_common = SEV_COMMON(MACHINE(qdev_get_machine())->cgs); > + ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs; > + SevCommonState *sev_common = SEV_COMMON( > + object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON)); > > /* Only update if we have valid reset information */ > if (!sev_common || !sev_common->reset_data_valid) {
On Thu, Jun 6, 2024 at 6:07 PM Xiaoyao Li <xiaoyao.li@intel.com> wrote: > > On 6/6/2024 6:44 AM, Paolo Bonzini wrote: > > There can be other confidential computing classes that are not derived > > from sev-common. Avoid aborting when encountering them. > > I hit it today when rebasing TDX patches to latest QEMU master, which > has the SEV-SNP series merged. (I didn't get time to review it between > it gets merged.) > > my approach is to guard with sev_enabled() when calling > sev_es_set_reset_vector() in kvm_arch_reset_vcpu(), because calling sev* > specific function in generic kvm code doesn't look reasonable to me. On the other hand I would like to avoid too many sev/tdx conditionals in common code. Neither choice is great. Another possibility is to make this a X86ConfidentialGuest method, if the TDX code has anything similar. Feel free to keep this patch, or anything that replaces it, in your TDX series. Apart from this issue, I could rebase the previous TDX patches on top of SEV-SNP without any problems. Paolo
diff --git a/target/i386/sev.c b/target/i386/sev.c index 004c667ac14..97e15f8b7a9 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -1710,7 +1710,9 @@ void sev_es_set_reset_vector(CPUState *cpu) { X86CPU *x86; CPUX86State *env; - SevCommonState *sev_common = SEV_COMMON(MACHINE(qdev_get_machine())->cgs); + ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs; + SevCommonState *sev_common = SEV_COMMON( + object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON)); /* Only update if we have valid reset information */ if (!sev_common || !sev_common->reset_data_valid) {
There can be other confidential computing classes that are not derived from sev-common. Avoid aborting when encountering them. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- target/i386/sev.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)