Re: [Qemu-devel] [PATCH qemu] spapr-pci: Make MMIO spacing a machine property and increase it

[David Gibson]

On Thu, Mar 03, 2016 at 12:42:53PM +1100, Alexey Kardashevskiy wrote:
> The pseries machine supports multiple PHBs. Each PHB's MMIO/IO space is
> mapped to the CPU address space starting at SPAPR_PCI_WINDOW_BASE plus
> some offset which is calculated from PHB's index and
> SPAPR_PCI_WINDOW_SPACING which is defined now as 64GB.
> 
> Since the default 32bit DMA window is using first 2GB of MMIO space,
> the amount of MMIO which the PCI devices can actually use is reduced
> to 62GB. This is a problem if the user wants to use devices with
> huge BARs.
> 
> For example, 2 PCI functions of a NVIDIA K80 adapter being passed through
> will exceed this limit as they have 16M + 16G + 32M BARs which
> (when aligned) will need 64GB.
> 
> This converts SPAPR_PCI_WINDOW_BASE and SPAPR_PCI_WINDOW_SPACING to
> sPAPRMachineState properties. This uses old values for pseries machine
> before 2.6 and increases the spacing to 128GB so MMIO space becomes 126GB.
> 
> This changes the default value of sPAPRPHBState::mem_win_size to -1 for
> pseries-2.6 and adds setup to spapr_phb_realize.
> 
> Signed-off-by: Alexey Kardashevskiy <address@hidden>

So, in theory I dislike the spapr_pci device reaching into the machine
type to get the spacing configuration.  But.. I don't know of a better
way to achieve the desired outcome.

A couple of other details concern me a little more.

> ---
>  hw/ppc/spapr.c              | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  hw/ppc/spapr_pci.c          | 14 ++++++++++----
>  include/hw/pci-host/spapr.h |  4 +---
>  include/hw/ppc/spapr.h      |  1 +
>  4 files changed, 54 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e9d4abf..d21ad8a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -40,6 +40,7 @@
>  #include "migration/migration.h"
>  #include "mmu-hash64.h"
>  #include "qom/cpu.h"
> +#include "qapi/visitor.h"
>  
>  #include "hw/boards.h"
>  #include "hw/ppc/ppc.h"
> @@ -2100,6 +2101,29 @@ static void spapr_set_kvm_type(Object *obj, const char 
> *value, Error **errp)
>      spapr->kvm_type = g_strdup(value);
>  }
>  
> +static void spapr_prop_get_uint64(Object *obj, Visitor *v, const char *name,
> +                                  void *opaque, Error **errp)
> +{
> +    uint64_t value = *(uint64_t *)opaque;
> +    visit_type_uint64(v, name, &value, errp);
> +}
> +
> +static void spapr_prop_set_uint64(Object *obj, Visitor *v, const char *name,
> +                                  void *opaque, Error **errp)
> +{
> +    uint64_t value = -1;
> +    visit_type_uint64(v, name, &value, errp);
> +    *(uint64_t *)opaque = value;
> +}

Pity there aren't standard helpers for this.

> +static void spapr_prop_add_uint64(Object *obj, const char *name,
> +                                  uint64_t *pval, const char *desc)
> +{
> +    object_property_add(obj, name, "uint64", spapr_prop_get_uint64,
> +                        spapr_prop_set_uint64, NULL, pval, NULL);
> +    object_property_set_description(obj, name, desc, NULL);
> +}
> +
>  static void spapr_machine_initfn(Object *obj)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> @@ -2110,6 +2134,10 @@ static void spapr_machine_initfn(Object *obj)
>      object_property_set_description(obj, "kvm-type",
>                                      "Specifies the KVM virtualization mode 
> (HV, PR)",
>                                      NULL);
> +    spapr_prop_add_uint64(obj, "phb-mmio-base", &spapr->phb_mmio_base,
> +                          "Base address for PCI host bridge MMIO");
> +    spapr_prop_add_uint64(obj, "phb-mmio-spacing", &spapr->phb_mmio_spacing,
> +                          "Amount of MMIO space per PCI host bridge");

Hmm.. what happens if someone tries to change these propertis at
runtime with qom-set?  That sounds bad.

>  }
>  
>  static void spapr_machine_finalizefn(Object *obj)
> @@ -2357,6 +2385,10 @@ static const TypeInfo spapr_machine_info = {
>   */
>  static void spapr_machine_2_6_instance_options(MachineState *machine)
>  {
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> +
> +    spapr->phb_mmio_base = SPAPR_PCI_WINDOW_BASE;
> +    spapr->phb_mmio_spacing = SPAPR_PCI_WINDOW_SPACING;
>  }
>  
>  static void spapr_machine_2_6_class_options(MachineClass *mc)
> @@ -2370,10 +2402,19 @@ DEFINE_SPAPR_MACHINE(2_6, "2.6", true);
>   * pseries-2.5
>   */
>  #define SPAPR_COMPAT_2_5 \
> -        HW_COMPAT_2_5
> +        HW_COMPAT_2_5 \
> +        {\
> +            .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
> +            .property = "mem_win_size",\
> +            .value    = "0x1000000000",\
> +        },
>  
>  static void spapr_machine_2_5_instance_options(MachineState *machine)
>  {
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> +
> +    spapr->phb_mmio_base = 0x10000000000ULL;
> +    spapr->phb_mmio_spacing = 0x1000000000ULL;
>  }
>  
>  static void spapr_machine_2_5_class_options(MachineClass *mc)
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index e8edad3..bae01dd 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1260,9 +1260,11 @@ static void spapr_phb_realize(DeviceState *dev, Error 
> **errp)
>          sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
>          sphb->dma_liobn = SPAPR_PCI_LIOBN(sphb->index, 0);
>  
> -        windows_base = SPAPR_PCI_WINDOW_BASE
> -            + sphb->index * SPAPR_PCI_WINDOW_SPACING;
> +        windows_base = spapr->phb_mmio_base
> +            + sphb->index * spapr->phb_mmio_spacing;
>          sphb->mem_win_addr = windows_base + SPAPR_PCI_MMIO_WIN_OFF;
> +        sphb->mem_win_size = spapr->phb_mmio_spacing -
> +            SPAPR_PCI_MEM_WIN_BUS_OFFSET;
>          sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF;
>      }
>  
> @@ -1281,6 +1283,11 @@ static void spapr_phb_realize(DeviceState *dev, Error 
> **errp)
>          return;
>      }
>  
> +    if (sphb->mem_win_size == (hwaddr)-1) {
> +        error_setg(errp, "Memory window size not specified for PHB");
> +        return;
> +    }
> +
>      if (sphb->io_win_addr == (hwaddr)-1) {
>          error_setg(errp, "IO window address not specified for PHB");
>          return;
> @@ -1441,8 +1448,7 @@ static Property spapr_phb_properties[] = {
>      DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1),
>      DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn, -1),
>      DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
> -    DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size,
> -                       SPAPR_PCI_MMIO_WIN_SIZE),
> +    DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size, -1),
>      DEFINE_PROP_UINT64("io_win_addr", sPAPRPHBState, io_win_addr, -1),
>      DEFINE_PROP_UINT64("io_win_size", sPAPRPHBState, io_win_size,
>                         SPAPR_PCI_IO_WIN_SIZE),
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 7de5e02..b828c31 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -107,10 +107,8 @@ struct sPAPRPHBVFIOState {
>  #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
>  
>  #define SPAPR_PCI_WINDOW_BASE        0x10000000000ULL
> -#define SPAPR_PCI_WINDOW_SPACING     0x1000000000ULL
> +#define SPAPR_PCI_WINDOW_SPACING     0x2000000000ULL
>  #define SPAPR_PCI_MMIO_WIN_OFF       0xA0000000
> -#define SPAPR_PCI_MMIO_WIN_SIZE      (SPAPR_PCI_WINDOW_SPACING - \
> -                                     SPAPR_PCI_MEM_WIN_BUS_OFFSET)
>  #define SPAPR_PCI_IO_WIN_OFF         0x80000000
>  #define SPAPR_PCI_IO_WIN_SIZE        0x10000
>  
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 098d85d..8b1369e 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -48,6 +48,7 @@ struct sPAPRMachineState {
>  
>      struct VIOsPAPRBus *vio_bus;
>      QLIST_HEAD(, sPAPRPHBState) phbs;
> +    uint64_t phb_mmio_base, phb_mmio_spacing;
>      struct sPAPRNVRAM *nvram;
>      XICSState *icp;
>      DeviceState *rtc;

-- 
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

signature.asc
Description: PGP signature