On Wed, Jan 05, 2022 at 07:40:57PM +0000, Sean Christopherson wrote:
On Wed, Jan 05, 2022, Michael Roth wrote:
On Wed, Jan 05, 2022 at 05:43:21PM +0000, Sean Christopherson wrote:
Because it uses multiple VMs, and my rough sketch only allows for a single VM to use ucall. Though I suppose we could simply keep appending to the ucall list for every VM. The requirement would then be that all VMs are of the same type, i.e. utilize the same ucall_ops.
Hmm, maybe I misread your patch. Not supporting multiple VMs was the reason I gave up on having the ucall structs allocated on-demand and went with requiring them to be passed as arguments to ucall().
I thought with your patch you had solved that by having each vm have it's own pool, via vm->ucall_list, and then mapping each pool into each guest separately via:
ucall_init(vm): ucall_list = vm->ucall_list sync_global_to_guest(ucall_list).
then as long as that ucall_init() is done *after* the guest calls kvm_vm_elf_load(), it will end up with a 'ucall_list' global that points to it's own specific vm->ucall_list. Then on the test side it doesn't matter what the 'ucall_list' global is currently set to since you have the GPA and know what vm exited.
Or am I missing something there?
Ha, that was not at all intented. But yes, it should work. I'd rather be lucky than good?
:)
Although even if that is the case, now that we're proposing doing the ucall_init() inside vm_create(), then we run the risk of a test calling kvm_vm_elf_load() after, which might clobber the guest's copy of ucall_list global if ucall_init() had since been called for another VM. But that could maybe be worked around by having whatever vm_create() variant we use also do the kvm_vm_elf_load() unconditionally as part of creation.
Will sync_global_to_guest() even work as intended if kvm_vm_elf_load() hasn't been called? If not, then sync_global_{to,from}_guest() should really assert if the test hasn't been loaded.
Yah, seems like it would get clobbered by kvm_vm_elf_load() later. And can't think of any good reason to use sync_global_to_guest() without also needing kvm_vm_elf_load() at some point, so makes sense to enforce it.
As for ucall_init(), I think the best approach would be to make kvm_vm_elf_load() a static and replace all calls with:
kvm_vm_load_guest(vm);
where its implementation is:
void kvm_vm_load_guest(struct kvm_vm *vm) { kvm_vm_elf_load(vm, program_invocation_name);
ucall_init(vm); }
The logic being that if a test creates a VM but never loads any code into the guest, e.g. kvm_create_max_vcpus, then it _can't_ make ucalls.
Makes sense. And if different ops are needed for vmgexit()/tdcall() it could be something like (if based on patches 1-5 of this series, and extending vm_guest_mode as you suggested earlier):
void kvm_vm_load_guest(struct kvm_vm *vm) {
kvm_vm_elf_load(vm, program_invocation_name);
if (vm->mode == VM_MODE_SEV) ucall_init_ops(vm, ucall_ops_pio_vmgexit); else (vm->vm_type == VM_MODE_TDX) ucall_init_ops(vm, ucall_ops_pio_tdcall); else ucall_init_ops(vm, ucall_ops_pio);
Shame we have to update all the kvm_vm_elf_load() call-sites, but they'd end up potentially breaking things if left as-is anyway.
Were you planning on sending patches for these changes, or should I incorporate your prototype and take a stab at the other changes as part of v2 of this series?