Message-ID: <CAMkAt6pvT15teuYWjz7r1vmUP5McDp76qjxQ26_oeg5mTnv5NA@mail.gmail.com>
Date:   Tue, 18 Oct 2022 08:59:40 -0600
From:   Peter Gonda <pgonda@...gle.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        marcorr@...gle.com, michael.roth@....com, thomas.lendacky@....com,
        joro@...tes.org, mizhang@...gle.com, pbonzini@...hat.com,
        andrew.jones@...ux.dev
Subject: Re: [V4 6/8] KVM: selftests: add library for creating/interacting
 with SEV guests

On Mon, Oct 17, 2022 at 2:34 PM Sean Christopherson <seanjc@...gle.com> wrote:
>
> On Mon, Oct 17, 2022, Peter Gonda wrote:
> > On Mon, Oct 17, 2022 at 12:04 PM Sean Christopherson <seanjc@...gle.com> wrote:
> > >
> > > On Mon, Oct 17, 2022, Peter Gonda wrote:
> > > > 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?
>
> Configuring the C-bit inside vm_sev_create_*() won't work (at least not well).
> The C-bit needs to be known before kvm_vm_elf_load(), i.e. can't be handled after
> __vm_create(), and needs to be tracked inside the VM, i.e. can't be handled before
> __vm_create().
>
> The proposed kvm_init_vm_address_properties() seems like the best fit since the
> C-bit (and TDX's S-bit) is stolen from GPA space, i.e. directly affects the other
> values computed in that path.
>
> As for the kvm_vm_arch allocation ugliness, when we talked off-list I didn't
> consider the need to allocate in kvm_init_vm_address_properties().  That's quite
> gross, especially since the pointer will be larger than the thing being allocated.
>
> With that in mind, adding .../include/<arch>/kvm_util.h so that "struct kvm_vm_arch"
> can be defined and referenced directly doesn't seem so bad.  Having to stub in the
> struct for the other architectures is annoying, but not the end of the world.

I'll make "struct kvm_vm_arch" a non pointer member, so adding
/include/<arch>/kvm_util.h files.

But I think we do not need VM_MODE_PXXV48_4K_SEV, see:

struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t policy, void *guest_code,
                                           struct kvm_vcpu **cpu)
{
        enum vm_guest_mode mode = VM_MODE_PXXV48_4K;
        uint64_t nr_pages = vm_nr_pages_required(mode, 1, 0);
        struct kvm_vm *vm;
        uint8_t measurement[512];
        int i;

        vm = ____vm_create(mode, nr_pages);

        kvm_sev_ioctl(vm, KVM_SEV_INIT, NULL);

        configure_sev_pte_masks(vm);

        *cpu = vm_vcpu_add(vm, 0, guest_code);
        kvm_vm_elf_load(vm, program_invocation_name);

        sev_vm_launch(vm, policy);

        /* Dump the initial measurement. A test to actually verify it
would be nice. */
        sev_vm_launch_measure(vm, measurement);
        pr_info("guest measurement: ");
        for (i = 0; i < 32; ++i)
                pr_info("%02x", measurement[i]);
        pr_info("\n");

        sev_vm_launch_finish(vm);

        return vm;
}

Powered by blists - more mailing lists