From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35158) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abh7D-0001Bi-S3 for qemu-devel@nongnu.org; Thu, 03 Mar 2016 23:13:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abh79-0005jZ-SR for qemu-devel@nongnu.org; Thu, 03 Mar 2016 23:13:22 -0500 Received: from mail-pf0-x244.google.com ([2607:f8b0:400e:c00::244]:34202) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abh79-0005j8-HR for qemu-devel@nongnu.org; Thu, 03 Mar 2016 23:13:19 -0500 Received: by mail-pf0-x244.google.com with SMTP id 184so2495733pff.1 for ; Thu, 03 Mar 2016 20:13:18 -0800 (PST) References: <1456969373-6741-1-git-send-email-aik@ozlabs.ru> <20160304033926.GV1620@voom.redhat.com> From: Alexey Kardashevskiy Message-ID: <56D90B58.9000803@ozlabs.ru> Date: Fri, 4 Mar 2016 15:13:12 +1100 MIME-Version: 1.0 In-Reply-To: <20160304033926.GV1620@voom.redhat.com> 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 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. > > 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