Re: [PATCH v2 06/24] KVM: arm64: Unify identifiers used to distinguish host and hypervisor

[Oliver Upton <oliver.upton@linux.dev>]

Hi Will,

Sorry, I didn't see your reply til now.

On Wed, Jul 20, 2022 at 07:14:07PM +0100, Will Deacon wrote:
> Hi Oliver,
> 
> Thanks for having a look.
> 
> On Wed, Jul 20, 2022 at 03:11:04PM +0000, Oliver Upton wrote:
> > On Thu, Jun 30, 2022 at 02:57:29PM +0100, Will Deacon wrote:
> > > The 'pkvm_component_id' enum type provides constants to refer to the
> > > host and the hypervisor, yet this information is duplicated by the
> > > 'pkvm_hyp_id' constant.
> > > 
> > > Remove the definition of 'pkvm_hyp_id' and move the 'pkvm_component_id'
> > > type definition to 'mem_protect.h' so that it can be used outside of
> > > the memory protection code.
> > > 
> > > Signed-off-by: Will Deacon <will@kernel.org>
> > > ---
> > >  arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 6 +++++-
> > >  arch/arm64/kvm/hyp/nvhe/mem_protect.c         | 8 --------
> > >  arch/arm64/kvm/hyp/nvhe/setup.c               | 2 +-
> > >  3 files changed, 6 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h b/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
> > > index 80e99836eac7..f5705a1e972f 100644
> > > --- a/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
> > > +++ b/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
> > > @@ -51,7 +51,11 @@ struct host_kvm {
> > >  };
> > >  extern struct host_kvm host_kvm;
> > >  
> > > -extern const u8 pkvm_hyp_id;
> > > +/* This corresponds to page-table locking order */
> > > +enum pkvm_component_id {
> > > +	PKVM_ID_HOST,
> > > +	PKVM_ID_HYP,
> > > +};
> > 
> > Since we have the concept of PTE ownership in pgtable.c, WDYT about
> > moving the owner ID enumeration there? KVM_MAX_OWNER_ID should be
> > incorporated in the enum too.
> 
> Interesting idea... I think we need the definition in a header file so that
> it can be used by mem_protect.c, so I'm not entirely sure where you'd like
> to see it moved.
> 
> The main worry I have is that if we ever need to distinguish e.g. one guest
> instance from another, which is likely needed for sharing of memory
> between more than just two components, then the pgtable code really cares
> about the number of instances ("which guest is it?") whilst the mem_protect
> cares about the component type ("is it a guest?").
> 
> Finally, the pgtable code is also used outside of pKVM so, although the
> concept of ownership doesn't yet apply elsewhere, keeping the concept
> available without dictacting the different types of owners makes sense to
> me.

Sorry, it was a silly suggestion to wedge the enum there. I don't think
it matters too much where it winds up, but something like:

  enum kvm_pgtable_owner_id {
  	OWNER_ID_PKVM_HOST,
	OWNER_ID_PKVM_HYP,
	NR_PGTABLE_OWNER_IDS,
  }

And put it somewhere that both pgtable.c and mem_protect.c can get at
it. That way bound checks (like in kvm_pgtable_stage2_set_owner())
organically work as new IDs are added.

--
Thanks,
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm