lkml.org 
[lkml]   [2022]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [V4 6/8] KVM: selftests: add library for creating/interacting with SEV guests
    On Mon, Oct 17, 2022, Peter Gonda wrote:
    > On Mon, Oct 17, 2022 at 12:04 PM Sean Christopherson <seanjc@google.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.

    \
     
     \ /
      Last update: 2022-10-17 22:39    [W:4.115 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site