From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Gibson Date: Wed, 05 Dec 2018 22:40:40 +0000 Subject: Re: [PATCH kernel v4 04/19] powerpc/powernv: Move npu struct from pnv_phb to pci_controller Message-Id: <20181205224040.GJ768@umbus.fritz.box> MIME-Version: 1 Content-Type: multipart/mixed; boundary="6J7GEvtanOfV9oXA" List-Id: References: <20181123055304.25116-1-aik@ozlabs.ru> <20181123055304.25116-5-aik@ozlabs.ru> <20181205051424.GB768@umbus.fritz.box> <05cb6d97-9949-1d90-33e1-8d5cc1bb83f2@ozlabs.ru> In-Reply-To: <05cb6d97-9949-1d90-33e1-8d5cc1bb83f2@ozlabs.ru> To: Alexey Kardashevskiy Cc: Alex Williamson , Jose Ricardo Ziviani , Sam Bobroff , Alistair Popple , Daniel Henrique Barboza , linuxppc-dev@lists.ozlabs.org, kvm-ppc@vger.kernel.org, Piotr Jaroszynski , Oliver O'Halloran , Andrew Donnellan , Leonardo Augusto =?iso-8859-1?Q?Guimar=E3es?= Garcia , Reza Arbab --6J7GEvtanOfV9oXA Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Dec 05, 2018 at 05:17:57PM +1100, Alexey Kardashevskiy wrote: >=20 >=20 > On 05/12/2018 16:47, Alexey Kardashevskiy wrote: > >=20 > >=20 > > On 05/12/2018 16:14, David Gibson wrote: > >> On Fri, Nov 23, 2018 at 04:52:49PM +1100, Alexey Kardashevskiy wrote: > >>> The powernv PCI code stores NPU data in the pnv_phb struct. The latter > >>> is referenced by pci_controller::private_data. We are going to have N= PU2 > >>> support in the pseries platform as well but it does not store any > >>> private_data in in the pci_controller struct; and even if it did, > >>> it would be a different data structure. > >>> > >>> This makes npu a pointer and stores it one level higher in > >>> the pci_controller struct. > >>> > >>> Signed-off-by: Alexey Kardashevskiy > >>> --- > >>> Changes: > >>> v4: > >>> * changed subj from "powerpc/powernv: Detach npu struct from pnv_phb" > >>> * got rid of global list of npus - store them now in pci_controller > >>> * got rid of npdev_to_npu() helper > >>> --- > >>> arch/powerpc/include/asm/pci-bridge.h | 1 + > >>> arch/powerpc/platforms/powernv/pci.h | 16 ----- > >>> arch/powerpc/platforms/powernv/npu-dma.c | 81 ++++++++++++++++++----= -- > >>> 3 files changed, 64 insertions(+), 34 deletions(-) > >>> > >>> diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/inc= lude/asm/pci-bridge.h > >>> index 94d4490..aee4fcc 100644 > >>> --- a/arch/powerpc/include/asm/pci-bridge.h > >>> +++ b/arch/powerpc/include/asm/pci-bridge.h > >>> @@ -129,6 +129,7 @@ struct pci_controller { > >>> #endif /* CONFIG_PPC64 */ > >>> =20 > >>> void *private_data; > >>> + struct npu *npu; > >>> }; > >>> =20 > >>> /* These are used for config access before all the PCI probing > >>> diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/plat= forms/powernv/pci.h > >>> index 2131373..f2d50974 100644 > >>> --- a/arch/powerpc/platforms/powernv/pci.h > >>> +++ b/arch/powerpc/platforms/powernv/pci.h > >>> @@ -8,9 +8,6 @@ > >>> =20 > >>> struct pci_dn; > >>> =20 > >>> -/* Maximum possible number of ATSD MMIO registers per NPU */ > >>> -#define NV_NMMU_ATSD_REGS 8 > >>> - > >>> enum pnv_phb_type { > >>> PNV_PHB_IODA1 =3D 0, > >>> PNV_PHB_IODA2 =3D 1, > >>> @@ -176,19 +173,6 @@ struct pnv_phb { > >>> unsigned int diag_data_size; > >>> u8 *diag_data; > >>> =20 > >>> - /* Nvlink2 data */ > >>> - struct npu { > >>> - int index; > >>> - __be64 *mmio_atsd_regs[NV_NMMU_ATSD_REGS]; > >>> - unsigned int mmio_atsd_count; > >>> - > >>> - /* Bitmask for MMIO register usage */ > >>> - unsigned long mmio_atsd_usage; > >>> - > >>> - /* Do we need to explicitly flush the nest mmu? */ > >>> - bool nmmu_flush; > >>> - } npu; > >>> - > >>> int p2p_target_count; > >>> }; > >>> =20 > >>> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/= platforms/powernv/npu-dma.c > >>> index 91d488f..7dd5c0e5 100644 > >>> --- a/arch/powerpc/platforms/powernv/npu-dma.c > >>> +++ b/arch/powerpc/platforms/powernv/npu-dma.c > >>> @@ -327,6 +327,25 @@ struct pnv_ioda_pe *pnv_pci_npu_setup_iommu(stru= ct pnv_ioda_pe *npe) > >>> return gpe; > >>> } > >>> =20 > >>> +/* > >>> + * NPU2 ATS > >>> + */ > >>> +/* Maximum possible number of ATSD MMIO registers per NPU */ > >>> +#define NV_NMMU_ATSD_REGS 8 > >>> + > >>> +/* An NPU descriptor, valid for POWER9 only */ > >>> +struct npu { > >>> + int index; > >>> + __be64 *mmio_atsd_regs[NV_NMMU_ATSD_REGS]; > >>> + unsigned int mmio_atsd_count; > >>> + > >>> + /* Bitmask for MMIO register usage */ > >>> + unsigned long mmio_atsd_usage; > >>> + > >>> + /* Do we need to explicitly flush the nest mmu? */ > >>> + bool nmmu_flush; > >>> +}; > >>> + > >>> /* Maximum number of nvlinks per npu */ > >>> #define NV_MAX_LINKS 6 > >>> =20 > >>> @@ -478,7 +497,6 @@ static void acquire_atsd_reg(struct npu_context *= npu_context, > >>> int i, j; > >>> struct npu *npu; > >>> struct pci_dev *npdev; > >>> - struct pnv_phb *nphb; > >>> =20 > >>> for (i =3D 0; i <=3D max_npu2_index; i++) { > >>> mmio_atsd_reg[i].reg =3D -1; > >>> @@ -493,8 +511,10 @@ static void acquire_atsd_reg(struct npu_context = *npu_context, > >>> if (!npdev) > >>> continue; > >>> =20 > >>> - nphb =3D pci_bus_to_host(npdev->bus)->private_data; > >>> - npu =3D &nphb->npu; > >>> + npu =3D pci_bus_to_host(npdev->bus)->npu; > >>> + if (!npu) > >>> + continue; > >> > >> This patch changes a bunch of places that used to unconditionally > >> locate an NPU now have a failure path. > >> > >> Given that this used to always have an NPU, doesn't that mean that if > >> the NPU is not present something has already gone wrong, and we should > >> WARN_ON() or something? > >=20 > >=20 > >=20 > > That means this is a leftover since I dropped that npdev_to_npu helper > > which could help but there was no real value in it. I'll remove the > > check here in the next respin. >=20 >=20 > Well, technically kmalloc() can fail in pnv_npu2_init() (but not later) > so can (in theory) end up with an NPU PHB and npu=3D=3DNULL but it is sooo > unlikely... More to the point, shouldn't you then fail immediately, rather than leaving the NULL floating around for later code? --=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 --6J7GEvtanOfV9oXA Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEdfRlhq5hpmzETofcbDjKyiDZs5IFAlwIU+cACgkQbDjKyiDZ s5Li4Q//Wk2AMsEHjeglulvU7Oiu/UkSjERrXwIpevgHRVmiOaOHUcaY16EC97CM 2MB2/le5Bj3Wgi/CicPeFqCxwckPEka8t5cs91m6ofwKnh59CVCVvinpNsWmDfIU 34RAYgA7IDHqQ/DPElWzcZu17bHCZGNfrCFUJMRVd/v/KsZwp6XYfMTBNimugvb1 brheR2OAN5DNc32tVR76EoRRqrPGjnGOotB832l+MYUcUCgat9NA84eU3ay+rvhs xTaaJETrnMuAGW3v3/pfsivfCnzAcWRf0etESuVch8qyUk4ZWtCq6njHD6IHU14x me7H6iQT9y3dnVrjvf8B45EoQwIO1BV97dfglkc4zRfCwFAgzU08+nkr1GN8SleL EpJfaXdz52wW+TmaTSwXtMNHJZ7ptlB/H5z/jR2PnBvHyALAHG6a4Id147IBult1 hqZYPxIZYj/WuGFxbY1BVNhOgstvVGmjPsJ8TOdMaDawyPlNbVkJaKGIZga1JlhM zVFRMuRv2bH8JRFwHXKjB1aRZbbXcDBMg5uBuqA02ziqPwCDVPFfHI0U/e2kO7U9 ABFqjSlfKMWovF7nKY+vZSqcL2UZ33gSiplaYHYuXBo4KLIXy1C4P89i6a9igZe/ n47piSIQzWXTNWnTLwke79d12vS+PYhGlYdDiKmDch0LpTFKNT0= =HiY3 -----END PGP SIGNATURE----- --6J7GEvtanOfV9oXA--