[PATCH 0/5] KVM: arm64: General cleanups

[PATCH 0/5] KVM: arm64: General cleanups

[Anshuman Khandual]

This series contains mostly unrelated general cleanups. This series applies
on v5.14-rc5 and has been boot tested with different page sized guests.

Cc: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: kvmarm@lists.cs.columbia.edu
Cc: linux-kernel@vger.kernel.org

Anshuman Khandual (5):
  KVM: arm64: Drop direct PAGE_[SHIFT|SIZE] usage as page size
  KVM: arm64: Drop init_common_resources()
  KVM: arm64: Drop check_kvm_target_cpu() based percpu probe
  KVM: arm64: Drop unused REQUIRES_VIRT
  KVM: arm64: Define KVM_PHYS_SHIFT_MIN

 arch/arm64/include/asm/kvm_mmu.h |  3 ++-
 arch/arm64/kvm/arm.c             | 25 +------------------------
 arch/arm64/kvm/hyp/pgtable.c     |  6 +++---
 arch/arm64/kvm/reset.c           |  2 +-
 arch/arm64/mm/mmu.c              |  2 +-
 5 files changed, 8 insertions(+), 30 deletions(-)

-- 
2.20.1

[PATCH 1/5] KVM: arm64: Drop direct PAGE_[SHIFT|SIZE] usage as page size

[Anshuman Khandual]

All instances here could just directly test against CONFIG_ARM64_XXK_PAGES
instead of evaluating via PAGE_SHIFT or PAGE_SIZE. With this change, there
will be no such usage left.

Cc: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: kvmarm@lists.cs.columbia.edu
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/kvm/hyp/pgtable.c | 6 +++---
 arch/arm64/mm/mmu.c          | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 05321f4165e3..a6112b6d6ef6 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -85,7 +85,7 @@ static bool kvm_level_supports_block_mapping(u32 level)
 	 * Reject invalid block mappings and don't bother with 4TB mappings for
 	 * 52-bit PAs.
 	 */
-	return !(level == 0 || (PAGE_SIZE != SZ_4K && level == 1));
+	return !(level == 0 || (!IS_ENABLED(CONFIG_ARM64_4K_PAGES) && level == 1));
 }
 
 static bool kvm_block_mapping_supported(u64 addr, u64 end, u64 phys, u32 level)
@@ -155,7 +155,7 @@ static u64 kvm_pte_to_phys(kvm_pte_t pte)
 {
 	u64 pa = pte & KVM_PTE_ADDR_MASK;
 
-	if (PAGE_SHIFT == 16)
+	if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
 		pa |= FIELD_GET(KVM_PTE_ADDR_51_48, pte) << 48;
 
 	return pa;
@@ -165,7 +165,7 @@ static kvm_pte_t kvm_phys_to_pte(u64 pa)
 {
 	kvm_pte_t pte = pa & KVM_PTE_ADDR_MASK;
 
-	if (PAGE_SHIFT == 16)
+	if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
 		pte |= FIELD_PREP(KVM_PTE_ADDR_51_48, pa >> 48);
 
 	return pte;
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 9ff0de1b2b93..8fdfca179815 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -296,7 +296,7 @@ static void alloc_init_cont_pmd(pud_t *pudp, unsigned long addr,
 static inline bool use_1G_block(unsigned long addr, unsigned long next,
 			unsigned long phys)
 {
-	if (PAGE_SHIFT != 12)
+	if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES))
 		return false;
 
 	if (((addr | next | phys) & ~PUD_MASK) != 0)
-- 
2.20.1

[PATCH 2/5] KVM: arm64: Drop init_common_resources()

[Anshuman Khandual]

Could do without this additional indirection via init_common_resources() by
just calling kvm_set_ipa_limit() directly instead. This change saves memory
and cycles.

Cc: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: kvmarm@lists.cs.columbia.edu
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/kvm/arm.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index e9a2b8f27792..19560e457c11 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1696,11 +1696,6 @@ static bool init_psci_relay(void)
 	return true;
 }
 
-static int init_common_resources(void)
-{
-	return kvm_set_ipa_limit();
-}
-
 static int init_subsystems(void)
 {
 	int err = 0;
@@ -2102,7 +2097,7 @@ int kvm_arch_init(void *opaque)
 		}
 	}
 
-	err = init_common_resources();
+	err = kvm_set_ipa_limit();
 	if (err)
 		return err;
 
-- 
2.20.1

[PATCH 3/5] KVM: arm64: Drop check_kvm_target_cpu() based percpu probe

[Anshuman Khandual]

kvm_target_cpu() never returns a negative error code, so check_kvm_target()
would never have 'ret' filled with a negative error code. Hence the percpu
probe via check_kvm_target_cpu() does not make sense as its never going to
find an unsupported CPU, forcing kvm_arch_init() to exit early. Hence lets
just drop this percpu probe (and also check_kvm_target_cpu()) altogether.

Cc: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: kvmarm@lists.cs.columbia.edu
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/kvm/arm.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 19560e457c11..16f93678c17e 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -2010,11 +2010,6 @@ static int finalize_hyp_mode(void)
 	return 0;
 }
 
-static void check_kvm_target_cpu(void *ret)
-{
-	*(int *)ret = kvm_target_cpu();
-}
-
 struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr)
 {
 	struct kvm_vcpu *vcpu;
@@ -2074,7 +2069,6 @@ void kvm_arch_irq_bypass_start(struct irq_bypass_consumer *cons)
 int kvm_arch_init(void *opaque)
 {
 	int err;
-	int ret, cpu;
 	bool in_hyp_mode;
 
 	if (!is_hyp_mode_available()) {
@@ -2089,14 +2083,6 @@ int kvm_arch_init(void *opaque)
 		kvm_info("Guests without required CPU erratum workarounds can deadlock system!\n" \
 			 "Only trusted guests should be used on this system.\n");
 
-	for_each_online_cpu(cpu) {
-		smp_call_function_single(cpu, check_kvm_target_cpu, &ret, 1);
-		if (ret < 0) {
-			kvm_err("Error, CPU %d not supported!\n", cpu);
-			return -ENODEV;
-		}
-	}
-
 	err = kvm_set_ipa_limit();
 	if (err)
 		return err;
-- 
2.20.1

[PATCH 4/5] KVM: arm64: Drop unused REQUIRES_VIRT

[Anshuman Khandual]

This seems like a residue from the past. REQUIRES_VIRT is no more available
. Hence it can just be dropped along with the related code section.

Cc: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: kvmarm@lists.cs.columbia.edu
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/kvm/arm.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 16f93678c17e..bae22d2bdca9 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -42,10 +42,6 @@
 #include <kvm/arm_pmu.h>
 #include <kvm/arm_psci.h>
 
-#ifdef REQUIRES_VIRT
-__asm__(".arch_extension	virt");
-#endif
-
 static enum kvm_mode kvm_mode = KVM_MODE_DEFAULT;
 DEFINE_STATIC_KEY_FALSE(kvm_protected_mode_initialized);
 
-- 
2.20.1

[PATCH 5/5] KVM: arm64: Define KVM_PHYS_SHIFT_MIN

[Anshuman Khandual]

Drop the hard coded value for the minimum IPA range i.e 32 bit. Instead
define a macro KVM_PHYS_SHIFT_MIN which improves the code readability.

Cc: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: kvmarm@lists.cs.columbia.edu
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/include/asm/kvm_mmu.h | 3 ++-
 arch/arm64/kvm/reset.c           | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index b52c5c4b9a3d..716f999818d9 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -141,7 +141,8 @@ static __always_inline unsigned long __kern_hyp_va(unsigned long v)
  * We currently support using a VM-specified IPA size. For backward
  * compatibility, the default IPA size is fixed to 40bits.
  */
-#define KVM_PHYS_SHIFT	(40)
+#define KVM_PHYS_SHIFT		(40)
+#define KVM_PHYS_SHIFT_MIN	(32)
 
 #define kvm_phys_shift(kvm)		VTCR_EL2_IPA(kvm->arch.vtcr)
 #define kvm_phys_size(kvm)		(_AC(1, ULL) << kvm_phys_shift(kvm))
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index cba7872d69a8..8dc8b4b9de37 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -369,7 +369,7 @@ int kvm_arm_setup_stage2(struct kvm *kvm, unsigned long type)
 	phys_shift = KVM_VM_TYPE_ARM_IPA_SIZE(type);
 	if (phys_shift) {
 		if (phys_shift > kvm_ipa_limit ||
-		    phys_shift < 32)
+		    phys_shift < KVM_PHYS_SHIFT_MIN)
 			return -EINVAL;
 	} else {
 		phys_shift = KVM_PHYS_SHIFT;
-- 
2.20.1

Re: [PATCH 1/5] KVM: arm64: Drop direct PAGE_[SHIFT|SIZE] usage as page size

[Will Deacon]

On Tue, Aug 10, 2021 at 12:32:37PM +0530, Anshuman Khandual wrote:
> All instances here could just directly test against CONFIG_ARM64_XXK_PAGES
> instead of evaluating via PAGE_SHIFT or PAGE_SIZE. With this change, there
> will be no such usage left.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: James Morse <james.morse@arm.com>
> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: kvmarm@lists.cs.columbia.edu
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/kvm/hyp/pgtable.c | 6 +++---
>  arch/arm64/mm/mmu.c          | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)

Why is this better?

WIll

Re: [PATCH 2/5] KVM: arm64: Drop init_common_resources()

[Will Deacon]

On Tue, Aug 10, 2021 at 12:32:38PM +0530, Anshuman Khandual wrote:
> Could do without this additional indirection via init_common_resources() by
> just calling kvm_set_ipa_limit() directly instead. This change saves memory
> and cycles.

Does it? Really?

Will

> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index e9a2b8f27792..19560e457c11 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1696,11 +1696,6 @@ static bool init_psci_relay(void)
>  	return true;
>  }
>  
> -static int init_common_resources(void)
> -{
> -	return kvm_set_ipa_limit();
> -}
> -
>  static int init_subsystems(void)
>  {
>  	int err = 0;
> @@ -2102,7 +2097,7 @@ int kvm_arch_init(void *opaque)
>  		}
>  	}
>  
> -	err = init_common_resources();
> +	err = kvm_set_ipa_limit();
>  	if (err)
>  		return err;
>  
> -- 
> 2.20.1
> 

Re: [PATCH 3/5] KVM: arm64: Drop check_kvm_target_cpu() based percpu probe

[Will Deacon]

On Tue, Aug 10, 2021 at 12:32:39PM +0530, Anshuman Khandual wrote:
> kvm_target_cpu() never returns a negative error code, so check_kvm_target()
> would never have 'ret' filled with a negative error code. Hence the percpu
> probe via check_kvm_target_cpu() does not make sense as its never going to
> find an unsupported CPU, forcing kvm_arch_init() to exit early. Hence lets
> just drop this percpu probe (and also check_kvm_target_cpu()) altogether.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: James Morse <james.morse@arm.com>
> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: kvmarm@lists.cs.columbia.edu
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/kvm/arm.c | 14 --------------
>  1 file changed, 14 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 19560e457c11..16f93678c17e 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -2010,11 +2010,6 @@ static int finalize_hyp_mode(void)
>  	return 0;
>  }
>  
> -static void check_kvm_target_cpu(void *ret)
> -{
> -	*(int *)ret = kvm_target_cpu();
> -}
> -
>  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr)
>  {
>  	struct kvm_vcpu *vcpu;
> @@ -2074,7 +2069,6 @@ void kvm_arch_irq_bypass_start(struct irq_bypass_consumer *cons)
>  int kvm_arch_init(void *opaque)
>  {
>  	int err;
> -	int ret, cpu;
>  	bool in_hyp_mode;
>  
>  	if (!is_hyp_mode_available()) {
> @@ -2089,14 +2083,6 @@ int kvm_arch_init(void *opaque)
>  		kvm_info("Guests without required CPU erratum workarounds can deadlock system!\n" \
>  			 "Only trusted guests should be used on this system.\n");
>  
> -	for_each_online_cpu(cpu) {
> -		smp_call_function_single(cpu, check_kvm_target_cpu, &ret, 1);
> -		if (ret < 0) {
> -			kvm_err("Error, CPU %d not supported!\n", cpu);
> -			return -ENODEV;
> -		}
> -	}

Looks like kvm_target_cpu() *could* return an error at one time of day (at
least on 32-bit), but agreed that this checking is no longer needed:

Acked-by: Will Deacon <will@kernel.org>

Perhaps it's worth making the return type of kvm_target_cpu() a u32 to
make it a bit more explicit that you shouldn't be returning an error code
there?

Will

Re: [PATCH 5/5] KVM: arm64: Define KVM_PHYS_SHIFT_MIN

[Marc Zyngier]

On 2021-08-10 08:02, Anshuman Khandual wrote:
> Drop the hard coded value for the minimum IPA range i.e 32 bit. Instead
> define a macro KVM_PHYS_SHIFT_MIN which improves the code readability.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: James Morse <james.morse@arm.com>
> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: kvmarm@lists.cs.columbia.edu
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/include/asm/kvm_mmu.h | 3 ++-
>  arch/arm64/kvm/reset.c           | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_mmu.h 
> b/arch/arm64/include/asm/kvm_mmu.h
> index b52c5c4b9a3d..716f999818d9 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -141,7 +141,8 @@ static __always_inline unsigned long
> __kern_hyp_va(unsigned long v)
>   * We currently support using a VM-specified IPA size. For backward
>   * compatibility, the default IPA size is fixed to 40bits.
>   */
> -#define KVM_PHYS_SHIFT	(40)
> +#define KVM_PHYS_SHIFT		(40)
> +#define KVM_PHYS_SHIFT_MIN	(32)
> 
>  #define kvm_phys_shift(kvm)		VTCR_EL2_IPA(kvm->arch.vtcr)
>  #define kvm_phys_size(kvm)		(_AC(1, ULL) << kvm_phys_shift(kvm))
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index cba7872d69a8..8dc8b4b9de37 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -369,7 +369,7 @@ int kvm_arm_setup_stage2(struct kvm *kvm, unsigned
> long type)
>  	phys_shift = KVM_VM_TYPE_ARM_IPA_SIZE(type);
>  	if (phys_shift) {
>  		if (phys_shift > kvm_ipa_limit ||
> -		    phys_shift < 32)
> +		    phys_shift < KVM_PHYS_SHIFT_MIN)
>  			return -EINVAL;
>  	} else {
>  		phys_shift = KVM_PHYS_SHIFT;

This is not a KVM property, but an architectural one. If you
want to replace it with something more readable, please
make it global to the whole of arm64 (ARM64_MIN_PARANGE?).

Thanks,

       M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/5] KVM: arm64: Drop direct PAGE_[SHIFT|SIZE] usage as page size

[Marc Zyngier]

On 2021-08-10 08:02, Anshuman Khandual wrote:
> All instances here could just directly test against 
> CONFIG_ARM64_XXK_PAGES
> instead of evaluating via PAGE_SHIFT or PAGE_SIZE. With this change, 
> there
> will be no such usage left.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: James Morse <james.morse@arm.com>
> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: kvmarm@lists.cs.columbia.edu
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/kvm/hyp/pgtable.c | 6 +++---
>  arch/arm64/mm/mmu.c          | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/pgtable.c 
> b/arch/arm64/kvm/hyp/pgtable.c
> index 05321f4165e3..a6112b6d6ef6 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -85,7 +85,7 @@ static bool kvm_level_supports_block_mapping(u32 
> level)
>  	 * Reject invalid block mappings and don't bother with 4TB mappings 
> for
>  	 * 52-bit PAs.
>  	 */
> -	return !(level == 0 || (PAGE_SIZE != SZ_4K && level == 1));
> +	return !(level == 0 || (!IS_ENABLED(CONFIG_ARM64_4K_PAGES) && level 
> == 1));
>  }
> 
>  static bool kvm_block_mapping_supported(u64 addr, u64 end, u64 phys, 
> u32 level)
> @@ -155,7 +155,7 @@ static u64 kvm_pte_to_phys(kvm_pte_t pte)
>  {
>  	u64 pa = pte & KVM_PTE_ADDR_MASK;
> 
> -	if (PAGE_SHIFT == 16)
> +	if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
>  		pa |= FIELD_GET(KVM_PTE_ADDR_51_48, pte) << 48;
> 
>  	return pa;
> @@ -165,7 +165,7 @@ static kvm_pte_t kvm_phys_to_pte(u64 pa)
>  {
>  	kvm_pte_t pte = pa & KVM_PTE_ADDR_MASK;
> 
> -	if (PAGE_SHIFT == 16)
> +	if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
>  		pte |= FIELD_PREP(KVM_PTE_ADDR_51_48, pa >> 48);
> 
>  	return pte;
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 9ff0de1b2b93..8fdfca179815 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -296,7 +296,7 @@ static void alloc_init_cont_pmd(pud_t *pudp,
> unsigned long addr,
>  static inline bool use_1G_block(unsigned long addr, unsigned long 
> next,
>  			unsigned long phys)
>  {
> -	if (PAGE_SHIFT != 12)
> +	if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES))
>  		return false;
> 
>  	if (((addr | next | phys) & ~PUD_MASK) != 0)

I personally find it a lot less readable.

Also, there is no evaluation whatsoever. All the code guarded
by a PAGE_SIZE/PAGE_SHIFT that doesn't match the configuration
is dropped at compile time.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 2/5] KVM: arm64: Drop init_common_resources()

[Anshuman Khandual]

On 8/10/21 6:51 PM, Will Deacon wrote:
> On Tue, Aug 10, 2021 at 12:32:38PM +0530, Anshuman Khandual wrote:
>> Could do without this additional indirection via init_common_resources() by
>> just calling kvm_set_ipa_limit() directly instead. This change saves memory
>> and cycles.
> 
> Does it? Really?

TBH, I did not really measure them to be sure. It was more like an intuitive
assumption. But do you suspect that the compiler might be already optimizing
this out, hence giving memory and CPU cycle benefits ? Regardless this still
drops an unnecessary function.

> 
> Will
> 
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index e9a2b8f27792..19560e457c11 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -1696,11 +1696,6 @@ static bool init_psci_relay(void)
>>  	return true;
>>  }
>>  
>> -static int init_common_resources(void)
>> -{
>> -	return kvm_set_ipa_limit();
>> -}
>> -
>>  static int init_subsystems(void)
>>  {
>>  	int err = 0;
>> @@ -2102,7 +2097,7 @@ int kvm_arch_init(void *opaque)
>>  		}
>>  	}
>>  
>> -	err = init_common_resources();
>> +	err = kvm_set_ipa_limit();
>>  	if (err)
>>  		return err;
>>  
>> -- 
>> 2.20.1
>>

Re: [PATCH 5/5] KVM: arm64: Define KVM_PHYS_SHIFT_MIN

[Anshuman Khandual]

On 8/10/21 6:59 PM, Marc Zyngier wrote:
> On 2021-08-10 08:02, Anshuman Khandual wrote:
>> Drop the hard coded value for the minimum IPA range i.e 32 bit. Instead
>> define a macro KVM_PHYS_SHIFT_MIN which improves the code readability.
>>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: James Morse <james.morse@arm.com>
>> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: kvmarm@lists.cs.columbia.edu
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/include/asm/kvm_mmu.h | 3 ++-
>>  arch/arm64/kvm/reset.c           | 2 +-
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
>> index b52c5c4b9a3d..716f999818d9 100644
>> --- a/arch/arm64/include/asm/kvm_mmu.h
>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>> @@ -141,7 +141,8 @@ static __always_inline unsigned long
>> __kern_hyp_va(unsigned long v)
>>   * We currently support using a VM-specified IPA size. For backward
>>   * compatibility, the default IPA size is fixed to 40bits.
>>   */
>> -#define KVM_PHYS_SHIFT    (40)
>> +#define KVM_PHYS_SHIFT        (40)
>> +#define KVM_PHYS_SHIFT_MIN    (32)
>>
>>  #define kvm_phys_shift(kvm)        VTCR_EL2_IPA(kvm->arch.vtcr)
>>  #define kvm_phys_size(kvm)        (_AC(1, ULL) << kvm_phys_shift(kvm))
>> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
>> index cba7872d69a8..8dc8b4b9de37 100644
>> --- a/arch/arm64/kvm/reset.c
>> +++ b/arch/arm64/kvm/reset.c
>> @@ -369,7 +369,7 @@ int kvm_arm_setup_stage2(struct kvm *kvm, unsigned
>> long type)
>>      phys_shift = KVM_VM_TYPE_ARM_IPA_SIZE(type);
>>      if (phys_shift) {
>>          if (phys_shift > kvm_ipa_limit ||
>> -            phys_shift < 32)
>> +            phys_shift < KVM_PHYS_SHIFT_MIN)
>>              return -EINVAL;
>>      } else {
>>          phys_shift = KVM_PHYS_SHIFT;
> 
> This is not a KVM property, but an architectural one. If you

Architectural property which suggest the minimum physical address shift
supported on a platform, as indicated via ID_AA64MMFR0.PARANGE = 0x0 ?

> want to replace it with something more readable, please
> make it global to the whole of arm64 (ARM64_MIN_PARANGE?).

Sure, will do.

Re: [PATCH 5/5] KVM: arm64: Define KVM_PHYS_SHIFT_MIN

[Marc Zyngier]

On 2021-08-10 16:19, Anshuman Khandual wrote:
> On 8/10/21 6:59 PM, Marc Zyngier wrote:
>> On 2021-08-10 08:02, Anshuman Khandual wrote:
>>> Drop the hard coded value for the minimum IPA range i.e 32 bit. 
>>> Instead
>>> define a macro KVM_PHYS_SHIFT_MIN which improves the code 
>>> readability.
>>> 
>>> Cc: Marc Zyngier <maz@kernel.org>
>>> Cc: James Morse <james.morse@arm.com>
>>> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>> Cc: Will Deacon <will@kernel.org>
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: kvmarm@lists.cs.columbia.edu
>>> Cc: linux-kernel@vger.kernel.org
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>> ---
>>>  arch/arm64/include/asm/kvm_mmu.h | 3 ++-
>>>  arch/arm64/kvm/reset.c           | 2 +-
>>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/arch/arm64/include/asm/kvm_mmu.h 
>>> b/arch/arm64/include/asm/kvm_mmu.h
>>> index b52c5c4b9a3d..716f999818d9 100644
>>> --- a/arch/arm64/include/asm/kvm_mmu.h
>>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>>> @@ -141,7 +141,8 @@ static __always_inline unsigned long
>>> __kern_hyp_va(unsigned long v)
>>>   * We currently support using a VM-specified IPA size. For backward
>>>   * compatibility, the default IPA size is fixed to 40bits.
>>>   */
>>> -#define KVM_PHYS_SHIFT    (40)
>>> +#define KVM_PHYS_SHIFT        (40)
>>> +#define KVM_PHYS_SHIFT_MIN    (32)
>>> 
>>>  #define kvm_phys_shift(kvm)        VTCR_EL2_IPA(kvm->arch.vtcr)
>>>  #define kvm_phys_size(kvm)        (_AC(1, ULL) << 
>>> kvm_phys_shift(kvm))
>>> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
>>> index cba7872d69a8..8dc8b4b9de37 100644
>>> --- a/arch/arm64/kvm/reset.c
>>> +++ b/arch/arm64/kvm/reset.c
>>> @@ -369,7 +369,7 @@ int kvm_arm_setup_stage2(struct kvm *kvm, 
>>> unsigned
>>> long type)
>>>      phys_shift = KVM_VM_TYPE_ARM_IPA_SIZE(type);
>>>      if (phys_shift) {
>>>          if (phys_shift > kvm_ipa_limit ||
>>> -            phys_shift < 32)
>>> +            phys_shift < KVM_PHYS_SHIFT_MIN)
>>>              return -EINVAL;
>>>      } else {
>>>          phys_shift = KVM_PHYS_SHIFT;
>> 
>> This is not a KVM property, but an architectural one. If you
> 
> Architectural property which suggest the minimum physical address shift
> supported on a platform, as indicated via ID_AA64MMFR0.PARANGE = 0x0 ?

That's the one.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/5] KVM: arm64: Drop direct PAGE_[SHIFT|SIZE] usage as page size

[Anshuman Khandual]

On 8/10/21 6:50 PM, Will Deacon wrote:
> On Tue, Aug 10, 2021 at 12:32:37PM +0530, Anshuman Khandual wrote:
>> All instances here could just directly test against CONFIG_ARM64_XXK_PAGES
>> instead of evaluating via PAGE_SHIFT or PAGE_SIZE. With this change, there
>> will be no such usage left.
>>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: James Morse <james.morse@arm.com>
>> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: kvmarm@lists.cs.columbia.edu
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/kvm/hyp/pgtable.c | 6 +++---
>>  arch/arm64/mm/mmu.c          | 2 +-
>>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> Why is this better?

There are two improvements here.

1. Avoids using hard coded numerical shift values to determine the page size

	e.g PAGE_SHIFT = 16 for 64K

2. There are already instances of IS_ENABLED() construct checking for the page
   size. After this change there will be just a single method to test page size
   , rather than checking for either config, PAGE_SHIFT or PAGE_SIZE etc through
   out arm64. This change helps in that unification around IS_ENABLED().

There is another patch which drops remaining usage for PAGE_SIZE as well.

https://patchwork.kernel.org/project/linux-arm-kernel/patch/1628569782-30213-1-git-send-email-anshuman.khandual@arm.com/ 

Re: [PATCH 1/5] KVM: arm64: Drop direct PAGE_[SHIFT|SIZE] usage as page size

[Anshuman Khandual]

On 8/10/21 7:03 PM, Marc Zyngier wrote:
> On 2021-08-10 08:02, Anshuman Khandual wrote:
>> All instances here could just directly test against CONFIG_ARM64_XXK_PAGES
>> instead of evaluating via PAGE_SHIFT or PAGE_SIZE. With this change, there
>> will be no such usage left.
>>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: James Morse <james.morse@arm.com>
>> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: kvmarm@lists.cs.columbia.edu
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/kvm/hyp/pgtable.c | 6 +++---
>>  arch/arm64/mm/mmu.c          | 2 +-
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
>> index 05321f4165e3..a6112b6d6ef6 100644
>> --- a/arch/arm64/kvm/hyp/pgtable.c
>> +++ b/arch/arm64/kvm/hyp/pgtable.c
>> @@ -85,7 +85,7 @@ static bool kvm_level_supports_block_mapping(u32 level)
>>       * Reject invalid block mappings and don't bother with 4TB mappings for
>>       * 52-bit PAs.
>>       */
>> -    return !(level == 0 || (PAGE_SIZE != SZ_4K && level == 1));
>> +    return !(level == 0 || (!IS_ENABLED(CONFIG_ARM64_4K_PAGES) && level == 1));
>>  }
>>
>>  static bool kvm_block_mapping_supported(u64 addr, u64 end, u64 phys, u32 level)
>> @@ -155,7 +155,7 @@ static u64 kvm_pte_to_phys(kvm_pte_t pte)
>>  {
>>      u64 pa = pte & KVM_PTE_ADDR_MASK;
>>
>> -    if (PAGE_SHIFT == 16)
>> +    if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
>>          pa |= FIELD_GET(KVM_PTE_ADDR_51_48, pte) << 48;
>>
>>      return pa;
>> @@ -165,7 +165,7 @@ static kvm_pte_t kvm_phys_to_pte(u64 pa)
>>  {
>>      kvm_pte_t pte = pa & KVM_PTE_ADDR_MASK;
>>
>> -    if (PAGE_SHIFT == 16)
>> +    if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
>>          pte |= FIELD_PREP(KVM_PTE_ADDR_51_48, pa >> 48);
>>
>>      return pte;
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 9ff0de1b2b93..8fdfca179815 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -296,7 +296,7 @@ static void alloc_init_cont_pmd(pud_t *pudp,
>> unsigned long addr,
>>  static inline bool use_1G_block(unsigned long addr, unsigned long next,
>>              unsigned long phys)
>>  {
>> -    if (PAGE_SHIFT != 12)
>> +    if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES))
>>          return false;
>>
>>      if (((addr | next | phys) & ~PUD_MASK) != 0)
> 
> I personally find it a lot less readable.
> 
> Also, there is no evaluation whatsoever. All the code guarded
> by a PAGE_SIZE/PAGE_SHIFT that doesn't match the configuration
> is dropped at compile time.

The primary idea here is to unify around IS_ENABLED(CONFIG_ARM64_XXK_PAGES)
usage in arm64, rather than having multiple methods to test page size when
ever required.

Re: [PATCH 3/5] KVM: arm64: Drop check_kvm_target_cpu() based percpu probe

[Anshuman Khandual]

On 8/10/21 6:58 PM, Will Deacon wrote:
> On Tue, Aug 10, 2021 at 12:32:39PM +0530, Anshuman Khandual wrote:
>> kvm_target_cpu() never returns a negative error code, so check_kvm_target()
>> would never have 'ret' filled with a negative error code. Hence the percpu
>> probe via check_kvm_target_cpu() does not make sense as its never going to
>> find an unsupported CPU, forcing kvm_arch_init() to exit early. Hence lets
>> just drop this percpu probe (and also check_kvm_target_cpu()) altogether.
>>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: James Morse <james.morse@arm.com>
>> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: kvmarm@lists.cs.columbia.edu
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/kvm/arm.c | 14 --------------
>>  1 file changed, 14 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 19560e457c11..16f93678c17e 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -2010,11 +2010,6 @@ static int finalize_hyp_mode(void)
>>  	return 0;
>>  }
>>  
>> -static void check_kvm_target_cpu(void *ret)
>> -{
>> -	*(int *)ret = kvm_target_cpu();
>> -}
>> -
>>  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr)
>>  {
>>  	struct kvm_vcpu *vcpu;
>> @@ -2074,7 +2069,6 @@ void kvm_arch_irq_bypass_start(struct irq_bypass_consumer *cons)
>>  int kvm_arch_init(void *opaque)
>>  {
>>  	int err;
>> -	int ret, cpu;
>>  	bool in_hyp_mode;
>>  
>>  	if (!is_hyp_mode_available()) {
>> @@ -2089,14 +2083,6 @@ int kvm_arch_init(void *opaque)
>>  		kvm_info("Guests without required CPU erratum workarounds can deadlock system!\n" \
>>  			 "Only trusted guests should be used on this system.\n");
>>  
>> -	for_each_online_cpu(cpu) {
>> -		smp_call_function_single(cpu, check_kvm_target_cpu, &ret, 1);
>> -		if (ret < 0) {
>> -			kvm_err("Error, CPU %d not supported!\n", cpu);
>> -			return -ENODEV;
>> -		}
>> -	}
> 
> Looks like kvm_target_cpu() *could* return an error at one time of day (at
> least on 32-bit), but agreed that this checking is no longer needed:
> 
> Acked-by: Will Deacon <will@kernel.org>
> 
> Perhaps it's worth making the return type of kvm_target_cpu() a u32 to
> make it a bit more explicit that you shouldn't be returning an error code
> there?

Sure, will change the return type to u32.

Re: [PATCH 1/5] KVM: arm64: Drop direct PAGE_[SHIFT|SIZE] usage as page size

[Marc Zyngier]

On Wed, 11 Aug 2021 06:34:46 +0100,
Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> 
> 
> 
> On 8/10/21 7:03 PM, Marc Zyngier wrote:
> > On 2021-08-10 08:02, Anshuman Khandual wrote:
> >> All instances here could just directly test against CONFIG_ARM64_XXK_PAGES
> >> instead of evaluating via PAGE_SHIFT or PAGE_SIZE. With this change, there
> >> will be no such usage left.
> >>
> >> Cc: Marc Zyngier <maz@kernel.org>
> >> Cc: James Morse <james.morse@arm.com>
> >> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> >> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> Cc: Will Deacon <will@kernel.org>
> >> Cc: linux-arm-kernel@lists.infradead.org
> >> Cc: kvmarm@lists.cs.columbia.edu
> >> Cc: linux-kernel@vger.kernel.org
> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> >> ---
> >>  arch/arm64/kvm/hyp/pgtable.c | 6 +++---
> >>  arch/arm64/mm/mmu.c          | 2 +-
> >>  2 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> >> index 05321f4165e3..a6112b6d6ef6 100644
> >> --- a/arch/arm64/kvm/hyp/pgtable.c
> >> +++ b/arch/arm64/kvm/hyp/pgtable.c
> >> @@ -85,7 +85,7 @@ static bool kvm_level_supports_block_mapping(u32 level)
> >>       * Reject invalid block mappings and don't bother with 4TB mappings for
> >>       * 52-bit PAs.
> >>       */
> >> -    return !(level == 0 || (PAGE_SIZE != SZ_4K && level == 1));
> >> +    return !(level == 0 || (!IS_ENABLED(CONFIG_ARM64_4K_PAGES) && level == 1));
> >>  }
> >>
> >>  static bool kvm_block_mapping_supported(u64 addr, u64 end, u64 phys, u32 level)
> >> @@ -155,7 +155,7 @@ static u64 kvm_pte_to_phys(kvm_pte_t pte)
> >>  {
> >>      u64 pa = pte & KVM_PTE_ADDR_MASK;
> >>
> >> -    if (PAGE_SHIFT == 16)
> >> +    if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
> >>          pa |= FIELD_GET(KVM_PTE_ADDR_51_48, pte) << 48;
> >>
> >>      return pa;
> >> @@ -165,7 +165,7 @@ static kvm_pte_t kvm_phys_to_pte(u64 pa)
> >>  {
> >>      kvm_pte_t pte = pa & KVM_PTE_ADDR_MASK;
> >>
> >> -    if (PAGE_SHIFT == 16)
> >> +    if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
> >>          pte |= FIELD_PREP(KVM_PTE_ADDR_51_48, pa >> 48);
> >>
> >>      return pte;
> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> index 9ff0de1b2b93..8fdfca179815 100644
> >> --- a/arch/arm64/mm/mmu.c
> >> +++ b/arch/arm64/mm/mmu.c
> >> @@ -296,7 +296,7 @@ static void alloc_init_cont_pmd(pud_t *pudp,
> >> unsigned long addr,
> >>  static inline bool use_1G_block(unsigned long addr, unsigned long next,
> >>              unsigned long phys)
> >>  {
> >> -    if (PAGE_SHIFT != 12)
> >> +    if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES))
> >>          return false;
> >>
> >>      if (((addr | next | phys) & ~PUD_MASK) != 0)
> > 
> > I personally find it a lot less readable.
> > 
> > Also, there is no evaluation whatsoever. All the code guarded
> > by a PAGE_SIZE/PAGE_SHIFT that doesn't match the configuration
> > is dropped at compile time.
> 
> The primary idea here is to unify around IS_ENABLED(CONFIG_ARM64_XXK_PAGES)
> usage in arm64, rather than having multiple methods to test page size when
> ever required.

I'm sorry, but I find the idiom extremely painful to parse. If you are
annoyed with the 'PAGE_SHIFT == 12/14/16', consider replacing it with
'PAGE_SIZE == SZ_4/16/64K' instead.

IS_ENABLED(CONFIG_ARM64_XXK_PAGES) also gives the wrong impression
that *multiple* page sizes can be selected at any given time. That's
obviously not the case, which actually makes PAGE_SIZE a much better
choice.

As things stand, I don't plan to take such a patch.

Thanks,

	M.

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

Re: [PATCH 1/5] KVM: arm64: Drop direct PAGE_[SHIFT|SIZE] usage as page size

[Anshuman Khandual]

On 8/11/21 1:41 PM, Marc Zyngier wrote:
> On Wed, 11 Aug 2021 06:34:46 +0100,
> Anshuman Khandual <anshuman.khandual@arm.com> wrote:
>>
>>
>>
>> On 8/10/21 7:03 PM, Marc Zyngier wrote:
>>> On 2021-08-10 08:02, Anshuman Khandual wrote:
>>>> All instances here could just directly test against CONFIG_ARM64_XXK_PAGES
>>>> instead of evaluating via PAGE_SHIFT or PAGE_SIZE. With this change, there
>>>> will be no such usage left.
>>>>
>>>> Cc: Marc Zyngier <maz@kernel.org>
>>>> Cc: James Morse <james.morse@arm.com>
>>>> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
>>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Will Deacon <will@kernel.org>
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: kvmarm@lists.cs.columbia.edu
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>> ---
>>>>  arch/arm64/kvm/hyp/pgtable.c | 6 +++---
>>>>  arch/arm64/mm/mmu.c          | 2 +-
>>>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
>>>> index 05321f4165e3..a6112b6d6ef6 100644
>>>> --- a/arch/arm64/kvm/hyp/pgtable.c
>>>> +++ b/arch/arm64/kvm/hyp/pgtable.c
>>>> @@ -85,7 +85,7 @@ static bool kvm_level_supports_block_mapping(u32 level)
>>>>       * Reject invalid block mappings and don't bother with 4TB mappings for
>>>>       * 52-bit PAs.
>>>>       */
>>>> -    return !(level == 0 || (PAGE_SIZE != SZ_4K && level == 1));
>>>> +    return !(level == 0 || (!IS_ENABLED(CONFIG_ARM64_4K_PAGES) && level == 1));
>>>>  }
>>>>
>>>>  static bool kvm_block_mapping_supported(u64 addr, u64 end, u64 phys, u32 level)
>>>> @@ -155,7 +155,7 @@ static u64 kvm_pte_to_phys(kvm_pte_t pte)
>>>>  {
>>>>      u64 pa = pte & KVM_PTE_ADDR_MASK;
>>>>
>>>> -    if (PAGE_SHIFT == 16)
>>>> +    if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
>>>>          pa |= FIELD_GET(KVM_PTE_ADDR_51_48, pte) << 48;
>>>>
>>>>      return pa;
>>>> @@ -165,7 +165,7 @@ static kvm_pte_t kvm_phys_to_pte(u64 pa)
>>>>  {
>>>>      kvm_pte_t pte = pa & KVM_PTE_ADDR_MASK;
>>>>
>>>> -    if (PAGE_SHIFT == 16)
>>>> +    if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
>>>>          pte |= FIELD_PREP(KVM_PTE_ADDR_51_48, pa >> 48);
>>>>
>>>>      return pte;
>>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>>> index 9ff0de1b2b93..8fdfca179815 100644
>>>> --- a/arch/arm64/mm/mmu.c
>>>> +++ b/arch/arm64/mm/mmu.c
>>>> @@ -296,7 +296,7 @@ static void alloc_init_cont_pmd(pud_t *pudp,
>>>> unsigned long addr,
>>>>  static inline bool use_1G_block(unsigned long addr, unsigned long next,
>>>>              unsigned long phys)
>>>>  {
>>>> -    if (PAGE_SHIFT != 12)
>>>> +    if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES))
>>>>          return false;
>>>>
>>>>      if (((addr | next | phys) & ~PUD_MASK) != 0)
>>>
>>> I personally find it a lot less readable.
>>>
>>> Also, there is no evaluation whatsoever. All the code guarded
>>> by a PAGE_SIZE/PAGE_SHIFT that doesn't match the configuration
>>> is dropped at compile time.
>>
>> The primary idea here is to unify around IS_ENABLED(CONFIG_ARM64_XXK_PAGES)
>> usage in arm64, rather than having multiple methods to test page size when
>> ever required.
> 
> I'm sorry, but I find the idiom extremely painful to parse. If you are

Okay, it was not explained very well. My bad.

> annoyed with the 'PAGE_SHIFT == 12/14/16', consider replacing it with
> 'PAGE_SIZE == SZ_4/16/64K' instead.

Sure, understood. But the problem here is not with PAGE_SHIFT/PAGE_SIZE
based tests but rather having multiple ways of doing the same thing in
arm64 tree. Please find further explanation below.

> 
> IS_ENABLED(CONFIG_ARM64_XXK_PAGES) also gives the wrong impression
> that *multiple* page sizes can be selected at any given time. That's
> obviously not the case, which actually makes PAGE_SIZE a much better
> choice.

PAGE_SHIFT and PAGE_SIZE are derived from CONFIG_ARM64_XXK_PAGES. Hence
why not just directly use the original user selected config option that
eventually decides PAGE_SHIFT and PAGE_SIZE.

config ARM64_PAGE_SHIFT
        int
        default 16 if ARM64_64K_PAGES
        default 14 if ARM64_16K_PAGES
        default 12

arch/arm64/include/asm/page-def.h:#define PAGE_SHIFT	CONFIG_ARM64_PAGE_SHIFT
arch/arm64/include/asm/page-def.h:#define PAGE_SIZE	(_AC(1, UL) << PAGE_SHIFT)

Also there are already similar IS_ENABLED() instances which do not
create much confusion. The point here being, to have just a single
method that checks compiled page size support, instead of three
different ways of doing the same thing.

- IS_ENABLED(CONFIG_ARM64_XXK_PAGES)
- if (PAGE_SHIFT == XX)
- if (PAGE_SIZE == XX)

$git grep IS_ENABLED arch/arm64/ | grep PAGES

arch/arm64/include/asm/vmalloc.h:	return IS_ENABLED(CONFIG_ARM64_4K_PAGES) &&
arch/arm64/mm/mmu.c:		BUG_ON(!IS_ENABLED(CONFIG_ARM64_16K_PAGES));
arch/arm64/mm/mmu.c:		BUG_ON(!IS_ENABLED(CONFIG_ARM64_16K_PAGES));

> 
> As things stand, I don't plan to take such a patch.

Sure, will drop it from the series if the above explanation and
the rationale for the patch still does not convince you.

> 
> Thanks,
> 
> 	M.
> 

Re: [PATCH 1/5] KVM: arm64: Drop direct PAGE_[SHIFT|SIZE] usage as page size

[Marc Zyngier]

On Wed, 11 Aug 2021 10:37:36 +0100,
Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> 
> 
> 
> On 8/11/21 1:41 PM, Marc Zyngier wrote:
> > On Wed, 11 Aug 2021 06:34:46 +0100,
> > Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> >>
> >>
> >>
> >> On 8/10/21 7:03 PM, Marc Zyngier wrote:
> >>> On 2021-08-10 08:02, Anshuman Khandual wrote:
> >>>> All instances here could just directly test against CONFIG_ARM64_XXK_PAGES
> >>>> instead of evaluating via PAGE_SHIFT or PAGE_SIZE. With this change, there
> >>>> will be no such usage left.
> >>>>
> >>>> Cc: Marc Zyngier <maz@kernel.org>
> >>>> Cc: James Morse <james.morse@arm.com>
> >>>> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> >>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> >>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >>>> Cc: Will Deacon <will@kernel.org>
> >>>> Cc: linux-arm-kernel@lists.infradead.org
> >>>> Cc: kvmarm@lists.cs.columbia.edu
> >>>> Cc: linux-kernel@vger.kernel.org
> >>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> >>>> ---
> >>>>  arch/arm64/kvm/hyp/pgtable.c | 6 +++---
> >>>>  arch/arm64/mm/mmu.c          | 2 +-
> >>>>  2 files changed, 4 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> >>>> index 05321f4165e3..a6112b6d6ef6 100644
> >>>> --- a/arch/arm64/kvm/hyp/pgtable.c
> >>>> +++ b/arch/arm64/kvm/hyp/pgtable.c
> >>>> @@ -85,7 +85,7 @@ static bool kvm_level_supports_block_mapping(u32 level)
> >>>>       * Reject invalid block mappings and don't bother with 4TB mappings for
> >>>>       * 52-bit PAs.
> >>>>       */
> >>>> -    return !(level == 0 || (PAGE_SIZE != SZ_4K && level == 1));
> >>>> +    return !(level == 0 || (!IS_ENABLED(CONFIG_ARM64_4K_PAGES) && level == 1));
> >>>>  }
> >>>>
> >>>>  static bool kvm_block_mapping_supported(u64 addr, u64 end, u64 phys, u32 level)
> >>>> @@ -155,7 +155,7 @@ static u64 kvm_pte_to_phys(kvm_pte_t pte)
> >>>>  {
> >>>>      u64 pa = pte & KVM_PTE_ADDR_MASK;
> >>>>
> >>>> -    if (PAGE_SHIFT == 16)
> >>>> +    if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
> >>>>          pa |= FIELD_GET(KVM_PTE_ADDR_51_48, pte) << 48;
> >>>>
> >>>>      return pa;
> >>>> @@ -165,7 +165,7 @@ static kvm_pte_t kvm_phys_to_pte(u64 pa)
> >>>>  {
> >>>>      kvm_pte_t pte = pa & KVM_PTE_ADDR_MASK;
> >>>>
> >>>> -    if (PAGE_SHIFT == 16)
> >>>> +    if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
> >>>>          pte |= FIELD_PREP(KVM_PTE_ADDR_51_48, pa >> 48);
> >>>>
> >>>>      return pte;
> >>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >>>> index 9ff0de1b2b93..8fdfca179815 100644
> >>>> --- a/arch/arm64/mm/mmu.c
> >>>> +++ b/arch/arm64/mm/mmu.c
> >>>> @@ -296,7 +296,7 @@ static void alloc_init_cont_pmd(pud_t *pudp,
> >>>> unsigned long addr,
> >>>>  static inline bool use_1G_block(unsigned long addr, unsigned long next,
> >>>>              unsigned long phys)
> >>>>  {
> >>>> -    if (PAGE_SHIFT != 12)
> >>>> +    if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES))
> >>>>          return false;
> >>>>
> >>>>      if (((addr | next | phys) & ~PUD_MASK) != 0)
> >>>
> >>> I personally find it a lot less readable.
> >>>
> >>> Also, there is no evaluation whatsoever. All the code guarded
> >>> by a PAGE_SIZE/PAGE_SHIFT that doesn't match the configuration
> >>> is dropped at compile time.
> >>
> >> The primary idea here is to unify around IS_ENABLED(CONFIG_ARM64_XXK_PAGES)
> >> usage in arm64, rather than having multiple methods to test page size when
> >> ever required.
> > 
> > I'm sorry, but I find the idiom extremely painful to parse. If you are
> 
> Okay, it was not explained very well. My bad.
> 
> > annoyed with the 'PAGE_SHIFT == 12/14/16', consider replacing it with
> > 'PAGE_SIZE == SZ_4/16/64K' instead.
> 
> Sure, understood. But the problem here is not with PAGE_SHIFT/PAGE_SIZE
> based tests but rather having multiple ways of doing the same thing in
> arm64 tree. Please find further explanation below.
> 
> > 
> > IS_ENABLED(CONFIG_ARM64_XXK_PAGES) also gives the wrong impression
> > that *multiple* page sizes can be selected at any given time. That's
> > obviously not the case, which actually makes PAGE_SIZE a much better
> > choice.
> 
> PAGE_SHIFT and PAGE_SIZE are derived from CONFIG_ARM64_XXK_PAGES. Hence
> why not just directly use the original user selected config option that
> eventually decides PAGE_SHIFT and PAGE_SIZE.
> 
> config ARM64_PAGE_SHIFT
>         int
>         default 16 if ARM64_64K_PAGES
>         default 14 if ARM64_16K_PAGES
>         default 12
> 
> arch/arm64/include/asm/page-def.h:#define PAGE_SHIFT	CONFIG_ARM64_PAGE_SHIFT
> arch/arm64/include/asm/page-def.h:#define PAGE_SIZE	(_AC(1, UL) << PAGE_SHIFT)

I'm sorry, but that's only a build system artefact. PAGE_SIZE/SHIFT is
what we use in the kernel at large, not IS_ENABLED(BLAH). It is short,
to the point, and it is guaranteed to be what it says on the tin.

If by some miracle you were going to enable multiple *simultaneous*
page sizes support in the arm64 kernel, I'd certainly look at things
differently. Thankfully, this isn't the case.

> Also there are already similar IS_ENABLED() instances which do not
> create much confusion. The point here being, to have just a single
> method that checks compiled page size support, instead of three
> different ways of doing the same thing.
> 
> - IS_ENABLED(CONFIG_ARM64_XXK_PAGES)
> - if (PAGE_SHIFT == XX)
> - if (PAGE_SIZE == XX)
> 
> $git grep IS_ENABLED arch/arm64/ | grep PAGES
> 
> arch/arm64/include/asm/vmalloc.h:	return IS_ENABLED(CONFIG_ARM64_4K_PAGES) &&
> arch/arm64/mm/mmu.c:		BUG_ON(!IS_ENABLED(CONFIG_ARM64_16K_PAGES));
> arch/arm64/mm/mmu.c:		BUG_ON(!IS_ENABLED(CONFIG_ARM64_16K_PAGES));

3 instances are hardly a convincing argument.

maz@hot-poop:~/arm-platforms$ git grep -w PAGE_SIZE | grep  '== SZ'
arch/powerpc/include/asm/nohash/32/mmu-8xx.h:	if (PAGE_SIZE == SZ_16K)
fs/btrfs/disk-io.c:	if ((PAGE_SIZE == SZ_4K && sectorsize != PAGE_SIZE) ||
fs/btrfs/disk-io.c:	    (PAGE_SIZE == SZ_64K && (sectorsize != SZ_4K &&
fs/btrfs/disk-io.c:	if (PAGE_SIZE == SZ_64K && sectorsize == SZ_4K) {

Look, I win! :-)

> > 
> > As things stand, I don't plan to take such a patch.
> 
> Sure, will drop it from the series if the above explanation and
> the rationale for the patch still does not convince you.

It really doesn't. This is only a bike-shedding exercise, which
introduce pointless churn.

Thanks,

	M.

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