>
> [...snip...]
>
>>
>> I still think kvm_arch_vm_finalize_vcpus() is an odd place to be
>> finalizing the VM.
>
> That's literally why the function exists though. The one and only existing
> implementation (on arm64) uses it to initialize the vGIC.
>
> void kvm_arch_vm_finalize_vcpus(struct kvm_vm *vm)
> {
> if (vm->arch.has_gic)
> __vgic_v3_init(vm->arch.gic_fd);
> }
>
> That's *very* similar to the proposed TDX usage, where some per-VM asset(s)
> can
> be initialized/frozen only after all vCPUs have been added. In other words,
> the
> name is describing where in the VM creation/setup flow the hook is called, and
> perhaps more importantly, the impact of the call: vCPUs are finalized
> (obviously
> with a different definition of "finalized" based on the VM properties).
>
>> I would prefer to not have to explicitly call some function like
>> kvm_arch_vm_finalize() (no vcpu in the name), but a common arch function
>> calling vm_sev_launch() and tdx_vm_finalize() is what I can think of
>> for test setup flexibility, without too much magic.
>
> We can't have our cake and eat it too. Either we launch/finalize SEV/TDX VMs
> as
> part of the common VM creation flows (as proposed for TDX), or we force tests
> to
> manually launch/finalize the VM after additional setup. The only way to have
> it
> both ways is to utilize crazy macro shenanigans to execute arbitrary code
> between
> creating the VM and launching/finalizing the VM, and even I would balk at
> that.
>
> I honestly don't see any reason to try to figure out which of the two
> approaches
> is optimal at this time. Whatever decision we make isn't set in stone, and in
> fact should be relative easy to change. And without being able to see all the
> TDX/SEV tests that are waiting in the wings, it's more or less impossible to
> make
> an informed decision.
>
> I definitely want to have SEV and TDX use the same core approach in the end,
> but
> I don't think we should force the issue right now, because the cost of
> reworking
> the SEV and/or TDX infrastructure when there are only a bare handful of tests
> is
> so low. It will cost more to try to predict the future of a 50/50 outcome
> than
> it will to make a completely wild guess between the two options and rework
> things
> if we guess wrong.
>
Makes sense. I'm good with merging this as it is done in this
patch. Thanks :)
>> For now, I can't think of many uses of __shared. ucall shared memory is
>> allocated dynamically, and we can also make it shared cleanly within
>> ucall code.
>
> Uh, every selftest that uses global variables to communicate between guest and
> host?
>
>> The global variables (sync_global_to_guest()) will appear in the guest
>> as long as sync_global_to_guest() is called before
>> kvm_arch_vm_finalize(), which I think makes sense to people writing
>> tests for CoCo.
>
> Yes, but that's completely orthogonal to all of this.
>
>> So 2 questions to push this along:
>>
>> 1. What do you think of a kvm_arch_vm_finalize() that calls
>> vm_sev_launch() and tdx_vm_finalize()? My key issue is that
>> kvm_arch_vm_finalize_*vcpus*() seems to be for finalizing vCPUs
>> rather than the whole VM.
>
> Key word "seems". I'm pretty sure Oliver picked kvm_arch_vm_finalize_vcpus()
> as
> the name in commit 8911c7dbc607 ("KVM: arm64: selftests: Create a VGICv3 for
> 'default' VMs") for the same reasons I think it's a good fit for coco VMs:
> like
> finalizing TDX VMs, initializing the vGIC effectively finalizes vCPUs.
>
> We could rename it to kvm_arch_vm_finalize(), but that won't change the fact
> that
> we'll need to decide between automagic vs. manual finalization, and it
> definitely
> should be a separate discussion.
>
This definitely should not block this series.
It's coming together for me now with your explanation:
kvm_arch_vm_finalize_vcpus() actually means finalizing vCPUs! vGIC ==
Virtual Generic Interrupt Controller, which has to be done after all the
vCPUs are set up. Since the name is describing where in the VM
creation/setup flow the hook is called (after creating VM and after
creating vCPUs), maybe something like kvm_arch_vm_post_vcpu_create()?
Renaming this to kvm_arch_vm_finalize() makes it sound like it is
finalizing the VM, but this function shouldn't finalize the VM since for
CoCo finalizing the VM also loads the guest image into the guest - deals
with memory, not just vCPUs.
8911c7dbc607 ("KVM: arm64: selftests: Create a VGICv3 for 'default'
VMs") also includes a test_disable_default_vgic() function, we could
also use something like that to skip CoCo VM finalization for some
tests? Maybe that's a good middle ground.
>> 3. Would you like __shared implemented together with this series, as a
>> prerequisite, or later?
>
> Only if __shared is a hard requirement for base TDX support, which I assume is
> not the case.
Yup!
