[PATCH 0/5] x86/dpci: bind legacy PCI interrupts to PVHv2 Dom0

[PATCH 0/5] x86/dpci: bind legacy PCI interrupts to PVHv2 Dom0

[Roger Pau Monne]

Hello,

The following patches allow binding bare-metal GSIs into a PVHv2 Dom0, by
snooping on the vIO APICs writes made by Dom0.

In order to implement this a new PT_IRQ_TYPE_GSI bind type has to be
introduced, since PT_IRQ_TYPE_PCI is not suitable because Xen doesn't know the
PCI device(s) behind each GSI. Most of the code is shared with the
PT_IRQ_TYPE_PCI bind type, except for the EOI. The actual binding of the GSI
into Dom0 is performed when Dom0 unmasks the vIO APIC pin.

Patches #3 and #5 is where the actual meat is. The rest are mostly prerequisite
changes for those two.

The series has been tested with a PVHv2 Dom0 on a box with 3 IO APICs,
although all devices are wired up into the first IO APIC sadly.

A branch with the changes can be found at:

git://xenbits.xen.org/people/royger/xen.git dom0_gsi_v1

Note that this builds on top of the "x86/vioapic: introduce support for
multiple vIO APICs" series.

Thanks, Roger.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/5] x86/dpci: allow hvm_irq_dpci to handle a variable number of GSIs

[Roger Pau Monne]

By making the girq array variable length. For the hardware domain this array is
going to match the actual number of GSIs present on the system.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/physdev.c       | 2 +-
 xen/drivers/passthrough/io.c | 8 +++++---
 xen/include/xen/hvm/irq.h    | 6 ++++--
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 6c15f9bf49..d12086d0ec 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -52,7 +52,7 @@ static int physdev_hvm_map_pirq(
         {
             const struct hvm_girq_dpci_mapping *girq;
 
-            BUILD_BUG_ON(ARRAY_SIZE(hvm_irq_dpci->girq) < NR_HVM_IRQS);
+            BUG_ON(hvm_domain_irq(d)->nr_gsis < NR_HVM_IRQS);
             list_for_each_entry ( girq,
                                   &hvm_irq_dpci->girq[*index],
                                   list )
diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index 50e2f00214..3345db5759 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -322,15 +322,17 @@ int pt_irq_create_bind(
     hvm_irq_dpci = domain_get_irq_dpci(d);
     if ( hvm_irq_dpci == NULL )
     {
-        unsigned int i;
+        unsigned int i, nr_gsis;
 
-        hvm_irq_dpci = xzalloc(struct hvm_irq_dpci);
+        nr_gsis = is_hardware_domain(d) ? hvm_domain_irq(d)->nr_gsis
+                                        : NR_HVM_IRQS;
+        hvm_irq_dpci = xzalloc_bytes(hvm_irq_dpci_size(nr_gsis));
         if ( hvm_irq_dpci == NULL )
         {
             spin_unlock(&d->event_lock);
             return -ENOMEM;
         }
-        for ( i = 0; i < NR_HVM_IRQS; i++ )
+        for ( i = 0; i < nr_gsis; i++ )
             INIT_LIST_HEAD(&hvm_irq_dpci->girq[i]);
 
         d->arch.hvm_domain.irq->dpci = hvm_irq_dpci;
diff --git a/xen/include/xen/hvm/irq.h b/xen/include/xen/hvm/irq.h
index d3f8623c0c..8304cb5725 100644
--- a/xen/include/xen/hvm/irq.h
+++ b/xen/include/xen/hvm/irq.h
@@ -81,14 +81,16 @@ struct hvm_girq_dpci_mapping {
 
 /* Protected by domain's event_lock */
 struct hvm_irq_dpci {
-    /* Guest IRQ to guest device/intx mapping. */
-    struct list_head girq[NR_HVM_IRQS];
     /* Record of mapped ISA IRQs */
     DECLARE_BITMAP(isairq_map, NR_ISAIRQS);
     /* Record of mapped Links */
     uint8_t link_cnt[NR_LINK];
+    /* Guest IRQ to guest device/intx mapping. */
+    struct list_head girq[];
 };
 
+#define hvm_irq_dpci_size(cnt) offsetof(struct hvm_irq_dpci, girq[cnt])
+
 /* Machine IRQ to guest device/intx mapping. */
 struct hvm_pirq_dpci {
     uint32_t flags;
-- 
2.12.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/5] x86/ioapic: introduce helper to fetch triggering mode of GSI

[Roger Pau Monne]

This helper is used in order to fetch the triggering mode of a GSI. This is
needed in order to figure out if a GSI can be bound with the shared attribute
or not (only level triggered interrupts should be shared).

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/io_apic.c        | 22 ++++++++++++++++++++++
 xen/include/asm-x86/io_apic.h |  1 +
 2 files changed, 23 insertions(+)

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index d18046067c..df5bc52392 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2261,6 +2261,28 @@ int io_apic_set_pci_routing (int ioapic, int pin, int irq, int edge_level, int a
     return 0;
 }
 
+unsigned int io_apic_get_gsi_trigger(unsigned int gsi)
+{
+    struct IO_APIC_route_entry entry;
+    unsigned int ioapic, base_gsi;
+
+    ASSERT(gsi < nr_irqs_gsi);
+
+    /* For GSI type find if the GSI is level or edge triggered */
+    for ( ioapic = 0; ioapic < nr_ioapics; ioapic++ )
+    {
+        base_gsi = io_apic_gsi_base(ioapic);
+
+        if ( gsi >= base_gsi && gsi < base_gsi + nr_ioapic_entries[ioapic] )
+            break;
+    }
+    ASSERT(ioapic < nr_ioapics);
+
+    entry = ioapic_read_entry(ioapic, gsi - base_gsi, 0);
+
+    return entry.trigger;
+}
+
 static int ioapic_physbase_to_id(unsigned long physbase)
 {
     int apic;
diff --git a/xen/include/asm-x86/io_apic.h b/xen/include/asm-x86/io_apic.h
index 8029c8f400..43bcb656ee 100644
--- a/xen/include/asm-x86/io_apic.h
+++ b/xen/include/asm-x86/io_apic.h
@@ -179,6 +179,7 @@ extern int io_apic_get_unique_id (int ioapic, int apic_id);
 extern int io_apic_get_version (int ioapic);
 extern int io_apic_get_redir_entries (int ioapic);
 extern int io_apic_set_pci_routing (int ioapic, int pin, int irq, int edge_level, int active_high_low);
+extern unsigned int io_apic_get_gsi_trigger(unsigned int gsi);
 
 extern void init_ioapic_mappings(void);
 
-- 
2.12.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 3/5] x86/pt: introduce PT_IRQ_TYPE_GSI to bind GSIs to a PVH Dom0

[Roger Pau Monne]

The current type used to bind a legacy IRQ into a HVM guest is not suitable for
PVH Dom0 because the PCI device that's originating the interrupt is not know to
Xen.

To solve that a new bind type is introduced (PT_IRQ_TYPE_GSI), that takes a gsi
as a guest destination parameter (instead of a PCI device). This new binding
type builds on top of the existing PT_IRQ_TYPE_PCI, but since it's an identity
gsi binding some of the functionality needs to be branched (like the
assert/deassert of the gsi itself).

Right now this is limited to the hardware domain only, and to identity map
gsis.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/irq.c       |  25 ++++++++
 xen/drivers/passthrough/io.c | 147 ++++++++++++++++++++++++++++++++-----------
 xen/include/public/domctl.h  |   4 ++
 xen/include/xen/hvm/irq.h    |   6 ++
 4 files changed, 144 insertions(+), 38 deletions(-)

diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 6e67cae9bd..2c35a1e044 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -118,6 +118,31 @@ void hvm_pci_intx_deassert(
     spin_unlock(&d->arch.hvm_domain.irq_lock);
 }
 
+void hvm_gsi_assert(struct domain *d, unsigned int gsi)
+{
+    struct hvm_irq *hvm_irq = hvm_domain_irq(d);
+
+    ASSERT(gsi < hvm_irq->nr_gsis);
+    ASSERT(!has_vpic(d));
+    spin_lock(&d->arch.hvm_domain.irq_lock);
+    if ( hvm_irq->gsi_assert_count[gsi]++ == 0 )
+        assert_gsi(d, gsi);
+    spin_unlock(&d->arch.hvm_domain.irq_lock);
+}
+
+void hvm_gsi_deassert(struct domain *d, unsigned int gsi)
+{
+    struct hvm_irq *hvm_irq = hvm_domain_irq(d);
+
+    ASSERT(gsi < hvm_irq->nr_gsis);
+    ASSERT(!has_vpic(d));
+    spin_lock(&d->arch.hvm_domain.irq_lock);
+    if ( hvm_irq->gsi_assert_count[gsi] )
+        hvm_irq->gsi_assert_count[gsi]--;
+    spin_unlock(&d->arch.hvm_domain.irq_lock);
+}
+
+
 void hvm_isa_irq_assert(
     struct domain *d, unsigned int isa_irq)
 {
diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index 3345db5759..e4cd22cf18 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -156,6 +156,21 @@ static int pt_irq_guest_eoi(struct domain *d, struct hvm_pirq_dpci *pirq_dpci,
     return 0;
 }
 
+static void pt_girq_time_out(struct domain *d, unsigned int guest_gsi)
+{
+    const struct hvm_irq_dpci *dpci = domain_get_irq_dpci(d);
+    const struct hvm_girq_dpci_mapping *girq;
+
+    ASSERT(dpci);
+
+    list_for_each_entry ( girq, &dpci->girq[guest_gsi], list )
+    {
+        struct pirq *pirq = pirq_info(d, girq->machine_gsi);
+
+        pirq_dpci(pirq)->flags |= HVM_IRQ_DPCI_EOI_LATCH;
+    }
+}
+
 static void pt_irq_time_out(void *data)
 {
     struct hvm_pirq_dpci *irq_map = data;
@@ -173,18 +188,18 @@ static void pt_irq_time_out(void *data)
     }
     list_for_each_entry ( digl, &irq_map->digl_list, list )
     {
-        unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx);
-        const struct hvm_girq_dpci_mapping *girq;
-
-        list_for_each_entry ( girq, &dpci->girq[guest_gsi], list )
-        {
-            struct pirq *pirq = pirq_info(irq_map->dom, girq->machine_gsi);
-
-            pirq_dpci(pirq)->flags |= HVM_IRQ_DPCI_EOI_LATCH;
-        }
+        pt_girq_time_out(irq_map->dom,
+                         hvm_pci_intx_gsi(digl->device, digl->intx));
         hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx);
     }
 
+    if ( irq_map->guest_gsi != DPCI_INVALID_GSI )
+    {
+        ASSERT(is_hardware_domain(irq_map->dom));
+        pt_girq_time_out(irq_map->dom, irq_map->guest_gsi);
+        hvm_gsi_deassert(irq_map->dom, irq_map->guest_gsi);
+    }
+
     pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL);
 
     spin_unlock(&irq_map->dom->event_lock);
@@ -316,6 +331,9 @@ int pt_irq_create_bind(
     if ( pirq < 0 || pirq >= d->nr_pirqs )
         return -EINVAL;
 
+    if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_GSI && !is_hardware_domain(d) )
+        return -EPERM;
+
  restart:
     spin_lock(&d->event_lock);
 
@@ -361,6 +379,7 @@ int pt_irq_create_bind(
         goto restart;
     }
 
+    pirq_dpci->guest_gsi = DPCI_INVALID_GSI;
     switch ( pt_irq_bind->irq_type )
     {
     case PT_IRQ_TYPE_MSI:
@@ -464,32 +483,56 @@ int pt_irq_create_bind(
         break;
     }
 
+    case PT_IRQ_TYPE_GSI:
     case PT_IRQ_TYPE_PCI:
     case PT_IRQ_TYPE_MSI_TRANSLATE:
     {
-        unsigned int bus = pt_irq_bind->u.pci.bus;
-        unsigned int device = pt_irq_bind->u.pci.device;
-        unsigned int intx = pt_irq_bind->u.pci.intx;
-        unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx);
-        unsigned int link = hvm_pci_intx_link(device, intx);
-        struct dev_intx_gsi_link *digl = xmalloc(struct dev_intx_gsi_link);
+        unsigned int bus, device, intx, guest_gsi, link;
+        struct dev_intx_gsi_link *digl = NULL;
         struct hvm_girq_dpci_mapping *girq =
             xmalloc(struct hvm_girq_dpci_mapping);
 
-        if ( !digl || !girq )
+        if ( !girq )
         {
             spin_unlock(&d->event_lock);
-            xfree(girq);
-            xfree(digl);
             return -ENOMEM;
         }
 
-        hvm_irq_dpci->link_cnt[link]++;
+        if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_GSI )
+        {
+            digl = xmalloc(struct dev_intx_gsi_link);
+            if ( !digl )
+            {
+                spin_unlock(&d->event_lock);
+                xfree(girq);
+                return -ENOMEM;
+            }
+
+            digl->bus = bus = pt_irq_bind->u.pci.bus;
+            digl->device = device = pt_irq_bind->u.pci.device;
+            digl->intx = intx = pt_irq_bind->u.pci.intx;
+            list_add_tail(&digl->list, &pirq_dpci->digl_list);
+
+            guest_gsi = hvm_pci_intx_gsi(device, intx);
+            link = hvm_pci_intx_link(device, intx);
 
-        digl->bus = bus;
-        digl->device = device;
-        digl->intx = intx;
-        list_add_tail(&digl->list, &pirq_dpci->digl_list);
+            hvm_irq_dpci->link_cnt[link]++;
+        }
+        else
+        {
+            guest_gsi = pt_irq_bind->u.gsi.gsi;
+
+            /* PT_IRQ_TYPE_GSI should only be used for identity bindings */
+            ASSERT(guest_gsi == pirq);
+            ASSERT(guest_gsi < hvm_domain_irq(d)->nr_gsis);
+
+            /*
+             * The actual PCI device(s) that use this GSI are unknown, Xen
+             * relies on machine_gsi to be identity bound into the guest.
+             */
+            bus = device = intx = 0;
+            pirq_dpci->guest_gsi = guest_gsi;
+        }
 
         girq->bus = bus;
         girq->device = device;
@@ -512,12 +555,15 @@ int pt_irq_create_bind(
                                    HVM_IRQ_DPCI_TRANSLATE;
                 share = 0;
             }
-            else    /* PT_IRQ_TYPE_PCI */
+            else    /* PT_IRQ_TYPE_PCI or PT_IRQ_TYPE_GSI */
             {
                 pirq_dpci->flags = HVM_IRQ_DPCI_MAPPED |
                                    HVM_IRQ_DPCI_MACH_PCI |
                                    HVM_IRQ_DPCI_GUEST_PCI;
-                share = BIND_PIRQ__WILL_SHARE;
+                if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_PCI )
+                    share = BIND_PIRQ__WILL_SHARE;
+                else
+                    share = io_apic_get_gsi_trigger(pirq);
             }
 
             /* Init timer before binding */
@@ -535,8 +581,11 @@ int pt_irq_create_bind(
                  */
                 pirq_dpci->dom = NULL;
                 list_del(&girq->list);
-                list_del(&digl->list);
-                hvm_irq_dpci->link_cnt[link]--;
+                if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_GSI)
+                {
+                    list_del(&digl->list);
+                    hvm_irq_dpci->link_cnt[link]--;
+                }
                 pirq_dpci->flags = 0;
                 pirq_cleanup_check(info, d);
                 spin_unlock(&d->event_lock);
@@ -610,14 +659,26 @@ int pt_irq_destroy_bind(
 
     if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_MSI )
     {
-        unsigned int bus = pt_irq_bind->u.pci.bus;
-        unsigned int device = pt_irq_bind->u.pci.device;
-        unsigned int intx = pt_irq_bind->u.pci.intx;
-        unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx);
-        unsigned int link = hvm_pci_intx_link(device, intx);
+        unsigned int bus, device, intx, guest_gsi, link;
         struct hvm_girq_dpci_mapping *girq;
         struct dev_intx_gsi_link *digl, *tmp;
 
+        if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_GSI )
+        {
+            bus = device = intx = 0;
+            guest_gsi = pt_irq_bind->u.gsi.gsi;
+        }
+        else
+        {
+             bus = pt_irq_bind->u.pci.bus;
+             device = pt_irq_bind->u.pci.device;
+             intx = pt_irq_bind->u.pci.intx;
+             guest_gsi = hvm_pci_intx_gsi(device, intx);
+             link = hvm_pci_intx_link(device, intx);
+        }
+
+
+
         list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list )
         {
             if ( girq->bus         == bus &&
@@ -638,7 +699,8 @@ int pt_irq_destroy_bind(
             return -EINVAL;
         }
 
-        hvm_irq_dpci->link_cnt[link]--;
+        if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_GSI )
+            hvm_irq_dpci->link_cnt[link]--;
 
         /* clear the mirq info */
         if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
@@ -654,6 +716,7 @@ int pt_irq_destroy_bind(
                 }
             }
             what = list_empty(&pirq_dpci->digl_list) ? "final" : "partial";
+            pirq_dpci->guest_gsi = DPCI_INVALID_GSI;
         }
         else
             what = "bogus";
@@ -835,6 +898,11 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
             hvm_pci_intx_assert(d, digl->device, digl->intx);
             pirq_dpci->pending++;
         }
+        if ( pirq_dpci->guest_gsi != DPCI_INVALID_GSI )
+        {
+            hvm_gsi_assert(d, pirq_dpci->guest_gsi);
+            pirq_dpci->pending++;
+        }
 
         if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE )
         {
@@ -862,12 +930,15 @@ static void __hvm_dpci_eoi(struct domain *d,
                            const union vioapic_redir_entry *ent)
 {
     struct pirq *pirq = pirq_info(d, girq->machine_gsi);
-    struct hvm_pirq_dpci *pirq_dpci;
+    struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
 
     if ( !hvm_domain_use_pirq(d, pirq) )
-        hvm_pci_intx_deassert(d, girq->device, girq->intx);
-
-    pirq_dpci = pirq_dpci(pirq);
+    {
+        if ( pirq_dpci->guest_gsi == DPCI_INVALID_GSI )
+            hvm_pci_intx_deassert(d, girq->device, girq->intx);
+        else
+            hvm_gsi_deassert(d, pirq_dpci->guest_gsi);
+    }
 
     /*
      * No need to get vector lock for timer
@@ -891,7 +962,7 @@ void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi,
     if ( !iommu_enabled )
         return;
 
-    if ( guest_gsi < NR_ISAIRQS )
+    if ( guest_gsi < NR_ISAIRQS && !is_hardware_domain(d) )
     {
         hvm_dpci_isairq_eoi(d, guest_gsi);
         return;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 9e3ce21f71..31dd6ab986 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -554,6 +554,7 @@ typedef enum pt_irq_type_e {
     PT_IRQ_TYPE_MSI,
     PT_IRQ_TYPE_MSI_TRANSLATE,
     PT_IRQ_TYPE_SPI,    /* ARM: valid range 32-1019 */
+    PT_IRQ_TYPE_GSI,
 } pt_irq_type_t;
 struct xen_domctl_bind_pt_irq {
     uint32_t machine_irq;
@@ -577,6 +578,9 @@ struct xen_domctl_bind_pt_irq {
         struct {
             uint16_t spi;
         } spi;
+        struct {
+          uint32_t gsi;
+        } gsi;
     } u;
 };
 typedef struct xen_domctl_bind_pt_irq xen_domctl_bind_pt_irq_t;
diff --git a/xen/include/xen/hvm/irq.h b/xen/include/xen/hvm/irq.h
index 8304cb5725..89bc505ca6 100644
--- a/xen/include/xen/hvm/irq.h
+++ b/xen/include/xen/hvm/irq.h
@@ -91,6 +91,7 @@ struct hvm_irq_dpci {
 
 #define hvm_irq_dpci_size(cnt) offsetof(struct hvm_irq_dpci, girq[cnt])
 
+#define DPCI_INVALID_GSI UINT_MAX
 /* Machine IRQ to guest device/intx mapping. */
 struct hvm_pirq_dpci {
     uint32_t flags;
@@ -98,6 +99,7 @@ struct hvm_pirq_dpci {
     bool_t masked;
     uint16_t pending;
     struct list_head digl_list;
+    unsigned int guest_gsi;
     struct domain *dom;
     struct hvm_gmsi_info gmsi;
     struct timer timer;
@@ -124,6 +126,10 @@ void hvm_isa_irq_assert(
 void hvm_isa_irq_deassert(
     struct domain *d, unsigned int isa_irq);
 
+/* Modify state of GSIs. */
+void hvm_gsi_assert(struct domain *d, unsigned int gsi);
+void hvm_gsi_deassert(struct domain *d, unsigned int gsi);
+
 int hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq);
 
 int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data);
-- 
2.12.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 4/5] x86/physdev: move prototypes of physdev_{map/unmap}_pirq to headers

[Roger Pau Monne]

So they can be called outside of the physdev.c file.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/physdev.c    | 4 ----
 xen/include/asm-x86/irq.h | 5 +++++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index d12086d0ec..4a0a890e55 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -18,10 +18,6 @@
 #include <xsm/xsm.h>
 #include <asm/p2m.h>
 
-int physdev_map_pirq(domid_t, int type, int *index, int *pirq_p,
-                     struct msi_info *);
-int physdev_unmap_pirq(domid_t, int pirq);
-
 #include "x86_64/mmconfig.h"
 
 #ifndef COMPAT
diff --git a/xen/include/asm-x86/irq.h b/xen/include/asm-x86/irq.h
index ef625ebb13..a0ddd6096a 100644
--- a/xen/include/asm-x86/irq.h
+++ b/xen/include/asm-x86/irq.h
@@ -147,6 +147,11 @@ int map_domain_emuirq_pirq(struct domain *d, int pirq, int irq);
 int unmap_domain_pirq_emuirq(struct domain *d, int pirq);
 bool_t hvm_domain_use_pirq(const struct domain *, const struct pirq *);
 
+struct msi_info;
+int physdev_map_pirq(domid_t, int type, int *index, int *pirq_p,
+                     struct msi_info *);
+int physdev_unmap_pirq(domid_t, int pirq);
+
 /* Reset irq affinities to match the given CPU mask. */
 void fixup_irqs(const cpumask_t *mask, bool_t verbose);
 void fixup_eoi(void);
-- 
2.12.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 5/5] x86/vioapic: bind interrupts to PVH Dom0

[Roger Pau Monne]

Add the glue in order to bind the PVH Dom0 GSI from bare metal. This is done
when Dom0 unmasks the vIO APIC pins, by fetching the current pin settings and
setting up the PIRQ, which will then be bound to Dom0 using the newly
introduced PT_IRQ_TYPE_GSI bind type.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/vioapic.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index c349a3ee61..ca3aab16ef 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -199,6 +199,34 @@ static void vioapic_write_redirent(
         unmasked = unmasked && !ent.fields.mask;
     }
 
+    if ( is_hardware_domain(d) && unmasked )
+    {
+        xen_domctl_bind_pt_irq_t pt_irq_bind = {
+            .irq_type = PT_IRQ_TYPE_GSI,
+            .machine_irq = gsi,
+            .u.gsi.gsi = gsi,
+            .hvm_domid = DOMID_SELF,
+        };
+        int ret, pirq = gsi;
+
+        /* Interrupt has been unmasked, bind it now. */
+        ret = mp_register_gsi(gsi, ent.fields.trig_mode, ent.fields.polarity);
+        if ( ret && ret != -EEXIST )
+        {
+            gdprintk(XENLOG_WARNING,
+                     "%s: error registering GSI %u: %d\n", __func__, gsi, ret);
+        }
+        if ( !ret )
+        {
+            ret = physdev_map_pirq(DOMID_SELF, MAP_PIRQ_TYPE_GSI, &pirq, &pirq,
+                                   NULL);
+            BUG_ON(ret);
+
+            ret = pt_irq_create_bind(d, &pt_irq_bind);
+            BUG_ON(ret);
+        }
+    }
+
     *pent = ent;
 
     if ( gsi == 0 )
-- 
2.12.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/5] x86/pt: introduce PT_IRQ_TYPE_GSI to bind GSIs to a PVH Dom0

[Roger Pau Monne]

On Mon, Mar 27, 2017 at 11:44:27AM +0100, Roger Pau Monne wrote:
[...]
> @@ -464,32 +483,56 @@ int pt_irq_create_bind(
>          break;
>      }
>  
> +    case PT_IRQ_TYPE_GSI:
>      case PT_IRQ_TYPE_PCI:
>      case PT_IRQ_TYPE_MSI_TRANSLATE:
>      {
> -        unsigned int bus = pt_irq_bind->u.pci.bus;
> -        unsigned int device = pt_irq_bind->u.pci.device;
> -        unsigned int intx = pt_irq_bind->u.pci.intx;
> -        unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx);
> -        unsigned int link = hvm_pci_intx_link(device, intx);
> -        struct dev_intx_gsi_link *digl = xmalloc(struct dev_intx_gsi_link);
> +        unsigned int bus, device, intx, guest_gsi, link;
> +        struct dev_intx_gsi_link *digl = NULL;
>          struct hvm_girq_dpci_mapping *girq =
>              xmalloc(struct hvm_girq_dpci_mapping);
>  
> -        if ( !digl || !girq )
> +        if ( !girq )
>          {
>              spin_unlock(&d->event_lock);
> -            xfree(girq);
> -            xfree(digl);
>              return -ENOMEM;
>          }
>  
> -        hvm_irq_dpci->link_cnt[link]++;
> +        if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_GSI )
> +        {
> +            digl = xmalloc(struct dev_intx_gsi_link);
> +            if ( !digl )
> +            {
> +                spin_unlock(&d->event_lock);
> +                xfree(girq);
> +                return -ENOMEM;
> +            }
> +
> +            digl->bus = bus = pt_irq_bind->u.pci.bus;
> +            digl->device = device = pt_irq_bind->u.pci.device;
> +            digl->intx = intx = pt_irq_bind->u.pci.intx;
> +            list_add_tail(&digl->list, &pirq_dpci->digl_list);
> +
> +            guest_gsi = hvm_pci_intx_gsi(device, intx);
> +            link = hvm_pci_intx_link(device, intx);
>  
> -        digl->bus = bus;
> -        digl->device = device;
> -        digl->intx = intx;
> -        list_add_tail(&digl->list, &pirq_dpci->digl_list);
> +            hvm_irq_dpci->link_cnt[link]++;
> +        }
> +        else
> +        {
> +            guest_gsi = pt_irq_bind->u.gsi.gsi;
> +
> +            /* PT_IRQ_TYPE_GSI should only be used for identity bindings */
> +            ASSERT(guest_gsi == pirq);
> +            ASSERT(guest_gsi < hvm_domain_irq(d)->nr_gsis);
> +
> +            /*
> +             * The actual PCI device(s) that use this GSI are unknown, Xen
> +             * relies on machine_gsi to be identity bound into the guest.
> +             */
> +            bus = device = intx = 0;

This should be:

link = bus = device = intx = 0;

because...

> +            pirq_dpci->guest_gsi = guest_gsi;
> +        }
>  
>          girq->bus = bus;
>          girq->device = device;
> @@ -535,8 +581,11 @@ int pt_irq_create_bind(
>                   */
>                  pirq_dpci->dom = NULL;
>                  list_del(&girq->list);
> -                list_del(&digl->list);
> -                hvm_irq_dpci->link_cnt[link]--;
> +                if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_GSI)
> +                {
> +                    list_del(&digl->list);
> +                    hvm_irq_dpci->link_cnt[link]--;

... GCC complains that link might be uninitialized here.
> @@ -610,14 +659,26 @@ int pt_irq_destroy_bind(
>  
>      if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_MSI )
>      {
> -        unsigned int bus = pt_irq_bind->u.pci.bus;
> -        unsigned int device = pt_irq_bind->u.pci.device;
> -        unsigned int intx = pt_irq_bind->u.pci.intx;
> -        unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx);
> -        unsigned int link = hvm_pci_intx_link(device, intx);
> +        unsigned int bus, device, intx, guest_gsi, link;
>          struct hvm_girq_dpci_mapping *girq;
>          struct dev_intx_gsi_link *digl, *tmp;
>  
> +        if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_GSI )
> +        {
> +            bus = device = intx = 0;

Same here...

> +            guest_gsi = pt_irq_bind->u.gsi.gsi;
> +        }
> +        else
> +        {
> +             bus = pt_irq_bind->u.pci.bus;
> +             device = pt_irq_bind->u.pci.device;
> +             intx = pt_irq_bind->u.pci.intx;
> +             guest_gsi = hvm_pci_intx_gsi(device, intx);
> +             link = hvm_pci_intx_link(device, intx);
> +        }
> +
> +
> +
>          list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list )
>          {
>              if ( girq->bus         == bus &&
> @@ -638,7 +699,8 @@ int pt_irq_destroy_bind(
>              return -EINVAL;
>          }
>  
> -        hvm_irq_dpci->link_cnt[link]--;
> +        if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_GSI )
> +            hvm_irq_dpci->link_cnt[link]--;

Because GCC complains in the same way here.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 0/5] x86/dpci: bind legacy PCI interrupts to PVHv2 Dom0

[Jan Beulich]

>>> On 27.03.17 at 12:44, <roger.pau@citrix.com> wrote:
> The series has been tested with a PVHv2 Dom0 on a box with 3 IO APICs,
> although all devices are wired up into the first IO APIC sadly.

Did you try with something plugged into the (presumably present)
extension slots?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 0/5] x86/dpci: bind legacy PCI interrupts to PVHv2 Dom0

[Roger Pau Monne]

On Mon, Mar 27, 2017 at 06:19:19AM -0600, Jan Beulich wrote:
> >>> On 27.03.17 at 12:44, <roger.pau@citrix.com> wrote:
> > The series has been tested with a PVHv2 Dom0 on a box with 3 IO APICs,
> > although all devices are wired up into the first IO APIC sadly.
> 
> Did you try with something plugged into the (presumably present)
> extension slots?

This is in a remote location, I don't have physical access to the box itself. I
guess I can try to request IT to plug something there, but I'm not even sure if
that's possible or how long it's going to take (IIRC this box is a blade, so
I'm not sure how much space there is).

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 0/5] x86/dpci: bind legacy PCI interrupts to PVHv2 Dom0

[Roger Pau Monne]

On Mon, Mar 27, 2017 at 01:56:46PM +0100, Roger Pau Monne wrote:
> On Mon, Mar 27, 2017 at 06:19:19AM -0600, Jan Beulich wrote:
> > >>> On 27.03.17 at 12:44, <roger.pau@citrix.com> wrote:
> > > The series has been tested with a PVHv2 Dom0 on a box with 3 IO APICs,
> > > although all devices are wired up into the first IO APIC sadly.
> > 
> > Did you try with something plugged into the (presumably present)
> > extension slots?
> 
> This is in a remote location, I don't have physical access to the box itself. I
> guess I can try to request IT to plug something there, but I'm not even sure if
> that's possible or how long it's going to take (IIRC this box is a blade, so
> I'm not sure how much space there is).

After asking IT and someone gone to the colo, it looks like the box I'm using
doesn't have a single extensions slot, so it's very unlikely that I will be
able to test interrupt delivery on any vIO APIC != 0 ATM. I can assure that the
vIO APICs are detected correctly by Dom0, and that masking the pins works
(well, they are already masked, but FreeBSD does that by default when booting).

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 0/5] x86/dpci: bind legacy PCI interrupts to PVHv2 Dom0

[Jan Beulich]

>>> On 30.03.17 at 12:56, <roger.pau@citrix.com> wrote:
> On Mon, Mar 27, 2017 at 01:56:46PM +0100, Roger Pau Monne wrote:
>> On Mon, Mar 27, 2017 at 06:19:19AM -0600, Jan Beulich wrote:
>> > >>> On 27.03.17 at 12:44, <roger.pau@citrix.com> wrote:
>> > > The series has been tested with a PVHv2 Dom0 on a box with 3 IO APICs,
>> > > although all devices are wired up into the first IO APIC sadly.
>> > 
>> > Did you try with something plugged into the (presumably present)
>> > extension slots?
>> 
>> This is in a remote location, I don't have physical access to the box itself. I
>> guess I can try to request IT to plug something there, but I'm not even sure if
>> that's possible or how long it's going to take (IIRC this box is a blade, so
>> I'm not sure how much space there is).
> 
> After asking IT and someone gone to the colo, it looks like the box I'm using
> doesn't have a single extensions slot, so it's very unlikely that I will be
> able to test interrupt delivery on any vIO APIC != 0 ATM. I can assure that the
> vIO APICs are detected correctly by Dom0, and that masking the pins works
> (well, they are already masked, but FreeBSD does that by default when 
> booting).

Since this series isn't for 4.9 anyway, I don't see a big issue if it goes
in early after branching - we'll then have ample time to adjust things
if needed.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/5] x86/dpci: allow hvm_irq_dpci to handle a variable number of GSIs

[Jan Beulich]

>>> On 27.03.17 at 12:44, <roger.pau@citrix.com> wrote:
> --- a/xen/include/xen/hvm/irq.h
> +++ b/xen/include/xen/hvm/irq.h
> @@ -81,14 +81,16 @@ struct hvm_girq_dpci_mapping {
>  
>  /* Protected by domain's event_lock */
>  struct hvm_irq_dpci {
> -    /* Guest IRQ to guest device/intx mapping. */
> -    struct list_head girq[NR_HVM_IRQS];
>      /* Record of mapped ISA IRQs */
>      DECLARE_BITMAP(isairq_map, NR_ISAIRQS);
>      /* Record of mapped Links */
>      uint8_t link_cnt[NR_LINK];
> +    /* Guest IRQ to guest device/intx mapping. */
> +    struct list_head girq[];
>  };

Considering what you say in the overview mail I don't think the
comment can be moved without adjusting it, as it doesn't seem
to reflect Dom0 in any way. Which then puts under question
whether struct hvm_girq_dpci_mapping is the right data format
for Dom0 in the first place: With bus, device, and intx taken
out, all that's left if machine_gsi, and iirc you identity map GSIs.

Even if the array needed to remain, the sparseness of the GSI
space opens up the question whether using a simple array here
is the right choice.

The patch needs re-basing anyway afaict.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/5] x86/ioapic: introduce helper to fetch triggering mode of GSI

[Jan Beulich]

>>> On 27.03.17 at 12:44, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -2261,6 +2261,28 @@ int io_apic_set_pci_routing (int ioapic, int pin, int irq, int edge_level, int a
>      return 0;
>  }
>  
> +unsigned int io_apic_get_gsi_trigger(unsigned int gsi)

bool

> +{
> +    struct IO_APIC_route_entry entry;
> +    unsigned int ioapic, base_gsi;
> +
> +    ASSERT(gsi < nr_irqs_gsi);
> +
> +    /* For GSI type find if the GSI is level or edge triggered */

The comment looks like it wants to go ahead of the function instead
of here. And what's "GSI type"?

> +    for ( ioapic = 0; ioapic < nr_ioapics; ioapic++ )
> +    {
> +        base_gsi = io_apic_gsi_base(ioapic);
> +
> +        if ( gsi >= base_gsi && gsi < base_gsi + nr_ioapic_entries[ioapic] )
> +            break;
> +    }
> +    ASSERT(ioapic < nr_ioapics);
> +
> +    entry = ioapic_read_entry(ioapic, gsi - base_gsi, 0);
> +
> +    return entry.trigger;
> +}

It is presumably not entirely without reason that you add this function
without having an immediate user (in existing code) for it: The trigger
bit is entirely meaningless in a masked RTE.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/5] x86/physdev: move prototypes of physdev_{map/unmap}_pirq to headers

[Jan Beulich]

>>> On 27.03.17 at 12:44, <roger.pau@citrix.com> wrote:
> So they can be called outside of the physdev.c file.

But they aren't meant to be, which is the entire reason the
declarations aren't in a header. Hence the rationale needs to be
extended here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 5/5] x86/vioapic: bind interrupts to PVH Dom0

[Jan Beulich]

>>> On 27.03.17 at 12:44, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -199,6 +199,34 @@ static void vioapic_write_redirent(
>          unmasked = unmasked && !ent.fields.mask;
>      }
>  
> +    if ( is_hardware_domain(d) && unmasked )
> +    {
> +        xen_domctl_bind_pt_irq_t pt_irq_bind = {
> +            .irq_type = PT_IRQ_TYPE_GSI,
> +            .machine_irq = gsi,
> +            .u.gsi.gsi = gsi,
> +            .hvm_domid = DOMID_SELF,
> +        };
> +        int ret, pirq = gsi;
> +
> +        /* Interrupt has been unmasked, bind it now. */
> +        ret = mp_register_gsi(gsi, ent.fields.trig_mode, ent.fields.polarity);
> +        if ( ret && ret != -EEXIST )
> +        {
> +            gdprintk(XENLOG_WARNING,
> +                     "%s: error registering GSI %u: %d\n", __func__, gsi, ret);
> +        }
> +        if ( !ret )
> +        {
> +            ret = physdev_map_pirq(DOMID_SELF, MAP_PIRQ_TYPE_GSI, &pirq, &pirq,
> +                                   NULL);

With this call you basically admit that PVH can't really do without
physdev ops, just that you hide it behind IO-APIC RTE writes.
Along the lines of the comment on the previous patch I wonder
though whether you really need to use this function, i.e.
whether you can't instead get away with little more than the
call to map_domain_pirq() which that function does.

> +            BUG_ON(ret);

You absolutely don't want to bring down the entire system if a
failure occurs here or ...

> +            ret = pt_irq_create_bind(d, &pt_irq_bind);
> +            BUG_ON(ret);

... here. Probably the best you can do besides issuing a log
message is to mask the RTE.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 5/5] x86/vioapic: bind interrupts to PVH Dom0

[Roger Pau Monne]

On Tue, Apr 18, 2017 at 06:35:57AM -0600, Jan Beulich wrote:
> >>> On 27.03.17 at 12:44, <roger.pau@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/vioapic.c
> > +++ b/xen/arch/x86/hvm/vioapic.c
> > @@ -199,6 +199,34 @@ static void vioapic_write_redirent(
> >          unmasked = unmasked && !ent.fields.mask;
> >      }
> >  
> > +    if ( is_hardware_domain(d) && unmasked )
> > +    {
> > +        xen_domctl_bind_pt_irq_t pt_irq_bind = {
> > +            .irq_type = PT_IRQ_TYPE_GSI,
> > +            .machine_irq = gsi,
> > +            .u.gsi.gsi = gsi,
> > +            .hvm_domid = DOMID_SELF,
> > +        };
> > +        int ret, pirq = gsi;
> > +
> > +        /* Interrupt has been unmasked, bind it now. */
> > +        ret = mp_register_gsi(gsi, ent.fields.trig_mode, ent.fields.polarity);
> > +        if ( ret && ret != -EEXIST )
> > +        {
> > +            gdprintk(XENLOG_WARNING,
> > +                     "%s: error registering GSI %u: %d\n", __func__, gsi, ret);
> > +        }
> > +        if ( !ret )
> > +        {
> > +            ret = physdev_map_pirq(DOMID_SELF, MAP_PIRQ_TYPE_GSI, &pirq, &pirq,
> > +                                   NULL);
> 
> With this call you basically admit that PVH can't really do without
> physdev ops, just that you hide it behind IO-APIC RTE writes.

Yes, I just want to get rid of the physdev hypercalls for PVH Dom0, not the
physdev ops inside of Xen.

> Along the lines of the comment on the previous patch I wonder
> though whether you really need to use this function, i.e.
> whether you can't instead get away with little more than the
> call to map_domain_pirq() which that function does.

Well, I could certainly open-code part of physdev_map_pirq here, AFAICT I just
need to make sure the GSI is not used, and bind it to Dom0, but that's just
duplicating the code inside of physdev_map_pirq.

For MSI/MSI-X I certainly need to use physdev_map_pirq, because the logic in
that case is more complex, so I will have to end up making physdev_map_pirq
available to external callers in any case.

> 
> > +            BUG_ON(ret);
> 
> You absolutely don't want to bring down the entire system if a
> failure occurs here or ...
> 
> > +            ret = pt_irq_create_bind(d, &pt_irq_bind);
> > +            BUG_ON(ret);
> 
> ... here. Probably the best you can do besides issuing a log
> message is to mask the RTE.

Right, those are leftovers from when I was still debugging this.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 5/5] x86/vioapic: bind interrupts to PVH Dom0

[Jan Beulich]

>>> On 18.04.17 at 15:44, <roger.pau@citrix.com> wrote:
> On Tue, Apr 18, 2017 at 06:35:57AM -0600, Jan Beulich wrote:
>> >>> On 27.03.17 at 12:44, <roger.pau@citrix.com> wrote:
>> > --- a/xen/arch/x86/hvm/vioapic.c
>> > +++ b/xen/arch/x86/hvm/vioapic.c
>> > @@ -199,6 +199,34 @@ static void vioapic_write_redirent(
>> >          unmasked = unmasked && !ent.fields.mask;
>> >      }
>> >  
>> > +    if ( is_hardware_domain(d) && unmasked )
>> > +    {
>> > +        xen_domctl_bind_pt_irq_t pt_irq_bind = {
>> > +            .irq_type = PT_IRQ_TYPE_GSI,
>> > +            .machine_irq = gsi,
>> > +            .u.gsi.gsi = gsi,
>> > +            .hvm_domid = DOMID_SELF,
>> > +        };
>> > +        int ret, pirq = gsi;
>> > +
>> > +        /* Interrupt has been unmasked, bind it now. */
>> > +        ret = mp_register_gsi(gsi, ent.fields.trig_mode, ent.fields.polarity);
>> > +        if ( ret && ret != -EEXIST )
>> > +        {
>> > +            gdprintk(XENLOG_WARNING,
>> > +                     "%s: error registering GSI %u: %d\n", __func__, gsi, ret);
>> > +        }
>> > +        if ( !ret )
>> > +        {
>> > +            ret = physdev_map_pirq(DOMID_SELF, MAP_PIRQ_TYPE_GSI, &pirq, &pirq,
>> > +                                   NULL);
>> 
>> With this call you basically admit that PVH can't really do without
>> physdev ops, just that you hide it behind IO-APIC RTE writes.
> 
> Yes, I just want to get rid of the physdev hypercalls for PVH Dom0, not the
> physdev ops inside of Xen.
> 
>> Along the lines of the comment on the previous patch I wonder
>> though whether you really need to use this function, i.e.
>> whether you can't instead get away with little more than the
>> call to map_domain_pirq() which that function does.
> 
> Well, I could certainly open-code part of physdev_map_pirq here, AFAICT I just
> need to make sure the GSI is not used, and bind it to Dom0, but that's just
> duplicating the code inside of physdev_map_pirq.
> 
> For MSI/MSI-X I certainly need to use physdev_map_pirq, because the logic in
> that case is more complex, so I will have to end up making physdev_map_pirq
> available to external callers in any case.

Hmm, ugly. I'd really like to keep them local as they are - as their
name says - physdev-op specific. Would it perhaps be possible to
split out GSI and MSI specific helpers from it (which you could
then call from here and from the MSI code you're going to add),
or would that still leave too much code duplication?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/5] x86/dpci: allow hvm_irq_dpci to handle a variable number of GSIs

[Roger Pau Monne]

On Tue, Apr 18, 2017 at 06:13:54AM -0600, Jan Beulich wrote:
> >>> On 27.03.17 at 12:44, <roger.pau@citrix.com> wrote:
> > --- a/xen/include/xen/hvm/irq.h
> > +++ b/xen/include/xen/hvm/irq.h
> > @@ -81,14 +81,16 @@ struct hvm_girq_dpci_mapping {
> >  
> >  /* Protected by domain's event_lock */
> >  struct hvm_irq_dpci {
> > -    /* Guest IRQ to guest device/intx mapping. */
> > -    struct list_head girq[NR_HVM_IRQS];
> >      /* Record of mapped ISA IRQs */
> >      DECLARE_BITMAP(isairq_map, NR_ISAIRQS);
> >      /* Record of mapped Links */
> >      uint8_t link_cnt[NR_LINK];
> > +    /* Guest IRQ to guest device/intx mapping. */
> > +    struct list_head girq[];
> >  };
> 
> Considering what you say in the overview mail I don't think the
> comment can be moved without adjusting it, as it doesn't seem
> to reflect Dom0 in any way. Which then puts under question
> whether struct hvm_girq_dpci_mapping is the right data format
> for Dom0 in the first place: With bus, device, and intx taken
> out, all that's left if machine_gsi, and iirc you identity map GSIs.

Yes, I also got the same feeling. I've done it this way so that I don't have to
touch a lot of code, it's mostly using the same paths as the ones a HVM guest
would use for pass-through.

OTHO, there's a lot of unneeded faff here. I could certainly do with simpler
structures for PVH Dom0 GSI passthrough, but then I would have to do more
changes to pt_irq_create_bind and probably the hvm/irq.c functions. I can look
into that.

> Even if the array needed to remain, the sparseness of the GSI
> space opens up the question whether using a simple array here
> is the right choice.
> 
> The patch needs re-basing anyway afaict.

Sure, most of those where sent a while back.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/5] x86/ioapic: introduce helper to fetch triggering mode of GSI

[Roger Pau Monne]

On Tue, Apr 18, 2017 at 06:19:34AM -0600, Jan Beulich wrote:
> >>> On 27.03.17 at 12:44, <roger.pau@citrix.com> wrote:
> > --- a/xen/arch/x86/io_apic.c
> > +++ b/xen/arch/x86/io_apic.c
> > @@ -2261,6 +2261,28 @@ int io_apic_set_pci_routing (int ioapic, int pin, int irq, int edge_level, int a
> >      return 0;
> >  }
> >  
> > +unsigned int io_apic_get_gsi_trigger(unsigned int gsi)
> 
> bool
> 
> > +{
> > +    struct IO_APIC_route_entry entry;
> > +    unsigned int ioapic, base_gsi;
> > +
> > +    ASSERT(gsi < nr_irqs_gsi);
> > +
> > +    /* For GSI type find if the GSI is level or edge triggered */
> 
> The comment looks like it wants to go ahead of the function instead
> of here. And what's "GSI type"?

GSI type of interrupt, better worded as "For a given GSI find...". But I'm
thinking that it might be better to fetch this from the Dom0 vIO APIC instead.

> > +    for ( ioapic = 0; ioapic < nr_ioapics; ioapic++ )
> > +    {
> > +        base_gsi = io_apic_gsi_base(ioapic);
> > +
> > +        if ( gsi >= base_gsi && gsi < base_gsi + nr_ioapic_entries[ioapic] )
> > +            break;
> > +    }
> > +    ASSERT(ioapic < nr_ioapics);
> > +
> > +    entry = ioapic_read_entry(ioapic, gsi - base_gsi, 0);
> > +
> > +    return entry.trigger;
> > +}
> 
> It is presumably not entirely without reason that you add this function
> without having an immediate user (in existing code) for it: The trigger
> bit is entirely meaningless in a masked RTE.

As said above, it might be better to fetch this from the Dom0 vIO APIC. The
caller is added in the next patch, but if I fetch this from the vIO APIC I
don't need this patch anymore.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel