From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59570) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abgaW-0002V6-PS for qemu-devel@nongnu.org; Thu, 03 Mar 2016 22:39:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abgaU-0006qU-Su for qemu-devel@nongnu.org; Thu, 03 Mar 2016 22:39:36 -0500 Date: Fri, 4 Mar 2016 14:39:26 +1100 From: David Gibson Message-ID: <20160304033926.GV1620@voom.redhat.com> References: <1456969373-6741-1-git-send-email-aik@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="pkBzO0lo80FoaZii" Content-Disposition: inline In-Reply-To: <1456969373-6741-1-git-send-email-aik@ozlabs.ru> 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: Alexey Kardashevskiy Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Alexander Graf --pkBzO0lo80FoaZii Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > This changes the default value of sPAPRPHBState::mem_win_size to -1 for > pseries-2.6 and adds setup to spapr_phb_realize. >=20 > 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. 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(-) >=20 > 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" > =20 > #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 =3D g_strdup(value); > } > =20 > +static void spapr_prop_get_uint64(Object *obj, Visitor *v, const char *n= ame, > + void *opaque, Error **errp) > +{ > + uint64_t value =3D *(uint64_t *)opaque; > + visit_type_uint64(v, name, &value, errp); > +} > + > +static void spapr_prop_set_uint64(Object *obj, Visitor *v, const char *n= ame, > + void *opaque, Error **errp) > +{ > + uint64_t value =3D -1; > + visit_type_uint64(v, name, &value, errp); > + *(uint64_t *)opaque =3D 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 =3D 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 mo= de (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_spac= ing, > + "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. > } > =20 > static void spapr_machine_finalizefn(Object *obj) > @@ -2357,6 +2385,10 @@ static const TypeInfo spapr_machine_info =3D { > */ > static void spapr_machine_2_6_instance_options(MachineState *machine) > { > + sPAPRMachineState *spapr =3D SPAPR_MACHINE(machine); > + > + spapr->phb_mmio_base =3D SPAPR_PCI_WINDOW_BASE; > + spapr->phb_mmio_spacing =3D SPAPR_PCI_WINDOW_SPACING; > } > =20 > 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 =3D TYPE_SPAPR_PCI_HOST_BRIDGE,\ > + .property =3D "mem_win_size",\ > + .value =3D "0x1000000000",\ > + }, > =20 > static void spapr_machine_2_5_instance_options(MachineState *machine) > { > + sPAPRMachineState *spapr =3D SPAPR_MACHINE(machine); > + > + spapr->phb_mmio_base =3D 0x10000000000ULL; > + spapr->phb_mmio_spacing =3D 0x1000000000ULL; > } > =20 > 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, Er= ror **errp) > sphb->buid =3D SPAPR_PCI_BASE_BUID + sphb->index; > sphb->dma_liobn =3D SPAPR_PCI_LIOBN(sphb->index, 0); > =20 > - windows_base =3D SPAPR_PCI_WINDOW_BASE > - + sphb->index * SPAPR_PCI_WINDOW_SPACING; > + windows_base =3D spapr->phb_mmio_base > + + sphb->index * spapr->phb_mmio_spacing; > sphb->mem_win_addr =3D windows_base + SPAPR_PCI_MMIO_WIN_OFF; > + sphb->mem_win_size =3D spapr->phb_mmio_spacing - > + SPAPR_PCI_MEM_WIN_BUS_OFFSET; > sphb->io_win_addr =3D windows_base + SPAPR_PCI_IO_WIN_OFF; > } > =20 > @@ -1281,6 +1283,11 @@ static void spapr_phb_realize(DeviceState *dev, Er= ror **errp) > return; > } > =20 > + if (sphb->mem_win_size =3D=3D (hwaddr)-1) { > + error_setg(errp, "Memory window size not specified for PHB"); > + return; > + } > + > if (sphb->io_win_addr =3D=3D (hwaddr)-1) { > error_setg(errp, "IO window address not specified for PHB"); > return; > @@ -1441,8 +1448,7 @@ static Property spapr_phb_properties[] =3D { > 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 > =20 > #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 > =20 > 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 { > =20 > struct VIOsPAPRBus *vio_bus; > QLIST_HEAD(, sPAPRPHBState) phbs; > + uint64_t phb_mmio_base, phb_mmio_spacing; > struct sPAPRNVRAM *nvram; > XICSState *icp; > DeviceState *rtc; --=20 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 --pkBzO0lo80FoaZii Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW2QNtAAoJEGw4ysog2bOSseMP/RuL24r8YDnqNs8ssAXl6hqG ju43T/rkahyQQrh5TifTl4JInkxW4DDnNZUNBHjBUIb0BxLPHwvZ1nFqs1PXjoci tDSN4eWdj9IVHv/N2szehAoVSF6RV2bupC+TqNcBNZFBZX3JQBXcThpnKi8JfGaJ scIIlpPBB4woxUgU8bOmkqvXVXcSiuw8imBhApaXRU1sWAXcg1FcvOSH96lclWRG JInE9LnYNH4cE9fQPGW9pLaOsZy5jcS6UMowUKEYVYYsN8EOAzxu7hP041NDIXpf sqYFvrgUvekKvULSfOsAaFTvdQ7qa5d73kw2AIEa02Iy73O6+QGNhfmPPutBXFWf b64PTcTKWpJcE0t80Gfyk1Ol2pUUM/SQs5R0AFs21CyokCto3STl49SU6b28dPm+ 8xbZppIXfTyG/MnKYA+sPCFeZ/781sQ5IyIIGdliHzKgNKKOYVeW3bS34vAWNwGb SWQ6PJNG0CE4ykaf/o+JVbE3qrwRQLexoAWC39efSsLwQe1D8Zu9JqpvrARBuRQs 9cqjhqdUhl9VZxtisjXsX5+cWegREA6N57t4RDLWfA4P4ALXI/ou0WzG09+9LHjE 7wGBT0GaNIcaSmB3Foops9u2jh/mzoMVRK7gUAkeMB8Q0JrfmquF8M+HRzEcNWJi LW061YZjRmULO4HeF+X0 =Kwjd -----END PGP SIGNATURE----- --pkBzO0lo80FoaZii--