Re: [Qemu-ppc] [Qemu-devel] [PATCH] RFCv3 kvm irqfd: support msimessage to irq translation in PHB

[Anthony Liguori]

On Sat, Jun 29, 2013 at 8:45 AM, Alexey Kardashevskiy <address@hidden> wrote:
> On PPC64 systems MSI Messages are translated to system IRQ in a PCI
> host bridge. This is already supported for emulated MSI/MSIX but
> not for irqfd where the current QEMU allocates IRQ numbers from
> irqchip and maps MSIMessages to those IRQ in the host kernel.
>
> The patch extends irqfd support in order to avoid unnecessary
> mapping and reuse the one which already exists in a PCI host bridge.
>
> Specifically, a map_msi callback is added to PCIBus and pci_bus_map_msi()
> to PCI API. The latter returns -1 if a specific PHB does not provide
> with any trsnslation so the existing code will work.

I think there's a bit of confusion here.  The kernel needs a "virq"
number to create an eventfd.  virq is just a KVM concept, it doesn't
correspond to anything useful in hardware.

On pseries, there is a 1-1 mapping between XICS IRQs and VIRQs and MSI
can be trivially mapped to a virq.

On x86, we need to call a special kernel function which essentially
creates an apic message->virq mapping such that we can deliver the
irqfd.

So what this should look like is:

1) A PCI bus function to do the MSI -> virq mapping
2) On x86 (and e500), this is implemented by calling kvm_irqchip_add_msi_route()
3) On pseries, this just returns msi->data

Perhaps (2) can just be the default PCI bus implementation to simplify things.

> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>
> ---
>
> Looks like we agreed that in general PHB is the right place for this,
> not KVM, so I am trying again.
>
> Probably something should be done to kvm_irqchip_update_msi_route()
> as well but I do not really understand what exactly. Any suggestions?
>
>
> ---
>  hw/misc/vfio.c           |    7 +++++--
>  hw/pci/pci.c             |   13 +++++++++++++
>  hw/ppc/spapr_pci.c       |    6 ++++++
>  hw/virtio/virtio-pci.c   |    2 +-
>  include/hw/pci/pci.h     |    4 ++++
>  include/hw/pci/pci_bus.h |    1 +
>  include/sysemu/kvm.h     |    2 +-
>  kvm-all.c                |    7 ++++++-
>  8 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index 52fb036..59911bb 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -624,7 +624,9 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, 
> unsigned int nr,
>       * Attempt to enable route through KVM irqchip,
>       * default to userspace handling if unavailable.
>       */
> -    vector->virq = msg ? kvm_irqchip_add_msi_route(kvm_state, *msg) : -1;
> +
> +    vector->virq = msg ?
> +            kvm_irqchip_add_msi_route(kvm_state, vdev->pdev.bus, *msg) : -1;

This is wrong.  You could call the bus function to map an MSI message
to a virq here.

>      if (vector->virq < 0 ||
>          kvm_irqchip_add_irqfd_notifier(kvm_state, &vector->interrupt,
>                                         vector->virq) < 0) {
> @@ -792,7 +794,8 @@ retry:
>           * Attempt to enable route through KVM irqchip,
>           * default to userspace handling if unavailable.
>           */
> -        vector->virq = kvm_irqchip_add_msi_route(kvm_state, msg);
> +        vector->virq = kvm_irqchip_add_msi_route(kvm_state, vdev->pdev.bus,
> +                                                 msg);

And here.

>          if (vector->virq < 0 ||
>              kvm_irqchip_add_irqfd_notifier(kvm_state, &vector->interrupt,
>                                             vector->virq) < 0) {
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 61b681a..543f172 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1240,6 +1240,19 @@ void pci_device_set_intx_routing_notifier(PCIDevice 
> *dev,
>      dev->intx_routing_notifier = notifier;
>  }
>
> +void pci_bus_set_map_msi_fn(PCIBus *bus, pci_map_msi_fn map_msi_fn)
> +{
> +    bus->map_msi = map_msi_fn;
> +}

You don't need this function.  You can do this overloading as part of
the PCI bus initialization in spapr_pci.c

Regards,

Anthony Liguori

> +int pci_bus_map_msi(PCIBus *bus, MSIMessage msg)
> +{
> +    if (bus->map_msi) {
> +        return bus->map_msi(bus, msg);
> +    }
> +    return -1;
> +}
> +
>  /*
>   * PCI-to-PCI bridge specification
>   * 9.1: Interrupt routing. Table 9-1
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 23dbc0e..bae4faf 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -486,6 +486,11 @@ static void spapr_msi_write(void *opaque, hwaddr addr,
>      qemu_irq_pulse(xics_get_qirq(spapr->icp, irq));
>  }
>
> +static int spapr_msi_get_irq(PCIBus *bus, MSIMessage msg)
> +{
> +    return msg.data;
> +}
> +
>  static const MemoryRegionOps spapr_msi_ops = {
>      /* There is no .read as the read result is undefined by PCI spec */
>      .read = NULL,
> @@ -657,6 +662,7 @@ static int spapr_phb_init(SysBusDevice *s)
>
>          sphb->lsi_table[i].irq = irq;
>      }
> +    pci_bus_set_map_msi_fn(bus, spapr_msi_get_irq);
>
>      return 0;
>  }
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index b070b64..06a4e13 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -481,7 +481,7 @@ static int kvm_virtio_pci_vq_vector_use(VirtIOPCIProxy 
> *proxy,
>      int ret;
>
>      if (irqfd->users == 0) {
> -        ret = kvm_irqchip_add_msi_route(kvm_state, msg);
> +        ret = kvm_irqchip_add_msi_route(kvm_state, proxy->pci_dev.bus, msg);
>          if (ret < 0) {
>              return ret;
>          }
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 6ef1f97..8c1edd6 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -332,6 +332,7 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev);
>  typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
>  typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
>  typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
> +typedef int (*pci_map_msi_fn)(PCIBus *bus, MSIMessage msg);
>
>  typedef enum {
>      PCI_HOTPLUG_DISABLED,
> @@ -375,6 +376,9 @@ bool pci_intx_route_changed(PCIINTxRoute *old, 
> PCIINTxRoute *new);
>  void pci_bus_fire_intx_routing_notifier(PCIBus *bus);
>  void pci_device_set_intx_routing_notifier(PCIDevice *dev,
>                                            PCIINTxRoutingNotifier notifier);
> +void pci_bus_set_map_msi_fn(PCIBus *bus, pci_map_msi_fn map_msi_fn);
> +int pci_bus_map_msi(PCIBus *bus, MSIMessage msg);
> +
>  void pci_device_reset(PCIDevice *dev);
>  void pci_bus_reset(PCIBus *bus);
>
> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> index 66762f6..81efd2b 100644
> --- a/include/hw/pci/pci_bus.h
> +++ b/include/hw/pci/pci_bus.h
> @@ -16,6 +16,7 @@ struct PCIBus {
>      pci_set_irq_fn set_irq;
>      pci_map_irq_fn map_irq;
>      pci_route_irq_fn route_intx_to_irq;
> +    pci_map_msi_fn map_msi;
>      pci_hotplug_fn hotplug;
>      DeviceState *hotplug_qdev;
>      void *irq_opaque;
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index f404d16..1bf2abe 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -305,8 +305,8 @@ static inline void cpu_synchronize_post_init(CPUState 
> *cpu)
>      }
>  }
>
> -int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg);
>  int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg);
> +int kvm_irqchip_add_msi_route(KVMState *s, PCIBus *pbus, MSIMessage msg);
>  void kvm_irqchip_release_virq(KVMState *s, int virq);
>
>  int kvm_irqchip_add_irqfd_notifier(KVMState *s, EventNotifier *n, int virq);
> diff --git a/kvm-all.c b/kvm-all.c
> index 1f81cca..3b7710d 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1180,11 +1180,16 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
>      return kvm_set_irq(s, route->kroute.gsi, 1);
>  }
>
> -int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
> +int kvm_irqchip_add_msi_route(KVMState *s, PCIBus *pbus, MSIMessage msg)
>  {
>      struct kvm_irq_routing_entry kroute;
>      int virq;
>
> +    virq = pci_bus_map_msi(pbus, msg);
> +    if (virq >= 0) {
> +        return virq;
> +    }
> +
>      if (!kvm_gsi_routing_enabled()) {
>          return -ENOSYS;
>      }
> --
> 1.7.10.4
>
>