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 <[email protected]>
> >
> > Acked-by: Roger Pau Monné <[email protected]>
>
> 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.
