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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?

Thanks, Roger.