| Message ID | 20210331103303.79705-9-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/intr: introduce EOI callbacks and fix vPT | expand |
On 31.03.2021 12:33, Roger Pau Monne wrote: > @@ -515,17 +528,44 @@ 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 = xmalloc(struct hvm_gsi_eoi_callback); Is this comment as well as ... > 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; > + cb->data = d; > + /* > + * 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); ... the one here really true? vpci_msi_arch_update() and vpci_msi_disable() seem to tell me otherwise (or for the former comment, they suggest there should be un-registration somewhere). It also doesn't seem logical to me, considering (yet to be made work) pass-through of devices or hot-unplugged ones, at which point Dom0 shouldn't retain IRQ bindings, I would think. Jan
On Thu, Apr 08, 2021 at 04:49:48PM +0200, Jan Beulich wrote: > On 31.03.2021 12:33, Roger Pau Monne wrote: > > @@ -515,17 +528,44 @@ 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 = xmalloc(struct hvm_gsi_eoi_callback); > > Is this comment as well as ... > > > 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; > > + cb->data = d; > > + /* > > + * 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); > > ... the one here really true? vpci_msi_arch_update() and > vpci_msi_disable() seem to tell me otherwise (or for the former > comment, they suggest there should be un-registration somewhere). MSI doesn't use hvm_gsi_register_callback at all, since those are only used for GSI interrupts. I should replace IRQ with GSI in the comment above to make it clearer. > It also doesn't seem logical to me, considering (yet to be made > work) pass-through of devices or hot-unplugged ones, at which > point Dom0 shouldn't retain IRQ bindings, I would think. Hm, maybe. I think we are still very far from that. Right now GSIs are bound to a PVH dom0 based on the unmasked vIO-APIC pins, and they are never unbound. We could see about unbinding them, but TBH I would expect a PVH dom0 to just mask the vIO-APIC pin when it has no devices using it if those are unplugged. Thanks, Roger.
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c index 099c29466ba..4cdb95ce835 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(gsi); hvm_gsi_execute_callbacks(gsi); } @@ -420,13 +419,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); - 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 ca484c31b6a..e0f3f6276dc 100644 --- a/xen/arch/x86/hvm/vpic.c +++ b/xen/arch/x86/hvm/vpic.c @@ -237,7 +237,6 @@ static void vpic_ioport_write( ASSERT(pin < 8); hvm_gsi_execute_callbacks( hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : pin)); - hvm_dpci_eoi(hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : pin)); __clear_bit(pin, &pending); } return; @@ -288,7 +287,6 @@ static void vpic_ioport_write( vpic_unlock(vpic); hvm_gsi_execute_callbacks( hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : pin)); - hvm_dpci_eoi(hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : pin)); return; /* bail immediately */ case 6: /* Set Priority */ diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c index ecc7d66e600..4ae678d69b4 100644 --- a/xen/drivers/passthrough/x86/hvm.c +++ b/xen/drivers/passthrough/x86/hvm.c @@ -252,9 +252,9 @@ static void hvm_gsi_eoi(struct domain *d, unsigned int gsi) hvm_pirq_eoi(pirq); } -void hvm_dpci_eoi(unsigned int guest_gsi) +static void dpci_eoi(unsigned int guest_gsi, void *data) { - struct domain *d = current->domain; + struct domain *d = data; const struct hvm_irq_dpci *hvm_irq_dpci; const struct hvm_girq_dpci_mapping *girq; @@ -477,6 +477,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; /* @@ -503,11 +504,23 @@ 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; + girq->cb.data = d; 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; @@ -515,17 +528,44 @@ 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 = xmalloc(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; + cb->data = d; + /* + * 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 */ @@ -597,12 +637,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; } } @@ -709,6 +754,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 9ac3e4f48f6..a05bdbe5555 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(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 0828c01dd18..f49c4c3b6e5 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
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 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 | 2 -- xen/drivers/passthrough/x86/hvm.c | 54 ++++++++++++++++++++++++++++--- xen/include/asm-x86/hvm/io.h | 1 - xen/include/asm-x86/hvm/irq.h | 1 + 5 files changed, 51 insertions(+), 15 deletions(-)