| Message ID | 20181123055304.25116-5-aik@ozlabs.ru |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | powerpc/powernv/npu, vfio: NVIDIA V100 + P9 passthrough | expand |
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 NPU2 > 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 <aik@ozlabs.ru> > --- > 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/include/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 */ > > void *private_data; > + struct npu *npu; > }; > > /* These are used for config access before all the PCI probing > diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/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 @@ > > struct pci_dn; > > -/* Maximum possible number of ATSD MMIO registers per NPU */ > -#define NV_NMMU_ATSD_REGS 8 > - > enum pnv_phb_type { > PNV_PHB_IODA1 = 0, > PNV_PHB_IODA2 = 1, > @@ -176,19 +173,6 @@ struct pnv_phb { > unsigned int diag_data_size; > u8 *diag_data; > > - /* 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; > }; > > 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(struct pnv_ioda_pe *npe) > return gpe; > } > > +/* > + * 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 > > @@ -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; > > for (i = 0; i <= max_npu2_index; i++) { > mmio_atsd_reg[i].reg = -1; > @@ -493,8 +511,10 @@ static void acquire_atsd_reg(struct npu_context *npu_context, > if (!npdev) > continue; > > - nphb = pci_bus_to_host(npdev->bus)->private_data; > - npu = &nphb->npu; > + npu = 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? > mmio_atsd_reg[i].npu = npu; > mmio_atsd_reg[i].reg = get_mmio_atsd_reg(npu); > while (mmio_atsd_reg[i].reg < 0) { > @@ -662,6 +682,7 @@ struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, > struct pnv_phb *nphb; > struct npu *npu; > struct npu_context *npu_context; > + struct pci_controller *hose; > > /* > * At present we don't support GPUs connected to multiple NPUs and I'm > @@ -689,8 +710,11 @@ struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, > return ERR_PTR(-EINVAL); > } > > - nphb = pci_bus_to_host(npdev->bus)->private_data; > - npu = &nphb->npu; > + hose = pci_bus_to_host(npdev->bus); > + nphb = hose->private_data; > + npu = hose->npu; > + if (!npu) > + return ERR_PTR(-ENODEV); > > /* > * Setup the NPU context table for a particular GPU. These need to be > @@ -764,7 +788,7 @@ struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, > */ > WRITE_ONCE(npu_context->npdev[npu->index][nvlink_index], npdev); > > - if (!nphb->npu.nmmu_flush) { > + if (!npu->nmmu_flush) { > /* > * If we're not explicitly flushing ourselves we need to mark > * the thread for global flushes > @@ -802,6 +826,7 @@ void pnv_npu2_destroy_context(struct npu_context *npu_context, > struct pci_dev *npdev = pnv_pci_get_npu_dev(gpdev, 0); > struct device_node *nvlink_dn; > u32 nvlink_index; > + struct pci_controller *hose; > > if (WARN_ON(!npdev)) > return; > @@ -809,8 +834,11 @@ void pnv_npu2_destroy_context(struct npu_context *npu_context, > if (!firmware_has_feature(FW_FEATURE_OPAL)) > return; > > - nphb = pci_bus_to_host(npdev->bus)->private_data; > - npu = &nphb->npu; > + hose = pci_bus_to_host(npdev->bus); > + nphb = hose->private_data; > + npu = hose->npu; > + if (!npu) > + return; > nvlink_dn = of_parse_phandle(npdev->dev.of_node, "ibm,nvlink", 0); > if (WARN_ON(of_property_read_u32(nvlink_dn, "ibm,npu-link-index", > &nvlink_index))) > @@ -888,9 +916,15 @@ int pnv_npu2_init(struct pnv_phb *phb) > struct pci_dev *gpdev; > static int npu_index; > uint64_t rc = 0; > + struct pci_controller *hose = phb->hose; > + struct npu *npu; > + int ret; > > - phb->npu.nmmu_flush = > - of_property_read_bool(phb->hose->dn, "ibm,nmmu-flush"); > + npu = kzalloc(sizeof(*npu), GFP_KERNEL); > + if (!npu) > + return -ENOMEM; > + > + npu->nmmu_flush = of_property_read_bool(hose->dn, "ibm,nmmu-flush"); > for_each_child_of_node(phb->hose->dn, dn) { > gpdev = pnv_pci_get_gpu_dev(get_pci_dev(dn)); > if (gpdev) { > @@ -904,18 +938,29 @@ int pnv_npu2_init(struct pnv_phb *phb) > } > } > > - for (i = 0; !of_property_read_u64_index(phb->hose->dn, "ibm,mmio-atsd", > + for (i = 0; !of_property_read_u64_index(hose->dn, "ibm,mmio-atsd", > i, &mmio_atsd); i++) > - phb->npu.mmio_atsd_regs[i] = ioremap(mmio_atsd, 32); > + npu->mmio_atsd_regs[i] = ioremap(mmio_atsd, 32); > > - pr_info("NPU%lld: Found %d MMIO ATSD registers", phb->opal_id, i); > - phb->npu.mmio_atsd_count = i; > - phb->npu.mmio_atsd_usage = 0; > + pr_info("NPU%d: Found %d MMIO ATSD registers", hose->global_number, i); > + npu->mmio_atsd_count = i; > + npu->mmio_atsd_usage = 0; > npu_index++; > - if (WARN_ON(npu_index >= NV_MAX_NPUS)) > - return -ENOSPC; > + if (WARN_ON(npu_index >= NV_MAX_NPUS)) { > + ret = -ENOSPC; > + goto fail_exit; > + } > max_npu2_index = npu_index; > - phb->npu.index = npu_index; > + npu->index = npu_index; > + hose->npu = npu; > > return 0; > + > +fail_exit: > + for (i = 0; i < npu->mmio_atsd_count; ++i) > + iounmap(npu->mmio_atsd_regs[i]); > + > + kfree(npu); > + > + return ret; > }
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 NPU2 >> 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 <aik@ozlabs.ru> >> --- >> 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/include/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 */ >> >> void *private_data; >> + struct npu *npu; >> }; >> >> /* These are used for config access before all the PCI probing >> diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/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 @@ >> >> struct pci_dn; >> >> -/* Maximum possible number of ATSD MMIO registers per NPU */ >> -#define NV_NMMU_ATSD_REGS 8 >> - >> enum pnv_phb_type { >> PNV_PHB_IODA1 = 0, >> PNV_PHB_IODA2 = 1, >> @@ -176,19 +173,6 @@ struct pnv_phb { >> unsigned int diag_data_size; >> u8 *diag_data; >> >> - /* 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; >> }; >> >> 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(struct pnv_ioda_pe *npe) >> return gpe; >> } >> >> +/* >> + * 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 >> >> @@ -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; >> >> for (i = 0; i <= max_npu2_index; i++) { >> mmio_atsd_reg[i].reg = -1; >> @@ -493,8 +511,10 @@ static void acquire_atsd_reg(struct npu_context *npu_context, >> if (!npdev) >> continue; >> >> - nphb = pci_bus_to_host(npdev->bus)->private_data; >> - npu = &nphb->npu; >> + npu = 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? 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. I'll probably add checks for npu!=NULL where we used to have firmware_has_feature(FW_FEATURE_OPAL) in 05/19.
On 05/12/2018 16:47, Alexey Kardashevskiy wrote: > > > 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 NPU2 >>> 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 <aik@ozlabs.ru> >>> --- >>> 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/include/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 */ >>> >>> void *private_data; >>> + struct npu *npu; >>> }; >>> >>> /* These are used for config access before all the PCI probing >>> diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/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 @@ >>> >>> struct pci_dn; >>> >>> -/* Maximum possible number of ATSD MMIO registers per NPU */ >>> -#define NV_NMMU_ATSD_REGS 8 >>> - >>> enum pnv_phb_type { >>> PNV_PHB_IODA1 = 0, >>> PNV_PHB_IODA2 = 1, >>> @@ -176,19 +173,6 @@ struct pnv_phb { >>> unsigned int diag_data_size; >>> u8 *diag_data; >>> >>> - /* 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; >>> }; >>> >>> 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(struct pnv_ioda_pe *npe) >>> return gpe; >>> } >>> >>> +/* >>> + * 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 >>> >>> @@ -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; >>> >>> for (i = 0; i <= max_npu2_index; i++) { >>> mmio_atsd_reg[i].reg = -1; >>> @@ -493,8 +511,10 @@ static void acquire_atsd_reg(struct npu_context *npu_context, >>> if (!npdev) >>> continue; >>> >>> - nphb = pci_bus_to_host(npdev->bus)->private_data; >>> - npu = &nphb->npu; >>> + npu = 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? > > > > 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. Well, technically kmalloc() can fail in pnv_npu2_init() (but not later) so can (in theory) end up with an NPU PHB and npu==NULL but it is sooo unlikely... > > I'll probably add checks for npu!=NULL where we used to have > firmware_has_feature(FW_FEATURE_OPAL) in 05/19.
On Wed, Dec 05, 2018 at 05:17:57PM +1100, Alexey Kardashevskiy wrote: > > > On 05/12/2018 16:47, Alexey Kardashevskiy wrote: > > > > > > 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 NPU2 > >>> 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 <aik@ozlabs.ru> > >>> --- > >>> 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/include/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 */ > >>> > >>> void *private_data; > >>> + struct npu *npu; > >>> }; > >>> > >>> /* These are used for config access before all the PCI probing > >>> diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/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 @@ > >>> > >>> struct pci_dn; > >>> > >>> -/* Maximum possible number of ATSD MMIO registers per NPU */ > >>> -#define NV_NMMU_ATSD_REGS 8 > >>> - > >>> enum pnv_phb_type { > >>> PNV_PHB_IODA1 = 0, > >>> PNV_PHB_IODA2 = 1, > >>> @@ -176,19 +173,6 @@ struct pnv_phb { > >>> unsigned int diag_data_size; > >>> u8 *diag_data; > >>> > >>> - /* 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; > >>> }; > >>> > >>> 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(struct pnv_ioda_pe *npe) > >>> return gpe; > >>> } > >>> > >>> +/* > >>> + * 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 > >>> > >>> @@ -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; > >>> > >>> for (i = 0; i <= max_npu2_index; i++) { > >>> mmio_atsd_reg[i].reg = -1; > >>> @@ -493,8 +511,10 @@ static void acquire_atsd_reg(struct npu_context *npu_context, > >>> if (!npdev) > >>> continue; > >>> > >>> - nphb = pci_bus_to_host(npdev->bus)->private_data; > >>> - npu = &nphb->npu; > >>> + npu = 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? > > > > > > > > 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. > > > Well, technically kmalloc() can fail in pnv_npu2_init() (but not later) > so can (in theory) end up with an NPU PHB and npu==NULL 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?
On 06/12/2018 09:40, David Gibson wrote: > On Wed, Dec 05, 2018 at 05:17:57PM +1100, Alexey Kardashevskiy wrote: >> >> >> On 05/12/2018 16:47, Alexey Kardashevskiy wrote: >>> >>> >>> 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 NPU2 >>>>> 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 <aik@ozlabs.ru> >>>>> --- >>>>> 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/include/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 */ >>>>> >>>>> void *private_data; >>>>> + struct npu *npu; >>>>> }; >>>>> >>>>> /* These are used for config access before all the PCI probing >>>>> diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/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 @@ >>>>> >>>>> struct pci_dn; >>>>> >>>>> -/* Maximum possible number of ATSD MMIO registers per NPU */ >>>>> -#define NV_NMMU_ATSD_REGS 8 >>>>> - >>>>> enum pnv_phb_type { >>>>> PNV_PHB_IODA1 = 0, >>>>> PNV_PHB_IODA2 = 1, >>>>> @@ -176,19 +173,6 @@ struct pnv_phb { >>>>> unsigned int diag_data_size; >>>>> u8 *diag_data; >>>>> >>>>> - /* 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; >>>>> }; >>>>> >>>>> 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(struct pnv_ioda_pe *npe) >>>>> return gpe; >>>>> } >>>>> >>>>> +/* >>>>> + * 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 >>>>> >>>>> @@ -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; >>>>> >>>>> for (i = 0; i <= max_npu2_index; i++) { >>>>> mmio_atsd_reg[i].reg = -1; >>>>> @@ -493,8 +511,10 @@ static void acquire_atsd_reg(struct npu_context *npu_context, >>>>> if (!npdev) >>>>> continue; >>>>> >>>>> - nphb = pci_bus_to_host(npdev->bus)->private_data; >>>>> - npu = &nphb->npu; >>>>> + npu = 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? >>> >>> >>> >>> 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. >> >> >> Well, technically kmalloc() can fail in pnv_npu2_init() (but not later) >> so can (in theory) end up with an NPU PHB and npu==NULL 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? Not sure I am following. pnv_npu2_init() is called at boot time so failing-and-not-leaving-null-pointer here means panic and I definitely do not want that. I am adding !npu checks in the next respin though, pretty much replacing firmware_has_feature(FW_FEATURE_OPAL), do i miss anything here?
On Mon, Dec 10, 2018 at 01:50:35PM +1100, Alexey Kardashevskiy wrote: > > > On 06/12/2018 09:40, David Gibson wrote: > > On Wed, Dec 05, 2018 at 05:17:57PM +1100, Alexey Kardashevskiy wrote: > >> > >> > >> On 05/12/2018 16:47, Alexey Kardashevskiy wrote: > >>> > >>> > >>> 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 NPU2 > >>>>> 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 <aik@ozlabs.ru> > >>>>> --- > >>>>> 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/include/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 */ > >>>>> > >>>>> void *private_data; > >>>>> + struct npu *npu; > >>>>> }; > >>>>> > >>>>> /* These are used for config access before all the PCI probing > >>>>> diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/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 @@ > >>>>> > >>>>> struct pci_dn; > >>>>> > >>>>> -/* Maximum possible number of ATSD MMIO registers per NPU */ > >>>>> -#define NV_NMMU_ATSD_REGS 8 > >>>>> - > >>>>> enum pnv_phb_type { > >>>>> PNV_PHB_IODA1 = 0, > >>>>> PNV_PHB_IODA2 = 1, > >>>>> @@ -176,19 +173,6 @@ struct pnv_phb { > >>>>> unsigned int diag_data_size; > >>>>> u8 *diag_data; > >>>>> > >>>>> - /* 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; > >>>>> }; > >>>>> > >>>>> 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(struct pnv_ioda_pe *npe) > >>>>> return gpe; > >>>>> } > >>>>> > >>>>> +/* > >>>>> + * 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 > >>>>> > >>>>> @@ -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; > >>>>> > >>>>> for (i = 0; i <= max_npu2_index; i++) { > >>>>> mmio_atsd_reg[i].reg = -1; > >>>>> @@ -493,8 +511,10 @@ static void acquire_atsd_reg(struct npu_context *npu_context, > >>>>> if (!npdev) > >>>>> continue; > >>>>> > >>>>> - nphb = pci_bus_to_host(npdev->bus)->private_data; > >>>>> - npu = &nphb->npu; > >>>>> + npu = 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? > >>> > >>> > >>> > >>> 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. > >> > >> > >> Well, technically kmalloc() can fail in pnv_npu2_init() (but not later) > >> so can (in theory) end up with an NPU PHB and npu==NULL 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? > > Not sure I am following. pnv_npu2_init() is called at boot time so > failing-and-not-leaving-null-pointer here means panic and I definitely > do not want that. Well, if it's a choice between panic then and panic later, I'm not so sure that's true. > I am adding !npu checks in the next respin though, > pretty much replacing firmware_has_feature(FW_FEATURE_OPAL), do i miss > anything here? That seems reasonable.
diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/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 */ void *private_data; + struct npu *npu; }; /* These are used for config access before all the PCI probing diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/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 @@ struct pci_dn; -/* Maximum possible number of ATSD MMIO registers per NPU */ -#define NV_NMMU_ATSD_REGS 8 - enum pnv_phb_type { PNV_PHB_IODA1 = 0, PNV_PHB_IODA2 = 1, @@ -176,19 +173,6 @@ struct pnv_phb { unsigned int diag_data_size; u8 *diag_data; - /* 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; }; 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(struct pnv_ioda_pe *npe) return gpe; } +/* + * 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 @@ -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; for (i = 0; i <= max_npu2_index; i++) { mmio_atsd_reg[i].reg = -1; @@ -493,8 +511,10 @@ static void acquire_atsd_reg(struct npu_context *npu_context, if (!npdev) continue; - nphb = pci_bus_to_host(npdev->bus)->private_data; - npu = &nphb->npu; + npu = pci_bus_to_host(npdev->bus)->npu; + if (!npu) + continue; + mmio_atsd_reg[i].npu = npu; mmio_atsd_reg[i].reg = get_mmio_atsd_reg(npu); while (mmio_atsd_reg[i].reg < 0) { @@ -662,6 +682,7 @@ struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, struct pnv_phb *nphb; struct npu *npu; struct npu_context *npu_context; + struct pci_controller *hose; /* * At present we don't support GPUs connected to multiple NPUs and I'm @@ -689,8 +710,11 @@ struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, return ERR_PTR(-EINVAL); } - nphb = pci_bus_to_host(npdev->bus)->private_data; - npu = &nphb->npu; + hose = pci_bus_to_host(npdev->bus); + nphb = hose->private_data; + npu = hose->npu; + if (!npu) + return ERR_PTR(-ENODEV); /* * Setup the NPU context table for a particular GPU. These need to be @@ -764,7 +788,7 @@ struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, */ WRITE_ONCE(npu_context->npdev[npu->index][nvlink_index], npdev); - if (!nphb->npu.nmmu_flush) { + if (!npu->nmmu_flush) { /* * If we're not explicitly flushing ourselves we need to mark * the thread for global flushes @@ -802,6 +826,7 @@ void pnv_npu2_destroy_context(struct npu_context *npu_context, struct pci_dev *npdev = pnv_pci_get_npu_dev(gpdev, 0); struct device_node *nvlink_dn; u32 nvlink_index; + struct pci_controller *hose; if (WARN_ON(!npdev)) return; @@ -809,8 +834,11 @@ void pnv_npu2_destroy_context(struct npu_context *npu_context, if (!firmware_has_feature(FW_FEATURE_OPAL)) return; - nphb = pci_bus_to_host(npdev->bus)->private_data; - npu = &nphb->npu; + hose = pci_bus_to_host(npdev->bus); + nphb = hose->private_data; + npu = hose->npu; + if (!npu) + return; nvlink_dn = of_parse_phandle(npdev->dev.of_node, "ibm,nvlink", 0); if (WARN_ON(of_property_read_u32(nvlink_dn, "ibm,npu-link-index", &nvlink_index))) @@ -888,9 +916,15 @@ int pnv_npu2_init(struct pnv_phb *phb) struct pci_dev *gpdev; static int npu_index; uint64_t rc = 0; + struct pci_controller *hose = phb->hose; + struct npu *npu; + int ret; - phb->npu.nmmu_flush = - of_property_read_bool(phb->hose->dn, "ibm,nmmu-flush"); + npu = kzalloc(sizeof(*npu), GFP_KERNEL); + if (!npu) + return -ENOMEM; + + npu->nmmu_flush = of_property_read_bool(hose->dn, "ibm,nmmu-flush"); for_each_child_of_node(phb->hose->dn, dn) { gpdev = pnv_pci_get_gpu_dev(get_pci_dev(dn)); if (gpdev) { @@ -904,18 +938,29 @@ int pnv_npu2_init(struct pnv_phb *phb) } } - for (i = 0; !of_property_read_u64_index(phb->hose->dn, "ibm,mmio-atsd", + for (i = 0; !of_property_read_u64_index(hose->dn, "ibm,mmio-atsd", i, &mmio_atsd); i++) - phb->npu.mmio_atsd_regs[i] = ioremap(mmio_atsd, 32); + npu->mmio_atsd_regs[i] = ioremap(mmio_atsd, 32); - pr_info("NPU%lld: Found %d MMIO ATSD registers", phb->opal_id, i); - phb->npu.mmio_atsd_count = i; - phb->npu.mmio_atsd_usage = 0; + pr_info("NPU%d: Found %d MMIO ATSD registers", hose->global_number, i); + npu->mmio_atsd_count = i; + npu->mmio_atsd_usage = 0; npu_index++; - if (WARN_ON(npu_index >= NV_MAX_NPUS)) - return -ENOSPC; + if (WARN_ON(npu_index >= NV_MAX_NPUS)) { + ret = -ENOSPC; + goto fail_exit; + } max_npu2_index = npu_index; - phb->npu.index = npu_index; + npu->index = npu_index; + hose->npu = npu; return 0; + +fail_exit: + for (i = 0; i < npu->mmio_atsd_count; ++i) + iounmap(npu->mmio_atsd_regs[i]); + + kfree(npu); + + return ret; }
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 NPU2 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 <aik@ozlabs.ru> --- 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(-)