[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH qemu] spapr-pci: Make MMIO spacing a machine pro
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [PATCH qemu] spapr-pci: Make MMIO spacing a machine property and increase it |
Date: |
Fri, 4 Mar 2016 14:39:26 +1100 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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