hv_vpset_nr_banks() incorrectly counts the number of bank by using hweight64()
instead of flsl(). This for instance problematic in case only the second bank
is selected (i.e >64 vCPUs, where here hweight64 gives 1), causing only the first
bank to be checked (non-valid) and the second (meaningful) one to be skipped.

Fixes: b4124682db6e ("viridian: add ExProcessorMasks variants of the flush hypercalls")
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
 xen/arch/x86/hvm/viridian/viridian.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 90e749ceb5..f5e526241d 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -601,7 +601,7 @@ static DEFINE_PER_CPU(union hypercall_vpset, hypercall_vpset);
 
 static unsigned int hv_vpset_nr_banks(struct hv_vpset *vpset)
 {
-    return hweight64(vpset->valid_bank_mask);
+    return fls64(vpset->valid_bank_mask);
 }
 
 static int hv_vpset_to_vpmask(const struct hv_vpset *in, paddr_t bank_gpa,
-- 
2.51.2



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

On 01/12/2025 4:29 pm, Teddy Astie wrote:
> hv_vpset_nr_banks() incorrectly counts the number of bank by using hweight64()
> instead of flsl(). This for instance problematic in case only the second bank
> is selected (i.e >64 vCPUs, where here hweight64 gives 1), causing only the first
> bank to be checked (non-valid) and the second (meaningful) one to be skipped.
>
> Fixes: b4124682db6e ("viridian: add ExProcessorMasks variants of the flush hypercalls")
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> ---
>  xen/arch/x86/hvm/viridian/viridian.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
> index 90e749ceb5..f5e526241d 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -601,7 +601,7 @@ static DEFINE_PER_CPU(union hypercall_vpset, hypercall_vpset);
>  
>  static unsigned int hv_vpset_nr_banks(struct hv_vpset *vpset)
>  {
> -    return hweight64(vpset->valid_bank_mask);
> +    return fls64(vpset->valid_bank_mask);
>  }
>  
>  static int hv_vpset_to_vpmask(const struct hv_vpset *in, paddr_t bank_gpa,

https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/datatypes/hv_vp_set#processor-set-example

This example is poor (it has several errors in the text), but the final
statement says fairly clearly that the banks are tightly packed and not
contiguous.

Therefore, hweight64() the correct calculation, and AFAICT, the single
use of this helper in Xen correct.

~Andrew

Le 01/12/2025 à 18:48, Andrew Cooper a écrit :
> On 01/12/2025 4:29 pm, Teddy Astie wrote:
>> hv_vpset_nr_banks() incorrectly counts the number of bank by using hweight64()
>> instead of flsl(). This for instance problematic in case only the second bank
>> is selected (i.e >64 vCPUs, where here hweight64 gives 1), causing only the first
>> bank to be checked (non-valid) and the second (meaningful) one to be skipped.
>>
>> Fixes: b4124682db6e ("viridian: add ExProcessorMasks variants of the flush hypercalls")
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>> ---
>>   xen/arch/x86/hvm/viridian/viridian.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
>> index 90e749ceb5..f5e526241d 100644
>> --- a/xen/arch/x86/hvm/viridian/viridian.c
>> +++ b/xen/arch/x86/hvm/viridian/viridian.c
>> @@ -601,7 +601,7 @@ static DEFINE_PER_CPU(union hypercall_vpset, hypercall_vpset);
>>   
>>   static unsigned int hv_vpset_nr_banks(struct hv_vpset *vpset)
>>   {
>> -    return hweight64(vpset->valid_bank_mask);
>> +    return fls64(vpset->valid_bank_mask);
>>   }
>>   
>>   static int hv_vpset_to_vpmask(const struct hv_vpset *in, paddr_t bank_gpa,
> 
> https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/datatypes/hv_vp_set#processor-set-example
> 
> This example is poor (it has several errors in the text), but the final
> statement says fairly clearly that the banks are tightly packed and not
> contiguous.
> 
> Therefore, hweight64() the correct calculation, and AFAICT, the single
> use of this helper in Xen correct.
> 

Ok, I see what you mean, such that only set bit of the mask imply a bank 
in the "BankContents". Which means that 2 set bits (regardless of their 
position) means 2 banks (regardless of which one).

But that looks contradictory with the second sentence
> The total set of virtual processors is split up into chunks of 64, known as a “bank”. For example,
> processors 0-63 are in bank 0, 64-127 are in bank 1, and so on.

Even though, all the banks and "BankContents" are different things.

Interestingly, all open-source users of this hypercall I found fills the 
mask with 1s (up to the maximum vCPU they want to target). So in such 
case the layout is fully contiguous at the end.

> ~Andrew
> 



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech