Re: map_domain_pirq(): pirq already mapped?

["Roger Pau Monné" <roger.pau@citrix.com>]

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.