target/i386: SEV: do not assume machine->cgs is SEV

[PATCH] target/i386: SEV: do not assume machine->cgs is SEV

Posted by Paolo Bonzini 5 months, 3 weeks ago

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));
 
     /* Only update if we have valid reset information */
     if (!sev_common || !sev_common->reset_data_valid) {
-- 
2.45.1

Re: [PATCH] target/i386: SEV: do not assume machine->cgs is SEV

Posted by Xiaoyao Li 5 months, 3 weeks ago

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) {

Re: [PATCH] target/i386: SEV: do not assume machine->cgs is SEV

Posted by Paolo Bonzini 5 months, 3 weeks ago

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

Re: [PATCH] target/i386: SEV: do not assume machine->cgs is SEV

Posted by Zhao Liu 5 months, 3 weeks ago

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
> 
>

Re: [PATCH] target/i386: SEV: do not assume machine->cgs is SEV

Posted by Richard Henderson 5 months, 3 weeks ago

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~

Re: [PATCH] target/i386: SEV: do not assume machine->cgs is SEV

Posted by Paolo Bonzini 5 months, 3 weeks ago

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~
>
>