Re: [Qemu-devel] [PATCH 21/24] pseries: Add PCI MSI/MSI-X support

[Alexander Graf]

On 15.08.2012, at 19:19, Andreas Färber wrote:

> Am 15.08.2012 11:58, schrieb Alexander Graf:
>> From: Alexey Kardashevskiy <address@hidden>
>> 
>> This patch implements MSI and MSI-X support for the pseries PCI host
>> bridge.  To do this it adds:
>> 
>> * A "config_space_address to msi_table" map, since the MSI RTAS calls
>> take a PCI config space address as an identifier.
>> 
>> * A MSIX memory region to catch msi_notify()/msix_notiry() from
>> virtio-pci and pass them to the guest via qemu_irq_pulse().
>> 
>> * RTAS call "ibm,change-msi" which sets up MSI vectors for a
>> device. Note that this call may configure and return lesser number of
>> vectors than requested.
>> 
>> * RTAS call "ibm,query-interrupt-source-number" which translates MSI
>> vector to interrupt controller (XICS) IRQ number.
>> 
>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>> Signed-off-by: David Gibson <address@hidden>
>> Signed-off-by: Alexander Graf <address@hidden>
>> ---
>> hw/spapr.c     |    7 ++-
>> hw/spapr_pci.c |  245 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> hw/spapr_pci.h |   15 +++-
>> trace-events   |    5 +
>> 4 files changed, 268 insertions(+), 4 deletions(-)
>> 
>> diff --git a/hw/spapr.c b/hw/spapr.c
>> index afbdbc5..5178721 100644
>> --- a/hw/spapr.c
>> +++ b/hw/spapr.c
>> @@ -41,6 +41,7 @@
>> #include "hw/spapr_vio.h"
>> #include "hw/spapr_pci.h"
>> #include "hw/xics.h"
>> +#include "hw/msi.h"
>> 
>> #include "kvm.h"
>> #include "kvm_ppc.h"
>> @@ -79,6 +80,7 @@
>> #define SPAPR_PCI_MEM_WIN_ADDR  (0x10000000000ULL + 0xA0000000)
>> #define SPAPR_PCI_MEM_WIN_SIZE  0x20000000
>> #define SPAPR_PCI_IO_WIN_ADDR   (0x10000000000ULL + 0x80000000)
>> +#define SPAPR_PCI_MSI_WIN_ADDR  (0x10000000000ULL + 0x90000000)
>> 
>> #define PHANDLE_XICP            0x00001111
>> 
>> @@ -619,6 +621,8 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>     long pteg_shift = 17;
>>     char *filename;
>> 
>> +    msi_supported = true;
>> +
>>     spapr = g_malloc0(sizeof(*spapr));
>>     QLIST_INIT(&spapr->phbs);
>> 
>> @@ -735,7 +739,8 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>     spapr_create_phb(spapr, "pci", SPAPR_PCI_BUID,
>>                      SPAPR_PCI_MEM_WIN_ADDR,
>>                      SPAPR_PCI_MEM_WIN_SIZE,
>> -                     SPAPR_PCI_IO_WIN_ADDR);
>> +                     SPAPR_PCI_IO_WIN_ADDR,
>> +                     SPAPR_PCI_MSI_WIN_ADDR);
>> 
>>     for (i = 0; i < nb_nics; i++) {
>>         NICInfo *nd = &nd_table[i];
>> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
>> index 1eb1a7e..1108eea 100644
>> --- a/hw/spapr_pci.c
>> +++ b/hw/spapr_pci.c
>> @@ -24,6 +24,8 @@
>>  */
>> #include "hw.h"
>> #include "pci.h"
>> +#include "msi.h"
>> +#include "msix.h"
>> #include "pci_host.h"
>> #include "hw/spapr.h"
>> #include "hw/spapr_pci.h"
>> @@ -33,6 +35,17 @@
>> 
>> #include "hw/pci_internals.h"
>> 
>> +/* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
>> +#define RTAS_QUERY_FN           0
>> +#define RTAS_CHANGE_FN          1
>> +#define RTAS_RESET_FN           2
>> +#define RTAS_CHANGE_MSI_FN      3
>> +#define RTAS_CHANGE_MSIX_FN     4
>> +
>> +/* Interrupt types to return on RTAS_CHANGE_* */
>> +#define RTAS_TYPE_MSI           1
>> +#define RTAS_TYPE_MSIX          2
>> +
>> static sPAPRPHBState *find_phb(sPAPREnvironment *spapr, uint64_t buid)
>> {
>>     sPAPRPHBState *phb;
>> @@ -211,6 +224,191 @@ static void rtas_write_pci_config(sPAPREnvironment 
>> *spapr,
>>     finish_write_pci_config(spapr, 0, addr, size, val, rets);
>> }
>> 
>> +/*
>> + * Find an entry with config_addr or returns the empty one if not found AND
>> + * alloc_new is set.
>> + * At the moment the msi_table entries are never released so there is
>> + * no point to look till the end of the list if we need to find the free 
>> entry.
>> + */
>> +static int spapr_msicfg_find(sPAPRPHBState *phb, uint32_t config_addr,
>> +                             bool alloc_new)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < SPAPR_MSIX_MAX_DEVS; ++i) {
>> +        if (!phb->msi_table[i].nvec) {
>> +            break;
>> +        }
>> +        if (phb->msi_table[i].config_addr == config_addr) {
>> +            return i;
>> +        }
>> +    }
>> +    if ((i < SPAPR_MSIX_MAX_DEVS) && alloc_new) {
>> +        trace_spapr_pci_msi("Allocating new MSI config", i, config_addr);
>> +        return i;
>> +    }
>> +
>> +    return -1;
>> +}
>> +
>> +/*
>> + * Set MSI/MSIX message data.
>> + * This is required for msi_notify()/msix_notify() which
>> + * will write at the addresses via spapr_msi_write().
>> + */
>> +static void spapr_msi_setmsg(PCIDevice *pdev, target_phys_addr_t addr,
>> +                             bool msix, unsigned req_num)
>> +{
>> +    unsigned i;
>> +    MSIMessage msg = { .address = addr, .data = 0 };
>> +
>> +    if (!msix) {
>> +        msi_set_message(pdev, msg);
>> +        trace_spapr_pci_msi_setup(pdev->name, 0, msg.address);
>> +        return;
>> +    }
>> +
>> +    for (i = 0; i < req_num; ++i) {
>> +        msg.address = addr | (i << 2);
>> +        msix_set_message(pdev, i, msg);
>> +        trace_spapr_pci_msi_setup(pdev->name, i, msg.address);
>> +    }
>> +}
>> +
>> +static void rtas_ibm_change_msi(sPAPREnvironment *spapr,
>> +                                uint32_t token, uint32_t nargs,
>> +                                target_ulong args, uint32_t nret,
>> +                                target_ulong rets)
>> +{
>> +    uint32_t config_addr = rtas_ld(args, 0);
>> +    uint64_t buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>> +    unsigned int func = rtas_ld(args, 3);
>> +    unsigned int req_num = rtas_ld(args, 4); /* 0 == remove all */
>> +    unsigned int seq_num = rtas_ld(args, 5);
>> +    unsigned int ret_intr_type;
>> +    int ndev, irq;
>> +    sPAPRPHBState *phb = NULL;
>> +    PCIDevice *pdev = NULL;
>> +
>> +    switch (func) {
>> +    case RTAS_CHANGE_MSI_FN:
>> +    case RTAS_CHANGE_FN:
>> +        ret_intr_type = RTAS_TYPE_MSI;
>> +        break;
>> +    case RTAS_CHANGE_MSIX_FN:
>> +        ret_intr_type = RTAS_TYPE_MSIX;
>> +        break;
>> +    default:
>> +        fprintf(stderr, "rtas_ibm_change_msi(%u) is not implemented\n", 
>> func);
>> +        rtas_st(rets, 0, -3); /* Parameter error */
>> +        return;
>> +    }
>> +
>> +    /* Fins sPAPRPHBState */
>> +    phb = find_phb(spapr, buid);
>> +    if (phb) {
>> +        pdev = find_dev(spapr, buid, config_addr);
>> +    }
>> +    if (!phb || !pdev) {
>> +        rtas_st(rets, 0, -3); /* Parameter error */
>> +        return;
>> +    }
>> +
>> +    /* Releasing MSIs */
>> +    if (!req_num) {
>> +        ndev = spapr_msicfg_find(phb, config_addr, false);
>> +        if (ndev < 0) {
>> +            trace_spapr_pci_msi("MSI has not been enabled", -1, 
>> config_addr);
>> +            rtas_st(rets, 0, -1); /* Hardware error */
>> +            return;
>> +        }
>> +        trace_spapr_pci_msi("Released MSIs", ndev, config_addr);
>> +        rtas_st(rets, 0, 0);
>> +        rtas_st(rets, 1, 0);
>> +        return;
>> +    }
>> +
>> +    /* Enabling MSI */
>> +
>> +    /* Find a device number in the map to add or reuse the existing one */
>> +    ndev = spapr_msicfg_find(phb, config_addr, true);
>> +    if (ndev >= SPAPR_MSIX_MAX_DEVS) {
>> +        fprintf(stderr, "No free entry for a new MSI device\n");
>> +        rtas_st(rets, 0, -1); /* Hardware error */
>> +        return;
>> +    }
>> +    trace_spapr_pci_msi("Configuring MSI", ndev, config_addr);
>> +
>> +    /* Check if there is an old config and MSI number has not changed */
>> +    if (phb->msi_table[ndev].nvec && (req_num != 
>> phb->msi_table[ndev].nvec)) {
> 
> 
> Breaks the build on ppc-next using openSUSE's gcc 4.6.2:
> 
>  CC    ppc64-softmmu/hw/ppc/../spapr_pci.o
> /home/andreas/QEMU/qemu-ppc/hw/ppc/../spapr_pci.c: In function
> ‘rtas_ibm_change_msi’:
> /home/andreas/QEMU/qemu-ppc/hw/ppc/../spapr_pci.c:343:23: error: array
> subscript is below array bounds [-Werror=array-bounds]
> cc1: all warnings being treated as errors
> 
> ndev is declared as int above...

Ok, I made the return check also check for ndev < 0:

diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
index 1108eea..b92583a 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -332,7 +332,7 @@ static void rtas_ibm_change_msi(sPAPREnvironment *spapr,
 
     /* Find a device number in the map to add or reuse the existing one */
     ndev = spapr_msicfg_find(phb, config_addr, true);
-    if (ndev >= SPAPR_MSIX_MAX_DEVS) {
+    if (ndev >= SPAPR_MSIX_MAX_DEVS || ndev < 0) {
         fprintf(stderr, "No free entry for a new MSI device\n");
         rtas_st(rets, 0, -1); /* Hardware error */
         return;

and squashed that patch into the respective patch in ppc-next as well as the 
for-upstream branch. It should be safe to pull now.


Alex