Re: map_domain_pirq(): pirq already mapped?

[Jason Andryuk <jason.andryuk@xxxxxxx>]

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