On Wed, Aug 10, 2016 at 8:41 AM, Peter Xu <pet...@redhat.com> wrote: > On Tue, Aug 09, 2016 at 05:32:16PM +0300, David Kiarie wrote: > > When using IOMMU platform devices like IOAPIC are required to make > > interrupt remapping requests using explicit SID.We affiliate an MSI > > route with a requester ID and a PCI device if present which ensures > > that platform devices can call IOMMU interrupt remapping code with > > explicit SID while maintaining compatility with the original code > > which mainly dealt with PCI devices. > > > > Signed-off-by: David Kiarie <davidkiar...@gmail.com> > > Hi, > > This idea is good to me overall, with some tiny comments below. > > [...] > > > -static void ioapic_service(IOAPICCommonState *s) > > +static void ioapic_write_ioapic_as(IOAPICCommonState *s, uint32_t > data, uint64_t addr) > > Rename to ioapic_as_write()? > > [...] > > > @@ -385,12 +393,23 @@ static void ioapic_machine_done_notify(Notifier > *notifier, void *data) > > > > if (kvm_irqchip_is_split()) { > > X86IOMMUState *iommu = x86_iommu_get_default(); > > + MSIMessage msg = {0, 0}; > > + int i; > > + > > if (iommu) { > > /* Register this IOAPIC with IOMMU IEC notifier, so that > > * when there are IR invalidates, we can be notified to > > * update kernel IR cache. */ > > - x86_iommu_iec_register_notifier(iommu, > ioapic_iec_notifier, s); > > + s->devid = iommu->ioapic_bdf; > > + /* update IOAPIC routes to the right SID */ > > + for (i = 0; i < IOAPIC_NUM_PINS; i++) { > > + kvm_irqchip_update_msi_route(kvm_state, i, msg, NULL, > s->devid); > > + } > > + kvm_irqchip_commit_routes(kvm_state); > > Here, not sure whether it'll be better if we remove > kvm_irqchip_add_msi_route() in kvm_arch_init_irq_routing() directly > and call them here. So no extra update needed. >
Thought about that too but I was worried another device might reserve routes before IOAPIC does. > > > } > > + > > + kvm_irqchip_update_msi_route(kvm_state, i, msg, NULL, > s->devid); > > What is this line used for? This one shouldn't be here. It got left over. > > > + x86_iommu_iec_register_notifier(iommu, ioapic_iec_notifier, s); > > } > > #endif > > } > > @@ -407,6 +426,7 @@ static void ioapic_realize(DeviceState *dev, Error > **errp) > > > > memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s, > > "ioapic", 0x1000); > > + s->devid = 0; > > Nit: We can remove this line. > > [...] > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index 755f921..54e27fc 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -705,7 +705,8 @@ static int kvm_virtio_pci_vq_vector_use(VirtIOPCIProxy > *proxy, > > int ret; > > > > if (irqfd->users == 0) { > > - ret = kvm_irqchip_add_msi_route(kvm_state, vector, > &proxy->pci_dev); > > + ret = kvm_irqchip_add_msi_route(kvm_state, vector, > &proxy->pci_dev, > > + pci_requester_id(&proxy->pci_dev)); > > if (ret < 0) { > > return ret; > > } > > @@ -838,7 +839,8 @@ static int virtio_pci_vq_vector_unmask(VirtIOPCIProxy > *proxy, > > irqfd = &proxy->vector_irqfd[vector]; > > if (irqfd->msg.data != msg.data || irqfd->msg.address != > msg.address) { > > ret = kvm_irqchip_update_msi_route(kvm_state, irqfd->virq, > msg, > > - &proxy->pci_dev); > > + &proxy->pci_dev, > > Nit: Here you changed indentation, I would suggest keep it, as well in > the next line. > > > + pci_requester_id(&proxy->pci_ > dev)); > > if (ret < 0) { > > return ret; > > } > > diff --git a/include/hw/i386/ioapic_internal.h b/include/hw/i386/ioapic_ > internal.h > > index a11d86d..d68a24f 100644 > > --- a/include/hw/i386/ioapic_internal.h > > +++ b/include/hw/i386/ioapic_internal.h > > @@ -103,6 +103,7 @@ typedef struct IOAPICCommonClass { > > struct IOAPICCommonState { > > SysBusDevice busdev; > > MemoryRegion io_memory; > > + uint16_t devid; > > uint8_t id; > > uint8_t ioregsel; > > uint32_t irr; > > diff --git a/include/hw/i386/x86-iommu.h b/include/hw/i386/x86-iommu.h > > index c48e8dd..19454e0 100644 > > --- a/include/hw/i386/x86-iommu.h > > +++ b/include/hw/i386/x86-iommu.h > > @@ -66,6 +66,7 @@ typedef struct IEC_Notifier IEC_Notifier; > > > > struct X86IOMMUState { > > SysBusDevice busdev; > > + uint16_t ioapic_bdf; /* expected IOAPIC SID */ > > If we do not init ioapic_bdf in this patch, I think it should break > system boot with IR? I'd suggest introduce ioapic_bdf with meaningful > value in the first patch. > Thanks, I will. > > Also, when you send the formal patches, please don't forget to CC > Paolo since he is the maintainer for irqchips and kvm stuffs. > > -- peterx >