Re: [PATCH] xen/vpci: Improve code generation in mask_write()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/03/2024 3:13 pm, Roger Pau Monné wrote:
> On Fri, Mar 15, 2024 at 12:13:22PM +0000, Andrew Cooper wrote:
>> The use of __clear_bit() forces dmask to be spilled to the stack, and
>> interferes with the compiler heuristcs for some upcoming improvements to the
>> ffs() code generation.
>>
>> First, shrink dmask to just the active vectors by making out the upper bits.
>> This replaces the "i < msi->vectors" part of the loop condition.
>>
>> Next, use a simple while() loop with "clear bottom bit" expressed in plane C,
>> which affords the optimiser a far better understanding of what the loop is
>> doing.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>
>> Noticed when looking at the ffs() code gen improvements.
>>
>> Any suggestion on how to test this?  test_vcpi doesn't seem to check anything
>> here.  I think I've got the boundary conditions for msi->vectors right, but
>> I'd be lying if I said I was certain...
> There's no easy way to test this because it relies on having a PCI
> device underneath.  test_vpci just checks the logic to add & remove
> handlers, but doesn't get remotely close as to attempting to provide
> some kind of dummy environment for pass through to be sanity tested.
>
> I should look into it.
>
>> bloat-o-meter reports:
>>
>>   add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-28 (-28)
>>   Function                                     old     new   delta
>>   mask_write                                   142     114     -28
>>
>> which is a consequence of the compiler having a much better idea of what's
>> going on in the loop.  There's more to come with the ffs() improvements too.
>> ---
>>  xen/drivers/vpci/msi.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
>> index d3aa5df08941..30adcf7df05d 100644
>> --- a/xen/drivers/vpci/msi.c
>> +++ b/xen/drivers/vpci/msi.c
>> @@ -169,13 +169,15 @@ static void cf_check mask_write(
>>  
>>      if ( msi->enabled )
>>      {
>> -        unsigned int i;
>> +        /* Skip changes to vectors which aren't enabled. */
>> +        dmask &= (~0U >> (32 - msi->vectors));
> Do we need to ASSERT that msi->vectors <= 32 in order to avoid
> theoretical UB?

I don't think so.  Things have gone catastrophically wrong elsewhere to
get here with 64 or 128.

All this does is stop calling the set-mask callback for disabled vectors.

~Andrew