Re: [PATCH] x86/HVM: adjust pIRQ calculation in hvm_inject_msi()

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

On Mon, Jul 17, 2023 at 11:31:57AM +0200, Jan Beulich wrote:
> While the referenced commit came without any update to the public header
> (which doesn't clarify how the upper address bits are used), the
> intention looks to have been that bits 12..19 and 40..63 form the pIRQ.
> Negative values simply make no sense, and pirq_info() also generally
> wants invoking with an unsigned (and not just positive) value.
> 
> Since the line was pointed out by Eclair, address Misra rule 7.2 at the
> same time, by adding the missing U suffix.
> 
> Fixes: 88fccdd11ca0 ("xen: event channel remapping for emulated MSIs")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I have a question below, but not related to the change here.

> 
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -383,7 +383,7 @@ int hvm_inject_msi(struct domain *d, uin
>  
>      if ( !vector )
>      {
> -        int pirq = ((addr >> 32) & 0xffffff00) | dest;
> +        unsigned int pirq = ((addr >> 32) & 0xffffff00U) | dest;
>  
>          if ( pirq > 0 )

I do wonder whether this check is also accurate, as pIRQ 0 could be a
valid value.  Should it instead use INVALID_PIRQ?

Since there's no specification or documentation how this is supposed
to work it's hard to tell.

Thanks, Roger.