[PATCH 0/3] XSA-96 follow-ups

[PATCH 0/3] XSA-96 follow-ups

[Jan Beulich]

1: x86/HVM: properly propagate errors from HVMOP_inject_msi
2: x86/HVM: make vmsi_deliver() return proper error values
3: pt-irq fixes and improvements

Signed-off-by: Jan Beulich <jbeulich@suse.com>

[PATCH 1/3] x86/HVM: properly propagate errors from HVMOP_inject_msi

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]

There are a number of ways this operation can go wrong, all of which
got ignored so far.

In the context of this I wonder whether map_domain_emuirq_pirq()
returning 0 in the "already mapped" case is really intended to be that
way (this is why the subsequent NULL check here can't be an ASSERT()).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5096,7 +5096,7 @@ static int hvmop_inject_msi(
     if ( rc )
         goto out;
 
-    hvm_inject_msi(d, op.addr, op.data);
+    rc = hvm_inject_msi(d, op.addr, op.data);
 
  out:
     rcu_unlock_domain(d);
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -270,7 +270,7 @@ void hvm_set_pci_link_route(struct domai
             d->domain_id, link, old_isa_irq, isa_irq);
 }
 
-void hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
+int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
 {
     uint32_t tmp = (uint32_t) addr;
     uint8_t  dest = (tmp & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT;
@@ -292,20 +292,28 @@ void hvm_inject_msi(struct domain *d, ui
             /* if it is the first time, allocate the pirq */
             if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND )
             {
+                int rc;
+
                 spin_lock(&d->event_lock);
-                map_domain_emuirq_pirq(d, pirq, IRQ_MSI_EMU);
+                rc = map_domain_emuirq_pirq(d, pirq, IRQ_MSI_EMU);
                 spin_unlock(&d->event_lock);
+                if ( rc )
+                    return rc;
                 info = pirq_info(d, pirq);
                 if ( !info )
-                    return;
-            } else if (info->arch.hvm.emuirq != IRQ_MSI_EMU)
-                return;
+                    return -EBUSY;
+            }
+            else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU )
+                return -EINVAL;
             send_guest_pirq(d, info);
-            return;
+            return 0;
         }
+        return -ERANGE;
     }
 
     vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
+
+    return 0;
 }
 
 void hvm_set_callback_via(struct domain *d, uint64_t via)
--- a/xen/include/xen/hvm/irq.h
+++ b/xen/include/xen/hvm/irq.h
@@ -123,7 +123,7 @@ void hvm_isa_irq_deassert(
 
 void hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq);
 
-void hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data);
+int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data);
 
 void hvm_maybe_deassert_evtchn_irq(void);
 void hvm_assert_evtchn_irq(struct vcpu *v);




[-- Attachment #2: x86-HVM-inject-MSI-retval.patch --]
[-- Type: text/plain, Size: 2731 bytes --]

x86/HVM: properly propagate errors from HVMOP_inject_msi

There are a number of ways this operation can go wrong, all of which
got ignored so far.

In the context of this I wonder whether map_domain_emuirq_pirq()
returning 0 in the "already mapped" case is really intended to be that
way (this is why the subsequent NULL check here can't be an ASSERT()).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5096,7 +5096,7 @@ static int hvmop_inject_msi(
     if ( rc )
         goto out;
 
-    hvm_inject_msi(d, op.addr, op.data);
+    rc = hvm_inject_msi(d, op.addr, op.data);
 
  out:
     rcu_unlock_domain(d);
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -270,7 +270,7 @@ void hvm_set_pci_link_route(struct domai
             d->domain_id, link, old_isa_irq, isa_irq);
 }
 
-void hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
+int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
 {
     uint32_t tmp = (uint32_t) addr;
     uint8_t  dest = (tmp & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT;
@@ -292,20 +292,28 @@ void hvm_inject_msi(struct domain *d, ui
             /* if it is the first time, allocate the pirq */
             if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND )
             {
+                int rc;
+
                 spin_lock(&d->event_lock);
-                map_domain_emuirq_pirq(d, pirq, IRQ_MSI_EMU);
+                rc = map_domain_emuirq_pirq(d, pirq, IRQ_MSI_EMU);
                 spin_unlock(&d->event_lock);
+                if ( rc )
+                    return rc;
                 info = pirq_info(d, pirq);
                 if ( !info )
-                    return;
-            } else if (info->arch.hvm.emuirq != IRQ_MSI_EMU)
-                return;
+                    return -EBUSY;
+            }
+            else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU )
+                return -EINVAL;
             send_guest_pirq(d, info);
-            return;
+            return 0;
         }
+        return -ERANGE;
     }
 
     vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
+
+    return 0;
 }
 
 void hvm_set_callback_via(struct domain *d, uint64_t via)
--- a/xen/include/xen/hvm/irq.h
+++ b/xen/include/xen/hvm/irq.h
@@ -123,7 +123,7 @@ void hvm_isa_irq_deassert(
 
 void hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq);
 
-void hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data);
+int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data);
 
 void hvm_maybe_deassert_evtchn_irq(void);
 void hvm_assert_evtchn_irq(struct vcpu *v);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

[PATCH 2/3] x86/HVM: make vmsi_deliver() return proper error values

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3477 bytes --]

... and propagate this from hvm_inject_msi(). In the course of this I
spotted further room for cleanup:
- vmsi_inj_irq()'s struct domain * parameter was unused
- vmsi_deliver() pointlessly passed on dest_ExtINT to vmsi_inj_irq()
  (which that one validly refused to handle)
- vmsi_inj_irq()'s sole caller guarantees a proper delivery mode (i.e.
  rather than printing an obscure message we can just BUG())
- some formatting and log message quirks

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -311,9 +311,7 @@ int hvm_inject_msi(struct domain *d, uin
         return -ERANGE;
     }
 
-    vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
-
-    return 0;
+    return vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
 }
 
 void hvm_set_callback_via(struct domain *d, uint64_t via)
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -43,14 +43,12 @@
 #include <asm/io_apic.h>
 
 static void vmsi_inj_irq(
-    struct domain *d,
     struct vlapic *target,
     uint8_t vector,
     uint8_t trig_mode,
     uint8_t delivery_mode)
 {
-    HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "vmsi_inj_irq "
-                "irq %d trig %d delive mode %d\n",
+    HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "vmsi_inj_irq: vec %02x trig %d dm %d\n",
                 vector, trig_mode, delivery_mode);
 
     switch ( delivery_mode )
@@ -60,8 +58,7 @@ static void vmsi_inj_irq(
         vlapic_set_irq(target, vector, trig_mode);
         break;
     default:
-        gdprintk(XENLOG_WARNING, "error delivery mode %d\n", delivery_mode);
-        break;
+        BUG();
     }
 }
 
@@ -76,38 +73,31 @@ int vmsi_deliver(
     switch ( delivery_mode )
     {
     case dest_LowestPrio:
-    {
         target = vlapic_lowest_prio(d, NULL, 0, dest, dest_mode);
         if ( target != NULL )
-            vmsi_inj_irq(d, target, vector, trig_mode, delivery_mode);
-        else
-            HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "null round robin: "
-                        "vector=%x delivery_mode=%x\n",
-                        vector, dest_LowestPrio);
-        break;
-    }
+        {
+            vmsi_inj_irq(target, vector, trig_mode, delivery_mode);
+            break;
+        }
+        HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "null MSI round robin: vector=%02x\n",
+                    vector);
+        return -ESRCH;
 
     case dest_Fixed:
-    case dest_ExtINT:
-    {
         for_each_vcpu ( d, v )
             if ( vlapic_match_dest(vcpu_vlapic(v), NULL,
                                    0, dest, dest_mode) )
-                vmsi_inj_irq(d, vcpu_vlapic(v),
-                             vector, trig_mode, delivery_mode);
+                vmsi_inj_irq(vcpu_vlapic(v), vector,
+                             trig_mode, delivery_mode);
         break;
-    }
 
-    case dest_SMI:
-    case dest_NMI:
-    case dest_INIT:
-    case dest__reserved_2:
     default:
-        gdprintk(XENLOG_WARNING, "Unsupported delivery mode %d\n",
-                 delivery_mode);
-        break;
+        printk(XENLOG_G_WARNING
+               "%pv: Unsupported MSI delivery mode %d for Dom%d\n",
+               current, delivery_mode, d->domain_id);
+        return -EINVAL;
     }
-    return 1;
+    return 0;
 }
 
 void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci *pirq_dpci)




[-- Attachment #2: x86-HVM-vmsi_deliver-retval.patch --]
[-- Type: text/plain, Size: 3530 bytes --]

x86/HVM: make vmsi_deliver() return proper error values

... and propagate this from hvm_inject_msi(). In the course of this I
spotted further room for cleanup:
- vmsi_inj_irq()'s struct domain * parameter was unused
- vmsi_deliver() pointlessly passed on dest_ExtINT to vmsi_inj_irq()
  (which that one validly refused to handle)
- vmsi_inj_irq()'s sole caller guarantees a proper delivery mode (i.e.
  rather than printing an obscure message we can just BUG())
- some formatting and log message quirks

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -311,9 +311,7 @@ int hvm_inject_msi(struct domain *d, uin
         return -ERANGE;
     }
 
-    vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
-
-    return 0;
+    return vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
 }
 
 void hvm_set_callback_via(struct domain *d, uint64_t via)
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -43,14 +43,12 @@
 #include <asm/io_apic.h>
 
 static void vmsi_inj_irq(
-    struct domain *d,
     struct vlapic *target,
     uint8_t vector,
     uint8_t trig_mode,
     uint8_t delivery_mode)
 {
-    HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "vmsi_inj_irq "
-                "irq %d trig %d delive mode %d\n",
+    HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "vmsi_inj_irq: vec %02x trig %d dm %d\n",
                 vector, trig_mode, delivery_mode);
 
     switch ( delivery_mode )
@@ -60,8 +58,7 @@ static void vmsi_inj_irq(
         vlapic_set_irq(target, vector, trig_mode);
         break;
     default:
-        gdprintk(XENLOG_WARNING, "error delivery mode %d\n", delivery_mode);
-        break;
+        BUG();
     }
 }
 
@@ -76,38 +73,31 @@ int vmsi_deliver(
     switch ( delivery_mode )
     {
     case dest_LowestPrio:
-    {
         target = vlapic_lowest_prio(d, NULL, 0, dest, dest_mode);
         if ( target != NULL )
-            vmsi_inj_irq(d, target, vector, trig_mode, delivery_mode);
-        else
-            HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "null round robin: "
-                        "vector=%x delivery_mode=%x\n",
-                        vector, dest_LowestPrio);
-        break;
-    }
+        {
+            vmsi_inj_irq(target, vector, trig_mode, delivery_mode);
+            break;
+        }
+        HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "null MSI round robin: vector=%02x\n",
+                    vector);
+        return -ESRCH;
 
     case dest_Fixed:
-    case dest_ExtINT:
-    {
         for_each_vcpu ( d, v )
             if ( vlapic_match_dest(vcpu_vlapic(v), NULL,
                                    0, dest, dest_mode) )
-                vmsi_inj_irq(d, vcpu_vlapic(v),
-                             vector, trig_mode, delivery_mode);
+                vmsi_inj_irq(vcpu_vlapic(v), vector,
+                             trig_mode, delivery_mode);
         break;
-    }
 
-    case dest_SMI:
-    case dest_NMI:
-    case dest_INIT:
-    case dest__reserved_2:
     default:
-        gdprintk(XENLOG_WARNING, "Unsupported delivery mode %d\n",
-                 delivery_mode);
-        break;
+        printk(XENLOG_G_WARNING
+               "%pv: Unsupported MSI delivery mode %d for Dom%d\n",
+               current, delivery_mode, d->domain_id);
+        return -EINVAL;
     }
-    return 1;
+    return 0;
 }
 
 void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci *pirq_dpci)

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

[PATCH 3/3] pt-irq fixes and improvements

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 17597 bytes --]

Tools side:
- don't silently ignore unrecognized PT_IRQ_TYPE_* values
- respect that the interface type contains a union, making the code at
  once no longer depend on the hypervisor ignoring the bus field of the
  PCI portion of the interface structure)

Hypervisor side:
- don't ignore the PCI bus number passed in
- don't store values (gsi, link) calculated from other stored values
- avoid calling xfree() with a spin lock held where easily possible
- have pt_irq_destroy_bind() respect the passed in type
- scope reduction and constification of various variables
- use switch instead of if/else-if chains
- formatting

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1702,15 +1702,20 @@ int xc_domain_bind_pt_irq(
     bind->hvm_domid = domid;
     bind->irq_type = irq_type;
     bind->machine_irq = machine_irq;
-    if ( irq_type == PT_IRQ_TYPE_PCI ||
-         irq_type == PT_IRQ_TYPE_MSI_TRANSLATE )
+    switch ( irq_type )
     {
+    case PT_IRQ_TYPE_PCI:
+    case PT_IRQ_TYPE_MSI_TRANSLATE:
         bind->u.pci.bus = bus;
         bind->u.pci.device = device;    
         bind->u.pci.intx = intx;
-    } 
-    else if ( irq_type == PT_IRQ_TYPE_ISA )
+    case PT_IRQ_TYPE_ISA:
         bind->u.isa.isa_irq = isa_irq;
+        break;
+    default:
+        errno = EINVAL;
+        return -1;
+    }
     
     rc = do_domctl(xch, &domctl);
     return rc;
@@ -1737,10 +1742,21 @@ int xc_domain_unbind_pt_irq(
     bind->hvm_domid = domid;
     bind->irq_type = irq_type;
     bind->machine_irq = machine_irq;
-    bind->u.pci.bus = bus;
-    bind->u.pci.device = device;    
-    bind->u.pci.intx = intx;
-    bind->u.isa.isa_irq = isa_irq;
+    switch ( irq_type )
+    {
+    case PT_IRQ_TYPE_PCI:
+    case PT_IRQ_TYPE_MSI_TRANSLATE:
+        bind->u.pci.bus = bus;
+        bind->u.pci.device = device;
+        bind->u.pci.intx = intx;
+        break;
+    case PT_IRQ_TYPE_ISA:
+        bind->u.isa.isa_irq = isa_irq;
+        break;
+    default:
+        errno = EINVAL;
+        return -1;
+    }
     
     rc = do_domctl(xch, &domctl);
     return rc;
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -37,9 +37,8 @@ static int physdev_hvm_map_pirq(
     switch ( type )
     {
     case MAP_PIRQ_TYPE_GSI: {
-        struct hvm_irq_dpci *hvm_irq_dpci;
-        struct hvm_girq_dpci_mapping *girq;
-        uint32_t machine_gsi = 0;
+        const struct hvm_irq_dpci *hvm_irq_dpci;
+        unsigned int machine_gsi = 0;
 
         if ( *index < 0 || *index >= NR_HVM_IRQS )
         {
@@ -52,6 +51,8 @@ static int physdev_hvm_map_pirq(
         hvm_irq_dpci = domain_get_irq_dpci(d);
         if ( hvm_irq_dpci )
         {
+            const struct hvm_girq_dpci_mapping *girq;
+
             BUILD_BUG_ON(ARRAY_SIZE(hvm_irq_dpci->girq) < NR_HVM_IRQS);
             list_for_each_entry ( girq,
                                   &hvm_irq_dpci->girq[*index],
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -51,11 +51,8 @@ static int pt_irq_guest_eoi(struct domai
 static void pt_irq_time_out(void *data)
 {
     struct hvm_pirq_dpci *irq_map = data;
-    unsigned int guest_gsi;
-    struct hvm_irq_dpci *dpci = NULL;
-    struct dev_intx_gsi_link *digl;
-    struct hvm_girq_dpci_mapping *girq;
-    uint32_t device, intx;
+    const struct hvm_irq_dpci *dpci;
+    const struct dev_intx_gsi_link *digl;
 
     spin_lock(&irq_map->dom->event_lock);
 
@@ -63,16 +60,16 @@ static void pt_irq_time_out(void *data)
     ASSERT(dpci);
     list_for_each_entry ( digl, &irq_map->digl_list, list )
     {
-        guest_gsi = digl->gsi;
+        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;
         }
-        device = digl->device;
-        intx = digl->intx;
-        hvm_pci_intx_deassert(irq_map->dom, device, intx);
+        hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx);
     }
 
     pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL);
@@ -96,13 +93,9 @@ void free_hvm_irq_dpci(struct hvm_irq_dp
 int pt_irq_create_bind(
     struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind)
 {
-    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
+    struct hvm_irq_dpci *hvm_irq_dpci;
     struct hvm_pirq_dpci *pirq_dpci;
     struct pirq *info;
-    uint32_t guest_gsi;
-    uint32_t device, intx, link;
-    struct dev_intx_gsi_link *digl;
-    struct hvm_girq_dpci_mapping *girq;
     int rc, pirq = pt_irq_bind->machine_irq;
 
     if ( pirq < 0 || pirq >= d->nr_pirqs )
@@ -136,7 +129,9 @@ int pt_irq_create_bind(
     }
     pirq_dpci = pirq_dpci(info);
 
-    if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI )
+    switch ( pt_irq_bind->irq_type )
+    {
+    case PT_IRQ_TYPE_MSI:
     {
         uint8_t dest, dest_mode;
         int dest_vcpu_id;
@@ -169,15 +164,16 @@ int pt_irq_create_bind(
         {
             uint32_t mask = HVM_IRQ_DPCI_MACH_MSI | HVM_IRQ_DPCI_GUEST_MSI;
 
-            if ( (pirq_dpci->flags & mask) != mask)
+            if ( (pirq_dpci->flags & mask) != mask )
             {
-	            spin_unlock(&d->event_lock);
-        	    return -EBUSY;
+                spin_unlock(&d->event_lock);
+                return -EBUSY;
             }
 
-            /* if pirq is already mapped as vmsi, update the guest data/addr */
+            /* If pirq is already mapped as vmsi, update guest data/addr. */
             if ( pirq_dpci->gmsi.gvec != pt_irq_bind->u.msi.gvec ||
-                 pirq_dpci->gmsi.gflags != pt_irq_bind->u.msi.gflags) {
+                 pirq_dpci->gmsi.gflags != pt_irq_bind->u.msi.gflags )
+            {
                 /* Directly clear pending EOIs before enabling new MSI info. */
                 pirq_guest_eoi(info);
 
@@ -185,7 +181,7 @@ int pt_irq_create_bind(
                 pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags;
             }
         }
-        /* Caculate dest_vcpu_id for MSI-type pirq migration */
+        /* Calculate dest_vcpu_id for MSI-type pirq migration. */
         dest = pirq_dpci->gmsi.gflags & VMSI_DEST_ID_MASK;
         dest_mode = !!(pirq_dpci->gmsi.gflags & VMSI_DM_MASK);
         dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
@@ -193,36 +189,37 @@ int pt_irq_create_bind(
         spin_unlock(&d->event_lock);
         if ( dest_vcpu_id >= 0 )
             hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]);
+        break;
     }
-    else
+
+    case PT_IRQ_TYPE_PCI:
+    case PT_IRQ_TYPE_MSI_TRANSLATE:
     {
-        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);
-        hvm_irq_dpci->link_cnt[link]++;
+        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);
+        struct hvm_girq_dpci_mapping *girq =
+            xmalloc(struct hvm_girq_dpci_mapping);
 
-        digl = xmalloc(struct dev_intx_gsi_link);
-        if ( !digl )
+        if ( !digl || !girq )
         {
             spin_unlock(&d->event_lock);
-            return -ENOMEM;
-        }
-
-        girq = xmalloc(struct hvm_girq_dpci_mapping);
-        if ( !girq )
-        {
+            xfree(girq);
             xfree(digl);
-            spin_unlock(&d->event_lock);
             return -ENOMEM;
         }
 
+        hvm_irq_dpci->link_cnt[link]++;
+
+        digl->bus = bus;
         digl->device = device;
         digl->intx = intx;
-        digl->gsi = guest_gsi;
-        digl->link = link;
         list_add_tail(&digl->list, &pirq_dpci->digl_list);
 
+        girq->bus = bus;
         girq->device = device;
         girq->intx = intx;
         girq->machine_gsi = pirq;
@@ -261,12 +258,12 @@ int pt_irq_create_bind(
                     kill_timer(&pirq_dpci->timer);
                 pirq_dpci->dom = NULL;
                 list_del(&girq->list);
-                xfree(girq);
                 list_del(&digl->list);
                 hvm_irq_dpci->link_cnt[link]--;
                 pirq_dpci->flags = 0;
                 pirq_cleanup_check(info, d);
                 spin_unlock(&d->event_lock);
+                xfree(girq);
                 xfree(digl);
                 return rc;
             }
@@ -276,33 +273,51 @@ int pt_irq_create_bind(
 
         if ( iommu_verbose )
             dprintk(XENLOG_G_INFO,
-                    "d%d: bind: m_gsi=%u g_gsi=%u device=%u intx=%u\n",
-                    d->domain_id, pirq, guest_gsi, device, intx);
+                    "d%d: bind: m_gsi=%u g_gsi=%u dev=%02x.%02x.%u intx=%u\n",
+                    d->domain_id, pirq, guest_gsi, bus,
+                    PCI_SLOT(device), PCI_FUNC(device), intx);
+        break;
+    }
+
+    default:
+        spin_unlock(&d->event_lock);
+        return -EOPNOTSUPP;
     }
+
     return 0;
 }
 
 int pt_irq_destroy_bind(
     struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind)
 {
-    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
+    struct hvm_irq_dpci *hvm_irq_dpci;
     struct hvm_pirq_dpci *pirq_dpci;
-    uint32_t machine_gsi, guest_gsi;
-    uint32_t device, intx, link;
+    unsigned int machine_gsi = pt_irq_bind->machine_irq;
+    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, *tmp;
     struct hvm_girq_dpci_mapping *girq;
     struct pirq *pirq;
 
-    machine_gsi = pt_irq_bind->machine_irq;
-    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);
+    switch ( pt_irq_bind->irq_type )
+    {
+    case PT_IRQ_TYPE_PCI:
+    case PT_IRQ_TYPE_MSI_TRANSLATE:
+        break;
+    case PT_IRQ_TYPE_MSI:
+        return 0;
+    default:
+        return -EOPNOTSUPP;
+    }
 
     if ( iommu_verbose )
         dprintk(XENLOG_G_INFO,
-                "d%d: unbind: m_gsi=%u g_gsi=%u device=%u intx=%u\n",
-                d->domain_id, machine_gsi, guest_gsi, device, intx);
+                "d%d: unbind: m_gsi=%u g_gsi=%u dev=%02x:%02x.%u intx=%u\n",
+                d->domain_id, machine_gsi, guest_gsi, bus,
+                PCI_SLOT(device), PCI_FUNC(device), intx);
 
     spin_lock(&d->event_lock);
 
@@ -314,18 +329,28 @@ int pt_irq_destroy_bind(
         return -EINVAL;
     }
 
-    hvm_irq_dpci->link_cnt[link]--;
-
     list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list )
     {
-        if ( girq->machine_gsi == machine_gsi )
+        if ( girq->bus         == bus &&
+             girq->device      == device &&
+             girq->intx        == intx &&
+             girq->machine_gsi == machine_gsi )
         {
-                list_del(&girq->list);
-                xfree(girq);
-                break;
+            list_del(&girq->list);
+            xfree(girq);
+            girq = NULL;
+            break;
         }
     }
 
+    if ( girq )
+    {
+        spin_unlock(&d->event_lock);
+        return -EINVAL;
+    }
+
+    hvm_irq_dpci->link_cnt[link]--;
+
     pirq = pirq_info(d, machine_gsi);
     pirq_dpci = pirq_dpci(pirq);
 
@@ -334,10 +359,9 @@ int pt_irq_destroy_bind(
     {
         list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
         {
-            if ( digl->device == device &&
-                 digl->intx   == intx &&
-                 digl->link   == link &&
-                 digl->gsi    == guest_gsi )
+            if ( digl->bus    == bus &&
+                 digl->device == device &&
+                 digl->intx   == intx )
             {
                 list_del(&digl->list);
                 xfree(digl);
@@ -359,8 +383,9 @@ int pt_irq_destroy_bind(
 
     if ( iommu_verbose )
         dprintk(XENLOG_G_INFO,
-                "d%d unmap: m_irq=%u device=%u intx=%u\n",
-                d->domain_id, machine_gsi, device, intx);
+                "d%d unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n",
+                d->domain_id, machine_gsi, bus,
+                PCI_SLOT(device), PCI_FUNC(device), intx);
 
     return 0;
 }
@@ -481,11 +506,10 @@ static void hvm_pci_msi_assert(
 static int _hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci,
                             void *arg)
 {
-    uint32_t device, intx;
-    struct dev_intx_gsi_link *digl;
-
     if ( test_and_clear_bool(pirq_dpci->masked) )
     {
+        const struct dev_intx_gsi_link *digl;
+
         if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI )
         {
             hvm_pci_msi_assert(d, pirq_dpci);
@@ -496,12 +520,10 @@ static int _hvm_dirq_assist(struct domai
         {
             struct pirq *info = dpci_pirq(pirq_dpci);
 
-            device = digl->device;
-            intx = digl->intx;
             if ( hvm_domain_use_pirq(d, info) )
                 send_guest_pirq(d, info);
             else
-                hvm_pci_intx_assert(d, device, intx);
+                hvm_pci_intx_assert(d, digl->device, digl->intx);
             pirq_dpci->pending++;
 
             if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE )
@@ -537,16 +559,13 @@ static void hvm_dirq_assist(unsigned lon
 }
 
 static void __hvm_dpci_eoi(struct domain *d,
-                           struct hvm_girq_dpci_mapping *girq,
-                           union vioapic_redir_entry *ent)
+                           const struct hvm_girq_dpci_mapping *girq,
+                           const union vioapic_redir_entry *ent)
 {
-    uint32_t device, intx;
     struct pirq *pirq;
     struct hvm_pirq_dpci *pirq_dpci;
 
-    device = girq->device;
-    intx = girq->intx;
-    hvm_pci_intx_deassert(d, device, intx);
+    hvm_pci_intx_deassert(d, girq->device, girq->intx);
 
     pirq = pirq_info(d, girq->machine_gsi);
     pirq_dpci = pirq_dpci(pirq);
@@ -556,8 +575,8 @@ static void __hvm_dpci_eoi(struct domain
      * since interrupt is still not EOIed
      */
     if ( --pirq_dpci->pending ||
-         ( ent && ent->fields.mask ) ||
-         ! pt_irq_need_timer(pirq_dpci->flags) )
+         (ent && ent->fields.mask) ||
+         !pt_irq_need_timer(pirq_dpci->flags) )
         return;
 
     stop_timer(&pirq_dpci->timer);
@@ -565,10 +584,10 @@ static void __hvm_dpci_eoi(struct domain
 }
 
 void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi,
-                  union vioapic_redir_entry *ent)
+                  const union vioapic_redir_entry *ent)
 {
-    struct hvm_irq_dpci *hvm_irq_dpci;
-    struct hvm_girq_dpci_mapping *girq;
+    const struct hvm_irq_dpci *hvm_irq_dpci;
+    const struct hvm_girq_dpci_mapping *girq;
 
     if ( !iommu_enabled )
         return;
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -69,11 +69,13 @@ static int _hvm_dpci_isairq_eoi(struct d
 {
     struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
     unsigned int isairq = (long)arg;
-    struct dev_intx_gsi_link *digl, *tmp;
+    const struct dev_intx_gsi_link *digl;
 
-    list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
+    list_for_each_entry ( digl, &pirq_dpci->digl_list, list )
     {
-        if ( hvm_irq->pci_link.route[digl->link] == isairq )
+        unsigned int link = hvm_pci_intx_link(digl->device, digl->intx);
+
+        if ( hvm_irq->pci_link.route[link] == isairq )
         {
             hvm_pci_intx_deassert(d, digl->device, digl->intx);
             if ( --pirq_dpci->pending == 0 )
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -123,7 +123,7 @@ int handle_pio(uint16_t port, unsigned i
 void hvm_interrupt_post(struct vcpu *v, int vector, int type);
 void hvm_io_assist(ioreq_t *p);
 void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq,
-                  union vioapic_redir_entry *ent);
+                  const union vioapic_redir_entry *ent);
 void msix_write_completion(struct vcpu *);
 
 struct hvm_hw_stdvga {
--- a/xen/include/xen/hvm/irq.h
+++ b/xen/include/xen/hvm/irq.h
@@ -30,10 +30,9 @@
 
 struct dev_intx_gsi_link {
     struct list_head list;
+    uint8_t bus;
     uint8_t device;
     uint8_t intx;
-    uint8_t gsi;
-    uint8_t link;
 };
 
 #define _HVM_IRQ_DPCI_MACH_PCI_SHIFT            0
@@ -69,6 +68,7 @@ struct hvm_gmsi_info {
 
 struct hvm_girq_dpci_mapping {
     struct list_head list;
+    uint8_t bus;
     uint8_t device;
     uint8_t intx;
     uint8_t machine_gsi;



[-- Attachment #2: pt-irq-binding.patch --]
[-- Type: text/plain, Size: 17626 bytes --]

pt-irq fixes and improvements

Tools side:
- don't silently ignore unrecognized PT_IRQ_TYPE_* values
- respect that the interface type contains a union, making the code at
  once no longer depend on the hypervisor ignoring the bus field of the
  PCI portion of the interface structure)

Hypervisor side:
- don't ignore the PCI bus number passed in
- don't store values (gsi, link) calculated from other stored values
- avoid calling xfree() with a spin lock held where easily possible
- have pt_irq_destroy_bind() respect the passed in type
- scope reduction and constification of various variables
- use switch instead of if/else-if chains
- formatting

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1702,15 +1702,20 @@ int xc_domain_bind_pt_irq(
     bind->hvm_domid = domid;
     bind->irq_type = irq_type;
     bind->machine_irq = machine_irq;
-    if ( irq_type == PT_IRQ_TYPE_PCI ||
-         irq_type == PT_IRQ_TYPE_MSI_TRANSLATE )
+    switch ( irq_type )
     {
+    case PT_IRQ_TYPE_PCI:
+    case PT_IRQ_TYPE_MSI_TRANSLATE:
         bind->u.pci.bus = bus;
         bind->u.pci.device = device;    
         bind->u.pci.intx = intx;
-    } 
-    else if ( irq_type == PT_IRQ_TYPE_ISA )
+    case PT_IRQ_TYPE_ISA:
         bind->u.isa.isa_irq = isa_irq;
+        break;
+    default:
+        errno = EINVAL;
+        return -1;
+    }
     
     rc = do_domctl(xch, &domctl);
     return rc;
@@ -1737,10 +1742,21 @@ int xc_domain_unbind_pt_irq(
     bind->hvm_domid = domid;
     bind->irq_type = irq_type;
     bind->machine_irq = machine_irq;
-    bind->u.pci.bus = bus;
-    bind->u.pci.device = device;    
-    bind->u.pci.intx = intx;
-    bind->u.isa.isa_irq = isa_irq;
+    switch ( irq_type )
+    {
+    case PT_IRQ_TYPE_PCI:
+    case PT_IRQ_TYPE_MSI_TRANSLATE:
+        bind->u.pci.bus = bus;
+        bind->u.pci.device = device;
+        bind->u.pci.intx = intx;
+        break;
+    case PT_IRQ_TYPE_ISA:
+        bind->u.isa.isa_irq = isa_irq;
+        break;
+    default:
+        errno = EINVAL;
+        return -1;
+    }
     
     rc = do_domctl(xch, &domctl);
     return rc;
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -37,9 +37,8 @@ static int physdev_hvm_map_pirq(
     switch ( type )
     {
     case MAP_PIRQ_TYPE_GSI: {
-        struct hvm_irq_dpci *hvm_irq_dpci;
-        struct hvm_girq_dpci_mapping *girq;
-        uint32_t machine_gsi = 0;
+        const struct hvm_irq_dpci *hvm_irq_dpci;
+        unsigned int machine_gsi = 0;
 
         if ( *index < 0 || *index >= NR_HVM_IRQS )
         {
@@ -52,6 +51,8 @@ static int physdev_hvm_map_pirq(
         hvm_irq_dpci = domain_get_irq_dpci(d);
         if ( hvm_irq_dpci )
         {
+            const struct hvm_girq_dpci_mapping *girq;
+
             BUILD_BUG_ON(ARRAY_SIZE(hvm_irq_dpci->girq) < NR_HVM_IRQS);
             list_for_each_entry ( girq,
                                   &hvm_irq_dpci->girq[*index],
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -51,11 +51,8 @@ static int pt_irq_guest_eoi(struct domai
 static void pt_irq_time_out(void *data)
 {
     struct hvm_pirq_dpci *irq_map = data;
-    unsigned int guest_gsi;
-    struct hvm_irq_dpci *dpci = NULL;
-    struct dev_intx_gsi_link *digl;
-    struct hvm_girq_dpci_mapping *girq;
-    uint32_t device, intx;
+    const struct hvm_irq_dpci *dpci;
+    const struct dev_intx_gsi_link *digl;
 
     spin_lock(&irq_map->dom->event_lock);
 
@@ -63,16 +60,16 @@ static void pt_irq_time_out(void *data)
     ASSERT(dpci);
     list_for_each_entry ( digl, &irq_map->digl_list, list )
     {
-        guest_gsi = digl->gsi;
+        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;
         }
-        device = digl->device;
-        intx = digl->intx;
-        hvm_pci_intx_deassert(irq_map->dom, device, intx);
+        hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx);
     }
 
     pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL);
@@ -96,13 +93,9 @@ void free_hvm_irq_dpci(struct hvm_irq_dp
 int pt_irq_create_bind(
     struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind)
 {
-    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
+    struct hvm_irq_dpci *hvm_irq_dpci;
     struct hvm_pirq_dpci *pirq_dpci;
     struct pirq *info;
-    uint32_t guest_gsi;
-    uint32_t device, intx, link;
-    struct dev_intx_gsi_link *digl;
-    struct hvm_girq_dpci_mapping *girq;
     int rc, pirq = pt_irq_bind->machine_irq;
 
     if ( pirq < 0 || pirq >= d->nr_pirqs )
@@ -136,7 +129,9 @@ int pt_irq_create_bind(
     }
     pirq_dpci = pirq_dpci(info);
 
-    if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI )
+    switch ( pt_irq_bind->irq_type )
+    {
+    case PT_IRQ_TYPE_MSI:
     {
         uint8_t dest, dest_mode;
         int dest_vcpu_id;
@@ -169,15 +164,16 @@ int pt_irq_create_bind(
         {
             uint32_t mask = HVM_IRQ_DPCI_MACH_MSI | HVM_IRQ_DPCI_GUEST_MSI;
 
-            if ( (pirq_dpci->flags & mask) != mask)
+            if ( (pirq_dpci->flags & mask) != mask )
             {
-	            spin_unlock(&d->event_lock);
-        	    return -EBUSY;
+                spin_unlock(&d->event_lock);
+                return -EBUSY;
             }
 
-            /* if pirq is already mapped as vmsi, update the guest data/addr */
+            /* If pirq is already mapped as vmsi, update guest data/addr. */
             if ( pirq_dpci->gmsi.gvec != pt_irq_bind->u.msi.gvec ||
-                 pirq_dpci->gmsi.gflags != pt_irq_bind->u.msi.gflags) {
+                 pirq_dpci->gmsi.gflags != pt_irq_bind->u.msi.gflags )
+            {
                 /* Directly clear pending EOIs before enabling new MSI info. */
                 pirq_guest_eoi(info);
 
@@ -185,7 +181,7 @@ int pt_irq_create_bind(
                 pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags;
             }
         }
-        /* Caculate dest_vcpu_id for MSI-type pirq migration */
+        /* Calculate dest_vcpu_id for MSI-type pirq migration. */
         dest = pirq_dpci->gmsi.gflags & VMSI_DEST_ID_MASK;
         dest_mode = !!(pirq_dpci->gmsi.gflags & VMSI_DM_MASK);
         dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
@@ -193,36 +189,37 @@ int pt_irq_create_bind(
         spin_unlock(&d->event_lock);
         if ( dest_vcpu_id >= 0 )
             hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]);
+        break;
     }
-    else
+
+    case PT_IRQ_TYPE_PCI:
+    case PT_IRQ_TYPE_MSI_TRANSLATE:
     {
-        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);
-        hvm_irq_dpci->link_cnt[link]++;
+        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);
+        struct hvm_girq_dpci_mapping *girq =
+            xmalloc(struct hvm_girq_dpci_mapping);
 
-        digl = xmalloc(struct dev_intx_gsi_link);
-        if ( !digl )
+        if ( !digl || !girq )
         {
             spin_unlock(&d->event_lock);
-            return -ENOMEM;
-        }
-
-        girq = xmalloc(struct hvm_girq_dpci_mapping);
-        if ( !girq )
-        {
+            xfree(girq);
             xfree(digl);
-            spin_unlock(&d->event_lock);
             return -ENOMEM;
         }
 
+        hvm_irq_dpci->link_cnt[link]++;
+
+        digl->bus = bus;
         digl->device = device;
         digl->intx = intx;
-        digl->gsi = guest_gsi;
-        digl->link = link;
         list_add_tail(&digl->list, &pirq_dpci->digl_list);
 
+        girq->bus = bus;
         girq->device = device;
         girq->intx = intx;
         girq->machine_gsi = pirq;
@@ -261,12 +258,12 @@ int pt_irq_create_bind(
                     kill_timer(&pirq_dpci->timer);
                 pirq_dpci->dom = NULL;
                 list_del(&girq->list);
-                xfree(girq);
                 list_del(&digl->list);
                 hvm_irq_dpci->link_cnt[link]--;
                 pirq_dpci->flags = 0;
                 pirq_cleanup_check(info, d);
                 spin_unlock(&d->event_lock);
+                xfree(girq);
                 xfree(digl);
                 return rc;
             }
@@ -276,33 +273,51 @@ int pt_irq_create_bind(
 
         if ( iommu_verbose )
             dprintk(XENLOG_G_INFO,
-                    "d%d: bind: m_gsi=%u g_gsi=%u device=%u intx=%u\n",
-                    d->domain_id, pirq, guest_gsi, device, intx);
+                    "d%d: bind: m_gsi=%u g_gsi=%u dev=%02x.%02x.%u intx=%u\n",
+                    d->domain_id, pirq, guest_gsi, bus,
+                    PCI_SLOT(device), PCI_FUNC(device), intx);
+        break;
+    }
+
+    default:
+        spin_unlock(&d->event_lock);
+        return -EOPNOTSUPP;
     }
+
     return 0;
 }
 
 int pt_irq_destroy_bind(
     struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind)
 {
-    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
+    struct hvm_irq_dpci *hvm_irq_dpci;
     struct hvm_pirq_dpci *pirq_dpci;
-    uint32_t machine_gsi, guest_gsi;
-    uint32_t device, intx, link;
+    unsigned int machine_gsi = pt_irq_bind->machine_irq;
+    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, *tmp;
     struct hvm_girq_dpci_mapping *girq;
     struct pirq *pirq;
 
-    machine_gsi = pt_irq_bind->machine_irq;
-    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);
+    switch ( pt_irq_bind->irq_type )
+    {
+    case PT_IRQ_TYPE_PCI:
+    case PT_IRQ_TYPE_MSI_TRANSLATE:
+        break;
+    case PT_IRQ_TYPE_MSI:
+        return 0;
+    default:
+        return -EOPNOTSUPP;
+    }
 
     if ( iommu_verbose )
         dprintk(XENLOG_G_INFO,
-                "d%d: unbind: m_gsi=%u g_gsi=%u device=%u intx=%u\n",
-                d->domain_id, machine_gsi, guest_gsi, device, intx);
+                "d%d: unbind: m_gsi=%u g_gsi=%u dev=%02x:%02x.%u intx=%u\n",
+                d->domain_id, machine_gsi, guest_gsi, bus,
+                PCI_SLOT(device), PCI_FUNC(device), intx);
 
     spin_lock(&d->event_lock);
 
@@ -314,18 +329,28 @@ int pt_irq_destroy_bind(
         return -EINVAL;
     }
 
-    hvm_irq_dpci->link_cnt[link]--;
-
     list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list )
     {
-        if ( girq->machine_gsi == machine_gsi )
+        if ( girq->bus         == bus &&
+             girq->device      == device &&
+             girq->intx        == intx &&
+             girq->machine_gsi == machine_gsi )
         {
-                list_del(&girq->list);
-                xfree(girq);
-                break;
+            list_del(&girq->list);
+            xfree(girq);
+            girq = NULL;
+            break;
         }
     }
 
+    if ( girq )
+    {
+        spin_unlock(&d->event_lock);
+        return -EINVAL;
+    }
+
+    hvm_irq_dpci->link_cnt[link]--;
+
     pirq = pirq_info(d, machine_gsi);
     pirq_dpci = pirq_dpci(pirq);
 
@@ -334,10 +359,9 @@ int pt_irq_destroy_bind(
     {
         list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
         {
-            if ( digl->device == device &&
-                 digl->intx   == intx &&
-                 digl->link   == link &&
-                 digl->gsi    == guest_gsi )
+            if ( digl->bus    == bus &&
+                 digl->device == device &&
+                 digl->intx   == intx )
             {
                 list_del(&digl->list);
                 xfree(digl);
@@ -359,8 +383,9 @@ int pt_irq_destroy_bind(
 
     if ( iommu_verbose )
         dprintk(XENLOG_G_INFO,
-                "d%d unmap: m_irq=%u device=%u intx=%u\n",
-                d->domain_id, machine_gsi, device, intx);
+                "d%d unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n",
+                d->domain_id, machine_gsi, bus,
+                PCI_SLOT(device), PCI_FUNC(device), intx);
 
     return 0;
 }
@@ -481,11 +506,10 @@ static void hvm_pci_msi_assert(
 static int _hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci,
                             void *arg)
 {
-    uint32_t device, intx;
-    struct dev_intx_gsi_link *digl;
-
     if ( test_and_clear_bool(pirq_dpci->masked) )
     {
+        const struct dev_intx_gsi_link *digl;
+
         if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI )
         {
             hvm_pci_msi_assert(d, pirq_dpci);
@@ -496,12 +520,10 @@ static int _hvm_dirq_assist(struct domai
         {
             struct pirq *info = dpci_pirq(pirq_dpci);
 
-            device = digl->device;
-            intx = digl->intx;
             if ( hvm_domain_use_pirq(d, info) )
                 send_guest_pirq(d, info);
             else
-                hvm_pci_intx_assert(d, device, intx);
+                hvm_pci_intx_assert(d, digl->device, digl->intx);
             pirq_dpci->pending++;
 
             if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE )
@@ -537,16 +559,13 @@ static void hvm_dirq_assist(unsigned lon
 }
 
 static void __hvm_dpci_eoi(struct domain *d,
-                           struct hvm_girq_dpci_mapping *girq,
-                           union vioapic_redir_entry *ent)
+                           const struct hvm_girq_dpci_mapping *girq,
+                           const union vioapic_redir_entry *ent)
 {
-    uint32_t device, intx;
     struct pirq *pirq;
     struct hvm_pirq_dpci *pirq_dpci;
 
-    device = girq->device;
-    intx = girq->intx;
-    hvm_pci_intx_deassert(d, device, intx);
+    hvm_pci_intx_deassert(d, girq->device, girq->intx);
 
     pirq = pirq_info(d, girq->machine_gsi);
     pirq_dpci = pirq_dpci(pirq);
@@ -556,8 +575,8 @@ static void __hvm_dpci_eoi(struct domain
      * since interrupt is still not EOIed
      */
     if ( --pirq_dpci->pending ||
-         ( ent && ent->fields.mask ) ||
-         ! pt_irq_need_timer(pirq_dpci->flags) )
+         (ent && ent->fields.mask) ||
+         !pt_irq_need_timer(pirq_dpci->flags) )
         return;
 
     stop_timer(&pirq_dpci->timer);
@@ -565,10 +584,10 @@ static void __hvm_dpci_eoi(struct domain
 }
 
 void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi,
-                  union vioapic_redir_entry *ent)
+                  const union vioapic_redir_entry *ent)
 {
-    struct hvm_irq_dpci *hvm_irq_dpci;
-    struct hvm_girq_dpci_mapping *girq;
+    const struct hvm_irq_dpci *hvm_irq_dpci;
+    const struct hvm_girq_dpci_mapping *girq;
 
     if ( !iommu_enabled )
         return;
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -69,11 +69,13 @@ static int _hvm_dpci_isairq_eoi(struct d
 {
     struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
     unsigned int isairq = (long)arg;
-    struct dev_intx_gsi_link *digl, *tmp;
+    const struct dev_intx_gsi_link *digl;
 
-    list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
+    list_for_each_entry ( digl, &pirq_dpci->digl_list, list )
     {
-        if ( hvm_irq->pci_link.route[digl->link] == isairq )
+        unsigned int link = hvm_pci_intx_link(digl->device, digl->intx);
+
+        if ( hvm_irq->pci_link.route[link] == isairq )
         {
             hvm_pci_intx_deassert(d, digl->device, digl->intx);
             if ( --pirq_dpci->pending == 0 )
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -123,7 +123,7 @@ int handle_pio(uint16_t port, unsigned i
 void hvm_interrupt_post(struct vcpu *v, int vector, int type);
 void hvm_io_assist(ioreq_t *p);
 void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq,
-                  union vioapic_redir_entry *ent);
+                  const union vioapic_redir_entry *ent);
 void msix_write_completion(struct vcpu *);
 
 struct hvm_hw_stdvga {
--- a/xen/include/xen/hvm/irq.h
+++ b/xen/include/xen/hvm/irq.h
@@ -30,10 +30,9 @@
 
 struct dev_intx_gsi_link {
     struct list_head list;
+    uint8_t bus;
     uint8_t device;
     uint8_t intx;
-    uint8_t gsi;
-    uint8_t link;
 };
 
 #define _HVM_IRQ_DPCI_MACH_PCI_SHIFT            0
@@ -69,6 +68,7 @@ struct hvm_gmsi_info {
 
 struct hvm_girq_dpci_mapping {
     struct list_head list;
+    uint8_t bus;
     uint8_t device;
     uint8_t intx;
     uint8_t machine_gsi;

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 1/3] x86/HVM: properly propagate errors from HVMOP_inject_msi

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2984 bytes --]

On 03/06/14 14:58, Jan Beulich wrote:
> There are a number of ways this operation can go wrong, all of which
> got ignored so far.
>
> In the context of this I wonder whether map_domain_emuirq_pirq()
> returning 0 in the "already mapped" case is really intended to be that
> way (this is why the subsequent NULL check here can't be an ASSERT()).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5096,7 +5096,7 @@ static int hvmop_inject_msi(
>      if ( rc )
>          goto out;
>  
> -    hvm_inject_msi(d, op.addr, op.data);
> +    rc = hvm_inject_msi(d, op.addr, op.data);
>  
>   out:
>      rcu_unlock_domain(d);
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -270,7 +270,7 @@ void hvm_set_pci_link_route(struct domai
>              d->domain_id, link, old_isa_irq, isa_irq);
>  }
>  
> -void hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
> +int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
>  {
>      uint32_t tmp = (uint32_t) addr;
>      uint8_t  dest = (tmp & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT;
> @@ -292,20 +292,28 @@ void hvm_inject_msi(struct domain *d, ui
>              /* if it is the first time, allocate the pirq */
>              if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND )
>              {
> +                int rc;
> +
>                  spin_lock(&d->event_lock);
> -                map_domain_emuirq_pirq(d, pirq, IRQ_MSI_EMU);
> +                rc = map_domain_emuirq_pirq(d, pirq, IRQ_MSI_EMU);
>                  spin_unlock(&d->event_lock);
> +                if ( rc )
> +                    return rc;
>                  info = pirq_info(d, pirq);
>                  if ( !info )
> -                    return;
> -            } else if (info->arch.hvm.emuirq != IRQ_MSI_EMU)
> -                return;
> +                    return -EBUSY;
> +            }
> +            else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU )
> +                return -EINVAL;
>              send_guest_pirq(d, info);
> -            return;
> +            return 0;
>          }
> +        return -ERANGE;
>      }
>  
>      vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
> +
> +    return 0;
>  }
>  
>  void hvm_set_callback_via(struct domain *d, uint64_t via)
> --- a/xen/include/xen/hvm/irq.h
> +++ b/xen/include/xen/hvm/irq.h
> @@ -123,7 +123,7 @@ void hvm_isa_irq_deassert(
>  
>  void hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq);
>  
> -void hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data);
> +int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data);
>  
>  void hvm_maybe_deassert_evtchn_irq(void);
>  void hvm_assert_evtchn_irq(struct vcpu *v);
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 3793 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/3] x86/HVM: make vmsi_deliver() return proper error values

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 3937 bytes --]

On 03/06/14 14:59, Jan Beulich wrote:
> ... and propagate this from hvm_inject_msi(). In the course of this I
> spotted further room for cleanup:
> - vmsi_inj_irq()'s struct domain * parameter was unused
> - vmsi_deliver() pointlessly passed on dest_ExtINT to vmsi_inj_irq()
>   (which that one validly refused to handle)
> - vmsi_inj_irq()'s sole caller guarantees a proper delivery mode (i.e.
>   rather than printing an obscure message we can just BUG())
> - some formatting and log message quirks
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Arguably under Xen style, the final addition in the final hunk should
have a newline before return 0, but that's trivial.

>
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -311,9 +311,7 @@ int hvm_inject_msi(struct domain *d, uin
>          return -ERANGE;
>      }
>  
> -    vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
> -
> -    return 0;
> +    return vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
>  }
>  
>  void hvm_set_callback_via(struct domain *d, uint64_t via)
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -43,14 +43,12 @@
>  #include <asm/io_apic.h>
>  
>  static void vmsi_inj_irq(
> -    struct domain *d,
>      struct vlapic *target,
>      uint8_t vector,
>      uint8_t trig_mode,
>      uint8_t delivery_mode)
>  {
> -    HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "vmsi_inj_irq "
> -                "irq %d trig %d delive mode %d\n",
> +    HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "vmsi_inj_irq: vec %02x trig %d dm %d\n",
>                  vector, trig_mode, delivery_mode);
>  
>      switch ( delivery_mode )
> @@ -60,8 +58,7 @@ static void vmsi_inj_irq(
>          vlapic_set_irq(target, vector, trig_mode);
>          break;
>      default:
> -        gdprintk(XENLOG_WARNING, "error delivery mode %d\n", delivery_mode);
> -        break;
> +        BUG();
>      }
>  }
>  
> @@ -76,38 +73,31 @@ int vmsi_deliver(
>      switch ( delivery_mode )
>      {
>      case dest_LowestPrio:
> -    {
>          target = vlapic_lowest_prio(d, NULL, 0, dest, dest_mode);
>          if ( target != NULL )
> -            vmsi_inj_irq(d, target, vector, trig_mode, delivery_mode);
> -        else
> -            HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "null round robin: "
> -                        "vector=%x delivery_mode=%x\n",
> -                        vector, dest_LowestPrio);
> -        break;
> -    }
> +        {
> +            vmsi_inj_irq(target, vector, trig_mode, delivery_mode);
> +            break;
> +        }
> +        HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "null MSI round robin: vector=%02x\n",
> +                    vector);
> +        return -ESRCH;
>  
>      case dest_Fixed:
> -    case dest_ExtINT:
> -    {
>          for_each_vcpu ( d, v )
>              if ( vlapic_match_dest(vcpu_vlapic(v), NULL,
>                                     0, dest, dest_mode) )
> -                vmsi_inj_irq(d, vcpu_vlapic(v),
> -                             vector, trig_mode, delivery_mode);
> +                vmsi_inj_irq(vcpu_vlapic(v), vector,
> +                             trig_mode, delivery_mode);
>          break;
> -    }
>  
> -    case dest_SMI:
> -    case dest_NMI:
> -    case dest_INIT:
> -    case dest__reserved_2:
>      default:
> -        gdprintk(XENLOG_WARNING, "Unsupported delivery mode %d\n",
> -                 delivery_mode);
> -        break;
> +        printk(XENLOG_G_WARNING
> +               "%pv: Unsupported MSI delivery mode %d for Dom%d\n",
> +               current, delivery_mode, d->domain_id);
> +        return -EINVAL;
>      }
> -    return 1;
> +    return 0;
>  }
>  
>  void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci *pirq_dpci)
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 4680 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 3/3] pt-irq fixes and improvements

[Andrew Cooper]

On 03/06/14 15:00, Jan Beulich wrote:

> Tools side:
> - don't silently ignore unrecognized PT_IRQ_TYPE_* values
> - respect that the interface type contains a union, making the code at
>   once no longer depend on the hypervisor ignoring the bus field of the
>   PCI portion of the interface structure)
>
> Hypervisor side:
> - don't ignore the PCI bus number passed in
> - don't store values (gsi, link) calculated from other stored values
> - avoid calling xfree() with a spin lock held where easily possible
> - have pt_irq_destroy_bind() respect the passed in type
> - scope reduction and constification of various variables
> - use switch instead of if/else-if chains
> - formatting
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>   with a few
suggestions for further cleanup...

> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -1702,15 +1702,20 @@ int xc_domain_bind_pt_irq(
>      bind->hvm_domid = domid;
>      bind->irq_type = irq_type;
>      bind->machine_irq = machine_irq;
> -    if ( irq_type == PT_IRQ_TYPE_PCI ||
> -         irq_type == PT_IRQ_TYPE_MSI_TRANSLATE )
> +    switch ( irq_type )
>      {
> +    case PT_IRQ_TYPE_PCI:
> +    case PT_IRQ_TYPE_MSI_TRANSLATE:
>          bind->u.pci.bus = bus;
>          bind->u.pci.device = device;    

Given all this other cleanup, can you nuke the trailing whitespace here...

>          bind->u.pci.intx = intx;
> -    } 
> -    else if ( irq_type == PT_IRQ_TYPE_ISA )
> +    case PT_IRQ_TYPE_ISA:
>          bind->u.isa.isa_irq = isa_irq;
> +        break;
> +    default:
> +        errno = EINVAL;
> +        return -1;
> +    }
>      

... and here.

>      rc = do_domctl(xch, &domctl);
>      return rc;
> @@ -1737,10 +1742,21 @@ int xc_domain_unbind_pt_irq(
>      bind->hvm_domid = domid;
>      bind->irq_type = irq_type;
>      bind->machine_irq = machine_irq;
> -    bind->u.pci.bus = bus;
> -    bind->u.pci.device = device;    
> -    bind->u.pci.intx = intx;
> -    bind->u.isa.isa_irq = isa_irq;
> +    switch ( irq_type )
> +    {
> +    case PT_IRQ_TYPE_PCI:
> +    case PT_IRQ_TYPE_MSI_TRANSLATE:
> +        bind->u.pci.bus = bus;
> +        bind->u.pci.device = device;
> +        bind->u.pci.intx = intx;
> +        break;
> +    case PT_IRQ_TYPE_ISA:
> +        bind->u.isa.isa_irq = isa_irq;
> +        break;
> +    default:
> +        errno = EINVAL;
> +        return -1;
> +    }
>      

And here in this function.

> @@ -96,13 +93,9 @@ void free_hvm_irq_dpci(struct hvm_irq_dp
>  int pt_irq_create_bind(
>      struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind)
>  {
> -    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
> +    struct hvm_irq_dpci *hvm_irq_dpci;
>      struct hvm_pirq_dpci *pirq_dpci;
>      struct pirq *info;
> -    uint32_t guest_gsi;
> -    uint32_t device, intx, link;
> -    struct dev_intx_gsi_link *digl;
> -    struct hvm_girq_dpci_mapping *girq;
>      int rc, pirq = pt_irq_bind->machine_irq;
>  
>      if ( pirq < 0 || pirq >= d->nr_pirqs )

Given all the cleanup, there is a further piece which could be done
between these two hunks.

There is an int i (declared in a for loop initialiser, despite our
fairly consistent C89 style), which should be unsigned.

> @@ -136,7 +129,9 @@ int pt_irq_create_bind(
>      }
>      pirq_dpci = pirq_dpci(info);
>  
> -    if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI )
> +    switch ( pt_irq_bind->irq_type )
> +    {
> +    case PT_IRQ_TYPE_MSI:
>      {
>          uint8_t dest, dest_mode;
>          int dest_vcpu_id;
>

[PATCH v2 3/3] pt-irq fixes and improvements

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 18436 bytes --]

Tools side:
- don't silently ignore unrecognized PT_IRQ_TYPE_* values
- respect that the interface type contains a union, making the code at
  once no longer depend on the hypervisor ignoring the bus field of the
  PCI portion of the interface structure)

Hypervisor side:
- don't ignore the PCI bus number passed in
- don't store values (gsi, link) calculated from other stored values
- avoid calling xfree() with a spin lock held where easily possible
- have pt_irq_destroy_bind() respect the passed in type
- scope reduction and constification of various variables
- use switch instead of if/else-if chains
- formatting

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v2: A few extra coding style corrections according to Andrew's
    suggestions.

--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1702,16 +1702,21 @@ int xc_domain_bind_pt_irq(
     bind->hvm_domid = domid;
     bind->irq_type = irq_type;
     bind->machine_irq = machine_irq;
-    if ( irq_type == PT_IRQ_TYPE_PCI ||
-         irq_type == PT_IRQ_TYPE_MSI_TRANSLATE )
+    switch ( irq_type )
     {
+    case PT_IRQ_TYPE_PCI:
+    case PT_IRQ_TYPE_MSI_TRANSLATE:
         bind->u.pci.bus = bus;
-        bind->u.pci.device = device;    
+        bind->u.pci.device = device;
         bind->u.pci.intx = intx;
-    } 
-    else if ( irq_type == PT_IRQ_TYPE_ISA )
+    case PT_IRQ_TYPE_ISA:
         bind->u.isa.isa_irq = isa_irq;
-    
+        break;
+    default:
+        errno = EINVAL;
+        return -1;
+    }
+
     rc = do_domctl(xch, &domctl);
     return rc;
 }
@@ -1737,11 +1742,22 @@ int xc_domain_unbind_pt_irq(
     bind->hvm_domid = domid;
     bind->irq_type = irq_type;
     bind->machine_irq = machine_irq;
-    bind->u.pci.bus = bus;
-    bind->u.pci.device = device;    
-    bind->u.pci.intx = intx;
-    bind->u.isa.isa_irq = isa_irq;
-    
+    switch ( irq_type )
+    {
+    case PT_IRQ_TYPE_PCI:
+    case PT_IRQ_TYPE_MSI_TRANSLATE:
+        bind->u.pci.bus = bus;
+        bind->u.pci.device = device;
+        bind->u.pci.intx = intx;
+        break;
+    case PT_IRQ_TYPE_ISA:
+        bind->u.isa.isa_irq = isa_irq;
+        break;
+    default:
+        errno = EINVAL;
+        return -1;
+    }
+
     rc = do_domctl(xch, &domctl);
     return rc;
 }
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -37,9 +37,8 @@ static int physdev_hvm_map_pirq(
     switch ( type )
     {
     case MAP_PIRQ_TYPE_GSI: {
-        struct hvm_irq_dpci *hvm_irq_dpci;
-        struct hvm_girq_dpci_mapping *girq;
-        uint32_t machine_gsi = 0;
+        const struct hvm_irq_dpci *hvm_irq_dpci;
+        unsigned int machine_gsi = 0;
 
         if ( *index < 0 || *index >= NR_HVM_IRQS )
         {
@@ -52,6 +51,8 @@ static int physdev_hvm_map_pirq(
         hvm_irq_dpci = domain_get_irq_dpci(d);
         if ( hvm_irq_dpci )
         {
+            const struct hvm_girq_dpci_mapping *girq;
+
             BUILD_BUG_ON(ARRAY_SIZE(hvm_irq_dpci->girq) < NR_HVM_IRQS);
             list_for_each_entry ( girq,
                                   &hvm_irq_dpci->girq[*index],
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -51,11 +51,8 @@ static int pt_irq_guest_eoi(struct domai
 static void pt_irq_time_out(void *data)
 {
     struct hvm_pirq_dpci *irq_map = data;
-    unsigned int guest_gsi;
-    struct hvm_irq_dpci *dpci = NULL;
-    struct dev_intx_gsi_link *digl;
-    struct hvm_girq_dpci_mapping *girq;
-    uint32_t device, intx;
+    const struct hvm_irq_dpci *dpci;
+    const struct dev_intx_gsi_link *digl;
 
     spin_lock(&irq_map->dom->event_lock);
 
@@ -63,16 +60,16 @@ static void pt_irq_time_out(void *data)
     ASSERT(dpci);
     list_for_each_entry ( digl, &irq_map->digl_list, list )
     {
-        guest_gsi = digl->gsi;
+        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;
         }
-        device = digl->device;
-        intx = digl->intx;
-        hvm_pci_intx_deassert(irq_map->dom, device, intx);
+        hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx);
     }
 
     pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL);
@@ -96,13 +93,9 @@ void free_hvm_irq_dpci(struct hvm_irq_dp
 int pt_irq_create_bind(
     struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind)
 {
-    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
+    struct hvm_irq_dpci *hvm_irq_dpci;
     struct hvm_pirq_dpci *pirq_dpci;
     struct pirq *info;
-    uint32_t guest_gsi;
-    uint32_t device, intx, link;
-    struct dev_intx_gsi_link *digl;
-    struct hvm_girq_dpci_mapping *girq;
     int rc, pirq = pt_irq_bind->machine_irq;
 
     if ( pirq < 0 || pirq >= d->nr_pirqs )
@@ -113,6 +106,8 @@ int pt_irq_create_bind(
     hvm_irq_dpci = domain_get_irq_dpci(d);
     if ( hvm_irq_dpci == NULL )
     {
+        unsigned int i;
+
         hvm_irq_dpci = xzalloc(struct hvm_irq_dpci);
         if ( hvm_irq_dpci == NULL )
         {
@@ -122,7 +117,7 @@ int pt_irq_create_bind(
         softirq_tasklet_init(
             &hvm_irq_dpci->dirq_tasklet,
             hvm_dirq_assist, (unsigned long)d);
-        for ( int i = 0; i < NR_HVM_IRQS; i++ )
+        for ( i = 0; i < NR_HVM_IRQS; i++ )
             INIT_LIST_HEAD(&hvm_irq_dpci->girq[i]);
 
         d->arch.hvm_domain.irq.dpci = hvm_irq_dpci;
@@ -136,7 +131,9 @@ int pt_irq_create_bind(
     }
     pirq_dpci = pirq_dpci(info);
 
-    if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI )
+    switch ( pt_irq_bind->irq_type )
+    {
+    case PT_IRQ_TYPE_MSI:
     {
         uint8_t dest, dest_mode;
         int dest_vcpu_id;
@@ -169,15 +166,16 @@ int pt_irq_create_bind(
         {
             uint32_t mask = HVM_IRQ_DPCI_MACH_MSI | HVM_IRQ_DPCI_GUEST_MSI;
 
-            if ( (pirq_dpci->flags & mask) != mask)
+            if ( (pirq_dpci->flags & mask) != mask )
             {
-	            spin_unlock(&d->event_lock);
-        	    return -EBUSY;
+                spin_unlock(&d->event_lock);
+                return -EBUSY;
             }
 
-            /* if pirq is already mapped as vmsi, update the guest data/addr */
+            /* If pirq is already mapped as vmsi, update guest data/addr. */
             if ( pirq_dpci->gmsi.gvec != pt_irq_bind->u.msi.gvec ||
-                 pirq_dpci->gmsi.gflags != pt_irq_bind->u.msi.gflags) {
+                 pirq_dpci->gmsi.gflags != pt_irq_bind->u.msi.gflags )
+            {
                 /* Directly clear pending EOIs before enabling new MSI info. */
                 pirq_guest_eoi(info);
 
@@ -185,7 +183,7 @@ int pt_irq_create_bind(
                 pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags;
             }
         }
-        /* Caculate dest_vcpu_id for MSI-type pirq migration */
+        /* Calculate dest_vcpu_id for MSI-type pirq migration. */
         dest = pirq_dpci->gmsi.gflags & VMSI_DEST_ID_MASK;
         dest_mode = !!(pirq_dpci->gmsi.gflags & VMSI_DM_MASK);
         dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
@@ -193,36 +191,37 @@ int pt_irq_create_bind(
         spin_unlock(&d->event_lock);
         if ( dest_vcpu_id >= 0 )
             hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]);
+        break;
     }
-    else
+
+    case PT_IRQ_TYPE_PCI:
+    case PT_IRQ_TYPE_MSI_TRANSLATE:
     {
-        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);
-        hvm_irq_dpci->link_cnt[link]++;
+        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);
+        struct hvm_girq_dpci_mapping *girq =
+            xmalloc(struct hvm_girq_dpci_mapping);
 
-        digl = xmalloc(struct dev_intx_gsi_link);
-        if ( !digl )
+        if ( !digl || !girq )
         {
             spin_unlock(&d->event_lock);
-            return -ENOMEM;
-        }
-
-        girq = xmalloc(struct hvm_girq_dpci_mapping);
-        if ( !girq )
-        {
+            xfree(girq);
             xfree(digl);
-            spin_unlock(&d->event_lock);
             return -ENOMEM;
         }
 
+        hvm_irq_dpci->link_cnt[link]++;
+
+        digl->bus = bus;
         digl->device = device;
         digl->intx = intx;
-        digl->gsi = guest_gsi;
-        digl->link = link;
         list_add_tail(&digl->list, &pirq_dpci->digl_list);
 
+        girq->bus = bus;
         girq->device = device;
         girq->intx = intx;
         girq->machine_gsi = pirq;
@@ -261,12 +260,12 @@ int pt_irq_create_bind(
                     kill_timer(&pirq_dpci->timer);
                 pirq_dpci->dom = NULL;
                 list_del(&girq->list);
-                xfree(girq);
                 list_del(&digl->list);
                 hvm_irq_dpci->link_cnt[link]--;
                 pirq_dpci->flags = 0;
                 pirq_cleanup_check(info, d);
                 spin_unlock(&d->event_lock);
+                xfree(girq);
                 xfree(digl);
                 return rc;
             }
@@ -276,33 +275,51 @@ int pt_irq_create_bind(
 
         if ( iommu_verbose )
             dprintk(XENLOG_G_INFO,
-                    "d%d: bind: m_gsi=%u g_gsi=%u device=%u intx=%u\n",
-                    d->domain_id, pirq, guest_gsi, device, intx);
+                    "d%d: bind: m_gsi=%u g_gsi=%u dev=%02x.%02x.%u intx=%u\n",
+                    d->domain_id, pirq, guest_gsi, bus,
+                    PCI_SLOT(device), PCI_FUNC(device), intx);
+        break;
     }
+
+    default:
+        spin_unlock(&d->event_lock);
+        return -EOPNOTSUPP;
+    }
+
     return 0;
 }
 
 int pt_irq_destroy_bind(
     struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind)
 {
-    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
+    struct hvm_irq_dpci *hvm_irq_dpci;
     struct hvm_pirq_dpci *pirq_dpci;
-    uint32_t machine_gsi, guest_gsi;
-    uint32_t device, intx, link;
+    unsigned int machine_gsi = pt_irq_bind->machine_irq;
+    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, *tmp;
     struct hvm_girq_dpci_mapping *girq;
     struct pirq *pirq;
 
-    machine_gsi = pt_irq_bind->machine_irq;
-    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);
+    switch ( pt_irq_bind->irq_type )
+    {
+    case PT_IRQ_TYPE_PCI:
+    case PT_IRQ_TYPE_MSI_TRANSLATE:
+        break;
+    case PT_IRQ_TYPE_MSI:
+        return 0;
+    default:
+        return -EOPNOTSUPP;
+    }
 
     if ( iommu_verbose )
         dprintk(XENLOG_G_INFO,
-                "d%d: unbind: m_gsi=%u g_gsi=%u device=%u intx=%u\n",
-                d->domain_id, machine_gsi, guest_gsi, device, intx);
+                "d%d: unbind: m_gsi=%u g_gsi=%u dev=%02x:%02x.%u intx=%u\n",
+                d->domain_id, machine_gsi, guest_gsi, bus,
+                PCI_SLOT(device), PCI_FUNC(device), intx);
 
     spin_lock(&d->event_lock);
 
@@ -314,18 +331,28 @@ int pt_irq_destroy_bind(
         return -EINVAL;
     }
 
-    hvm_irq_dpci->link_cnt[link]--;
-
     list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list )
     {
-        if ( girq->machine_gsi == machine_gsi )
+        if ( girq->bus         == bus &&
+             girq->device      == device &&
+             girq->intx        == intx &&
+             girq->machine_gsi == machine_gsi )
         {
-                list_del(&girq->list);
-                xfree(girq);
-                break;
+            list_del(&girq->list);
+            xfree(girq);
+            girq = NULL;
+            break;
         }
     }
 
+    if ( girq )
+    {
+        spin_unlock(&d->event_lock);
+        return -EINVAL;
+    }
+
+    hvm_irq_dpci->link_cnt[link]--;
+
     pirq = pirq_info(d, machine_gsi);
     pirq_dpci = pirq_dpci(pirq);
 
@@ -334,10 +361,9 @@ int pt_irq_destroy_bind(
     {
         list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
         {
-            if ( digl->device == device &&
-                 digl->intx   == intx &&
-                 digl->link   == link &&
-                 digl->gsi    == guest_gsi )
+            if ( digl->bus    == bus &&
+                 digl->device == device &&
+                 digl->intx   == intx )
             {
                 list_del(&digl->list);
                 xfree(digl);
@@ -359,8 +385,9 @@ int pt_irq_destroy_bind(
 
     if ( iommu_verbose )
         dprintk(XENLOG_G_INFO,
-                "d%d unmap: m_irq=%u device=%u intx=%u\n",
-                d->domain_id, machine_gsi, device, intx);
+                "d%d unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n",
+                d->domain_id, machine_gsi, bus,
+                PCI_SLOT(device), PCI_FUNC(device), intx);
 
     return 0;
 }
@@ -481,11 +508,10 @@ static void hvm_pci_msi_assert(
 static int _hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci,
                             void *arg)
 {
-    uint32_t device, intx;
-    struct dev_intx_gsi_link *digl;
-
     if ( test_and_clear_bool(pirq_dpci->masked) )
     {
+        const struct dev_intx_gsi_link *digl;
+
         if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI )
         {
             hvm_pci_msi_assert(d, pirq_dpci);
@@ -496,12 +522,10 @@ static int _hvm_dirq_assist(struct domai
         {
             struct pirq *info = dpci_pirq(pirq_dpci);
 
-            device = digl->device;
-            intx = digl->intx;
             if ( hvm_domain_use_pirq(d, info) )
                 send_guest_pirq(d, info);
             else
-                hvm_pci_intx_assert(d, device, intx);
+                hvm_pci_intx_assert(d, digl->device, digl->intx);
             pirq_dpci->pending++;
 
             if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE )
@@ -537,16 +561,13 @@ static void hvm_dirq_assist(unsigned lon
 }
 
 static void __hvm_dpci_eoi(struct domain *d,
-                           struct hvm_girq_dpci_mapping *girq,
-                           union vioapic_redir_entry *ent)
+                           const struct hvm_girq_dpci_mapping *girq,
+                           const union vioapic_redir_entry *ent)
 {
-    uint32_t device, intx;
     struct pirq *pirq;
     struct hvm_pirq_dpci *pirq_dpci;
 
-    device = girq->device;
-    intx = girq->intx;
-    hvm_pci_intx_deassert(d, device, intx);
+    hvm_pci_intx_deassert(d, girq->device, girq->intx);
 
     pirq = pirq_info(d, girq->machine_gsi);
     pirq_dpci = pirq_dpci(pirq);
@@ -556,8 +577,8 @@ static void __hvm_dpci_eoi(struct domain
      * since interrupt is still not EOIed
      */
     if ( --pirq_dpci->pending ||
-         ( ent && ent->fields.mask ) ||
-         ! pt_irq_need_timer(pirq_dpci->flags) )
+         (ent && ent->fields.mask) ||
+         !pt_irq_need_timer(pirq_dpci->flags) )
         return;
 
     stop_timer(&pirq_dpci->timer);
@@ -565,10 +586,10 @@ static void __hvm_dpci_eoi(struct domain
 }
 
 void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi,
-                  union vioapic_redir_entry *ent)
+                  const union vioapic_redir_entry *ent)
 {
-    struct hvm_irq_dpci *hvm_irq_dpci;
-    struct hvm_girq_dpci_mapping *girq;
+    const struct hvm_irq_dpci *hvm_irq_dpci;
+    const struct hvm_girq_dpci_mapping *girq;
 
     if ( !iommu_enabled )
         return;
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -69,11 +69,13 @@ static int _hvm_dpci_isairq_eoi(struct d
 {
     struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
     unsigned int isairq = (long)arg;
-    struct dev_intx_gsi_link *digl, *tmp;
+    const struct dev_intx_gsi_link *digl;
 
-    list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
+    list_for_each_entry ( digl, &pirq_dpci->digl_list, list )
     {
-        if ( hvm_irq->pci_link.route[digl->link] == isairq )
+        unsigned int link = hvm_pci_intx_link(digl->device, digl->intx);
+
+        if ( hvm_irq->pci_link.route[link] == isairq )
         {
             hvm_pci_intx_deassert(d, digl->device, digl->intx);
             if ( --pirq_dpci->pending == 0 )
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -123,7 +123,7 @@ int handle_pio(uint16_t port, unsigned i
 void hvm_interrupt_post(struct vcpu *v, int vector, int type);
 void hvm_io_assist(ioreq_t *p);
 void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq,
-                  union vioapic_redir_entry *ent);
+                  const union vioapic_redir_entry *ent);
 void msix_write_completion(struct vcpu *);
 
 struct hvm_hw_stdvga {
--- a/xen/include/xen/hvm/irq.h
+++ b/xen/include/xen/hvm/irq.h
@@ -30,10 +30,9 @@
 
 struct dev_intx_gsi_link {
     struct list_head list;
+    uint8_t bus;
     uint8_t device;
     uint8_t intx;
-    uint8_t gsi;
-    uint8_t link;
 };
 
 #define _HVM_IRQ_DPCI_MACH_PCI_SHIFT            0
@@ -69,6 +68,7 @@ struct hvm_gmsi_info {
 
 struct hvm_girq_dpci_mapping {
     struct list_head list;
+    uint8_t bus;
     uint8_t device;
     uint8_t intx;
     uint8_t machine_gsi;



[-- Attachment #2: pt-irq-binding.patch --]
[-- Type: text/plain, Size: 18465 bytes --]

pt-irq fixes and improvements

Tools side:
- don't silently ignore unrecognized PT_IRQ_TYPE_* values
- respect that the interface type contains a union, making the code at
  once no longer depend on the hypervisor ignoring the bus field of the
  PCI portion of the interface structure)

Hypervisor side:
- don't ignore the PCI bus number passed in
- don't store values (gsi, link) calculated from other stored values
- avoid calling xfree() with a spin lock held where easily possible
- have pt_irq_destroy_bind() respect the passed in type
- scope reduction and constification of various variables
- use switch instead of if/else-if chains
- formatting

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v2: A few extra coding style corrections according to Andrew's
    suggestions.

--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1702,16 +1702,21 @@ int xc_domain_bind_pt_irq(
     bind->hvm_domid = domid;
     bind->irq_type = irq_type;
     bind->machine_irq = machine_irq;
-    if ( irq_type == PT_IRQ_TYPE_PCI ||
-         irq_type == PT_IRQ_TYPE_MSI_TRANSLATE )
+    switch ( irq_type )
     {
+    case PT_IRQ_TYPE_PCI:
+    case PT_IRQ_TYPE_MSI_TRANSLATE:
         bind->u.pci.bus = bus;
-        bind->u.pci.device = device;    
+        bind->u.pci.device = device;
         bind->u.pci.intx = intx;
-    } 
-    else if ( irq_type == PT_IRQ_TYPE_ISA )
+    case PT_IRQ_TYPE_ISA:
         bind->u.isa.isa_irq = isa_irq;
-    
+        break;
+    default:
+        errno = EINVAL;
+        return -1;
+    }
+
     rc = do_domctl(xch, &domctl);
     return rc;
 }
@@ -1737,11 +1742,22 @@ int xc_domain_unbind_pt_irq(
     bind->hvm_domid = domid;
     bind->irq_type = irq_type;
     bind->machine_irq = machine_irq;
-    bind->u.pci.bus = bus;
-    bind->u.pci.device = device;    
-    bind->u.pci.intx = intx;
-    bind->u.isa.isa_irq = isa_irq;
-    
+    switch ( irq_type )
+    {
+    case PT_IRQ_TYPE_PCI:
+    case PT_IRQ_TYPE_MSI_TRANSLATE:
+        bind->u.pci.bus = bus;
+        bind->u.pci.device = device;
+        bind->u.pci.intx = intx;
+        break;
+    case PT_IRQ_TYPE_ISA:
+        bind->u.isa.isa_irq = isa_irq;
+        break;
+    default:
+        errno = EINVAL;
+        return -1;
+    }
+
     rc = do_domctl(xch, &domctl);
     return rc;
 }
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -37,9 +37,8 @@ static int physdev_hvm_map_pirq(
     switch ( type )
     {
     case MAP_PIRQ_TYPE_GSI: {
-        struct hvm_irq_dpci *hvm_irq_dpci;
-        struct hvm_girq_dpci_mapping *girq;
-        uint32_t machine_gsi = 0;
+        const struct hvm_irq_dpci *hvm_irq_dpci;
+        unsigned int machine_gsi = 0;
 
         if ( *index < 0 || *index >= NR_HVM_IRQS )
         {
@@ -52,6 +51,8 @@ static int physdev_hvm_map_pirq(
         hvm_irq_dpci = domain_get_irq_dpci(d);
         if ( hvm_irq_dpci )
         {
+            const struct hvm_girq_dpci_mapping *girq;
+
             BUILD_BUG_ON(ARRAY_SIZE(hvm_irq_dpci->girq) < NR_HVM_IRQS);
             list_for_each_entry ( girq,
                                   &hvm_irq_dpci->girq[*index],
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -51,11 +51,8 @@ static int pt_irq_guest_eoi(struct domai
 static void pt_irq_time_out(void *data)
 {
     struct hvm_pirq_dpci *irq_map = data;
-    unsigned int guest_gsi;
-    struct hvm_irq_dpci *dpci = NULL;
-    struct dev_intx_gsi_link *digl;
-    struct hvm_girq_dpci_mapping *girq;
-    uint32_t device, intx;
+    const struct hvm_irq_dpci *dpci;
+    const struct dev_intx_gsi_link *digl;
 
     spin_lock(&irq_map->dom->event_lock);
 
@@ -63,16 +60,16 @@ static void pt_irq_time_out(void *data)
     ASSERT(dpci);
     list_for_each_entry ( digl, &irq_map->digl_list, list )
     {
-        guest_gsi = digl->gsi;
+        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;
         }
-        device = digl->device;
-        intx = digl->intx;
-        hvm_pci_intx_deassert(irq_map->dom, device, intx);
+        hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx);
     }
 
     pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL);
@@ -96,13 +93,9 @@ void free_hvm_irq_dpci(struct hvm_irq_dp
 int pt_irq_create_bind(
     struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind)
 {
-    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
+    struct hvm_irq_dpci *hvm_irq_dpci;
     struct hvm_pirq_dpci *pirq_dpci;
     struct pirq *info;
-    uint32_t guest_gsi;
-    uint32_t device, intx, link;
-    struct dev_intx_gsi_link *digl;
-    struct hvm_girq_dpci_mapping *girq;
     int rc, pirq = pt_irq_bind->machine_irq;
 
     if ( pirq < 0 || pirq >= d->nr_pirqs )
@@ -113,6 +106,8 @@ int pt_irq_create_bind(
     hvm_irq_dpci = domain_get_irq_dpci(d);
     if ( hvm_irq_dpci == NULL )
     {
+        unsigned int i;
+
         hvm_irq_dpci = xzalloc(struct hvm_irq_dpci);
         if ( hvm_irq_dpci == NULL )
         {
@@ -122,7 +117,7 @@ int pt_irq_create_bind(
         softirq_tasklet_init(
             &hvm_irq_dpci->dirq_tasklet,
             hvm_dirq_assist, (unsigned long)d);
-        for ( int i = 0; i < NR_HVM_IRQS; i++ )
+        for ( i = 0; i < NR_HVM_IRQS; i++ )
             INIT_LIST_HEAD(&hvm_irq_dpci->girq[i]);
 
         d->arch.hvm_domain.irq.dpci = hvm_irq_dpci;
@@ -136,7 +131,9 @@ int pt_irq_create_bind(
     }
     pirq_dpci = pirq_dpci(info);
 
-    if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI )
+    switch ( pt_irq_bind->irq_type )
+    {
+    case PT_IRQ_TYPE_MSI:
     {
         uint8_t dest, dest_mode;
         int dest_vcpu_id;
@@ -169,15 +166,16 @@ int pt_irq_create_bind(
         {
             uint32_t mask = HVM_IRQ_DPCI_MACH_MSI | HVM_IRQ_DPCI_GUEST_MSI;
 
-            if ( (pirq_dpci->flags & mask) != mask)
+            if ( (pirq_dpci->flags & mask) != mask )
             {
-	            spin_unlock(&d->event_lock);
-        	    return -EBUSY;
+                spin_unlock(&d->event_lock);
+                return -EBUSY;
             }
 
-            /* if pirq is already mapped as vmsi, update the guest data/addr */
+            /* If pirq is already mapped as vmsi, update guest data/addr. */
             if ( pirq_dpci->gmsi.gvec != pt_irq_bind->u.msi.gvec ||
-                 pirq_dpci->gmsi.gflags != pt_irq_bind->u.msi.gflags) {
+                 pirq_dpci->gmsi.gflags != pt_irq_bind->u.msi.gflags )
+            {
                 /* Directly clear pending EOIs before enabling new MSI info. */
                 pirq_guest_eoi(info);
 
@@ -185,7 +183,7 @@ int pt_irq_create_bind(
                 pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags;
             }
         }
-        /* Caculate dest_vcpu_id for MSI-type pirq migration */
+        /* Calculate dest_vcpu_id for MSI-type pirq migration. */
         dest = pirq_dpci->gmsi.gflags & VMSI_DEST_ID_MASK;
         dest_mode = !!(pirq_dpci->gmsi.gflags & VMSI_DM_MASK);
         dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
@@ -193,36 +191,37 @@ int pt_irq_create_bind(
         spin_unlock(&d->event_lock);
         if ( dest_vcpu_id >= 0 )
             hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]);
+        break;
     }
-    else
+
+    case PT_IRQ_TYPE_PCI:
+    case PT_IRQ_TYPE_MSI_TRANSLATE:
     {
-        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);
-        hvm_irq_dpci->link_cnt[link]++;
+        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);
+        struct hvm_girq_dpci_mapping *girq =
+            xmalloc(struct hvm_girq_dpci_mapping);
 
-        digl = xmalloc(struct dev_intx_gsi_link);
-        if ( !digl )
+        if ( !digl || !girq )
         {
             spin_unlock(&d->event_lock);
-            return -ENOMEM;
-        }
-
-        girq = xmalloc(struct hvm_girq_dpci_mapping);
-        if ( !girq )
-        {
+            xfree(girq);
             xfree(digl);
-            spin_unlock(&d->event_lock);
             return -ENOMEM;
         }
 
+        hvm_irq_dpci->link_cnt[link]++;
+
+        digl->bus = bus;
         digl->device = device;
         digl->intx = intx;
-        digl->gsi = guest_gsi;
-        digl->link = link;
         list_add_tail(&digl->list, &pirq_dpci->digl_list);
 
+        girq->bus = bus;
         girq->device = device;
         girq->intx = intx;
         girq->machine_gsi = pirq;
@@ -261,12 +260,12 @@ int pt_irq_create_bind(
                     kill_timer(&pirq_dpci->timer);
                 pirq_dpci->dom = NULL;
                 list_del(&girq->list);
-                xfree(girq);
                 list_del(&digl->list);
                 hvm_irq_dpci->link_cnt[link]--;
                 pirq_dpci->flags = 0;
                 pirq_cleanup_check(info, d);
                 spin_unlock(&d->event_lock);
+                xfree(girq);
                 xfree(digl);
                 return rc;
             }
@@ -276,33 +275,51 @@ int pt_irq_create_bind(
 
         if ( iommu_verbose )
             dprintk(XENLOG_G_INFO,
-                    "d%d: bind: m_gsi=%u g_gsi=%u device=%u intx=%u\n",
-                    d->domain_id, pirq, guest_gsi, device, intx);
+                    "d%d: bind: m_gsi=%u g_gsi=%u dev=%02x.%02x.%u intx=%u\n",
+                    d->domain_id, pirq, guest_gsi, bus,
+                    PCI_SLOT(device), PCI_FUNC(device), intx);
+        break;
     }
+
+    default:
+        spin_unlock(&d->event_lock);
+        return -EOPNOTSUPP;
+    }
+
     return 0;
 }
 
 int pt_irq_destroy_bind(
     struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind)
 {
-    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
+    struct hvm_irq_dpci *hvm_irq_dpci;
     struct hvm_pirq_dpci *pirq_dpci;
-    uint32_t machine_gsi, guest_gsi;
-    uint32_t device, intx, link;
+    unsigned int machine_gsi = pt_irq_bind->machine_irq;
+    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, *tmp;
     struct hvm_girq_dpci_mapping *girq;
     struct pirq *pirq;
 
-    machine_gsi = pt_irq_bind->machine_irq;
-    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);
+    switch ( pt_irq_bind->irq_type )
+    {
+    case PT_IRQ_TYPE_PCI:
+    case PT_IRQ_TYPE_MSI_TRANSLATE:
+        break;
+    case PT_IRQ_TYPE_MSI:
+        return 0;
+    default:
+        return -EOPNOTSUPP;
+    }
 
     if ( iommu_verbose )
         dprintk(XENLOG_G_INFO,
-                "d%d: unbind: m_gsi=%u g_gsi=%u device=%u intx=%u\n",
-                d->domain_id, machine_gsi, guest_gsi, device, intx);
+                "d%d: unbind: m_gsi=%u g_gsi=%u dev=%02x:%02x.%u intx=%u\n",
+                d->domain_id, machine_gsi, guest_gsi, bus,
+                PCI_SLOT(device), PCI_FUNC(device), intx);
 
     spin_lock(&d->event_lock);
 
@@ -314,18 +331,28 @@ int pt_irq_destroy_bind(
         return -EINVAL;
     }
 
-    hvm_irq_dpci->link_cnt[link]--;
-
     list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list )
     {
-        if ( girq->machine_gsi == machine_gsi )
+        if ( girq->bus         == bus &&
+             girq->device      == device &&
+             girq->intx        == intx &&
+             girq->machine_gsi == machine_gsi )
         {
-                list_del(&girq->list);
-                xfree(girq);
-                break;
+            list_del(&girq->list);
+            xfree(girq);
+            girq = NULL;
+            break;
         }
     }
 
+    if ( girq )
+    {
+        spin_unlock(&d->event_lock);
+        return -EINVAL;
+    }
+
+    hvm_irq_dpci->link_cnt[link]--;
+
     pirq = pirq_info(d, machine_gsi);
     pirq_dpci = pirq_dpci(pirq);
 
@@ -334,10 +361,9 @@ int pt_irq_destroy_bind(
     {
         list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
         {
-            if ( digl->device == device &&
-                 digl->intx   == intx &&
-                 digl->link   == link &&
-                 digl->gsi    == guest_gsi )
+            if ( digl->bus    == bus &&
+                 digl->device == device &&
+                 digl->intx   == intx )
             {
                 list_del(&digl->list);
                 xfree(digl);
@@ -359,8 +385,9 @@ int pt_irq_destroy_bind(
 
     if ( iommu_verbose )
         dprintk(XENLOG_G_INFO,
-                "d%d unmap: m_irq=%u device=%u intx=%u\n",
-                d->domain_id, machine_gsi, device, intx);
+                "d%d unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n",
+                d->domain_id, machine_gsi, bus,
+                PCI_SLOT(device), PCI_FUNC(device), intx);
 
     return 0;
 }
@@ -481,11 +508,10 @@ static void hvm_pci_msi_assert(
 static int _hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci,
                             void *arg)
 {
-    uint32_t device, intx;
-    struct dev_intx_gsi_link *digl;
-
     if ( test_and_clear_bool(pirq_dpci->masked) )
     {
+        const struct dev_intx_gsi_link *digl;
+
         if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI )
         {
             hvm_pci_msi_assert(d, pirq_dpci);
@@ -496,12 +522,10 @@ static int _hvm_dirq_assist(struct domai
         {
             struct pirq *info = dpci_pirq(pirq_dpci);
 
-            device = digl->device;
-            intx = digl->intx;
             if ( hvm_domain_use_pirq(d, info) )
                 send_guest_pirq(d, info);
             else
-                hvm_pci_intx_assert(d, device, intx);
+                hvm_pci_intx_assert(d, digl->device, digl->intx);
             pirq_dpci->pending++;
 
             if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE )
@@ -537,16 +561,13 @@ static void hvm_dirq_assist(unsigned lon
 }
 
 static void __hvm_dpci_eoi(struct domain *d,
-                           struct hvm_girq_dpci_mapping *girq,
-                           union vioapic_redir_entry *ent)
+                           const struct hvm_girq_dpci_mapping *girq,
+                           const union vioapic_redir_entry *ent)
 {
-    uint32_t device, intx;
     struct pirq *pirq;
     struct hvm_pirq_dpci *pirq_dpci;
 
-    device = girq->device;
-    intx = girq->intx;
-    hvm_pci_intx_deassert(d, device, intx);
+    hvm_pci_intx_deassert(d, girq->device, girq->intx);
 
     pirq = pirq_info(d, girq->machine_gsi);
     pirq_dpci = pirq_dpci(pirq);
@@ -556,8 +577,8 @@ static void __hvm_dpci_eoi(struct domain
      * since interrupt is still not EOIed
      */
     if ( --pirq_dpci->pending ||
-         ( ent && ent->fields.mask ) ||
-         ! pt_irq_need_timer(pirq_dpci->flags) )
+         (ent && ent->fields.mask) ||
+         !pt_irq_need_timer(pirq_dpci->flags) )
         return;
 
     stop_timer(&pirq_dpci->timer);
@@ -565,10 +586,10 @@ static void __hvm_dpci_eoi(struct domain
 }
 
 void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi,
-                  union vioapic_redir_entry *ent)
+                  const union vioapic_redir_entry *ent)
 {
-    struct hvm_irq_dpci *hvm_irq_dpci;
-    struct hvm_girq_dpci_mapping *girq;
+    const struct hvm_irq_dpci *hvm_irq_dpci;
+    const struct hvm_girq_dpci_mapping *girq;
 
     if ( !iommu_enabled )
         return;
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -69,11 +69,13 @@ static int _hvm_dpci_isairq_eoi(struct d
 {
     struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
     unsigned int isairq = (long)arg;
-    struct dev_intx_gsi_link *digl, *tmp;
+    const struct dev_intx_gsi_link *digl;
 
-    list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
+    list_for_each_entry ( digl, &pirq_dpci->digl_list, list )
     {
-        if ( hvm_irq->pci_link.route[digl->link] == isairq )
+        unsigned int link = hvm_pci_intx_link(digl->device, digl->intx);
+
+        if ( hvm_irq->pci_link.route[link] == isairq )
         {
             hvm_pci_intx_deassert(d, digl->device, digl->intx);
             if ( --pirq_dpci->pending == 0 )
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -123,7 +123,7 @@ int handle_pio(uint16_t port, unsigned i
 void hvm_interrupt_post(struct vcpu *v, int vector, int type);
 void hvm_io_assist(ioreq_t *p);
 void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq,
-                  union vioapic_redir_entry *ent);
+                  const union vioapic_redir_entry *ent);
 void msix_write_completion(struct vcpu *);
 
 struct hvm_hw_stdvga {
--- a/xen/include/xen/hvm/irq.h
+++ b/xen/include/xen/hvm/irq.h
@@ -30,10 +30,9 @@
 
 struct dev_intx_gsi_link {
     struct list_head list;
+    uint8_t bus;
     uint8_t device;
     uint8_t intx;
-    uint8_t gsi;
-    uint8_t link;
 };
 
 #define _HVM_IRQ_DPCI_MACH_PCI_SHIFT            0
@@ -69,6 +68,7 @@ struct hvm_gmsi_info {
 
 struct hvm_girq_dpci_mapping {
     struct list_head list;
+    uint8_t bus;
     uint8_t device;
     uint8_t intx;
     uint8_t machine_gsi;

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 3/3] pt-irq fixes and improvements

[Zhang, Yang Z]

Jan Beulich wrote on 2014-06-04:
> Tools side:
> - don't silently ignore unrecognized PT_IRQ_TYPE_* values
> - respect that the interface type contains a union, making the code at
>   once no longer depend on the hypervisor ignoring the bus field of the
>   PCI portion of the interface structure) Hypervisor side:
> - don't ignore the PCI bus number passed in
> - don't store values (gsi, link) calculated from other stored values
> - avoid calling xfree() with a spin lock held where easily possible
> - have pt_irq_destroy_bind() respect the passed in type
> - scope reduction and constification of various variables
> - use switch instead of if/else-if chains
> - formatting
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Yang Zhang <yang.z.zhang@intel.com>

> ---
> v2: A few extra coding style corrections according to Andrew's
>     suggestions.
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -1702,16 +1702,21 @@ int xc_domain_bind_pt_irq(
>      bind->hvm_domid = domid;
>      bind->irq_type = irq_type;
>      bind->machine_irq = machine_irq;
> -    if ( irq_type == PT_IRQ_TYPE_PCI ||
> -         irq_type == PT_IRQ_TYPE_MSI_TRANSLATE )
> +    switch ( irq_type )
>      {
> +    case PT_IRQ_TYPE_PCI:
> +    case PT_IRQ_TYPE_MSI_TRANSLATE:
>          bind->u.pci.bus = bus;
> -        bind->u.pci.device = device;
> +        bind->u.pci.device = device;
>          bind->u.pci.intx = intx;
> -    }
> -    else if ( irq_type == PT_IRQ_TYPE_ISA )
> +    case PT_IRQ_TYPE_ISA:
>          bind->u.isa.isa_irq = isa_irq;
> -
> +        break;
> +    default:
> +        errno = EINVAL;
> +        return -1;
> +    }
> +
>      rc = do_domctl(xch, &domctl);
>      return rc;
>  }
> @@ -1737,11 +1742,22 @@ int xc_domain_unbind_pt_irq(
>      bind->hvm_domid = domid;
>      bind->irq_type = irq_type;
>      bind->machine_irq = machine_irq;
> -    bind->u.pci.bus = bus;
> -    bind->u.pci.device = device;
> -    bind->u.pci.intx = intx;
> -    bind->u.isa.isa_irq = isa_irq;
> -
> +    switch ( irq_type )
> +    {
> +    case PT_IRQ_TYPE_PCI:
> +    case PT_IRQ_TYPE_MSI_TRANSLATE:
> +        bind->u.pci.bus = bus;
> +        bind->u.pci.device = device;
> +        bind->u.pci.intx = intx;
> +        break;
> +    case PT_IRQ_TYPE_ISA:
> +        bind->u.isa.isa_irq = isa_irq;
> +        break;
> +    default:
> +        errno = EINVAL;
> +        return -1;
> +    }
> +
>      rc = do_domctl(xch, &domctl);
>      return rc;
>  }
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -37,9 +37,8 @@ static int physdev_hvm_map_pirq(
>      switch ( type )
>      {
>      case MAP_PIRQ_TYPE_GSI: {
> -        struct hvm_irq_dpci *hvm_irq_dpci;
> -        struct hvm_girq_dpci_mapping *girq;
> -        uint32_t machine_gsi = 0;
> +        const struct hvm_irq_dpci *hvm_irq_dpci;
> +        unsigned int machine_gsi = 0;
> 
>          if ( *index < 0 || *index >= NR_HVM_IRQS ) { @@ -52,6 +51,8 @@
>          static int physdev_hvm_map_pirq( hvm_irq_dpci =
>          domain_get_irq_dpci(d); if ( hvm_irq_dpci ) {
> +            const struct hvm_girq_dpci_mapping *girq;
> +
>              BUILD_BUG_ON(ARRAY_SIZE(hvm_irq_dpci->girq) < NR_HVM_IRQS);
>              list_for_each_entry ( girq,
>                                    &hvm_irq_dpci->girq[*index],
> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/io.c
> @@ -51,11 +51,8 @@ static int pt_irq_guest_eoi(struct domai  static 
> void pt_irq_time_out(void *data)  {
>      struct hvm_pirq_dpci *irq_map = data;
> -    unsigned int guest_gsi;
> -    struct hvm_irq_dpci *dpci = NULL;
> -    struct dev_intx_gsi_link *digl;
> -    struct hvm_girq_dpci_mapping *girq;
> -    uint32_t device, intx;
> +    const struct hvm_irq_dpci *dpci;
> +    const struct dev_intx_gsi_link *digl;
> 
>      spin_lock(&irq_map->dom->event_lock);
> @@ -63,16 +60,16 @@ static void pt_irq_time_out(void *data)
>      ASSERT(dpci);
>      list_for_each_entry ( digl, &irq_map->digl_list, list )
>      {
> -        guest_gsi = digl->gsi;
> +        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;
>          }
> -        device = digl->device;
> -        intx = digl->intx;
> -        hvm_pci_intx_deassert(irq_map->dom, device, intx);
> +        hvm_pci_intx_deassert(irq_map->dom, digl->device,
> + digl->intx);
>      }
>      
>      pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL); @@ -96,13
> +93,9 @@ void free_hvm_irq_dpci(struct hvm_irq_dp  int 
> +pt_irq_create_bind(
>      struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind)  {
> -    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
> +    struct hvm_irq_dpci *hvm_irq_dpci;
>      struct hvm_pirq_dpci *pirq_dpci;
>      struct pirq *info;
> -    uint32_t guest_gsi;
> -    uint32_t device, intx, link;
> -    struct dev_intx_gsi_link *digl;
> -    struct hvm_girq_dpci_mapping *girq;
>      int rc, pirq = pt_irq_bind->machine_irq;
>      
>      if ( pirq < 0 || pirq >= d->nr_pirqs ) @@ -113,6 +106,8 @@ int
>      pt_irq_create_bind( hvm_irq_dpci = domain_get_irq_dpci(d); if (
>      hvm_irq_dpci == NULL ) {
> +        unsigned int i;
> +
>          hvm_irq_dpci = xzalloc(struct hvm_irq_dpci); if ( hvm_irq_dpci
>          == NULL ) { @@ -122,7 +117,7 @@ int pt_irq_create_bind(
>          softirq_tasklet_init(
>              &hvm_irq_dpci->dirq_tasklet,
>              hvm_dirq_assist, (unsigned long)d);
> -        for ( int i = 0; i < NR_HVM_IRQS; i++ )
> +        for ( i = 0; i < NR_HVM_IRQS; i++ )
>              INIT_LIST_HEAD(&hvm_irq_dpci->girq[i]);
>          d->arch.hvm_domain.irq.dpci = hvm_irq_dpci; @@ -136,7 +131,9 
> @@ int pt_irq_create_bind(
>      }
>      pirq_dpci = pirq_dpci(info);
> -    if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI )
> +    switch ( pt_irq_bind->irq_type )
> +    {
> +    case PT_IRQ_TYPE_MSI:
>      {
>          uint8_t dest, dest_mode; int dest_vcpu_id; @@ -169,15 +166,16
>          @@ int pt_irq_create_bind( {
>              uint32_t mask = HVM_IRQ_DPCI_MACH_MSI | 
> HVM_IRQ_DPCI_GUEST_MSI;
> 
> -            if ( (pirq_dpci->flags & mask) != mask)
> +            if ( (pirq_dpci->flags & mask) != mask )
>              {
> -	            spin_unlock(&d->event_lock);
> -        	    return -EBUSY;
> +                spin_unlock(&d->event_lock);
> +                return -EBUSY;
>              }
> -            /* if pirq is already mapped as vmsi, update the guest
> data/addr */
> +            /* If pirq is already mapped as vmsi, update guest 
> + data/addr. */
>              if ( pirq_dpci->gmsi.gvec != pt_irq_bind->u.msi.gvec ||
> -                 pirq_dpci->gmsi.gflags != pt_irq_bind->u.msi.gflags) {
> +                 pirq_dpci->gmsi.gflags != pt_irq_bind->u.msi.gflags )
> +            {
>                  /* Directly clear pending EOIs before enabling new MSI
>                  info. */ pirq_guest_eoi(info); @@ -185,7 +183,7 @@ 
> int pt_irq_create_bind(
>                  pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags;
>              }
>          }
> -        /* Caculate dest_vcpu_id for MSI-type pirq migration */
> +        /* Calculate dest_vcpu_id for MSI-type pirq migration. */
>          dest = pirq_dpci->gmsi.gflags & VMSI_DEST_ID_MASK; dest_mode =
>          !!(pirq_dpci->gmsi.gflags & VMSI_DM_MASK); dest_vcpu_id =
>          hvm_girq_dest_2_vcpu_id(d, dest, dest_mode); @@ -193,36 +191,37
>          @@ int pt_irq_create_bind( spin_unlock(&d->event_lock); if (
>          dest_vcpu_id >= 0 )
>              hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]);
> +        break;
>      }
> -    else
> +
> +    case PT_IRQ_TYPE_PCI:
> +    case PT_IRQ_TYPE_MSI_TRANSLATE:
>      {
> -        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);
> -        hvm_irq_dpci->link_cnt[link]++;
> +        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);
> +        struct hvm_girq_dpci_mapping *girq =
> +            xmalloc(struct hvm_girq_dpci_mapping);
> 
> -        digl = xmalloc(struct dev_intx_gsi_link);
> -        if ( !digl )
> +        if ( !digl || !girq )
>          {
>              spin_unlock(&d->event_lock);
> -            return -ENOMEM;
> -        }
> -
> -        girq = xmalloc(struct hvm_girq_dpci_mapping);
> -        if ( !girq )
> -        {
> +            xfree(girq);
>              xfree(digl); -            spin_unlock(&d->event_lock);
>              return -ENOMEM;
>          }
> +        hvm_irq_dpci->link_cnt[link]++;
> +
> +        digl->bus = bus;
>          digl->device = device;
>          digl->intx = intx;
> -        digl->gsi = guest_gsi;
> -        digl->link = link;
>          list_add_tail(&digl->list, &pirq_dpci->digl_list);
> +        girq->bus = bus;
>          girq->device = device;
>          girq->intx = intx;
>          girq->machine_gsi = pirq;
> @@ -261,12 +260,12 @@ int pt_irq_create_bind(
>                      kill_timer(&pirq_dpci->timer);
>                  pirq_dpci->dom = NULL; list_del(&girq->list); -        
>                         xfree(girq); list_del(&digl->list);
>                  hvm_irq_dpci->link_cnt[link]--; pirq_dpci->flags = 0;
>                  pirq_cleanup_check(info, d);
>                  spin_unlock(&d->event_lock); +               
>                  xfree(girq); xfree(digl); return rc;
>              }
> @@ -276,33 +275,51 @@ int pt_irq_create_bind(
> 
>          if ( iommu_verbose )
>              dprintk(XENLOG_G_INFO,
> -                    "d%d: bind: m_gsi=%u g_gsi=%u device=%u intx=%u\n",
> -                    d->domain_id, pirq, guest_gsi, device, intx); +    
>                "d%d: bind: m_gsi=%u g_gsi=%u dev=%02x.%02x.%u
> intx=%u\n", +                    d->domain_id, pirq, guest_gsi, bus, +  
>                  PCI_SLOT(device), PCI_FUNC(device), intx); +       
> break;
>      }
> +
> +    default:
> +        spin_unlock(&d->event_lock);
> +        return -EOPNOTSUPP;
> +    }
> +
>      return 0;
>  }
>  
>  int pt_irq_destroy_bind(
>      struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind)  {
> -    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
> +    struct hvm_irq_dpci *hvm_irq_dpci;
>      struct hvm_pirq_dpci *pirq_dpci;
> -    uint32_t machine_gsi, guest_gsi;
> -    uint32_t device, intx, link;
> +    unsigned int machine_gsi = pt_irq_bind->machine_irq;
> +    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, *tmp;
>      struct hvm_girq_dpci_mapping *girq;
>      struct pirq *pirq;
> -    machine_gsi = pt_irq_bind->machine_irq;
> -    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);
> +    switch ( pt_irq_bind->irq_type )
> +    {
> +    case PT_IRQ_TYPE_PCI:
> +    case PT_IRQ_TYPE_MSI_TRANSLATE:
> +        break;
> +    case PT_IRQ_TYPE_MSI:
> +        return 0;
> +    default:
> +        return -EOPNOTSUPP;
> +    }
> 
>      if ( iommu_verbose )
>          dprintk(XENLOG_G_INFO,
> -                "d%d: unbind: m_gsi=%u g_gsi=%u device=%u intx=%u\n", -
>                d->domain_id, machine_gsi, guest_gsi, device, intx); +   
>             "d%d: unbind: m_gsi=%u g_gsi=%u dev=%02x:%02x.%u 
> intx=%u\n",
> +                d->domain_id, machine_gsi, guest_gsi, bus, +           
>     PCI_SLOT(device), PCI_FUNC(device), intx);
> 
>      spin_lock(&d->event_lock);
> @@ -314,18 +331,28 @@ int pt_irq_destroy_bind(
>          return -EINVAL;
>      }
> -    hvm_irq_dpci->link_cnt[link]--;
> -
>      list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list )
>      {
> -        if ( girq->machine_gsi == machine_gsi )
> +        if ( girq->bus         == bus &&
> +             girq->device      == device &&
> +             girq->intx        == intx &&
> +             girq->machine_gsi == machine_gsi )
>          {
> -                list_del(&girq->list);
> -                xfree(girq);
> -                break;
> +            list_del(&girq->list);
> +            xfree(girq);
> +            girq = NULL;
> +            break;
>          }
>      }
> +    if ( girq )
> +    {
> +        spin_unlock(&d->event_lock);
> +        return -EINVAL;
> +    }
> +
> +    hvm_irq_dpci->link_cnt[link]--;
> +
>      pirq = pirq_info(d, machine_gsi);
>      pirq_dpci = pirq_dpci(pirq);
> @@ -334,10 +361,9 @@ int pt_irq_destroy_bind(
>      {
>          list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
>          {
> -            if ( digl->device == device &&
> -                 digl->intx   == intx &&
> -                 digl->link   == link &&
> -                 digl->gsi    == guest_gsi )
> +            if ( digl->bus    == bus &&
> +                 digl->device == device &&
> +                 digl->intx   == intx )
>              {
>                  list_del(&digl->list);
>                  xfree(digl);
> @@ -359,8 +385,9 @@ int pt_irq_destroy_bind(
> 
>      if ( iommu_verbose )
>          dprintk(XENLOG_G_INFO,
> -                "d%d unmap: m_irq=%u device=%u intx=%u\n",
> -                d->domain_id, machine_gsi, device, intx);
> +                "d%d unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n",
> +                d->domain_id, machine_gsi, bus,
> +                PCI_SLOT(device), PCI_FUNC(device), intx);
> 
>      return 0;
>  }
> @@ -481,11 +508,10 @@ static void hvm_pci_msi_assert(  static int 
> _hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci,
>                              void *arg)  {
> -    uint32_t device, intx;
> -    struct dev_intx_gsi_link *digl;
> -
>      if ( test_and_clear_bool(pirq_dpci->masked) )
>      {
> +        const struct dev_intx_gsi_link *digl;
> +
>          if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI )
>          {
>              hvm_pci_msi_assert(d, pirq_dpci); @@ -496,12 +522,10 @@ 
> static int _hvm_dirq_assist(struct domai
>          {
>              struct pirq *info = dpci_pirq(pirq_dpci);
> -            device = digl->device;
> -            intx = digl->intx;
>              if ( hvm_domain_use_pirq(d, info) )
>                  send_guest_pirq(d, info);
>              else
> -                hvm_pci_intx_assert(d, device, intx);
> +                hvm_pci_intx_assert(d, digl->device, digl->intx);
>              pirq_dpci->pending++;
>              
>              if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE ) @@
> -537,16 +561,13 @@ static void hvm_dirq_assist(unsigned lon  }
> 
>  static void __hvm_dpci_eoi(struct domain *d,
> -                           struct hvm_girq_dpci_mapping *girq,
> -                           union vioapic_redir_entry *ent)
> +                           const struct hvm_girq_dpci_mapping *girq,
> +                           const union vioapic_redir_entry *ent)
>  {
> -    uint32_t device, intx;
>      struct pirq *pirq;
>      struct hvm_pirq_dpci *pirq_dpci;
> -    device = girq->device;
> -    intx = girq->intx;
> -    hvm_pci_intx_deassert(d, device, intx);
> +    hvm_pci_intx_deassert(d, girq->device, girq->intx);
> 
>      pirq = pirq_info(d, girq->machine_gsi);
>      pirq_dpci = pirq_dpci(pirq);
> @@ -556,8 +577,8 @@ static void __hvm_dpci_eoi(struct domain
>       * since interrupt is still not EOIed
>       */
>      if ( --pirq_dpci->pending ||
> -         ( ent && ent->fields.mask ) ||
> -         ! pt_irq_need_timer(pirq_dpci->flags) )
> +         (ent && ent->fields.mask) ||
> +         !pt_irq_need_timer(pirq_dpci->flags) )
>          return;
>      stop_timer(&pirq_dpci->timer);
> @@ -565,10 +586,10 @@ static void __hvm_dpci_eoi(struct domain  }
> 
>  void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi,
> -                  union vioapic_redir_entry *ent)
> +                  const union vioapic_redir_entry *ent)
>  {
> -    struct hvm_irq_dpci *hvm_irq_dpci;
> -    struct hvm_girq_dpci_mapping *girq;
> +    const struct hvm_irq_dpci *hvm_irq_dpci;
> +    const struct hvm_girq_dpci_mapping *girq;
> 
>      if ( !iommu_enabled )
>          return;
> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
> @@ -69,11 +69,13 @@ static int _hvm_dpci_isairq_eoi(struct d  {
>      struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
>      unsigned int isairq = (long)arg;
> -    struct dev_intx_gsi_link *digl, *tmp;
> +    const struct dev_intx_gsi_link *digl;
> 
> -    list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
> +    list_for_each_entry ( digl, &pirq_dpci->digl_list, list )
>      {
> -        if ( hvm_irq->pci_link.route[digl->link] == isairq )
> +        unsigned int link = hvm_pci_intx_link(digl->device,
> + digl->intx);
> +
> +        if ( hvm_irq->pci_link.route[link] == isairq )
>          {
>              hvm_pci_intx_deassert(d, digl->device, digl->intx);
>              if ( --pirq_dpci->pending == 0 )
> --- a/xen/include/asm-x86/hvm/io.h
> +++ b/xen/include/asm-x86/hvm/io.h
> @@ -123,7 +123,7 @@ int handle_pio(uint16_t port, unsigned i  void 
> hvm_interrupt_post(struct vcpu *v, int vector, int type);  void 
> hvm_io_assist(ioreq_t *p);  void hvm_dpci_eoi(struct domain *d, 
> unsigned int guest_irq,
> -                  union vioapic_redir_entry *ent);
> +                  const union vioapic_redir_entry *ent);
>  void msix_write_completion(struct vcpu *);
>  
>  struct hvm_hw_stdvga {
> --- a/xen/include/xen/hvm/irq.h
> +++ b/xen/include/xen/hvm/irq.h
> @@ -30,10 +30,9 @@
> 
>  struct dev_intx_gsi_link {
>      struct list_head list; +    uint8_t bus; uint8_t device; uint8_t
>      intx;
> -    uint8_t gsi;
> -    uint8_t link;
>  };
>  
>  #define _HVM_IRQ_DPCI_MACH_PCI_SHIFT            0
> @@ -69,6 +68,7 @@ struct hvm_gmsi_info {
> 
>  struct hvm_girq_dpci_mapping {
>      struct list_head list; +    uint8_t bus; uint8_t device; uint8_t
>      intx; uint8_t machine_gsi;


Best regards,
Yang

Ping [tools]: [PATCH v2 3/3] pt-irq fixes and improvements

[Jan Beulich]

>>> On 04.06.14 at 10:24, <JBeulich@suse.com> wrote:
> Tools side:
> - don't silently ignore unrecognized PT_IRQ_TYPE_* values
> - respect that the interface type contains a union, making the code at
>   once no longer depend on the hypervisor ignoring the bus field of the
>   PCI portion of the interface structure)

This part is still awaiting a tools maintainer's opinion.

Thanks, Jan

> Hypervisor side:
> - don't ignore the PCI bus number passed in
> - don't store values (gsi, link) calculated from other stored values
> - avoid calling xfree() with a spin lock held where easily possible
> - have pt_irq_destroy_bind() respect the passed in type
> - scope reduction and constification of various variables
> - use switch instead of if/else-if chains
> - formatting
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> v2: A few extra coding style corrections according to Andrew's
>     suggestions.
> 
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -1702,16 +1702,21 @@ int xc_domain_bind_pt_irq(
>      bind->hvm_domid = domid;
>      bind->irq_type = irq_type;
>      bind->machine_irq = machine_irq;
> -    if ( irq_type == PT_IRQ_TYPE_PCI ||
> -         irq_type == PT_IRQ_TYPE_MSI_TRANSLATE )
> +    switch ( irq_type )
>      {
> +    case PT_IRQ_TYPE_PCI:
> +    case PT_IRQ_TYPE_MSI_TRANSLATE:
>          bind->u.pci.bus = bus;
> -        bind->u.pci.device = device;    
> +        bind->u.pci.device = device;
>          bind->u.pci.intx = intx;
> -    } 
> -    else if ( irq_type == PT_IRQ_TYPE_ISA )
> +    case PT_IRQ_TYPE_ISA:
>          bind->u.isa.isa_irq = isa_irq;
> -    
> +        break;
> +    default:
> +        errno = EINVAL;
> +        return -1;
> +    }
> +
>      rc = do_domctl(xch, &domctl);
>      return rc;
>  }
> @@ -1737,11 +1742,22 @@ int xc_domain_unbind_pt_irq(
>      bind->hvm_domid = domid;
>      bind->irq_type = irq_type;
>      bind->machine_irq = machine_irq;
> -    bind->u.pci.bus = bus;
> -    bind->u.pci.device = device;    
> -    bind->u.pci.intx = intx;
> -    bind->u.isa.isa_irq = isa_irq;
> -    
> +    switch ( irq_type )
> +    {
> +    case PT_IRQ_TYPE_PCI:
> +    case PT_IRQ_TYPE_MSI_TRANSLATE:
> +        bind->u.pci.bus = bus;
> +        bind->u.pci.device = device;
> +        bind->u.pci.intx = intx;
> +        break;
> +    case PT_IRQ_TYPE_ISA:
> +        bind->u.isa.isa_irq = isa_irq;
> +        break;
> +    default:
> +        errno = EINVAL;
> +        return -1;
> +    }
> +
>      rc = do_domctl(xch, &domctl);
>      return rc;
>  }
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -37,9 +37,8 @@ static int physdev_hvm_map_pirq(
>      switch ( type )
>      {
>      case MAP_PIRQ_TYPE_GSI: {
> -        struct hvm_irq_dpci *hvm_irq_dpci;
> -        struct hvm_girq_dpci_mapping *girq;
> -        uint32_t machine_gsi = 0;
> +        const struct hvm_irq_dpci *hvm_irq_dpci;
> +        unsigned int machine_gsi = 0;
>  
>          if ( *index < 0 || *index >= NR_HVM_IRQS )
>          {
> @@ -52,6 +51,8 @@ static int physdev_hvm_map_pirq(
>          hvm_irq_dpci = domain_get_irq_dpci(d);
>          if ( hvm_irq_dpci )
>          {
> +            const struct hvm_girq_dpci_mapping *girq;
> +
>              BUILD_BUG_ON(ARRAY_SIZE(hvm_irq_dpci->girq) < NR_HVM_IRQS);
>              list_for_each_entry ( girq,
>                                    &hvm_irq_dpci->girq[*index],
> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/io.c
> @@ -51,11 +51,8 @@ static int pt_irq_guest_eoi(struct domai
>  static void pt_irq_time_out(void *data)
>  {
>      struct hvm_pirq_dpci *irq_map = data;
> -    unsigned int guest_gsi;
> -    struct hvm_irq_dpci *dpci = NULL;
> -    struct dev_intx_gsi_link *digl;
> -    struct hvm_girq_dpci_mapping *girq;
> -    uint32_t device, intx;
> +    const struct hvm_irq_dpci *dpci;
> +    const struct dev_intx_gsi_link *digl;
>  
>      spin_lock(&irq_map->dom->event_lock);
>  
> @@ -63,16 +60,16 @@ static void pt_irq_time_out(void *data)
>      ASSERT(dpci);
>      list_for_each_entry ( digl, &irq_map->digl_list, list )
>      {
> -        guest_gsi = digl->gsi;
> +        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;
>          }
> -        device = digl->device;
> -        intx = digl->intx;
> -        hvm_pci_intx_deassert(irq_map->dom, device, intx);
> +        hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx);
>      }
>  
>      pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL);
> @@ -96,13 +93,9 @@ void free_hvm_irq_dpci(struct hvm_irq_dp
>  int pt_irq_create_bind(
>      struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind)
>  {
> -    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
> +    struct hvm_irq_dpci *hvm_irq_dpci;
>      struct hvm_pirq_dpci *pirq_dpci;
>      struct pirq *info;
> -    uint32_t guest_gsi;
> -    uint32_t device, intx, link;
> -    struct dev_intx_gsi_link *digl;
> -    struct hvm_girq_dpci_mapping *girq;
>      int rc, pirq = pt_irq_bind->machine_irq;
>  
>      if ( pirq < 0 || pirq >= d->nr_pirqs )
> @@ -113,6 +106,8 @@ int pt_irq_create_bind(
>      hvm_irq_dpci = domain_get_irq_dpci(d);
>      if ( hvm_irq_dpci == NULL )
>      {
> +        unsigned int i;
> +
>          hvm_irq_dpci = xzalloc(struct hvm_irq_dpci);
>          if ( hvm_irq_dpci == NULL )
>          {
> @@ -122,7 +117,7 @@ int pt_irq_create_bind(
>          softirq_tasklet_init(
>              &hvm_irq_dpci->dirq_tasklet,
>              hvm_dirq_assist, (unsigned long)d);
> -        for ( int i = 0; i < NR_HVM_IRQS; i++ )
> +        for ( i = 0; i < NR_HVM_IRQS; i++ )
>              INIT_LIST_HEAD(&hvm_irq_dpci->girq[i]);
>  
>          d->arch.hvm_domain.irq.dpci = hvm_irq_dpci;
> @@ -136,7 +131,9 @@ int pt_irq_create_bind(
>      }
>      pirq_dpci = pirq_dpci(info);
>  
> -    if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI )
> +    switch ( pt_irq_bind->irq_type )
> +    {
> +    case PT_IRQ_TYPE_MSI:
>      {
>          uint8_t dest, dest_mode;
>          int dest_vcpu_id;
> @@ -169,15 +166,16 @@ int pt_irq_create_bind(
>          {
>              uint32_t mask = HVM_IRQ_DPCI_MACH_MSI | HVM_IRQ_DPCI_GUEST_MSI;
>  
> -            if ( (pirq_dpci->flags & mask) != mask)
> +            if ( (pirq_dpci->flags & mask) != mask )
>              {
> -	            spin_unlock(&d->event_lock);
> -        	    return -EBUSY;
> +                spin_unlock(&d->event_lock);
> +                return -EBUSY;
>              }
>  
> -            /* if pirq is already mapped as vmsi, update the guest data/addr 
> */
> +            /* If pirq is already mapped as vmsi, update guest data/addr. 
> */
>              if ( pirq_dpci->gmsi.gvec != pt_irq_bind->u.msi.gvec ||
> -                 pirq_dpci->gmsi.gflags != pt_irq_bind->u.msi.gflags) {
> +                 pirq_dpci->gmsi.gflags != pt_irq_bind->u.msi.gflags )
> +            {
>                  /* Directly clear pending EOIs before enabling new MSI 
> info. */
>                  pirq_guest_eoi(info);
>  
> @@ -185,7 +183,7 @@ int pt_irq_create_bind(
>                  pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags;
>              }
>          }
> -        /* Caculate dest_vcpu_id for MSI-type pirq migration */
> +        /* Calculate dest_vcpu_id for MSI-type pirq migration. */
>          dest = pirq_dpci->gmsi.gflags & VMSI_DEST_ID_MASK;
>          dest_mode = !!(pirq_dpci->gmsi.gflags & VMSI_DM_MASK);
>          dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
> @@ -193,36 +191,37 @@ int pt_irq_create_bind(
>          spin_unlock(&d->event_lock);
>          if ( dest_vcpu_id >= 0 )
>              hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]);
> +        break;
>      }
> -    else
> +
> +    case PT_IRQ_TYPE_PCI:
> +    case PT_IRQ_TYPE_MSI_TRANSLATE:
>      {
> -        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);
> -        hvm_irq_dpci->link_cnt[link]++;
> +        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);
> +        struct hvm_girq_dpci_mapping *girq =
> +            xmalloc(struct hvm_girq_dpci_mapping);
>  
> -        digl = xmalloc(struct dev_intx_gsi_link);
> -        if ( !digl )
> +        if ( !digl || !girq )
>          {
>              spin_unlock(&d->event_lock);
> -            return -ENOMEM;
> -        }
> -
> -        girq = xmalloc(struct hvm_girq_dpci_mapping);
> -        if ( !girq )
> -        {
> +            xfree(girq);
>              xfree(digl);
> -            spin_unlock(&d->event_lock);
>              return -ENOMEM;
>          }
>  
> +        hvm_irq_dpci->link_cnt[link]++;
> +
> +        digl->bus = bus;
>          digl->device = device;
>          digl->intx = intx;
> -        digl->gsi = guest_gsi;
> -        digl->link = link;
>          list_add_tail(&digl->list, &pirq_dpci->digl_list);
>  
> +        girq->bus = bus;
>          girq->device = device;
>          girq->intx = intx;
>          girq->machine_gsi = pirq;
> @@ -261,12 +260,12 @@ int pt_irq_create_bind(
>                      kill_timer(&pirq_dpci->timer);
>                  pirq_dpci->dom = NULL;
>                  list_del(&girq->list);
> -                xfree(girq);
>                  list_del(&digl->list);
>                  hvm_irq_dpci->link_cnt[link]--;
>                  pirq_dpci->flags = 0;
>                  pirq_cleanup_check(info, d);
>                  spin_unlock(&d->event_lock);
> +                xfree(girq);
>                  xfree(digl);
>                  return rc;
>              }
> @@ -276,33 +275,51 @@ int pt_irq_create_bind(
>  
>          if ( iommu_verbose )
>              dprintk(XENLOG_G_INFO,
> -                    "d%d: bind: m_gsi=%u g_gsi=%u device=%u intx=%u\n",
> -                    d->domain_id, pirq, guest_gsi, device, intx);
> +                    "d%d: bind: m_gsi=%u g_gsi=%u dev=%02x.%02x.%u 
> intx=%u\n",
> +                    d->domain_id, pirq, guest_gsi, bus,
> +                    PCI_SLOT(device), PCI_FUNC(device), intx);
> +        break;
>      }
> +
> +    default:
> +        spin_unlock(&d->event_lock);
> +        return -EOPNOTSUPP;
> +    }
> +
>      return 0;
>  }
>  
>  int pt_irq_destroy_bind(
>      struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind)
>  {
> -    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
> +    struct hvm_irq_dpci *hvm_irq_dpci;
>      struct hvm_pirq_dpci *pirq_dpci;
> -    uint32_t machine_gsi, guest_gsi;
> -    uint32_t device, intx, link;
> +    unsigned int machine_gsi = pt_irq_bind->machine_irq;
> +    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, *tmp;
>      struct hvm_girq_dpci_mapping *girq;
>      struct pirq *pirq;
>  
> -    machine_gsi = pt_irq_bind->machine_irq;
> -    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);
> +    switch ( pt_irq_bind->irq_type )
> +    {
> +    case PT_IRQ_TYPE_PCI:
> +    case PT_IRQ_TYPE_MSI_TRANSLATE:
> +        break;
> +    case PT_IRQ_TYPE_MSI:
> +        return 0;
> +    default:
> +        return -EOPNOTSUPP;
> +    }
>  
>      if ( iommu_verbose )
>          dprintk(XENLOG_G_INFO,
> -                "d%d: unbind: m_gsi=%u g_gsi=%u device=%u intx=%u\n",
> -                d->domain_id, machine_gsi, guest_gsi, device, intx);
> +                "d%d: unbind: m_gsi=%u g_gsi=%u dev=%02x:%02x.%u 
> intx=%u\n",
> +                d->domain_id, machine_gsi, guest_gsi, bus,
> +                PCI_SLOT(device), PCI_FUNC(device), intx);
>  
>      spin_lock(&d->event_lock);
>  
> @@ -314,18 +331,28 @@ int pt_irq_destroy_bind(
>          return -EINVAL;
>      }
>  
> -    hvm_irq_dpci->link_cnt[link]--;
> -
>      list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list )
>      {
> -        if ( girq->machine_gsi == machine_gsi )
> +        if ( girq->bus         == bus &&
> +             girq->device      == device &&
> +             girq->intx        == intx &&
> +             girq->machine_gsi == machine_gsi )
>          {
> -                list_del(&girq->list);
> -                xfree(girq);
> -                break;
> +            list_del(&girq->list);
> +            xfree(girq);
> +            girq = NULL;
> +            break;
>          }
>      }
>  
> +    if ( girq )
> +    {
> +        spin_unlock(&d->event_lock);
> +        return -EINVAL;
> +    }
> +
> +    hvm_irq_dpci->link_cnt[link]--;
> +
>      pirq = pirq_info(d, machine_gsi);
>      pirq_dpci = pirq_dpci(pirq);
>  
> @@ -334,10 +361,9 @@ int pt_irq_destroy_bind(
>      {
>          list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
>          {
> -            if ( digl->device == device &&
> -                 digl->intx   == intx &&
> -                 digl->link   == link &&
> -                 digl->gsi    == guest_gsi )
> +            if ( digl->bus    == bus &&
> +                 digl->device == device &&
> +                 digl->intx   == intx )
>              {
>                  list_del(&digl->list);
>                  xfree(digl);
> @@ -359,8 +385,9 @@ int pt_irq_destroy_bind(
>  
>      if ( iommu_verbose )
>          dprintk(XENLOG_G_INFO,
> -                "d%d unmap: m_irq=%u device=%u intx=%u\n",
> -                d->domain_id, machine_gsi, device, intx);
> +                "d%d unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n",
> +                d->domain_id, machine_gsi, bus,
> +                PCI_SLOT(device), PCI_FUNC(device), intx);
>  
>      return 0;
>  }
> @@ -481,11 +508,10 @@ static void hvm_pci_msi_assert(
>  static int _hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci 
> *pirq_dpci,
>                              void *arg)
>  {
> -    uint32_t device, intx;
> -    struct dev_intx_gsi_link *digl;
> -
>      if ( test_and_clear_bool(pirq_dpci->masked) )
>      {
> +        const struct dev_intx_gsi_link *digl;
> +
>          if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI )
>          {
>              hvm_pci_msi_assert(d, pirq_dpci);
> @@ -496,12 +522,10 @@ static int _hvm_dirq_assist(struct domai
>          {
>              struct pirq *info = dpci_pirq(pirq_dpci);
>  
> -            device = digl->device;
> -            intx = digl->intx;
>              if ( hvm_domain_use_pirq(d, info) )
>                  send_guest_pirq(d, info);
>              else
> -                hvm_pci_intx_assert(d, device, intx);
> +                hvm_pci_intx_assert(d, digl->device, digl->intx);
>              pirq_dpci->pending++;
>  
>              if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE )
> @@ -537,16 +561,13 @@ static void hvm_dirq_assist(unsigned lon
>  }
>  
>  static void __hvm_dpci_eoi(struct domain *d,
> -                           struct hvm_girq_dpci_mapping *girq,
> -                           union vioapic_redir_entry *ent)
> +                           const struct hvm_girq_dpci_mapping *girq,
> +                           const union vioapic_redir_entry *ent)
>  {
> -    uint32_t device, intx;
>      struct pirq *pirq;
>      struct hvm_pirq_dpci *pirq_dpci;
>  
> -    device = girq->device;
> -    intx = girq->intx;
> -    hvm_pci_intx_deassert(d, device, intx);
> +    hvm_pci_intx_deassert(d, girq->device, girq->intx);
>  
>      pirq = pirq_info(d, girq->machine_gsi);
>      pirq_dpci = pirq_dpci(pirq);
> @@ -556,8 +577,8 @@ static void __hvm_dpci_eoi(struct domain
>       * since interrupt is still not EOIed
>       */
>      if ( --pirq_dpci->pending ||
> -         ( ent && ent->fields.mask ) ||
> -         ! pt_irq_need_timer(pirq_dpci->flags) )
> +         (ent && ent->fields.mask) ||
> +         !pt_irq_need_timer(pirq_dpci->flags) )
>          return;
>  
>      stop_timer(&pirq_dpci->timer);
> @@ -565,10 +586,10 @@ static void __hvm_dpci_eoi(struct domain
>  }
>  
>  void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi,
> -                  union vioapic_redir_entry *ent)
> +                  const union vioapic_redir_entry *ent)
>  {
> -    struct hvm_irq_dpci *hvm_irq_dpci;
> -    struct hvm_girq_dpci_mapping *girq;
> +    const struct hvm_irq_dpci *hvm_irq_dpci;
> +    const struct hvm_girq_dpci_mapping *girq;
>  
>      if ( !iommu_enabled )
>          return;
> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
> @@ -69,11 +69,13 @@ static int _hvm_dpci_isairq_eoi(struct d
>  {
>      struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
>      unsigned int isairq = (long)arg;
> -    struct dev_intx_gsi_link *digl, *tmp;
> +    const struct dev_intx_gsi_link *digl;
>  
> -    list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
> +    list_for_each_entry ( digl, &pirq_dpci->digl_list, list )
>      {
> -        if ( hvm_irq->pci_link.route[digl->link] == isairq )
> +        unsigned int link = hvm_pci_intx_link(digl->device, digl->intx);
> +
> +        if ( hvm_irq->pci_link.route[link] == isairq )
>          {
>              hvm_pci_intx_deassert(d, digl->device, digl->intx);
>              if ( --pirq_dpci->pending == 0 )
> --- a/xen/include/asm-x86/hvm/io.h
> +++ b/xen/include/asm-x86/hvm/io.h
> @@ -123,7 +123,7 @@ int handle_pio(uint16_t port, unsigned i
>  void hvm_interrupt_post(struct vcpu *v, int vector, int type);
>  void hvm_io_assist(ioreq_t *p);
>  void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq,
> -                  union vioapic_redir_entry *ent);
> +                  const union vioapic_redir_entry *ent);
>  void msix_write_completion(struct vcpu *);
>  
>  struct hvm_hw_stdvga {
> --- a/xen/include/xen/hvm/irq.h
> +++ b/xen/include/xen/hvm/irq.h
> @@ -30,10 +30,9 @@
>  
>  struct dev_intx_gsi_link {
>      struct list_head list;
> +    uint8_t bus;
>      uint8_t device;
>      uint8_t intx;
> -    uint8_t gsi;
> -    uint8_t link;
>  };
>  
>  #define _HVM_IRQ_DPCI_MACH_PCI_SHIFT            0
> @@ -69,6 +68,7 @@ struct hvm_gmsi_info {
>  
>  struct hvm_girq_dpci_mapping {
>      struct list_head list;
> +    uint8_t bus;
>      uint8_t device;
>      uint8_t intx;
>      uint8_t machine_gsi;

Re: Ping [tools]: [PATCH v2 3/3] pt-irq fixes and improvements

[Ian Campbell]

On Wed, 2014-06-11 at 13:32 +0100, Jan Beulich wrote:
> >>> On 04.06.14 at 10:24, <JBeulich@suse.com> wrote:
> > Tools side:
> > - don't silently ignore unrecognized PT_IRQ_TYPE_* values
> > - respect that the interface type contains a union, making the code at
> >   once no longer depend on the hypervisor ignoring the bus field of the
> >   PCI portion of the interface structure)
> 
> This part is still awaiting a tools maintainer's opinion.

Sorry, I missed that there was a tools bit here. I've moved it into my
queue folder and will look at it ASAP.

Ian.

Re: [PATCH v2 3/3] pt-irq fixes and improvements

[Ian Campbell]

On Wed, 2014-06-04 at 09:24 +0100, Jan Beulich wrote:
> Tools side:
> - don't silently ignore unrecognized PT_IRQ_TYPE_* values
> - respect that the interface type contains a union, making the code at
>   once no longer depend on the hypervisor ignoring the bus field of the
>   PCI portion of the interface structure)
> 
> Hypervisor side:
> - don't ignore the PCI bus number passed in
> - don't store values (gsi, link) calculated from other stored values
> - avoid calling xfree() with a spin lock held where easily possible
> - have pt_irq_destroy_bind() respect the passed in type
> - scope reduction and constification of various variables
> - use switch instead of if/else-if chains
> - formatting
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Toosl:
Acked-by: Ian Campbell <ian.campbell@citrix.com>