* [PATCH v3 for-next 0/3] x86/dpci: bind legacy PCI interrupts to PVHv2 Dom0
@ 2017-05-17 15:15 Roger Pau Monne
2017-05-17 15:15 ` [PATCH v3 for-next 1/3] x86/physdev: factor out the code to allocate and map a pirq Roger Pau Monne
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Roger Pau Monne @ 2017-05-17 15:15 UTC (permalink / raw)
To: xen-devel; +Cc: boris.ostrovsky
Hello,
The following patches allow binding bare-metal GSIs into a PVHv2 Dom0, by
snooping on the vIO APICs writes made by Dom0.
First two patches introduce the necessary code to bind GSIs into a PVH Dom0,
and patch 3 snoops on vIO APIC writes for unmask and binds the GSI to Dom0.
The series has been tested with a PVHv2 Dom0 on a box with 3 IO APICs, although
all devices are wired up into the first IO APIC sadly.
A branch with the changes can be found at:
git://xenbits.xen.org/people/royger/xen.git dom0_gsi_v3
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 for-next 1/3] x86/physdev: factor out the code to allocate and map a pirq
2017-05-17 15:15 [PATCH v3 for-next 0/3] x86/dpci: bind legacy PCI interrupts to PVHv2 Dom0 Roger Pau Monne
@ 2017-05-17 15:15 ` Roger Pau Monne
2017-05-30 9:16 ` Jan Beulich
2017-05-17 15:15 ` [PATCH v3 for-next 2/3] x86/pt: enable binding of GSIs to a PVH Dom0 Roger Pau Monne
2017-05-17 15:15 ` [PATCH v3 for-next 3/3] x86/vioapic: bind interrupts to " Roger Pau Monne
2 siblings, 1 reply; 12+ messages in thread
From: Roger Pau Monne @ 2017-05-17 15:15 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, boris.ostrovsky, Roger Pau Monne, Jan Beulich
Move the code to allocate and map a domain pirq (either GSI or MSI) into the
x86 irq code base, so that it can be used outside of the physdev ops.
This change shouldn't affect the functionality of the already existing physdev
ops.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v2:
- Factor out the code to allocate the pirq.
- Fix coding style issues.
- Do not take the pci lock to bind a GSI.
- Pass a type parameter to the MSI bind function.
Changes since v1:
- New in this version.
---
xen/arch/x86/irq.c | 159 ++++++++++++++++++++++++++++++++++++++++++++++
xen/arch/x86/physdev.c | 124 ++----------------------------------
xen/include/asm-x86/irq.h | 5 ++
3 files changed, 169 insertions(+), 119 deletions(-)
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 676ba5216f..4590e85303 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2537,3 +2537,162 @@ bool_t hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
return is_hvm_domain(d) && pirq &&
pirq->arch.hvm.emuirq != IRQ_UNBOUND;
}
+
+static int allocate_pirq(struct domain *d, int pirq, int irq, int type,
+ int *nr)
+{
+ int current_pirq;
+
+ ASSERT(spin_is_locked(&d->event_lock));
+ current_pirq = domain_irq_to_pirq(d, irq);
+ if ( pirq < 0 )
+ {
+ if ( current_pirq )
+ {
+ dprintk(XENLOG_G_ERR, "dom%d: %d:%d already mapped to %d\n",
+ d->domain_id, irq, pirq, current_pirq);
+ if ( current_pirq < 0 )
+ return -EBUSY;
+ }
+ else if ( type == MAP_PIRQ_TYPE_MULTI_MSI )
+ {
+ if ( *nr <= 0 || *nr > 32 )
+ return -EDOM;
+ else if ( *nr != 1 && !iommu_intremap )
+ return -EOPNOTSUPP;
+ else
+ {
+ while ( *nr & (*nr - 1) )
+ *nr += *nr & -*nr;
+ pirq = get_free_pirqs(d, *nr);
+ if ( pirq < 0 )
+ {
+ while ( (*nr >>= 1) > 1 )
+ if ( get_free_pirqs(d, *nr) > 0 )
+ break;
+ dprintk(XENLOG_G_ERR, "dom%d: no block of %d free pirqs\n",
+ d->domain_id, *nr << 1);
+ }
+ }
+ }
+ else
+ {
+ pirq = get_free_pirq(d, type);
+ if ( pirq < 0 )
+ dprintk(XENLOG_G_ERR, "dom%d: no free pirq\n", d->domain_id);
+ }
+ }
+ else if ( current_pirq && pirq != current_pirq )
+ {
+ dprintk(XENLOG_G_ERR, "dom%d: irq %d already mapped to pirq %d\n",
+ d->domain_id, irq, current_pirq);
+ pirq = -EEXIST;
+ }
+
+ return pirq;
+}
+
+int allocate_and_map_gsi_pirq(struct domain *d, int *index, int *pirq_p)
+{
+ int irq, pirq, ret;
+
+ if ( *index < 0 || *index >= nr_irqs_gsi )
+ {
+ dprintk(XENLOG_G_ERR, "dom%d: map invalid irq %d\n", d->domain_id,
+ *index);
+ return -EINVAL;
+ }
+
+ irq = domain_pirq_to_irq(current->domain, *index);
+ if ( irq <= 0 )
+ {
+ if ( is_hardware_domain(current->domain) )
+ irq = *index;
+ else
+ {
+ dprintk(XENLOG_G_ERR, "dom%d: map pirq with incorrect irq!\n",
+ d->domain_id);
+ return -EINVAL;
+ }
+ }
+
+ /* Verify or get pirq. */
+ spin_lock(&d->event_lock);
+ pirq = allocate_pirq(d, *pirq_p, irq, MAP_PIRQ_TYPE_GSI, 0);
+ if ( pirq < 0 )
+ {
+ ret = pirq;
+ goto done;
+ }
+
+ ret = map_domain_pirq(d, pirq, irq, MAP_PIRQ_TYPE_GSI, NULL);
+ if ( ret == 0 )
+ *pirq_p = pirq;
+
+ done:
+ spin_unlock(&d->event_lock);
+ return ret;
+}
+
+int allocate_and_map_msi_pirq(struct domain *d, int *index, int *pirq_p,
+ int type, struct msi_info *msi)
+{
+ int irq, pirq, ret;
+
+ switch ( type )
+ {
+ case MAP_PIRQ_TYPE_MSI:
+ if ( !msi->table_base )
+ msi->entry_nr = 1;
+ irq = *index;
+ if ( irq == -1 )
+ case MAP_PIRQ_TYPE_MULTI_MSI:
+ irq = create_irq(NUMA_NO_NODE);
+
+ if ( irq < nr_irqs_gsi || irq >= nr_irqs )
+ {
+ dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n",
+ d->domain_id);
+ return -EINVAL;
+ }
+
+ msi->irq = irq;
+ break;
+
+ default:
+ dprintk(XENLOG_G_ERR, "dom%d: wrong pirq type %x\n",
+ d->domain_id, type);
+ return -EINVAL;
+ }
+
+ msi->irq = irq;
+
+ pcidevs_lock();
+ /* Verify or get pirq. */
+ spin_lock(&d->event_lock);
+ pirq = allocate_pirq(d, *pirq_p, irq, type, &msi->entry_nr);
+ if ( pirq < 0 )
+ {
+ ret = pirq;
+ goto done;
+ }
+
+ ret = map_domain_pirq(d, pirq, irq, type, msi);
+ if ( ret == 0 )
+ *pirq_p = pirq;
+
+ done:
+ spin_unlock(&d->event_lock);
+ pcidevs_unlock();
+ if ( ret != 0 )
+ switch ( type )
+ {
+ case MAP_PIRQ_TYPE_MSI:
+ if ( *index == -1 )
+ case MAP_PIRQ_TYPE_MULTI_MSI:
+ destroy_irq(irq);
+ break;
+ }
+ return ret;
+}
+
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index eec4a41231..e99fd9a35f 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -92,8 +92,7 @@ int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p,
struct msi_info *msi)
{
struct domain *d = current->domain;
- int pirq, irq, ret = 0;
- void *map_data = NULL;
+ int ret;
if ( domid == DOMID_SELF && is_hvm_domain(d) && has_pirq(d) )
{
@@ -119,135 +118,22 @@ int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p,
switch ( type )
{
case MAP_PIRQ_TYPE_GSI:
- if ( *index < 0 || *index >= nr_irqs_gsi )
- {
- dprintk(XENLOG_G_ERR, "dom%d: map invalid irq %d\n",
- d->domain_id, *index);
- ret = -EINVAL;
- goto free_domain;
- }
-
- irq = domain_pirq_to_irq(current->domain, *index);
- if ( irq <= 0 )
- {
- if ( is_hardware_domain(current->domain) )
- irq = *index;
- else {
- dprintk(XENLOG_G_ERR, "dom%d: map pirq with incorrect irq!\n",
- d->domain_id);
- ret = -EINVAL;
- goto free_domain;
- }
- }
+ ret = allocate_and_map_gsi_pirq(d, index, pirq_p);
break;
-
case MAP_PIRQ_TYPE_MSI:
if ( !msi->table_base )
msi->entry_nr = 1;
- irq = *index;
- if ( irq == -1 )
+ /* fallthrough */
case MAP_PIRQ_TYPE_MULTI_MSI:
- irq = create_irq(NUMA_NO_NODE);
-
- if ( irq < nr_irqs_gsi || irq >= nr_irqs )
- {
- dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n",
- d->domain_id);
- ret = -EINVAL;
- goto free_domain;
- }
-
- msi->irq = irq;
- map_data = msi;
+ ret = allocate_and_map_msi_pirq(d, index, pirq_p, type, msi);
break;
-
default:
dprintk(XENLOG_G_ERR, "dom%d: wrong map_pirq type %x\n",
d->domain_id, type);
ret = -EINVAL;
- goto free_domain;
- }
-
- pcidevs_lock();
- /* Verify or get pirq. */
- spin_lock(&d->event_lock);
- pirq = domain_irq_to_pirq(d, irq);
- if ( *pirq_p < 0 )
- {
- if ( pirq )
- {
- dprintk(XENLOG_G_ERR, "dom%d: %d:%d already mapped to %d\n",
- d->domain_id, *index, *pirq_p, pirq);
- if ( pirq < 0 )
- {
- ret = -EBUSY;
- goto done;
- }
- }
- else if ( type == MAP_PIRQ_TYPE_MULTI_MSI )
- {
- if ( msi->entry_nr <= 0 || msi->entry_nr > 32 )
- ret = -EDOM;
- else if ( msi->entry_nr != 1 && !iommu_intremap )
- ret = -EOPNOTSUPP;
- else
- {
- while ( msi->entry_nr & (msi->entry_nr - 1) )
- msi->entry_nr += msi->entry_nr & -msi->entry_nr;
- pirq = get_free_pirqs(d, msi->entry_nr);
- if ( pirq < 0 )
- {
- while ( (msi->entry_nr >>= 1) > 1 )
- if ( get_free_pirqs(d, msi->entry_nr) > 0 )
- break;
- dprintk(XENLOG_G_ERR, "dom%d: no block of %d free pirqs\n",
- d->domain_id, msi->entry_nr << 1);
- ret = pirq;
- }
- }
- if ( ret < 0 )
- goto done;
- }
- else
- {
- pirq = get_free_pirq(d, type);
- if ( pirq < 0 )
- {
- dprintk(XENLOG_G_ERR, "dom%d: no free pirq\n", d->domain_id);
- ret = pirq;
- goto done;
- }
- }
- }
- else
- {
- if ( pirq && pirq != *pirq_p )
- {
- dprintk(XENLOG_G_ERR, "dom%d: pirq %d conflicts with irq %d\n",
- d->domain_id, *index, *pirq_p);
- ret = -EEXIST;
- goto done;
- }
- else
- pirq = *pirq_p;
+ break;
}
- ret = map_domain_pirq(d, pirq, irq, type, map_data);
- if ( ret == 0 )
- *pirq_p = pirq;
-
- done:
- spin_unlock(&d->event_lock);
- pcidevs_unlock();
- if ( ret != 0 )
- switch ( type )
- {
- case MAP_PIRQ_TYPE_MSI:
- if ( *index == -1 )
- case MAP_PIRQ_TYPE_MULTI_MSI:
- destroy_irq(irq);
- break;
- }
free_domain:
rcu_unlock_domain(d);
return ret;
diff --git a/xen/include/asm-x86/irq.h b/xen/include/asm-x86/irq.h
index ef625ebb13..d8d965b642 100644
--- a/xen/include/asm-x86/irq.h
+++ b/xen/include/asm-x86/irq.h
@@ -200,4 +200,9 @@ bool_t cpu_has_pending_apic_eoi(void);
static inline void arch_move_irqs(struct vcpu *v) { }
+struct msi_info;
+int allocate_and_map_gsi_pirq(struct domain *d, int *index, int *pirq_p);
+int allocate_and_map_msi_pirq(struct domain *d, int *index, int *pirq_p,
+ int type, struct msi_info *msi);
+
#endif /* _ASM_HW_IRQ_H */
--
2.11.0 (Apple Git-81)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 for-next 2/3] x86/pt: enable binding of GSIs to a PVH Dom0
2017-05-17 15:15 [PATCH v3 for-next 0/3] x86/dpci: bind legacy PCI interrupts to PVHv2 Dom0 Roger Pau Monne
2017-05-17 15:15 ` [PATCH v3 for-next 1/3] x86/physdev: factor out the code to allocate and map a pirq Roger Pau Monne
@ 2017-05-17 15:15 ` Roger Pau Monne
2017-05-30 10:01 ` Jan Beulich
2017-05-17 15:15 ` [PATCH v3 for-next 3/3] x86/vioapic: bind interrupts to " Roger Pau Monne
2 siblings, 1 reply; 12+ messages in thread
From: Roger Pau Monne @ 2017-05-17 15:15 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, boris.ostrovsky, Roger Pau Monne, Jan Beulich
Achieve this by expanding pt_irq_create_bind in order to support mapping
interrupts of type PT_IRQ_TYPE_PCI to a PVH Dom0. GSIs bound to Dom0 are always
identity bound, which means the all the fields inside of the u.pci sub-struct
are ignored, and only the machine_irq is actually used in order to determine
which GSI the caller wants to bind.
Also, the hvm_irq_dpci struct is not used by a PVH Dom0, since that's used to
route interrupts and allow different host to guest GSI mappings, which is not
used by a PVH Dom0.
This requires adding some specific handlers for such directly mapped GSIs,
which bypass the PCI interrupt routing done by Xen for HVM guests.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v2:
- Turn the assert in hvm_gsi_{de}assert into an assert_unreachable (like it's
done in __hvm_pci_intx_{de}assert.
- Do not increase/decrease gsi_assert_count, instead set it to 1/0.
- Fix a comment grammar error.
- Convert the pt_irq_create_bind asserts for bind type and pirq range into an
error path.
- Reduce the size of the message buffers, 24 should be enough.
- Allow pt_irq_create_bind to unbind hardware domain GSIs.
- s/__hvm_pirq_eoi/hvm_pirq_eoi/.
- Remove ASSERT(pirq_dpci) from hvm_pirq_eoi.
- Remove pirq_dpci local variable from hvm_gsi_eoi (it's used only once).
- s/__hvm_gsi_eoi/hvm_gsi_eoi/.
- Add a comment to document hvm_gsi_assert usage of gsi_assert_count.
Changes since v1:
- Remove the PT_IRQ_TYPE_GSI and instead just use PT_IRQ_TYPE_PCI with a
hardware domain special casing.
- Check the trigger mode of the Dom0 vIO APIC in order to set the shareable
flags in pt_irq_create_bind.
---
xen/arch/x86/hvm/irq.c | 43 ++++++++
xen/drivers/passthrough/io.c | 242 +++++++++++++++++++++++++++++++------------
xen/include/xen/hvm/irq.h | 6 ++
3 files changed, 225 insertions(+), 66 deletions(-)
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 86255847a6..6d40d1f94f 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -126,6 +126,49 @@ void hvm_pci_intx_deassert(
spin_unlock(&d->arch.hvm_domain.irq_lock);
}
+void hvm_gsi_assert(struct domain *d, unsigned int gsi)
+{
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
+
+ if ( gsi >= hvm_irq->nr_gsis )
+ {
+ ASSERT_UNREACHABLE();
+ return;
+ }
+
+ /*
+ * __hvm_pci_intx_{de}assert uses an array to track the status of each
+ * interrupt line, and Xen does the routing and GSI assertion based on
+ * that. This prevents the same line from triggering multiple times, which
+ * is not available here, and thus Xen needs to rely on gsi_assert_count in
+ * order to know if the GSI is pending or not.
+ */
+ spin_lock(&d->arch.hvm_domain.irq_lock);
+ if ( !hvm_irq->gsi_assert_count[gsi] )
+ {
+ hvm_irq->gsi_assert_count[gsi] = 1;
+ assert_gsi(d, gsi);
+ }
+ spin_unlock(&d->arch.hvm_domain.irq_lock);
+}
+
+void hvm_gsi_deassert(struct domain *d, unsigned int gsi)
+{
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
+
+ if ( gsi >= hvm_irq->nr_gsis )
+ {
+ ASSERT_UNREACHABLE();
+ return;
+ }
+
+ spin_lock(&d->arch.hvm_domain.irq_lock);
+ if ( hvm_irq->gsi_assert_count[gsi] )
+ hvm_irq->gsi_assert_count[gsi] = 0;
+ ASSERT(!hvm_irq->gsi_assert_count[gsi]);
+ spin_unlock(&d->arch.hvm_domain.irq_lock);
+}
+
void hvm_isa_irq_assert(
struct domain *d, unsigned int isa_irq)
{
diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index e5a43e508f..3654288b74 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -164,6 +164,20 @@ static void pt_irq_time_out(void *data)
spin_lock(&irq_map->dom->event_lock);
+ if ( irq_map->flags & HVM_IRQ_DPCI_IDENTITY_GSI )
+ {
+ struct pirq *pirq = dpci_pirq(irq_map);
+
+ ASSERT(is_hardware_domain(irq_map->dom));
+ /*
+ * Identity mapped, no need to iterate over the guest GSI list to find
+ * other pirqs sharing the same guest GSI.
+ */
+ irq_map->flags |= HVM_IRQ_DPCI_EOI_LATCH;
+ hvm_gsi_deassert(irq_map->dom, pirq->pirq);
+ goto out;
+ }
+
dpci = domain_get_irq_dpci(irq_map->dom);
if ( unlikely(!dpci) )
{
@@ -185,6 +199,7 @@ static void pt_irq_time_out(void *data)
hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx);
}
+ out:
pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL);
spin_unlock(&irq_map->dom->event_lock);
@@ -274,10 +289,16 @@ int pt_irq_create_bind(
spin_lock(&d->event_lock);
hvm_irq_dpci = domain_get_irq_dpci(d);
- if ( hvm_irq_dpci == NULL )
+ if ( hvm_irq_dpci == NULL && !is_hardware_domain(d) )
{
unsigned int i;
+ /*
+ * NB: the hardware domain doesn't use a hvm_irq_dpci struct because
+ * it's only allowed to identity map GSIs, and so the data contained in
+ * that struct (used to map guest GSIs into machine GSIs and perform
+ * interrupt routing) it's completely useless to it.
+ */
hvm_irq_dpci = xzalloc(struct hvm_irq_dpci);
if ( hvm_irq_dpci == NULL )
{
@@ -422,35 +443,52 @@ int pt_irq_create_bind(
case PT_IRQ_TYPE_PCI:
case PT_IRQ_TYPE_MSI_TRANSLATE:
{
- unsigned int bus = pt_irq_bind->u.pci.bus;
- unsigned int device = pt_irq_bind->u.pci.device;
- unsigned int intx = pt_irq_bind->u.pci.intx;
- unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx);
- unsigned int link = hvm_pci_intx_link(device, intx);
- struct dev_intx_gsi_link *digl = xmalloc(struct dev_intx_gsi_link);
- struct hvm_girq_dpci_mapping *girq =
- xmalloc(struct hvm_girq_dpci_mapping);
+ struct dev_intx_gsi_link *digl = NULL;
+ struct hvm_girq_dpci_mapping *girq = NULL;
+ unsigned int guest_gsi;
- if ( !digl || !girq )
+ /*
+ * Mapping GSIs for the hardware domain is different than doing it for
+ * an unpriviledged guest, the hardware domain is only allowed to
+ * identity map GSIs, and as such all the data in the u.pci union is
+ * discarded.
+ */
+ if ( !is_hardware_domain(d) )
{
- spin_unlock(&d->event_lock);
- xfree(girq);
- xfree(digl);
- return -ENOMEM;
- }
+ unsigned int link;
+
+ digl = xmalloc(struct dev_intx_gsi_link);
+ girq = xmalloc(struct hvm_girq_dpci_mapping);
+
+ if ( !digl || !girq )
+ {
+ spin_unlock(&d->event_lock);
+ xfree(girq);
+ xfree(digl);
+ return -ENOMEM;
+ }
+
+ girq->bus = digl->bus = pt_irq_bind->u.pci.bus;
+ girq->device = digl->device = pt_irq_bind->u.pci.device;
+ girq->intx = digl->intx = pt_irq_bind->u.pci.intx;
+ list_add_tail(&digl->list, &pirq_dpci->digl_list);
- hvm_irq_dpci->link_cnt[link]++;
+ guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx);
+ link = hvm_pci_intx_link(digl->device, digl->intx);
- digl->bus = bus;
- digl->device = device;
- digl->intx = intx;
- list_add_tail(&digl->list, &pirq_dpci->digl_list);
+ hvm_irq_dpci->link_cnt[link]++;
- girq->bus = bus;
- girq->device = device;
- girq->intx = intx;
- girq->machine_gsi = pirq;
- list_add_tail(&girq->list, &hvm_irq_dpci->girq[guest_gsi]);
+ girq->machine_gsi = pirq;
+ list_add_tail(&girq->list, &hvm_irq_dpci->girq[guest_gsi]);
+ }
+ else
+ {
+ /* MSI_TRANSLATE is not supported by the hardware domain. */
+ if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_PCI ||
+ pirq >= hvm_domain_irq(d)->nr_gsis )
+ return -EINVAL;
+ guest_gsi = pirq;
+ }
/* Bind the same mirq once in the same domain */
if ( !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
@@ -472,7 +510,27 @@ int pt_irq_create_bind(
pirq_dpci->flags = HVM_IRQ_DPCI_MAPPED |
HVM_IRQ_DPCI_MACH_PCI |
HVM_IRQ_DPCI_GUEST_PCI;
- share = BIND_PIRQ__WILL_SHARE;
+ if ( !is_hardware_domain(d) )
+ share = BIND_PIRQ__WILL_SHARE;
+ else
+ {
+ unsigned int pin;
+ struct hvm_vioapic *vioapic = gsi_vioapic(d, guest_gsi,
+ &pin);
+
+ if ( !vioapic )
+ {
+ ASSERT_UNREACHABLE();
+ return -EINVAL;
+ }
+ pirq_dpci->flags |= HVM_IRQ_DPCI_IDENTITY_GSI;
+ /*
+ * Check if the corresponding vIO APIC pin is configured
+ * level or edge trigger, level triggered interrupts will
+ * be marked as shareable.
+ */
+ share = vioapic->redirtbl[pin].fields.trig_mode;
+ }
}
/* Init timer before binding */
@@ -489,9 +547,15 @@ int pt_irq_create_bind(
* IRQ_GUEST is not set. As such we can reset 'dom' directly.
*/
pirq_dpci->dom = NULL;
- list_del(&girq->list);
- list_del(&digl->list);
- hvm_irq_dpci->link_cnt[link]--;
+ if ( !is_hardware_domain(d) )
+ {
+ unsigned int link = hvm_pci_intx_link(digl->device,
+ digl->intx);
+
+ list_del(&girq->list);
+ list_del(&digl->list);
+ hvm_irq_dpci->link_cnt[link]--;
+ }
pirq_dpci->flags = 0;
pirq_cleanup_check(info, d);
spin_unlock(&d->event_lock);
@@ -504,10 +568,17 @@ int pt_irq_create_bind(
spin_unlock(&d->event_lock);
if ( iommu_verbose )
- printk(XENLOG_G_INFO
- "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);
+ {
+ char buf[24] = "";
+
+ if ( !is_hardware_domain(d) )
+ snprintf(buf, ARRAY_SIZE(buf), " dev=%02x.%02x.%u intx=%u",
+ digl->bus, PCI_SLOT(digl->device),
+ PCI_FUNC(digl->device), digl->intx);
+
+ printk(XENLOG_G_INFO "d%d: bind: m_gsi=%u g_gsi=%u%s\n",
+ d->domain_id, pirq, guest_gsi, buf);
+ }
break;
}
@@ -554,7 +625,7 @@ int pt_irq_destroy_bind(
hvm_irq_dpci = domain_get_irq_dpci(d);
- if ( hvm_irq_dpci == NULL )
+ if ( hvm_irq_dpci == NULL && !is_hardware_domain(d) )
{
spin_unlock(&d->event_lock);
return -EINVAL;
@@ -573,27 +644,30 @@ int pt_irq_destroy_bind(
struct hvm_girq_dpci_mapping *girq;
struct dev_intx_gsi_link *digl, *tmp;
- list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list )
+ if ( hvm_irq_dpci )
{
- if ( girq->bus == bus &&
- girq->device == device &&
- girq->intx == intx &&
- girq->machine_gsi == machine_gsi )
+ list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list )
{
- list_del(&girq->list);
- xfree(girq);
- girq = NULL;
- break;
+ if ( girq->bus == bus &&
+ girq->device == device &&
+ girq->intx == intx &&
+ girq->machine_gsi == machine_gsi )
+ {
+ list_del(&girq->list);
+ xfree(girq);
+ girq = NULL;
+ break;
+ }
}
- }
- if ( girq )
- {
- spin_unlock(&d->event_lock);
- return -EINVAL;
- }
+ if ( girq )
+ {
+ spin_unlock(&d->event_lock);
+ return -EINVAL;
+ }
- hvm_irq_dpci->link_cnt[link]--;
+ hvm_irq_dpci->link_cnt[link]--;
+ }
/* clear the mirq info */
if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
@@ -638,11 +712,15 @@ int pt_irq_destroy_bind(
if ( what && iommu_verbose )
{
unsigned int device = pt_irq_bind->u.pci.device;
+ char buf[24] = "";
- printk(XENLOG_G_INFO
- "d%d %s unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n",
- d->domain_id, what, machine_gsi, pt_irq_bind->u.pci.bus,
- PCI_SLOT(device), PCI_FUNC(device), pt_irq_bind->u.pci.intx);
+ if ( !is_hardware_domain(d) )
+ snprintf(buf, ARRAY_SIZE(buf), " dev=%02x.%02x.%u intx=%u",
+ pt_irq_bind->u.pci.bus, PCI_SLOT(device),
+ PCI_FUNC(device), pt_irq_bind->u.pci.intx);
+
+ printk(XENLOG_G_INFO "d%d %s unmap: m_irq=%u%s\n",
+ d->domain_id, what, machine_gsi, buf);
}
return 0;
@@ -696,7 +774,7 @@ int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq)
struct hvm_irq_dpci *dpci = domain_get_irq_dpci(d);
struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
- if ( !iommu_enabled || !dpci || !pirq_dpci ||
+ if ( !iommu_enabled || (!is_hardware_domain(d) && !dpci) || !pirq_dpci ||
!(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
return 0;
@@ -757,7 +835,7 @@ void hvm_dpci_msi_eoi(struct domain *d, int vector)
static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
{
- if ( unlikely(!hvm_domain_irq(d)->dpci) )
+ if ( unlikely(!hvm_domain_irq(d)->dpci) && !is_hardware_domain(d) )
{
ASSERT_UNREACHABLE();
return;
@@ -789,10 +867,17 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
list_for_each_entry ( digl, &pirq_dpci->digl_list, list )
{
+ ASSERT(!(pirq_dpci->flags & HVM_IRQ_DPCI_IDENTITY_GSI));
hvm_pci_intx_assert(d, digl->device, digl->intx);
pirq_dpci->pending++;
}
+ if ( pirq_dpci->flags & HVM_IRQ_DPCI_IDENTITY_GSI )
+ {
+ hvm_gsi_assert(d, pirq->pirq);
+ pirq_dpci->pending++;
+ }
+
if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE )
{
/* for translated MSI to INTx interrupt, eoi as early as possible */
@@ -814,17 +899,10 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
spin_unlock(&d->event_lock);
}
-static void __hvm_dpci_eoi(struct domain *d,
- const struct hvm_girq_dpci_mapping *girq,
- const union vioapic_redir_entry *ent)
+static void hvm_pirq_eoi(struct pirq *pirq,
+ const union vioapic_redir_entry *ent)
{
- struct pirq *pirq = pirq_info(d, girq->machine_gsi);
- struct hvm_pirq_dpci *pirq_dpci;
-
- if ( !hvm_domain_use_pirq(d, pirq) )
- hvm_pci_intx_deassert(d, girq->device, girq->intx);
-
- pirq_dpci = pirq_dpci(pirq);
+ struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
/*
* No need to get vector lock for timer
@@ -839,6 +917,31 @@ static void __hvm_dpci_eoi(struct domain *d,
pirq_guest_eoi(pirq);
}
+static void __hvm_dpci_eoi(struct domain *d,
+ const struct hvm_girq_dpci_mapping *girq,
+ const union vioapic_redir_entry *ent)
+{
+ struct pirq *pirq = pirq_info(d, girq->machine_gsi);
+
+ if ( !hvm_domain_use_pirq(d, pirq) )
+ hvm_pci_intx_deassert(d, girq->device, girq->intx);
+
+ hvm_pirq_eoi(pirq, ent);
+}
+
+static void hvm_gsi_eoi(struct domain *d, unsigned int gsi,
+ const union vioapic_redir_entry *ent)
+{
+ struct pirq *pirq = pirq_info(d, gsi);
+
+ /* Check if GSI is actually mapped. */
+ if ( !pirq_dpci(pirq) )
+ return;
+
+ hvm_gsi_deassert(d, gsi);
+ hvm_pirq_eoi(pirq, ent);
+}
+
void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi,
const union vioapic_redir_entry *ent)
{
@@ -848,6 +951,13 @@ void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi,
if ( !iommu_enabled )
return;
+ if ( is_hardware_domain(d) )
+ {
+ spin_lock(&d->event_lock);
+ hvm_gsi_eoi(d, guest_gsi, ent);
+ goto unlock;
+ }
+
if ( guest_gsi < NR_ISAIRQS )
{
hvm_dpci_isairq_eoi(d, guest_gsi);
diff --git a/xen/include/xen/hvm/irq.h b/xen/include/xen/hvm/irq.h
index 671a6f2e06..0d2c72c109 100644
--- a/xen/include/xen/hvm/irq.h
+++ b/xen/include/xen/hvm/irq.h
@@ -40,6 +40,7 @@ struct dev_intx_gsi_link {
#define _HVM_IRQ_DPCI_EOI_LATCH_SHIFT 3
#define _HVM_IRQ_DPCI_GUEST_PCI_SHIFT 4
#define _HVM_IRQ_DPCI_GUEST_MSI_SHIFT 5
+#define _HVM_IRQ_DPCI_IDENTITY_GSI_SHIFT 6
#define _HVM_IRQ_DPCI_TRANSLATE_SHIFT 15
#define HVM_IRQ_DPCI_MACH_PCI (1 << _HVM_IRQ_DPCI_MACH_PCI_SHIFT)
#define HVM_IRQ_DPCI_MACH_MSI (1 << _HVM_IRQ_DPCI_MACH_MSI_SHIFT)
@@ -47,6 +48,7 @@ struct dev_intx_gsi_link {
#define HVM_IRQ_DPCI_EOI_LATCH (1 << _HVM_IRQ_DPCI_EOI_LATCH_SHIFT)
#define HVM_IRQ_DPCI_GUEST_PCI (1 << _HVM_IRQ_DPCI_GUEST_PCI_SHIFT)
#define HVM_IRQ_DPCI_GUEST_MSI (1 << _HVM_IRQ_DPCI_GUEST_MSI_SHIFT)
+#define HVM_IRQ_DPCI_IDENTITY_GSI (1 << _HVM_IRQ_DPCI_IDENTITY_GSI_SHIFT)
#define HVM_IRQ_DPCI_TRANSLATE (1 << _HVM_IRQ_DPCI_TRANSLATE_SHIFT)
#define VMSI_DEST_ID_MASK 0xff
@@ -123,6 +125,10 @@ void hvm_isa_irq_assert(
void hvm_isa_irq_deassert(
struct domain *d, unsigned int isa_irq);
+/* Modify state of GSIs. */
+void hvm_gsi_assert(struct domain *d, unsigned int gsi);
+void hvm_gsi_deassert(struct domain *d, unsigned int gsi);
+
int hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq);
int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data);
--
2.11.0 (Apple Git-81)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 for-next 3/3] x86/vioapic: bind interrupts to PVH Dom0
2017-05-17 15:15 [PATCH v3 for-next 0/3] x86/dpci: bind legacy PCI interrupts to PVHv2 Dom0 Roger Pau Monne
2017-05-17 15:15 ` [PATCH v3 for-next 1/3] x86/physdev: factor out the code to allocate and map a pirq Roger Pau Monne
2017-05-17 15:15 ` [PATCH v3 for-next 2/3] x86/pt: enable binding of GSIs to a PVH Dom0 Roger Pau Monne
@ 2017-05-17 15:15 ` Roger Pau Monne
2017-05-30 10:05 ` Jan Beulich
2 siblings, 1 reply; 12+ messages in thread
From: Roger Pau Monne @ 2017-05-17 15:15 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, boris.ostrovsky, Roger Pau Monne, Jan Beulich
Add the glue in order to bind the PVH Dom0 GSI from bare metal. This is done
when Dom0 unmasks the vIO APIC pins, by fetching the current pin settings and
setting up the PIRQ, which will then be bound to Dom0.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v2:
- s/vioapic_dom0_map_gsi/vioapic_hwdom_map_gsi/.
- Don't set hvm_domid in xen_domctl_bind_pt_irq_t (it's ignored).
- s/gdprintk/gprintk/.
- Change the logic of the error paths and remove the labels.
Changes since v1:
- Mask the pin on error (instead of panicking).
- Factor out the Dom0 specific code into a function.
- Use the newly introduced allocate_and_map_gsi_pirq instead of
physdev_map_pirq.
---
xen/arch/x86/hvm/vioapic.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index abcc473c88..4093082c31 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -158,6 +158,52 @@ static int vioapic_read(
return X86EMUL_OKAY;
}
+static int vioapic_hwdom_map_gsi(unsigned int gsi, unsigned int trig,
+ unsigned int pol)
+{
+ struct domain *d = current->domain;
+ xen_domctl_bind_pt_irq_t pt_irq_bind = {
+ .irq_type = PT_IRQ_TYPE_PCI,
+ .machine_irq = gsi,
+ };
+ int ret, pirq = gsi;
+
+ ASSERT(is_hardware_domain(d));
+
+ /* Interrupt has been unmasked, bind it now. */
+ ret = mp_register_gsi(gsi, trig, pol);
+ if ( ret == -EEXIST )
+ return 0;
+ if ( ret )
+ {
+ gprintk(XENLOG_WARNING, "vioapic: error registering GSI %u: %d\n",
+ gsi, ret);
+ return ret;
+ }
+
+ ret = allocate_and_map_gsi_pirq(d, &pirq, &pirq);
+ if ( ret )
+ {
+ gprintk(XENLOG_WARNING, "vioapic: error mapping GSI %u: %d\n",
+ gsi, ret);
+ return ret;
+ }
+
+ pcidevs_lock();
+ ret = pt_irq_create_bind(d, &pt_irq_bind);
+ if ( ret )
+ {
+ gprintk(XENLOG_WARNING, "vioapic: error binding GSI %u: %d\n",
+ gsi, ret);
+ spin_lock(&d->event_lock);
+ unmap_domain_pirq(d, pirq);
+ spin_unlock(&d->event_lock);
+ }
+ pcidevs_unlock();
+
+ return ret;
+}
+
static void vioapic_write_redirent(
struct hvm_vioapic *vioapic, unsigned int idx,
int top_word, uint32_t val)
@@ -188,6 +234,20 @@ static void vioapic_write_redirent(
unmasked = unmasked && !ent.fields.mask;
}
+ if ( is_hardware_domain(d) && unmasked )
+ {
+ int ret;
+
+ ret = vioapic_hwdom_map_gsi(gsi, ent.fields.trig_mode,
+ ent.fields.polarity);
+ if ( ret )
+ {
+ /* Mask the entry again. */
+ ent.fields.mask = 1;
+ unmasked = 0;
+ }
+ }
+
*pent = ent;
if ( gsi == 0 )
--
2.11.0 (Apple Git-81)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 for-next 1/3] x86/physdev: factor out the code to allocate and map a pirq
2017-05-17 15:15 ` [PATCH v3 for-next 1/3] x86/physdev: factor out the code to allocate and map a pirq Roger Pau Monne
@ 2017-05-30 9:16 ` Jan Beulich
2017-05-31 12:53 ` Roger Pau Monne
0 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2017-05-30 9:16 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, boris.ostrovsky, xen-devel
>>> On 17.05.17 at 17:15, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2537,3 +2537,162 @@ bool_t hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
> return is_hvm_domain(d) && pirq &&
> pirq->arch.hvm.emuirq != IRQ_UNBOUND;
> }
> +
> +static int allocate_pirq(struct domain *d, int pirq, int irq, int type,
> + int *nr)
> +{
> + int current_pirq;
> +
> + ASSERT(spin_is_locked(&d->event_lock));
> + current_pirq = domain_irq_to_pirq(d, irq);
> + if ( pirq < 0 )
> + {
> + if ( current_pirq )
> + {
> + dprintk(XENLOG_G_ERR, "dom%d: %d:%d already mapped to %d\n",
> + d->domain_id, irq, pirq, current_pirq);
Instead of "irq" the old code did pass "*index", i.e. a Dom0 kernel
specified value (which is going to be more useful, as any problem
here will need to be diagnosed in the Dom0 kernel). The two
values are identical only if, in the GSI case,
domain_pirq_to_irq(current->domain, *index) in
allocate_and_map_gsi_pirq() returned a negative value.
> + if ( current_pirq < 0 )
> + return -EBUSY;
> + }
> + else if ( type == MAP_PIRQ_TYPE_MULTI_MSI )
> + {
> + if ( *nr <= 0 || *nr > 32 )
> + return -EDOM;
> + else if ( *nr != 1 && !iommu_intremap )
> + return -EOPNOTSUPP;
> + else
Pointless "else" (twice).
> + {
> + while ( *nr & (*nr - 1) )
> + *nr += *nr & -*nr;
> + pirq = get_free_pirqs(d, *nr);
> + if ( pirq < 0 )
> + {
> + while ( (*nr >>= 1) > 1 )
> + if ( get_free_pirqs(d, *nr) > 0 )
> + break;
> + dprintk(XENLOG_G_ERR, "dom%d: no block of %d free pirqs\n",
> + d->domain_id, *nr << 1);
> + }
> + }
> + }
> + else
> + {
> + pirq = get_free_pirq(d, type);
> + if ( pirq < 0 )
> + dprintk(XENLOG_G_ERR, "dom%d: no free pirq\n", d->domain_id);
> + }
> + }
> + else if ( current_pirq && pirq != current_pirq )
> + {
> + dprintk(XENLOG_G_ERR, "dom%d: irq %d already mapped to pirq %d\n",
> + d->domain_id, irq, current_pirq);
> + pirq = -EEXIST;
"return -EEXIST" please, to match the direct returns you use further
up.
> + }
> +
> + return pirq;
> +}
> +
> +int allocate_and_map_gsi_pirq(struct domain *d, int *index, int *pirq_p)
Neither here nor in the MSI sibling you ever write to *index, so
there's no need for the parameter to have pointer type.
> +{
> + int irq, pirq, ret;
> +
> + if ( *index < 0 || *index >= nr_irqs_gsi )
> + {
> + dprintk(XENLOG_G_ERR, "dom%d: map invalid irq %d\n", d->domain_id,
> + *index);
> + return -EINVAL;
> + }
> +
> + irq = domain_pirq_to_irq(current->domain, *index);
> + if ( irq <= 0 )
> + {
> + if ( is_hardware_domain(current->domain) )
> + irq = *index;
> + else
> + {
> + dprintk(XENLOG_G_ERR, "dom%d: map pirq with incorrect irq!\n",
> + d->domain_id);
> + return -EINVAL;
> + }
> + }
> +
> + /* Verify or get pirq. */
> + spin_lock(&d->event_lock);
> + pirq = allocate_pirq(d, *pirq_p, irq, MAP_PIRQ_TYPE_GSI, 0);
The last parameter of allocate_pirq() is a pointer, so you really
mean NULL here.
> + if ( pirq < 0 )
> + {
> + ret = pirq;
> + goto done;
> + }
> +
> + ret = map_domain_pirq(d, pirq, irq, MAP_PIRQ_TYPE_GSI, NULL);
> + if ( ret == 0 )
> + *pirq_p = pirq;
> +
> + done:
> + spin_unlock(&d->event_lock);
> + return ret;
> +}
Blank line before final return statement please.
> +int allocate_and_map_msi_pirq(struct domain *d, int *index, int *pirq_p,
> + int type, struct msi_info *msi)
> +{
> + int irq, pirq, ret;
> +
> + switch ( type )
> + {
> + case MAP_PIRQ_TYPE_MSI:
> + if ( !msi->table_base )
> + msi->entry_nr = 1;
> + irq = *index;
> + if ( irq == -1 )
> + case MAP_PIRQ_TYPE_MULTI_MSI:
> + irq = create_irq(NUMA_NO_NODE);
> +
> + if ( irq < nr_irqs_gsi || irq >= nr_irqs )
> + {
> + dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n",
> + d->domain_id);
> + return -EINVAL;
> + }
> +
> + msi->irq = irq;
> + break;
> +
> + default:
> + dprintk(XENLOG_G_ERR, "dom%d: wrong pirq type %x\n",
> + d->domain_id, type);
> + return -EINVAL;
This really should be an ASSERT_UNREACHABLE() (with the return
statement kept).
> + }
> +
> + msi->irq = irq;
> +
> + pcidevs_lock();
> + /* Verify or get pirq. */
> + spin_lock(&d->event_lock);
> + pirq = allocate_pirq(d, *pirq_p, irq, type, &msi->entry_nr);
> + if ( pirq < 0 )
> + {
> + ret = pirq;
> + goto done;
> + }
> +
> + ret = map_domain_pirq(d, pirq, irq, type, msi);
> + if ( ret == 0 )
> + *pirq_p = pirq;
> +
> + done:
> + spin_unlock(&d->event_lock);
> + pcidevs_unlock();
> + if ( ret != 0 )
> + switch ( type )
> + {
> + case MAP_PIRQ_TYPE_MSI:
> + if ( *index == -1 )
> + case MAP_PIRQ_TYPE_MULTI_MSI:
> + destroy_irq(irq);
> + break;
> + }
> + return ret;
> +}
> +
Please don't end a file with a blank line.
> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
> index eec4a41231..e99fd9a35f 100644
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -92,8 +92,7 @@ int physdev_map_pirq(domid_t domid, int type, int *index,
> int *pirq_p,
> struct msi_info *msi)
> {
> struct domain *d = current->domain;
> - int pirq, irq, ret = 0;
> - void *map_data = NULL;
> + int ret;
>
> if ( domid == DOMID_SELF && is_hvm_domain(d) && has_pirq(d) )
> {
> @@ -119,135 +118,22 @@ int physdev_map_pirq(domid_t domid, int type, int
> *index, int *pirq_p,
> switch ( type )
> {
> case MAP_PIRQ_TYPE_GSI:
> - if ( *index < 0 || *index >= nr_irqs_gsi )
> - {
> - dprintk(XENLOG_G_ERR, "dom%d: map invalid irq %d\n",
> - d->domain_id, *index);
> - ret = -EINVAL;
> - goto free_domain;
> - }
> -
> - irq = domain_pirq_to_irq(current->domain, *index);
> - if ( irq <= 0 )
> - {
> - if ( is_hardware_domain(current->domain) )
> - irq = *index;
> - else {
> - dprintk(XENLOG_G_ERR, "dom%d: map pirq with incorrect irq!\n",
> - d->domain_id);
> - ret = -EINVAL;
> - goto free_domain;
> - }
> - }
> + ret = allocate_and_map_gsi_pirq(d, index, pirq_p);
> break;
> -
> case MAP_PIRQ_TYPE_MSI:
Please don't delete the blank lines between case blocks, even if
the blocks are much smaller now.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 for-next 2/3] x86/pt: enable binding of GSIs to a PVH Dom0
2017-05-17 15:15 ` [PATCH v3 for-next 2/3] x86/pt: enable binding of GSIs to a PVH Dom0 Roger Pau Monne
@ 2017-05-30 10:01 ` Jan Beulich
2017-05-31 14:29 ` Roger Pau Monne
0 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2017-05-30 10:01 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, boris.ostrovsky, xen-devel
>>> On 17.05.17 at 17:15, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -126,6 +126,49 @@ void hvm_pci_intx_deassert(
> spin_unlock(&d->arch.hvm_domain.irq_lock);
> }
>
> +void hvm_gsi_assert(struct domain *d, unsigned int gsi)
> +{
> + struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> +
> + if ( gsi >= hvm_irq->nr_gsis )
> + {
> + ASSERT_UNREACHABLE();
> + return;
> + }
> +
> + /*
> + * __hvm_pci_intx_{de}assert uses an array to track the status of each
> + * interrupt line, and Xen does the routing and GSI assertion based on
> + * that. This prevents the same line from triggering multiple times, which
> + * is not available here, and thus Xen needs to rely on gsi_assert_count in
> + * order to know if the GSI is pending or not.
> + */
The "which is not available here" part is at least confusing. I'm not
even sure whether the "which" is supposed to refer to the array or
something else, because you use the exact same array here.
> +void hvm_gsi_deassert(struct domain *d, unsigned int gsi)
> +{
> + struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> +
> + if ( gsi >= hvm_irq->nr_gsis )
> + {
> + ASSERT_UNREACHABLE();
> + return;
> + }
> +
> + spin_lock(&d->arch.hvm_domain.irq_lock);
> + if ( hvm_irq->gsi_assert_count[gsi] )
> + hvm_irq->gsi_assert_count[gsi] = 0;
> + ASSERT(!hvm_irq->gsi_assert_count[gsi]);
I don't think the if() and ASSERT() are of any use here anymore.
> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/io.c
> @@ -164,6 +164,20 @@ static void pt_irq_time_out(void *data)
>
> spin_lock(&irq_map->dom->event_lock);
>
> + if ( irq_map->flags & HVM_IRQ_DPCI_IDENTITY_GSI )
> + {
> + struct pirq *pirq = dpci_pirq(irq_map);
> +
> + ASSERT(is_hardware_domain(irq_map->dom));
> + /*
> + * Identity mapped, no need to iterate over the guest GSI list to find
> + * other pirqs sharing the same guest GSI.
> + */
> + irq_map->flags |= HVM_IRQ_DPCI_EOI_LATCH;
> + hvm_gsi_deassert(irq_map->dom, pirq->pirq);
> + goto out;
> + }
> +
> dpci = domain_get_irq_dpci(irq_map->dom);
> if ( unlikely(!dpci) )
> {
> @@ -185,6 +199,7 @@ static void pt_irq_time_out(void *data)
> hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx);
> }
>
> + out:
> pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL);
With the 1:1 mapping, do you really need to go through
pt_pirq_iterate() here? I.e. can't you invoke pt_irq_guest_eoi()
just once and be done? Or really it probably can be even more
straight, as then there also is no point in setting the
HVM_IRQ_DPCI_EOI_LATCH flag, but you could rather do
directly what pt_irq_guest_eoi() does in its if() body. Otoh I may
be missing something here, as I can't see why the code is using
pt_pirq_iterate() even before your change.
> @@ -472,7 +510,27 @@ int pt_irq_create_bind(
> pirq_dpci->flags = HVM_IRQ_DPCI_MAPPED |
> HVM_IRQ_DPCI_MACH_PCI |
> HVM_IRQ_DPCI_GUEST_PCI;
> - share = BIND_PIRQ__WILL_SHARE;
> + if ( !is_hardware_domain(d) )
> + share = BIND_PIRQ__WILL_SHARE;
> + else
> + {
> + unsigned int pin;
> + struct hvm_vioapic *vioapic = gsi_vioapic(d, guest_gsi,
> + &pin);
> +
> + if ( !vioapic )
> + {
> + ASSERT_UNREACHABLE();
> + return -EINVAL;
> + }
> + pirq_dpci->flags |= HVM_IRQ_DPCI_IDENTITY_GSI;
> + /*
> + * Check if the corresponding vIO APIC pin is configured
> + * level or edge trigger, level triggered interrupts will
> + * be marked as shareable.
> + */
> + share = vioapic->redirtbl[pin].fields.trig_mode;
As pointed out during prior review (perhaps of another patch of
yours) the trigger mode bit is meaningless for masked RTEs. At
the very least an ASSERT() needs to be here for that reason,
of course provided masked entries can never be seen here.
> @@ -489,9 +547,15 @@ int pt_irq_create_bind(
> * IRQ_GUEST is not set. As such we can reset 'dom' directly.
> */
> pirq_dpci->dom = NULL;
> - list_del(&girq->list);
> - list_del(&digl->list);
> - hvm_irq_dpci->link_cnt[link]--;
> + if ( !is_hardware_domain(d) )
To be honest I'd prefer if you checked digl and/or girq against NULL
here, to avoid someone updating the condition above without
updating this one in lock step.
> @@ -573,27 +644,30 @@ int pt_irq_destroy_bind(
> struct hvm_girq_dpci_mapping *girq;
> struct dev_intx_gsi_link *digl, *tmp;
>
> - list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list )
> + if ( hvm_irq_dpci )
> {
> - if ( girq->bus == bus &&
> - girq->device == device &&
> - girq->intx == intx &&
> - girq->machine_gsi == machine_gsi )
> + list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list )
> {
> - list_del(&girq->list);
> - xfree(girq);
> - girq = NULL;
> - break;
> + if ( girq->bus == bus &&
> + girq->device == device &&
> + girq->intx == intx &&
> + girq->machine_gsi == machine_gsi )
> + {
> + list_del(&girq->list);
> + xfree(girq);
> + girq = NULL;
> + break;
> + }
> }
> - }
>
> - if ( girq )
> - {
> - spin_unlock(&d->event_lock);
> - return -EINVAL;
> - }
> + if ( girq )
> + {
> + spin_unlock(&d->event_lock);
> + return -EINVAL;
> + }
>
> - hvm_irq_dpci->link_cnt[link]--;
> + hvm_irq_dpci->link_cnt[link]--;
> + }
>
> /* clear the mirq info */
> if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
What you leave untouched here is code freeing something you
never allocate for Dom0. While likely this is correct (as the list will
always be empty), it is confusing at the same time. Furthermore
if you also skip that part, I think you can avoid having to re-indent
all the code further up.
> @@ -638,11 +712,15 @@ int pt_irq_destroy_bind(
> if ( what && iommu_verbose )
> {
> unsigned int device = pt_irq_bind->u.pci.device;
> + char buf[24] = "";
>
> - printk(XENLOG_G_INFO
> - "d%d %s unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n",
> - d->domain_id, what, machine_gsi, pt_irq_bind->u.pci.bus,
> - PCI_SLOT(device), PCI_FUNC(device), pt_irq_bind->u.pci.intx);
> + if ( !is_hardware_domain(d) )
> + snprintf(buf, ARRAY_SIZE(buf), " dev=%02x.%02x.%u intx=%u",
> + pt_irq_bind->u.pci.bus, PCI_SLOT(device),
> + PCI_FUNC(device), pt_irq_bind->u.pci.intx);
Now that this supports Dom0, you also need to log the segment.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 for-next 3/3] x86/vioapic: bind interrupts to PVH Dom0
2017-05-17 15:15 ` [PATCH v3 for-next 3/3] x86/vioapic: bind interrupts to " Roger Pau Monne
@ 2017-05-30 10:05 ` Jan Beulich
2017-05-31 14:48 ` Roger Pau Monne
0 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2017-05-30 10:05 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, boris.ostrovsky, xen-devel
>>> On 17.05.17 at 17:15, <roger.pau@citrix.com> wrote:
> Changes since v2:
> - s/vioapic_dom0_map_gsi/vioapic_hwdom_map_gsi/.
> - Don't set hvm_domid in xen_domctl_bind_pt_irq_t (it's ignored).
The implication of the respective earlier comment was for there to
first be a prereq patch added removing this dead field. Otherwise
not setting the field is a latent bug.
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -158,6 +158,52 @@ static int vioapic_read(
> return X86EMUL_OKAY;
> }
>
> +static int vioapic_hwdom_map_gsi(unsigned int gsi, unsigned int trig,
> + unsigned int pol)
> +{
> + struct domain *d = current->domain;
> + xen_domctl_bind_pt_irq_t pt_irq_bind = {
> + .irq_type = PT_IRQ_TYPE_PCI,
> + .machine_irq = gsi,
Actually you still set the field, just that this is no implicit. Hence the
latent bug reduces to just the hwdom != Dom0 case, but anyway.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 for-next 1/3] x86/physdev: factor out the code to allocate and map a pirq
2017-05-30 9:16 ` Jan Beulich
@ 2017-05-31 12:53 ` Roger Pau Monne
0 siblings, 0 replies; 12+ messages in thread
From: Roger Pau Monne @ 2017-05-31 12:53 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, boris.ostrovsky, xen-devel
On Tue, May 30, 2017 at 03:16:51AM -0600, Jan Beulich wrote:
> >>> On 17.05.17 at 17:15, <roger.pau@citrix.com> wrote:
> > --- a/xen/arch/x86/irq.c
> > +++ b/xen/arch/x86/irq.c
> > @@ -2537,3 +2537,162 @@ bool_t hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
> > return is_hvm_domain(d) && pirq &&
> > pirq->arch.hvm.emuirq != IRQ_UNBOUND;
> > }
> > +
> > +static int allocate_pirq(struct domain *d, int pirq, int irq, int type,
> > + int *nr)
> > +{
> > + int current_pirq;
> > +
> > + ASSERT(spin_is_locked(&d->event_lock));
> > + current_pirq = domain_irq_to_pirq(d, irq);
> > + if ( pirq < 0 )
> > + {
> > + if ( current_pirq )
> > + {
> > + dprintk(XENLOG_G_ERR, "dom%d: %d:%d already mapped to %d\n",
> > + d->domain_id, irq, pirq, current_pirq);
>
> Instead of "irq" the old code did pass "*index", i.e. a Dom0 kernel
> specified value (which is going to be more useful, as any problem
> here will need to be diagnosed in the Dom0 kernel). The two
> values are identical only if, in the GSI case,
> domain_pirq_to_irq(current->domain, *index) in
> allocate_and_map_gsi_pirq() returned a negative value.
I guess I can propagate index into this function, I haven't done that
because it's only used by the error message, but not the code itself.
When I used the physdev hypoercalls in the past I think I was always
setting index == GSI IIRC.
> > + if ( current_pirq < 0 )
> > + return -EBUSY;
> > + }
> > + else if ( type == MAP_PIRQ_TYPE_MULTI_MSI )
> > + {
> > + if ( *nr <= 0 || *nr > 32 )
> > + return -EDOM;
> > + else if ( *nr != 1 && !iommu_intremap )
> > + return -EOPNOTSUPP;
> > + else
>
> Pointless "else" (twice).
Done (and re-indented the section below).
> > + {
> > + while ( *nr & (*nr - 1) )
> > + *nr += *nr & -*nr;
> > + pirq = get_free_pirqs(d, *nr);
> > + if ( pirq < 0 )
> > + {
> > + while ( (*nr >>= 1) > 1 )
> > + if ( get_free_pirqs(d, *nr) > 0 )
> > + break;
> > + dprintk(XENLOG_G_ERR, "dom%d: no block of %d free pirqs\n",
> > + d->domain_id, *nr << 1);
> > + }
> > + }
> > + }
> > + else
> > + {
> > + pirq = get_free_pirq(d, type);
> > + if ( pirq < 0 )
> > + dprintk(XENLOG_G_ERR, "dom%d: no free pirq\n", d->domain_id);
> > + }
> > + }
> > + else if ( current_pirq && pirq != current_pirq )
> > + {
> > + dprintk(XENLOG_G_ERR, "dom%d: irq %d already mapped to pirq %d\n",
> > + d->domain_id, irq, current_pirq);
> > + pirq = -EEXIST;
>
> "return -EEXIST" please, to match the direct returns you use further
> up.
Ack.
> > + }
> > +
> > + return pirq;
> > +}
> > +
> > +int allocate_and_map_gsi_pirq(struct domain *d, int *index, int *pirq_p)
>
> Neither here nor in the MSI sibling you ever write to *index, so
> there's no need for the parameter to have pointer type.
Done
> > +{
> > + int irq, pirq, ret;
> > +
> > + if ( *index < 0 || *index >= nr_irqs_gsi )
> > + {
> > + dprintk(XENLOG_G_ERR, "dom%d: map invalid irq %d\n", d->domain_id,
> > + *index);
> > + return -EINVAL;
> > + }
> > +
> > + irq = domain_pirq_to_irq(current->domain, *index);
> > + if ( irq <= 0 )
> > + {
> > + if ( is_hardware_domain(current->domain) )
> > + irq = *index;
> > + else
> > + {
> > + dprintk(XENLOG_G_ERR, "dom%d: map pirq with incorrect irq!\n",
> > + d->domain_id);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + /* Verify or get pirq. */
> > + spin_lock(&d->event_lock);
> > + pirq = allocate_pirq(d, *pirq_p, irq, MAP_PIRQ_TYPE_GSI, 0);
>
> The last parameter of allocate_pirq() is a pointer, so you really
> mean NULL here.
>
> > + if ( pirq < 0 )
> > + {
> > + ret = pirq;
> > + goto done;
> > + }
> > +
> > + ret = map_domain_pirq(d, pirq, irq, MAP_PIRQ_TYPE_GSI, NULL);
> > + if ( ret == 0 )
> > + *pirq_p = pirq;
> > +
> > + done:
> > + spin_unlock(&d->event_lock);
> > + return ret;
> > +}
>
> Blank line before final return statement please.
Fixed both of the above.
> > +int allocate_and_map_msi_pirq(struct domain *d, int *index, int *pirq_p,
> > + int type, struct msi_info *msi)
> > +{
> > + int irq, pirq, ret;
> > +
> > + switch ( type )
> > + {
> > + case MAP_PIRQ_TYPE_MSI:
> > + if ( !msi->table_base )
> > + msi->entry_nr = 1;
> > + irq = *index;
> > + if ( irq == -1 )
> > + case MAP_PIRQ_TYPE_MULTI_MSI:
> > + irq = create_irq(NUMA_NO_NODE);
> > +
> > + if ( irq < nr_irqs_gsi || irq >= nr_irqs )
> > + {
> > + dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n",
> > + d->domain_id);
> > + return -EINVAL;
> > + }
> > +
> > + msi->irq = irq;
> > + break;
> > +
> > + default:
> > + dprintk(XENLOG_G_ERR, "dom%d: wrong pirq type %x\n",
> > + d->domain_id, type);
> > + return -EINVAL;
>
> This really should be an ASSERT_UNREACHABLE() (with the return
> statement kept).
Fixed.
> > + }
> > +
> > + msi->irq = irq;
> > +
> > + pcidevs_lock();
> > + /* Verify or get pirq. */
> > + spin_lock(&d->event_lock);
> > + pirq = allocate_pirq(d, *pirq_p, irq, type, &msi->entry_nr);
> > + if ( pirq < 0 )
> > + {
> > + ret = pirq;
> > + goto done;
> > + }
> > +
> > + ret = map_domain_pirq(d, pirq, irq, type, msi);
> > + if ( ret == 0 )
> > + *pirq_p = pirq;
> > +
> > + done:
> > + spin_unlock(&d->event_lock);
> > + pcidevs_unlock();
> > + if ( ret != 0 )
> > + switch ( type )
> > + {
> > + case MAP_PIRQ_TYPE_MSI:
> > + if ( *index == -1 )
> > + case MAP_PIRQ_TYPE_MULTI_MSI:
> > + destroy_irq(irq);
> > + break;
> > + }
> > + return ret;
> > +}
> > +
>
> Please don't end a file with a blank line.
Fixed, and I've also added a new line before the return statement.
> > diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
> > index eec4a41231..e99fd9a35f 100644
> > --- a/xen/arch/x86/physdev.c
> > +++ b/xen/arch/x86/physdev.c
> > @@ -92,8 +92,7 @@ int physdev_map_pirq(domid_t domid, int type, int *index,
> > int *pirq_p,
> > struct msi_info *msi)
> > {
> > struct domain *d = current->domain;
> > - int pirq, irq, ret = 0;
> > - void *map_data = NULL;
> > + int ret;
> >
> > if ( domid == DOMID_SELF && is_hvm_domain(d) && has_pirq(d) )
> > {
> > @@ -119,135 +118,22 @@ int physdev_map_pirq(domid_t domid, int type, int
> > *index, int *pirq_p,
> > switch ( type )
> > {
> > case MAP_PIRQ_TYPE_GSI:
> > - if ( *index < 0 || *index >= nr_irqs_gsi )
> > - {
> > - dprintk(XENLOG_G_ERR, "dom%d: map invalid irq %d\n",
> > - d->domain_id, *index);
> > - ret = -EINVAL;
> > - goto free_domain;
> > - }
> > -
> > - irq = domain_pirq_to_irq(current->domain, *index);
> > - if ( irq <= 0 )
> > - {
> > - if ( is_hardware_domain(current->domain) )
> > - irq = *index;
> > - else {
> > - dprintk(XENLOG_G_ERR, "dom%d: map pirq with incorrect irq!\n",
> > - d->domain_id);
> > - ret = -EINVAL;
> > - goto free_domain;
> > - }
> > - }
> > + ret = allocate_and_map_gsi_pirq(d, index, pirq_p);
> > break;
> > -
> > case MAP_PIRQ_TYPE_MSI:
>
> Please don't delete the blank lines between case blocks, even if
> the blocks are much smaller now.
Fixed (and the one below).
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 for-next 2/3] x86/pt: enable binding of GSIs to a PVH Dom0
2017-05-30 10:01 ` Jan Beulich
@ 2017-05-31 14:29 ` Roger Pau Monne
2017-06-01 7:05 ` Jan Beulich
0 siblings, 1 reply; 12+ messages in thread
From: Roger Pau Monne @ 2017-05-31 14:29 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, boris.ostrovsky, xen-devel
On Tue, May 30, 2017 at 04:01:00AM -0600, Jan Beulich wrote:
> >>> On 17.05.17 at 17:15, <roger.pau@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/irq.c
> > +++ b/xen/arch/x86/hvm/irq.c
> > @@ -126,6 +126,49 @@ void hvm_pci_intx_deassert(
> > spin_unlock(&d->arch.hvm_domain.irq_lock);
> > }
> >
> > +void hvm_gsi_assert(struct domain *d, unsigned int gsi)
> > +{
> > + struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> > +
> > + if ( gsi >= hvm_irq->nr_gsis )
> > + {
> > + ASSERT_UNREACHABLE();
> > + return;
> > + }
> > +
> > + /*
> > + * __hvm_pci_intx_{de}assert uses an array to track the status of each
> > + * interrupt line, and Xen does the routing and GSI assertion based on
> > + * that. This prevents the same line from triggering multiple times, which
> > + * is not available here, and thus Xen needs to rely on gsi_assert_count in
> > + * order to know if the GSI is pending or not.
> > + */
>
> The "which is not available here" part is at least confusing. I'm not
> even sure whether the "which" is supposed to refer to the array or
> something else, because you use the exact same array here.
I agree, it's confusing and badly worded, how about:
__hvm_pci_intx_{de}assert uses a bitfield in pci_intx.i to track the
status of each interrupt line, and Xen does the routing and GSI
assertion based on that. The value of the pci_intx.i bitmap prevents
the same line from triggering multiple times, which is not available
here, and thus Xen needs to rely on gsi_assert_count in order to know
if the GSI is pending or not.
> > +void hvm_gsi_deassert(struct domain *d, unsigned int gsi)
> > +{
> > + struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> > +
> > + if ( gsi >= hvm_irq->nr_gsis )
> > + {
> > + ASSERT_UNREACHABLE();
> > + return;
> > + }
> > +
> > + spin_lock(&d->arch.hvm_domain.irq_lock);
> > + if ( hvm_irq->gsi_assert_count[gsi] )
> > + hvm_irq->gsi_assert_count[gsi] = 0;
> > + ASSERT(!hvm_irq->gsi_assert_count[gsi]);
>
> I don't think the if() and ASSERT() are of any use here anymore.
Fixed.
> > --- a/xen/drivers/passthrough/io.c
> > +++ b/xen/drivers/passthrough/io.c
> > @@ -164,6 +164,20 @@ static void pt_irq_time_out(void *data)
> >
> > spin_lock(&irq_map->dom->event_lock);
> >
> > + if ( irq_map->flags & HVM_IRQ_DPCI_IDENTITY_GSI )
> > + {
> > + struct pirq *pirq = dpci_pirq(irq_map);
> > +
> > + ASSERT(is_hardware_domain(irq_map->dom));
> > + /*
> > + * Identity mapped, no need to iterate over the guest GSI list to find
> > + * other pirqs sharing the same guest GSI.
> > + */
> > + irq_map->flags |= HVM_IRQ_DPCI_EOI_LATCH;
> > + hvm_gsi_deassert(irq_map->dom, pirq->pirq);
> > + goto out;
> > + }
> > +
> > dpci = domain_get_irq_dpci(irq_map->dom);
> > if ( unlikely(!dpci) )
> > {
> > @@ -185,6 +199,7 @@ static void pt_irq_time_out(void *data)
> > hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx);
> > }
> >
> > + out:
> > pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL);
>
> With the 1:1 mapping, do you really need to go through
> pt_pirq_iterate() here? I.e. can't you invoke pt_irq_guest_eoi()
> just once and be done? Or really it probably can be even more
> straight, as then there also is no point in setting the
> HVM_IRQ_DPCI_EOI_LATCH flag, but you could rather do
> directly what pt_irq_guest_eoi() does in its if() body. Otoh I may
> be missing something here, as I can't see why the code is using
> pt_pirq_iterate() even before your change.
I have to admit this is not obviously clear to me (or I'm also missing
something), there are too many translation layers and indirections in
this code, together with a complete lack of comments.
Previous to my change, pt_irq_time_out iterates over the list of guest
devices (digl) bound to a pirq, desserts the interrupt lines and marks
all the pirqs bound to the same guest GSI with the EOI_LATCH flag.
Finally pt_irq_time_out iterates over the list of guest pirqs and
clears (EOI) all the ones marked as EOI_LATCH.
I don't really understand the usefulness of the EOI_LATCH flag, can't
pt_irq_time_out just call pt_irq_guest_eoi and avoid the iteration?
Something like:
list_for_each_entry ( digl, &irq_map->digl_list, list )
{
unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx);
const struct hvm_girq_dpci_mapping *girq;
list_for_each_entry ( girq, &dpci->girq[guest_gsi], list )
{
struct pirq *pirq = pirq_info(irq_map->dom, girq->machine_gsi);
pirq_dpci(pirq)->masked = 0;
pirq_dpci(pirq)->pending = 0;
pirq_guest_eoi(dpci_pirq(pirq));
}
hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx);
}
I can of course also do something similar for the identity mapping
case.
> > @@ -472,7 +510,27 @@ int pt_irq_create_bind(
> > pirq_dpci->flags = HVM_IRQ_DPCI_MAPPED |
> > HVM_IRQ_DPCI_MACH_PCI |
> > HVM_IRQ_DPCI_GUEST_PCI;
> > - share = BIND_PIRQ__WILL_SHARE;
> > + if ( !is_hardware_domain(d) )
> > + share = BIND_PIRQ__WILL_SHARE;
> > + else
> > + {
> > + unsigned int pin;
> > + struct hvm_vioapic *vioapic = gsi_vioapic(d, guest_gsi,
> > + &pin);
> > +
> > + if ( !vioapic )
> > + {
> > + ASSERT_UNREACHABLE();
> > + return -EINVAL;
> > + }
> > + pirq_dpci->flags |= HVM_IRQ_DPCI_IDENTITY_GSI;
> > + /*
> > + * Check if the corresponding vIO APIC pin is configured
> > + * level or edge trigger, level triggered interrupts will
> > + * be marked as shareable.
> > + */
> > + share = vioapic->redirtbl[pin].fields.trig_mode;
>
> As pointed out during prior review (perhaps of another patch of
> yours) the trigger mode bit is meaningless for masked RTEs. At
> the very least an ASSERT() needs to be here for that reason,
> of course provided masked entries can never be seen here.
pt_irq_create_bind for GSIs will only be called when the hardware
domain unmasks the RTE, so an ASSERT is the right choice IMHO.
> > @@ -489,9 +547,15 @@ int pt_irq_create_bind(
> > * IRQ_GUEST is not set. As such we can reset 'dom' directly.
> > */
> > pirq_dpci->dom = NULL;
> > - list_del(&girq->list);
> > - list_del(&digl->list);
> > - hvm_irq_dpci->link_cnt[link]--;
> > + if ( !is_hardware_domain(d) )
>
> To be honest I'd prefer if you checked digl and/or girq against NULL
> here, to avoid someone updating the condition above without
> updating this one in lock step.
I've changed the condition to "girq && digl".
> > @@ -573,27 +644,30 @@ int pt_irq_destroy_bind(
> > struct hvm_girq_dpci_mapping *girq;
> > struct dev_intx_gsi_link *digl, *tmp;
> >
> > - list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list )
> > + if ( hvm_irq_dpci )
> > {
> > - if ( girq->bus == bus &&
> > - girq->device == device &&
> > - girq->intx == intx &&
> > - girq->machine_gsi == machine_gsi )
> > + list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list )
> > {
> > - list_del(&girq->list);
> > - xfree(girq);
> > - girq = NULL;
> > - break;
> > + if ( girq->bus == bus &&
> > + girq->device == device &&
> > + girq->intx == intx &&
> > + girq->machine_gsi == machine_gsi )
> > + {
> > + list_del(&girq->list);
> > + xfree(girq);
> > + girq = NULL;
> > + break;
> > + }
> > }
> > - }
> >
> > - if ( girq )
> > - {
> > - spin_unlock(&d->event_lock);
> > - return -EINVAL;
> > - }
> > + if ( girq )
> > + {
> > + spin_unlock(&d->event_lock);
> > + return -EINVAL;
> > + }
> >
> > - hvm_irq_dpci->link_cnt[link]--;
> > + hvm_irq_dpci->link_cnt[link]--;
> > + }
> >
> > /* clear the mirq info */
> > if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
>
> What you leave untouched here is code freeing something you
> never allocate for Dom0. While likely this is correct (as the list will
> always be empty), it is confusing at the same time. Furthermore
> if you also skip that part, I think you can avoid having to re-indent
> all the code further up.
I see what you mean, the hvm_irq_dpci condition can be moved to the
upper if, since none of what's inside this condition is relevant for
the hardware domain case, and will avoid this re-indentation.
> > @@ -638,11 +712,15 @@ int pt_irq_destroy_bind(
> > if ( what && iommu_verbose )
> > {
> > unsigned int device = pt_irq_bind->u.pci.device;
> > + char buf[24] = "";
> >
> > - printk(XENLOG_G_INFO
> > - "d%d %s unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n",
> > - d->domain_id, what, machine_gsi, pt_irq_bind->u.pci.bus,
> > - PCI_SLOT(device), PCI_FUNC(device), pt_irq_bind->u.pci.intx);
> > + if ( !is_hardware_domain(d) )
> > + snprintf(buf, ARRAY_SIZE(buf), " dev=%02x.%02x.%u intx=%u",
> > + pt_irq_bind->u.pci.bus, PCI_SLOT(device),
> > + PCI_FUNC(device), pt_irq_bind->u.pci.intx);
>
> Now that this supports Dom0, you also need to log the segment.
I'm not sure I follow you here, for the Dom case all the fields in
u.pci.* are unused, since Xen does an identity mapping of the GSI, but
it doesn't know to which device it belongs. That's different for the
MSI case, but then this fields are not used anyway.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 for-next 3/3] x86/vioapic: bind interrupts to PVH Dom0
2017-05-30 10:05 ` Jan Beulich
@ 2017-05-31 14:48 ` Roger Pau Monne
2017-06-01 7:08 ` Jan Beulich
0 siblings, 1 reply; 12+ messages in thread
From: Roger Pau Monne @ 2017-05-31 14:48 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, boris.ostrovsky, xen-devel
On Tue, May 30, 2017 at 04:05:39AM -0600, Jan Beulich wrote:
> >>> On 17.05.17 at 17:15, <roger.pau@citrix.com> wrote:
> > Changes since v2:
> > - s/vioapic_dom0_map_gsi/vioapic_hwdom_map_gsi/.
> > - Don't set hvm_domid in xen_domctl_bind_pt_irq_t (it's ignored).
>
> The implication of the respective earlier comment was for there to
> first be a prereq patch added removing this dead field. Otherwise
> not setting the field is a latent bug.
I've added a pre-patch to get rid of hvm_domid in the bind struct.
> > --- a/xen/arch/x86/hvm/vioapic.c
> > +++ b/xen/arch/x86/hvm/vioapic.c
> > @@ -158,6 +158,52 @@ static int vioapic_read(
> > return X86EMUL_OKAY;
> > }
> >
> > +static int vioapic_hwdom_map_gsi(unsigned int gsi, unsigned int trig,
> > + unsigned int pol)
> > +{
> > + struct domain *d = current->domain;
> > + xen_domctl_bind_pt_irq_t pt_irq_bind = {
> > + .irq_type = PT_IRQ_TYPE_PCI,
> > + .machine_irq = gsi,
>
> Actually you still set the field, just that this is no implicit. Hence the
> latent bug reduces to just the hwdom != Dom0 case, but anyway.
What do you mean by implicit? Is that because I'm passing d to the
bind function?
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 for-next 2/3] x86/pt: enable binding of GSIs to a PVH Dom0
2017-05-31 14:29 ` Roger Pau Monne
@ 2017-06-01 7:05 ` Jan Beulich
0 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2017-06-01 7:05 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, boris.ostrovsky, xen-devel
>>> On 31.05.17 at 16:29, <roger.pau@citrix.com> wrote:
> On Tue, May 30, 2017 at 04:01:00AM -0600, Jan Beulich wrote:
>> >>> On 17.05.17 at 17:15, <roger.pau@citrix.com> wrote:
>> > --- a/xen/arch/x86/hvm/irq.c
>> > +++ b/xen/arch/x86/hvm/irq.c
>> > @@ -126,6 +126,49 @@ void hvm_pci_intx_deassert(
>> > spin_unlock(&d->arch.hvm_domain.irq_lock);
>> > }
>> >
>> > +void hvm_gsi_assert(struct domain *d, unsigned int gsi)
>> > +{
>> > + struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>> > +
>> > + if ( gsi >= hvm_irq->nr_gsis )
>> > + {
>> > + ASSERT_UNREACHABLE();
>> > + return;
>> > + }
>> > +
>> > + /*
>> > + * __hvm_pci_intx_{de}assert uses an array to track the status of each
>> > + * interrupt line, and Xen does the routing and GSI assertion based on
>> > + * that. This prevents the same line from triggering multiple times, which
>> > + * is not available here, and thus Xen needs to rely on gsi_assert_count in
>> > + * order to know if the GSI is pending or not.
>> > + */
>>
>> The "which is not available here" part is at least confusing. I'm not
>> even sure whether the "which" is supposed to refer to the array or
>> something else, because you use the exact same array here.
>
> I agree, it's confusing and badly worded, how about:
>
> __hvm_pci_intx_{de}assert uses a bitfield in pci_intx.i to track the
> status of each interrupt line, and Xen does the routing and GSI
> assertion based on that. The value of the pci_intx.i bitmap prevents
> the same line from triggering multiple times, which is not available
> here, and thus Xen needs to rely on gsi_assert_count in order to know
> if the GSI is pending or not.
Well, it's better, but the "which is not available here" still leaves
open what the "which" refers to. How about "The value of the
pci_intx.i bitmap prevents the same line from triggering multiple
times. As we don't use that bitmap for the hardware domain, Xen
nneds to ..."?
>> > --- a/xen/drivers/passthrough/io.c
>> > +++ b/xen/drivers/passthrough/io.c
>> > @@ -164,6 +164,20 @@ static void pt_irq_time_out(void *data)
>> >
>> > spin_lock(&irq_map->dom->event_lock);
>> >
>> > + if ( irq_map->flags & HVM_IRQ_DPCI_IDENTITY_GSI )
>> > + {
>> > + struct pirq *pirq = dpci_pirq(irq_map);
>> > +
>> > + ASSERT(is_hardware_domain(irq_map->dom));
>> > + /*
>> > + * Identity mapped, no need to iterate over the guest GSI list to find
>> > + * other pirqs sharing the same guest GSI.
>> > + */
>> > + irq_map->flags |= HVM_IRQ_DPCI_EOI_LATCH;
>> > + hvm_gsi_deassert(irq_map->dom, pirq->pirq);
>> > + goto out;
>> > + }
>> > +
>> > dpci = domain_get_irq_dpci(irq_map->dom);
>> > if ( unlikely(!dpci) )
>> > {
>> > @@ -185,6 +199,7 @@ static void pt_irq_time_out(void *data)
>> > hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx);
>> > }
>> >
>> > + out:
>> > pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL);
>>
>> With the 1:1 mapping, do you really need to go through
>> pt_pirq_iterate() here? I.e. can't you invoke pt_irq_guest_eoi()
>> just once and be done? Or really it probably can be even more
>> straight, as then there also is no point in setting the
>> HVM_IRQ_DPCI_EOI_LATCH flag, but you could rather do
>> directly what pt_irq_guest_eoi() does in its if() body. Otoh I may
>> be missing something here, as I can't see why the code is using
>> pt_pirq_iterate() even before your change.
>
> I have to admit this is not obviously clear to me (or I'm also missing
> something), there are too many translation layers and indirections in
> this code, together with a complete lack of comments.
>
> Previous to my change, pt_irq_time_out iterates over the list of guest
> devices (digl) bound to a pirq, desserts the interrupt lines and marks
> all the pirqs bound to the same guest GSI with the EOI_LATCH flag.
> Finally pt_irq_time_out iterates over the list of guest pirqs and
> clears (EOI) all the ones marked as EOI_LATCH.
>
> I don't really understand the usefulness of the EOI_LATCH flag, can't
> pt_irq_time_out just call pt_irq_guest_eoi and avoid the iteration?
> Something like:
>
> list_for_each_entry ( digl, &irq_map->digl_list, list )
> {
> unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx);
> const struct hvm_girq_dpci_mapping *girq;
>
> list_for_each_entry ( girq, &dpci->girq[guest_gsi], list )
> {
> struct pirq *pirq = pirq_info(irq_map->dom, girq->machine_gsi);
>
> pirq_dpci(pirq)->masked = 0;
> pirq_dpci(pirq)->pending = 0;
> pirq_guest_eoi(dpci_pirq(pirq));
> }
> hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx);
> }
>
> I can of course also do something similar for the identity mapping
> case.
Well, that's basically what I did outline, yes. We may need to do
some archeology here to figure out whether once the more indirect
mechanism was really needed, but the need disappeared without
it being noticed. I don't think the original author(s) of the code
would still be around to ask, but I didn't check (yet) who they are.
>> > @@ -472,7 +510,27 @@ int pt_irq_create_bind(
>> > pirq_dpci->flags = HVM_IRQ_DPCI_MAPPED |
>> > HVM_IRQ_DPCI_MACH_PCI |
>> > HVM_IRQ_DPCI_GUEST_PCI;
>> > - share = BIND_PIRQ__WILL_SHARE;
>> > + if ( !is_hardware_domain(d) )
>> > + share = BIND_PIRQ__WILL_SHARE;
>> > + else
>> > + {
>> > + unsigned int pin;
>> > + struct hvm_vioapic *vioapic = gsi_vioapic(d, guest_gsi,
>> > + &pin);
>> > +
>> > + if ( !vioapic )
>> > + {
>> > + ASSERT_UNREACHABLE();
>> > + return -EINVAL;
>> > + }
>> > + pirq_dpci->flags |= HVM_IRQ_DPCI_IDENTITY_GSI;
>> > + /*
>> > + * Check if the corresponding vIO APIC pin is configured
>> > + * level or edge trigger, level triggered interrupts will
>> > + * be marked as shareable.
>> > + */
>> > + share = vioapic->redirtbl[pin].fields.trig_mode;
>>
>> As pointed out during prior review (perhaps of another patch of
>> yours) the trigger mode bit is meaningless for masked RTEs. At
>> the very least an ASSERT() needs to be here for that reason,
>> of course provided masked entries can never be seen here.
>
> pt_irq_create_bind for GSIs will only be called when the hardware
> domain unmasks the RTE, so an ASSERT is the right choice IMHO.
Yes, I think I did see this in later code, so an ASSERT() it is then.
>> > @@ -489,9 +547,15 @@ int pt_irq_create_bind(
>> > * IRQ_GUEST is not set. As such we can reset 'dom' directly.
>> > */
>> > pirq_dpci->dom = NULL;
>> > - list_del(&girq->list);
>> > - list_del(&digl->list);
>> > - hvm_irq_dpci->link_cnt[link]--;
>> > + if ( !is_hardware_domain(d) )
>>
>> To be honest I'd prefer if you checked digl and/or girq against NULL
>> here, to avoid someone updating the condition above without
>> updating this one in lock step.
>
> I've changed the condition to "girq && digl".
How about using || and then ASSERT()ing && inside the if() body?
>> > @@ -638,11 +712,15 @@ int pt_irq_destroy_bind(
>> > if ( what && iommu_verbose )
>> > {
>> > unsigned int device = pt_irq_bind->u.pci.device;
>> > + char buf[24] = "";
>> >
>> > - printk(XENLOG_G_INFO
>> > - "d%d %s unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n",
>> > - d->domain_id, what, machine_gsi, pt_irq_bind->u.pci.bus,
>> > - PCI_SLOT(device), PCI_FUNC(device), pt_irq_bind->u.pci.intx);
>> > + if ( !is_hardware_domain(d) )
>> > + snprintf(buf, ARRAY_SIZE(buf), " dev=%02x.%02x.%u intx=%u",
>> > + pt_irq_bind->u.pci.bus, PCI_SLOT(device),
>> > + PCI_FUNC(device), pt_irq_bind->u.pci.intx);
>>
>> Now that this supports Dom0, you also need to log the segment.
>
> I'm not sure I follow you here, for the Dom case all the fields in
> u.pci.* are unused, since Xen does an identity mapping of the GSI, but
> it doesn't know to which device it belongs. That's different for the
> MSI case, but then this fields are not used anyway.
Oh, right, sorry for the noise.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 for-next 3/3] x86/vioapic: bind interrupts to PVH Dom0
2017-05-31 14:48 ` Roger Pau Monne
@ 2017-06-01 7:08 ` Jan Beulich
0 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2017-06-01 7:08 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, boris.ostrovsky, xen-devel
>>> On 31.05.17 at 16:48, <roger.pau@citrix.com> wrote:
> On Tue, May 30, 2017 at 04:05:39AM -0600, Jan Beulich wrote:
>> >>> On 17.05.17 at 17:15, <roger.pau@citrix.com> wrote:
>> > Changes since v2:
>> > - s/vioapic_dom0_map_gsi/vioapic_hwdom_map_gsi/.
>> > - Don't set hvm_domid in xen_domctl_bind_pt_irq_t (it's ignored).
>>
>> The implication of the respective earlier comment was for there to
>> first be a prereq patch added removing this dead field. Otherwise
>> not setting the field is a latent bug.
>
> I've added a pre-patch to get rid of hvm_domid in the bind struct.
>
>> > --- a/xen/arch/x86/hvm/vioapic.c
>> > +++ b/xen/arch/x86/hvm/vioapic.c
>> > @@ -158,6 +158,52 @@ static int vioapic_read(
>> > return X86EMUL_OKAY;
>> > }
>> >
>> > +static int vioapic_hwdom_map_gsi(unsigned int gsi, unsigned int trig,
>> > + unsigned int pol)
>> > +{
>> > + struct domain *d = current->domain;
>> > + xen_domctl_bind_pt_irq_t pt_irq_bind = {
>> > + .irq_type = PT_IRQ_TYPE_PCI,
>> > + .machine_irq = gsi,
>>
>> Actually you still set the field, just that this is no implicit. Hence the
>> latent bug reduces to just the hwdom != Dom0 case, but anyway.
>
> What do you mean by implicit? Is that because I'm passing d to the
> bind function?
For one I see that me having misspelled thing ("is now implicit") may
have caused misunderstanding. But what I meant anyway was that
by omitting the initializer you implicitly initialize the field to zero,
which is correct if and only if Dom0 == hwdom. But the discussion is
moot with the field gone.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-06-01 7:09 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-17 15:15 [PATCH v3 for-next 0/3] x86/dpci: bind legacy PCI interrupts to PVHv2 Dom0 Roger Pau Monne
2017-05-17 15:15 ` [PATCH v3 for-next 1/3] x86/physdev: factor out the code to allocate and map a pirq Roger Pau Monne
2017-05-30 9:16 ` Jan Beulich
2017-05-31 12:53 ` Roger Pau Monne
2017-05-17 15:15 ` [PATCH v3 for-next 2/3] x86/pt: enable binding of GSIs to a PVH Dom0 Roger Pau Monne
2017-05-30 10:01 ` Jan Beulich
2017-05-31 14:29 ` Roger Pau Monne
2017-06-01 7:05 ` Jan Beulich
2017-05-17 15:15 ` [PATCH v3 for-next 3/3] x86/vioapic: bind interrupts to " Roger Pau Monne
2017-05-30 10:05 ` Jan Beulich
2017-05-31 14:48 ` Roger Pau Monne
2017-06-01 7:08 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.