Switch the dpci GSI EOI callback hooks to use the newly introduced
generic callback functionality, and remove the custom dpci calls found
on the vPIC and vIO-APIC implementations.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
- New in this version.
---
xen/arch/x86/hvm/vioapic.c | 7 ------
xen/arch/x86/hvm/vpic.c | 2 --
xen/drivers/passthrough/io.c | 41 ++++++++++++++++++++++++++++++++---
xen/include/asm-x86/hvm/io.h | 1 -
xen/include/asm-x86/hvm/irq.h | 1 +
5 files changed, 39 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 03b1350c04..ea4d60d33e 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -400,13 +400,6 @@ static void eoi_callback(unsigned int vector, void *data)
ent->fields.remote_irr = 0;
- if ( is_iommu_enabled(d) )
- {
- spin_unlock(&d->arch.hvm.irq_lock);
- hvm_dpci_eoi(gsi, ent);
- spin_lock(&d->arch.hvm.irq_lock);
- }
-
spin_unlock(&d->arch.hvm.irq_lock);
hvm_gsi_execute_callbacks(gsi, ent);
spin_lock(&d->arch.hvm.irq_lock);
diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
index 09c937c322..3c01c638fa 100644
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -266,8 +266,6 @@ static void vpic_ioport_write(
hvm_gsi_execute_callbacks(
hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : pin),
NULL);
- hvm_dpci_eoi(hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : pin),
- NULL);
return; /* bail immediately */
case 6: /* Set Priority */
diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index 770a5cce6b..6908438a94 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -327,9 +327,10 @@ static void hvm_gsi_eoi(struct domain *d, unsigned int gsi,
hvm_pirq_eoi(pirq, ent);
}
-void hvm_dpci_eoi(unsigned int guest_gsi, const union vioapic_redir_entry *ent)
+static void dpci_eoi(unsigned int guest_gsi, void *data)
{
struct domain *d = current->domain;
+ const union vioapic_redir_entry *ent = data;
const struct hvm_irq_dpci *hvm_irq_dpci;
const struct hvm_girq_dpci_mapping *girq;
@@ -565,7 +566,7 @@ int pt_irq_create_bind(
unsigned int link;
digl = xmalloc(struct dev_intx_gsi_link);
- girq = xmalloc(struct hvm_girq_dpci_mapping);
+ girq = xzalloc(struct hvm_girq_dpci_mapping);
if ( !digl || !girq )
{
@@ -578,11 +579,22 @@ int pt_irq_create_bind(
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);
+ girq->cb.callback = dpci_eoi;
guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx);
link = hvm_pci_intx_link(digl->device, digl->intx);
+ rc = hvm_gsi_register_callback(d, guest_gsi, &girq->cb);
+ if ( rc )
+ {
+ spin_unlock(&d->event_lock);
+ xfree(girq);
+ xfree(digl);
+ return rc;
+ }
+
+ list_add_tail(&digl->list, &pirq_dpci->digl_list);
+
hvm_irq_dpci->link_cnt[link]++;
girq->machine_gsi = pirq;
@@ -590,8 +602,17 @@ int pt_irq_create_bind(
}
else
{
+ struct hvm_gsi_eoi_callback *cb =
+ xzalloc(struct hvm_gsi_eoi_callback);
+
ASSERT(is_hardware_domain(d));
+ if ( !cb )
+ {
+ spin_unlock(&d->event_lock);
+ return -ENOMEM;
+ }
+
/* MSI_TRANSLATE is not supported for the hardware domain. */
if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_PCI ||
pirq >= hvm_domain_irq(d)->nr_gsis )
@@ -601,6 +622,19 @@ int pt_irq_create_bind(
return -EINVAL;
}
guest_gsi = pirq;
+
+ cb->callback = dpci_eoi;
+ /*
+ * IRQ binds created for the hardware domain are never destroyed,
+ * so it's fine to not keep a reference to cb here.
+ */
+ rc = hvm_gsi_register_callback(d, guest_gsi, cb);
+ if ( rc )
+ {
+ spin_unlock(&d->event_lock);
+ xfree(cb);
+ return rc;
+ }
}
/* Bind the same mirq once in the same domain */
@@ -789,6 +823,7 @@ int pt_irq_destroy_bind(
girq->machine_gsi == machine_gsi )
{
list_del(&girq->list);
+ hvm_gsi_unregister_callback(d, guest_gsi, &girq->cb);
xfree(girq);
girq = NULL;
break;
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index 759ee486af..e1bc556613 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -118,7 +118,6 @@ bool handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
struct npfec);
bool handle_pio(uint16_t port, unsigned int size, int dir);
void hvm_interrupt_post(struct vcpu *v, int vector, int type);
-void hvm_dpci_eoi(unsigned int guest_irq, const union vioapic_redir_entry *ent);
void msix_write_completion(struct vcpu *);
#ifdef CONFIG_HVM
diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-x86/hvm/irq.h
index db38c3e119..2f01d8fe64 100644
--- a/xen/include/asm-x86/hvm/irq.h
+++ b/xen/include/asm-x86/hvm/irq.h
@@ -158,6 +158,7 @@ struct hvm_girq_dpci_mapping {
uint8_t device;
uint8_t intx;
uint8_t machine_gsi;
+ struct hvm_gsi_eoi_callback cb;
};
#define NR_ISAIRQS 16
--
2.28.0
On 30.09.2020 12:41, Roger Pau Monne wrote:
> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/io.c
> @@ -327,9 +327,10 @@ static void hvm_gsi_eoi(struct domain *d, unsigned int gsi,
> hvm_pirq_eoi(pirq, ent);
> }
>
> -void hvm_dpci_eoi(unsigned int guest_gsi, const union vioapic_redir_entry *ent)
> +static void dpci_eoi(unsigned int guest_gsi, void *data)
> {
> struct domain *d = current->domain;
> + const union vioapic_redir_entry *ent = data;
> const struct hvm_irq_dpci *hvm_irq_dpci;
> const struct hvm_girq_dpci_mapping *girq;
>
> @@ -565,7 +566,7 @@ int pt_irq_create_bind(
> unsigned int link;
>
> digl = xmalloc(struct dev_intx_gsi_link);
> - girq = xmalloc(struct hvm_girq_dpci_mapping);
> + girq = xzalloc(struct hvm_girq_dpci_mapping);
>
> if ( !digl || !girq )
> {
> @@ -578,11 +579,22 @@ int pt_irq_create_bind(
> 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);
> + girq->cb.callback = dpci_eoi;
>
> guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx);
> link = hvm_pci_intx_link(digl->device, digl->intx);
>
> + rc = hvm_gsi_register_callback(d, guest_gsi, &girq->cb);
So this is where my question on the earlier patch gets answered:
You utilize passing NULL data to the callback to actually get
passed the IO-APIC redir entry pointer into the callback. This is
perhaps okay in principle if it was half way visible. May I ask
that at the very least instead of switching to xzalloc above you
set ->data to NULL here explicitly, accompanied by a comment on
the effect?
However, I wonder whether it wouldn't be better to have the
callback be passed const union vioapic_redir_entry * right away.
Albeit I haven't looked at the later patches yes, where it may
well be I'd find arguments against.
> @@ -590,8 +602,17 @@ int pt_irq_create_bind(
> }
> else
> {
> + struct hvm_gsi_eoi_callback *cb =
> + xzalloc(struct hvm_gsi_eoi_callback);
I can't seem to be able to spot anywhere that this would get freed
(except on an error path in this function).
> ASSERT(is_hardware_domain(d));
>
> + if ( !cb )
> + {
> + spin_unlock(&d->event_lock);
> + return -ENOMEM;
> + }
> +
> /* MSI_TRANSLATE is not supported for the hardware domain. */
> if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_PCI ||
> pirq >= hvm_domain_irq(d)->nr_gsis )
> @@ -601,6 +622,19 @@ int pt_irq_create_bind(
> return -EINVAL;
> }
There's an error path here where you don't free cb, and I think
one or two more further down (where you then also may need to
unregister it first).
Jan
© 2016 - 2026 Red Hat, Inc.