map_domain_pirq(): pirq already mapped?

map_domain_pirq(): pirq already mapped?

[Jason Andryuk]

Hi,

Early in map_domain_pirq(), we have this block:

     old_irq = domain_pirq_to_irq(d, pirq);
     old_pirq = domain_irq_to_pirq(d, irq);

     if ( (old_irq > 0 && (old_irq != irq) ) ||
          (old_pirq && (old_pirq != pirq)) )
     {
         dprintk(XENLOG_G_WARNING,
                 "dom%d: pirq %d or irq %d already mapped (%d,%d)\n",
                 d->domain_id, pirq, irq, old_pirq, old_irq);
         return 0;
     }

Why do we return 0 instead of -EEXIST?  Since the pirq is not updated, 
the caller doesn't know that pirq won't fire - only old_pirq.  For 
allocate_and_map_gsi_pirq(), the new pirq is still returned to the 
caller.  I would expect old_pirq to be returned so the caller knows what 
to use.  Am I missing something?

Thanks,
Jason

Re: map_domain_pirq(): pirq already mapped?

[Roger Pau Monné]

On Wed, May 13, 2026 at 09:18:46PM -0400, Jason Andryuk wrote:
> Hi,
> 
> Early in map_domain_pirq(), we have this block:
> 
>     old_irq = domain_pirq_to_irq(d, pirq);
>     old_pirq = domain_irq_to_pirq(d, irq);
> 
>     if ( (old_irq > 0 && (old_irq != irq) ) ||
>          (old_pirq && (old_pirq != pirq)) )
>     {
>         dprintk(XENLOG_G_WARNING,
>                 "dom%d: pirq %d or irq %d already mapped (%d,%d)\n",
>                 d->domain_id, pirq, irq, old_pirq, old_irq);
>         return 0;
>     }
> 
> Why do we return 0 instead of -EEXIST?  Since the pirq is not updated, the
> caller doesn't know that pirq won't fire - only old_pirq.  For
> allocate_and_map_gsi_pirq(), the new pirq is still returned to the caller.
> I would expect old_pirq to be returned so the caller knows what to use.  Am
> I missing something?

Looking at bfc341a65cfb2 it seems like this might have been an attempt
to keep the previous logic in ioapic_guest_write() that didn't return
an error when attempting to add/move an in use IRQ, while switching
ioapic_guest_write() to use map_domain_pirq()?

The commit description is not very helpful sadly.  I think the mention
of "And this patch also makes broken NetBSD dom0 work again." is
relevant. AFAICT NetBSD will do PHYSDEVOP_apic_read -> modify RTE (ie:
set mask bit for example) -> PHYSDEVOP_apic_write.  However the
semantics of those hypercalls is not symmetric.  PHYSDEVOP_apic_read
will return the vector used by Xen in the RTE, while
PHYSDEVOP_apic_write expects the vector field of the RTE to contain
the pIRQ.  I think this is why map_domain_pirq() was adjusted in such
a weird way, to ignore requests with bogus pIRQs and still succeed, so
that PHYSDEVOP_apic_write would also succeed.  Ideally the interface
should have been adjusted so that read/modify/write cycles using
PHYSDEVOP_apic_{read,write} would work as expected (iow:
PHYSDEVOP_apic_read should have returned the pIRQ in the vector
field).

In the context of GSIs, I think we aim for Xen to always identity map
them (so IRQ == pIRQ), but there might be (or might have been)
hypercalls that could allow you to create non-identity mappings
between GSIs and pIRQs.

Explicitly looking at allocate_and_map_gsi_pirq() do you know what
causes the domain_irq_to_pirq() in allocate_pirq() to not return the
already allocated pIRQ that matches the passed IRQ?

Overall we should likely adjust map_domain_pirq() to return -EEIXST,
and then fix ioapic_guest_write() to shallow such error so we can keep
the current behavior for that specific interface.

Regards, Roger.

Re: map_domain_pirq(): pirq already mapped?

[Jason Andryuk]

Hi Roger,

Thanks for taking a look.

On 2026-05-14 04:26, Roger Pau Monné wrote:
> On Wed, May 13, 2026 at 09:18:46PM -0400, Jason Andryuk wrote:
>> Hi,
>>
>> Early in map_domain_pirq(), we have this block:
>>
>>      old_irq = domain_pirq_to_irq(d, pirq);
>>      old_pirq = domain_irq_to_pirq(d, irq);
>>
>>      if ( (old_irq > 0 && (old_irq != irq) ) ||
>>           (old_pirq && (old_pirq != pirq)) )
>>      {
>>          dprintk(XENLOG_G_WARNING,
>>                  "dom%d: pirq %d or irq %d already mapped (%d,%d)\n",
>>                  d->domain_id, pirq, irq, old_pirq, old_irq);
>>          return 0;
>>      }
>>
>> Why do we return 0 instead of -EEXIST?  Since the pirq is not updated, the
>> caller doesn't know that pirq won't fire - only old_pirq.  For
>> allocate_and_map_gsi_pirq(), the new pirq is still returned to the caller.
>> I would expect old_pirq to be returned so the caller knows what to use.  Am
>> I missing something?
> 
> Looking at bfc341a65cfb2 it seems like this might have been an attempt
> to keep the previous logic in ioapic_guest_write() that didn't return
> an error when attempting to add/move an in use IRQ, while switching
> ioapic_guest_write() to use map_domain_pirq()?
> 
> The commit description is not very helpful sadly.  I think the mention
> of "And this patch also makes broken NetBSD dom0 work again." is
> relevant. AFAICT NetBSD will do PHYSDEVOP_apic_read -> modify RTE (ie:
> set mask bit for example) -> PHYSDEVOP_apic_write.  However the
> semantics of those hypercalls is not symmetric.  PHYSDEVOP_apic_read
> will return the vector used by Xen in the RTE, while
> PHYSDEVOP_apic_write expects the vector field of the RTE to contain
> the pIRQ.  I think this is why map_domain_pirq() was adjusted in such
> a weird way, to ignore requests with bogus pIRQs and still succeed, so
> that PHYSDEVOP_apic_write would also succeed.  Ideally the interface
> should have been adjusted so that read/modify/write cycles using
> PHYSDEVOP_apic_{read,write} would work as expected (iow:
> PHYSDEVOP_apic_read should have returned the pIRQ in the vector
> field).
> 
> In the context of GSIs, I think we aim for Xen to always identity map
> them (so IRQ == pIRQ), but there might be (or might have been)
> hypercalls that could allow you to create non-identity mappings
> between GSIs and pIRQs.

To support non-PCI passthrough with Hyperlaunch, we added code to map a 
GSI to a vIOAPIC.  It does not require identity mapping, and that works. 
  It was while testing conditions that I expected to fail that I found 
the behavior.

First map:
machine A -> guest B
Then map:
machine A -> guest C "success" from the return 0

> Explicitly looking at allocate_and_map_gsi_pirq() do you know what
> causes the domain_irq_to_pirq() in allocate_pirq() to not return the
> already allocated pIRQ that matches the passed IRQ?

Well, it's PVH and I'm making up a "PIRQ" for the vIOAPIC.

> Overall we should likely adjust map_domain_pirq() to return -EEIXST,
> and then fix ioapic_guest_write() to shallow such error so we can keep
> the current behavior for that specific interface.
Having been focused on PVH, I didn't look much at the PV behavior.  But 
it was still surprising to see the "no-op" success.

Thanks for the pointers.

Regards,
Jason

Re: map_domain_pirq(): pirq already mapped?

[Roger Pau Monné]

On Thu, May 14, 2026 at 09:01:28PM -0400, Jason Andryuk wrote:
> Hi Roger,
> 
> Thanks for taking a look.
> 
> On 2026-05-14 04:26, Roger Pau Monné wrote:
> > On Wed, May 13, 2026 at 09:18:46PM -0400, Jason Andryuk wrote:
> > > Hi,
> > > 
> > > Early in map_domain_pirq(), we have this block:
> > > 
> > >      old_irq = domain_pirq_to_irq(d, pirq);
> > >      old_pirq = domain_irq_to_pirq(d, irq);
> > > 
> > >      if ( (old_irq > 0 && (old_irq != irq) ) ||
> > >           (old_pirq && (old_pirq != pirq)) )
> > >      {
> > >          dprintk(XENLOG_G_WARNING,
> > >                  "dom%d: pirq %d or irq %d already mapped (%d,%d)\n",
> > >                  d->domain_id, pirq, irq, old_pirq, old_irq);
> > >          return 0;
> > >      }
> > > 
> > > Why do we return 0 instead of -EEXIST?  Since the pirq is not updated, the
> > > caller doesn't know that pirq won't fire - only old_pirq.  For
> > > allocate_and_map_gsi_pirq(), the new pirq is still returned to the caller.
> > > I would expect old_pirq to be returned so the caller knows what to use.  Am
> > > I missing something?
> > 
> > Looking at bfc341a65cfb2 it seems like this might have been an attempt
> > to keep the previous logic in ioapic_guest_write() that didn't return
> > an error when attempting to add/move an in use IRQ, while switching
> > ioapic_guest_write() to use map_domain_pirq()?
> > 
> > The commit description is not very helpful sadly.  I think the mention
> > of "And this patch also makes broken NetBSD dom0 work again." is
> > relevant. AFAICT NetBSD will do PHYSDEVOP_apic_read -> modify RTE (ie:
> > set mask bit for example) -> PHYSDEVOP_apic_write.  However the
> > semantics of those hypercalls is not symmetric.  PHYSDEVOP_apic_read
> > will return the vector used by Xen in the RTE, while
> > PHYSDEVOP_apic_write expects the vector field of the RTE to contain
> > the pIRQ.  I think this is why map_domain_pirq() was adjusted in such
> > a weird way, to ignore requests with bogus pIRQs and still succeed, so
> > that PHYSDEVOP_apic_write would also succeed.  Ideally the interface
> > should have been adjusted so that read/modify/write cycles using
> > PHYSDEVOP_apic_{read,write} would work as expected (iow:
> > PHYSDEVOP_apic_read should have returned the pIRQ in the vector
> > field).
> > 
> > In the context of GSIs, I think we aim for Xen to always identity map
> > them (so IRQ == pIRQ), but there might be (or might have been)
> > hypercalls that could allow you to create non-identity mappings
> > between GSIs and pIRQs.
> 
> To support non-PCI passthrough with Hyperlaunch, we added code to map a GSI
> to a vIOAPIC.  It does not require identity mapping, and that works.  It was
> while testing conditions that I expected to fail that I found the behavior.
> 
> First map:
> machine A -> guest B
> Then map:
> machine A -> guest C "success" from the return 0
> 
> > Explicitly looking at allocate_and_map_gsi_pirq() do you know what
> > causes the domain_irq_to_pirq() in allocate_pirq() to not return the
> > already allocated pIRQ that matches the passed IRQ?
> 
> Well, it's PVH and I'm making up a "PIRQ" for the vIOAPIC.

Yeah, the vIO-APIC auto-map interface in vioapic_hwdom_map_gsi() for
PVH hwdom does use identity mappings between GSIs and pIRQs, but if
you use the side-band physdev interface you can certainly create
non-identity mappings between GSIs and pIRQs.  That was introduced as
part of the PCI-passthrough work for PVH dom0 IIRC.

In fact what we do in vioapic_hwdom_map_gsi() might be dangerous now
that we allow a PVH dom0 to allocate and map GSIs using hypercalls
also.  Using the hypercall interface a domain could introduce
non-identity GSI relations, which would then cause
vioapic_hwdom_map_gsi() to fail.  It might be best to adjust the pirq
hint to be -1 in vioapic_hwdom_map_gsi() instead of gsi, and give up
on identity mappings GSIs to pIRQs on PVH hwdom from the vIO-APIC.

> > Overall we should likely adjust map_domain_pirq() to return -EEIXST,
> > and then fix ioapic_guest_write() to shallow such error so we can keep
> > the current behavior for that specific interface.
> Having been focused on PVH, I didn't look much at the PV behavior.  But it
> was still surprising to see the "no-op" success.

Indeed, I think we want to contain that behavior to
ioapic_guest_write() exclusively.  AFAICT the success is an unintended
effect of bfc341a65cfb2, which should have been limited to
ioapic_guest_write() exclusively.

Regards, Roger.