| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87im2x1vgr.wl-maz@kernel.org>
Date: Tue, 01 Jun 2021 16:00:04 +0100
From: Marc Zyngier <maz@...nel.org>
To: Quentin Perret <qperret@...gle.com>
Cc: will@...nel.org, james.morse@....com, alexandru.elisei@....com,
catalin.marinas@....com, suzuki.poulose@....com,
linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu,
kernel-team@...roid.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/7] KVM: arm64: Remove hyp_pool pointer from struct hyp_page
On Thu, 27 May 2021 13:51:32 +0100,
Quentin Perret <qperret@...gle.com> wrote:
>
> Each struct hyp_page currently contains a pointer to a hyp_pool struct
> where the page should be freed if its refcount reaches 0. However, this
> information can always be inferred from the context in the EL2 code, so
> drop the pointer to save a few bytes in the vmemmap.
>
> Signed-off-by: Quentin Perret <qperret@...gle.com>
> ---
> arch/arm64/kvm/hyp/include/nvhe/gfp.h | 4 ++--
> arch/arm64/kvm/hyp/include/nvhe/memory.h | 2 --
> arch/arm64/kvm/hyp/nvhe/mem_protect.c | 13 +++++++++++--
> arch/arm64/kvm/hyp/nvhe/page_alloc.c | 7 ++-----
> arch/arm64/kvm/hyp/nvhe/setup.c | 14 ++++++++++++--
> 5 files changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/gfp.h b/arch/arm64/kvm/hyp/include/nvhe/gfp.h
> index aada4d97de49..9ed374648364 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/gfp.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/gfp.h
> @@ -42,8 +42,8 @@ static inline void hyp_set_page_refcounted(struct hyp_page *p)
>
> /* Allocation */
> void *hyp_alloc_pages(struct hyp_pool *pool, unsigned int order);
> -void hyp_get_page(void *addr);
> -void hyp_put_page(void *addr);
> +void hyp_get_page(void *addr, struct hyp_pool *pool);
> +void hyp_put_page(void *addr, struct hyp_pool *pool);
It'd be good to be consistent with __hyp_put_page(), which has these
arguments in the opposite order. See below for an example.
>
> /* Used pages cannot be freed */
> int hyp_pool_init(struct hyp_pool *pool, u64 pfn, unsigned int nr_pages,
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> index 7691ab495eb4..991636be2f46 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> @@ -7,11 +7,9 @@
>
> #include <linux/types.h>
>
> -struct hyp_pool;
> struct hyp_page {
> unsigned int refcount;
> unsigned int order;
> - struct hyp_pool *pool;
> };
>
> extern u64 __hyp_vmemmap;
> diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> index fdd5b5702e8a..3603311eb41c 100644
> --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> @@ -42,6 +42,15 @@ static void *host_s2_zalloc_page(void *pool)
> {
> return hyp_alloc_pages(pool, 0);
> }
> +static void host_s2_get_page(void *addr)
nit: missing blank line.
> +{
> + hyp_get_page(addr, &host_s2_pool);
> +}
> +
> +static void host_s2_put_page(void *addr)
> +{
> + hyp_put_page(addr, &host_s2_pool);
> +}
>
> static int prepare_s2_pool(void *pgt_pool_base)
> {
> @@ -60,8 +69,8 @@ static int prepare_s2_pool(void *pgt_pool_base)
> .phys_to_virt = hyp_phys_to_virt,
> .virt_to_phys = hyp_virt_to_phys,
> .page_count = hyp_page_count,
> - .get_page = hyp_get_page,
> - .put_page = hyp_put_page,
> + .get_page = host_s2_get_page,
> + .put_page = host_s2_put_page,
> };
>
> return 0;
> diff --git a/arch/arm64/kvm/hyp/nvhe/page_alloc.c b/arch/arm64/kvm/hyp/nvhe/page_alloc.c
> index ce7379f1480b..e453108a2d95 100644
> --- a/arch/arm64/kvm/hyp/nvhe/page_alloc.c
> +++ b/arch/arm64/kvm/hyp/nvhe/page_alloc.c
> @@ -150,20 +150,18 @@ static void __hyp_put_page(struct hyp_pool *pool, struct hyp_page *p)
> __hyp_attach_page(pool, p);
> }
>
> -void hyp_put_page(void *addr)
> +void hyp_put_page(void *addr, struct hyp_pool *pool)
> {
> struct hyp_page *p = hyp_virt_to_page(addr);
> - struct hyp_pool *pool = hyp_page_to_pool(p);
>
> hyp_spin_lock(&pool->lock);
> __hyp_put_page(pool, p);
When I see this, my eyes get crossed, and I don't know what I'm
reading anymore! ;-) In general, I like the "container" as a first
argument, followed by the element that can be contained.
> hyp_spin_unlock(&pool->lock);
> }
>
> -void hyp_get_page(void *addr)
> +void hyp_get_page(void *addr, struct hyp_pool *pool)
> {
> struct hyp_page *p = hyp_virt_to_page(addr);
> - struct hyp_pool *pool = hyp_page_to_pool(p);
>
> hyp_spin_lock(&pool->lock);
> hyp_page_ref_inc(p);
> @@ -212,7 +210,6 @@ int hyp_pool_init(struct hyp_pool *pool, u64 pfn, unsigned int nr_pages,
> /* Init the vmemmap portion */
> p = hyp_phys_to_page(phys);
> for (i = 0; i < nr_pages; i++) {
> - p[i].pool = pool;
> p[i].order = 0;
> hyp_set_page_refcounted(&p[i]);
> }
> diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
> index 709cb3d19eb7..bf61abd4a330 100644
> --- a/arch/arm64/kvm/hyp/nvhe/setup.c
> +++ b/arch/arm64/kvm/hyp/nvhe/setup.c
> @@ -137,6 +137,16 @@ static void *hyp_zalloc_hyp_page(void *arg)
> return hyp_alloc_pages(&hpool, 0);
> }
>
> +static void hpool_get_page(void *addr)
> +{
> + hyp_get_page(addr, &hpool);
> +}
> +
> +static void hpool_put_page(void *addr)
> +{
> + hyp_put_page(addr, &hpool);
> +}
> +
> void __noreturn __pkvm_init_finalise(void)
> {
> struct kvm_host_data *host_data = this_cpu_ptr(&kvm_host_data);
> @@ -160,8 +170,8 @@ void __noreturn __pkvm_init_finalise(void)
> .zalloc_page = hyp_zalloc_hyp_page,
> .phys_to_virt = hyp_phys_to_virt,
> .virt_to_phys = hyp_virt_to_phys,
> - .get_page = hyp_get_page,
> - .put_page = hyp_put_page,
> + .get_page = hpool_get_page,
> + .put_page = hpool_put_page,
> };
> pkvm_pgtable.mm_ops = &pkvm_pgtable_mm_ops;
>
> --
> 2.31.1.818.g46aad6cb9e-goog
>
>
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists