Re: [PATCH v3 24/49] target/i386: Add handling for KVM_X86_SNP_VM VM type

[Paolo Bonzini]

On 3/20/24 09:39, Michael Roth wrote:
> An SNP VM requires VM type KVM_X86_SNP_VM to be passed to
> kvm_ioctl(KVM_CREATE_VM). Add it to the list of supported VM types, and
> return it appropriately via X86ConfidentialGuestClass->kvm_type().
>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>   target/i386/kvm/kvm.c |  1 +
>   target/i386/sev.c     | 10 ++++++++--
>   2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index e109648f26..59e9048e61 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -164,6 +164,7 @@ static int kvm_get_one_msr(X86CPU *cpu, int index, uint64_t 
> *value);
>   
>   static const char *vm_type_name[] = {
>       [KVM_X86_DEFAULT_VM] = "default",
> +    [KVM_X86_SNP_VM] = "snp"
>   };
>   
>   bool kvm_is_vm_type_supported(int type)
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 2eb13ba639..61af312a11 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -853,14 +853,20 @@ sev_vm_state_change(void *opaque, bool running, RunState 
> state)
>   static int sev_kvm_type(X86ConfidentialGuest *cg)
>   {
>       SevCommonState *sev_common = SEV_COMMON(cg);
> -    SevGuestState *sev_guest = SEV_GUEST(sev_common);
>       int kvm_type;
>   
>       if (sev_common->kvm_type != -1) {
>           goto out;
>       }
>   
> -    kvm_type = (sev_guest->policy & SEV_POLICY_ES) ? KVM_X86_SEV_ES_VM : KVM_X86_SEV_VM;
> +    if (sev_snp_enabled()) {
> +        kvm_type = KVM_X86_SNP_VM;
> +    } else if (sev_es_enabled()) {
> +        kvm_type = KVM_X86_SEV_ES_VM;
> +    } else {
> +        kvm_type = KVM_X86_SEV_VM;
> +    }

I don't really like this, the kvm_type method can be implemented 
separately in sev-guest and sev-snp-guest.

This is for two reasons:

1) it makes sev_kvm_type self-contained, instead of poking in 
current_machine via sev_*_enabled()

2) sev-snp-guest can just return KVM_X86_SNP_VM, relying on the "vm type 
supported" checks from common target/i386/kvm/kvm.c code.  Instead, 
sev-guest has to check kvm_is_vm_type_supported() in order to support 
the legacy ioctls (and that is why my patches didn't add the SEV/SEV-ES 
types to vm_type_name[], too).

Thanks,

Paolo

> +
>       if (kvm_is_vm_type_supported(kvm_type)) {
>           sev_common->kvm_type = kvm_type;
>       } else {