On Wed, Oct 19, 2022 at 10:34 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > On Tue, Oct 18, 2022, Peter Gonda wrote: > > On Mon, Oct 17, 2022 at 2:34 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > > > > On Mon, Oct 17, 2022, Peter Gonda wrote: > > > > 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: > > I really don't want to open code __vm_create() with a slight tweak. E.g. the > below code will be broken by Ricardo's series to add memslot0 is moved out of > ____vm_create()[1], and kinda sorta be broken again by Vishal's series to add an > arch hook to __vm_create()[2]. > > AFAICT, there is no requirement that KVM_SEV_INIT be called before computing the > C-Bit, the only requirement is that KVM_SEV_INIT is called before adding vCPUs. > > [1] https://lore.kernel.org/all/20221017195834.2295901-8-ricarkol@xxxxxxxxxx > [2] https://lore.kernel.org/all/YzsC4ibDqGh5qaP9@xxxxxxxxxx Oh I misunderstood your suggestion above. I should make KVM_SEV_INIT happen from kvm_arch_vm_post_create(). Add VM_MODE_PXXV48_4K_SEV for c-bit setting inside of kvm_init_vm_address_properties(). Inside of vm_sev_create_with_one_vcpu() I use __vm_create_with_vcpus(), then call KVM_SEV_LAUNCH_FINISH. Is that correct? > > > 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; > > }
