Re: [PATCH kernel v4 04/19] powerpc/powernv: Move npu struct from pnv_phb to pci_controller
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
> > ---
> > 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 intdiag_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;
>
Re: [PATCH kernel v4 04/19] powerpc/powernv: Move npu struct from pnv_phb to pci_controller
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
> ---
> 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 intdiag_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 = >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
Re: [PATCH kernel v4 04/19] powerpc/powernv: Move npu struct from pnv_phb to pci_controller
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
> >>> ---
> >>> 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 intdiag_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 = >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
Re: [PATCH kernel v4 04/19] powerpc/powernv: Move npu struct from pnv_phb to pci_controller
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
>>> ---
>>> 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 intdiag_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 = >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
Re: [PATCH kernel v4 04/19] powerpc/powernv: Move npu struct from pnv_phb to pci_controller
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
>> ---
>> 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 intdiag_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 = >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.
--
Alexey
Re: [PATCH kernel v4 04/19] powerpc/powernv: Move npu struct from pnv_phb to pci_controller
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
> ---
> 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 intdiag_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 = >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);
> }
>
> -
[PATCH kernel v4 04/19] powerpc/powernv: Move npu struct from pnv_phb to pci_controller
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
---
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 intdiag_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 = >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 = >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
