| Message ID | 20220630135747.26983-7-will@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Introduce pKVM shadow state at EL2 | expand |
Hi Will, 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. -- Thanks, Oliver
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. Does that make sense? Will
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
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, +}; int __pkvm_prot_finalize(void); int __pkvm_host_share_hyp(u64 pfn); diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c index 78edf077fa3b..10390b8dc841 100644 --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c @@ -26,8 +26,6 @@ struct host_kvm host_kvm; static struct hyp_pool host_s2_pool; -const u8 pkvm_hyp_id = 1; - static void host_lock_component(void) { hyp_spin_lock(&host_kvm.lock); @@ -384,12 +382,6 @@ void handle_host_mem_abort(struct kvm_cpu_context *host_ctxt) BUG_ON(ret && ret != -EAGAIN); } -/* This corresponds to locking order */ -enum pkvm_component_id { - PKVM_ID_HOST, - PKVM_ID_HYP, -}; - struct pkvm_mem_transition { u64 nr_pages; diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c index 8f2726d7e201..0312c9c74a5a 100644 --- a/arch/arm64/kvm/hyp/nvhe/setup.c +++ b/arch/arm64/kvm/hyp/nvhe/setup.c @@ -212,7 +212,7 @@ static int fix_host_ownership_walker(u64 addr, u64 end, u32 level, state = pkvm_getstate(kvm_pgtable_hyp_pte_prot(pte)); switch (state) { case PKVM_PAGE_OWNED: - return host_stage2_set_owner_locked(phys, PAGE_SIZE, pkvm_hyp_id); + return host_stage2_set_owner_locked(phys, PAGE_SIZE, PKVM_ID_HYP); case PKVM_PAGE_SHARED_OWNED: prot = pkvm_mkstate(PKVM_HOST_MEM_PROT, PKVM_PAGE_SHARED_BORROWED); break;
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(-)