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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.07.2023 12:51, Roger Pau Monné wrote:
> 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>

Thanks.

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

I think 0 is okay to use here, as the low IRQs (at least the 16 ISA
ones) are all 1:1 mapped to their "machine" (i.e. Xen's) IRQ numbers.
And IRQ0 is always the timer, never given to guests (not even to
Dom0).

If we wanted to use INVALID_PIRQ here, we'd first need to make this
part of the public interface.

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

Indeed.

Jan