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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
