flush_vcpu static function here is only used in one place which is just below
where it is defined. Inline the function to reduce the noise and clarify
what we are doing.

No functional change.

Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
 xen/arch/x86/mm/hap/hap.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 2f69ff9c7b..407c80afab 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -721,11 +721,6 @@ static pagetable_t cf_check hap_update_cr3(struct vcpu *v, bool noflush)
     return pagetable_null();
 }
 
-static bool flush_vcpu(const struct vcpu *v, const unsigned long *vcpu_bitmap)
-{
-    return !vcpu_bitmap || test_bit(v->vcpu_id, vcpu_bitmap);
-}
-
 /* Flush TLB of selected vCPUs.  NULL for all. */
 static bool cf_check flush_tlb(const unsigned long *vcpu_bitmap)
 {
@@ -742,7 +737,7 @@ static bool cf_check flush_tlb(const unsigned long *vcpu_bitmap)
     {
         unsigned int cpu;
 
-        if ( !flush_vcpu(v, vcpu_bitmap) )
+        if ( vcpu_bitmap && !test_bit(v->vcpu_id, vcpu_bitmap) )
             continue;
 
         hvm_asid_flush_vcpu(v);
-- 
2.51.0



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

On 29/09/2025 1:36 pm, Teddy Astie wrote:
> flush_vcpu static function here is only used in one place which is just below
> where it is defined. Inline the function to reduce the noise and clarify
> what we are doing.
>
> No functional change.
>
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>

Have you read the commit message introducing this pattern?  It's
11d9e114b53045e5f2009a26dad1d0d0f7df441e for reference.

The compiler will do the sensible thing.  What matters is the cognitive
complexity of the source code.

~Andrew

Le 29/09/2025 à 21:41, Andrew Cooper a écrit :
> On 29/09/2025 1:36 pm, Teddy Astie wrote:
>> flush_vcpu static function here is only used in one place which is just below
>> where it is defined. Inline the function to reduce the noise and clarify
>> what we are doing.
>>
>> No functional change.
>>
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> 
> Have you read the commit message introducing this pattern?  It's
> 11d9e114b53045e5f2009a26dad1d0d0f7df441e for reference.
> 

Yes, and while it makes sense in shadow paging code where we use it 
multiples times; it sounds to me a bit too much here to have such 
function just used once.

> The compiler will do the sensible thing.  What matters is the cognitive
> complexity of the source code.
> 
> ~Andrew
> 

Teddy


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

On Mon, Sep 29, 2025 at 12:36:30PM +0000, Teddy Astie wrote:
> flush_vcpu static function here is only used in one place which is just below
> where it is defined. Inline the function to reduce the noise and clarify
> what we are doing.

Did you check that the compiler doesn't inline it already?

It seems like an obvious optimization for the compiler to do.

> No functional change.
> 
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> ---
>  xen/arch/x86/mm/hap/hap.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 2f69ff9c7b..407c80afab 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -721,11 +721,6 @@ static pagetable_t cf_check hap_update_cr3(struct vcpu *v, bool noflush)
>      return pagetable_null();
>  }
>  
> -static bool flush_vcpu(const struct vcpu *v, const unsigned long *vcpu_bitmap)
> -{
> -    return !vcpu_bitmap || test_bit(v->vcpu_id, vcpu_bitmap);

The same construct is used in shadow code also, maybe it would be
helpful to place the flush_vcpu() helper in a common header as static
inline?

OTOH we don't care much for shadow, so it might be simpler to leave
shadow as-is and do the change just for HAP, but would be good to
mention in the commit message why shadow is not adjusted in the same
way.

Thanks, Roger.

Le 29/09/2025 à 16:09, Roger Pau Monné a écrit :
> On Mon, Sep 29, 2025 at 12:36:30PM +0000, Teddy Astie wrote:
>> flush_vcpu static function here is only used in one place which is just below
>> where it is defined. Inline the function to reduce the noise and clarify
>> what we are doing.
> 
> Did you check that the compiler doesn't inline it already?
> 
> It seems like an obvious optimization for the compiler to do.
> 

I assume that the compiler already does it, it's mostly meant to be a 
cosmetic change.

>> No functional change.
>>
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>> ---
>>   xen/arch/x86/mm/hap/hap.c | 7 +------
>>   1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
>> index 2f69ff9c7b..407c80afab 100644
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -721,11 +721,6 @@ static pagetable_t cf_check hap_update_cr3(struct vcpu *v, bool noflush)
>>       return pagetable_null();
>>   }
>>   
>> -static bool flush_vcpu(const struct vcpu *v, const unsigned long *vcpu_bitmap)
>> -{
>> -    return !vcpu_bitmap || test_bit(v->vcpu_id, vcpu_bitmap);
> 
> The same construct is used in shadow code also, maybe it would be
> helpful to place the flush_vcpu() helper in a common header as static
> inline?
> 

maybe, but given the simplicity of the function, it does feel a bit 
excessive it for hap code.

> OTOH we don't care much for shadow, so it might be simpler to leave
> shadow as-is and do the change just for HAP, but would be good to
> mention in the commit message why shadow is not adjusted in the same
> way.

Something like

"We only do this for hap where this function is only used once."

?

> 
> Thanks, Roger.
> 

Teddy


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech