[PATCH 0/4] xen/bitops: More for_each_bit() conversions

[PATCH 0/4] xen/bitops: More for_each_bit() conversions

[Andrew Cooper]

All patches are independent.  Patch 4 has been posted before, with only a
request for some wording changes.

https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1428997960

Andrew Cooper (4):
  xen/evtchn: Use bitmap_for_each() in evtchn_check_pollers()
  x86/hvm: Use for_each_set_bit() in hvm_emulate_writeback()
  x86/hvm: Rework hpet_write() for improved code generation
  x86/vmx: Rewrite vmx_sync_pir_to_irr() to be more efficient

 xen/arch/x86/hvm/emulate.c             | 20 ++++----
 xen/arch/x86/hvm/hpet.c                | 29 +++--------
 xen/arch/x86/hvm/vmx/vmx.c             | 70 +++++++++++++++++++++++---
 xen/arch/x86/include/asm/hvm/emulate.h |  4 +-
 xen/common/event_channel.c             |  4 +-
 5 files changed, 83 insertions(+), 44 deletions(-)

-- 
2.39.2

[PATCH 1/4] xen/evtchn: Use bitmap_for_each() in evtchn_check_pollers()

[Andrew Cooper]

It is a preprocessor-identical opencoding.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
---
 xen/common/event_channel.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 13b97c94d7fa..8db2ca4ba23e 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -1517,9 +1517,7 @@ void evtchn_check_pollers(struct domain *d, unsigned int port)
         return;
 
     /* Wake any interested (or potentially interested) pollers. */
-    for ( vcpuid = find_first_bit(d->poll_mask, d->max_vcpus);
-          vcpuid < d->max_vcpus;
-          vcpuid = find_next_bit(d->poll_mask, d->max_vcpus, vcpuid+1) )
+    bitmap_for_each ( vcpuid, d->poll_mask, d->max_vcpus )
     {
         v = d->vcpu[vcpuid];
         if ( ((v->poll_evtchn <= 0) || (v->poll_evtchn == port)) &&
-- 
2.39.2

Re: [PATCH 1/4] xen/evtchn: Use bitmap_for_each() in evtchn_check_pollers()

[Jan Beulich]

On 27.08.2024 15:57, Andrew Cooper wrote:
> It is a preprocessor-identical opencoding.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

[PATCH 2/4] x86/hvm: Use for_each_set_bit() in hvm_emulate_writeback()

[Andrew Cooper]

... which is more consise than the opencoded form.

Also, for production VMs, ~100% of emulations are simple MOVs, so it is likely
that there are no segments to write back.

Furthermore, now that find_{first,next}_bit() are no longer in use, the
seg_reg_{accessed,dirty} fields aren't forced to be unsigned long, although
they do need to remain unsigned int because of __set_bit() elsewhere.

No practical change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

Pulling current out into curr is good for code generation.  When using current
in the loop, GCC can't retain the calculation across the call to
hvm_set_segment_register() and is forced to re-read from the cpu_info block.

However, if curr is initialised, it's calculated even in the likely path...
---
 xen/arch/x86/hvm/emulate.c             | 20 ++++++++++----------
 xen/arch/x86/include/asm/hvm/emulate.h |  4 ++--
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index feb4792cc567..732bdbab25b0 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2908,18 +2908,18 @@ void hvm_emulate_init_per_insn(
 void hvm_emulate_writeback(
     struct hvm_emulate_ctxt *hvmemul_ctxt)
 {
-    enum x86_segment seg;
+    struct vcpu *curr;
+    unsigned int dirty = hvmemul_ctxt->seg_reg_dirty;
 
-    seg = find_first_bit(&hvmemul_ctxt->seg_reg_dirty,
-                         ARRAY_SIZE(hvmemul_ctxt->seg_reg));
+    if ( likely(!dirty) )
+        return;
 
-    while ( seg < ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
-    {
-        hvm_set_segment_register(current, seg, &hvmemul_ctxt->seg_reg[seg]);
-        seg = find_next_bit(&hvmemul_ctxt->seg_reg_dirty,
-                            ARRAY_SIZE(hvmemul_ctxt->seg_reg),
-                            seg+1);
-    }
+    curr = current;
+
+    for_each_set_bit ( seg, dirty )
+        hvm_set_segment_register(curr, seg, &hvmemul_ctxt->seg_reg[seg]);
+
+    hvmemul_ctxt->seg_reg_dirty = 0;
 }
 
 /*
diff --git a/xen/arch/x86/include/asm/hvm/emulate.h b/xen/arch/x86/include/asm/hvm/emulate.h
index 29d679442e10..972cdf1fa0cf 100644
--- a/xen/arch/x86/include/asm/hvm/emulate.h
+++ b/xen/arch/x86/include/asm/hvm/emulate.h
@@ -36,8 +36,8 @@ struct hvm_emulate_ctxt {
     unsigned int insn_buf_bytes;
 
     struct segment_register seg_reg[10];
-    unsigned long seg_reg_accessed;
-    unsigned long seg_reg_dirty;
+    unsigned int seg_reg_accessed;
+    unsigned int seg_reg_dirty;
 
     /*
      * MFNs behind temporary mappings in the write callback.  The length is
-- 
2.39.2

Re: [PATCH 2/4] x86/hvm: Use for_each_set_bit() in hvm_emulate_writeback()

[Jan Beulich]

On 27.08.2024 15:57, Andrew Cooper wrote:
> ... which is more consise than the opencoded form.
> 
> Also, for production VMs, ~100% of emulations are simple MOVs, so it is likely
> that there are no segments to write back.
> 
> Furthermore, now that find_{first,next}_bit() are no longer in use, the
> seg_reg_{accessed,dirty} fields aren't forced to be unsigned long, although
> they do need to remain unsigned int because of __set_bit() elsewhere.
> 
> No practical change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> Pulling current out into curr is good for code generation.  When using current
> in the loop, GCC can't retain the calculation across the call to
> hvm_set_segment_register() and is forced to re-read from the cpu_info block.
> 
> However, if curr is initialised, it's calculated even in the likely path...

That's a little odd, as I don't think I can spot what would force the compiler
into doing so. As a wild guess, ...

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2908,18 +2908,18 @@ void hvm_emulate_init_per_insn(
>  void hvm_emulate_writeback(
>      struct hvm_emulate_ctxt *hvmemul_ctxt)
>  {
> -    enum x86_segment seg;
> +    struct vcpu *curr;
> +    unsigned int dirty = hvmemul_ctxt->seg_reg_dirty;

... is the order of these two possibly relevant? Yet of course it's not the
end of the world whichever way it's done.

> -    seg = find_first_bit(&hvmemul_ctxt->seg_reg_dirty,
> -                         ARRAY_SIZE(hvmemul_ctxt->seg_reg));
> +    if ( likely(!dirty) )
> +        return;
>  
> -    while ( seg < ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
> -    {
> -        hvm_set_segment_register(current, seg, &hvmemul_ctxt->seg_reg[seg]);
> -        seg = find_next_bit(&hvmemul_ctxt->seg_reg_dirty,
> -                            ARRAY_SIZE(hvmemul_ctxt->seg_reg),
> -                            seg+1);
> -    }
> +    curr = current;
> +
> +    for_each_set_bit ( seg, dirty )
> +        hvm_set_segment_register(curr, seg, &hvmemul_ctxt->seg_reg[seg]);
> +
> +    hvmemul_ctxt->seg_reg_dirty = 0;

Why is this suddenly appearing here? You don't mention it in the description,
so it's not clear whether you found a (however minor) issue, or whether
that's purely cosmetic (yet then it's still an extra store we could do
without).

Jan

Re: [PATCH 2/4] x86/hvm: Use for_each_set_bit() in hvm_emulate_writeback()

[Andrew Cooper]

On 27/08/2024 5:07 pm, Jan Beulich wrote:
> On 27.08.2024 15:57, Andrew Cooper wrote:
>> ... which is more consise than the opencoded form.
>>
>> Also, for production VMs, ~100% of emulations are simple MOVs, so it is likely
>> that there are no segments to write back.
>>
>> Furthermore, now that find_{first,next}_bit() are no longer in use, the
>> seg_reg_{accessed,dirty} fields aren't forced to be unsigned long, although
>> they do need to remain unsigned int because of __set_bit() elsewhere.
>>
>> No practical change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Pulling current out into curr is good for code generation.  When using current
>> in the loop, GCC can't retain the calculation across the call to
>> hvm_set_segment_register() and is forced to re-read from the cpu_info block.
>>
>> However, if curr is initialised, it's calculated even in the likely path...
> That's a little odd, as I don't think I can spot what would force the compiler
> into doing so. As a wild guess, ...
>
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -2908,18 +2908,18 @@ void hvm_emulate_init_per_insn(
>>  void hvm_emulate_writeback(
>>      struct hvm_emulate_ctxt *hvmemul_ctxt)
>>  {
>> -    enum x86_segment seg;
>> +    struct vcpu *curr;
>> +    unsigned int dirty = hvmemul_ctxt->seg_reg_dirty;
> ... is the order of these two possibly relevant? Yet of course it's not the
> end of the world whichever way it's done.

My general conclusion is that GCC doesn't try very hard to optimise
likely() early exits.

I suspect there's some reasonably low hanging fruit to be found there.

>
>> -    seg = find_first_bit(&hvmemul_ctxt->seg_reg_dirty,
>> -                         ARRAY_SIZE(hvmemul_ctxt->seg_reg));
>> +    if ( likely(!dirty) )
>> +        return;
>>  
>> -    while ( seg < ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
>> -    {
>> -        hvm_set_segment_register(current, seg, &hvmemul_ctxt->seg_reg[seg]);
>> -        seg = find_next_bit(&hvmemul_ctxt->seg_reg_dirty,
>> -                            ARRAY_SIZE(hvmemul_ctxt->seg_reg),
>> -                            seg+1);
>> -    }
>> +    curr = current;
>> +
>> +    for_each_set_bit ( seg, dirty )
>> +        hvm_set_segment_register(curr, seg, &hvmemul_ctxt->seg_reg[seg]);
>> +
>> +    hvmemul_ctxt->seg_reg_dirty = 0;
> Why is this suddenly appearing here? You don't mention it in the description,
> so it's not clear whether you found a (however minor) issue, or whether
> that's purely cosmetic (yet then it's still an extra store we could do
> without).

Oh, yes.  Nothing anywhere in Xen ever clears these segment dirty bits.

I suspect the worst that will go wrong is that we'll waste time
re-{VMWRITE,memcpy}-ing the segment registers into the VMCS/VMCB, but
the logic in Xen is definitely not right.

~Andrew

Re: [PATCH 2/4] x86/hvm: Use for_each_set_bit() in hvm_emulate_writeback()

[Jan Beulich]

On 28.08.2024 16:44, Andrew Cooper wrote:
> On 27/08/2024 5:07 pm, Jan Beulich wrote:
>> On 27.08.2024 15:57, Andrew Cooper wrote:
>>> +    for_each_set_bit ( seg, dirty )
>>> +        hvm_set_segment_register(curr, seg, &hvmemul_ctxt->seg_reg[seg]);
>>> +
>>> +    hvmemul_ctxt->seg_reg_dirty = 0;
>> Why is this suddenly appearing here? You don't mention it in the description,
>> so it's not clear whether you found a (however minor) issue, or whether
>> that's purely cosmetic (yet then it's still an extra store we could do
>> without).
> 
> Oh, yes.  Nothing anywhere in Xen ever clears these segment dirty bits.

hvm_emulate_init_once()?

> I suspect the worst that will go wrong is that we'll waste time
> re-{VMWRITE,memcpy}-ing the segment registers into the VMCS/VMCB, but
> the logic in Xen is definitely not right.

I'm on the edge of asking to do such clearing before emulation, not after
processing the dirty bits. That would then be hvm_emulate_init_per_insn(),
well centralized.

Jan

Re: [PATCH 2/4] x86/hvm: Use for_each_set_bit() in hvm_emulate_writeback()

[Andrew Cooper]

On 28/08/2024 3:56 pm, Jan Beulich wrote:
> On 28.08.2024 16:44, Andrew Cooper wrote:
>> On 27/08/2024 5:07 pm, Jan Beulich wrote:
>>> On 27.08.2024 15:57, Andrew Cooper wrote:
>>>> +    for_each_set_bit ( seg, dirty )
>>>> +        hvm_set_segment_register(curr, seg, &hvmemul_ctxt->seg_reg[seg]);
>>>> +
>>>> +    hvmemul_ctxt->seg_reg_dirty = 0;
>>> Why is this suddenly appearing here? You don't mention it in the description,
>>> so it's not clear whether you found a (however minor) issue, or whether
>>> that's purely cosmetic (yet then it's still an extra store we could do
>>> without).
>> Oh, yes.  Nothing anywhere in Xen ever clears these segment dirty bits.
> hvm_emulate_init_once()?

I meant after emulation.  The value is initialised to 0 at the start of day.

>
>> I suspect the worst that will go wrong is that we'll waste time
>> re-{VMWRITE,memcpy}-ing the segment registers into the VMCS/VMCB, but
>> the logic in Xen is definitely not right.
> I'm on the edge of asking to do such clearing before emulation, not after
> processing the dirty bits. That would then be hvm_emulate_init_per_insn(),
> well centralized.

Specifically, hvmemul_ctxt should not believe itself to be dirty after a
call to hvm_emulate_writeback(), because that's the logic to make the
context no-longer-dirty.

That said, the more I look at this, the less convinced I am by it.  For
a function named writeback(), it's doing a very narrow thing that is not
the usual meaning of the term when it comes to pipelines or insn
emulation...

~Andrew

Re: [PATCH 2/4] x86/hvm: Use for_each_set_bit() in hvm_emulate_writeback()

[Jan Beulich]

On 28.08.2024 20:56, Andrew Cooper wrote:
> On 28/08/2024 3:56 pm, Jan Beulich wrote:
>> On 28.08.2024 16:44, Andrew Cooper wrote:
>>> On 27/08/2024 5:07 pm, Jan Beulich wrote:
>>>> On 27.08.2024 15:57, Andrew Cooper wrote:
>>>>> +    for_each_set_bit ( seg, dirty )
>>>>> +        hvm_set_segment_register(curr, seg, &hvmemul_ctxt->seg_reg[seg]);
>>>>> +
>>>>> +    hvmemul_ctxt->seg_reg_dirty = 0;
>>>> Why is this suddenly appearing here? You don't mention it in the description,
>>>> so it's not clear whether you found a (however minor) issue, or whether
>>>> that's purely cosmetic (yet then it's still an extra store we could do
>>>> without).
>>> Oh, yes.  Nothing anywhere in Xen ever clears these segment dirty bits.
>> hvm_emulate_init_once()?
> 
> I meant after emulation.  The value is initialised to 0 at the start of day.
> 
>>
>>> I suspect the worst that will go wrong is that we'll waste time
>>> re-{VMWRITE,memcpy}-ing the segment registers into the VMCS/VMCB, but
>>> the logic in Xen is definitely not right.
>> I'm on the edge of asking to do such clearing before emulation, not after
>> processing the dirty bits. That would then be hvm_emulate_init_per_insn(),
>> well centralized.
> 
> Specifically, hvmemul_ctxt should not believe itself to be dirty after a
> call to hvm_emulate_writeback(), because that's the logic to make the
> context no-longer-dirty.

That's one aspect, yes. Debuggability is another. For that retaining state
until it strictly needs clearing out may be helpful. Plus ...

> That said, the more I look at this, the less convinced I am by it.  For
> a function named writeback(), it's doing a very narrow thing that is not
> the usual meaning of the term when it comes to pipelines or insn
> emulation...

... as you say here.

Anyway - I'll leave where to put the clearing to you, just as long as it's
at least mentioned in the description.

Jan

[PATCH 3/4] x86/hvm: Rework hpet_write() for improved code generation

[Andrew Cooper]

In the HPET_STATUS handling, the use of __clear_bit(i, &new_val) is the only
thing causing it to be spilled to the stack.  Furthemore we only care about
the bottom 3 bits, so rewrite it to be a plain for loop.

For the {start,stop}_timer variables, these are spilled to the stack despite
the __{set,clear}_bit() calls.  Again we only care about the bottom 3 bits, so
shrink the variables from long to int.  Use for_each_set_bit() rather than
opencoding it at the end which amongst other things means the loop predicate
is no longer forced to the stack by the loop body.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

All in all, it's modest according to bloat-o-meter:

  add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-29 (-29)
  Function                                     old     new   delta
  hpet_write                                  2225    2196     -29

but we have shrunk the stack frame by 8 bytes; 0x28 as opposed to 0x30 before.
---
 xen/arch/x86/hvm/hpet.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 87642575f9cd..e3981d5e467c 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -349,8 +349,7 @@ static int cf_check hpet_write(
     unsigned int tn, i;
 
     /* Acculumate a bit mask of timers whos state is changed by this write. */
-    unsigned long start_timers = 0;
-    unsigned long stop_timers  = 0;
+    unsigned int start_timers = 0, stop_timers = 0;
 #define set_stop_timer(n)    (__set_bit((n), &stop_timers))
 #define set_start_timer(n)   (__set_bit((n), &start_timers))
 #define set_restart_timer(n) (set_stop_timer(n),set_start_timer(n))
@@ -405,16 +404,12 @@ static int cf_check hpet_write(
 
     case HPET_STATUS:
         /* write 1 to clear. */
-        while ( new_val )
+        for ( i = 0; i < HPET_TIMER_NUM; i++ )
         {
-            bool active;
-
-            i = ffsl(new_val) - 1;
-            if ( i >= HPET_TIMER_NUM )
-                break;
-            __clear_bit(i, &new_val);
-            active = __test_and_clear_bit(i, &h->hpet.isr);
-            if ( active )
+            if ( !(new_val & (1U << i)) )
+                continue;
+
+            if ( __test_and_clear_bit(i, &h->hpet.isr) )
             {
                 hvm_ioapic_deassert(v->domain, timer_int_route(h, i));
                 if ( hpet_enabled(h) && timer_enabled(h, i) &&
@@ -533,19 +528,11 @@ static int cf_check hpet_write(
     }
 
     /* stop/start timers whos state was changed by this write. */
-    while (stop_timers)
-    {
-        i = ffsl(stop_timers) - 1;
-        __clear_bit(i, &stop_timers);
+    for_each_set_bit ( i, stop_timers )
         hpet_stop_timer(h, i, guest_time);
-    }
 
-    while (start_timers)
-    {
-        i = ffsl(start_timers) - 1;
-        __clear_bit(i, &start_timers);
+    for_each_set_bit ( i, start_timers )
         hpet_set_timer(h, i, guest_time);
-    }
 
 #undef set_stop_timer
 #undef set_start_timer
-- 
2.39.2

Re: [PATCH 3/4] x86/hvm: Rework hpet_write() for improved code generation

[Jan Beulich]

On 27.08.2024 15:57, Andrew Cooper wrote:
> In the HPET_STATUS handling, the use of __clear_bit(i, &new_val) is the only
> thing causing it to be spilled to the stack.  Furthemore we only care about
> the bottom 3 bits, so rewrite it to be a plain for loop.
> 
> For the {start,stop}_timer variables, these are spilled to the stack despite
> the __{set,clear}_bit() calls.

That's an observation from what the compiler happens to do? I don't see any
other reason why they would need spilling; I expect it's merely a matter of
registers better be used for other variables. If we ever meant to build Xen
with APX fully in use, that might change. IOW may I at least ask for
s/are/happen to be/? I'm also a little irritated by "despite", but you're
the native speaker. It would have seemed to me that e.g. "irrespective of"
would better express what (I think) is meant.

>  Again we only care about the bottom 3 bits, so
> shrink the variables from long to int.  Use for_each_set_bit() rather than
> opencoding it at the end which amongst other things means the loop predicate
> is no longer forced to the stack by the loop body.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> All in all, it's modest according to bloat-o-meter:
> 
>   add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-29 (-29)
>   Function                                     old     new   delta
>   hpet_write                                  2225    2196     -29
> 
> but we have shrunk the stack frame by 8 bytes; 0x28 as opposed to 0x30 before.

However, on the negative side all the first of the loops you touch now always
takes 3 iterations, when previously we may have got away with as little as
none. Is there a reason not to use

    for_each_set_bit ( i, new_val & ((1U << HPET_TIMER_NUM) - 1) )

there (with the masking of the low bit possibly pulled out)?

> @@ -533,19 +528,11 @@ static int cf_check hpet_write(
>      }
>  
>      /* stop/start timers whos state was changed by this write. */
> -    while (stop_timers)
> -    {
> -        i = ffsl(stop_timers) - 1;
> -        __clear_bit(i, &stop_timers);
> +    for_each_set_bit ( i, stop_timers )
>          hpet_stop_timer(h, i, guest_time);
> -    }
>  
> -    while (start_timers)
> -    {
> -        i = ffsl(start_timers) - 1;
> -        __clear_bit(i, &start_timers);
> +    for_each_set_bit ( i, start_timers )
>          hpet_set_timer(h, i, guest_time);
> -    }

To avoid variable shadowing, I think you don't want to use i in these two
loops. Alternatively the function scope i would need constraining to the
individual loops.

Unrelated to the change you make, but related to the code you touch: Isn't
there a bug there with the length != 8 handling ahead of the switch()? The
bits being write-1-to-clear, using the value read for parts the original
insn didn't write means we might clear ISR bits we weren't asked to clear.
I guess I'll make a patch, which may want to go ahead of yours for ease of
backporting. (Of course guests should have no need to write to other than
the bottom part of the register, but still.)

Jan

Re: [PATCH 3/4] x86/hvm: Rework hpet_write() for improved code generation

[Andrew Cooper]

On 28/08/2024 9:13 am, Jan Beulich wrote:
> On 27.08.2024 15:57, Andrew Cooper wrote:
>> In the HPET_STATUS handling, the use of __clear_bit(i, &new_val) is the only
>> thing causing it to be spilled to the stack.  Furthemore we only care about
>> the bottom 3 bits, so rewrite it to be a plain for loop.
>>
>> For the {start,stop}_timer variables, these are spilled to the stack despite
>> the __{set,clear}_bit() calls.
> That's an observation from what the compiler happens to do? I don't see any
> other reason why they would need spilling; I expect it's merely a matter of
> registers better be used for other variables.

It is a consequence of how our helpers are written.  I do expect it to
improve when I get around to reworking them.

For example, the Linux helpers have enough constant folding capabilities
to allow the compiler to turn:

{
    int foo = 0;
    ...
    __set_bit(1, &foo);

into:

{
    int foo = 1;


as well as being able to emit LOCK AND/OR/XOR in place of LOCK BT{C,S,R}
for a constant bit position.

One thing I want to do, which I haven't figured out how to do yet, is to
allow the arch form to emit BT?Q forms.

Right now, code generation for PGC_* and PGT_* suffers quite a lot.  We
mix between reg/imm logic, then spill to the stack because top bits
aren't within range for the "I" constraint on 32-bit instructions, issue
a BT?L reg/mem (which has much higher latency than any other form), then
pick it back off the stack to do more reg/imm logic.

I was wondering if, because of the always_inline, I could do something
like __builtin_constant_p(bit) && __builtin_object_size(addr, 0) >= 8
and emitting long-granular logic, which will be able to pick the imm/reg
form rather than turning into reg/mem.

But, I've not had time to experiment here, and I doubt I'll get around
to it soon.

Another optimisation we're lacking vs Linux is that our test_bit() has a
volatile pointer where Linux's is non-volatile.  This makes a massive
difference for the ability to optimise looking at multiple bits.


>  If we ever meant to build Xen
> with APX fully in use, that might change. IOW may I at least ask for
> s/are/happen to be/? I'm also a little irritated by "despite", but you're
> the native speaker. It would have seemed to me that e.g. "irrespective of"
> would better express what (I think) is meant.

"despite" isn't really the right term, but I also wouldn't have said it
was something to be irritated over.

What I was trying to say was "they're spilled to the stack even with the
__set_bit() calls removed".  Which makes sense; they're values held for
almost the full duration of the function, that are not used in ~every
step of logic.

Interestingly, given that they're spilled to the stack, the __set_bit()
form is more efficient than the plain C "|= (1u << i);", but I'd still
like an implementation which could make that determination itself.

>
>>  Again we only care about the bottom 3 bits, so
>> shrink the variables from long to int.  Use for_each_set_bit() rather than
>> opencoding it at the end which amongst other things means the loop predicate
>> is no longer forced to the stack by the loop body.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>
>> All in all, it's modest according to bloat-o-meter:
>>
>>   add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-29 (-29)
>>   Function                                     old     new   delta
>>   hpet_write                                  2225    2196     -29
>>
>> but we have shrunk the stack frame by 8 bytes; 0x28 as opposed to 0x30 before.
> However, on the negative side all the first of the loops you touch now always
> takes 3 iterations, when previously we may have got away with as little as
> none. Is there a reason not to use
>
>     for_each_set_bit ( i, new_val & ((1U << HPET_TIMER_NUM) - 1) )
>
> there (with the masking of the low bit possibly pulled out)?

There are multiple angles here.

First, I got an unexpected surprise on ARM with an expression, and while
this one won't pick up pointer const-ness, I can never remember what
MISRA's view on this is.

Second, this is the odd-loop-out compared to rest of the function, which
are all of the form "for ( i = 0; i < HPET_TIMER_NUM ;".

But perhaps most importantly, OSes don't touch this register.  Xen not
at all, and Linux only in _hpet_print_config().  Neither bother
preserving/clearing it on suspend/resume, even when running the HPET in
legacy replacement mode.

I haven't checked windows behaviour, but I don't expect it to differ
here.  This register simply isn't interesting for the preferred type of
interrupts (edge), and also isn't useful for an ISR handling a line
interrupt.

So my choice was based on which produced the smallest code, because it's
an dead-in-practice codepath.

>
>> @@ -533,19 +528,11 @@ static int cf_check hpet_write(
>>      }
>>  
>>      /* stop/start timers whos state was changed by this write. */
>> -    while (stop_timers)
>> -    {
>> -        i = ffsl(stop_timers) - 1;
>> -        __clear_bit(i, &stop_timers);
>> +    for_each_set_bit ( i, stop_timers )
>>          hpet_stop_timer(h, i, guest_time);
>> -    }
>>  
>> -    while (start_timers)
>> -    {
>> -        i = ffsl(start_timers) - 1;
>> -        __clear_bit(i, &start_timers);
>> +    for_each_set_bit ( i, start_timers )
>>          hpet_set_timer(h, i, guest_time);
>> -    }
> To avoid variable shadowing, I think you don't want to use i in these two
> loops. Alternatively the function scope i would need constraining to the
> individual loops.

Yeah, I was bitten by that on one of the ARM patches.  I'll adjust.

~Andrew

Re: [PATCH 3/4] x86/hvm: Rework hpet_write() for improved code generation

[Jan Beulich]

On 28.08.2024 19:50, Andrew Cooper wrote:
> On 28/08/2024 9:13 am, Jan Beulich wrote:
>> On 27.08.2024 15:57, Andrew Cooper wrote:
>>> In the HPET_STATUS handling, the use of __clear_bit(i, &new_val) is the only
>>> thing causing it to be spilled to the stack.  Furthemore we only care about
>>> the bottom 3 bits, so rewrite it to be a plain for loop.
>>>
>>> For the {start,stop}_timer variables, these are spilled to the stack despite
>>> the __{set,clear}_bit() calls.
>> That's an observation from what the compiler happens to do? I don't see any
>> other reason why they would need spilling; I expect it's merely a matter of
>> registers better be used for other variables.
> 
> It is a consequence of how our helpers are written.  I do expect it to
> improve when I get around to reworking them.
> 
> For example, the Linux helpers have enough constant folding capabilities
> to allow the compiler to turn:
> 
> {
>     int foo = 0;
>     ...
>     __set_bit(1, &foo);
> 
> into:
> 
> {
>     int foo = 1;
> 
> 
> as well as being able to emit LOCK AND/OR/XOR in place of LOCK BT{C,S,R}
> for a constant bit position.
> 
> One thing I want to do, which I haven't figured out how to do yet, is to
> allow the arch form to emit BT?Q forms.
> 
> Right now, code generation for PGC_* and PGT_* suffers quite a lot.  We
> mix between reg/imm logic, then spill to the stack because top bits
> aren't within range for the "I" constraint on 32-bit instructions, issue
> a BT?L reg/mem (which has much higher latency than any other form), then
> pick it back off the stack to do more reg/imm logic.
> 
> I was wondering if, because of the always_inline, I could do something
> like __builtin_constant_p(bit) && __builtin_object_size(addr, 0) >= 8
> and emitting long-granular logic, which will be able to pick the imm/reg
> form rather than turning into reg/mem.

That may work, provided there actually was always_inline.

>>  If we ever meant to build Xen
>> with APX fully in use, that might change. IOW may I at least ask for
>> s/are/happen to be/? I'm also a little irritated by "despite", but you're
>> the native speaker. It would have seemed to me that e.g. "irrespective of"
>> would better express what (I think) is meant.
> 
> "despite" isn't really the right term, but I also wouldn't have said it
> was something to be irritated over.
> 
> What I was trying to say was "they're spilled to the stack even with the
> __set_bit() calls removed".  Which makes sense; they're values held for
> almost the full duration of the function, that are not used in ~every
> step of logic.

Right, the "not a good use for a register var" reason that I had alluded to.

>>>  Again we only care about the bottom 3 bits, so
>>> shrink the variables from long to int.  Use for_each_set_bit() rather than
>>> opencoding it at the end which amongst other things means the loop predicate
>>> is no longer forced to the stack by the loop body.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> All in all, it's modest according to bloat-o-meter:
>>>
>>>   add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-29 (-29)
>>>   Function                                     old     new   delta
>>>   hpet_write                                  2225    2196     -29
>>>
>>> but we have shrunk the stack frame by 8 bytes; 0x28 as opposed to 0x30 before.
>> However, on the negative side all the first of the loops you touch now always
>> takes 3 iterations, when previously we may have got away with as little as
>> none. Is there a reason not to use
>>
>>     for_each_set_bit ( i, new_val & ((1U << HPET_TIMER_NUM) - 1) )
>>
>> there (with the masking of the low bit possibly pulled out)?
> 
> There are multiple angles here.
> 
> First, I got an unexpected surprise on ARM with an expression, and while
> this one won't pick up pointer const-ness, I can never remember what
> MISRA's view on this is.
> 
> Second, this is the odd-loop-out compared to rest of the function, which
> are all of the form "for ( i = 0; i < HPET_TIMER_NUM ;".
> 
> But perhaps most importantly, OSes don't touch this register.  Xen not
> at all, and Linux only in _hpet_print_config().  Neither bother
> preserving/clearing it on suspend/resume, even when running the HPET in
> legacy replacement mode.
> 
> I haven't checked windows behaviour, but I don't expect it to differ
> here.  This register simply isn't interesting for the preferred type of
> interrupts (edge), and also isn't useful for an ISR handling a line
> interrupt.

Yet there must have been an environment where the register is of use, or
else Roger wouldn't have been prompted to make what is now be07023be115
("x86/vhpet: add support for level triggered interrupts").

Jan

[PATCH v2 4/4] x86/vmx: Rewrite vmx_sync_pir_to_irr() to be more efficient

[Andrew Cooper]

There are two issues.  First, pi_test_and_clear_on() pulls the cache-line to
the CPU and dirties it even if there's nothing outstanding, but the final
for_each_set_bit() is O(256) when O(8) would do, and would avoid multiple
atomic updates to the same IRR word.

Rewrite it from scratch, explaining what's going on at each step.

Bloat-o-meter reports 177 -> 145 (net -32), but the better aspect is the
removal calls to __find_{first,next}_bit() hidden behind for_each_set_bit().

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>

The main purpose of this is to get rid of bitmap_for_each().

v2:
 * Extend the comments
---
 xen/arch/x86/hvm/vmx/vmx.c | 70 +++++++++++++++++++++++++++++++++-----
 1 file changed, 62 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 12f8a66458db..1baed7e816c4 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2317,18 +2317,72 @@ static void cf_check vmx_deliver_posted_intr(struct vcpu *v, u8 vector)
 
 static void cf_check vmx_sync_pir_to_irr(struct vcpu *v)
 {
-    struct vlapic *vlapic = vcpu_vlapic(v);
-    unsigned int group, i;
-    DECLARE_BITMAP(pending_intr, X86_NR_VECTORS);
+    struct pi_desc *desc = &v->arch.hvm.vmx.pi_desc;
+    union {
+        uint64_t _64[X86_NR_VECTORS / (sizeof(uint64_t) * 8)];
+        uint32_t _32[X86_NR_VECTORS / (sizeof(uint32_t) * 8)];
+    } vec;
+    uint32_t *irr;
+    bool on;
 
-    if ( !pi_test_and_clear_on(&v->arch.hvm.vmx.pi_desc) )
+    /*
+     * The PIR is a contended cacheline which bounces between the CPU(s) and
+     * IOMMU(s).  An IOMMU updates the entire PIR atomically, but we can't
+     * express the same on the CPU side, so care has to be taken.
+     *
+     * First, do a plain read of ON.  If the PIR hasn't been modified, this
+     * will keep the cacheline Shared and not pull it Excusive on the current
+     * CPU.
+     */
+    if ( !pi_test_on(desc) )
         return;
 
-    for ( group = 0; group < ARRAY_SIZE(pending_intr); group++ )
-        pending_intr[group] = pi_get_pir(&v->arch.hvm.vmx.pi_desc, group);
+    /*
+     * Second, if the plain read said that ON was set, we must clear it with
+     * an atomic action.  This will bring the cachline to Exclusive on the
+     * current CPU.
+     *
+     * This should always succeed because noone else should be playing with
+     * the PIR behind our back, but assert so just in case.
+     */
+    on = pi_test_and_clear_on(desc);
+    ASSERT(on);
+
+    /*
+     * The cacheline is now Exclusive on the current CPU, and because ON was
+     * set, some other entity (an IOMMU, or Xen on another CPU) has indicated
+     * that at PIR needs re-scanning.
+     *
+     * Note: Entities which can't update the entire cacheline atomically
+     *       (i.e. Xen on another CPU) are required to update PIR first, then
+     *       set ON.  Therefore, there is a corner case where we may have
+     *       found and processed the PIR updates "last time around" and only
+     *       found ON this time around.  This is fine; the logic still
+     *       operates correctly.
+     *
+     * Atomically read and clear the entire pending bitmap as fast as we, to
+     * reduce the window where another entity may steal the cacheline back
+     * from us.  This is a performance concern, not a correctness concern.  If
+     * the another entity does steal the cacheline back, we'll just wait to
+     * get it back again.
+     */
+    for ( unsigned int i = 0; i < ARRAY_SIZE(vec._64); ++i )
+        vec._64[i] = xchg(&desc->pir[i], 0);
+
+    /*
+     * Finally, merge the pending vectors into IRR.  The IRR register is
+     * scattered in memory, so we have to do this 32 bits at a time.
+     */
+    irr = (uint32_t *)&vcpu_vlapic(v)->regs->data[APIC_IRR];
+    for ( unsigned int i = 0; i < ARRAY_SIZE(vec._32); ++i )
+    {
+        if ( !vec._32[i] )
+            continue;
 
-    bitmap_for_each ( i, pending_intr, X86_NR_VECTORS )
-        vlapic_set_vector(i, &vlapic->regs->data[APIC_IRR]);
+        asm ( "lock or %[val], %[irr]"
+              : [irr] "+m" (irr[i * 0x10])
+              : [val] "r" (vec._32[i]) );
+    }
 }
 
 static bool cf_check vmx_test_pir(const struct vcpu *v, uint8_t vec)
-- 
2.39.2

Re: [PATCH v2 4/4] x86/vmx: Rewrite vmx_sync_pir_to_irr() to be more efficient

[Andrew Cooper]

On 27/08/2024 2:57 pm, Andrew Cooper wrote:
> There are two issues.  First, pi_test_and_clear_on() pulls the cache-line to
> the CPU and dirties it even if there's nothing outstanding, but the final
> for_each_set_bit() is O(256) when O(8) would do, and would avoid multiple
> atomic updates to the same IRR word.
>
> Rewrite it from scratch, explaining what's going on at each step.
>
> Bloat-o-meter reports 177 -> 145 (net -32), but the better aspect is the
> removal calls to __find_{first,next}_bit() hidden behind for_each_set_bit().
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>
> The main purpose of this is to get rid of bitmap_for_each().
>
> v2:
>  * Extend the comments

FWIW, Gitlab CI has gained one reliable failure for this series (which
includes the hweight series too, because of how I've got my branch
arranged).

It is a timeout (domU not reporting in after boot), and as it is
specific to the AlderLake runner, it's very likely to be this patch.

I guess I need to triple-check the IRR scatter logic...

~Andrew

Re: [PATCH v2 4/4] x86/vmx: Rewrite vmx_sync_pir_to_irr() to be more efficient

[Jan Beulich]

On 27.08.2024 15:57, Andrew Cooper wrote:
> There are two issues.  First, pi_test_and_clear_on() pulls the cache-line to
> the CPU and dirties it even if there's nothing outstanding, but the final
> for_each_set_bit() is O(256) when O(8) would do,

Nit: That's bitmap_for_each() now, I think. And again ...

> and would avoid multiple
> atomic updates to the same IRR word.
> 
> Rewrite it from scratch, explaining what's going on at each step.
> 
> Bloat-o-meter reports 177 -> 145 (net -32), but the better aspect is the
> removal calls to __find_{first,next}_bit() hidden behind for_each_set_bit().

... here, and no underscore prefixes on the two find functions.

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2317,18 +2317,72 @@ static void cf_check vmx_deliver_posted_intr(struct vcpu *v, u8 vector)
>  
>  static void cf_check vmx_sync_pir_to_irr(struct vcpu *v)
>  {
> -    struct vlapic *vlapic = vcpu_vlapic(v);
> -    unsigned int group, i;
> -    DECLARE_BITMAP(pending_intr, X86_NR_VECTORS);
> +    struct pi_desc *desc = &v->arch.hvm.vmx.pi_desc;
> +    union {
> +        uint64_t _64[X86_NR_VECTORS / (sizeof(uint64_t) * 8)];

Using unsigned long here would imo be better, as that's what matches
struct pi_desc's DECLARE_BITMAP().

> +        uint32_t _32[X86_NR_VECTORS / (sizeof(uint32_t) * 8)];
> +    } vec;
> +    uint32_t *irr;
> +    bool on;
>  
> -    if ( !pi_test_and_clear_on(&v->arch.hvm.vmx.pi_desc) )
> +    /*
> +     * The PIR is a contended cacheline which bounces between the CPU(s) and
> +     * IOMMU(s).  An IOMMU updates the entire PIR atomically, but we can't
> +     * express the same on the CPU side, so care has to be taken.
> +     *
> +     * First, do a plain read of ON.  If the PIR hasn't been modified, this
> +     * will keep the cacheline Shared and not pull it Excusive on the current
> +     * CPU.
> +     */
> +    if ( !pi_test_on(desc) )
>          return;
>  
> -    for ( group = 0; group < ARRAY_SIZE(pending_intr); group++ )
> -        pending_intr[group] = pi_get_pir(&v->arch.hvm.vmx.pi_desc, group);
> +    /*
> +     * Second, if the plain read said that ON was set, we must clear it with
> +     * an atomic action.  This will bring the cachline to Exclusive on the

Nit (from my spell checker): cacheline.

> +     * current CPU.
> +     *
> +     * This should always succeed because noone else should be playing with
> +     * the PIR behind our back, but assert so just in case.
> +     */
> +    on = pi_test_and_clear_on(desc);
> +    ASSERT(on);
> +
> +    /*
> +     * The cacheline is now Exclusive on the current CPU, and because ON was

"is" is pretty ambitious. We can only hope it (still) is.

> +     * set, some other entity (an IOMMU, or Xen on another CPU) has indicated
> +     * that at PIR needs re-scanning.

Stray "at"?

> +     *
> +     * Note: Entities which can't update the entire cacheline atomically
> +     *       (i.e. Xen on another CPU) are required to update PIR first, then
> +     *       set ON.  Therefore, there is a corner case where we may have
> +     *       found and processed the PIR updates "last time around" and only
> +     *       found ON this time around.  This is fine; the logic still
> +     *       operates correctly.
> +     *
> +     * Atomically read and clear the entire pending bitmap as fast as we, to

Missing "can" before the comma?

> +     * reduce the window where another entity may steal the cacheline back
> +     * from us.  This is a performance concern, not a correctness concern.  If
> +     * the another entity does steal the cacheline back, we'll just wait to

"the other"?

> +     * get it back again.
> +     */
> +    for ( unsigned int i = 0; i < ARRAY_SIZE(vec._64); ++i )
> +        vec._64[i] = xchg(&desc->pir[i], 0);
> +
> +    /*
> +     * Finally, merge the pending vectors into IRR.  The IRR register is
> +     * scattered in memory, so we have to do this 32 bits at a time.
> +     */
> +    irr = (uint32_t *)&vcpu_vlapic(v)->regs->data[APIC_IRR];
> +    for ( unsigned int i = 0; i < ARRAY_SIZE(vec._32); ++i )
> +    {
> +        if ( !vec._32[i] )
> +            continue;
>  
> -    bitmap_for_each ( i, pending_intr, X86_NR_VECTORS )
> -        vlapic_set_vector(i, &vlapic->regs->data[APIC_IRR]);
> +        asm ( "lock or %[val], %[irr]"
> +              : [irr] "+m" (irr[i * 0x10])

This wants to be irr * 4 only, to account for sizeof(*irr) == 4.

Jan

Re: [PATCH v2 4/4] x86/vmx: Rewrite vmx_sync_pir_to_irr() to be more efficient

[Andrew Cooper]

On 28/08/2024 10:19 am, Jan Beulich wrote:
> On 27.08.2024 15:57, Andrew Cooper wrote:
>> There are two issues.  First, pi_test_and_clear_on() pulls the cache-line to
>> the CPU and dirties it even if there's nothing outstanding, but the final
>> for_each_set_bit() is O(256) when O(8) would do,
> Nit: That's bitmap_for_each() now, I think. And again ...
>
>> and would avoid multiple
>> atomic updates to the same IRR word.
>>
>> Rewrite it from scratch, explaining what's going on at each step.
>>
>> Bloat-o-meter reports 177 -> 145 (net -32), but the better aspect is the
>> removal calls to __find_{first,next}_bit() hidden behind for_each_set_bit().
> ... here, and no underscore prefixes on the two find functions.

Yes, and fixed.

>
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -2317,18 +2317,72 @@ static void cf_check vmx_deliver_posted_intr(struct vcpu *v, u8 vector)
>>  
>>  static void cf_check vmx_sync_pir_to_irr(struct vcpu *v)
>>  {
>> -    struct vlapic *vlapic = vcpu_vlapic(v);
>> -    unsigned int group, i;
>> -    DECLARE_BITMAP(pending_intr, X86_NR_VECTORS);
>> +    struct pi_desc *desc = &v->arch.hvm.vmx.pi_desc;
>> +    union {
>> +        uint64_t _64[X86_NR_VECTORS / (sizeof(uint64_t) * 8)];
> Using unsigned long here would imo be better, as that's what matches
> struct pi_desc's DECLARE_BITMAP().

Why?  It was also the primary contribution to particularly-bad code
generation in this function.

>
>> +        uint32_t _32[X86_NR_VECTORS / (sizeof(uint32_t) * 8)];
>> +    } vec;
>> +    uint32_t *irr;
>> +    bool on;
>>  
>> -    if ( !pi_test_and_clear_on(&v->arch.hvm.vmx.pi_desc) )
>> +    /*
>> +     * The PIR is a contended cacheline which bounces between the CPU(s) and
>> +     * IOMMU(s).  An IOMMU updates the entire PIR atomically, but we can't
>> +     * express the same on the CPU side, so care has to be taken.
>> +     *
>> +     * First, do a plain read of ON.  If the PIR hasn't been modified, this
>> +     * will keep the cacheline Shared and not pull it Excusive on the current
>> +     * CPU.
>> +     */
>> +    if ( !pi_test_on(desc) )
>>          return;
>>  
>> -    for ( group = 0; group < ARRAY_SIZE(pending_intr); group++ )
>> -        pending_intr[group] = pi_get_pir(&v->arch.hvm.vmx.pi_desc, group);
>> +    /*
>> +     * Second, if the plain read said that ON was set, we must clear it with
>> +     * an atomic action.  This will bring the cachline to Exclusive on the
> Nit (from my spell checker): cacheline.
>
>> +     * current CPU.
>> +     *
>> +     * This should always succeed because noone else should be playing with
>> +     * the PIR behind our back, but assert so just in case.
>> +     */
>> +    on = pi_test_and_clear_on(desc);
>> +    ASSERT(on);
>> +
>> +    /*
>> +     * The cacheline is now Exclusive on the current CPU, and because ON was
> "is" is pretty ambitious. We can only hope it (still) is.

I can't think of a clearer way of saying this.  "will have become
Exclusive" perhaps, but this is getting into some subtle tense gymnastics.

>> +     * get it back again.
>> +     */
>> +    for ( unsigned int i = 0; i < ARRAY_SIZE(vec._64); ++i )
>> +        vec._64[i] = xchg(&desc->pir[i], 0);
>> +
>> +    /*
>> +     * Finally, merge the pending vectors into IRR.  The IRR register is
>> +     * scattered in memory, so we have to do this 32 bits at a time.
>> +     */
>> +    irr = (uint32_t *)&vcpu_vlapic(v)->regs->data[APIC_IRR];
>> +    for ( unsigned int i = 0; i < ARRAY_SIZE(vec._32); ++i )
>> +    {
>> +        if ( !vec._32[i] )
>> +            continue;
>>  
>> -    bitmap_for_each ( i, pending_intr, X86_NR_VECTORS )
>> -        vlapic_set_vector(i, &vlapic->regs->data[APIC_IRR]);
>> +        asm ( "lock or %[val], %[irr]"
>> +              : [irr] "+m" (irr[i * 0x10])
> This wants to be irr * 4 only, to account for sizeof(*irr) == 4.

Ah, that will be where the AlderLake interrupts are disappearing to.

~Andrew

Re: [PATCH v2 4/4] x86/vmx: Rewrite vmx_sync_pir_to_irr() to be more efficient

[Andrew Cooper]

On 28/08/2024 7:08 pm, Andrew Cooper wrote:
> On 28/08/2024 10:19 am, Jan Beulich wrote:
>> On 27.08.2024 15:57, Andrew Cooper wrote:
>>> +     * get it back again.
>>> +     */
>>> +    for ( unsigned int i = 0; i < ARRAY_SIZE(vec._64); ++i )
>>> +        vec._64[i] = xchg(&desc->pir[i], 0);
>>> +
>>> +    /*
>>> +     * Finally, merge the pending vectors into IRR.  The IRR register is
>>> +     * scattered in memory, so we have to do this 32 bits at a time.
>>> +     */
>>> +    irr = (uint32_t *)&vcpu_vlapic(v)->regs->data[APIC_IRR];
>>> +    for ( unsigned int i = 0; i < ARRAY_SIZE(vec._32); ++i )
>>> +    {
>>> +        if ( !vec._32[i] )
>>> +            continue;
>>>  
>>> -    bitmap_for_each ( i, pending_intr, X86_NR_VECTORS )
>>> -        vlapic_set_vector(i, &vlapic->regs->data[APIC_IRR]);
>>> +        asm ( "lock or %[val], %[irr]"
>>> +              : [irr] "+m" (irr[i * 0x10])
>> This wants to be irr * 4 only, to account for sizeof(*irr) == 4.
> Ah, that will be where the AlderLake interrupts are disappearing to.

Indeed.  It's much happier now.

https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1431047447

~Andrew

Re: [PATCH v2 4/4] x86/vmx: Rewrite vmx_sync_pir_to_irr() to be more efficient

[Jan Beulich]

On 28.08.2024 20:08, Andrew Cooper wrote:
> On 28/08/2024 10:19 am, Jan Beulich wrote:
>> On 27.08.2024 15:57, Andrew Cooper wrote:
>>> There are two issues.  First, pi_test_and_clear_on() pulls the cache-line to
>>> the CPU and dirties it even if there's nothing outstanding, but the final
>>> for_each_set_bit() is O(256) when O(8) would do,
>> Nit: That's bitmap_for_each() now, I think. And again ...
>>
>>> and would avoid multiple
>>> atomic updates to the same IRR word.
>>>
>>> Rewrite it from scratch, explaining what's going on at each step.
>>>
>>> Bloat-o-meter reports 177 -> 145 (net -32), but the better aspect is the
>>> removal calls to __find_{first,next}_bit() hidden behind for_each_set_bit().
>> ... here, and no underscore prefixes on the two find functions.
> 
> Yes, and fixed.
> 
>>
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -2317,18 +2317,72 @@ static void cf_check vmx_deliver_posted_intr(struct vcpu *v, u8 vector)
>>>  
>>>  static void cf_check vmx_sync_pir_to_irr(struct vcpu *v)
>>>  {
>>> -    struct vlapic *vlapic = vcpu_vlapic(v);
>>> -    unsigned int group, i;
>>> -    DECLARE_BITMAP(pending_intr, X86_NR_VECTORS);
>>> +    struct pi_desc *desc = &v->arch.hvm.vmx.pi_desc;
>>> +    union {
>>> +        uint64_t _64[X86_NR_VECTORS / (sizeof(uint64_t) * 8)];
>> Using unsigned long here would imo be better, as that's what matches
>> struct pi_desc's DECLARE_BITMAP().
> 
> Why?  It was also the primary contribution to particularly-bad code
> generation in this function.

I answered the "why" already: Because of you copying from something ...

>>> +        uint32_t _32[X86_NR_VECTORS / (sizeof(uint32_t) * 8)];
>>> +    } vec;
>>> +    uint32_t *irr;
>>> +    bool on;
>>>  
>>> -    if ( !pi_test_and_clear_on(&v->arch.hvm.vmx.pi_desc) )
>>> +    /*
>>> +     * The PIR is a contended cacheline which bounces between the CPU(s) and
>>> +     * IOMMU(s).  An IOMMU updates the entire PIR atomically, but we can't
>>> +     * express the same on the CPU side, so care has to be taken.
>>> +     *
>>> +     * First, do a plain read of ON.  If the PIR hasn't been modified, this
>>> +     * will keep the cacheline Shared and not pull it Excusive on the current
>>> +     * CPU.
>>> +     */
>>> +    if ( !pi_test_on(desc) )
>>>          return;
>>>  
>>> -    for ( group = 0; group < ARRAY_SIZE(pending_intr); group++ )
>>> -        pending_intr[group] = pi_get_pir(&v->arch.hvm.vmx.pi_desc, group);
>>> +    /*
>>> +     * Second, if the plain read said that ON was set, we must clear it with
>>> +     * an atomic action.  This will bring the cachline to Exclusive on the
>> Nit (from my spell checker): cacheline.
>>
>>> +     * current CPU.
>>> +     *
>>> +     * This should always succeed because noone else should be playing with
>>> +     * the PIR behind our back, but assert so just in case.
>>> +     */
>>> +    on = pi_test_and_clear_on(desc);
>>> +    ASSERT(on);
>>> +
>>> +    /*
>>> +     * The cacheline is now Exclusive on the current CPU, and because ON was
>> "is" is pretty ambitious. We can only hope it (still) is.
> 
> I can't think of a clearer way of saying this.  "will have become
> Exclusive" perhaps, but this is getting into some subtle tense gymnastics.
> 
>>> +     * get it back again.
>>> +     */
>>> +    for ( unsigned int i = 0; i < ARRAY_SIZE(vec._64); ++i )
>>> +        vec._64[i] = xchg(&desc->pir[i], 0);

... that is the result of DECLARE_BITMAP(), i.e. an array of unsigned longs.
If you make that part of the new union unsigned long[] too, you'll have code
which is bitness-independent (i.e. would also have worked correctly in 32-bit
Xen, and would work correctly in hypothetical 128-bit Xen). I don't think the
array _type_ was "the primary contribution to particularly-bad code
generation in this function"; it was how that bitmap was used.

Jan