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

[Peter Xu <peterx@redhat.com>]

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 <davidkiarie4@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.

>          }
> +
> +        kvm_irqchip_update_msi_route(kvm_state, i, msg, NULL, s->devid);

What is this line used for?

> +        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.

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