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 v3:
- Print a warning message if the EOI callback cannot be unregistered.
Changes since v2:
- Avoid leaking the allocated callback on error paths of
pt_irq_create_bind.
Changes since v1:
- New in this version.
---
xen/arch/x86/hvm/vioapic.c | 8 -----
xen/arch/x86/hvm/vpic.c | 4 ---
xen/drivers/passthrough/x86/hvm.c | 57 ++++++++++++++++++++++++++++---
xen/include/asm-x86/hvm/io.h | 1 -
xen/include/asm-x86/hvm/irq.h | 1 +
5 files changed, 54 insertions(+), 17 deletions(-)
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 0824ede91ab..4f844965423 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -284,7 +284,6 @@ static void vioapic_write_redirent(
*/
ASSERT(prev_level);
ASSERT(!top_word);
- hvm_dpci_eoi(d, gsi);
hvm_gsi_execute_callbacks(d, gsi);
}
@@ -418,13 +417,6 @@ static void eoi_callback(struct vcpu *v, 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(d, gsi);
- spin_lock(&d->arch.hvm.irq_lock);
- }
-
/*
* Callbacks don't expect to be executed with any lock held, so
* drop the lock that protects the vIO-APIC fields from changing.
diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
index ef668f3668a..60d6740f69a 100644
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -237,8 +237,6 @@ static void vpic_ioport_write(
ASSERT(pin < 8);
hvm_gsi_execute_callbacks(current->domain,
hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : pin));
- hvm_dpci_eoi(current->domain,
- hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : pin));
__clear_bit(pin, &pending);
}
return;
@@ -289,8 +287,6 @@ static void vpic_ioport_write(
vpic_unlock(vpic);
hvm_gsi_execute_callbacks(current->domain,
hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : pin));
- hvm_dpci_eoi(current->domain,
- hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : pin));
return; /* bail immediately */
case 6: /* Set Priority */
vpic->priority_add = (val + 1) & 7;
diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
index 0db26e5dbb2..02e027cff8c 100644
--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -252,7 +252,7 @@ static void hvm_gsi_eoi(struct domain *d, unsigned int gsi)
hvm_pirq_eoi(pirq);
}
-void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi)
+static void dpci_eoi(struct domain *d, unsigned int guest_gsi, void *data)
{
const struct hvm_irq_dpci *hvm_irq_dpci;
const struct hvm_girq_dpci_mapping *girq;
@@ -476,6 +476,7 @@ int pt_irq_create_bind(
{
struct dev_intx_gsi_link *digl = NULL;
struct hvm_girq_dpci_mapping *girq = NULL;
+ struct hvm_gsi_eoi_callback *cb = NULL;
unsigned int guest_gsi;
/*
@@ -489,7 +490,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 )
{
@@ -502,11 +503,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;
@@ -514,17 +526,43 @@ int pt_irq_create_bind(
}
else
{
+ /*
+ * NB: the callback structure allocated below will never be freed
+ * once setup because it's used by the hardware domain and will
+ * never be unregistered.
+ */
+ 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 )
{
spin_unlock(&d->event_lock);
-
+ xfree(cb);
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 */
@@ -596,12 +634,17 @@ int pt_irq_create_bind(
list_del(&digl->list);
link = hvm_pci_intx_link(digl->device, digl->intx);
hvm_irq_dpci->link_cnt[link]--;
+ hvm_gsi_unregister_callback(d, guest_gsi, &girq->cb);
}
+ else
+ hvm_gsi_unregister_callback(d, guest_gsi, cb);
+
pirq_dpci->flags = 0;
pirq_cleanup_check(info, d);
spin_unlock(&d->event_lock);
xfree(girq);
xfree(digl);
+ xfree(cb);
return rc;
}
}
@@ -699,6 +742,7 @@ int pt_irq_destroy_bind(
unsigned int link = hvm_pci_intx_link(device, intx);
struct hvm_girq_dpci_mapping *girq;
struct dev_intx_gsi_link *digl, *tmp;
+ int rc;
list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list )
{
@@ -708,6 +752,11 @@ int pt_irq_destroy_bind(
girq->machine_gsi == machine_gsi )
{
list_del(&girq->list);
+ rc = hvm_gsi_unregister_callback(d, guest_gsi, &girq->cb);
+ if ( rc )
+ printk(XENLOG_G_WARNING
+ "%pd: unable to remove callback for GSI %u: %d\n",
+ d, guest_gsi, rc);
xfree(girq);
girq = NULL;
break;
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index 7f30dfa7fea..977a857f729 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -101,7 +101,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(struct domain *d, unsigned int guest_irq);
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 37b8d2ba8fb..57d51c15863 100644
--- a/xen/include/asm-x86/hvm/irq.h
+++ b/xen/include/asm-x86/hvm/irq.h
@@ -156,6 +156,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.30.1
On 20.04.2021 16:07, Roger Pau Monne wrote:
> @@ -476,6 +476,7 @@ int pt_irq_create_bind(
> {
> struct dev_intx_gsi_link *digl = NULL;
> struct hvm_girq_dpci_mapping *girq = NULL;
> + struct hvm_gsi_eoi_callback *cb = NULL;
I wonder if this wouldn't benefit from a brief "hwdom only" comment.
> @@ -502,11 +503,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]++;
Could we keep calculation and use of "link" together, please, so the
compiler can avoid spilling the value to the stack or allocating a
callee-saved register for it?
> @@ -514,17 +526,43 @@ int pt_irq_create_bind(
> }
> else
> {
> + /*
> + * NB: the callback structure allocated below will never be freed
> + * once setup because it's used by the hardware domain and will
> + * never be unregistered.
> + */
> + cb = xzalloc(struct hvm_gsi_eoi_callback);
> +
> ASSERT(is_hardware_domain(d));
>
> + if ( !cb )
> + {
> + spin_unlock(&d->event_lock);
> + return -ENOMEM;
> + }
I'm inclined to ask that the ASSERT() remain first in this "else" block.
In fact, you could ...
> /* 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 )
> {
> spin_unlock(&d->event_lock);
> -
> + xfree(cb);
... avoid this extra cleanup by ...
> return -EINVAL;
> }
... putting the allocation here.
> 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);
In reply to a v3 comment of mine you said "I should replace IRQ with
GSI in the comment above to make it clearer." And while the question
of the comment being (and going to remain) true in the first place
was discussed, I would have hoped for the commit message to say a
word on this. If this ever changed, chances are the place here would
go unnoticed and unchanged, leading to a memory leak.
> @@ -596,12 +634,17 @@ int pt_irq_create_bind(
> list_del(&digl->list);
> link = hvm_pci_intx_link(digl->device, digl->intx);
> hvm_irq_dpci->link_cnt[link]--;
> + hvm_gsi_unregister_callback(d, guest_gsi, &girq->cb);
> }
> + else
> + hvm_gsi_unregister_callback(d, guest_gsi, cb);
> +
> pirq_dpci->flags = 0;
> pirq_cleanup_check(info, d);
> spin_unlock(&d->event_lock);
> xfree(girq);
> xfree(digl);
> + xfree(cb);
May I suggest that you move the xfree() into the "else" you add, and
perhaps even make it conditional upon the un-register being successful?
> @@ -708,6 +752,11 @@ int pt_irq_destroy_bind(
> girq->machine_gsi == machine_gsi )
> {
> list_del(&girq->list);
> + rc = hvm_gsi_unregister_callback(d, guest_gsi, &girq->cb);
> + if ( rc )
> + printk(XENLOG_G_WARNING
> + "%pd: unable to remove callback for GSI %u: %d\n",
> + d, guest_gsi, rc);
> xfree(girq);
If the un-registration really failed (here as well as further up),
is it safe to free girq?
Jan
© 2016 - 2026 Red Hat, Inc.