On 06/30/2013 12:28 AM, Anthony Liguori wrote:
> On Sat, Jun 29, 2013 at 8:45 AM, Alexey Kardashevskiy <a...@ozlabs.ru> 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.
Yes, it does not. But... There is a global IRQ number space and PHBs
convert MSIMessage to global IRQ, this is what our real hardware does on a
ppc64-pseries host (if I do not confuse things again). And I am trying to
follow the same principle here too.
> 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.
hw/pci/pci.c does not have any kvm code yet and I would like not to be the
first person who tries adding this there :)
But ok, I'll do it.
>> Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
>>
>> ---
>>
>> 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
pci_bus_set_route_irq_fn is there already and I tried to follow the
existing pattern (yeah, missed assert though). Or this is different?
>
> 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
>>
>>
--
Alexey
