Re: [RFC XEN PATCH 6/6] tools/libs/light: pci: translate irq to gsi

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

On Mon, Mar 20, 2023 at 04:29:25PM +0100, Jan Beulich wrote:
> On 20.03.2023 16:16, Roger Pau Monné wrote:
> > @@ -244,12 +242,18 @@ static void vioapic_write_redirent(
> >      }
> >      else
> >      {
> > +        int ret;
> > +
> >          unmasked = ent.fields.mask;
> >          /* Remote IRR and Delivery Status are read-only. */
> >          ent.bits = ((ent.bits >> 32) << 32) | val;
> >          ent.fields.delivery_status = 0;
> >          ent.fields.remote_irr = pent->fields.remote_irr;
> >          unmasked = unmasked && !ent.fields.mask;
> > +        ret = mp_register_gsi(gsi, ent.fields.trig_mode, ent.fields.polarity);
> > +        if ( ret && ret !=  -EEXIST )
> > +            gprintk(XENLOG_WARNING, "vioapic: error registering GSI %u: %d\n",
> > +                    gsi, ret);
> >      }
> 
> I assume this is only meant to be experimental, as I'm missing confinement
> to Dom0 here.

Indeed.  I've attached a fixed version below, let's make sure this
doesn't influence testing.

> I also question this when the mask bit as set, as in that
> case neither the trigger mode bit nor the polarity one can be relied upon.
> At which point it would look to me as if it was necessary for Dom0 to use
> a hypercall instead (which naturally would then be PHYSDEVOP_setup_gsi).

AFAICT Linux does correctly set the trigger/polarity even when the
pins are masked, so this should be safe as a proof of concept. Let's
first figure out whether the issue is really with the lack of setup of
the IO-APIC pins.  At the end without input from Ray this is just a
wild guess.

Regards, Roger.
----
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 405d0a95af..cc53a3bd12 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -86,6 +86,8 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     {
     case PHYSDEVOP_map_pirq:
     case PHYSDEVOP_unmap_pirq:
+        break;
+
     case PHYSDEVOP_eoi:
     case PHYSDEVOP_irq_status_query:
     case PHYSDEVOP_get_free_pirq:
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 41e3c4d5e4..64f7b5bcc5 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -180,9 +180,7 @@ static int vioapic_hwdom_map_gsi(unsigned int gsi, unsigned int trig,
 
     /* Interrupt has been unmasked, bind it now. */
     ret = mp_register_gsi(gsi, trig, pol);
-    if ( ret == -EEXIST )
-        return 0;
-    if ( ret )
+    if ( ret && ret != -EEXIST )
     {
         gprintk(XENLOG_WARNING, "vioapic: error registering GSI %u: %d\n",
                  gsi, ret);
@@ -250,6 +248,16 @@ static void vioapic_write_redirent(
         ent.fields.delivery_status = 0;
         ent.fields.remote_irr = pent->fields.remote_irr;
         unmasked = unmasked && !ent.fields.mask;
+        if ( is_hardware_domain(d) )
+        {
+            int ret = mp_register_gsi(gsi, ent.fields.trig_mode,
+                                      ent.fields.polarity);
+
+            if ( ret && ret !=  -EEXIST )
+                gprintk(XENLOG_WARNING,
+                        "vioapic: error registering GSI %u: %d\n",
+                        gsi, ret);
+        }
     }
 
     *pent = ent;