[PATCH 0/6] Move pKVM ownership state to hyp_vmemmap

[PATCH 0/6] Move pKVM ownership state to hyp_vmemmap

[Quentin Perret]

Hi all,

This series moves the hypervisor's ownership state to the hyp_vmemmap,
as discussed in [1]. The two main benefits are:

 1. much cheaper hyp state lookups, since we can avoid the hyp stage-1
    page-table walk;

 2. de-correlates the hyp state from the presence of a mapping in the
    linear map range of the hypervisor; which enables a bunch of
    clean-ups in the existing code and will simplify the introduction of
    other features in the future (hyp tracing, ...)

Patch 01 is a self-sufficient cleanup that I found thanks to patch 05.
Patches 02-04 implement the aforementioned migration of the hyp state
to the vmemmap. Patches 05 and 06 are cleanups enabled by that
migration.

Patches based on 6.14-rc4, tested on Qemu.

Thanks!
Quentin

[1] https://lore.kernel.org/kvmarm/Z79ZJVOHtNu6YsVt@google.com/

Fuad Tabba (1):
  KVM: arm64: Track SVE state in the hypervisor vcpu structure

Quentin Perret (5):
  KVM: arm64: Use 0b11 for encoding PKVM_NOPAGE
  KVM: arm64: Introduce {get,set}_host_state() helpers
  KVM: arm64: Move hyp state to hyp_vmemmap
  KVM: arm64: Defer EL2 stage-1 mapping on share
  KVM: arm64: Unconditionally cross check hyp state

 arch/arm64/include/asm/kvm_host.h        |  12 +--
 arch/arm64/kvm/hyp/include/nvhe/memory.h |  35 ++++++--
 arch/arm64/kvm/hyp/nvhe/hyp-main.c       |   4 -
 arch/arm64/kvm/hyp/nvhe/mem_protect.c    | 106 ++++++++++++-----------
 arch/arm64/kvm/hyp/nvhe/pkvm.c           |  54 ++++++++++--
 arch/arm64/kvm/hyp/nvhe/setup.c          |  10 ++-
 6 files changed, 147 insertions(+), 74 deletions(-)

-- 
2.48.1.658.g4767266eb4-goog

[PATCH 1/6] KVM: arm64: Track SVE state in the hypervisor vcpu structure

[Quentin Perret]

From: Fuad Tabba <tabba@google.com>

When dealing with a guest with SVE enabled, make sure the host SVE
state is pinned at EL2 S1, and that the hypervisor vCPU state is
correctly initialised (and then unpinned on teardown).

Co-authored-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Fuad Tabba <tabba@google.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Quentin Perret <qperret@google.com>
---
 arch/arm64/include/asm/kvm_host.h  | 12 ++++---
 arch/arm64/kvm/hyp/nvhe/hyp-main.c |  4 ---
 arch/arm64/kvm/hyp/nvhe/pkvm.c     | 54 +++++++++++++++++++++++++++---
 3 files changed, 56 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 3a7ec98ef123..90b58f87b107 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -930,20 +930,22 @@ struct kvm_vcpu_arch {
 #define vcpu_sve_zcr_elx(vcpu)						\
 	(unlikely(is_hyp_ctxt(vcpu)) ? ZCR_EL2 : ZCR_EL1)
 
-#define vcpu_sve_state_size(vcpu) ({					\
+#define sve_state_size_from_vl(sve_max_vl) ({				\
 	size_t __size_ret;						\
-	unsigned int __vcpu_vq;						\
+	unsigned int __vq;						\
 									\
-	if (WARN_ON(!sve_vl_valid((vcpu)->arch.sve_max_vl))) {		\
+	if (WARN_ON(!sve_vl_valid(sve_max_vl))) {			\
 		__size_ret = 0;						\
 	} else {							\
-		__vcpu_vq = vcpu_sve_max_vq(vcpu);			\
-		__size_ret = SVE_SIG_REGS_SIZE(__vcpu_vq);		\
+		__vq = sve_vq_from_vl(sve_max_vl);			\
+		__size_ret = SVE_SIG_REGS_SIZE(__vq);			\
 	}								\
 									\
 	__size_ret;							\
 })
 
+#define vcpu_sve_state_size(vcpu) sve_state_size_from_vl((vcpu)->arch.sve_max_vl)
+
 #define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE | \
 				 KVM_GUESTDBG_USE_SW_BP | \
 				 KVM_GUESTDBG_USE_HW | \
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index 2c37680d954c..59db9606e6e1 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -123,10 +123,6 @@ static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
 
 	hyp_vcpu->vcpu.arch.ctxt	= host_vcpu->arch.ctxt;
 
-	hyp_vcpu->vcpu.arch.sve_state	= kern_hyp_va(host_vcpu->arch.sve_state);
-	/* Limit guest vector length to the maximum supported by the host.  */
-	hyp_vcpu->vcpu.arch.sve_max_vl	= min(host_vcpu->arch.sve_max_vl, kvm_host_sve_max_vl);
-
 	hyp_vcpu->vcpu.arch.mdcr_el2	= host_vcpu->arch.mdcr_el2;
 	hyp_vcpu->vcpu.arch.hcr_el2 &= ~(HCR_TWI | HCR_TWE);
 	hyp_vcpu->vcpu.arch.hcr_el2 |= READ_ONCE(host_vcpu->arch.hcr_el2) &
diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
index 3927fe52a3dd..3ec27e12b043 100644
--- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
+++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c
@@ -356,13 +356,29 @@ static void unpin_host_vcpu(struct kvm_vcpu *host_vcpu)
 		hyp_unpin_shared_mem(host_vcpu, host_vcpu + 1);
 }
 
+static void unpin_host_sve_state(struct pkvm_hyp_vcpu *hyp_vcpu)
+{
+	void *sve_state;
+
+	if (!vcpu_has_feature(&hyp_vcpu->vcpu, KVM_ARM_VCPU_SVE))
+		return;
+
+	sve_state = kern_hyp_va(hyp_vcpu->vcpu.arch.sve_state);
+	hyp_unpin_shared_mem(sve_state,
+			     sve_state + vcpu_sve_state_size(&hyp_vcpu->vcpu));
+}
+
 static void unpin_host_vcpus(struct pkvm_hyp_vcpu *hyp_vcpus[],
 			     unsigned int nr_vcpus)
 {
 	int i;
 
-	for (i = 0; i < nr_vcpus; i++)
-		unpin_host_vcpu(hyp_vcpus[i]->host_vcpu);
+	for (i = 0; i < nr_vcpus; i++) {
+		struct pkvm_hyp_vcpu *hyp_vcpu = hyp_vcpus[i];
+
+		unpin_host_vcpu(hyp_vcpu->host_vcpu);
+		unpin_host_sve_state(hyp_vcpu);
+	}
 }
 
 static void init_pkvm_hyp_vm(struct kvm *host_kvm, struct pkvm_hyp_vm *hyp_vm,
@@ -376,12 +392,40 @@ static void init_pkvm_hyp_vm(struct kvm *host_kvm, struct pkvm_hyp_vm *hyp_vm,
 	pkvm_init_features_from_host(hyp_vm, host_kvm);
 }
 
-static void pkvm_vcpu_init_sve(struct pkvm_hyp_vcpu *hyp_vcpu, struct kvm_vcpu *host_vcpu)
+static int pkvm_vcpu_init_sve(struct pkvm_hyp_vcpu *hyp_vcpu, struct kvm_vcpu *host_vcpu)
 {
 	struct kvm_vcpu *vcpu = &hyp_vcpu->vcpu;
+	unsigned int sve_max_vl;
+	size_t sve_state_size;
+	void *sve_state;
+	int ret = 0;
 
-	if (!vcpu_has_feature(vcpu, KVM_ARM_VCPU_SVE))
+	if (!vcpu_has_feature(vcpu, KVM_ARM_VCPU_SVE)) {
 		vcpu_clear_flag(vcpu, VCPU_SVE_FINALIZED);
+		return 0;
+	}
+
+	/* Limit guest vector length to the maximum supported by the host. */
+	sve_max_vl = min(READ_ONCE(host_vcpu->arch.sve_max_vl), kvm_host_sve_max_vl);
+	sve_state_size = sve_state_size_from_vl(sve_max_vl);
+	sve_state = kern_hyp_va(READ_ONCE(host_vcpu->arch.sve_state));
+
+	if (!sve_state || !sve_state_size) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	ret = hyp_pin_shared_mem(sve_state, sve_state + sve_state_size);
+	if (ret)
+		goto err;
+
+	vcpu->arch.sve_state = sve_state;
+	vcpu->arch.sve_max_vl = sve_max_vl;
+
+	return 0;
+err:
+	clear_bit(KVM_ARM_VCPU_SVE, vcpu->kvm->arch.vcpu_features);
+	return ret;
 }
 
 static int init_pkvm_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu,
@@ -416,7 +460,7 @@ static int init_pkvm_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu,
 	if (ret)
 		goto done;
 
-	pkvm_vcpu_init_sve(hyp_vcpu, host_vcpu);
+	ret = pkvm_vcpu_init_sve(hyp_vcpu, host_vcpu);
 done:
 	if (ret)
 		unpin_host_vcpu(host_vcpu);
-- 
2.48.1.658.g4767266eb4-goog

[PATCH 2/6] KVM: arm64: Use 0b11 for encoding PKVM_NOPAGE

[Quentin Perret]

The page ownership state encoded as 0b11 is currently considered
reserved for future use, and PKVM_NOPAGE uses bit 2. In order to
simplify the relocation of the hyp ownership state into the
vmemmap in later patches, let's use the 'reserved' encoding for
the PKVM_NOPAGE state. The struct hyp_page layout isn't guaranteed
stable at all, so there is no real reason to have 'reserved' encodings.

No functional changes intended.

Signed-off-by: Quentin Perret <qperret@google.com>
---
 arch/arm64/kvm/hyp/include/nvhe/memory.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
index 34233d586060..642b5e05fe77 100644
--- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
+++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
@@ -13,18 +13,15 @@
  *   01: The page is owned by the page-table owner, but is shared
  *       with another entity.
  *   10: The page is shared with, but not owned by the page-table owner.
- *   11: Reserved for future use (lending).
  */
 enum pkvm_page_state {
 	PKVM_PAGE_OWNED			= 0ULL,
 	PKVM_PAGE_SHARED_OWNED		= BIT(0),
 	PKVM_PAGE_SHARED_BORROWED	= BIT(1),
-	__PKVM_PAGE_RESERVED		= BIT(0) | BIT(1),
 
 	/* Meta-states which aren't encoded directly in the PTE's SW bits */
-	PKVM_NOPAGE			= BIT(2),
+	PKVM_NOPAGE			= BIT(0) | BIT(1),
 };
-#define PKVM_PAGE_META_STATES_MASK	(~__PKVM_PAGE_RESERVED)
 
 #define PKVM_PAGE_STATE_PROT_MASK	(KVM_PGTABLE_PROT_SW0 | KVM_PGTABLE_PROT_SW1)
 static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
-- 
2.48.1.658.g4767266eb4-goog

[PATCH 3/6] KVM: arm64: Introduce {get,set}_host_state() helpers

[Quentin Perret]

Instead of directly accessing the host_state member in struct hyp_page,
introduce static inline accessors to do it. The future hyp_state member
will follow the same pattern as it will need some logic in the accessors.

Signed-off-by: Quentin Perret <qperret@google.com>
---
 arch/arm64/kvm/hyp/include/nvhe/memory.h | 12 +++++++++++-
 arch/arm64/kvm/hyp/nvhe/mem_protect.c    | 14 +++++++-------
 arch/arm64/kvm/hyp/nvhe/setup.c          |  4 ++--
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
index 642b5e05fe77..4a3c55d26ef3 100644
--- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
+++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
@@ -42,7 +42,7 @@ struct hyp_page {
 	u8 order;
 
 	/* Host (non-meta) state. Guarded by the host stage-2 lock. */
-	enum pkvm_page_state host_state : 8;
+	unsigned __host_state : 8;
 
 	u32 host_share_guest_count;
 };
@@ -79,6 +79,16 @@ static inline struct hyp_page *hyp_phys_to_page(phys_addr_t phys)
 #define hyp_page_to_virt(page)	__hyp_va(hyp_page_to_phys(page))
 #define hyp_page_to_pool(page)	(((struct hyp_page *)page)->pool)
 
+static inline enum pkvm_page_state get_host_state(phys_addr_t phys)
+{
+	return (enum pkvm_page_state)hyp_phys_to_page(phys)->__host_state;
+}
+
+static inline void set_host_state(phys_addr_t phys, enum pkvm_page_state state)
+{
+	hyp_phys_to_page(phys)->__host_state = state;
+}
+
 /*
  * Refcounting for 'struct hyp_page'.
  * hyp_pool::lock must be held if atomic access to the refcount is required.
diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
index 19c3c631708c..a45ffdec7612 100644
--- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
+++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
@@ -467,7 +467,7 @@ static int host_stage2_adjust_range(u64 addr, struct kvm_mem_range *range)
 		return -EAGAIN;
 
 	if (pte) {
-		WARN_ON(addr_is_memory(addr) && hyp_phys_to_page(addr)->host_state != PKVM_NOPAGE);
+		WARN_ON(addr_is_memory(addr) && get_host_state(addr) != PKVM_NOPAGE);
 		return -EPERM;
 	}
 
@@ -496,7 +496,7 @@ static void __host_update_page_state(phys_addr_t addr, u64 size, enum pkvm_page_
 	phys_addr_t end = addr + size;
 
 	for (; addr < end; addr += PAGE_SIZE)
-		hyp_phys_to_page(addr)->host_state = state;
+		set_host_state(addr, state);
 }
 
 int host_stage2_set_owner_locked(phys_addr_t addr, u64 size, u8 owner_id)
@@ -620,7 +620,7 @@ static int __host_check_page_state_range(u64 addr, u64 size,
 
 	hyp_assert_lock_held(&host_mmu.lock);
 	for (; addr < end; addr += PAGE_SIZE) {
-		if (hyp_phys_to_page(addr)->host_state != state)
+		if (get_host_state(addr) != state)
 			return -EPERM;
 	}
 
@@ -630,7 +630,7 @@ static int __host_check_page_state_range(u64 addr, u64 size,
 static int __host_set_page_state_range(u64 addr, u64 size,
 				       enum pkvm_page_state state)
 {
-	if (hyp_phys_to_page(addr)->host_state == PKVM_NOPAGE) {
+	if (get_host_state(addr) == PKVM_NOPAGE) {
 		int ret = host_stage2_idmap_locked(addr, size, PKVM_HOST_MEM_PROT);
 
 		if (ret)
@@ -904,7 +904,7 @@ int __pkvm_host_share_guest(u64 pfn, u64 gfn, struct pkvm_hyp_vcpu *vcpu,
 		goto unlock;
 
 	page = hyp_phys_to_page(phys);
-	switch (page->host_state) {
+	switch (get_host_state(phys)) {
 	case PKVM_PAGE_OWNED:
 		WARN_ON(__host_set_page_state_range(phys, PAGE_SIZE, PKVM_PAGE_SHARED_OWNED));
 		break;
@@ -957,9 +957,9 @@ static int __check_host_shared_guest(struct pkvm_hyp_vm *vm, u64 *__phys, u64 ip
 	if (WARN_ON(ret))
 		return ret;
 
-	page = hyp_phys_to_page(phys);
-	if (page->host_state != PKVM_PAGE_SHARED_OWNED)
+	if (get_host_state(phys) != PKVM_PAGE_SHARED_OWNED)
 		return -EPERM;
+	page = hyp_phys_to_page(phys);
 	if (WARN_ON(!page->host_share_guest_count))
 		return -EINVAL;
 
diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
index d62bcb5634a2..1a414288fe8c 100644
--- a/arch/arm64/kvm/hyp/nvhe/setup.c
+++ b/arch/arm64/kvm/hyp/nvhe/setup.c
@@ -201,10 +201,10 @@ static int fix_host_ownership_walker(const struct kvm_pgtable_visit_ctx *ctx,
 	case PKVM_PAGE_OWNED:
 		return host_stage2_set_owner_locked(phys, PAGE_SIZE, PKVM_ID_HYP);
 	case PKVM_PAGE_SHARED_OWNED:
-		hyp_phys_to_page(phys)->host_state = PKVM_PAGE_SHARED_BORROWED;
+		set_host_state(phys, PKVM_PAGE_SHARED_BORROWED);
 		break;
 	case PKVM_PAGE_SHARED_BORROWED:
-		hyp_phys_to_page(phys)->host_state = PKVM_PAGE_SHARED_OWNED;
+		set_host_state(phys, PKVM_PAGE_SHARED_OWNED);
 		break;
 	default:
 		return -EINVAL;
-- 
2.48.1.658.g4767266eb4-goog

[PATCH 4/6] KVM: arm64: Move hyp state to hyp_vmemmap

[Quentin Perret]

Tracking the hypervisor's ownership state into struct hyp_page has
several benefits, including allowing far more efficient lookups (no
page-table walk needed) and de-corelating the state from the presence
of a mapping. This will later allow to map pages into EL2 stage-1 less
proactively which is generally a good thing for security. And in the
future this will help with tracking the state of pages mapped into the
hypervisor's private range without requiring an alias into the 'linear
map' range.

Signed-off-by: Quentin Perret <qperret@google.com>
---
 arch/arm64/kvm/hyp/include/nvhe/memory.h | 20 +++++++++-
 arch/arm64/kvm/hyp/nvhe/mem_protect.c    | 51 ++++++++++++------------
 arch/arm64/kvm/hyp/nvhe/setup.c          |  6 ++-
 3 files changed, 49 insertions(+), 28 deletions(-)

diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
index 4a3c55d26ef3..cc4c01158368 100644
--- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
+++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
@@ -22,6 +22,7 @@ enum pkvm_page_state {
 	/* Meta-states which aren't encoded directly in the PTE's SW bits */
 	PKVM_NOPAGE			= BIT(0) | BIT(1),
 };
+#define PKVM_PAGE_STATE_MASK		(BIT(0) | BIT(1))
 
 #define PKVM_PAGE_STATE_PROT_MASK	(KVM_PGTABLE_PROT_SW0 | KVM_PGTABLE_PROT_SW1)
 static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
@@ -42,7 +43,14 @@ struct hyp_page {
 	u8 order;
 
 	/* Host (non-meta) state. Guarded by the host stage-2 lock. */
-	unsigned __host_state : 8;
+	unsigned __host_state : 4;
+
+	/*
+	 * Complement of the hyp (non-meta) state. Guarded by the hyp stage-1 lock. We use the
+	 * complement so that the initial 0 in __hyp_state_comp (due to the entire vmemmap starting
+	 * off zeroed) encodes PKVM_NOPAGE.
+	 */
+	unsigned __hyp_state_comp : 4;
 
 	u32 host_share_guest_count;
 };
@@ -89,6 +97,16 @@ static inline void set_host_state(phys_addr_t phys, enum pkvm_page_state state)
 	hyp_phys_to_page(phys)->__host_state = state;
 }
 
+static inline enum pkvm_page_state get_hyp_state(phys_addr_t phys)
+{
+	return hyp_phys_to_page(phys)->__hyp_state_comp ^ PKVM_PAGE_STATE_MASK;
+}
+
+static inline void set_hyp_state(phys_addr_t phys, enum pkvm_page_state state)
+{
+	hyp_phys_to_page(phys)->__hyp_state_comp = state ^ PKVM_PAGE_STATE_MASK;
+}
+
 /*
  * Refcounting for 'struct hyp_page'.
  * hyp_pool::lock must be held if atomic access to the refcount is required.
diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
index a45ffdec7612..3ab8c81500c2 100644
--- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
+++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
@@ -642,24 +642,24 @@ static int __host_set_page_state_range(u64 addr, u64 size,
 	return 0;
 }
 
-static enum pkvm_page_state hyp_get_page_state(kvm_pte_t pte, u64 addr)
+static void __hyp_set_page_state_range(phys_addr_t phys, u64 size, enum pkvm_page_state state)
 {
-	if (!kvm_pte_valid(pte))
-		return PKVM_NOPAGE;
+	phys_addr_t end = phys + size;
 
-	return pkvm_getstate(kvm_pgtable_hyp_pte_prot(pte));
+	for (; phys < end; phys += PAGE_SIZE)
+		set_hyp_state(phys, state);
 }
 
-static int __hyp_check_page_state_range(u64 addr, u64 size,
-					enum pkvm_page_state state)
+static int __hyp_check_page_state_range(phys_addr_t phys, u64 size, enum pkvm_page_state state)
 {
-	struct check_walk_data d = {
-		.desired	= state,
-		.get_page_state	= hyp_get_page_state,
-	};
+	phys_addr_t end = phys + size;
+
+	for (; phys < end; phys += PAGE_SIZE) {
+		if (get_hyp_state(phys) != state)
+			return -EPERM;
+	}
 
-	hyp_assert_lock_held(&pkvm_pgd_lock);
-	return check_page_state_range(&pkvm_pgtable, addr, size, &d);
+	return 0;
 }
 
 static enum pkvm_page_state guest_get_page_state(kvm_pte_t pte, u64 addr)
@@ -687,7 +687,6 @@ int __pkvm_host_share_hyp(u64 pfn)
 {
 	u64 phys = hyp_pfn_to_phys(pfn);
 	void *virt = __hyp_va(phys);
-	enum kvm_pgtable_prot prot;
 	u64 size = PAGE_SIZE;
 	int ret;
 
@@ -698,13 +697,13 @@ int __pkvm_host_share_hyp(u64 pfn)
 	if (ret)
 		goto unlock;
 	if (IS_ENABLED(CONFIG_NVHE_EL2_DEBUG)) {
-		ret = __hyp_check_page_state_range((u64)virt, size, PKVM_NOPAGE);
+		ret = __hyp_check_page_state_range(phys, size, PKVM_NOPAGE);
 		if (ret)
 			goto unlock;
 	}
 
-	prot = pkvm_mkstate(PAGE_HYP, PKVM_PAGE_SHARED_BORROWED);
-	WARN_ON(pkvm_create_mappings_locked(virt, virt + size, prot));
+	__hyp_set_page_state_range(phys, size, PKVM_PAGE_SHARED_BORROWED);
+	WARN_ON(pkvm_create_mappings_locked(virt, virt + size, PAGE_HYP));
 	WARN_ON(__host_set_page_state_range(phys, size, PKVM_PAGE_SHARED_OWNED));
 
 unlock:
@@ -727,7 +726,7 @@ int __pkvm_host_unshare_hyp(u64 pfn)
 	ret = __host_check_page_state_range(phys, size, PKVM_PAGE_SHARED_OWNED);
 	if (ret)
 		goto unlock;
-	ret = __hyp_check_page_state_range(virt, size, PKVM_PAGE_SHARED_BORROWED);
+	ret = __hyp_check_page_state_range(phys, size, PKVM_PAGE_SHARED_BORROWED);
 	if (ret)
 		goto unlock;
 	if (hyp_page_count((void *)virt)) {
@@ -735,6 +734,7 @@ int __pkvm_host_unshare_hyp(u64 pfn)
 		goto unlock;
 	}
 
+	__hyp_set_page_state_range(phys, size, PKVM_NOPAGE);
 	WARN_ON(kvm_pgtable_hyp_unmap(&pkvm_pgtable, virt, size) != size);
 	WARN_ON(__host_set_page_state_range(phys, size, PKVM_PAGE_OWNED));
 
@@ -750,7 +750,6 @@ int __pkvm_host_donate_hyp(u64 pfn, u64 nr_pages)
 	u64 phys = hyp_pfn_to_phys(pfn);
 	u64 size = PAGE_SIZE * nr_pages;
 	void *virt = __hyp_va(phys);
-	enum kvm_pgtable_prot prot;
 	int ret;
 
 	host_lock_component();
@@ -760,13 +759,13 @@ int __pkvm_host_donate_hyp(u64 pfn, u64 nr_pages)
 	if (ret)
 		goto unlock;
 	if (IS_ENABLED(CONFIG_NVHE_EL2_DEBUG)) {
-		ret = __hyp_check_page_state_range((u64)virt, size, PKVM_NOPAGE);
+		ret = __hyp_check_page_state_range(phys, size, PKVM_NOPAGE);
 		if (ret)
 			goto unlock;
 	}
 
-	prot = pkvm_mkstate(PAGE_HYP, PKVM_PAGE_OWNED);
-	WARN_ON(pkvm_create_mappings_locked(virt, virt + size, prot));
+	__hyp_set_page_state_range(phys, size, PKVM_PAGE_OWNED);
+	WARN_ON(pkvm_create_mappings_locked(virt, virt + size, PAGE_HYP));
 	WARN_ON(host_stage2_set_owner_locked(phys, size, PKVM_ID_HYP));
 
 unlock:
@@ -786,7 +785,7 @@ int __pkvm_hyp_donate_host(u64 pfn, u64 nr_pages)
 	host_lock_component();
 	hyp_lock_component();
 
-	ret = __hyp_check_page_state_range(virt, size, PKVM_PAGE_OWNED);
+	ret = __hyp_check_page_state_range(phys, size, PKVM_PAGE_OWNED);
 	if (ret)
 		goto unlock;
 	if (IS_ENABLED(CONFIG_NVHE_EL2_DEBUG)) {
@@ -795,6 +794,7 @@ int __pkvm_hyp_donate_host(u64 pfn, u64 nr_pages)
 			goto unlock;
 	}
 
+	__hyp_set_page_state_range(phys, size, PKVM_NOPAGE);
 	WARN_ON(kvm_pgtable_hyp_unmap(&pkvm_pgtable, virt, size) != size);
 	WARN_ON(host_stage2_set_owner_locked(phys, size, PKVM_ID_HOST));
 
@@ -809,19 +809,18 @@ int hyp_pin_shared_mem(void *from, void *to)
 {
 	u64 cur, start = ALIGN_DOWN((u64)from, PAGE_SIZE);
 	u64 end = PAGE_ALIGN((u64)to);
+	u64 phys = __hyp_pa(start);
 	u64 size = end - start;
 	int ret;
 
 	host_lock_component();
 	hyp_lock_component();
 
-	ret = __host_check_page_state_range(__hyp_pa(start), size,
-					    PKVM_PAGE_SHARED_OWNED);
+	ret = __host_check_page_state_range(phys, size, PKVM_PAGE_SHARED_OWNED);
 	if (ret)
 		goto unlock;
 
-	ret = __hyp_check_page_state_range(start, size,
-					   PKVM_PAGE_SHARED_BORROWED);
+	ret = __hyp_check_page_state_range(phys, size, PKVM_PAGE_SHARED_BORROWED);
 	if (ret)
 		goto unlock;
 
diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
index 1a414288fe8c..955c431af5d0 100644
--- a/arch/arm64/kvm/hyp/nvhe/setup.c
+++ b/arch/arm64/kvm/hyp/nvhe/setup.c
@@ -194,16 +194,20 @@ static int fix_host_ownership_walker(const struct kvm_pgtable_visit_ctx *ctx,
 
 	/*
 	 * Adjust the host stage-2 mappings to match the ownership attributes
-	 * configured in the hypervisor stage-1.
+	 * configured in the hypervisor stage-1, and make sure to propagate them
+	 * to the hyp_vmemmap state.
 	 */
 	state = pkvm_getstate(kvm_pgtable_hyp_pte_prot(ctx->old));
 	switch (state) {
 	case PKVM_PAGE_OWNED:
+		set_hyp_state(phys, PKVM_PAGE_OWNED);
 		return host_stage2_set_owner_locked(phys, PAGE_SIZE, PKVM_ID_HYP);
 	case PKVM_PAGE_SHARED_OWNED:
+		set_hyp_state(phys, PKVM_PAGE_SHARED_OWNED);
 		set_host_state(phys, PKVM_PAGE_SHARED_BORROWED);
 		break;
 	case PKVM_PAGE_SHARED_BORROWED:
+		set_hyp_state(phys, PKVM_PAGE_SHARED_BORROWED);
 		set_host_state(phys, PKVM_PAGE_SHARED_OWNED);
 		break;
 	default:
-- 
2.48.1.658.g4767266eb4-goog

[PATCH 5/6] KVM: arm64: Defer EL2 stage-1 mapping on share

[Quentin Perret]

We currently blindly map into EL2 stage-1 *any* page passed to the
__pkvm_host_share_hyp() HVC. This is less than ideal from a security
perspective as it makes exploitation of potential hypervisor gadgets
easier than it should be. But interestingly, pKVM should never need to
access pages that it hasn't previously pinned, so there is no need to
map the page before that.

Signed-off-by: Quentin Perret <qperret@google.com>
---
 arch/arm64/kvm/hyp/nvhe/mem_protect.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
index 3ab8c81500c2..ae39d74be1f2 100644
--- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
+++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
@@ -686,7 +686,6 @@ static int __guest_check_page_state_range(struct pkvm_hyp_vcpu *vcpu, u64 addr,
 int __pkvm_host_share_hyp(u64 pfn)
 {
 	u64 phys = hyp_pfn_to_phys(pfn);
-	void *virt = __hyp_va(phys);
 	u64 size = PAGE_SIZE;
 	int ret;
 
@@ -703,7 +702,6 @@ int __pkvm_host_share_hyp(u64 pfn)
 	}
 
 	__hyp_set_page_state_range(phys, size, PKVM_PAGE_SHARED_BORROWED);
-	WARN_ON(pkvm_create_mappings_locked(virt, virt + size, PAGE_HYP));
 	WARN_ON(__host_set_page_state_range(phys, size, PKVM_PAGE_SHARED_OWNED));
 
 unlock:
@@ -735,7 +733,6 @@ int __pkvm_host_unshare_hyp(u64 pfn)
 	}
 
 	__hyp_set_page_state_range(phys, size, PKVM_NOPAGE);
-	WARN_ON(kvm_pgtable_hyp_unmap(&pkvm_pgtable, virt, size) != size);
 	WARN_ON(__host_set_page_state_range(phys, size, PKVM_PAGE_OWNED));
 
 unlock:
@@ -811,6 +808,7 @@ int hyp_pin_shared_mem(void *from, void *to)
 	u64 end = PAGE_ALIGN((u64)to);
 	u64 phys = __hyp_pa(start);
 	u64 size = end - start;
+	struct hyp_page *p;
 	int ret;
 
 	host_lock_component();
@@ -824,8 +822,14 @@ int hyp_pin_shared_mem(void *from, void *to)
 	if (ret)
 		goto unlock;
 
-	for (cur = start; cur < end; cur += PAGE_SIZE)
-		hyp_page_ref_inc(hyp_virt_to_page(cur));
+	for (cur = start; cur < end; cur += PAGE_SIZE) {
+		p = hyp_virt_to_page(cur);
+		hyp_page_ref_inc(p);
+		if (p->refcount == 1)
+			WARN_ON(pkvm_create_mappings_locked((void *)cur,
+							    (void *)cur + PAGE_SIZE,
+							    PAGE_HYP));
+	}
 
 unlock:
 	hyp_unlock_component();
@@ -838,12 +842,17 @@ void hyp_unpin_shared_mem(void *from, void *to)
 {
 	u64 cur, start = ALIGN_DOWN((u64)from, PAGE_SIZE);
 	u64 end = PAGE_ALIGN((u64)to);
+	struct hyp_page *p;
 
 	host_lock_component();
 	hyp_lock_component();
 
-	for (cur = start; cur < end; cur += PAGE_SIZE)
-		hyp_page_ref_dec(hyp_virt_to_page(cur));
+	for (cur = start; cur < end; cur += PAGE_SIZE) {
+		p = hyp_virt_to_page(cur);
+		if (p->refcount == 1)
+			WARN_ON(kvm_pgtable_hyp_unmap(&pkvm_pgtable, cur, PAGE_SIZE) != PAGE_SIZE);
+		hyp_page_ref_dec(p);
+	}
 
 	hyp_unlock_component();
 	host_unlock_component();
-- 
2.48.1.658.g4767266eb4-goog

[PATCH 6/6] KVM: arm64: Unconditionally cross check hyp state

[Quentin Perret]

Now that the hypervisor's state is stored in the hyp_vmemmap, we no
longer need an expensive page-table walk to read it. This means we can
now afford to cross check the hyp-state during all memory ownership
transitions where the hyp is involved unconditionally, hence avoiding
problems such as [1].

[1] https://lore.kernel.org/kvmarm/20241128154406.602875-1-qperret@google.com/

Signed-off-by: Quentin Perret <qperret@google.com>
---
 arch/arm64/kvm/hyp/nvhe/mem_protect.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
index ae39d74be1f2..22a906c7973a 100644
--- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
+++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
@@ -695,11 +695,9 @@ int __pkvm_host_share_hyp(u64 pfn)
 	ret = __host_check_page_state_range(phys, size, PKVM_PAGE_OWNED);
 	if (ret)
 		goto unlock;
-	if (IS_ENABLED(CONFIG_NVHE_EL2_DEBUG)) {
-		ret = __hyp_check_page_state_range(phys, size, PKVM_NOPAGE);
-		if (ret)
-			goto unlock;
-	}
+	ret = __hyp_check_page_state_range(phys, size, PKVM_NOPAGE);
+	if (ret)
+		goto unlock;
 
 	__hyp_set_page_state_range(phys, size, PKVM_PAGE_SHARED_BORROWED);
 	WARN_ON(__host_set_page_state_range(phys, size, PKVM_PAGE_SHARED_OWNED));
@@ -755,11 +753,9 @@ int __pkvm_host_donate_hyp(u64 pfn, u64 nr_pages)
 	ret = __host_check_page_state_range(phys, size, PKVM_PAGE_OWNED);
 	if (ret)
 		goto unlock;
-	if (IS_ENABLED(CONFIG_NVHE_EL2_DEBUG)) {
-		ret = __hyp_check_page_state_range(phys, size, PKVM_NOPAGE);
-		if (ret)
-			goto unlock;
-	}
+	ret = __hyp_check_page_state_range(phys, size, PKVM_NOPAGE);
+	if (ret)
+		goto unlock;
 
 	__hyp_set_page_state_range(phys, size, PKVM_PAGE_OWNED);
 	WARN_ON(pkvm_create_mappings_locked(virt, virt + size, PAGE_HYP));
@@ -785,11 +781,9 @@ int __pkvm_hyp_donate_host(u64 pfn, u64 nr_pages)
 	ret = __hyp_check_page_state_range(phys, size, PKVM_PAGE_OWNED);
 	if (ret)
 		goto unlock;
-	if (IS_ENABLED(CONFIG_NVHE_EL2_DEBUG)) {
-		ret = __host_check_page_state_range(phys, size, PKVM_NOPAGE);
-		if (ret)
-			goto unlock;
-	}
+	ret = __host_check_page_state_range(phys, size, PKVM_NOPAGE);
+	if (ret)
+		goto unlock;
 
 	__hyp_set_page_state_range(phys, size, PKVM_NOPAGE);
 	WARN_ON(kvm_pgtable_hyp_unmap(&pkvm_pgtable, virt, size) != size);
-- 
2.48.1.658.g4767266eb4-goog

Re: [PATCH 4/6] KVM: arm64: Move hyp state to hyp_vmemmap

[Vincent Donnefort]

[...]

> diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
> index 1a414288fe8c..955c431af5d0 100644
> --- a/arch/arm64/kvm/hyp/nvhe/setup.c
> +++ b/arch/arm64/kvm/hyp/nvhe/setup.c
> @@ -194,16 +194,20 @@ static int fix_host_ownership_walker(const struct kvm_pgtable_visit_ctx *ctx,
>  
>  	/*
>  	 * Adjust the host stage-2 mappings to match the ownership attributes
> -	 * configured in the hypervisor stage-1.
> +	 * configured in the hypervisor stage-1, and make sure to propagate them
> +	 * to the hyp_vmemmap state.
>  	 */
>  	state = pkvm_getstate(kvm_pgtable_hyp_pte_prot(ctx->old));
>  	switch (state) {
>  	case PKVM_PAGE_OWNED:
> +		set_hyp_state(phys, PKVM_PAGE_OWNED);
>  		return host_stage2_set_owner_locked(phys, PAGE_SIZE, PKVM_ID_HYP);
>  	case PKVM_PAGE_SHARED_OWNED:
> +		set_hyp_state(phys, PKVM_PAGE_SHARED_OWNED);
>  		set_host_state(phys, PKVM_PAGE_SHARED_BORROWED);
>  		break;
>  	case PKVM_PAGE_SHARED_BORROWED:
> +		set_hyp_state(phys, PKVM_PAGE_SHARED_BORROWED);
>  		set_host_state(phys, PKVM_PAGE_SHARED_OWNED);
>  		break;
>  	default:

Are the SHARED_OWNED/SHARED_BORROWED still relevant since the introduction of
"KVM: arm64: Don't map 'kvm_vgic_global_state' at EL2 with pKVM"? It doesn't
seem we have any !OWNED pages in the hyp anymore at setup, do we?

> -- 
> 2.48.1.658.g4767266eb4-goog
> 

Re: [PATCH 4/6] KVM: arm64: Move hyp state to hyp_vmemmap

[Quentin Perret]

On Monday 03 Mar 2025 at 09:47:48 (+0000), Vincent Donnefort wrote:
> [...]
> 
> > diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
> > index 1a414288fe8c..955c431af5d0 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/setup.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/setup.c
> > @@ -194,16 +194,20 @@ static int fix_host_ownership_walker(const struct kvm_pgtable_visit_ctx *ctx,
> >  
> >  	/*
> >  	 * Adjust the host stage-2 mappings to match the ownership attributes
> > -	 * configured in the hypervisor stage-1.
> > +	 * configured in the hypervisor stage-1, and make sure to propagate them
> > +	 * to the hyp_vmemmap state.
> >  	 */
> >  	state = pkvm_getstate(kvm_pgtable_hyp_pte_prot(ctx->old));
> >  	switch (state) {
> >  	case PKVM_PAGE_OWNED:
> > +		set_hyp_state(phys, PKVM_PAGE_OWNED);
> >  		return host_stage2_set_owner_locked(phys, PAGE_SIZE, PKVM_ID_HYP);
> >  	case PKVM_PAGE_SHARED_OWNED:
> > +		set_hyp_state(phys, PKVM_PAGE_SHARED_OWNED);
> >  		set_host_state(phys, PKVM_PAGE_SHARED_BORROWED);
> >  		break;
> >  	case PKVM_PAGE_SHARED_BORROWED:
> > +		set_hyp_state(phys, PKVM_PAGE_SHARED_BORROWED);
> >  		set_host_state(phys, PKVM_PAGE_SHARED_OWNED);
> >  		break;
> >  	default:
> 
> Are the SHARED_OWNED/SHARED_BORROWED still relevant since the introduction of
> "KVM: arm64: Don't map 'kvm_vgic_global_state' at EL2 with pKVM"? It doesn't
> seem we have any !OWNED pages in the hyp anymore at setup, do we?

That's a good point. I personally don't hate that we have this code here
for completeness though -- it's simple enough that maintaining it isn't
too bad, and if we were to add shared pages in the future it would 'just
work'. But no strong opinion. I guess we could also remove this code as
a separate clean-up, this isn't specific to this series.

Thanks!
Quentin

Re: [PATCH 2/6] KVM: arm64: Use 0b11 for encoding PKVM_NOPAGE

[Marc Zyngier]

On Thu, 27 Feb 2025 00:33:06 +0000,
Quentin Perret <qperret@google.com> wrote:
> 
> The page ownership state encoded as 0b11 is currently considered
> reserved for future use, and PKVM_NOPAGE uses bit 2. In order to
> simplify the relocation of the hyp ownership state into the
> vmemmap in later patches, let's use the 'reserved' encoding for
> the PKVM_NOPAGE state. The struct hyp_page layout isn't guaranteed
> stable at all, so there is no real reason to have 'reserved' encodings.
> 
> No functional changes intended.
> 
> Signed-off-by: Quentin Perret <qperret@google.com>
> ---
>  arch/arm64/kvm/hyp/include/nvhe/memory.h | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> index 34233d586060..642b5e05fe77 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> @@ -13,18 +13,15 @@
>   *   01: The page is owned by the page-table owner, but is shared
>   *       with another entity.
>   *   10: The page is shared with, but not owned by the page-table owner.
> - *   11: Reserved for future use (lending).
>   */
>  enum pkvm_page_state {
>  	PKVM_PAGE_OWNED			= 0ULL,
>  	PKVM_PAGE_SHARED_OWNED		= BIT(0),
>  	PKVM_PAGE_SHARED_BORROWED	= BIT(1),
> -	__PKVM_PAGE_RESERVED		= BIT(0) | BIT(1),
>  
>  	/* Meta-states which aren't encoded directly in the PTE's SW bits */
> -	PKVM_NOPAGE			= BIT(2),
> +	PKVM_NOPAGE			= BIT(0) | BIT(1),

Isn't this comment stale now?

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 4/6] KVM: arm64: Move hyp state to hyp_vmemmap

[Marc Zyngier]

On Thu, 27 Feb 2025 00:33:08 +0000,
Quentin Perret <qperret@google.com> wrote:
> 
> Tracking the hypervisor's ownership state into struct hyp_page has
> several benefits, including allowing far more efficient lookups (no
> page-table walk needed) and de-corelating the state from the presence
> of a mapping. This will later allow to map pages into EL2 stage-1 less
> proactively which is generally a good thing for security. And in the
> future this will help with tracking the state of pages mapped into the
> hypervisor's private range without requiring an alias into the 'linear
> map' range.
> 
> Signed-off-by: Quentin Perret <qperret@google.com>
> ---
>  arch/arm64/kvm/hyp/include/nvhe/memory.h | 20 +++++++++-
>  arch/arm64/kvm/hyp/nvhe/mem_protect.c    | 51 ++++++++++++------------
>  arch/arm64/kvm/hyp/nvhe/setup.c          |  6 ++-
>  3 files changed, 49 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> index 4a3c55d26ef3..cc4c01158368 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> @@ -22,6 +22,7 @@ enum pkvm_page_state {
>  	/* Meta-states which aren't encoded directly in the PTE's SW bits */
>  	PKVM_NOPAGE			= BIT(0) | BIT(1),
>  };
> +#define PKVM_PAGE_STATE_MASK		(BIT(0) | BIT(1))
>  
>  #define PKVM_PAGE_STATE_PROT_MASK	(KVM_PGTABLE_PROT_SW0 | KVM_PGTABLE_PROT_SW1)
>  static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
> @@ -42,7 +43,14 @@ struct hyp_page {
>  	u8 order;
>  
>  	/* Host (non-meta) state. Guarded by the host stage-2 lock. */
> -	unsigned __host_state : 8;
> +	unsigned __host_state : 4;
> +
> +	/*
> +	 * Complement of the hyp (non-meta) state. Guarded by the hyp stage-1 lock. We use the
> +	 * complement so that the initial 0 in __hyp_state_comp (due to the entire vmemmap starting
> +	 * off zeroed) encodes PKVM_NOPAGE.
> +	 */
> +	unsigned __hyp_state_comp : 4;
>  
>  	u32 host_share_guest_count;
>  };
> @@ -89,6 +97,16 @@ static inline void set_host_state(phys_addr_t phys, enum pkvm_page_state state)
>  	hyp_phys_to_page(phys)->__host_state = state;
>  }
>  
> +static inline enum pkvm_page_state get_hyp_state(phys_addr_t phys)
> +{
> +	return hyp_phys_to_page(phys)->__hyp_state_comp ^ PKVM_PAGE_STATE_MASK;
> +}
> +
> +static inline void set_hyp_state(phys_addr_t phys, enum pkvm_page_state state)
> +{
> +	hyp_phys_to_page(phys)->__hyp_state_comp = state ^ PKVM_PAGE_STATE_MASK;
> +}
> +
>  /*
>   * Refcounting for 'struct hyp_page'.
>   * hyp_pool::lock must be held if atomic access to the refcount is required.
> diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> index a45ffdec7612..3ab8c81500c2 100644
> --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> @@ -642,24 +642,24 @@ static int __host_set_page_state_range(u64 addr, u64 size,
>  	return 0;
>  }
>  
> -static enum pkvm_page_state hyp_get_page_state(kvm_pte_t pte, u64 addr)
> +static void __hyp_set_page_state_range(phys_addr_t phys, u64 size, enum pkvm_page_state state)
>  {
> -	if (!kvm_pte_valid(pte))
> -		return PKVM_NOPAGE;
> +	phys_addr_t end = phys + size;
>  
> -	return pkvm_getstate(kvm_pgtable_hyp_pte_prot(pte));
> +	for (; phys < end; phys += PAGE_SIZE)
> +		set_hyp_state(phys, state);
>  }
>  
> -static int __hyp_check_page_state_range(u64 addr, u64 size,
> -					enum pkvm_page_state state)
> +static int __hyp_check_page_state_range(phys_addr_t phys, u64 size, enum pkvm_page_state state)
>  {
> -	struct check_walk_data d = {
> -		.desired	= state,
> -		.get_page_state	= hyp_get_page_state,
> -	};
> +	phys_addr_t end = phys + size;
> +
> +	for (; phys < end; phys += PAGE_SIZE) {
> +		if (get_hyp_state(phys) != state)
> +			return -EPERM;
> +	}
>  
> -	hyp_assert_lock_held(&pkvm_pgd_lock);
> -	return check_page_state_range(&pkvm_pgtable, addr, size, &d);
> +	return 0;
>  }
>  
>  static enum pkvm_page_state guest_get_page_state(kvm_pte_t pte, u64 addr)
> @@ -687,7 +687,6 @@ int __pkvm_host_share_hyp(u64 pfn)
>  {
>  	u64 phys = hyp_pfn_to_phys(pfn);
>  	void *virt = __hyp_va(phys);
> -	enum kvm_pgtable_prot prot;
>  	u64 size = PAGE_SIZE;
>  	int ret;
>  
> @@ -698,13 +697,13 @@ int __pkvm_host_share_hyp(u64 pfn)
>  	if (ret)
>  		goto unlock;
>  	if (IS_ENABLED(CONFIG_NVHE_EL2_DEBUG)) {
> -		ret = __hyp_check_page_state_range((u64)virt, size, PKVM_NOPAGE);
> +		ret = __hyp_check_page_state_range(phys, size, PKVM_NOPAGE);

OK, I think I finally clicked here. Does it mean that all the tracking
is now done in terms of PAs instead of VAs?

>  		if (ret)
>  			goto unlock;
>  	}
>  
> -	prot = pkvm_mkstate(PAGE_HYP, PKVM_PAGE_SHARED_BORROWED);
> -	WARN_ON(pkvm_create_mappings_locked(virt, virt + size, prot));
> +	__hyp_set_page_state_range(phys, size, PKVM_PAGE_SHARED_BORROWED);
> +	WARN_ON(pkvm_create_mappings_locked(virt, virt + size, PAGE_HYP));

And this is the split between the state now being kept in the on a PA
base and the actual mapping that is now only takes the page attributes
and no SW bits?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 4/6] KVM: arm64: Move hyp state to hyp_vmemmap

[Quentin Perret]

On Friday 14 Mar 2025 at 11:31:36 (+0000), Marc Zyngier wrote:
> On Thu, 27 Feb 2025 00:33:08 +0000,
> Quentin Perret <qperret@google.com> wrote:
> > @@ -698,13 +697,13 @@ int __pkvm_host_share_hyp(u64 pfn)
> >  	if (ret)
> >  		goto unlock;
> >  	if (IS_ENABLED(CONFIG_NVHE_EL2_DEBUG)) {
> > -		ret = __hyp_check_page_state_range((u64)virt, size, PKVM_NOPAGE);
> > +		ret = __hyp_check_page_state_range(phys, size, PKVM_NOPAGE);
> 
> OK, I think I finally clicked here. Does it mean that all the tracking
> is now done in terms of PAs instead of VAs?

Yep, that's exactly that. The hyp_vmemmap is indexed by pfn, so I felt
that the conversion to a PA-based tracking made sense. That also make it
clear that the 'hyp state' is not a property of a mapping, but really of
the underlying physical page.

> >  		if (ret)
> >  			goto unlock;
> >  	}
> >  
> > -	prot = pkvm_mkstate(PAGE_HYP, PKVM_PAGE_SHARED_BORROWED);
> > -	WARN_ON(pkvm_create_mappings_locked(virt, virt + size, prot));
> > +	__hyp_set_page_state_range(phys, size, PKVM_PAGE_SHARED_BORROWED);
> > +	WARN_ON(pkvm_create_mappings_locked(virt, virt + size, PAGE_HYP));
> 
> And this is the split between the state now being kept in the on a PA
> base and the actual mapping that is now only takes the page attributes
> and no SW bits?

Precisely, and the next patch in this series takes advantage of the
fact that we're now de-correlating the hyp state from the presence of a
hyp s1 mapping in the linear map range. In the future there'll be more
use-cases for this I think (e.g. the hyp allocator where we'll have
pages owned by the hypervisor but only mapped in the 'private' range,
things like that).

Thanks,
Quentin

Re: [PATCH 2/6] KVM: arm64: Use 0b11 for encoding PKVM_NOPAGE

[Quentin Perret]

On Friday 14 Mar 2025 at 11:07:01 (+0000), Marc Zyngier wrote:
> On Thu, 27 Feb 2025 00:33:06 +0000,
> Quentin Perret <qperret@google.com> wrote:
> > 
> > The page ownership state encoded as 0b11 is currently considered
> > reserved for future use, and PKVM_NOPAGE uses bit 2. In order to
> > simplify the relocation of the hyp ownership state into the
> > vmemmap in later patches, let's use the 'reserved' encoding for
> > the PKVM_NOPAGE state. The struct hyp_page layout isn't guaranteed
> > stable at all, so there is no real reason to have 'reserved' encodings.
> > 
> > No functional changes intended.
> > 
> > Signed-off-by: Quentin Perret <qperret@google.com>
> > ---
> >  arch/arm64/kvm/hyp/include/nvhe/memory.h | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > index 34233d586060..642b5e05fe77 100644
> > --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > @@ -13,18 +13,15 @@
> >   *   01: The page is owned by the page-table owner, but is shared
> >   *       with another entity.
> >   *   10: The page is shared with, but not owned by the page-table owner.
> > - *   11: Reserved for future use (lending).
> >   */
> >  enum pkvm_page_state {
> >  	PKVM_PAGE_OWNED			= 0ULL,
> >  	PKVM_PAGE_SHARED_OWNED		= BIT(0),
> >  	PKVM_PAGE_SHARED_BORROWED	= BIT(1),
> > -	__PKVM_PAGE_RESERVED		= BIT(0) | BIT(1),
> >  
> >  	/* Meta-states which aren't encoded directly in the PTE's SW bits */
> > -	PKVM_NOPAGE			= BIT(2),
> > +	PKVM_NOPAGE			= BIT(0) | BIT(1),
> 
> Isn't this comment stale now?

I believe it still applies to guest stage-2 page-tables as the three
other states above are still stored into PTE SW bits (well, sort of,
only SHARED_BORROWED is at the moment as we don't supported protected
VMs, but OWNED and SHARED_OWNED will be a thing for protected). NOPAGE
is still the only one that is a bit different and doesn't go there.

With that said, the comment could be made more explicit about that and
explain this is now guest s2 only. Happy to spin another version of the
series with that changed if that helps.

Thanks!
Quentin

Re: [PATCH 2/6] KVM: arm64: Use 0b11 for encoding PKVM_NOPAGE

[Marc Zyngier]

On Fri, 14 Mar 2025 14:13:39 +0000,
Quentin Perret <qperret@google.com> wrote:
> 
> On Friday 14 Mar 2025 at 11:07:01 (+0000), Marc Zyngier wrote:
> > On Thu, 27 Feb 2025 00:33:06 +0000,
> > Quentin Perret <qperret@google.com> wrote:
> > > 
> > > The page ownership state encoded as 0b11 is currently considered
> > > reserved for future use, and PKVM_NOPAGE uses bit 2. In order to
> > > simplify the relocation of the hyp ownership state into the
> > > vmemmap in later patches, let's use the 'reserved' encoding for
> > > the PKVM_NOPAGE state. The struct hyp_page layout isn't guaranteed
> > > stable at all, so there is no real reason to have 'reserved' encodings.
> > > 
> > > No functional changes intended.
> > > 
> > > Signed-off-by: Quentin Perret <qperret@google.com>
> > > ---
> > >  arch/arm64/kvm/hyp/include/nvhe/memory.h | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > index 34233d586060..642b5e05fe77 100644
> > > --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > @@ -13,18 +13,15 @@
> > >   *   01: The page is owned by the page-table owner, but is shared
> > >   *       with another entity.
> > >   *   10: The page is shared with, but not owned by the page-table owner.
> > > - *   11: Reserved for future use (lending).
> > >   */
> > >  enum pkvm_page_state {
> > >  	PKVM_PAGE_OWNED			= 0ULL,
> > >  	PKVM_PAGE_SHARED_OWNED		= BIT(0),
> > >  	PKVM_PAGE_SHARED_BORROWED	= BIT(1),
> > > -	__PKVM_PAGE_RESERVED		= BIT(0) | BIT(1),
> > >  
> > >  	/* Meta-states which aren't encoded directly in the PTE's SW bits */
> > > -	PKVM_NOPAGE			= BIT(2),
> > > +	PKVM_NOPAGE			= BIT(0) | BIT(1),
> > 
> > Isn't this comment stale now?
> 
> I believe it still applies to guest stage-2 page-tables as the three
> other states above are still stored into PTE SW bits (well, sort of,
> only SHARED_BORROWED is at the moment as we don't supported protected
> VMs, but OWNED and SHARED_OWNED will be a thing for protected). NOPAGE
> is still the only one that is a bit different and doesn't go there.
>
> With that said, the comment could be made more explicit about that and
> explain this is now guest s2 only. Happy to spin another version of the
> series with that changed if that helps.

Right. It is slightly misleading that we end-up with two notions of
state:

- the per-mapping state encoded in the PTs
- the per-page state encoded in the vmemmap

The two are obviously related, but it is all a bit confusing for the
unsuspecting reader. There is no urgency to address this, but maybe
it'd be good to clarify this aspect in the future (no need to respin
on that account).

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 4/6] KVM: arm64: Move hyp state to hyp_vmemmap

[Marc Zyngier]

On Fri, 14 Mar 2025 14:06:48 +0000,
Quentin Perret <qperret@google.com> wrote:
> 
> On Friday 14 Mar 2025 at 11:31:36 (+0000), Marc Zyngier wrote:
> > On Thu, 27 Feb 2025 00:33:08 +0000,
> > Quentin Perret <qperret@google.com> wrote:
> > > @@ -698,13 +697,13 @@ int __pkvm_host_share_hyp(u64 pfn)
> > >  	if (ret)
> > >  		goto unlock;
> > >  	if (IS_ENABLED(CONFIG_NVHE_EL2_DEBUG)) {
> > > -		ret = __hyp_check_page_state_range((u64)virt, size, PKVM_NOPAGE);
> > > +		ret = __hyp_check_page_state_range(phys, size, PKVM_NOPAGE);
> > 
> > OK, I think I finally clicked here. Does it mean that all the tracking
> > is now done in terms of PAs instead of VAs?
> 
> Yep, that's exactly that. The hyp_vmemmap is indexed by pfn, so I felt
> that the conversion to a PA-based tracking made sense. That also make it
> clear that the 'hyp state' is not a property of a mapping, but really of
> the underlying physical page.

It indeed makes sense. It is just that it took me some time to realise
the extent of the change.

> 
> > >  		if (ret)
> > >  			goto unlock;
> > >  	}
> > >  
> > > -	prot = pkvm_mkstate(PAGE_HYP, PKVM_PAGE_SHARED_BORROWED);
> > > -	WARN_ON(pkvm_create_mappings_locked(virt, virt + size, prot));
> > > +	__hyp_set_page_state_range(phys, size, PKVM_PAGE_SHARED_BORROWED);
> > > +	WARN_ON(pkvm_create_mappings_locked(virt, virt + size, PAGE_HYP));
> > 
> > And this is the split between the state now being kept in the on a PA
> > base and the actual mapping that is now only takes the page attributes
> > and no SW bits?
> 
> Precisely, and the next patch in this series takes advantage of the
> fact that we're now de-correlating the hyp state from the presence of a
> hyp s1 mapping in the linear map range. In the future there'll be more
> use-cases for this I think (e.g. the hyp allocator where we'll have
> pages owned by the hypervisor but only mapped in the 'private' range,
> things like that).

Yup, that's probably the correct direction of travel. The hypervisor
shouldn't need to map everything -- quite the opposite actually.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/6] Move pKVM ownership state to hyp_vmemmap

[Marc Zyngier]

On Thu, 27 Feb 2025 00:33:04 +0000,
Quentin Perret <qperret@google.com> wrote:
> 
> Hi all,
> 
> This series moves the hypervisor's ownership state to the hyp_vmemmap,
> as discussed in [1]. The two main benefits are:
> 
>  1. much cheaper hyp state lookups, since we can avoid the hyp stage-1
>     page-table walk;
> 
>  2. de-correlates the hyp state from the presence of a mapping in the
>     linear map range of the hypervisor; which enables a bunch of
>     clean-ups in the existing code and will simplify the introduction of
>     other features in the future (hyp tracing, ...)
> 
> Patch 01 is a self-sufficient cleanup that I found thanks to patch 05.
> Patches 02-04 implement the aforementioned migration of the hyp state
> to the vmemmap. Patches 05 and 06 are cleanups enabled by that
> migration.
> 
> Patches based on 6.14-rc4, tested on Qemu.
> 
> Thanks!
> Quentin
> 
> [1] https://lore.kernel.org/kvmarm/Z79ZJVOHtNu6YsVt@google.com/
> 
> Fuad Tabba (1):
>   KVM: arm64: Track SVE state in the hypervisor vcpu structure
> 
> Quentin Perret (5):
>   KVM: arm64: Use 0b11 for encoding PKVM_NOPAGE
>   KVM: arm64: Introduce {get,set}_host_state() helpers
>   KVM: arm64: Move hyp state to hyp_vmemmap
>   KVM: arm64: Defer EL2 stage-1 mapping on share
>   KVM: arm64: Unconditionally cross check hyp state
> 
>  arch/arm64/include/asm/kvm_host.h        |  12 +--
>  arch/arm64/kvm/hyp/include/nvhe/memory.h |  35 ++++++--
>  arch/arm64/kvm/hyp/nvhe/hyp-main.c       |   4 -
>  arch/arm64/kvm/hyp/nvhe/mem_protect.c    | 106 ++++++++++++-----------
>  arch/arm64/kvm/hyp/nvhe/pkvm.c           |  54 ++++++++++--
>  arch/arm64/kvm/hyp/nvhe/setup.c          |  10 ++-
>  6 files changed, 147 insertions(+), 74 deletions(-)
> 

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.