Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Oct 2022 18:04:28 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Peter Gonda <pgonda@google.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        marcorr@google.com, michael.roth@amd.com, thomas.lendacky@amd.com,
        joro@8bytes.org, mizhang@google.com, pbonzini@redhat.com,
        andrew.jones@linux.dev
Subject: Re: [V4 6/8] KVM: selftests: add library for creating/interacting
 with SEV guests
Message-ID: <Y02ZLFcDQbX6lP9z@google.com>
References: <20220829171021.701198-1-pgonda@google.com>
 <20220829171021.701198-7-pgonda@google.com>
 <Yz8dpB5+RFjEhA3n@google.com>
 <CAMkAt6oZQc4jqF7FOXOKkpbP3c4NXxPumVVjX9gXwPCh-zbtYg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAMkAt6oZQc4jqF7FOXOKkpbP3c4NXxPumVVjX9gXwPCh-zbtYg@mail.gmail.com>
Precedence: bulk

On Mon, Oct 17, 2022, Peter Gonda wrote:
> On Thu, Oct 6, 2022 at 12:25 PM Sean Christopherson <seanjc@google.com> wrote:
> > And with that, I believe sev_vm_create() can go away entirely and the SEV encryption
> > stuff can be handled via a new vm_guest_mode.  ____vm_create() already has a gross
> > __x86_64__ hook that we can tweak, e.g.
> >
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index 54b8d8825f5d..2d6cbca2c01a 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -238,9 +238,10 @@ struct kvm_vm *____vm_create(enum vm_guest_mode mode, uint64_t nr_pages)
> >         case VM_MODE_P36V47_16K:
> >                 vm->pgtable_levels = 3;
> >                 break;
> > +       case VM_MODE_PXXV48_4K_SEV:
> >         case VM_MODE_PXXV48_4K:
> >  #ifdef __x86_64__
> > -               kvm_get_cpu_address_width(&vm->pa_bits, &vm->va_bits);
> > +               kvm_init_vm_address_properties(vm);
> >                 /*
> >                  * Ignore KVM support for 5-level paging (vm->va_bits == 57),
> >                  * it doesn't take effect unless a CR4.LA57 is set, which it
> >
> > Then kvm_init_vm_address_properties() can pivot on vm->mode to deal with SEV
> > specific stuff.

...

> This refactor sounds good, working on this with a few changes.
> 
> Instead of kvm_init_vm_address_properties() as you suggested I've added this:
> 
> @@ -272,6 +275,8 @@ struct kvm_vm *____vm_create(enum vm_guest_mode
>  mode, uint64_t nr_pages)
>                 vm->type = KVM_VM_TYPE_ARM_IPA_SIZE(vm->pa_bits);
>  #endif
> 
> +       kvm_init_vm_arch(vm);

Why?  I'm not necessarily opposed to adding kvm_init_vm_arch(), but since x86
"needs" a dedicated hook to unpack the mode, why not piggyback that one?

> +
>         vm_open(vm);
> 
>         /* Limit to VA-bit canonical virtual addresses. */
> 
> And I need to put kvm_arch_vm_post_create() after the vCPUs are
> created because the ordering we need is: KVM_SEV_INIT -> Create vCPUS
> -> KVM_SEV_LAUNCH_FINISH.

Hrm, that's annoying.  Please don't use kvm_arch_vm_post_create() as the name,
that's a better fit for what Vishal is doing since the "vm_post_create()" implies
that it's called for "all" VM creation paths, where "all" means "everything
except barebones VMs".  E.g. in Vishal's series, kvm_arch_vm_post_create() can
be used to drop the vm_create_irqchip() call in common code.  In your case, IIUC
the hook will be invoked from __vm_create_with_vcpus().

I'm a little hesitant to have an arch hook for this case since it can't be
all-or-nothing (again, ignoring barebones VMs).  If a "finalize" arch hook is added,
then arguably tests that do __vm_create() and manually add vCPUs should call the
arch hook, i.e. we'd be adding maintenance burden to tests that in all likelihood
don't care about SEV and never will.

It's somewhat unfortunate, but dedicated vm_sev_create_with_one_vcpu() and
and vm_sev_create_with_vcpus() wrappers is probably the least awful solution.