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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> 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