From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 15729DDAB for ; Fri, 20 Oct 2023 10:42:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="Qkg4xe/9" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2B785C433C7; Fri, 20 Oct 2023 10:42:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1697798536; bh=i3Z44WfVde9QeZ+ipeoAFQQufo90upOHL6pzPAJToEM=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=Qkg4xe/98ecIpW2lLesKSo1lBU3XqzFfxin5wnzg2ElfieeRRVSrihnI2B7XCp5RT G/JapF51ntpFWPonEhauLZg9X1ncn0zfT59ttryLS/hltLpMSXpOw9iSW3C3ctt0jB ly1AvW9ZcuR/Fz6RZ/4RzYU+5+QuvfuW759s3wPX07fMquOhxRkHBBeS85ZWrBWNRq 3RwQIzuF8UH435s0tiQcc7J/oqQrhwSA0AgOiWoP5cwtxFJNXbeWUCCSe1CsTBulJb vi3InboJd39fIVdgbqa8rufueRQE1mugCUjyHjjRnwvZQ0GuwavN1doBIzdXVggrvw 9zSzctq40xAYw== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1qtmxJ-0065Q7-T0; Fri, 20 Oct 2023 11:42:14 +0100 Date: Fri, 20 Oct 2023 11:42:12 +0100 Message-ID: <868r7xmv7f.wl-maz@kernel.org> From: Marc Zyngier To: Ryan Roberts Cc: Catalin Marinas , Will Deacon , Oliver Upton , Suzuki K Poulose , James Morse , Zenghui Yu , Ard Biesheuvel , Anshuman Khandual , linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev Subject: Re: [PATCH v4 08/12] KVM: arm64: Convert translation level parameter to s8 In-Reply-To: <20231009185008.3803879-9-ryan.roberts@arm.com> References: <20231009185008.3803879-1-ryan.roberts@arm.com> <20231009185008.3803879-9-ryan.roberts@arm.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Precedence: bulk X-Mailing-List: kvmarm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: ryan.roberts@arm.com, catalin.marinas@arm.com, will@kernel.org, oliver.upton@linux.dev, suzuki.poulose@arm.com, james.morse@arm.com, yuzenghui@huawei.com, ardb@kernel.org, anshuman.khandual@arm.com, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false On Mon, 09 Oct 2023 19:50:04 +0100, Ryan Roberts wrote: > > With the introduction of FEAT_LPA2, the Arm ARM adds a new level of > translation, level -1, so levels can now be in the range [-1;3]. 3 is > always the last level and the first level is determined based on the > number of VA bits in use. > > Convert level variables to use a signed type in preparation for > supporting this new level -1. > > Since the last level is always anchored at 3, and the first level varies > to suit the number of VA/IPA bits, take the opportunity to replace > KVM_PGTABLE_MAX_LEVELS with the 2 macros KVM_PGTABLE_FIRST_LEVEL and > KVM_PGTABLE_LAST_LEVEL. This removes the assumption from the code that > levels run from 0 to KVM_PGTABLE_MAX_LEVELS - 1, which will soon no > longer be true. > > No behavioral changes intended. Shrug. Unless you have compared the binaries before and after and proven that they are strictly identical, there will be behaviour changes, intended or otherwise. I know what you're trying to convey, but I've seen so many patches carrying a sentence of this sort and yet turning the kernel on its head that I've become allergic to it. Sorry. > > Signed-off-by: Ryan Roberts > --- > arch/arm64/include/asm/kvm_emulate.h | 2 +- > arch/arm64/include/asm/kvm_pgtable.h | 31 ++++++------- > arch/arm64/include/asm/kvm_pkvm.h | 5 ++- > arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +-- > arch/arm64/kvm/hyp/nvhe/mm.c | 4 +- > arch/arm64/kvm/hyp/nvhe/setup.c | 2 +- > arch/arm64/kvm/hyp/pgtable.c | 64 ++++++++++++++------------- > arch/arm64/kvm/mmu.c | 16 ++++--- > 8 files changed, 69 insertions(+), 61 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index 3d6725ff0bf6..bf3ef66eb51f 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -404,7 +404,7 @@ static __always_inline u8 kvm_vcpu_trap_get_fault_type(const struct kvm_vcpu *vc > return kvm_vcpu_get_esr(vcpu) & ESR_ELx_FSC_TYPE; > } > > -static __always_inline u8 kvm_vcpu_trap_get_fault_level(const struct kvm_vcpu *vcpu) > +static __always_inline s8 kvm_vcpu_trap_get_fault_level(const struct kvm_vcpu *vcpu) > { > return kvm_vcpu_get_esr(vcpu) & ESR_ELx_FSC_LEVEL; > } > diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h > index b240158e1218..c61bb9709201 100644 > --- a/arch/arm64/include/asm/kvm_pgtable.h > +++ b/arch/arm64/include/asm/kvm_pgtable.h > @@ -11,7 +11,8 @@ > #include > #include > > -#define KVM_PGTABLE_MAX_LEVELS 4U > +#define KVM_PGTABLE_FIRST_LEVEL 0 > +#define KVM_PGTABLE_LAST_LEVEL 3 > > /* > * The largest supported block sizes for KVM (no 52-bit PA support): > @@ -20,9 +21,9 @@ > * - 64K (level 2): 512MB > */ > #ifdef CONFIG_ARM64_4K_PAGES > -#define KVM_PGTABLE_MIN_BLOCK_LEVEL 1U > +#define KVM_PGTABLE_MIN_BLOCK_LEVEL 1 > #else > -#define KVM_PGTABLE_MIN_BLOCK_LEVEL 2U > +#define KVM_PGTABLE_MIN_BLOCK_LEVEL 2 > #endif > > static inline u64 kvm_get_parange_max(void) > @@ -101,28 +102,28 @@ static inline kvm_pfn_t kvm_pte_to_pfn(kvm_pte_t pte) > return __phys_to_pfn(kvm_pte_to_phys(pte)); > } > > -static inline u64 kvm_granule_shift(u32 level) > +static inline u64 kvm_granule_shift(s8 level) > { > - /* Assumes KVM_PGTABLE_MAX_LEVELS is 4 */ > + /* Assumes KVM_PGTABLE_LAST_LEVEL is 3 */ > return ARM64_HW_PGTABLE_LEVEL_SHIFT(level); I'm amazed that the macro tolerates a negative level, but it really does. > } > > -static inline u64 kvm_granule_size(u32 level) > +static inline u64 kvm_granule_size(s8 level) > { > return BIT(kvm_granule_shift(level)); > } > > -static inline bool kvm_level_supports_block_mapping(u32 level) > +static inline bool kvm_level_supports_block_mapping(s8 level) > { > return level >= KVM_PGTABLE_MIN_BLOCK_LEVEL; > } > > static inline u32 kvm_supported_block_sizes(void) > { > - u32 level = KVM_PGTABLE_MIN_BLOCK_LEVEL; > + s8 level = KVM_PGTABLE_MIN_BLOCK_LEVEL; > u32 r = 0; > > - for (; level < KVM_PGTABLE_MAX_LEVELS; level++) > + for (; level <= KVM_PGTABLE_LAST_LEVEL; level++) > r |= BIT(kvm_granule_shift(level)); > > return r; > @@ -167,7 +168,7 @@ struct kvm_pgtable_mm_ops { > void* (*zalloc_page)(void *arg); > void* (*zalloc_pages_exact)(size_t size); > void (*free_pages_exact)(void *addr, size_t size); > - void (*free_unlinked_table)(void *addr, u32 level); > + void (*free_unlinked_table)(void *addr, s8 level); > void (*get_page)(void *addr); > void (*put_page)(void *addr); > int (*page_count)(void *addr); > @@ -263,7 +264,7 @@ struct kvm_pgtable_visit_ctx { > u64 start; > u64 addr; > u64 end; > - u32 level; > + s8 level; > enum kvm_pgtable_walk_flags flags; > }; > > @@ -366,7 +367,7 @@ static inline bool kvm_pgtable_walk_lock_held(void) > */ > struct kvm_pgtable { > u32 ia_bits; > - u32 start_level; > + s8 start_level; > kvm_pteref_t pgd; > struct kvm_pgtable_mm_ops *mm_ops; > > @@ -500,7 +501,7 @@ void kvm_pgtable_stage2_destroy(struct kvm_pgtable *pgt); > * The page-table is assumed to be unreachable by any hardware walkers prior to > * freeing and therefore no TLB invalidation is performed. > */ > -void kvm_pgtable_stage2_free_unlinked(struct kvm_pgtable_mm_ops *mm_ops, void *pgtable, u32 level); > +void kvm_pgtable_stage2_free_unlinked(struct kvm_pgtable_mm_ops *mm_ops, void *pgtable, s8 level); > > /** > * kvm_pgtable_stage2_create_unlinked() - Create an unlinked stage-2 paging structure. > @@ -524,7 +525,7 @@ void kvm_pgtable_stage2_free_unlinked(struct kvm_pgtable_mm_ops *mm_ops, void *p > * an ERR_PTR(error) on failure. > */ > kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt, > - u64 phys, u32 level, > + u64 phys, s8 level, > enum kvm_pgtable_prot prot, > void *mc, bool force_pte); > > @@ -750,7 +751,7 @@ int kvm_pgtable_walk(struct kvm_pgtable *pgt, u64 addr, u64 size, > * Return: 0 on success, negative error code on failure. > */ > int kvm_pgtable_get_leaf(struct kvm_pgtable *pgt, u64 addr, > - kvm_pte_t *ptep, u32 *level); > + kvm_pte_t *ptep, s8 *level); > > /** > * kvm_pgtable_stage2_pte_prot() - Retrieve the protection attributes of a > diff --git a/arch/arm64/include/asm/kvm_pkvm.h b/arch/arm64/include/asm/kvm_pkvm.h > index e46250a02017..ad9cfb5c1ff4 100644 > --- a/arch/arm64/include/asm/kvm_pkvm.h > +++ b/arch/arm64/include/asm/kvm_pkvm.h > @@ -56,10 +56,11 @@ static inline unsigned long hyp_vm_table_pages(void) > > static inline unsigned long __hyp_pgtable_max_pages(unsigned long nr_pages) > { > - unsigned long total = 0, i; > + unsigned long total = 0; > + int i; > > /* Provision the worst case scenario */ > - for (i = 0; i < KVM_PGTABLE_MAX_LEVELS; i++) { > + for (i = KVM_PGTABLE_FIRST_LEVEL; i <= KVM_PGTABLE_LAST_LEVEL; i++) { > nr_pages = DIV_ROUND_UP(nr_pages, PTRS_PER_PTE); > total += nr_pages; > } > diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > index 9d703441278b..2cfb6352a8ea 100644 > --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c > +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > @@ -91,7 +91,7 @@ static void host_s2_put_page(void *addr) > hyp_put_page(&host_s2_pool, addr); > } > > -static void host_s2_free_unlinked_table(void *addr, u32 level) > +static void host_s2_free_unlinked_table(void *addr, s8 level) > { > kvm_pgtable_stage2_free_unlinked(&host_mmu.mm_ops, addr, level); > } > @@ -443,7 +443,7 @@ static int host_stage2_adjust_range(u64 addr, struct kvm_mem_range *range) > { > struct kvm_mem_range cur; > kvm_pte_t pte; > - u32 level; > + s8 level; > int ret; > > hyp_assert_lock_held(&host_mmu.lock); > @@ -462,7 +462,7 @@ static int host_stage2_adjust_range(u64 addr, struct kvm_mem_range *range) > cur.start = ALIGN_DOWN(addr, granule); > cur.end = cur.start + granule; > level++; > - } while ((level < KVM_PGTABLE_MAX_LEVELS) && > + } while ((level <= KVM_PGTABLE_LAST_LEVEL) && > !(kvm_level_supports_block_mapping(level) && > range_included(&cur, range))); > > diff --git a/arch/arm64/kvm/hyp/nvhe/mm.c b/arch/arm64/kvm/hyp/nvhe/mm.c > index 65a7a186d7b2..b01a3d1078a8 100644 > --- a/arch/arm64/kvm/hyp/nvhe/mm.c > +++ b/arch/arm64/kvm/hyp/nvhe/mm.c > @@ -260,7 +260,7 @@ static void fixmap_clear_slot(struct hyp_fixmap_slot *slot) > * https://lore.kernel.org/kvm/20221017115209.2099-1-will@kernel.org/T/#mf10dfbaf1eaef9274c581b81c53758918c1d0f03 > */ > dsb(ishst); > - __tlbi_level(vale2is, __TLBI_VADDR(addr, 0), (KVM_PGTABLE_MAX_LEVELS - 1)); > + __tlbi_level(vale2is, __TLBI_VADDR(addr, 0), KVM_PGTABLE_LAST_LEVEL); > dsb(ish); > isb(); > } > @@ -275,7 +275,7 @@ static int __create_fixmap_slot_cb(const struct kvm_pgtable_visit_ctx *ctx, > { > struct hyp_fixmap_slot *slot = per_cpu_ptr(&fixmap_slots, (u64)ctx->arg); > > - if (!kvm_pte_valid(ctx->old) || ctx->level != KVM_PGTABLE_MAX_LEVELS - 1) > + if (!kvm_pte_valid(ctx->old) || ctx->level != KVM_PGTABLE_LAST_LEVEL) > return -EINVAL; > > slot->addr = ctx->addr; > diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c > index 0d5e0a89ddce..bc58d1b515af 100644 > --- a/arch/arm64/kvm/hyp/nvhe/setup.c > +++ b/arch/arm64/kvm/hyp/nvhe/setup.c > @@ -181,7 +181,7 @@ static int fix_host_ownership_walker(const struct kvm_pgtable_visit_ctx *ctx, > if (!kvm_pte_valid(ctx->old)) > return 0; > > - if (ctx->level != (KVM_PGTABLE_MAX_LEVELS - 1)) > + if (ctx->level != KVM_PGTABLE_LAST_LEVEL) > return -EINVAL; > > phys = kvm_pte_to_phys(ctx->old); > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > index 062eb7bcdb8a..8e79ff6972ce 100644 > --- a/arch/arm64/kvm/hyp/pgtable.c > +++ b/arch/arm64/kvm/hyp/pgtable.c > @@ -101,7 +101,7 @@ static bool kvm_block_mapping_supported(const struct kvm_pgtable_visit_ctx *ctx, > return IS_ALIGNED(ctx->addr, granule); > } > > -static u32 kvm_pgtable_idx(struct kvm_pgtable_walk_data *data, u32 level) > +static u32 kvm_pgtable_idx(struct kvm_pgtable_walk_data *data, s8 level) > { > u64 shift = kvm_granule_shift(level); > u64 mask = BIT(PAGE_SHIFT - 3) - 1; > @@ -117,7 +117,7 @@ static u32 kvm_pgd_page_idx(struct kvm_pgtable *pgt, u64 addr) > return (addr & mask) >> shift; > } > > -static u32 kvm_pgd_pages(u32 ia_bits, u32 start_level) > +static u32 kvm_pgd_pages(u32 ia_bits, s8 start_level) > { > struct kvm_pgtable pgt = { > .ia_bits = ia_bits, > @@ -127,9 +127,9 @@ static u32 kvm_pgd_pages(u32 ia_bits, u32 start_level) > return kvm_pgd_page_idx(&pgt, -1ULL) + 1; > } > > -static bool kvm_pte_table(kvm_pte_t pte, u32 level) > +static bool kvm_pte_table(kvm_pte_t pte, s8 level) > { > - if (level == KVM_PGTABLE_MAX_LEVELS - 1) > + if (level == KVM_PGTABLE_LAST_LEVEL) > return false; > > if (!kvm_pte_valid(pte)) > @@ -157,11 +157,11 @@ static kvm_pte_t kvm_init_table_pte(kvm_pte_t *childp, struct kvm_pgtable_mm_ops > return pte; > } > > -static kvm_pte_t kvm_init_valid_leaf_pte(u64 pa, kvm_pte_t attr, u32 level) > +static kvm_pte_t kvm_init_valid_leaf_pte(u64 pa, kvm_pte_t attr, s8 level) > { > kvm_pte_t pte = kvm_phys_to_pte(pa); > - u64 type = (level == KVM_PGTABLE_MAX_LEVELS - 1) ? KVM_PTE_TYPE_PAGE : > - KVM_PTE_TYPE_BLOCK; > + u64 type = (level == KVM_PGTABLE_LAST_LEVEL) ? KVM_PTE_TYPE_PAGE : > + KVM_PTE_TYPE_BLOCK; > > pte |= attr & (KVM_PTE_LEAF_ATTR_LO | KVM_PTE_LEAF_ATTR_HI); > pte |= FIELD_PREP(KVM_PTE_TYPE, type); > @@ -206,11 +206,11 @@ static bool kvm_pgtable_walk_continue(const struct kvm_pgtable_walker *walker, > } > > static int __kvm_pgtable_walk(struct kvm_pgtable_walk_data *data, > - struct kvm_pgtable_mm_ops *mm_ops, kvm_pteref_t pgtable, u32 level); > + struct kvm_pgtable_mm_ops *mm_ops, kvm_pteref_t pgtable, s8 level); > > static inline int __kvm_pgtable_visit(struct kvm_pgtable_walk_data *data, > struct kvm_pgtable_mm_ops *mm_ops, > - kvm_pteref_t pteref, u32 level) > + kvm_pteref_t pteref, s8 level) > { > enum kvm_pgtable_walk_flags flags = data->walker->flags; > kvm_pte_t *ptep = kvm_dereference_pteref(data->walker, pteref); > @@ -275,12 +275,12 @@ static inline int __kvm_pgtable_visit(struct kvm_pgtable_walk_data *data, > } > > static int __kvm_pgtable_walk(struct kvm_pgtable_walk_data *data, > - struct kvm_pgtable_mm_ops *mm_ops, kvm_pteref_t pgtable, u32 level) > + struct kvm_pgtable_mm_ops *mm_ops, kvm_pteref_t pgtable, s8 level) > { > u32 idx; > int ret = 0; > > - if (WARN_ON_ONCE(level >= KVM_PGTABLE_MAX_LEVELS)) > + if (WARN_ON_ONCE(level > KVM_PGTABLE_LAST_LEVEL)) > return -EINVAL; Now that level can be negative, you may want to check it against KVM_PGTABLE_FIRST_LEVEL as well. > > for (idx = kvm_pgtable_idx(data, level); idx < PTRS_PER_PTE; ++idx) { > @@ -343,7 +343,7 @@ int kvm_pgtable_walk(struct kvm_pgtable *pgt, u64 addr, u64 size, > > struct leaf_walk_data { > kvm_pte_t pte; > - u32 level; > + s8 level; > }; > > static int leaf_walker(const struct kvm_pgtable_visit_ctx *ctx, > @@ -358,7 +358,7 @@ static int leaf_walker(const struct kvm_pgtable_visit_ctx *ctx, > } > > int kvm_pgtable_get_leaf(struct kvm_pgtable *pgt, u64 addr, > - kvm_pte_t *ptep, u32 *level) > + kvm_pte_t *ptep, s8 *level) > { > struct leaf_walk_data data; > struct kvm_pgtable_walker walker = { > @@ -471,7 +471,7 @@ static int hyp_map_walker(const struct kvm_pgtable_visit_ctx *ctx, > if (hyp_map_walker_try_leaf(ctx, data)) > return 0; > > - if (WARN_ON(ctx->level == KVM_PGTABLE_MAX_LEVELS - 1)) > + if (WARN_ON(ctx->level == KVM_PGTABLE_LAST_LEVEL)) > return -EINVAL; Same thing. > > childp = (kvm_pte_t *)mm_ops->zalloc_page(NULL); > @@ -567,14 +567,18 @@ u64 kvm_pgtable_hyp_unmap(struct kvm_pgtable *pgt, u64 addr, u64 size) > int kvm_pgtable_hyp_init(struct kvm_pgtable *pgt, u32 va_bits, > struct kvm_pgtable_mm_ops *mm_ops) > { > - u64 levels = ARM64_HW_PGTABLE_LEVELS(va_bits); > + s8 start_level = KVM_PGTABLE_LAST_LEVEL + 1 - > + ARM64_HW_PGTABLE_LEVELS(va_bits); > + if (start_level < KVM_PGTABLE_FIRST_LEVEL || > + start_level > KVM_PGTABLE_LAST_LEVEL) > + return -EINVAL; Please add a new line between the variable definition and the if () statement. > > pgt->pgd = (kvm_pteref_t)mm_ops->zalloc_page(NULL); > if (!pgt->pgd) > return -ENOMEM; > > pgt->ia_bits = va_bits; > - pgt->start_level = KVM_PGTABLE_MAX_LEVELS - levels; > + pgt->start_level = start_level; > pgt->mm_ops = mm_ops; > pgt->mmu = NULL; > pgt->force_pte_cb = NULL; > @@ -628,7 +632,7 @@ struct stage2_map_data { > u64 kvm_get_vtcr(u64 mmfr0, u64 mmfr1, u32 phys_shift) > { > u64 vtcr = VTCR_EL2_FLAGS; > - u8 lvls; > + s8 lvls; > > vtcr |= kvm_get_parange(mmfr0) << VTCR_EL2_PS_SHIFT; > vtcr |= VTCR_EL2_T0SZ(phys_shift); > @@ -911,7 +915,7 @@ static bool stage2_leaf_mapping_allowed(const struct kvm_pgtable_visit_ctx *ctx, > { > u64 phys = stage2_map_walker_phys_addr(ctx, data); > > - if (data->force_pte && (ctx->level < (KVM_PGTABLE_MAX_LEVELS - 1))) > + if (data->force_pte && ctx->level < KVM_PGTABLE_LAST_LEVEL) > return false; > > return kvm_block_mapping_supported(ctx, phys); > @@ -990,7 +994,7 @@ static int stage2_map_walk_leaf(const struct kvm_pgtable_visit_ctx *ctx, > if (ret != -E2BIG) > return ret; > > - if (WARN_ON(ctx->level == KVM_PGTABLE_MAX_LEVELS - 1)) > + if (WARN_ON(ctx->level == KVM_PGTABLE_LAST_LEVEL)) > return -EINVAL; > > if (!data->memcache) > @@ -1160,7 +1164,7 @@ struct stage2_attr_data { > kvm_pte_t attr_set; > kvm_pte_t attr_clr; > kvm_pte_t pte; > - u32 level; > + s8 level; > }; > > static int stage2_attr_walker(const struct kvm_pgtable_visit_ctx *ctx, > @@ -1203,7 +1207,7 @@ static int stage2_attr_walker(const struct kvm_pgtable_visit_ctx *ctx, > static int stage2_update_leaf_attrs(struct kvm_pgtable *pgt, u64 addr, > u64 size, kvm_pte_t attr_set, > kvm_pte_t attr_clr, kvm_pte_t *orig_pte, > - u32 *level, enum kvm_pgtable_walk_flags flags) > + s8 *level, enum kvm_pgtable_walk_flags flags) > { > int ret; > kvm_pte_t attr_mask = KVM_PTE_LEAF_ATTR_LO | KVM_PTE_LEAF_ATTR_HI; > @@ -1305,7 +1309,7 @@ int kvm_pgtable_stage2_relax_perms(struct kvm_pgtable *pgt, u64 addr, > enum kvm_pgtable_prot prot) > { > int ret; > - u32 level; > + s8 level; > kvm_pte_t set = 0, clr = 0; > > if (prot & KVM_PTE_LEAF_ATTR_HI_SW) > @@ -1358,7 +1362,7 @@ int kvm_pgtable_stage2_flush(struct kvm_pgtable *pgt, u64 addr, u64 size) > } > > kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt, > - u64 phys, u32 level, > + u64 phys, s8 level, > enum kvm_pgtable_prot prot, > void *mc, bool force_pte) > { > @@ -1416,7 +1420,7 @@ kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt, > * fully populated tree up to the PTE entries. Note that @level is > * interpreted as in "level @level entry". > */ > -static int stage2_block_get_nr_page_tables(u32 level) > +static int stage2_block_get_nr_page_tables(s8 level) > { > switch (level) { > case 1: > @@ -1427,7 +1431,7 @@ static int stage2_block_get_nr_page_tables(u32 level) > return 0; > default: > WARN_ON_ONCE(level < KVM_PGTABLE_MIN_BLOCK_LEVEL || > - level >= KVM_PGTABLE_MAX_LEVELS); > + level > KVM_PGTABLE_LAST_LEVEL); > return -EINVAL; > }; > } > @@ -1440,13 +1444,13 @@ static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx, > struct kvm_s2_mmu *mmu; > kvm_pte_t pte = ctx->old, new, *childp; > enum kvm_pgtable_prot prot; > - u32 level = ctx->level; > + s8 level = ctx->level; > bool force_pte; > int nr_pages; > u64 phys; > > /* No huge-pages exist at the last level */ > - if (level == KVM_PGTABLE_MAX_LEVELS - 1) > + if (level == KVM_PGTABLE_LAST_LEVEL) > return 0; > > /* We only split valid block mappings */ > @@ -1523,7 +1527,7 @@ int __kvm_pgtable_stage2_init(struct kvm_pgtable *pgt, struct kvm_s2_mmu *mmu, > u64 vtcr = mmu->arch->vtcr; > u32 ia_bits = VTCR_EL2_IPA(vtcr); > u32 sl0 = FIELD_GET(VTCR_EL2_SL0_MASK, vtcr); > - u32 start_level = VTCR_EL2_TGRAN_SL0_BASE - sl0; > + s8 start_level = VTCR_EL2_TGRAN_SL0_BASE - sl0; > > pgd_sz = kvm_pgd_pages(ia_bits, start_level) * PAGE_SIZE; > pgt->pgd = (kvm_pteref_t)mm_ops->zalloc_pages_exact(pgd_sz); > @@ -1546,7 +1550,7 @@ size_t kvm_pgtable_stage2_pgd_size(u64 vtcr) > { > u32 ia_bits = VTCR_EL2_IPA(vtcr); > u32 sl0 = FIELD_GET(VTCR_EL2_SL0_MASK, vtcr); > - u32 start_level = VTCR_EL2_TGRAN_SL0_BASE - sl0; > + s8 start_level = VTCR_EL2_TGRAN_SL0_BASE - sl0; > > return kvm_pgd_pages(ia_bits, start_level) * PAGE_SIZE; > } > @@ -1582,7 +1586,7 @@ void kvm_pgtable_stage2_destroy(struct kvm_pgtable *pgt) > pgt->pgd = NULL; > } > > -void kvm_pgtable_stage2_free_unlinked(struct kvm_pgtable_mm_ops *mm_ops, void *pgtable, u32 level) > +void kvm_pgtable_stage2_free_unlinked(struct kvm_pgtable_mm_ops *mm_ops, void *pgtable, s8 level) > { > kvm_pteref_t ptep = (kvm_pteref_t)pgtable; > struct kvm_pgtable_walker walker = { > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index 482280fe22d7..73110ba3624c 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -223,12 +223,12 @@ static void stage2_free_unlinked_table_rcu_cb(struct rcu_head *head) > { > struct page *page = container_of(head, struct page, rcu_head); > void *pgtable = page_to_virt(page); > - u32 level = page_private(page); > + s8 level = page_private(page); > > kvm_pgtable_stage2_free_unlinked(&kvm_s2_mm_ops, pgtable, level); > } > > -static void stage2_free_unlinked_table(void *addr, u32 level) > +static void stage2_free_unlinked_table(void *addr, s8 level) > { > struct page *page = virt_to_page(addr); > > @@ -804,13 +804,13 @@ static int get_user_mapping_size(struct kvm *kvm, u64 addr) > struct kvm_pgtable pgt = { > .pgd = (kvm_pteref_t)kvm->mm->pgd, > .ia_bits = vabits_actual, > - .start_level = (KVM_PGTABLE_MAX_LEVELS - > - CONFIG_PGTABLE_LEVELS), > + .start_level = (KVM_PGTABLE_LAST_LEVEL - > + CONFIG_PGTABLE_LEVELS + 1), > .mm_ops = &kvm_user_mm_ops, > }; > unsigned long flags; > kvm_pte_t pte = 0; /* Keep GCC quiet... */ > - u32 level = ~0; > + s8 level = ~0; Well, that's a semantic difference. ~0 == -1, which is a valid level, while the original code was trying to initialise level to something invalid. On the bright side, this function is going away in 6.7... > int ret; > > /* > @@ -829,7 +829,9 @@ static int get_user_mapping_size(struct kvm *kvm, u64 addr) > * Not seeing an error, but not updating level? Something went > * deeply wrong... > */ > - if (WARN_ON(level >= KVM_PGTABLE_MAX_LEVELS)) > + if (WARN_ON(level > KVM_PGTABLE_LAST_LEVEL)) > + return -EFAULT; > + if (WARN_ON(level < KVM_PGTABLE_FIRST_LEVEL)) > return -EFAULT; > > /* Oops, the userspace PTs are gone... Replay the fault */ > @@ -1407,7 +1409,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > gfn_t gfn; > kvm_pfn_t pfn; > bool logging_active = memslot_is_logging(memslot); > - unsigned long fault_level = kvm_vcpu_trap_get_fault_level(vcpu); > + s8 fault_level = kvm_vcpu_trap_get_fault_level(vcpu); > long vma_pagesize, fault_granule; > enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R; > struct kvm_pgtable *pgt; Thanks, M. -- Without deviation from the norm, progress is not possible. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 58DA0C001DF for ; Fri, 20 Oct 2023 10:42:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Fnfo1FTNHaMjcbfzO4GGEA46royze+q+zyJaQdlHmaE=; b=N31kgsPrQBCZFL qjm67xxPYVQk3fCvRFSR3+GOHWKAmFIOFbeszp3imi70t5BZfvCVN+Yy9QbLhH3+TuzTdaCGW7s61 rhbxMvmcmA8psD3oNCDC9/+zAOojDMJ0Ig7EzRJwuQ3OgdDeHGjg4vL58H7S6vqRcGQUhGAmn2SjQ vPxvwKksFFFpcxZ/JI2XeFdznghQCTOev0Yx8Vmbb9kGJDqFJ/X0fLD0br3EBVcO4To3OhXC9A53K JGxRjPdCCPigttwWUDGvx5VQQtT7yoEQL2IsG0XxJATEhGMpUMwQu2K74EXl2QJ2HUEhC0Vrs58lb Vnv5w++JfhmAQ2E4otFQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qtmxT-001ow8-2K; Fri, 20 Oct 2023 10:42:23 +0000 Received: from ams.source.kernel.org ([2604:1380:4601:e00::1]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qtmxO-001ouM-2q for linux-arm-kernel@lists.infradead.org; Fri, 20 Oct 2023 10:42:22 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by ams.source.kernel.org (Postfix) with ESMTP id 1DF44B83071; Fri, 20 Oct 2023 10:42:17 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2B785C433C7; Fri, 20 Oct 2023 10:42:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1697798536; bh=i3Z44WfVde9QeZ+ipeoAFQQufo90upOHL6pzPAJToEM=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=Qkg4xe/98ecIpW2lLesKSo1lBU3XqzFfxin5wnzg2ElfieeRRVSrihnI2B7XCp5RT G/JapF51ntpFWPonEhauLZg9X1ncn0zfT59ttryLS/hltLpMSXpOw9iSW3C3ctt0jB ly1AvW9ZcuR/Fz6RZ/4RzYU+5+QuvfuW759s3wPX07fMquOhxRkHBBeS85ZWrBWNRq 3RwQIzuF8UH435s0tiQcc7J/oqQrhwSA0AgOiWoP5cwtxFJNXbeWUCCSe1CsTBulJb vi3InboJd39fIVdgbqa8rufueRQE1mugCUjyHjjRnwvZQ0GuwavN1doBIzdXVggrvw 9zSzctq40xAYw== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1qtmxJ-0065Q7-T0; Fri, 20 Oct 2023 11:42:14 +0100 Date: Fri, 20 Oct 2023 11:42:12 +0100 Message-ID: <868r7xmv7f.wl-maz@kernel.org> From: Marc Zyngier To: Ryan Roberts Cc: Catalin Marinas , Will Deacon , Oliver Upton , Suzuki K Poulose , James Morse , Zenghui Yu , Ard Biesheuvel , Anshuman Khandual , linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev Subject: Re: [PATCH v4 08/12] KVM: arm64: Convert translation level parameter to s8 In-Reply-To: <20231009185008.3803879-9-ryan.roberts@arm.com> References: <20231009185008.3803879-1-ryan.roberts@arm.com> <20231009185008.3803879-9-ryan.roberts@arm.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: ryan.roberts@arm.com, catalin.marinas@arm.com, will@kernel.org, oliver.upton@linux.dev, suzuki.poulose@arm.com, james.morse@arm.com, yuzenghui@huawei.com, ardb@kernel.org, anshuman.khandual@arm.com, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231020_034219_205352_1FAFC724 X-CRM114-Status: GOOD ( 40.11 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, 09 Oct 2023 19:50:04 +0100, Ryan Roberts wrote: > > With the introduction of FEAT_LPA2, the Arm ARM adds a new level of > translation, level -1, so levels can now be in the range [-1;3]. 3 is > always the last level and the first level is determined based on the > number of VA bits in use. > > Convert level variables to use a signed type in preparation for > supporting this new level -1. > > Since the last level is always anchored at 3, and the first level varies > to suit the number of VA/IPA bits, take the opportunity to replace > KVM_PGTABLE_MAX_LEVELS with the 2 macros KVM_PGTABLE_FIRST_LEVEL and > KVM_PGTABLE_LAST_LEVEL. This removes the assumption from the code that > levels run from 0 to KVM_PGTABLE_MAX_LEVELS - 1, which will soon no > longer be true. > > No behavioral changes intended. Shrug. Unless you have compared the binaries before and after and proven that they are strictly identical, there will be behaviour changes, intended or otherwise. I know what you're trying to convey, but I've seen so many patches carrying a sentence of this sort and yet turning the kernel on its head that I've become allergic to it. Sorry. > > Signed-off-by: Ryan Roberts > --- > arch/arm64/include/asm/kvm_emulate.h | 2 +- > arch/arm64/include/asm/kvm_pgtable.h | 31 ++++++------- > arch/arm64/include/asm/kvm_pkvm.h | 5 ++- > arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 +-- > arch/arm64/kvm/hyp/nvhe/mm.c | 4 +- > arch/arm64/kvm/hyp/nvhe/setup.c | 2 +- > arch/arm64/kvm/hyp/pgtable.c | 64 ++++++++++++++------------- > arch/arm64/kvm/mmu.c | 16 ++++--- > 8 files changed, 69 insertions(+), 61 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index 3d6725ff0bf6..bf3ef66eb51f 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -404,7 +404,7 @@ static __always_inline u8 kvm_vcpu_trap_get_fault_type(const struct kvm_vcpu *vc > return kvm_vcpu_get_esr(vcpu) & ESR_ELx_FSC_TYPE; > } > > -static __always_inline u8 kvm_vcpu_trap_get_fault_level(const struct kvm_vcpu *vcpu) > +static __always_inline s8 kvm_vcpu_trap_get_fault_level(const struct kvm_vcpu *vcpu) > { > return kvm_vcpu_get_esr(vcpu) & ESR_ELx_FSC_LEVEL; > } > diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h > index b240158e1218..c61bb9709201 100644 > --- a/arch/arm64/include/asm/kvm_pgtable.h > +++ b/arch/arm64/include/asm/kvm_pgtable.h > @@ -11,7 +11,8 @@ > #include > #include > > -#define KVM_PGTABLE_MAX_LEVELS 4U > +#define KVM_PGTABLE_FIRST_LEVEL 0 > +#define KVM_PGTABLE_LAST_LEVEL 3 > > /* > * The largest supported block sizes for KVM (no 52-bit PA support): > @@ -20,9 +21,9 @@ > * - 64K (level 2): 512MB > */ > #ifdef CONFIG_ARM64_4K_PAGES > -#define KVM_PGTABLE_MIN_BLOCK_LEVEL 1U > +#define KVM_PGTABLE_MIN_BLOCK_LEVEL 1 > #else > -#define KVM_PGTABLE_MIN_BLOCK_LEVEL 2U > +#define KVM_PGTABLE_MIN_BLOCK_LEVEL 2 > #endif > > static inline u64 kvm_get_parange_max(void) > @@ -101,28 +102,28 @@ static inline kvm_pfn_t kvm_pte_to_pfn(kvm_pte_t pte) > return __phys_to_pfn(kvm_pte_to_phys(pte)); > } > > -static inline u64 kvm_granule_shift(u32 level) > +static inline u64 kvm_granule_shift(s8 level) > { > - /* Assumes KVM_PGTABLE_MAX_LEVELS is 4 */ > + /* Assumes KVM_PGTABLE_LAST_LEVEL is 3 */ > return ARM64_HW_PGTABLE_LEVEL_SHIFT(level); I'm amazed that the macro tolerates a negative level, but it really does. > } > > -static inline u64 kvm_granule_size(u32 level) > +static inline u64 kvm_granule_size(s8 level) > { > return BIT(kvm_granule_shift(level)); > } > > -static inline bool kvm_level_supports_block_mapping(u32 level) > +static inline bool kvm_level_supports_block_mapping(s8 level) > { > return level >= KVM_PGTABLE_MIN_BLOCK_LEVEL; > } > > static inline u32 kvm_supported_block_sizes(void) > { > - u32 level = KVM_PGTABLE_MIN_BLOCK_LEVEL; > + s8 level = KVM_PGTABLE_MIN_BLOCK_LEVEL; > u32 r = 0; > > - for (; level < KVM_PGTABLE_MAX_LEVELS; level++) > + for (; level <= KVM_PGTABLE_LAST_LEVEL; level++) > r |= BIT(kvm_granule_shift(level)); > > return r; > @@ -167,7 +168,7 @@ struct kvm_pgtable_mm_ops { > void* (*zalloc_page)(void *arg); > void* (*zalloc_pages_exact)(size_t size); > void (*free_pages_exact)(void *addr, size_t size); > - void (*free_unlinked_table)(void *addr, u32 level); > + void (*free_unlinked_table)(void *addr, s8 level); > void (*get_page)(void *addr); > void (*put_page)(void *addr); > int (*page_count)(void *addr); > @@ -263,7 +264,7 @@ struct kvm_pgtable_visit_ctx { > u64 start; > u64 addr; > u64 end; > - u32 level; > + s8 level; > enum kvm_pgtable_walk_flags flags; > }; > > @@ -366,7 +367,7 @@ static inline bool kvm_pgtable_walk_lock_held(void) > */ > struct kvm_pgtable { > u32 ia_bits; > - u32 start_level; > + s8 start_level; > kvm_pteref_t pgd; > struct kvm_pgtable_mm_ops *mm_ops; > > @@ -500,7 +501,7 @@ void kvm_pgtable_stage2_destroy(struct kvm_pgtable *pgt); > * The page-table is assumed to be unreachable by any hardware walkers prior to > * freeing and therefore no TLB invalidation is performed. > */ > -void kvm_pgtable_stage2_free_unlinked(struct kvm_pgtable_mm_ops *mm_ops, void *pgtable, u32 level); > +void kvm_pgtable_stage2_free_unlinked(struct kvm_pgtable_mm_ops *mm_ops, void *pgtable, s8 level); > > /** > * kvm_pgtable_stage2_create_unlinked() - Create an unlinked stage-2 paging structure. > @@ -524,7 +525,7 @@ void kvm_pgtable_stage2_free_unlinked(struct kvm_pgtable_mm_ops *mm_ops, void *p > * an ERR_PTR(error) on failure. > */ > kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt, > - u64 phys, u32 level, > + u64 phys, s8 level, > enum kvm_pgtable_prot prot, > void *mc, bool force_pte); > > @@ -750,7 +751,7 @@ int kvm_pgtable_walk(struct kvm_pgtable *pgt, u64 addr, u64 size, > * Return: 0 on success, negative error code on failure. > */ > int kvm_pgtable_get_leaf(struct kvm_pgtable *pgt, u64 addr, > - kvm_pte_t *ptep, u32 *level); > + kvm_pte_t *ptep, s8 *level); > > /** > * kvm_pgtable_stage2_pte_prot() - Retrieve the protection attributes of a > diff --git a/arch/arm64/include/asm/kvm_pkvm.h b/arch/arm64/include/asm/kvm_pkvm.h > index e46250a02017..ad9cfb5c1ff4 100644 > --- a/arch/arm64/include/asm/kvm_pkvm.h > +++ b/arch/arm64/include/asm/kvm_pkvm.h > @@ -56,10 +56,11 @@ static inline unsigned long hyp_vm_table_pages(void) > > static inline unsigned long __hyp_pgtable_max_pages(unsigned long nr_pages) > { > - unsigned long total = 0, i; > + unsigned long total = 0; > + int i; > > /* Provision the worst case scenario */ > - for (i = 0; i < KVM_PGTABLE_MAX_LEVELS; i++) { > + for (i = KVM_PGTABLE_FIRST_LEVEL; i <= KVM_PGTABLE_LAST_LEVEL; i++) { > nr_pages = DIV_ROUND_UP(nr_pages, PTRS_PER_PTE); > total += nr_pages; > } > diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > index 9d703441278b..2cfb6352a8ea 100644 > --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c > +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > @@ -91,7 +91,7 @@ static void host_s2_put_page(void *addr) > hyp_put_page(&host_s2_pool, addr); > } > > -static void host_s2_free_unlinked_table(void *addr, u32 level) > +static void host_s2_free_unlinked_table(void *addr, s8 level) > { > kvm_pgtable_stage2_free_unlinked(&host_mmu.mm_ops, addr, level); > } > @@ -443,7 +443,7 @@ static int host_stage2_adjust_range(u64 addr, struct kvm_mem_range *range) > { > struct kvm_mem_range cur; > kvm_pte_t pte; > - u32 level; > + s8 level; > int ret; > > hyp_assert_lock_held(&host_mmu.lock); > @@ -462,7 +462,7 @@ static int host_stage2_adjust_range(u64 addr, struct kvm_mem_range *range) > cur.start = ALIGN_DOWN(addr, granule); > cur.end = cur.start + granule; > level++; > - } while ((level < KVM_PGTABLE_MAX_LEVELS) && > + } while ((level <= KVM_PGTABLE_LAST_LEVEL) && > !(kvm_level_supports_block_mapping(level) && > range_included(&cur, range))); > > diff --git a/arch/arm64/kvm/hyp/nvhe/mm.c b/arch/arm64/kvm/hyp/nvhe/mm.c > index 65a7a186d7b2..b01a3d1078a8 100644 > --- a/arch/arm64/kvm/hyp/nvhe/mm.c > +++ b/arch/arm64/kvm/hyp/nvhe/mm.c > @@ -260,7 +260,7 @@ static void fixmap_clear_slot(struct hyp_fixmap_slot *slot) > * https://lore.kernel.org/kvm/20221017115209.2099-1-will@kernel.org/T/#mf10dfbaf1eaef9274c581b81c53758918c1d0f03 > */ > dsb(ishst); > - __tlbi_level(vale2is, __TLBI_VADDR(addr, 0), (KVM_PGTABLE_MAX_LEVELS - 1)); > + __tlbi_level(vale2is, __TLBI_VADDR(addr, 0), KVM_PGTABLE_LAST_LEVEL); > dsb(ish); > isb(); > } > @@ -275,7 +275,7 @@ static int __create_fixmap_slot_cb(const struct kvm_pgtable_visit_ctx *ctx, > { > struct hyp_fixmap_slot *slot = per_cpu_ptr(&fixmap_slots, (u64)ctx->arg); > > - if (!kvm_pte_valid(ctx->old) || ctx->level != KVM_PGTABLE_MAX_LEVELS - 1) > + if (!kvm_pte_valid(ctx->old) || ctx->level != KVM_PGTABLE_LAST_LEVEL) > return -EINVAL; > > slot->addr = ctx->addr; > diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c > index 0d5e0a89ddce..bc58d1b515af 100644 > --- a/arch/arm64/kvm/hyp/nvhe/setup.c > +++ b/arch/arm64/kvm/hyp/nvhe/setup.c > @@ -181,7 +181,7 @@ static int fix_host_ownership_walker(const struct kvm_pgtable_visit_ctx *ctx, > if (!kvm_pte_valid(ctx->old)) > return 0; > > - if (ctx->level != (KVM_PGTABLE_MAX_LEVELS - 1)) > + if (ctx->level != KVM_PGTABLE_LAST_LEVEL) > return -EINVAL; > > phys = kvm_pte_to_phys(ctx->old); > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > index 062eb7bcdb8a..8e79ff6972ce 100644 > --- a/arch/arm64/kvm/hyp/pgtable.c > +++ b/arch/arm64/kvm/hyp/pgtable.c > @@ -101,7 +101,7 @@ static bool kvm_block_mapping_supported(const struct kvm_pgtable_visit_ctx *ctx, > return IS_ALIGNED(ctx->addr, granule); > } > > -static u32 kvm_pgtable_idx(struct kvm_pgtable_walk_data *data, u32 level) > +static u32 kvm_pgtable_idx(struct kvm_pgtable_walk_data *data, s8 level) > { > u64 shift = kvm_granule_shift(level); > u64 mask = BIT(PAGE_SHIFT - 3) - 1; > @@ -117,7 +117,7 @@ static u32 kvm_pgd_page_idx(struct kvm_pgtable *pgt, u64 addr) > return (addr & mask) >> shift; > } > > -static u32 kvm_pgd_pages(u32 ia_bits, u32 start_level) > +static u32 kvm_pgd_pages(u32 ia_bits, s8 start_level) > { > struct kvm_pgtable pgt = { > .ia_bits = ia_bits, > @@ -127,9 +127,9 @@ static u32 kvm_pgd_pages(u32 ia_bits, u32 start_level) > return kvm_pgd_page_idx(&pgt, -1ULL) + 1; > } > > -static bool kvm_pte_table(kvm_pte_t pte, u32 level) > +static bool kvm_pte_table(kvm_pte_t pte, s8 level) > { > - if (level == KVM_PGTABLE_MAX_LEVELS - 1) > + if (level == KVM_PGTABLE_LAST_LEVEL) > return false; > > if (!kvm_pte_valid(pte)) > @@ -157,11 +157,11 @@ static kvm_pte_t kvm_init_table_pte(kvm_pte_t *childp, struct kvm_pgtable_mm_ops > return pte; > } > > -static kvm_pte_t kvm_init_valid_leaf_pte(u64 pa, kvm_pte_t attr, u32 level) > +static kvm_pte_t kvm_init_valid_leaf_pte(u64 pa, kvm_pte_t attr, s8 level) > { > kvm_pte_t pte = kvm_phys_to_pte(pa); > - u64 type = (level == KVM_PGTABLE_MAX_LEVELS - 1) ? KVM_PTE_TYPE_PAGE : > - KVM_PTE_TYPE_BLOCK; > + u64 type = (level == KVM_PGTABLE_LAST_LEVEL) ? KVM_PTE_TYPE_PAGE : > + KVM_PTE_TYPE_BLOCK; > > pte |= attr & (KVM_PTE_LEAF_ATTR_LO | KVM_PTE_LEAF_ATTR_HI); > pte |= FIELD_PREP(KVM_PTE_TYPE, type); > @@ -206,11 +206,11 @@ static bool kvm_pgtable_walk_continue(const struct kvm_pgtable_walker *walker, > } > > static int __kvm_pgtable_walk(struct kvm_pgtable_walk_data *data, > - struct kvm_pgtable_mm_ops *mm_ops, kvm_pteref_t pgtable, u32 level); > + struct kvm_pgtable_mm_ops *mm_ops, kvm_pteref_t pgtable, s8 level); > > static inline int __kvm_pgtable_visit(struct kvm_pgtable_walk_data *data, > struct kvm_pgtable_mm_ops *mm_ops, > - kvm_pteref_t pteref, u32 level) > + kvm_pteref_t pteref, s8 level) > { > enum kvm_pgtable_walk_flags flags = data->walker->flags; > kvm_pte_t *ptep = kvm_dereference_pteref(data->walker, pteref); > @@ -275,12 +275,12 @@ static inline int __kvm_pgtable_visit(struct kvm_pgtable_walk_data *data, > } > > static int __kvm_pgtable_walk(struct kvm_pgtable_walk_data *data, > - struct kvm_pgtable_mm_ops *mm_ops, kvm_pteref_t pgtable, u32 level) > + struct kvm_pgtable_mm_ops *mm_ops, kvm_pteref_t pgtable, s8 level) > { > u32 idx; > int ret = 0; > > - if (WARN_ON_ONCE(level >= KVM_PGTABLE_MAX_LEVELS)) > + if (WARN_ON_ONCE(level > KVM_PGTABLE_LAST_LEVEL)) > return -EINVAL; Now that level can be negative, you may want to check it against KVM_PGTABLE_FIRST_LEVEL as well. > > for (idx = kvm_pgtable_idx(data, level); idx < PTRS_PER_PTE; ++idx) { > @@ -343,7 +343,7 @@ int kvm_pgtable_walk(struct kvm_pgtable *pgt, u64 addr, u64 size, > > struct leaf_walk_data { > kvm_pte_t pte; > - u32 level; > + s8 level; > }; > > static int leaf_walker(const struct kvm_pgtable_visit_ctx *ctx, > @@ -358,7 +358,7 @@ static int leaf_walker(const struct kvm_pgtable_visit_ctx *ctx, > } > > int kvm_pgtable_get_leaf(struct kvm_pgtable *pgt, u64 addr, > - kvm_pte_t *ptep, u32 *level) > + kvm_pte_t *ptep, s8 *level) > { > struct leaf_walk_data data; > struct kvm_pgtable_walker walker = { > @@ -471,7 +471,7 @@ static int hyp_map_walker(const struct kvm_pgtable_visit_ctx *ctx, > if (hyp_map_walker_try_leaf(ctx, data)) > return 0; > > - if (WARN_ON(ctx->level == KVM_PGTABLE_MAX_LEVELS - 1)) > + if (WARN_ON(ctx->level == KVM_PGTABLE_LAST_LEVEL)) > return -EINVAL; Same thing. > > childp = (kvm_pte_t *)mm_ops->zalloc_page(NULL); > @@ -567,14 +567,18 @@ u64 kvm_pgtable_hyp_unmap(struct kvm_pgtable *pgt, u64 addr, u64 size) > int kvm_pgtable_hyp_init(struct kvm_pgtable *pgt, u32 va_bits, > struct kvm_pgtable_mm_ops *mm_ops) > { > - u64 levels = ARM64_HW_PGTABLE_LEVELS(va_bits); > + s8 start_level = KVM_PGTABLE_LAST_LEVEL + 1 - > + ARM64_HW_PGTABLE_LEVELS(va_bits); > + if (start_level < KVM_PGTABLE_FIRST_LEVEL || > + start_level > KVM_PGTABLE_LAST_LEVEL) > + return -EINVAL; Please add a new line between the variable definition and the if () statement. > > pgt->pgd = (kvm_pteref_t)mm_ops->zalloc_page(NULL); > if (!pgt->pgd) > return -ENOMEM; > > pgt->ia_bits = va_bits; > - pgt->start_level = KVM_PGTABLE_MAX_LEVELS - levels; > + pgt->start_level = start_level; > pgt->mm_ops = mm_ops; > pgt->mmu = NULL; > pgt->force_pte_cb = NULL; > @@ -628,7 +632,7 @@ struct stage2_map_data { > u64 kvm_get_vtcr(u64 mmfr0, u64 mmfr1, u32 phys_shift) > { > u64 vtcr = VTCR_EL2_FLAGS; > - u8 lvls; > + s8 lvls; > > vtcr |= kvm_get_parange(mmfr0) << VTCR_EL2_PS_SHIFT; > vtcr |= VTCR_EL2_T0SZ(phys_shift); > @@ -911,7 +915,7 @@ static bool stage2_leaf_mapping_allowed(const struct kvm_pgtable_visit_ctx *ctx, > { > u64 phys = stage2_map_walker_phys_addr(ctx, data); > > - if (data->force_pte && (ctx->level < (KVM_PGTABLE_MAX_LEVELS - 1))) > + if (data->force_pte && ctx->level < KVM_PGTABLE_LAST_LEVEL) > return false; > > return kvm_block_mapping_supported(ctx, phys); > @@ -990,7 +994,7 @@ static int stage2_map_walk_leaf(const struct kvm_pgtable_visit_ctx *ctx, > if (ret != -E2BIG) > return ret; > > - if (WARN_ON(ctx->level == KVM_PGTABLE_MAX_LEVELS - 1)) > + if (WARN_ON(ctx->level == KVM_PGTABLE_LAST_LEVEL)) > return -EINVAL; > > if (!data->memcache) > @@ -1160,7 +1164,7 @@ struct stage2_attr_data { > kvm_pte_t attr_set; > kvm_pte_t attr_clr; > kvm_pte_t pte; > - u32 level; > + s8 level; > }; > > static int stage2_attr_walker(const struct kvm_pgtable_visit_ctx *ctx, > @@ -1203,7 +1207,7 @@ static int stage2_attr_walker(const struct kvm_pgtable_visit_ctx *ctx, > static int stage2_update_leaf_attrs(struct kvm_pgtable *pgt, u64 addr, > u64 size, kvm_pte_t attr_set, > kvm_pte_t attr_clr, kvm_pte_t *orig_pte, > - u32 *level, enum kvm_pgtable_walk_flags flags) > + s8 *level, enum kvm_pgtable_walk_flags flags) > { > int ret; > kvm_pte_t attr_mask = KVM_PTE_LEAF_ATTR_LO | KVM_PTE_LEAF_ATTR_HI; > @@ -1305,7 +1309,7 @@ int kvm_pgtable_stage2_relax_perms(struct kvm_pgtable *pgt, u64 addr, > enum kvm_pgtable_prot prot) > { > int ret; > - u32 level; > + s8 level; > kvm_pte_t set = 0, clr = 0; > > if (prot & KVM_PTE_LEAF_ATTR_HI_SW) > @@ -1358,7 +1362,7 @@ int kvm_pgtable_stage2_flush(struct kvm_pgtable *pgt, u64 addr, u64 size) > } > > kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt, > - u64 phys, u32 level, > + u64 phys, s8 level, > enum kvm_pgtable_prot prot, > void *mc, bool force_pte) > { > @@ -1416,7 +1420,7 @@ kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt, > * fully populated tree up to the PTE entries. Note that @level is > * interpreted as in "level @level entry". > */ > -static int stage2_block_get_nr_page_tables(u32 level) > +static int stage2_block_get_nr_page_tables(s8 level) > { > switch (level) { > case 1: > @@ -1427,7 +1431,7 @@ static int stage2_block_get_nr_page_tables(u32 level) > return 0; > default: > WARN_ON_ONCE(level < KVM_PGTABLE_MIN_BLOCK_LEVEL || > - level >= KVM_PGTABLE_MAX_LEVELS); > + level > KVM_PGTABLE_LAST_LEVEL); > return -EINVAL; > }; > } > @@ -1440,13 +1444,13 @@ static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx, > struct kvm_s2_mmu *mmu; > kvm_pte_t pte = ctx->old, new, *childp; > enum kvm_pgtable_prot prot; > - u32 level = ctx->level; > + s8 level = ctx->level; > bool force_pte; > int nr_pages; > u64 phys; > > /* No huge-pages exist at the last level */ > - if (level == KVM_PGTABLE_MAX_LEVELS - 1) > + if (level == KVM_PGTABLE_LAST_LEVEL) > return 0; > > /* We only split valid block mappings */ > @@ -1523,7 +1527,7 @@ int __kvm_pgtable_stage2_init(struct kvm_pgtable *pgt, struct kvm_s2_mmu *mmu, > u64 vtcr = mmu->arch->vtcr; > u32 ia_bits = VTCR_EL2_IPA(vtcr); > u32 sl0 = FIELD_GET(VTCR_EL2_SL0_MASK, vtcr); > - u32 start_level = VTCR_EL2_TGRAN_SL0_BASE - sl0; > + s8 start_level = VTCR_EL2_TGRAN_SL0_BASE - sl0; > > pgd_sz = kvm_pgd_pages(ia_bits, start_level) * PAGE_SIZE; > pgt->pgd = (kvm_pteref_t)mm_ops->zalloc_pages_exact(pgd_sz); > @@ -1546,7 +1550,7 @@ size_t kvm_pgtable_stage2_pgd_size(u64 vtcr) > { > u32 ia_bits = VTCR_EL2_IPA(vtcr); > u32 sl0 = FIELD_GET(VTCR_EL2_SL0_MASK, vtcr); > - u32 start_level = VTCR_EL2_TGRAN_SL0_BASE - sl0; > + s8 start_level = VTCR_EL2_TGRAN_SL0_BASE - sl0; > > return kvm_pgd_pages(ia_bits, start_level) * PAGE_SIZE; > } > @@ -1582,7 +1586,7 @@ void kvm_pgtable_stage2_destroy(struct kvm_pgtable *pgt) > pgt->pgd = NULL; > } > > -void kvm_pgtable_stage2_free_unlinked(struct kvm_pgtable_mm_ops *mm_ops, void *pgtable, u32 level) > +void kvm_pgtable_stage2_free_unlinked(struct kvm_pgtable_mm_ops *mm_ops, void *pgtable, s8 level) > { > kvm_pteref_t ptep = (kvm_pteref_t)pgtable; > struct kvm_pgtable_walker walker = { > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index 482280fe22d7..73110ba3624c 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -223,12 +223,12 @@ static void stage2_free_unlinked_table_rcu_cb(struct rcu_head *head) > { > struct page *page = container_of(head, struct page, rcu_head); > void *pgtable = page_to_virt(page); > - u32 level = page_private(page); > + s8 level = page_private(page); > > kvm_pgtable_stage2_free_unlinked(&kvm_s2_mm_ops, pgtable, level); > } > > -static void stage2_free_unlinked_table(void *addr, u32 level) > +static void stage2_free_unlinked_table(void *addr, s8 level) > { > struct page *page = virt_to_page(addr); > > @@ -804,13 +804,13 @@ static int get_user_mapping_size(struct kvm *kvm, u64 addr) > struct kvm_pgtable pgt = { > .pgd = (kvm_pteref_t)kvm->mm->pgd, > .ia_bits = vabits_actual, > - .start_level = (KVM_PGTABLE_MAX_LEVELS - > - CONFIG_PGTABLE_LEVELS), > + .start_level = (KVM_PGTABLE_LAST_LEVEL - > + CONFIG_PGTABLE_LEVELS + 1), > .mm_ops = &kvm_user_mm_ops, > }; > unsigned long flags; > kvm_pte_t pte = 0; /* Keep GCC quiet... */ > - u32 level = ~0; > + s8 level = ~0; Well, that's a semantic difference. ~0 == -1, which is a valid level, while the original code was trying to initialise level to something invalid. On the bright side, this function is going away in 6.7... > int ret; > > /* > @@ -829,7 +829,9 @@ static int get_user_mapping_size(struct kvm *kvm, u64 addr) > * Not seeing an error, but not updating level? Something went > * deeply wrong... > */ > - if (WARN_ON(level >= KVM_PGTABLE_MAX_LEVELS)) > + if (WARN_ON(level > KVM_PGTABLE_LAST_LEVEL)) > + return -EFAULT; > + if (WARN_ON(level < KVM_PGTABLE_FIRST_LEVEL)) > return -EFAULT; > > /* Oops, the userspace PTs are gone... Replay the fault */ > @@ -1407,7 +1409,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > gfn_t gfn; > kvm_pfn_t pfn; > bool logging_active = memslot_is_logging(memslot); > - unsigned long fault_level = kvm_vcpu_trap_get_fault_level(vcpu); > + s8 fault_level = kvm_vcpu_trap_get_fault_level(vcpu); > long vma_pagesize, fault_granule; > enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R; > struct kvm_pgtable *pgt; Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel