Re: [PATCH v4 02/10] KVM: arm64: Introduce for_each_hyp_page
From: Marc Zyngier <maz@kernel.org>
Date: 2025-05-16 12:57:54
Also in:
kvmarm, lkml
On Fri, 09 May 2025 14:16:58 +0100, Vincent Donnefort [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Add a helper to iterate over the hypervisor vmemmap. This will be particularly handy with the introduction of huge mapping support for the np-guest stage-2. Signed-off-by: Vincent Donnefort <redacted>diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h index eb0c2ebd1743..676a0a13c741 100644 --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h@@ -96,24 +96,24 @@ 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) +static inline enum pkvm_page_state get_host_state(struct hyp_page *p) { - return (enum pkvm_page_state)hyp_phys_to_page(phys)->__host_state; + return (enum pkvm_page_state)p->__host_state;
I'm not quite sure why we have this cast the first place. If we are so keen on type consistency, why isn't __host_state an enum pkvm_page_state the first place?
}
-static inline void set_host_state(phys_addr_t phys, enum pkvm_page_state state)
+static inline void set_host_state(struct hyp_page *p, enum pkvm_page_state state)
{
- hyp_phys_to_page(phys)->__host_state = state;
+ p->__host_state = state;
}
-static inline enum pkvm_page_state get_hyp_state(phys_addr_t phys)
+static inline enum pkvm_page_state get_hyp_state(struct hyp_page *p)
{
- return hyp_phys_to_page(phys)->__hyp_state_comp ^ PKVM_PAGE_STATE_MASK;
+ return p->__hyp_state_comp ^ PKVM_PAGE_STATE_MASK;And we don't seem that bothered here.
quoted hunk ↗ jump to hunk
} -static inline void set_hyp_state(phys_addr_t phys, enum pkvm_page_state state) +static inline void set_hyp_state(struct hyp_page *p, enum pkvm_page_state state) { - hyp_phys_to_page(phys)->__hyp_state_comp = state ^ PKVM_PAGE_STATE_MASK; + p->__hyp_state_comp = state ^ PKVM_PAGE_STATE_MASK; } /*diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c index 23544928a637..4d269210dae0 100644 --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c@@ -60,6 +60,9 @@ static void hyp_unlock_component(void) hyp_spin_unlock(&pkvm_pgd_lock); } +#define for_each_hyp_page(start, size, page) \ + for (page = hyp_phys_to_page(start); page < hyp_phys_to_page((start) + (size)); page++) +
Huh. This really should be something like: #define for_each_hyp_page(__p, __st, __sz) \ for (struct hyp_page *__p = hyp_phys_to_page(__st), \ *__e = __p + ((__sz) >> PAGE_SHIFT); \ __p < __e; __p++) so that: - the iterator variable comes first and looks like a normal for-loop - the iterator has a scope limited to the loop - we don't evaluate parameters multiple times - we don't evaluate the end of the loop multiple times - we try not to reuse common variable names
quoted hunk ↗ jump to hunk
static void *host_s2_zalloc_pages_exact(size_t size) { void *addr = hyp_alloc_pages(&host_s2_pool, get_order(size));@@ -481,7 +484,8 @@ static int host_stage2_adjust_range(u64 addr, struct kvm_mem_range *range) return -EAGAIN; if (pte) { - WARN_ON(addr_is_memory(addr) && get_host_state(addr) != PKVM_NOPAGE); + WARN_ON(addr_is_memory(addr) && + get_host_state(hyp_phys_to_page(addr)) != PKVM_NOPAGE); return -EPERM; }@@ -507,10 +511,10 @@ int host_stage2_idmap_locked(phys_addr_t addr, u64 size, static void __host_update_page_state(phys_addr_t addr, u64 size, enum pkvm_page_state state) { - phys_addr_t end = addr + size; + struct hyp_page *page;
and then these extra declarations can go. Thanks, M. -- Without deviation from the norm, progress is not possible.