x86/HVM: adjust pIRQ calculation in hvm_inject_msi()

Commit Message

Jan Beulich July 17, 2023, 9:31 a.m. UTC

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 <[e-mail]>

Comments

Roger Pau Monné July 17, 2023, 10:51 a.m. UTC | #1

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 <[e-mail]>

Acked-by: Roger Pau Monné <[e-mail]>

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.

Jan Beulich July 17, 2023, 11:49 a.m. UTC | #2

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 <[e-mail]>
> 
> Acked-by: Roger Pau Monné <[e-mail]>

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

Roger Pau Monné July 17, 2023, 12:47 p.m. UTC | #3

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 <[e-mail]>
> > 
> > Acked-by: Roger Pau Monné <[e-mail]>
> 
> 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?

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.

Thanks, Roger.

Jan Beulich July 17, 2023, 1:28 p.m. UTC | #4

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 <[e-mail]>
>>>
>>> Acked-by: Roger Pau Monné <[e-mail]>
>>
>> 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

Patch

--- 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 )
         {