Re: [RFC PATCH 2/4] i386/sev: Switch to use confidential_guest_kvm_init()

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 2/6/2024 10:16 PM, Daniel P. Berrangé wrote:
> On Tue, Feb 06, 2024 at 03:28:50AM -0500, Xiaoyao Li wrote:
>> Use confidential_guest_kvm_init() instead of calling SEV specific
>> sev_kvm_init(). As a bouns, it fits to future TDX when TDX implements
>> its own confidential_guest_support and .kvm_init().
>>
>> Move the "TypeInfo sev_guest_info" definition and related functions to
>> the end of the file, to avoid declaring the sev_kvm_init() ahead.
>>
>> Clean up the sve-stub.c since it's not needed anymore.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   target/i386/kvm/kvm.c       |   2 +-
>>   target/i386/kvm/meson.build |   2 -
>>   target/i386/kvm/sev-stub.c  |   5 --
>>   target/i386/sev.c           | 120 +++++++++++++++++++-----------------
>>   target/i386/sev.h           |   2 -
>>   5 files changed, 63 insertions(+), 68 deletions(-)
>>
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index 76a66246eb72..bb63bba61fa1 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -2534,7 +2534,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>        * mechanisms are supported in future (e.g. TDX), they'll need
>>        * their own initialization either here or elsewhere.
>>        */
>> -    ret = sev_kvm_init(ms->cgs, &local_err);
>> +    ret = confidential_guest_kvm_init(ms->cgs, &local_err);
> 
> If you agree with my comment in patch 1 about the API expecting non-NULL,
> then this would need to be conditionalized (same for the 2 following
> patches too)

sure. Will change.

>     if (ms->cgs) {
>        ret = confidential_guest_kvm_init(....)
>        if (ret < 0) {
>           ....
>        }
>     }
> 
>>       if (ret < 0) {
>>           error_report_err(local_err);
>>           return ret;
>> diff --git a/target/i386/kvm/meson.build b/target/i386/kvm/meson.build
>> index 84d9143e6029..e7850981e62d 100644
>> --- a/target/i386/kvm/meson.build
>> +++ b/target/i386/kvm/meson.build
>> @@ -7,8 +7,6 @@ i386_kvm_ss.add(files(
>>   
>>   i386_kvm_ss.add(when: 'CONFIG_XEN_EMU', if_true: files('xen-emu.c'))
>>   
>> -i386_kvm_ss.add(when: 'CONFIG_SEV', if_false: files('sev-stub.c'))
>> -
>>   i386_system_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'), if_false: files('hyperv-stub.c'))
>>   
>>   i386_system_ss.add_all(when: 'CONFIG_KVM', if_true: i386_kvm_ss)
>> diff --git a/target/i386/kvm/sev-stub.c b/target/i386/kvm/sev-stub.c
>> index 1be5341e8a6a..4a1560cf8ad7 100644
>> --- a/target/i386/kvm/sev-stub.c
>> +++ b/target/i386/kvm/sev-stub.c
>> @@ -14,8 +14,3 @@
>>   #include "qemu/osdep.h"
>>   #include "sev.h"
>>   
>> -int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>> -{
>> -    /* If we get here, cgs must be some non-SEV thing */
>> -    return 0;
>> -}
> 
> You can actually delete this entire file, since you removed the
> only method in it, and stopped building it in the meson.build
> patch above.

I intented to do it. Apprarently I missed it somehow and didn't catch it 
before sending out.

will fix in next version.

>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>> index 173de91afe7d..19e79d3631d0 100644
>> --- a/target/i386/sev.c
>> +++ b/target/i386/sev.c
>> @@ -353,63 +353,6 @@ static void sev_guest_set_kernel_hashes(Object *obj, bool value, Error **errp)
>>       sev->kernel_hashes = value;
>>   }
>>   
>> -static void
>> -sev_guest_class_init(ObjectClass *oc, void *data)
>> -{
>> -    object_class_property_add_str(oc, "sev-device",
>> -                                  sev_guest_get_sev_device,
>> -                                  sev_guest_set_sev_device);
>> -    object_class_property_set_description(oc, "sev-device",
>> -            "SEV device to use");
>> -    object_class_property_add_str(oc, "dh-cert-file",
>> -                                  sev_guest_get_dh_cert_file,
>> -                                  sev_guest_set_dh_cert_file);
>> -    object_class_property_set_description(oc, "dh-cert-file",
>> -            "guest owners DH certificate (encoded with base64)");
>> -    object_class_property_add_str(oc, "session-file",
>> -                                  sev_guest_get_session_file,
>> -                                  sev_guest_set_session_file);
>> -    object_class_property_set_description(oc, "session-file",
>> -            "guest owners session parameters (encoded with base64)");
>> -    object_class_property_add_bool(oc, "kernel-hashes",
>> -                                   sev_guest_get_kernel_hashes,
>> -                                   sev_guest_set_kernel_hashes);
>> -    object_class_property_set_description(oc, "kernel-hashes",
>> -            "add kernel hashes to guest firmware for measured Linux boot");
>> -}
>> -
>> -static void
>> -sev_guest_instance_init(Object *obj)
>> -{
>> -    SevGuestState *sev = SEV_GUEST(obj);
>> -
>> -    sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
>> -    sev->policy = DEFAULT_GUEST_POLICY;
>> -    object_property_add_uint32_ptr(obj, "policy", &sev->policy,
>> -                                   OBJ_PROP_FLAG_READWRITE);
>> -    object_property_add_uint32_ptr(obj, "handle", &sev->handle,
>> -                                   OBJ_PROP_FLAG_READWRITE);
>> -    object_property_add_uint32_ptr(obj, "cbitpos", &sev->cbitpos,
>> -                                   OBJ_PROP_FLAG_READWRITE);
>> -    object_property_add_uint32_ptr(obj, "reduced-phys-bits",
>> -                                   &sev->reduced_phys_bits,
>> -                                   OBJ_PROP_FLAG_READWRITE);
>> -}
>> -
>> -/* sev guest info */
>> -static const TypeInfo sev_guest_info = {
>> -    .parent = TYPE_CONFIDENTIAL_GUEST_SUPPORT,
>> -    .name = TYPE_SEV_GUEST,
>> -    .instance_size = sizeof(SevGuestState),
>> -    .instance_finalize = sev_guest_finalize,
>> -    .class_init = sev_guest_class_init,
>> -    .instance_init = sev_guest_instance_init,
>> -    .interfaces = (InterfaceInfo[]) {
>> -        { TYPE_USER_CREATABLE },
>> -        { }
>> -    }
>> -};
>> -
>>   bool
>>   sev_enabled(void)
>>   {
>> @@ -906,7 +849,7 @@ sev_vm_state_change(void *opaque, bool running, RunState state)
>>       }
>>   }
>>   
>> -int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>> +static int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>>   {
>>       SevGuestState *sev
>>           = (SevGuestState *)object_dynamic_cast(OBJECT(cgs), TYPE_SEV_GUEST);
>> @@ -1383,6 +1326,67 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
>>       return ret;
>>   }
>>   
>> +static void
>> +sev_guest_class_init(ObjectClass *oc, void *data)
>> +{
>> +    ConfidentialGuestSupportClass *klass = CONFIDENTIAL_GUEST_SUPPORT_CLASS(oc);
>> +
>> +    klass->kvm_init = sev_kvm_init;
>> +
>> +    object_class_property_add_str(oc, "sev-device",
>> +                                  sev_guest_get_sev_device,
>> +                                  sev_guest_set_sev_device);
>> +    object_class_property_set_description(oc, "sev-device",
>> +            "SEV device to use");
>> +    object_class_property_add_str(oc, "dh-cert-file",
>> +                                  sev_guest_get_dh_cert_file,
>> +                                  sev_guest_set_dh_cert_file);
>> +    object_class_property_set_description(oc, "dh-cert-file",
>> +            "guest owners DH certificate (encoded with base64)");
>> +    object_class_property_add_str(oc, "session-file",
>> +                                  sev_guest_get_session_file,
>> +                                  sev_guest_set_session_file);
>> +    object_class_property_set_description(oc, "session-file",
>> +            "guest owners session parameters (encoded with base64)");
>> +    object_class_property_add_bool(oc, "kernel-hashes",
>> +                                   sev_guest_get_kernel_hashes,
>> +                                   sev_guest_set_kernel_hashes);
>> +    object_class_property_set_description(oc, "kernel-hashes",
>> +            "add kernel hashes to guest firmware for measured Linux boot");
>> +}
>> +
>> +static void
>> +sev_guest_instance_init(Object *obj)
>> +{
>> +    SevGuestState *sev = SEV_GUEST(obj);
>> +
>> +    sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
>> +    sev->policy = DEFAULT_GUEST_POLICY;
>> +    object_property_add_uint32_ptr(obj, "policy", &sev->policy,
>> +                                   OBJ_PROP_FLAG_READWRITE);
>> +    object_property_add_uint32_ptr(obj, "handle", &sev->handle,
>> +                                   OBJ_PROP_FLAG_READWRITE);
>> +    object_property_add_uint32_ptr(obj, "cbitpos", &sev->cbitpos,
>> +                                   OBJ_PROP_FLAG_READWRITE);
>> +    object_property_add_uint32_ptr(obj, "reduced-phys-bits",
>> +                                   &sev->reduced_phys_bits,
>> +                                   OBJ_PROP_FLAG_READWRITE);
>> +}
>> +
>> +/* sev guest info */
>> +static const TypeInfo sev_guest_info = {
>> +    .parent = TYPE_CONFIDENTIAL_GUEST_SUPPORT,
>> +    .name = TYPE_SEV_GUEST,
>> +    .instance_size = sizeof(SevGuestState),
>> +    .instance_finalize = sev_guest_finalize,
>> +    .class_init = sev_guest_class_init,
>> +    .instance_init = sev_guest_instance_init,
>> +    .interfaces = (InterfaceInfo[]) {
>> +        { TYPE_USER_CREATABLE },
>> +        { }
>> +    }
>> +};
>> +
>>   static void
>>   sev_register_types(void)
>>   {
>> diff --git a/target/i386/sev.h b/target/i386/sev.h
>> index e7499c95b1e8..9e10d09539a7 100644
>> --- a/target/i386/sev.h
>> +++ b/target/i386/sev.h
>> @@ -57,6 +57,4 @@ int sev_inject_launch_secret(const char *hdr, const char *secret,
>>   int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size);
>>   void sev_es_set_reset_vector(CPUState *cpu);
>>   
>> -int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
>> -
>>   #endif
>> -- 
>> 2.34.1
>>
> 
> With regards,
> Daniel