Re: [Qemu-devel] Xen: enabling emulated MSI injection

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2011-05-26 09:35, Wei Liu wrote:
> On Thu, May 26, 2011 at 3:21 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> A cleaner way of hooking into this would be registering on the MSI MMIO
>> target region (replacing the APIC). That would give you MSI support as well.
>>
>> However, KVM has much more complex requirements for MSI handling when
>> using the in-kernel irqchip. That's because we need to support, e.g.,
>> eventfd-based MSI injection in the hypervisor without looping over QEMU,
>> ie. we need to translate a binary event into a MSI address/data tuple.
>> This is currently solved a bit too pragmatic in our qemu-kvm tree -
>> not mergeable to upstream. So I started working out a better plan,
>> partly based on ideas Avi provided.
>>
>> One element in this concept will be a hook into the MSI delivery path,
>> both for events triggered by msi/msix_notify as well as those raised by
>> weird DMA (to be architecturally correct). See below for a snapshot
>> (depends on other patches). That will obsolete any open-coded hooking
>> like this here.
>>
>> If Xen support is urging, I could try to break out the relevant bits a
>> bit earlier. Otherwise I would prefer to postpone the topic by a few
>> weeks until we are done with evaluating the concept against KVM's
>> requirements so that we can find a truly generic solution.
>>
>> Jan
>>
>>
>> ------8<-------
>>
>> diff --git a/hw/apic.c b/hw/apic.c
>> index a45b57f..7447d91 100644
>> --- a/hw/apic.c
>> +++ b/hw/apic.c
>> @@ -19,6 +19,7 @@
>>  #include "hw.h"
>>  #include "apic.h"
>>  #include "ioapic.h"
>> +#include "msi.h"
>>  #include "qemu-timer.h"
>>  #include "host-utils.h"
>>  #include "sysbus.h"
>> @@ -795,7 +796,7 @@ static uint32_t apic_mem_readl(void *opaque, target_phys_addr_t addr)
>>     return val;
>>  }
>>
>> -static void apic_send_msi(target_phys_addr_t addr, uint32_t data)
>> +void apic_deliver_msi(uint64_t addr, uint32_t data)
>>  {
>>     uint8_t dest = (addr & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT;
>>     uint8_t vector = (data & MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT;
>> @@ -817,7 +818,9 @@ static void apic_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>>          * APIC is connected directly to the CPU.
>>          * Mapping them on the global bus happens to work because
>>          * MSI registers are reserved in APIC MMIO and vice versa. */
>> -        apic_send_msi(addr, val);
>> +        MSIMessage msg = { .address = addr, .data = val };
>> +
>> +        msi_deliver(&msg);
>>         return;
>>     }
>>
>> diff --git a/hw/apic.h b/hw/apic.h
>> index c857d52..d0971ae 100644
>> --- a/hw/apic.h
>> +++ b/hw/apic.h
>> @@ -20,6 +20,7 @@ void cpu_set_apic_tpr(DeviceState *s, uint8_t val);
>>  uint8_t cpu_get_apic_tpr(DeviceState *s);
>>  void apic_init_reset(DeviceState *s);
>>  void apic_sipi(DeviceState *s);
>> +void apic_deliver_msi(uint64_t addr, uint32_t data);
>>
>>  /* pc.c */
>>  int cpu_is_bsp(CPUState *env);
>> diff --git a/hw/msi.c b/hw/msi.c
>> index e111ceb..b88dcc4 100644
>> --- a/hw/msi.c
>> +++ b/hw/msi.c
>> @@ -37,6 +37,14 @@
>>
>>  #define PCI_MSI_VECTORS_MAX     32
>>
>> +static void msi_unsupported(MSIMessage *msg)
>> +{
>> +    /* If we get here, the board failed to register a delivery handler. */
>> +    abort();
>> +}
>> +
>> +void (*msi_deliver)(MSIMessage *msg) = msi_unsupported;
>> +
>>  /* If we get rid of cap allocator, we won't need this. */
>>  static inline uint8_t msi_cap_sizeof(uint16_t flags)
>>  {
>> @@ -361,7 +369,7 @@ void msi_notify(PCIDevice *dev, unsigned int vector)
>>                    "notify vector 0x%x"
>>                    " address: 0x%"PRIx64" data: 0x%"PRIx32"\n",
>>                    vector, msg.address, msg.data);
>> -    stl_phys(msg.address, msg.data);
>> +    msi_deliver(&msg);
>>  }
>>
>>  /* call this function after updating configs by pci_default_write_config(). */
>> diff --git a/hw/msi.h b/hw/msi.h
>> index 7a0e76f..ba76eca 100644
>> --- a/hw/msi.h
>> +++ b/hw/msi.h
>> @@ -44,4 +44,6 @@ static inline bool msi_present(const PCIDevice *dev)
>>     return dev->cap_present & QEMU_PCI_CAP_MSI;
>>  }
>>
>> +extern void (*msi_deliver)(MSIMessage *msg);
>> +
>>  #endif /* QEMU_MSI_H */
>> diff --git a/hw/msix.c b/hw/msix.c
>> index 6e55422..d893f88 100644
>> --- a/hw/msix.c
>> +++ b/hw/msix.c
>> @@ -514,7 +514,7 @@ void msix_notify(PCIDevice *dev, unsigned vector)
>>
>>     msix_message_from_vector(dev, vector, &msg);
>>
>> -    stl_phys(msg.address, msg.data);
>> +    msi_deliver(&msg);
>>  }
>>
>>  void msix_reset(PCIDevice *dev)
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 73398eb..f88ef03 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -24,6 +24,7 @@
>>  #include "hw.h"
>>  #include "pc.h"
>>  #include "apic.h"
>> +#include "msi.h"
>>  #include "fdc.h"
>>  #include "ide.h"
>>  #include "pci.h"
>> @@ -102,6 +103,15 @@ void isa_irq_handler(void *opaque, int n, int level)
>>         qemu_set_irq(isa->ioapic[n], level);
>>  };
>>
>> +static void pc_msi_deliver(MSIMessage *msg)
>> +{
>> +    if ((msg->address & 0xfff00000) == MSI_ADDR_BASE) {
>> +        apic_deliver_msi(msg->address, msg->data);
>> +    } else {
>> +        stl_phys(msg->address, msg->data);
>> +    }
>> +}
>> +
>>  static void ioport80_write(void *opaque, uint32_t addr, uint32_t data)
>>  {
>>  }
>> @@ -889,6 +899,7 @@ static DeviceState *apic_init(void *env, uint8_t apic_id)
>>            on the global memory bus. */
>>         /* XXX: what if the base changes? */
>>         sysbus_mmio_map(d, 0, MSI_ADDR_BASE);
>> +        msi_deliver = pc_msi_deliver;
>>         apic_mapped = 1;
>>     }
>>
>> --
>> 1.7.1
>>
>> --
>> Siemens AG, Corporate Technology, CT T DE IT 1
>> Corporate Competence Center Embedded Linux
>>
> 
> Hmm... This patch is just part of GSoC project Virtio on Xen. AFAICS,
> this is not so urgent.

Ah, I see. The Xen hypervisor part is actually new as well.

> 
> I would prefer a cleaner way. A standalone function that doesn't
> belong to any QEMU subsystem annoys me. And the hooking looks ugly.
> 
> Thanks for your patch, I think we can wait for a better design and
> cleaner solution for both Xen and KVM.

Perfect.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux