Re: [Qemu-devel] [RFC 1/2] hw/msi: Allow platform devices to use explicit SID

[David Kiarie]

On Wed, Aug 10, 2016 at 8:41 AM, Peter Xu <address@hidden> 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 <address@hidden>
>
> 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
>