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;
MIME-Version: 1.0
References: <20220829171021.701198-1-pgonda@google.com> <20220829171021.701198-7-pgonda@google.com>
 <Yz8dpB5+RFjEhA3n@google.com> <CAMkAt6oZQc4jqF7FOXOKkpbP3c4NXxPumVVjX9gXwPCh-zbtYg@mail.gmail.com>
 <Y02ZLFcDQbX6lP9z@google.com>
In-Reply-To: <Y02ZLFcDQbX6lP9z@google.com>
From:   Peter Gonda <pgonda@google.com>
Date:   Mon, 17 Oct 2022 12:25:32 -0600
Message-ID: <CAMkAt6q0g5Ua=PwLXa2oA4zCQUaHuEQ3pTXycD61HU6-dtQ5Gg@mail.gmail.com>
Subject: Re: [V4 6/8] KVM: selftests: add library for creating/interacting
 with SEV guests
To:     Sean Christopherson <seanjc@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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Oct 17, 2022 at 12:04 PM Sean Christopherson <seanjc@google.com> wrote:
>
> 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?
>

Well I since I need to do more than just
kvm_init_vm_address_properties() I thought the more generic name would
be better. We need to allocate kvm_vm_arch, find the c-bit, and call
KVM_SEV_INIT. I can put it back in that switch case if thats better,
thoughts?

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

Make sense. I think we can go back to your suggestion of
kvm_init_vm_address_properties() above since we can now do all the
KVM_SEV_* stuff. I think this means we don't need to add
VM_MODE_PXXV48_4K_SEV since we can set up the c-bit from inside of
vm_sev_create_*(), thoughts?