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 <redacted>
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 <redacted>
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 <redacted>
---
arch/arm64/kvm/hyp/pgtable.c | 6 +++---
arch/arm64/mm/mmu.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
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 <redacted>
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 <redacted>
---
arch/arm64/kvm/arm.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
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 <redacted>
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 <redacted>
---
arch/arm64/kvm/arm.c | 14 --------------
1 file changed, 14 deletions(-)
@@ -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)returnerr;
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 <redacted>
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 <redacted>
---
arch/arm64/kvm/arm.c | 4 ----
1 file changed, 4 deletions(-)
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 <redacted>
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 <redacted>
---
arch/arm64/include/asm/kvm_mmu.h | 3 ++-
arch/arm64/kvm/reset.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
From: Will Deacon <will@kernel.org> Date: 2021-08-10 13:20:28
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 <redacted>
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 <redacted>
---
arch/arm64/kvm/hyp/pgtable.c | 6 +++---
arch/arm64/mm/mmu.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
From: Will Deacon <will@kernel.org> Date: 2021-08-10 13:21:57
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.
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-10 13:29:19
On 2021-08-10 08:02, Anshuman Khandual wrote:
quoted hunk
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 <redacted>
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 <redacted>
---
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
@@ -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))
@@ -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...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2021-08-10 13:29:57
On Tue, Aug 10, 2021 at 12:32:39PM +0530, Anshuman Khandual wrote:
quoted hunk
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 <redacted>
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 <redacted>
---
arch/arm64/kvm/arm.c | 14 --------------
1 file changed, 14 deletions(-)
@@ -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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-10 13:33:22
On 2021-08-10 08:02, Anshuman Khandual wrote:
quoted hunk
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 <redacted>
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 <redacted>
---
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
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...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Aug 10, 2021 at 12:32:38PM +0530, Anshuman Khandual wrote:
quoted
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.
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 <redacted>
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 <redacted>
---
arch/arm64/include/asm/kvm_mmu.h | 3 ++-
arch/arm64/kvm/reset.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
@@ -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))
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-10 15:32:07
On 2021-08-10 16:19, Anshuman Khandual wrote:
On 8/10/21 6:59 PM, Marc Zyngier wrote:
quoted
On 2021-08-10 08:02, Anshuman Khandual wrote:
quoted
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 <redacted>
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 <redacted>
---
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
@@ -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))
On Tue, Aug 10, 2021 at 12:32:37PM +0530, Anshuman Khandual wrote:
quoted
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 <redacted>
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 <redacted>
---
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/
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 <redacted>
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 <redacted>
---
arch/arm64/kvm/hyp/pgtable.c | 6 +++---
arch/arm64/mm/mmu.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Aug 10, 2021 at 12:32:39PM +0530, Anshuman Khandual wrote:
quoted
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 <redacted>
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 <redacted>
---
arch/arm64/kvm/arm.c | 14 --------------
1 file changed, 14 deletions(-)
@@ -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?
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-11 08:11:43
On Wed, 11 Aug 2021 06:34:46 +0100,
Anshuman Khandual [off-list ref] wrote:
On 8/10/21 7:03 PM, Marc Zyngier wrote:
quoted
On 2021-08-10 08:02, Anshuman Khandual wrote:
quoted
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 <redacted>
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 <redacted>
---
arch/arm64/kvm/hyp/pgtable.c | 6 +++---
arch/arm64/mm/mmu.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, 11 Aug 2021 06:34:46 +0100,
Anshuman Khandual [off-list ref] wrote:
quoted
On 8/10/21 7:03 PM, Marc Zyngier wrote:
quoted
On 2021-08-10 08:02, Anshuman Khandual wrote:
quoted
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 <redacted>
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 <redacted>
---
arch/arm64/kvm/hyp/pgtable.c | 6 +++---
arch/arm64/mm/mmu.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
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.
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-11 10:15:56
On Wed, 11 Aug 2021 10:37:36 +0100,
Anshuman Khandual [off-list ref] wrote:
On 8/11/21 1:41 PM, Marc Zyngier wrote:
quoted
On Wed, 11 Aug 2021 06:34:46 +0100,
Anshuman Khandual [off-list ref] wrote:
quoted
On 8/10/21 7:03 PM, Marc Zyngier wrote:
quoted
On 2021-08-10 08:02, Anshuman Khandual wrote:
quoted
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 <redacted>
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 <redacted>
---
arch/arm64/kvm/hyp/pgtable.c | 6 +++---
arch/arm64/mm/mmu.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
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.
quoted
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.
quoted
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! :-)
quoted
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel