From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34578) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ad4vW-00069d-Mc for qemu-devel@nongnu.org; Mon, 07 Mar 2016 18:51:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ad4vS-0004sF-KC for qemu-devel@nongnu.org; Mon, 07 Mar 2016 18:51:02 -0500 Received: from mail-pa0-x244.google.com ([2607:f8b0:400e:c03::244]:36144) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ad4vS-0004s4-88 for qemu-devel@nongnu.org; Mon, 07 Mar 2016 18:50:58 -0500 Received: by mail-pa0-x244.google.com with SMTP id 1so2940275pal.3 for ; Mon, 07 Mar 2016 15:50:57 -0800 (PST) References: <1456969373-6741-1-git-send-email-aik@ozlabs.ru> <20160304033926.GV1620@voom.redhat.com> <56D90B58.9000803@ozlabs.ru> From: Alexey Kardashevskiy Message-ID: <56DE13DB.7020000@ozlabs.ru> Date: Tue, 8 Mar 2016 10:50:51 +1100 MIME-Version: 1.0 In-Reply-To: <56D90B58.9000803@ozlabs.ru> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH qemu] spapr-pci: Make MMIO spacing a machine property and increase it List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Alexander Graf On 03/04/2016 03:13 PM, Alexey Kardashevskiy wrote: > On 03/04/2016 02:39 PM, David Gibson wrote: >> 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 >> >> 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. > > > We could drop @index and spacing; and request the user to specify the MMIO > window start (at least) for every additional PHB. So what is the decision? :) > > >> >> 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. > > > What is the problem here exactly? These are the properties for new PHBs, > if/when we add an ability to hotplug PHBs, changes to these properties will > reflect in new PHB properties. > > Likewise writing to "kvm-type" does not switch from HV to PR and vice versa. > > >> >>> } >>> >>> 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; >> > > -- Alexey