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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@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.

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