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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@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)?

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