Re: [PATCH v2 00/24] KVM: arm64: Introduce pKVM shadow state at EL2

[Will Deacon <will@kernel.org>]

Hi Sean,

On Tue, Jul 19, 2022 at 04:11:32PM +0000, Sean Christopherson wrote:
> Apologies for the slow reply.

No problem; you've provided a tonne of insightful feedback here, so it was
worth the wait. Thanks!

> On Fri, Jul 08, 2022, Will Deacon wrote:
> > but I wanted to inherit the broader cc list so you were aware of this
> > break-away series. Sadly, I don't think beefing up the commit messages would
> > get us to a point where somebody unfamiliar with the EL2 code already could
> > give a constructive review, but we can try to expand them a bit if you
> > genuinely think it would help.
> 
> I'm not looking at it just from a review point, but also from a future readers
> perspective.  E.g. someone that looks at this changelog in isolation is going to
> have no idea what a "shadow VM" is:
> 
>   KVM: arm64: Introduce pKVM shadow VM state at EL2
> 
>   Introduce a table of shadow VM structures at EL2 and provide hypercalls
>   to the host for creating and destroying shadow VMs.
> 
> Obviously there will be some context available in surrounding patches, but if you
> avoid the "shadow" terminology and provide a bit more context, then it yields
> something like:
> 
>   KVM: arm64: Add infrastructure to create and track pKVM instances at EL2
> 
>   Introduce a global table (and lock) to track pKVM instances at EL2, and
>   provide hypercalls that can be used by the untrusted host to create and
>   destroy pKVM VMs.  pKVM VM/vCPU state is directly accessible only by the
>   trusted hypervisor (EL2).  
> 
>   Each pKVM VM is directly associated with an untrusted host KVM instance,
>   and is referenced by the host using an opaque handle.  Future patches will
>   provide hypercalls to allow the host to initialize/set/get pKVM VM/vCPU
>   state using the opaque handle.

Thanks, that's much better. I'll have to summon up the energy to go through
the others as well...

> > Perhaps we should s/shadow/hyp/ to make this a little clearer?
> 
> Or maybe just "pkvm"?

I think the "hyp" part is useful to distinguish the pkvm code running at EL2
from the pkvm code running at EL1. For example, we have a 'pkvm' member in
'struct kvm_arch' which is used by the _host_ at EL1.

So I'd say either "pkvm_hyp" or "hyp" instead of "shadow". The latter is
nice and short...

> I think that's especially viable if you do away with
> kvm_shadow_vcpu_state.  As of this series at least, kvm_shadow_vcpu_state is
> completely unnecessary.  kvm_vcpu.kvm can be used to get at the VM, and thus pKVM
> state via container_of().  Then the host_vcpu can be retrieved by using the
> vcpu_idx, e.g.
> 
> 	struct pkvm_vm *pkvm_vm = to_pkvm_vm(pkvm_vcpu->vm);
> 	struct kvm_vcpu *host_vcpu;
> 
> 	host_vcpu = kvm_get_vcpu(pkvm_vm->host_vm, pkvm_vcpu->vcpu_idx);

Using container_of() here is neat; we can definitely go ahead with that
change. However, looking at this in more detail with Fuad, removing
'struct kvm_shadow_vcpu_state' entirely isn't going to work:

> E.g. I believe you can make the code look like this:
> 
> struct kvm_arch {
> 	...
> 
> 	/*
> 	 * For an unstructed host VM, pkvm_handle is used to lookup the
> 	 * associated pKVM instance.
> 	 */
> 	pvk_handle_t pkvm_handle;
> };
> 
> struct pkvm_vm {
> 	struct kvm kvm;
> 
> 	/* Backpointer to the host's (untrusted) KVM instance. */
> 	struct kvm *host_kvm;
> 
> 	size_t donated_memory_size;
> 
> 	struct kvm_pgtable pgt;
> };
> 
> static struct kvm *pkvm_get_vm(pkvm_handle_t handle)
> {
> 	unsigned int idx = pkvm_handle_to_idx(handle);
> 
> 	if (unlikely(idx >= KVM_MAX_PVMS))
> 		return NULL;
> 
> 	return pkvm_vm_table[idx];
> }
> 
> struct kvm_vcpu *pkvm_vcpu_load(pkvm_handle_t handle, unsigned int vcpu_idx)
> {
> 	struct kvm_vpcu *pkvm_vcpu = NULL;
> 	struct kvm *vm;
> 
> 	hyp_spin_lock(&pkvm_global_lock);
> 	vm = pkvm_get_vm(handle);
> 	if (!vm || atomic_read(&vm->online_vcpus) <= vcpu_idx)
> 		goto unlock;
> 
> 	pkvm_vcpu = kvm_get_vcpu(vm, vcpu_idx);

kvm_get_vcpu() makes use of an xarray to hold the vCPUs pointers and this is
really something which we cannot support at EL2 where, amongst other things,
we do not have support for RCU. Consequently, we do need to keep our own
mapping from the shad^H^H^H^Hhyp vCPU to the host vCPU.

We also end up expanding the 'struct kvm_shadow_vcpu_state' structure later
to track additional vCPU state in the hypervisor, for example in the
mega-series:

https://lore.kernel.org/kvmarm/20220519134204.5379-78-will@kernel.org/#Z31arch:arm64:kvm:hyp:include:nvhe:pkvm.h

Cheers,

Will
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm