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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.07.2023 14:47, Roger Pau Monné wrote:
> On Mon, Jul 17, 2023 at 01:49:43PM +0200, Jan Beulich wrote:
>> 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).
> 
> I'm kind of confused by this not being dom0, but rather an
> HVM guest, so pIRQ 0 of that HVM guest should be available to the
> guest itself?

pIRQ is a Xen concept; Xen assigns them, for the guest to use them in
(e.g.) hypercalls. As long as Xen hands out 0 only ever for (guest)
GSI 0, all should be fine. That said, ...

> IOW: the possible values here should be the full pIRQ range, as there
> are never Xen owned pIRQs in the context of an HVM guest.  One further
> limitation is that even in that case pIRQs for (guest) GSIs would
> still be identity mapped, so GSI 0 won't be a suitable pIRQ for an MSI
> source.
> 
> The usage of pIRQs here even for emulated devices makes me very
> confused.

... I'm with you here; I'm not convinced this logic is sound.

Jan