qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] pseries: Properly handle allocat


From: David Gibson
Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] pseries: Properly handle allocation of multiple PCI host bridges
Date: Wed, 28 Nov 2012 13:50:08 +1100
User-agent: Mutt/1.5.21 (2010-09-15)

Alex,

If mst should happen to ack this in the next little while, please
don't merge it.  I've spotted a couple of little bugs with it.


On Tue, Nov 27, 2012 at 05:07:33PM +1100, David Gibson wrote:
> From: Alexey Kardashevskiy <address@hidden>
> 
> Multiple - even many - PCI host bridges (i.e. PCI domains) are very
> common on real PAPR compliant hardware.  For reasons related to the
> PAPR specified IOMMU interfaces, PCI device assignment with VFIO will
> generally require at least two (virtual) PHBs and possibly more
> depending on which devices are assigned.
> 
> At the moment the qemu PAPR PCI code will not deal with this well,
> leaving several crucial parameters of PHBs other than the default one
> uninitialized.  This patch reworks the code to allow this.
> 
> Every PHB needs a unique BUID (Bus Unit Identifier, the id used for
> the PAPR PCI related interfaces) and a unique LIOBN (Logical IO Bus
> Number, the id used for the PAPR IOMMU related interfaces).  In
> addition they need windows in CPU real address space to access PCI
> memory space, PCI IO space and MSIs.  Properties are added to the PCI
> host bridge qdevice to allow configuration of all these.
> 
> To simplify configuration of multiple PHBs for common cases, a
> convenience "index" property is also added.  This can be set instead
> of the low-level properties, and will generate suitable values for the
> other parameters, different for each index valu.
> 
> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/spapr.c     |   13 +-------
>  hw/spapr_pci.c |   97 
> +++++++++++++++++++++++++++++++++++++++++---------------
>  hw/spapr_pci.h |   21 ++++++++----
>  3 files changed, 87 insertions(+), 44 deletions(-)
> 
> diff --git a/hw/spapr.c b/hw/spapr.c
> index d23aa9d..1609f73 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -76,12 +76,6 @@
>  #define MAX_CPUS                256
>  #define XICS_IRQS               1024
>  
> -#define SPAPR_PCI_BUID          0x800000020000001ULL
> -#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
>  
>  #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
> @@ -854,12 +848,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>      /* Set up PCI */
>      spapr_pci_rtas_init();
>  
> -    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_MSI_WIN_ADDR);
> -    phb = PCI_HOST_BRIDGE(QLIST_FIRST(&spapr->phbs));
> +    phb = spapr_create_phb(spapr, 0);
>  
>      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 3c5b855..dc2ffaf 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -435,7 +435,7 @@ static void pci_spapr_set_irq(void *opaque, int irq_num, 
> int level)
>       */
>      sPAPRPHBState *phb = opaque;
>  
> -    trace_spapr_pci_lsi_set(phb->busname, irq_num, 
> phb->lsi_table[irq_num].irq);
> +    trace_spapr_pci_lsi_set(phb->dtbusname, irq_num, 
> phb->lsi_table[irq_num].irq);
>      qemu_set_irq(spapr_phb_lsi_qirq(phb, irq_num), level);
>  }
>  
> @@ -522,6 +522,58 @@ static int spapr_phb_init(SysBusDevice *s)
>      int i;
>      PCIBus *bus;
>  
> +    if (sphb->index != -1) {
> +        hwaddr windows_base;
> +
> +        if ((sphb->buid != -1) || (sphb->dma_liobn != -1)
> +            || (sphb->mem_win_addr != -1)
> +            || (sphb->io_win_addr != -1)
> +            || (sphb->msi_win_addr != -1)) {
> +            fprintf(stderr, "Either \"index\" or other parameters must"
> +                    " be specified for PAPR PHB, not both\n");
> +            return -1;
> +        }
> +
> +        sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
> +        sphb->dma_liobn = SPAPR_PCI_BASE_LIOBN + sphb->index;
> +
> +        windows_base = SPAPR_PCI_WINDOW_BASE
> +            + sphb->index * SPAPR_PCI_WINDOW_SPACING;
> +        sphb->mem_win_addr = windows_base + SPAPR_PCI_MMIO_WIN_OFF;
> +        sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF;
> +        sphb->msi_win_addr = windows_base + SPAPR_PCI_MSI_WIN_OFF;
> +    }
> +
> +    if (sphb->buid == -1) {
> +        fprintf(stderr, "BUID not specified for PHB\n");
> +        return -1;
> +    }
> +
> +    if (sphb->dma_liobn == -1) {
> +        fprintf(stderr, "LIOBN not specified for PHB\n");
> +        return -1;
> +    }
> +
> +    if (sphb->mem_win_addr == -1) {
> +        fprintf(stderr, "Memory window address not specified for PHB\n");
> +        return -1;
> +    }
> +
> +    if (sphb->io_win_addr == -1) {
> +        fprintf(stderr, "IO window address not specified for PHB\n");
> +        return -1;
> +    }
> +
> +    if (sphb->msi_win_addr == -1) {
> +        fprintf(stderr, "MSI window address not specified for PHB\n");
> +        return -1;
> +    }
> +
> +    if (find_phb(spapr, sphb->buid)) {
> +        fprintf(stderr, "PCI host bridges must have unique BUIDs\n");
> +        return -1;
> +    }               
> +
>      sphb->dtbusname = g_strdup_printf("address@hidden" PRIx64, sphb->buid);
>      namebuf = alloca(strlen(sphb->dtbusname) + 32);
>  
> @@ -565,17 +617,19 @@ static int spapr_phb_init(SysBusDevice *s)
>                                      &sphb->msiwindow);
>      }
>  
> -    bus = pci_register_bus(DEVICE(s),
> -                           sphb->busname ? sphb->busname : sphb->dtbusname,
> +    bus = pci_register_bus(DEVICE(s), sphb->dtbusname,
>                             pci_spapr_set_irq, pci_spapr_map_irq, sphb,
>                             &sphb->memspace, &sphb->iospace,
>                             PCI_DEVFN(0, 0), PCI_NUM_PINS);
>      phb->bus = bus;
>  
> -    sphb->dma_liobn = SPAPR_PCI_BASE_LIOBN | (pci_find_domain(bus) << 16);
>      sphb->dma_window_start = 0;
>      sphb->dma_window_size = 0x40000000;
>      sphb->dma = spapr_tce_new_dma_context(sphb->dma_liobn, 
> sphb->dma_window_size);
> +    if (!sphb->dma) {
> +        fprintf(stderr, "Unable to create TCE table for %s\n", 
> sphb->dtbusname);
> +        return -1;
> +    }
>      pci_setup_iommu(bus, spapr_pci_dma_context_fn, sphb);
>  
>      QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
> @@ -605,13 +659,16 @@ static void spapr_phb_reset(DeviceState *qdev)
>  }
>  
>  static Property spapr_phb_properties[] = {
> -    DEFINE_PROP_HEX64("buid", sPAPRPHBState, buid, 0),
> -    DEFINE_PROP_STRING("busname", sPAPRPHBState, busname),
> -    DEFINE_PROP_HEX64("mem_win_addr", sPAPRPHBState, mem_win_addr, 0),
> -    DEFINE_PROP_HEX64("mem_win_size", sPAPRPHBState, mem_win_size, 
> 0x20000000),
> -    DEFINE_PROP_HEX64("io_win_addr", sPAPRPHBState, io_win_addr, 0),
> -    DEFINE_PROP_HEX64("io_win_size", sPAPRPHBState, io_win_size, 0x10000),
> -    DEFINE_PROP_HEX64("msi_win_addr", sPAPRPHBState, msi_win_addr, 0),
> +    DEFINE_PROP_INT32("index", sPAPRPHBState, index, -1),
> +    DEFINE_PROP_HEX64("buid", sPAPRPHBState, buid, -1),
> +    DEFINE_PROP_HEX32("liobn", sPAPRPHBState, dma_liobn, -1),
> +    DEFINE_PROP_HEX64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
> +    DEFINE_PROP_HEX64("mem_win_size", sPAPRPHBState, mem_win_size,
> +                      SPAPR_PCI_MMIO_WIN_SIZE),
> +    DEFINE_PROP_HEX64("io_win_addr", sPAPRPHBState, io_win_addr, -1),
> +    DEFINE_PROP_HEX64("io_win_size", sPAPRPHBState, io_win_size,
> +                      SPAPR_PCI_IO_WIN_SIZE),
> +    DEFINE_PROP_HEX64("msi_win_addr", sPAPRPHBState, msi_win_addr, -1),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -632,25 +689,15 @@ static const TypeInfo spapr_phb_info = {
>      .class_init    = spapr_phb_class_init,
>  };
>  
> -void spapr_create_phb(sPAPREnvironment *spapr,
> -                      const char *busname, uint64_t buid,
> -                      uint64_t mem_win_addr, uint64_t mem_win_size,
> -                      uint64_t io_win_addr, uint64_t msi_win_addr)
> +PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
>  {
>      DeviceState *dev;
>  
>      dev = qdev_create(NULL, TYPE_SPAPR_PCI_HOST_BRIDGE);
> -
> -    if (busname) {
> -        qdev_prop_set_string(dev, "busname", g_strdup(busname));
> -    }
> -    qdev_prop_set_uint64(dev, "buid", buid);
> -    qdev_prop_set_uint64(dev, "mem_win_addr", mem_win_addr);
> -    qdev_prop_set_uint64(dev, "mem_win_size", mem_win_size);
> -    qdev_prop_set_uint64(dev, "io_win_addr", io_win_addr);
> -    qdev_prop_set_uint64(dev, "msi_win_addr", msi_win_addr);
> -
> +    qdev_prop_set_uint32(dev, "index", index);
>      qdev_init_nofail(dev);
> +
> +    return PCI_HOST_BRIDGE(dev);
>  }
>  
>  /* Macros to operate with address in OF binding to PCI */
> diff --git a/hw/spapr_pci.h b/hw/spapr_pci.h
> index e307ac8..76ba48a 100644
> --- a/hw/spapr_pci.h
> +++ b/hw/spapr_pci.h
> @@ -37,8 +37,8 @@
>  typedef struct sPAPRPHBState {
>      PCIHostState parent_obj;
>  
> +    int32_t index;
>      uint64_t buid;
> -    char *busname;
>      char *dtbusname;
>  
>      MemoryRegion memspace, iospace;
> @@ -64,18 +64,25 @@ typedef struct sPAPRPHBState {
>      QLIST_ENTRY(sPAPRPHBState) list;
>  } sPAPRPHBState;
>  
> +#define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
> +
> +#define SPAPR_PCI_WINDOW_BASE        0x10000000000ULL
> +#define SPAPR_PCI_WINDOW_SPACING     0x1000000000ULL
> +#define SPAPR_PCI_MMIO_WIN_OFF       0xA0000000
> +#define SPAPR_PCI_MMIO_WIN_SIZE      0x20000000
> +#define SPAPR_PCI_IO_WIN_OFF         0x80000000
> +#define SPAPR_PCI_IO_WIN_SIZE        0x10000
> +#define SPAPR_PCI_MSI_WIN_OFF        0x90000000
> +
> +#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
> +
>  static inline qemu_irq spapr_phb_lsi_qirq(struct sPAPRPHBState *phb, int pin)
>  {
>      return xics_get_qirq(spapr->icp, phb->lsi_table[pin].irq);
>  }
>  
> -#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
> -#define SPAPR_PCI_IO_WIN_SIZE        0x10000
>  
> -void spapr_create_phb(sPAPREnvironment *spapr,
> -                      const char *busname, uint64_t buid,
> -                      uint64_t mem_win_addr, uint64_t mem_win_size,
> -                      uint64_t io_win_addr, uint64_t msi_win_addr);
> +PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index);
>  
>  int spapr_populate_pci_dt(sPAPRPHBState *phb,
>                            uint32_t xics_phandle,

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson



reply via email to

[Prev in Thread] Current Thread [Next in Thread]