| Message ID | 1462568028-31037-2-git-send-email-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2016-05-06 22:53, Radim Kr?má? wrote: > The only way to send interrupts from outside of APIC devices is to use > the MSI-inspired memory mapped interface. The interface is not > sufficient because interrupt remapping in extended mode can exceed 8 bit > destination ids. > > The proper way to deliver interrupts would be to design a bus (QPI), but > that is a big undertaking. Real IR unit stores excessive bits in the > high work of MSI address register, which would be bit [63:40] in memory s/work/word/ > address space, so I was considering two options: > - a new special address space for APIC to handle MSI-compatible writes > - one callback to the APIC class that delivers extended MSIs > > This patch uses latter option, because it was easier for me, but I think > the former one could be a tad nicer. Makes sense. Eventually, we should also finally untangle the MSI DMA request handling from the xAPIC MMIO processing. The former could become pretty generic (instead of being reimplemented by each APIC flavour), just potentially redirected to an IOMMU when one is present. But that could come as separate patches. Jan
On Fri, May 06, 2016 at 10:53:45PM +0200, Radim Kr?má? wrote: > The only way to send interrupts from outside of APIC devices is to use > the MSI-inspired memory mapped interface. The interface is not > sufficient because interrupt remapping in extended mode can exceed 8 bit > destination ids. > > The proper way to deliver interrupts would be to design a bus (QPI), but > that is a big undertaking. Real IR unit stores excessive bits in the > high work of MSI address register, which would be bit [63:40] in memory > address space, so I was considering two options: > - a new special address space for APIC to handle MSI-compatible writes > - one callback to the APIC class that delivers extended MSIs > > This patch uses latter option, because it was easier for me, but I think > the former one could be a tad nicer. > > Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> But I have a suggestion below: > --- > hw/i386/kvm/apic.c | 21 ++++++++++++++------- > hw/i386/xen/xen_apic.c | 7 +++++++ > hw/intc/apic.c | 7 +++++++ > include/hw/i386/apic_internal.h | 5 +++++ > 4 files changed, 33 insertions(+), 7 deletions(-) > > diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c > index 3c7c8fa00709..7881f13abb96 100644 > --- a/hw/i386/kvm/apic.c > +++ b/hw/i386/kvm/apic.c > @@ -147,6 +147,17 @@ static void kvm_apic_external_nmi(APICCommonState *s) > run_on_cpu(CPU(s->cpu), do_inject_external_nmi, s); > } > > +static void kvm_deliver_msi(MSIMessage *msg) > +{ > + int ret; > + > + ret = kvm_irqchip_send_msi(kvm_state, *msg); > + if (ret < 0) { > + fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n", > + strerror(-ret)); > + } > +} > + > static uint64_t kvm_apic_mem_read(void *opaque, hwaddr addr, > unsigned size) > { > @@ -157,13 +168,7 @@ static void kvm_apic_mem_write(void *opaque, hwaddr addr, > uint64_t data, unsigned size) > { > MSIMessage msg = { .address = addr, .data = data }; > - int ret; > - > - ret = kvm_irqchip_send_msi(kvm_state, msg); > - if (ret < 0) { > - fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n", > - strerror(-ret)); > - } > + kvm_deliver_msi(&msg); > } You could call APICCommonClass::deliver_msi() here. Then (in a follow-up patch?) both kvm_apic_mem_write() and xen_apic_mem_write() could be replaced by a common function like: void apic_msi_write(void *opaque, hwaddr addr, uint64_t data, unsigned size) { APICommonState *s = opaque; APICCommonClass *k = APIC_COMMON_GET_CLASS(s); MSIMessage msg = { .address = addr, .data = data }; k->deliver_msi(&msg); } (apic_mem_writel() is more complex, but maybe it could call the common function instead of calling apic_send_msi() directly) > > static const MemoryRegionOps kvm_apic_io_ops = { > @@ -202,6 +207,8 @@ static void kvm_apic_class_init(ObjectClass *klass, void *data) > k->enable_tpr_reporting = kvm_apic_enable_tpr_reporting; > k->vapic_base_update = kvm_apic_vapic_base_update; > k->external_nmi = kvm_apic_external_nmi; > + > + k->deliver_msi = kvm_deliver_msi; > } > > static const TypeInfo kvm_apic_info = { > diff --git a/hw/i386/xen/xen_apic.c b/hw/i386/xen/xen_apic.c > index 21d68ee04b0a..0f34f63d449d 100644 > --- a/hw/i386/xen/xen_apic.c > +++ b/hw/i386/xen/xen_apic.c > @@ -68,6 +68,11 @@ static void xen_apic_external_nmi(APICCommonState *s) > { > } > > +static void xen_deliver_msi(MSIMessage *msi) > +{ > + xen_hvm_inject_msi(msi->address, msi->data); > +} > + > static void xen_apic_class_init(ObjectClass *klass, void *data) > { > APICCommonClass *k = APIC_COMMON_CLASS(klass); > @@ -78,6 +83,8 @@ static void xen_apic_class_init(ObjectClass *klass, void *data) > k->get_tpr = xen_apic_get_tpr; > k->vapic_base_update = xen_apic_vapic_base_update; > k->external_nmi = xen_apic_external_nmi; > + > + k->deliver_msi = xen_deliver_msi; > } > > static const TypeInfo xen_apic_info = { > diff --git a/hw/intc/apic.c b/hw/intc/apic.c > index 28c2ea540608..b893942c6e73 100644 > --- a/hw/intc/apic.c > +++ b/hw/intc/apic.c > @@ -877,6 +877,11 @@ static void apic_realize(DeviceState *dev, Error **errp) > msi_nonbroken = true; > } > > +static void apic_deliver_msi(MSIMessage *msi) > +{ > + apic_send_msi(msi->address, msi->data); > +} > + > static void apic_class_init(ObjectClass *klass, void *data) > { > APICCommonClass *k = APIC_COMMON_CLASS(klass); > @@ -889,6 +894,8 @@ static void apic_class_init(ObjectClass *klass, void *data) > k->external_nmi = apic_external_nmi; > k->pre_save = apic_pre_save; > k->post_load = apic_post_load; > + > + k->deliver_msi = apic_deliver_msi; > } > > static const TypeInfo apic_info = { > diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h > index 74fe935e8eab..227ef30c5027 100644 > --- a/include/hw/i386/apic_internal.h > +++ b/include/hw/i386/apic_internal.h > @@ -146,6 +146,11 @@ typedef struct APICCommonClass > void (*pre_save)(APICCommonState *s); > void (*post_load)(APICCommonState *s); > void (*reset)(APICCommonState *s); > + > + /* deliver_msi emulates an APIC bus and its proper place would be in a new > + * device, but it's convenient to have it here for now. > + */ > + void (*deliver_msi)(MSIMessage *msi); > } APICCommonClass; > > struct APICCommonState { > -- > 2.8.2 >
diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c index 3c7c8fa00709..7881f13abb96 100644 --- a/hw/i386/kvm/apic.c +++ b/hw/i386/kvm/apic.c @@ -147,6 +147,17 @@ static void kvm_apic_external_nmi(APICCommonState *s) run_on_cpu(CPU(s->cpu), do_inject_external_nmi, s); } +static void kvm_deliver_msi(MSIMessage *msg) +{ + int ret; + + ret = kvm_irqchip_send_msi(kvm_state, *msg); + if (ret < 0) { + fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n", + strerror(-ret)); + } +} + static uint64_t kvm_apic_mem_read(void *opaque, hwaddr addr, unsigned size) { @@ -157,13 +168,7 @@ static void kvm_apic_mem_write(void *opaque, hwaddr addr, uint64_t data, unsigned size) { MSIMessage msg = { .address = addr, .data = data }; - int ret; - - ret = kvm_irqchip_send_msi(kvm_state, msg); - if (ret < 0) { - fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n", - strerror(-ret)); - } + kvm_deliver_msi(&msg); } static const MemoryRegionOps kvm_apic_io_ops = { @@ -202,6 +207,8 @@ static void kvm_apic_class_init(ObjectClass *klass, void *data) k->enable_tpr_reporting = kvm_apic_enable_tpr_reporting; k->vapic_base_update = kvm_apic_vapic_base_update; k->external_nmi = kvm_apic_external_nmi; + + k->deliver_msi = kvm_deliver_msi; } static const TypeInfo kvm_apic_info = { diff --git a/hw/i386/xen/xen_apic.c b/hw/i386/xen/xen_apic.c index 21d68ee04b0a..0f34f63d449d 100644 --- a/hw/i386/xen/xen_apic.c +++ b/hw/i386/xen/xen_apic.c @@ -68,6 +68,11 @@ static void xen_apic_external_nmi(APICCommonState *s) { } +static void xen_deliver_msi(MSIMessage *msi) +{ + xen_hvm_inject_msi(msi->address, msi->data); +} + static void xen_apic_class_init(ObjectClass *klass, void *data) { APICCommonClass *k = APIC_COMMON_CLASS(klass); @@ -78,6 +83,8 @@ static void xen_apic_class_init(ObjectClass *klass, void *data) k->get_tpr = xen_apic_get_tpr; k->vapic_base_update = xen_apic_vapic_base_update; k->external_nmi = xen_apic_external_nmi; + + k->deliver_msi = xen_deliver_msi; } static const TypeInfo xen_apic_info = { diff --git a/hw/intc/apic.c b/hw/intc/apic.c index 28c2ea540608..b893942c6e73 100644 --- a/hw/intc/apic.c +++ b/hw/intc/apic.c @@ -877,6 +877,11 @@ static void apic_realize(DeviceState *dev, Error **errp) msi_nonbroken = true; } +static void apic_deliver_msi(MSIMessage *msi) +{ + apic_send_msi(msi->address, msi->data); +} + static void apic_class_init(ObjectClass *klass, void *data) { APICCommonClass *k = APIC_COMMON_CLASS(klass); @@ -889,6 +894,8 @@ static void apic_class_init(ObjectClass *klass, void *data) k->external_nmi = apic_external_nmi; k->pre_save = apic_pre_save; k->post_load = apic_post_load; + + k->deliver_msi = apic_deliver_msi; } static const TypeInfo apic_info = { diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h index 74fe935e8eab..227ef30c5027 100644 --- a/include/hw/i386/apic_internal.h +++ b/include/hw/i386/apic_internal.h @@ -146,6 +146,11 @@ typedef struct APICCommonClass void (*pre_save)(APICCommonState *s); void (*post_load)(APICCommonState *s); void (*reset)(APICCommonState *s); + + /* deliver_msi emulates an APIC bus and its proper place would be in a new + * device, but it's convenient to have it here for now. + */ + void (*deliver_msi)(MSIMessage *msi); } APICCommonClass; struct APICCommonState {
The only way to send interrupts from outside of APIC devices is to use the MSI-inspired memory mapped interface. The interface is not sufficient because interrupt remapping in extended mode can exceed 8 bit destination ids. The proper way to deliver interrupts would be to design a bus (QPI), but that is a big undertaking. Real IR unit stores excessive bits in the high work of MSI address register, which would be bit [63:40] in memory address space, so I was considering two options: - a new special address space for APIC to handle MSI-compatible writes - one callback to the APIC class that delivers extended MSIs This patch uses latter option, because it was easier for me, but I think the former one could be a tad nicer. Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com> --- hw/i386/kvm/apic.c | 21 ++++++++++++++------- hw/i386/xen/xen_apic.c | 7 +++++++ hw/intc/apic.c | 7 +++++++ include/hw/i386/apic_internal.h | 5 +++++ 4 files changed, 33 insertions(+), 7 deletions(-)