[PATCH] KVM: arm64: Drop sort_memblock_regions()

[PATCH] KVM: arm64: Drop sort_memblock_regions()

[Gavin Shan]

Drop sort_memblock_regions() and avoid sorting the copied memory
regions to be ascending order on their base addresses, because the
source memory regions should have been sorted correctly when they
are added by memblock_add() or its variants.

This is generally reverting commit a14307f5310c ("KVM: arm64: Sort
the hypervisor memblocks"). No functional changes intended.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/arm64/kvm/pkvm.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/arch/arm64/kvm/pkvm.c b/arch/arm64/kvm/pkvm.c
index 930b677eb9b0..d9c9174f89a1 100644
--- a/arch/arm64/kvm/pkvm.c
+++ b/arch/arm64/kvm/pkvm.c
@@ -10,7 +10,6 @@
 #include <asm/kvm_mmu.h>
 #include <linux/memblock.h>
 #include <linux/mutex.h>
-#include <linux/sort.h>
 
 #include <asm/kvm_pkvm.h>
 
@@ -24,23 +23,6 @@ static unsigned int *hyp_memblock_nr_ptr = &kvm_nvhe_sym(hyp_memblock_nr);
 phys_addr_t hyp_mem_base;
 phys_addr_t hyp_mem_size;
 
-static int cmp_hyp_memblock(const void *p1, const void *p2)
-{
-	const struct memblock_region *r1 = p1;
-	const struct memblock_region *r2 = p2;
-
-	return r1->base < r2->base ? -1 : (r1->base > r2->base);
-}
-
-static void __init sort_memblock_regions(void)
-{
-	sort(hyp_memory,
-	     *hyp_memblock_nr_ptr,
-	     sizeof(struct memblock_region),
-	     cmp_hyp_memblock,
-	     NULL);
-}
-
 static int __init register_memblock_regions(void)
 {
 	struct memblock_region *reg;
@@ -52,7 +34,6 @@ static int __init register_memblock_regions(void)
 		hyp_memory[*hyp_memblock_nr_ptr] = *reg;
 		(*hyp_memblock_nr_ptr)++;
 	}
-	sort_memblock_regions();
 
 	return 0;
 }
-- 
2.48.1

Re: [PATCH] KVM: arm64: Drop sort_memblock_regions()

[Anshuman Khandual]

On 3/11/25 10:07, Gavin Shan wrote:
> Drop sort_memblock_regions() and avoid sorting the copied memory
> regions to be ascending order on their base addresses, because the
> source memory regions should have been sorted correctly when they
> are added by memblock_add() or its variants.
> 
> This is generally reverting commit a14307f5310c ("KVM: arm64: Sort
> the hypervisor memblocks"). No functional changes intended.

Just wondering what prompted this change ?

> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  arch/arm64/kvm/pkvm.c | 19 -------------------
>  1 file changed, 19 deletions(-)
> 
> diff --git a/arch/arm64/kvm/pkvm.c b/arch/arm64/kvm/pkvm.c
> index 930b677eb9b0..d9c9174f89a1 100644
> --- a/arch/arm64/kvm/pkvm.c
> +++ b/arch/arm64/kvm/pkvm.c
> @@ -10,7 +10,6 @@
>  #include <asm/kvm_mmu.h>
>  #include <linux/memblock.h>
>  #include <linux/mutex.h>
> -#include <linux/sort.h>
>  
>  #include <asm/kvm_pkvm.h>
>  
> @@ -24,23 +23,6 @@ static unsigned int *hyp_memblock_nr_ptr = &kvm_nvhe_sym(hyp_memblock_nr);
>  phys_addr_t hyp_mem_base;
>  phys_addr_t hyp_mem_size;
>  
> -static int cmp_hyp_memblock(const void *p1, const void *p2)
> -{
> -	const struct memblock_region *r1 = p1;
> -	const struct memblock_region *r2 = p2;
> -
> -	return r1->base < r2->base ? -1 : (r1->base > r2->base);
> -}
> -
> -static void __init sort_memblock_regions(void)
> -{
> -	sort(hyp_memory,
> -	     *hyp_memblock_nr_ptr,
> -	     sizeof(struct memblock_region),
> -	     cmp_hyp_memblock,
> -	     NULL);
> -}
> -
>  static int __init register_memblock_regions(void)
>  {
>  	struct memblock_region *reg;
> @@ -52,7 +34,6 @@ static int __init register_memblock_regions(void)
>  		hyp_memory[*hyp_memblock_nr_ptr] = *reg;
>  		(*hyp_memblock_nr_ptr)++;
>  	}
> -	sort_memblock_regions();
>  
>  	return 0;
>  }

Re: [PATCH] KVM: arm64: Drop sort_memblock_regions()

[Gavin Shan]

On 3/13/25 12:53 PM, Anshuman Khandual wrote:
> On 3/11/25 10:07, Gavin Shan wrote:
>> Drop sort_memblock_regions() and avoid sorting the copied memory
>> regions to be ascending order on their base addresses, because the
>> source memory regions should have been sorted correctly when they
>> are added by memblock_add() or its variants.
>>
>> This is generally reverting commit a14307f5310c ("KVM: arm64: Sort
>> the hypervisor memblocks"). No functional changes intended.
> 
> Just wondering what prompted this change ?
> 

I found the unnecessary sorting by code inspection. Hope there is nothing I
missed and it's why Quentin Perret has been copied, to confirm it. Commit
a14307f5310c was introduced by the initial series [1/2] to support pKVM.

[1] https://lore.kernel.org/kvmarm/20201117181607.1761516-1-qperret@google.com/
[2] https://lore.kernel.org/all/20210319100146.1149909-1-qperret@google.com/

Thanks,
Gavin

>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   arch/arm64/kvm/pkvm.c | 19 -------------------
>>   1 file changed, 19 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/pkvm.c b/arch/arm64/kvm/pkvm.c
>> index 930b677eb9b0..d9c9174f89a1 100644
>> --- a/arch/arm64/kvm/pkvm.c
>> +++ b/arch/arm64/kvm/pkvm.c
>> @@ -10,7 +10,6 @@
>>   #include <asm/kvm_mmu.h>
>>   #include <linux/memblock.h>
>>   #include <linux/mutex.h>
>> -#include <linux/sort.h>
>>   
>>   #include <asm/kvm_pkvm.h>
>>   
>> @@ -24,23 +23,6 @@ static unsigned int *hyp_memblock_nr_ptr = &kvm_nvhe_sym(hyp_memblock_nr);
>>   phys_addr_t hyp_mem_base;
>>   phys_addr_t hyp_mem_size;
>>   
>> -static int cmp_hyp_memblock(const void *p1, const void *p2)
>> -{
>> -	const struct memblock_region *r1 = p1;
>> -	const struct memblock_region *r2 = p2;
>> -
>> -	return r1->base < r2->base ? -1 : (r1->base > r2->base);
>> -}
>> -
>> -static void __init sort_memblock_regions(void)
>> -{
>> -	sort(hyp_memory,
>> -	     *hyp_memblock_nr_ptr,
>> -	     sizeof(struct memblock_region),
>> -	     cmp_hyp_memblock,
>> -	     NULL);
>> -}
>> -
>>   static int __init register_memblock_regions(void)
>>   {
>>   	struct memblock_region *reg;
>> @@ -52,7 +34,6 @@ static int __init register_memblock_regions(void)
>>   		hyp_memory[*hyp_memblock_nr_ptr] = *reg;
>>   		(*hyp_memblock_nr_ptr)++;
>>   	}
>> -	sort_memblock_regions();
>>   
>>   	return 0;
>>   }
> 

Re: [PATCH] KVM: arm64: Drop sort_memblock_regions()

[Will Deacon]

On Tue, Mar 11, 2025 at 02:37:18PM +1000, Gavin Shan wrote:
> Drop sort_memblock_regions() and avoid sorting the copied memory
> regions to be ascending order on their base addresses, because the
> source memory regions should have been sorted correctly when they
> are added by memblock_add() or its variants.
> 
> This is generally reverting commit a14307f5310c ("KVM: arm64: Sort
> the hypervisor memblocks"). No functional changes intended.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  arch/arm64/kvm/pkvm.c | 19 -------------------
>  1 file changed, 19 deletions(-)

It's not especially obvious from the memblock API that the memblocks
are sorted and so for_each_mem_region() will traverse the regions in
order. But it does appear to be true, so I suppose this is fine:

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

Will

Re: [PATCH] KVM: arm64: Drop sort_memblock_regions()

[Quentin Perret]

On Tuesday 11 Mar 2025 at 14:37:18 (+1000), Gavin Shan wrote:
> Drop sort_memblock_regions() and avoid sorting the copied memory
> regions to be ascending order on their base addresses, because the
> source memory regions should have been sorted correctly when they
> are added by memblock_add() or its variants.
> 
> This is generally reverting commit a14307f5310c ("KVM: arm64: Sort
> the hypervisor memblocks"). No functional changes intended.

I think this was originally introduced in an early version of the code
where the reserved regions were also registered, hence requiring
sorting. But yes, with the code as it is today I can't see what would
break without it, so:

  Reviewed-by: Quentin Perret <qperret@google.com>

Thanks!
Quentin

Re: [PATCH] KVM: arm64: Drop sort_memblock_regions()

[Gavin Shan]

Hi Marc/Oliver,

On 3/11/25 2:37 PM, Gavin Shan wrote:
> Drop sort_memblock_regions() and avoid sorting the copied memory
> regions to be ascending order on their base addresses, because the
> source memory regions should have been sorted correctly when they
> are added by memblock_add() or its variants.
> 
> This is generally reverting commit a14307f5310c ("KVM: arm64: Sort
> the hypervisor memblocks"). No functional changes intended.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>   arch/arm64/kvm/pkvm.c | 19 -------------------
>   1 file changed, 19 deletions(-)
> 

Please let me know if I need to resend after a rebase, or anything I need to
do so that it can be merged? :-)

Thanks,
Gavin

> diff --git a/arch/arm64/kvm/pkvm.c b/arch/arm64/kvm/pkvm.c
> index 930b677eb9b0..d9c9174f89a1 100644
> --- a/arch/arm64/kvm/pkvm.c
> +++ b/arch/arm64/kvm/pkvm.c
> @@ -10,7 +10,6 @@
>   #include <asm/kvm_mmu.h>
>   #include <linux/memblock.h>
>   #include <linux/mutex.h>
> -#include <linux/sort.h>
>   
>   #include <asm/kvm_pkvm.h>
>   
> @@ -24,23 +23,6 @@ static unsigned int *hyp_memblock_nr_ptr = &kvm_nvhe_sym(hyp_memblock_nr);
>   phys_addr_t hyp_mem_base;
>   phys_addr_t hyp_mem_size;
>   
> -static int cmp_hyp_memblock(const void *p1, const void *p2)
> -{
> -	const struct memblock_region *r1 = p1;
> -	const struct memblock_region *r2 = p2;
> -
> -	return r1->base < r2->base ? -1 : (r1->base > r2->base);
> -}
> -
> -static void __init sort_memblock_regions(void)
> -{
> -	sort(hyp_memory,
> -	     *hyp_memblock_nr_ptr,
> -	     sizeof(struct memblock_region),
> -	     cmp_hyp_memblock,
> -	     NULL);
> -}
> -
>   static int __init register_memblock_regions(void)
>   {
>   	struct memblock_region *reg;
> @@ -52,7 +34,6 @@ static int __init register_memblock_regions(void)
>   		hyp_memory[*hyp_memblock_nr_ptr] = *reg;
>   		(*hyp_memblock_nr_ptr)++;
>   	}
> -	sort_memblock_regions();
>   
>   	return 0;
>   }

Re: [PATCH] KVM: arm64: Drop sort_memblock_regions()

[Marc Zyngier]

On Tue, 11 Mar 2025 14:37:18 +1000, Gavin Shan wrote:
> Drop sort_memblock_regions() and avoid sorting the copied memory
> regions to be ascending order on their base addresses, because the
> source memory regions should have been sorted correctly when they
> are added by memblock_add() or its variants.
> 
> This is generally reverting commit a14307f5310c ("KVM: arm64: Sort
> the hypervisor memblocks"). No functional changes intended.
> 
> [...]

Applied to next, thanks!

[1/1] KVM: arm64: Drop sort_memblock_regions()
      commit: 00b0300cf1e265b8f3fa57106b940eff6f388d57

Cheers,

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