On Mon, Oct 17, 2022 at 12:04 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > On Mon, Oct 17, 2022, Peter Gonda wrote: > > On Thu, Oct 6, 2022 at 12:25 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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?
