[RFC PATCH 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS

[RFC PATCH 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS

[Jayachandran C]

Add a new flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS to indicate bridges
that should not be considered during DMA alias search. This is
to support hardware (in this case Broadcom Vulcan PCIe subsystem)
that has internal bridges which have either missing or wrong PCIe
capabilities.

Update the function pci_for_each_dma_alias() to skip bridges with
this flag set.

Signed-off-by: Jayachandran C <jchandra@broadcom.com>
---

This patch is an RFC, if there is a better way to do this, please
let me know.

Thanks,
JC.

 drivers/pci/search.c | 2 ++
 include/linux/pci.h  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index a20ce7d..e5296aa 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -55,6 +55,8 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
 			continue;
 
 		tmp = bus->self;
+		if (tmp->dev_flags & PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS)
+			continue;
 
 		/*
 		 * PCIe-to-PCI/X bridges alias transactions from downstream
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 27df4a6..b4d8215 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -182,6 +182,8 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
 	/* Get VPD from function 0 VPD */
 	PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
+	/* Bridge should be ignored for alias search  */
+	PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS = (__force pci_dev_flags_t) (1 << 9),
 };
 
 enum pci_irq_reroute_variant {
-- 
1.9.1

[RFC PATCH 2/2] PCI: Quirks for Broadcom Vulcan

[Jayachandran C]

Handle 2 quirks of the Broadcom Vulcan PCI controller:
 - Make internal bridges to be skipped during DMA alias search
 - Skip BAR0 resource assignment internal bridges. The BARs of
   bridges cannot be assigned from the mem resource range.
---
 drivers/pci/quirks.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 0575a1e..afc186a 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3705,6 +3705,27 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
 DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
 
 /*
+ * Two levels of bridges in Broadcom Vulcan are not real PCI or PCIe bridges.
+ * These are internal bridges and should not be used for dma alias
+ * calculations. Additionally, the BAR0 of thes bridges should not be
+ * assigned with a mem resource from linux
+ */
+static void quirk_bridge_brcm_vulcan_internal(struct pci_dev *pdev)
+{
+	struct resource *r = &pdev->resource[0];
+
+	/* skip from alias search */
+	pdev->dev_flags |= PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS;
+
+	/* clear BAR0, should not be used from Linux */
+	memset(r, 0, sizeof(*r));
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9000,
+				quirk_bridge_brcm_vulcan_internal);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9039,
+				quirk_bridge_brcm_vulcan_internal);
+
+/*
  * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
  * class code.  Fix it.
  */
-- 
1.9.1

[RFC PATCH 2/2] PCI: Handle Broadcom Vulcan quirks

[Jayachandran C]

Handle two quirks of the PCI controller on Broadcom's Vulcan processor.
 - Mark internal bridges so that they are skipped during DMA alias
   search.
 - Skip BAR0 resource assignment for internal bridges. The BARs
   of internal bridges should not be assigned from the mem resource
   range.

Signed-off-by: Jayachandran C <jchandra@broadcom.com>
---

Resending, last patch was missing the Signed-off-by, also fixed the
comment a bit.

JC.

 drivers/pci/quirks.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 0575a1e..afc186a 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3705,6 +3705,27 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
 DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
 
 /*
+ * Two levels of bridges in Broadcom Vulcan are not real PCI or PCIe bridges.
+ * These are internal bridges and should not be used for dma alias
+ * calculations. Additionally, the BAR0 of thes bridges should not be
+ * assigned with a mem resource from linux
+ */
+static void quirk_bridge_brcm_vulcan_internal(struct pci_dev *pdev)
+{
+	struct resource *r = &pdev->resource[0];
+
+	/* skip from alias search */
+	pdev->dev_flags |= PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS;
+
+	/* clear BAR0, should not be used from Linux */
+	memset(r, 0, sizeof(*r));
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9000,
+				quirk_bridge_brcm_vulcan_internal);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9039,
+				quirk_bridge_brcm_vulcan_internal);
+
+/*
  * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
  * class code.  Fix it.
  */
-- 
1.9.1

Re: [RFC PATCH 2/2] PCI: Handle Broadcom Vulcan quirks

[Bjorn Helgaas]

On Mon, Feb 15, 2016 at 02:56:38PM +0530, Jayachandran C wrote:
> Handle two quirks of the PCI controller on Broadcom's Vulcan processor.
>  - Mark internal bridges so that they are skipped during DMA alias
>    search.
>  - Skip BAR0 resource assignment for internal bridges. The BARs
>    of internal bridges should not be assigned from the mem resource
>    range.
> 
> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> ---
> 
> Resending, last patch was missing the Signed-off-by, also fixed the
> comment a bit.

If you resend a patch, please increment the version number and resend
the entire series, no matter how minor the change was.  Version
numbers are free, and it's a hassle for me to sort out multiple
versions labeled with the same number.

> JC.
> 
>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 0575a1e..afc186a 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3705,6 +3705,27 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
>  DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
>  
>  /*
> + * Two levels of bridges in Broadcom Vulcan are not real PCI or PCIe bridges.
> + * These are internal bridges and should not be used for dma alias
> + * calculations. Additionally, the BAR0 of thes bridges should not be
> + * assigned with a mem resource from linux
> + */
> +static void quirk_bridge_brcm_vulcan_internal(struct pci_dev *pdev)
> +{
> +	struct resource *r = &pdev->resource[0];
> +
> +	/* skip from alias search */
> +	pdev->dev_flags |= PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS;
> +
> +	/* clear BAR0, should not be used from Linux */
> +	memset(r, 0, sizeof(*r));

This definitely needs some explanation.  The whole point of the
architected PCI config space header is so that generic OS code can
manage the device without having to add device-specific code.

Are you saying the register at 0x10 in config space is not actually a
BAR at all?  Or it is a BAR, but you don't think anybody should need
to use it?  BARs do not have enable bits, so no matter what value is
in the BAR, that value defines address space to which the device will
respond.  Linux needs to know about that, even if no driver actually
uses it.

> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9000,
> +				quirk_bridge_brcm_vulcan_internal);
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9039,
> +				quirk_bridge_brcm_vulcan_internal);
> +
> +/*
>   * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
>   * class code.  Fix it.
>   */
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 2/2] PCI: Handle Broadcom Vulcan quirks

[Jayachandran Chandrashekaran Nair]

On Tue, Feb 16, 2016 at 12:00 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Mon, Feb 15, 2016 at 02:56:38PM +0530, Jayachandran C wrote:
>> Handle two quirks of the PCI controller on Broadcom's Vulcan processor.
>>  - Mark internal bridges so that they are skipped during DMA alias
>>    search.
>>  - Skip BAR0 resource assignment for internal bridges. The BARs
>>    of internal bridges should not be assigned from the mem resource
>>    range.
>>
>> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
>> ---
>>
>> Resending, last patch was missing the Signed-off-by, also fixed the
>> comment a bit.
>
> If you resend a patch, please increment the version number and resend
> the entire series, no matter how minor the change was.  Version
> numbers are free, and it's a hassle for me to sort out multiple
> versions labeled with the same number.

Since it was an RFC, I thought setting the in-reply-to would be sufficient.
Looks like I was mistaken, sorry for the trouble.

>>
>>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 0575a1e..afc186a 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -3705,6 +3705,27 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
>>  DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
>>
>>  /*
>> + * Two levels of bridges in Broadcom Vulcan are not real PCI or PCIe bridges.
>> + * These are internal bridges and should not be used for dma alias
>> + * calculations. Additionally, the BAR0 of thes bridges should not be
>> + * assigned with a mem resource from linux
>> + */
>> +static void quirk_bridge_brcm_vulcan_internal(struct pci_dev *pdev)
>> +{
>> +     struct resource *r = &pdev->resource[0];
>> +
>> +     /* skip from alias search */
>> +     pdev->dev_flags |= PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS;
>> +
>> +     /* clear BAR0, should not be used from Linux */
>> +     memset(r, 0, sizeof(*r));
>
> This definitely needs some explanation.  The whole point of the
> architected PCI config space header is so that generic OS code can
> manage the device without having to add device-specific code.
>
> Are you saying the register at 0x10 in config space is not actually a
> BAR at all?  Or it is a BAR, but you don't think anybody should need
> to use it?

These is a proper BAR (64 bit pref) in the bridge's Type 1 header, but:
1. Assigning a memory resource from the pci memory window to this
   BAR causes the PCIe system to fail (this is a bug). So we cannot
   expose this BAR to standard Linux code without even more quirks
   and hacks.
2. This BAR is used for accessing debugging and private registers
   of the PCI controller, which is not useful in Linux.

So I thought it is better to handle it with a quirk to hide the BAR. The
device still works as a bridge and standard linux bridge configuration
happens correctly.

> BARs do not have enable bits, so no matter what value is
> in the BAR, that value defines address space to which the device will
> respond.  Linux needs to know about that, even if no driver actually
> uses it.

This is a good point. We might need to read the firmware setting of this
BAR and mark the physical address range reserved - but this may be
to document the value.

>> +}
>> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9000,
>> +                             quirk_bridge_brcm_vulcan_internal);
>> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9039,
>> +                             quirk_bridge_brcm_vulcan_internal);
>> +
>> +/*
>>   * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
>>   * class code.  Fix it.
>>   */

Thanks,
JC.

Re: [RFC PATCH 2/2] PCI: Handle Broadcom Vulcan quirks

[Bjorn Helgaas]

On Tue, Feb 16, 2016 at 09:47:34PM +0530, Jayachandran Chandrashekaran Nair wrote:
> On Tue, Feb 16, 2016 at 12:00 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Mon, Feb 15, 2016 at 02:56:38PM +0530, Jayachandran C wrote:
> >> Handle two quirks of the PCI controller on Broadcom's Vulcan processor.
> >>  - Mark internal bridges so that they are skipped during DMA alias
> >>    search.
> >>  - Skip BAR0 resource assignment for internal bridges. The BARs
> >>    of internal bridges should not be assigned from the mem resource
> >>    range.
> >>
> >> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> >> ---
> >>
> >> Resending, last patch was missing the Signed-off-by, also fixed the
> >> comment a bit.
> >
> > If you resend a patch, please increment the version number and resend
> > the entire series, no matter how minor the change was.  Version
> > numbers are free, and it's a hassle for me to sort out multiple
> > versions labeled with the same number.
> 
> Since it was an RFC, I thought setting the in-reply-to would be sufficient.
> Looks like I was mistaken, sorry for the trouble.
> 
> >>
> >>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
> >>  1 file changed, 21 insertions(+)
> >>
> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >> index 0575a1e..afc186a 100644
> >> --- a/drivers/pci/quirks.c
> >> +++ b/drivers/pci/quirks.c
> >> @@ -3705,6 +3705,27 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
> >>  DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
> >>
> >>  /*
> >> + * Two levels of bridges in Broadcom Vulcan are not real PCI or PCIe bridges.
> >> + * These are internal bridges and should not be used for dma alias
> >> + * calculations. Additionally, the BAR0 of thes bridges should not be
> >> + * assigned with a mem resource from linux
> >> + */
> >> +static void quirk_bridge_brcm_vulcan_internal(struct pci_dev *pdev)
> >> +{
> >> +     struct resource *r = &pdev->resource[0];
> >> +
> >> +     /* skip from alias search */
> >> +     pdev->dev_flags |= PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS;
> >> +
> >> +     /* clear BAR0, should not be used from Linux */
> >> +     memset(r, 0, sizeof(*r));
> >
> > This definitely needs some explanation.  The whole point of the
> > architected PCI config space header is so that generic OS code can
> > manage the device without having to add device-specific code.
> >
> > Are you saying the register at 0x10 in config space is not actually a
> > BAR at all?  Or it is a BAR, but you don't think anybody should need
> > to use it?
> 
> These is a proper BAR (64 bit pref) in the bridge's Type 1 header, but:
> 1. Assigning a memory resource from the pci memory window to this
>    BAR causes the PCIe system to fail (this is a bug). So we cannot
>    expose this BAR to standard Linux code without even more quirks
>    and hacks.

Are you saying assigning space for this BAR exposes a Linux PCI bug?
If so, I'd like to know more about that because we should fix it.

Or does it expose a hardware bug in the Broadcom device?

> 2. This BAR is used for accessing debugging and private registers
>    of the PCI controller, which is not useful in Linux.
> 
> So I thought it is better to handle it with a quirk to hide the BAR. The
> device still works as a bridge and standard linux bridge configuration
> happens correctly.

Unfortunately, there's no way in PCI for a device to communicate that
some of the resources it requests are more valuable than others.
There's no way for it to say "I'm asking for these resources (via the
BAR), but I didn't really mean it."

> > BARs do not have enable bits, so no matter what value is
> > in the BAR, that value defines address space to which the device will
> > respond.  Linux needs to know about that, even if no driver actually
> > uses it.
> 
> This is a good point. We might need to read the firmware setting of this
> BAR and mark the physical address range reserved - but this may be
> to document the value.
> 
> >> +}
> >> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9000,
> >> +                             quirk_bridge_brcm_vulcan_internal);
> >> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9039,
> >> +                             quirk_bridge_brcm_vulcan_internal);
> >> +
> >> +/*
> >>   * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
> >>   * class code.  Fix it.
> >>   */
> 
> Thanks,
> JC.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 2/2] PCI: Handle Broadcom Vulcan quirks

[Jayachandran Chandrashekaran Nair]

On Tue, Feb 16, 2016 at 10:44 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Tue, Feb 16, 2016 at 09:47:34PM +0530, Jayachandran Chandrashekaran Nair wrote:
>> On Tue, Feb 16, 2016 at 12:00 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > On Mon, Feb 15, 2016 at 02:56:38PM +0530, Jayachandran C wrote:
>> >> Handle two quirks of the PCI controller on Broadcom's Vulcan processor.
>> >>  - Mark internal bridges so that they are skipped during DMA alias
>> >>    search.
>> >>  - Skip BAR0 resource assignment for internal bridges. The BARs
>> >>    of internal bridges should not be assigned from the mem resource
>> >>    range.
>> >>
>> >> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
>> >> ---
>> >>
>> >> Resending, last patch was missing the Signed-off-by, also fixed the
>> >> comment a bit.
>> >
>> > If you resend a patch, please increment the version number and resend
>> > the entire series, no matter how minor the change was.  Version
>> > numbers are free, and it's a hassle for me to sort out multiple
>> > versions labeled with the same number.
>>
>> Since it was an RFC, I thought setting the in-reply-to would be sufficient.
>> Looks like I was mistaken, sorry for the trouble.
>>
>> >>
>> >>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
>> >>  1 file changed, 21 insertions(+)
>> >>
>> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> >> index 0575a1e..afc186a 100644
>> >> --- a/drivers/pci/quirks.c
>> >> +++ b/drivers/pci/quirks.c
>> >> @@ -3705,6 +3705,27 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
>> >>  DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
>> >>
>> >>  /*
>> >> + * Two levels of bridges in Broadcom Vulcan are not real PCI or PCIe bridges.
>> >> + * These are internal bridges and should not be used for dma alias
>> >> + * calculations. Additionally, the BAR0 of thes bridges should not be
>> >> + * assigned with a mem resource from linux
>> >> + */
>> >> +static void quirk_bridge_brcm_vulcan_internal(struct pci_dev *pdev)
>> >> +{
>> >> +     struct resource *r = &pdev->resource[0];
>> >> +
>> >> +     /* skip from alias search */
>> >> +     pdev->dev_flags |= PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS;
>> >> +
>> >> +     /* clear BAR0, should not be used from Linux */
>> >> +     memset(r, 0, sizeof(*r));
>> >
>> > This definitely needs some explanation.  The whole point of the
>> > architected PCI config space header is so that generic OS code can
>> > manage the device without having to add device-specific code.
>> >
>> > Are you saying the register at 0x10 in config space is not actually a
>> > BAR at all?  Or it is a BAR, but you don't think anybody should need
>> > to use it?
>>
>> These is a proper BAR (64 bit pref) in the bridge's Type 1 header, but:
>> 1. Assigning a memory resource from the pci memory window to this
>>    BAR causes the PCIe system to fail (this is a bug). So we cannot
>>    expose this BAR to standard Linux code without even more quirks
>>    and hacks.
>
> Are you saying assigning space for this BAR exposes a Linux PCI bug?
> If so, I'd like to know more about that because we should fix it.
>
> Or does it expose a hardware bug in the Broadcom device?

This is a hardware bug (or non-compliance). The BAR cannot be
assigned from the PCI MEM range. To use it we need to program
an address outside the areas assigned to PCI to the BAR. But like
I said it is better for us to ignore the BAR in linux and then the
device acts as a normal bridge.

>
>> 2. This BAR is used for accessing debugging and private registers
>>    of the PCI controller, which is not useful in Linux.
>>
>> So I thought it is better to handle it with a quirk to hide the BAR. The
>> device still works as a bridge and standard linux bridge configuration
>> happens correctly.
>
> Unfortunately, there's no way in PCI for a device to communicate that
> some of the resources it requests are more valuable than others.
> There's no way for it to say "I'm asking for these resources (via the
> BAR), but I didn't really mean it."
>

I think hiding the buggy BAR is the best option in my view.


>> > BARs do not have enable bits, so no matter what value is
>> > in the BAR, that value defines address space to which the device will
>> > respond.  Linux needs to know about that, even if no driver actually
>> > uses it.
>>
>> This is a good point. We might need to read the firmware setting of this
>> BAR and mark the physical address range reserved - but this may be
>> to document the value.
>>
>> >> +}
>> >> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9000,
>> >> +                             quirk_bridge_brcm_vulcan_internal);
>> >> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9039,
>> >> +                             quirk_bridge_brcm_vulcan_internal);
>> >> +
>> >> +/*
>> >>   * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
>> >>   * class code.  Fix it.
>> >>   */
>>

JC.

Re: [RFC PATCH 2/2] PCI: Handle Broadcom Vulcan quirks

[Bjorn Helgaas]

[+cc Arnd, Rob, DT host bridge description questions]

On Tue, Feb 16, 2016 at 11:39:41PM +0530, Jayachandran Chandrashekaran Nair wrote:
> On Tue, Feb 16, 2016 at 10:44 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Tue, Feb 16, 2016 at 09:47:34PM +0530, Jayachandran Chandrashekaran Nair wrote:
> >> On Tue, Feb 16, 2016 at 12:00 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >> > On Mon, Feb 15, 2016 at 02:56:38PM +0530, Jayachandran C wrote:
> >> >> Handle two quirks of the PCI controller on Broadcom's Vulcan processor.
> >> >>  - Mark internal bridges so that they are skipped during DMA alias
> >> >>    search.
> >> >>  - Skip BAR0 resource assignment for internal bridges. The BARs
> >> >>    of internal bridges should not be assigned from the mem resource
> >> >>    range.
> >> >> ...
> >> >>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
> >> >>  1 file changed, 21 insertions(+)
> >> >>
> >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >> >> index 0575a1e..afc186a 100644
> >> >> --- a/drivers/pci/quirks.c
> >> >> +++ b/drivers/pci/quirks.c
> >> >> @@ -3705,6 +3705,27 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
> >> >>  DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
> >> >>
> >> >>  /*
> >> >> + * Two levels of bridges in Broadcom Vulcan are not real PCI or PCIe bridges.
> >> >> + * These are internal bridges and should not be used for dma alias
> >> >> + * calculations. Additionally, the BAR0 of thes bridges should not be
> >> >> + * assigned with a mem resource from linux
> >> >> + */
> >> >> +static void quirk_bridge_brcm_vulcan_internal(struct pci_dev *pdev)
> >> >> +{
> >> >> +     struct resource *r = &pdev->resource[0];
> >> >> +
> >> >> +     /* skip from alias search */
> >> >> +     pdev->dev_flags |= PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS;
> >> >> +
> >> >> +     /* clear BAR0, should not be used from Linux */
> >> >> +     memset(r, 0, sizeof(*r));
> >> >
> >> > This definitely needs some explanation.  The whole point of the
> >> > architected PCI config space header is so that generic OS code can
> >> > manage the device without having to add device-specific code.
> >> >
> >> > Are you saying the register at 0x10 in config space is not actually a
> >> > BAR at all?  Or it is a BAR, but you don't think anybody should need
> >> > to use it?
> >>
> >> These is a proper BAR (64 bit pref) in the bridge's Type 1 header, but:
> >> 1. Assigning a memory resource from the pci memory window to this
> >>    BAR causes the PCIe system to fail (this is a bug). So we cannot
> >>    expose this BAR to standard Linux code without even more quirks
> >>    and hacks.
> >
> > Are you saying assigning space for this BAR exposes a Linux PCI bug?
> > If so, I'd like to know more about that because we should fix it.
> >
> > Or does it expose a hardware bug in the Broadcom device?
> 
> This is a hardware bug (or non-compliance). The BAR cannot be
> assigned from the PCI MEM range. To use it we need to program
> an address outside the areas assigned to PCI to the BAR. But like
> I said it is better for us to ignore the BAR in linux and then the
> device acts as a normal bridge.

Your PCI host bridge has a window of address space that it forwards
from the primary (CPU) side to the secondary (PCI) side.  I assume
that's what you mean by the "PCI MEM range."  Apparently if the BAR is
programmed inside that window, it causes some hardware error.  That
still seems strange; there's no driver for the device, and we know the
range is in use so it won't be assigned to any other device, so Linux
should never *access* the range.

Apparently if you program the BAR *outside* the window, the hardware
error does not happen, and the private registers are accessible.  But
Linux currently doesn't know where this space is.

If we ignore the BAR in Linux, apparently the hardware error does not
happen.  But the BAR still contains some value, so this is really the
same as having the BAR outside the window, presumably because it came
out of reset that way, or maybe firmware programmed it.

It sounds to me like you should do the following:

a) Have firmware program this BAR where you want it,

b) Describe these private registers as internal PCI bridge registers,
   using a DT "reg" property,

c) Describe the host bridge window (which doesn't include the above
   registers) using the normal "ranges" property, and

d) Arrange for config writes to BAR 0 to be dropped and for reads to
   return 0, maybe by checks in your config accessors.

> >> 2. This BAR is used for accessing debugging and private registers
> >>    of the PCI controller, which is not useful in Linux.
> >>
> >> So I thought it is better to handle it with a quirk to hide the BAR. The
> >> device still works as a bridge and standard linux bridge configuration
> >> happens correctly.
> >
> > Unfortunately, there's no way in PCI for a device to communicate that
> > some of the resources it requests are more valuable than others.
> > There's no way for it to say "I'm asking for these resources (via the
> > BAR), but I didn't really mean it."
> 
> I think hiding the buggy BAR is the best option in my view.
> 
> >> > BARs do not have enable bits, so no matter what value is
> >> > in the BAR, that value defines address space to which the device will
> >> > respond.  Linux needs to know about that, even if no driver actually
> >> > uses it.
> >>
> >> This is a good point. We might need to read the firmware setting of this
> >> BAR and mark the physical address range reserved - but this may be
> >> to document the value.
> >>
> >> >> +}
> >> >> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9000,
> >> >> +                             quirk_bridge_brcm_vulcan_internal);
> >> >> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_BROADCOM, 0x9039,
> >> >> +                             quirk_bridge_brcm_vulcan_internal);
> >> >> +
> >> >> +/*
> >> >>   * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
> >> >>   * class code.  Fix it.
> >> >>   */
> >>
> 
> JC.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 2/2] PCI: Handle Broadcom Vulcan quirks

[Arnd Bergmann]

On Tuesday 16 February 2016 15:03:37 Bjorn Helgaas wrote:
> 
> Your PCI host bridge has a window of address space that it forwards
> from the primary (CPU) side to the secondary (PCI) side.  I assume
> that's what you mean by the "PCI MEM range."  Apparently if the BAR is
> programmed inside that window, it causes some hardware error.  That
> still seems strange; there's no driver for the device, and we know the
> range is in use so it won't be assigned to any other device, so Linux
> should never *access* the range.
> 
> Apparently if you program the BAR *outside* the window, the hardware
> error does not happen, and the private registers are accessible.  But
> Linux currently doesn't know where this space is.
> 
> If we ignore the BAR in Linux, apparently the hardware error does not
> happen.  But the BAR still contains some value, so this is really the
> same as having the BAR outside the window, presumably because it came
> out of reset that way, or maybe firmware programmed it.
> 
> It sounds to me like you should do the following:
> 
> a) Have firmware program this BAR where you want it,
> 
> b) Describe these private registers as internal PCI bridge registers,
>    using a DT "reg" property,
> 
> c) Describe the host bridge window (which doesn't include the above
>    registers) using the normal "ranges" property, and
> 
> d) Arrange for config writes to BAR 0 to be dropped and for reads to
>    return 0, maybe by checks in your config accessors.

We should be able to express this in the ranges property as
a non-relocatable range, I'm pretty sure the PCI binding allows
this, but the Linux PCI code might not honor the flag at the
moment, which can be fixed.

	Arnd

Re: [RFC PATCH 2/2] PCI: Handle Broadcom Vulcan quirks

[Jayachandran Chandrashekaran Nair]

On Wed, Feb 17, 2016 at 2:33 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc Arnd, Rob, DT host bridge description questions]
>
> On Tue, Feb 16, 2016 at 11:39:41PM +0530, Jayachandran Chandrashekaran Nair wrote:
>> On Tue, Feb 16, 2016 at 10:44 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > On Tue, Feb 16, 2016 at 09:47:34PM +0530, Jayachandran Chandrashekaran Nair wrote:
>> >> On Tue, Feb 16, 2016 at 12:00 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> >> > On Mon, Feb 15, 2016 at 02:56:38PM +0530, Jayachandran C wrote:
>> >> >> Handle two quirks of the PCI controller on Broadcom's Vulcan processor.
>> >> >>  - Mark internal bridges so that they are skipped during DMA alias
>> >> >>    search.
>> >> >>  - Skip BAR0 resource assignment for internal bridges. The BARs
>> >> >>    of internal bridges should not be assigned from the mem resource
>> >> >>    range.
>> >> >> ...
>> >> >>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
>> >> >>  1 file changed, 21 insertions(+)
>> >> >>
>> >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> >> >> index 0575a1e..afc186a 100644
>> >> >> --- a/drivers/pci/quirks.c
>> >> >> +++ b/drivers/pci/quirks.c
>> >> >> @@ -3705,6 +3705,27 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
>> >> >>  DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
>> >> >>
>> >> >>  /*
>> >> >> + * Two levels of bridges in Broadcom Vulcan are not real PCI or PCIe bridges.
>> >> >> + * These are internal bridges and should not be used for dma alias
>> >> >> + * calculations. Additionally, the BAR0 of thes bridges should not be
>> >> >> + * assigned with a mem resource from linux
>> >> >> + */
>> >> >> +static void quirk_bridge_brcm_vulcan_internal(struct pci_dev *pdev)
>> >> >> +{
>> >> >> +     struct resource *r = &pdev->resource[0];
>> >> >> +
>> >> >> +     /* skip from alias search */
>> >> >> +     pdev->dev_flags |= PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS;
>> >> >> +
>> >> >> +     /* clear BAR0, should not be used from Linux */
>> >> >> +     memset(r, 0, sizeof(*r));
>> >> >
>> >> > This definitely needs some explanation.  The whole point of the
>> >> > architected PCI config space header is so that generic OS code can
>> >> > manage the device without having to add device-specific code.
>> >> >
>> >> > Are you saying the register at 0x10 in config space is not actually a
>> >> > BAR at all?  Or it is a BAR, but you don't think anybody should need
>> >> > to use it?
>> >>
>> >> These is a proper BAR (64 bit pref) in the bridge's Type 1 header, but:
>> >> 1. Assigning a memory resource from the pci memory window to this
>> >>    BAR causes the PCIe system to fail (this is a bug). So we cannot
>> >>    expose this BAR to standard Linux code without even more quirks
>> >>    and hacks.
>> >
>> > Are you saying assigning space for this BAR exposes a Linux PCI bug?
>> > If so, I'd like to know more about that because we should fix it.
>> >
>> > Or does it expose a hardware bug in the Broadcom device?
>>
>> This is a hardware bug (or non-compliance). The BAR cannot be
>> assigned from the PCI MEM range. To use it we need to program
>> an address outside the areas assigned to PCI to the BAR. But like
>> I said it is better for us to ignore the BAR in linux and then the
>> device acts as a normal bridge.
>
> Your PCI host bridge has a window of address space that it forwards
> from the primary (CPU) side to the secondary (PCI) side.  I assume
> that's what you mean by the "PCI MEM range."  Apparently if the BAR is
> programmed inside that window, it causes some hardware error.  That
> still seems strange; there's no driver for the device, and we know the
> range is in use so it won't be assigned to any other device, so Linux
> should never *access* the range.

This is correct. The write to this bridge BAR with a address from
the host bridge window will cause a hardware issue.

> Apparently if you program the BAR *outside* the window, the hardware
> error does not happen, and the private registers are accessible.  But
> Linux currently doesn't know where this space is.
>
> If we ignore the BAR in Linux, apparently the hardware error does not
> happen.  But the BAR still contains some value, so this is really the
> same as having the BAR outside the window, presumably because it came
> out of reset that way, or maybe firmware programmed it.

Yes.

> It sounds to me like you should do the following:
>
> a) Have firmware program this BAR where you want it,
>
> b) Describe these private registers as internal PCI bridge registers,
>    using a DT "reg" property,

Two ponts here:
- We have to support ACPI as well
- There is no need to use the private registers in linux, so the
   whole thing will be pointless.

> c) Describe the host bridge window (which doesn't include the above
>    registers) using the normal "ranges" property, and

This is standard code (and ACPI works as well)..

> d) Arrange for config writes to BAR 0 to be dropped and for reads to
>    return 0, maybe by checks in your config accessors.

Here we are hiding the BAR from linux using checks in config read
write (if i understand correctly). This will need custom config accessors
for Vulcan both on device tree as well as in ACPI.

The patch (above) is trying to do it in a much much simpler way by
clearing the bar0 resource in DECLARE_PCI_FIXUP_HEADER quirk.
I am not able to see why this is not valid.

Thanks,
JC.

Re: [RFC PATCH 2/2] PCI: Handle Broadcom Vulcan quirks

[Bjorn Helgaas]

On Wed, Feb 17, 2016 at 10:36:55PM +0530, Jayachandran Chandrashekaran Nair wrote:
> On Wed, Feb 17, 2016 at 2:33 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > [+cc Arnd, Rob, DT host bridge description questions]
> >
> > On Tue, Feb 16, 2016 at 11:39:41PM +0530, Jayachandran Chandrashekaran Nair wrote:
> >> On Tue, Feb 16, 2016 at 10:44 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >> > On Tue, Feb 16, 2016 at 09:47:34PM +0530, Jayachandran Chandrashekaran Nair wrote:
> >> >> On Tue, Feb 16, 2016 at 12:00 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >> >> > On Mon, Feb 15, 2016 at 02:56:38PM +0530, Jayachandran C wrote:
> >> >> >> Handle two quirks of the PCI controller on Broadcom's Vulcan processor.
> >> >> >>  - Mark internal bridges so that they are skipped during DMA alias
> >> >> >>    search.
> >> >> >>  - Skip BAR0 resource assignment for internal bridges. The BARs
> >> >> >>    of internal bridges should not be assigned from the mem resource
> >> >> >>    range.
> >> >> >> ...
> >> >> >>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
> >> >> >>  1 file changed, 21 insertions(+)
> >> >> >>
> >> >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >> >> >> index 0575a1e..afc186a 100644
> >> >> >> --- a/drivers/pci/quirks.c
> >> >> >> +++ b/drivers/pci/quirks.c
> >> >> >> @@ -3705,6 +3705,27 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
> >> >> >>  DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
> >> >> >>
> >> >> >>  /*
> >> >> >> + * Two levels of bridges in Broadcom Vulcan are not real PCI or PCIe bridges.
> >> >> >> + * These are internal bridges and should not be used for dma alias
> >> >> >> + * calculations. Additionally, the BAR0 of thes bridges should not be
> >> >> >> + * assigned with a mem resource from linux
> >> >> >> + */
> >> >> >> +static void quirk_bridge_brcm_vulcan_internal(struct pci_dev *pdev)
> >> >> >> +{
> >> >> >> +     struct resource *r = &pdev->resource[0];
> >> >> >> +
> >> >> >> +     /* skip from alias search */
> >> >> >> +     pdev->dev_flags |= PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS;
> >> >> >> +
> >> >> >> +     /* clear BAR0, should not be used from Linux */
> >> >> >> +     memset(r, 0, sizeof(*r));
> >> >> >
> >> >> > This definitely needs some explanation.  The whole point of the
> >> >> > architected PCI config space header is so that generic OS code can
> >> >> > manage the device without having to add device-specific code.
> >> >> >
> >> >> > Are you saying the register at 0x10 in config space is not actually a
> >> >> > BAR at all?  Or it is a BAR, but you don't think anybody should need
> >> >> > to use it?
> >> >>
> >> >> These is a proper BAR (64 bit pref) in the bridge's Type 1 header, but:
> >> >> 1. Assigning a memory resource from the pci memory window to this
> >> >>    BAR causes the PCIe system to fail (this is a bug). So we cannot
> >> >>    expose this BAR to standard Linux code without even more quirks
> >> >>    and hacks.
> >> >
> >> > Are you saying assigning space for this BAR exposes a Linux PCI bug?
> >> > If so, I'd like to know more about that because we should fix it.
> >> >
> >> > Or does it expose a hardware bug in the Broadcom device?
> >>
> >> This is a hardware bug (or non-compliance). The BAR cannot be
> >> assigned from the PCI MEM range. To use it we need to program
> >> an address outside the areas assigned to PCI to the BAR. But like
> >> I said it is better for us to ignore the BAR in linux and then the
> >> device acts as a normal bridge.
> >
> > Your PCI host bridge has a window of address space that it forwards
> > from the primary (CPU) side to the secondary (PCI) side.  I assume
> > that's what you mean by the "PCI MEM range."  Apparently if the BAR is
> > programmed inside that window, it causes some hardware error.  That
> > still seems strange; there's no driver for the device, and we know the
> > range is in use so it won't be assigned to any other device, so Linux
> > should never *access* the range.
> 
> This is correct. The write to this bridge BAR with a address from
> the host bridge window will cause a hardware issue.
> 
> > Apparently if you program the BAR *outside* the window, the hardware
> > error does not happen, and the private registers are accessible.  But
> > Linux currently doesn't know where this space is.
> >
> > If we ignore the BAR in Linux, apparently the hardware error does not
> > happen.  But the BAR still contains some value, so this is really the
> > same as having the BAR outside the window, presumably because it came
> > out of reset that way, or maybe firmware programmed it.
> 
> Yes.
> 
> > It sounds to me like you should do the following:
> >
> > a) Have firmware program this BAR where you want it,
> >
> > b) Describe these private registers as internal PCI bridge registers,
> >    using a DT "reg" property,
> 
> Two ponts here:
> - We have to support ACPI as well

ACPI can do something similar.  This register space would be described
in a _CRS method.  IIRC there is actually a bit in the _CRS descriptor
that was intended to tell you whether the resource is (a) consumed
directly by the device or (b) forwarded to downstream devices.  But I
think BIOSes didn't use that bit correctly, so it's useless, so the
_CRS method might have to be on a different device.

> - There is no need to use the private registers in linux, so the
>    whole thing will be pointless.
> 
> > c) Describe the host bridge window (which doesn't include the above
> >    registers) using the normal "ranges" property, and
> 
> This is standard code (and ACPI works as well)..
> 
> > d) Arrange for config writes to BAR 0 to be dropped and for reads to
> >    return 0, maybe by checks in your config accessors.
> 
> Here we are hiding the BAR from linux using checks in config read
> write (if i understand correctly). This will need custom config accessors
> for Vulcan both on device tree as well as in ACPI.

Config accessors are usually not specific to DT or ACPI.

> The patch (above) is trying to do it in a much much simpler way by
> clearing the bar0 resource in DECLARE_PCI_FIXUP_HEADER quirk.
> I am not able to see why this is not valid.

I think there are two problems:

1) If the device responds to any address space, Linux needs to know
about it.  If we don't know about it, we might assign that space to
something else.

2) The PCI core still reads and writes the BAR to size it, and we
don't know whether this will trigger the hardware issue.

*You* might know that neither of these is an issue today, but special
incidental knowledge like that is a maintenance problem because I
can't tell what PCI core changes might make them an issue in the
future.

Bjorn

Re: [RFC PATCH 2/2] PCI: Handle Broadcom Vulcan quirks

[Jayachandran Chandrashekaran Nair]

On Thu, Feb 18, 2016 at 9:19 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Wed, Feb 17, 2016 at 10:36:55PM +0530, Jayachandran Chandrashekaran Nair wrote:
>> On Wed, Feb 17, 2016 at 2:33 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > [+cc Arnd, Rob, DT host bridge description questions]
>> >
>> > On Tue, Feb 16, 2016 at 11:39:41PM +0530, Jayachandran Chandrashekaran Nair wrote:
>> >> On Tue, Feb 16, 2016 at 10:44 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> >> > On Tue, Feb 16, 2016 at 09:47:34PM +0530, Jayachandran Chandrashekaran Nair wrote:
>> >> >> On Tue, Feb 16, 2016 at 12:00 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> >> >> > On Mon, Feb 15, 2016 at 02:56:38PM +0530, Jayachandran C wrote:
>> >> >> >> Handle two quirks of the PCI controller on Broadcom's Vulcan processor.
>> >> >> >>  - Mark internal bridges so that they are skipped during DMA alias
>> >> >> >>    search.
>> >> >> >>  - Skip BAR0 resource assignment for internal bridges. The BARs
>> >> >> >>    of internal bridges should not be assigned from the mem resource
>> >> >> >>    range.
>> >> >> >> ...
>> >> >> >>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
>> >> >> >>  1 file changed, 21 insertions(+)
>> >> >> >>
>> >> >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> >> >> >> index 0575a1e..afc186a 100644
>> >> >> >> --- a/drivers/pci/quirks.c
>> >> >> >> +++ b/drivers/pci/quirks.c
>> >> >> >> @@ -3705,6 +3705,27 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
>> >> >> >>  DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
>> >> >> >>
>> >> >> >>  /*
>> >> >> >> + * Two levels of bridges in Broadcom Vulcan are not real PCI or PCIe bridges.
>> >> >> >> + * These are internal bridges and should not be used for dma alias
>> >> >> >> + * calculations. Additionally, the BAR0 of thes bridges should not be
>> >> >> >> + * assigned with a mem resource from linux
>> >> >> >> + */
>> >> >> >> +static void quirk_bridge_brcm_vulcan_internal(struct pci_dev *pdev)
>> >> >> >> +{
>> >> >> >> +     struct resource *r = &pdev->resource[0];
>> >> >> >> +
>> >> >> >> +     /* skip from alias search */
>> >> >> >> +     pdev->dev_flags |= PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS;
>> >> >> >> +
>> >> >> >> +     /* clear BAR0, should not be used from Linux */
>> >> >> >> +     memset(r, 0, sizeof(*r));
>> >> >> >
>> >> >> > This definitely needs some explanation.  The whole point of the
>> >> >> > architected PCI config space header is so that generic OS code can
>> >> >> > manage the device without having to add device-specific code.
>> >> >> >
>> >> >> > Are you saying the register at 0x10 in config space is not actually a
>> >> >> > BAR at all?  Or it is a BAR, but you don't think anybody should need
>> >> >> > to use it?
>> >> >>
>> >> >> These is a proper BAR (64 bit pref) in the bridge's Type 1 header, but:
>> >> >> 1. Assigning a memory resource from the pci memory window to this
>> >> >>    BAR causes the PCIe system to fail (this is a bug). So we cannot
>> >> >>    expose this BAR to standard Linux code without even more quirks
>> >> >>    and hacks.
>> >> >
>> >> > Are you saying assigning space for this BAR exposes a Linux PCI bug?
>> >> > If so, I'd like to know more about that because we should fix it.
>> >> >
>> >> > Or does it expose a hardware bug in the Broadcom device?
>> >>
>> >> This is a hardware bug (or non-compliance). The BAR cannot be
>> >> assigned from the PCI MEM range. To use it we need to program
>> >> an address outside the areas assigned to PCI to the BAR. But like
>> >> I said it is better for us to ignore the BAR in linux and then the
>> >> device acts as a normal bridge.
>> >
>> > Your PCI host bridge has a window of address space that it forwards
>> > from the primary (CPU) side to the secondary (PCI) side.  I assume
>> > that's what you mean by the "PCI MEM range."  Apparently if the BAR is
>> > programmed inside that window, it causes some hardware error.  That
>> > still seems strange; there's no driver for the device, and we know the
>> > range is in use so it won't be assigned to any other device, so Linux
>> > should never *access* the range.
>>
>> This is correct. The write to this bridge BAR with a address from
>> the host bridge window will cause a hardware issue.
>>
>> > Apparently if you program the BAR *outside* the window, the hardware
>> > error does not happen, and the private registers are accessible.  But
>> > Linux currently doesn't know where this space is.
>> >
>> > If we ignore the BAR in Linux, apparently the hardware error does not
>> > happen.  But the BAR still contains some value, so this is really the
>> > same as having the BAR outside the window, presumably because it came
>> > out of reset that way, or maybe firmware programmed it.
>>
>> Yes.
>>
>> > It sounds to me like you should do the following:
>> >
>> > a) Have firmware program this BAR where you want it,
>> >
>> > b) Describe these private registers as internal PCI bridge registers,
>> >    using a DT "reg" property,
>>
>> Two ponts here:
>> - We have to support ACPI as well
>
> ACPI can do something similar.  This register space would be described
> in a _CRS method.  IIRC there is actually a bit in the _CRS descriptor
> that was intended to tell you whether the resource is (a) consumed
> directly by the device or (b) forwarded to downstream devices.  But I
> think BIOSes didn't use that bit correctly, so it's useless, so the
> _CRS method might have to be on a different device.
>
>> - There is no need to use the private registers in linux, so the
>>    whole thing will be pointless.
>>
>> > c) Describe the host bridge window (which doesn't include the above
>> >    registers) using the normal "ranges" property, and
>>
>> This is standard code (and ACPI works as well)..
>>
>> > d) Arrange for config writes to BAR 0 to be dropped and for reads to
>> >    return 0, maybe by checks in your config accessors.
>>
>> Here we are hiding the BAR from linux using checks in config read
>> write (if i understand correctly). This will need custom config accessors
>> for Vulcan both on device tree as well as in ACPI.
>
> Config accessors are usually not specific to DT or ACPI.
>
>> The patch (above) is trying to do it in a much much simpler way by
>> clearing the bar0 resource in DECLARE_PCI_FIXUP_HEADER quirk.
>> I am not able to see why this is not valid.
>
> I think there are two problems:
>
> 1) If the device responds to any address space, Linux needs to know
> about it.  If we don't know about it, we might assign that space to
> something else.

If it is really needed I can mark the address space as reserved. And
Linux generally should not use physical address space outside the
areas specified by firmware of device tree.

> 2) The PCI core still reads and writes the BAR to size it, and we
> don't know whether this will trigger the hardware issue.

The sizing read/write is done after disabling the BAR by writing
to CMD register - I don't see why you think this will be a problem.

> *You* might know that neither of these is an issue today, but special
> incidental knowledge like that is a maintenance problem because I
> can't tell what PCI core changes might make them an issue in the
> future.

The worst case maintenance problem is that a future PCI change
might break our chip. This is not unexpected in Linux, and such
breakages can be easily fixed.

Anyway, looks like you do not like the approach, so I will go back
and see if anything else can be done.

Thanks.
JC.

Re: [RFC PATCH 2/2] PCI: Handle Broadcom Vulcan quirks

[Bjorn Helgaas]

[+cc Andi, linux-kernel, x86 for Intel Broadwell-EP non-BAR issue]

On Tue, Feb 23, 2016 at 08:10:03PM +0530, Jayachandran Chandrashekaran Nair wrote:
> On Thu, Feb 18, 2016 at 9:19 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Feb 17, 2016 at 10:36:55PM +0530, Jayachandran Chandrashekaran Nair wrote:
> >> On Wed, Feb 17, 2016 at 2:33 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >> > [+cc Arnd, Rob, DT host bridge description questions]
> >> >
> >> > On Tue, Feb 16, 2016 at 11:39:41PM +0530, Jayachandran Chandrashekaran Nair wrote:
> >> >> On Tue, Feb 16, 2016 at 10:44 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >> >> > On Tue, Feb 16, 2016 at 09:47:34PM +0530, Jayachandran Chandrashekaran Nair wrote:
> >> >> >> On Tue, Feb 16, 2016 at 12:00 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >> >> >> > On Mon, Feb 15, 2016 at 02:56:38PM +0530, Jayachandran C wrote:
> >> >> >> >> Handle two quirks of the PCI controller on Broadcom's Vulcan processor.
> >> >> >> >>  - Mark internal bridges so that they are skipped during DMA alias
> >> >> >> >>    search.
> >> >> >> >>  - Skip BAR0 resource assignment for internal bridges. The BARs
> >> >> >> >>    of internal bridges should not be assigned from the mem resource
> >> >> >> >>    range.
> >> >> >> >> ...
> >> >> >> >>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
> >> >> >> >>  1 file changed, 21 insertions(+)
> >> >> >> >>
> >> >> >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >> >> >> >> index 0575a1e..afc186a 100644
> >> >> >> >> --- a/drivers/pci/quirks.c
> >> >> >> >> +++ b/drivers/pci/quirks.c
> >> >> >> >> @@ -3705,6 +3705,27 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
> >> >> >> >>  DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
> >> >> >> >>
> >> >> >> >>  /*
> >> >> >> >> + * Two levels of bridges in Broadcom Vulcan are not real PCI or PCIe bridges.
> >> >> >> >> + * These are internal bridges and should not be used for dma alias
> >> >> >> >> + * calculations. Additionally, the BAR0 of thes bridges should not be
> >> >> >> >> + * assigned with a mem resource from linux
> >> >> >> >> + */
> >> >> >> >> +static void quirk_bridge_brcm_vulcan_internal(struct pci_dev *pdev)
> >> >> >> >> +{
> >> >> >> >> +     struct resource *r = &pdev->resource[0];
> >> >> >> >> +
> >> >> >> >> +     /* skip from alias search */
> >> >> >> >> +     pdev->dev_flags |= PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS;
> >> >> >> >> +
> >> >> >> >> +     /* clear BAR0, should not be used from Linux */
> >> >> >> >> +     memset(r, 0, sizeof(*r));
> >> >> >> >
> >> >> >> > This definitely needs some explanation.  The whole point of the
> >> >> >> > architected PCI config space header is so that generic OS code can
> >> >> >> > manage the device without having to add device-specific code.
> >> >> >> >
> >> >> >> > Are you saying the register at 0x10 in config space is not actually a
> >> >> >> > BAR at all?  Or it is a BAR, but you don't think anybody should need
> >> >> >> > to use it?
> >> >> >>
> >> >> >> These is a proper BAR (64 bit pref) in the bridge's Type 1 header, but:
> >> >> >> 1. Assigning a memory resource from the pci memory window to this
> >> >> >>    BAR causes the PCIe system to fail (this is a bug). So we cannot
> >> >> >>    expose this BAR to standard Linux code without even more quirks
> >> >> >>    and hacks.
> >> >> >
> >> >> > Are you saying assigning space for this BAR exposes a Linux PCI bug?
> >> >> > If so, I'd like to know more about that because we should fix it.
> >> >> >
> >> >> > Or does it expose a hardware bug in the Broadcom device?
> >> >>
> >> >> This is a hardware bug (or non-compliance). The BAR cannot be
> >> >> assigned from the PCI MEM range. To use it we need to program
> >> >> an address outside the areas assigned to PCI to the BAR. But like
> >> >> I said it is better for us to ignore the BAR in linux and then the
> >> >> device acts as a normal bridge.
> >> >
> >> > Your PCI host bridge has a window of address space that it forwards
> >> > from the primary (CPU) side to the secondary (PCI) side.  I assume
> >> > that's what you mean by the "PCI MEM range."  Apparently if the BAR is
> >> > programmed inside that window, it causes some hardware error.  That
> >> > still seems strange; there's no driver for the device, and we know the
> >> > range is in use so it won't be assigned to any other device, so Linux
> >> > should never *access* the range.
> >>
> >> This is correct. The write to this bridge BAR with a address from
> >> the host bridge window will cause a hardware issue.
> >>
> >> > Apparently if you program the BAR *outside* the window, the hardware
> >> > error does not happen, and the private registers are accessible.  But
> >> > Linux currently doesn't know where this space is.
> >> >
> >> > If we ignore the BAR in Linux, apparently the hardware error does not
> >> > happen.  But the BAR still contains some value, so this is really the
> >> > same as having the BAR outside the window, presumably because it came
> >> > out of reset that way, or maybe firmware programmed it.
> >>
> >> Yes.
> >>
> >> > It sounds to me like you should do the following:
> >> >
> >> > a) Have firmware program this BAR where you want it,
> >> >
> >> > b) Describe these private registers as internal PCI bridge registers,
> >> >    using a DT "reg" property,
> >>
> >> Two ponts here:
> >> - We have to support ACPI as well
> >
> > ACPI can do something similar.  This register space would be described
> > in a _CRS method.  IIRC there is actually a bit in the _CRS descriptor
> > that was intended to tell you whether the resource is (a) consumed
> > directly by the device or (b) forwarded to downstream devices.  But I
> > think BIOSes didn't use that bit correctly, so it's useless, so the
> > _CRS method might have to be on a different device.
> >
> >> - There is no need to use the private registers in linux, so the
> >>    whole thing will be pointless.
> >>
> >> > c) Describe the host bridge window (which doesn't include the above
> >> >    registers) using the normal "ranges" property, and
> >>
> >> This is standard code (and ACPI works as well)..
> >>
> >> > d) Arrange for config writes to BAR 0 to be dropped and for reads to
> >> >    return 0, maybe by checks in your config accessors.
> >>
> >> Here we are hiding the BAR from linux using checks in config read
> >> write (if i understand correctly). This will need custom config accessors
> >> for Vulcan both on device tree as well as in ACPI.
> >
> > Config accessors are usually not specific to DT or ACPI.
> >
> >> The patch (above) is trying to do it in a much much simpler way by
> >> clearing the bar0 resource in DECLARE_PCI_FIXUP_HEADER quirk.
> >> I am not able to see why this is not valid.
> >
> > I think there are two problems:
> >
> > 1) If the device responds to any address space, Linux needs to know
> > about it.  If we don't know about it, we might assign that space to
> > something else.
> 
> If it is really needed I can mark the address space as reserved. And
> Linux generally should not use physical address space outside the
> areas specified by firmware of device tree.

That's true, at least in theory.  Historically on x86, we only avoided
areas marked as reserved.  We *should* have only used areas marked as
available for MMIO, but Linux didn't pay much attention to the _CRS
methods that tell us that.  We just assumed anything that wasn't RAM
and wasn't marked as reserved in E820 was fair game for use by PCI.

> > 2) The PCI core still reads and writes the BAR to size it, and we
> > don't know whether this will trigger the hardware issue.
> 
> The sizing read/write is done after disabling the BAR by writing
> to CMD register - I don't see why you think this will be a problem.

You're right; I forgot about that CMD enable bit.  There are a few
cases where we don't disable decoding (see mmio_always_on), the most
important being for PCI_CLASS_BRIDGE_HOST devices.  But I think your
device is a PCI_CLASS_BRIDGE_PCI device, not a BRIDGE_HOST device, so
we should disable decoding on your device.

> > *You* might know that neither of these is an issue today, but special
> > incidental knowledge like that is a maintenance problem because I
> > can't tell what PCI core changes might make them an issue in the
> > future.
> 
> The worst case maintenance problem is that a future PCI change
> might break our chip. This is not unexpected in Linux, and such
> breakages can be easily fixed.

It's certainly unexpected by *me* :)  And the burden of fixing such
breakages doesn't scale well -- it usually involves triage by somebody
other than the chip folks, e.g., me.

Andi is doing something similar for some Broadwell-EP devices that
have BARs that aren't really BARs.  I suggested custom config
accessors, which is fairly clean from the core point of view, but it's
definitely clunky on the device side.

Andi's first proposals were to use a quirk to set a flag bit that told
us not to touch the BAR.  I'm starting to think that's a cleaner
approach.  I think it would be better to put the bit(s) in the
pci_dev, so generic struct resource users wouldn't see them, set them
via early quirks, then have __pci_read_base() check them and just
leave the struct resource zeroed out.

Bjorn

Re: [RFC PATCH 2/2] PCI: Handle Broadcom Vulcan quirks

[Jayachandran Chandrashekaran Nair]

Hi Bjorn,

On Tue, Feb 23, 2016 at 8:42 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Andi, linux-kernel, x86 for Intel Broadwell-EP non-BAR issue]
>
> On Tue, Feb 23, 2016 at 08:10:03PM +0530, Jayachandran Chandrashekaran Nair wrote:
> > On Thu, Feb 18, 2016 at 9:19 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Wed, Feb 17, 2016 at 10:36:55PM +0530, Jayachandran Chandrashekaran Nair wrote:
> > >> On Wed, Feb 17, 2016 at 2:33 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >> > [+cc Arnd, Rob, DT host bridge description questions]
> > >> >
> > >> > On Tue, Feb 16, 2016 at 11:39:41PM +0530, Jayachandran Chandrashekaran Nair wrote:
> > >> >> On Tue, Feb 16, 2016 at 10:44 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >> >> > On Tue, Feb 16, 2016 at 09:47:34PM +0530, Jayachandran Chandrashekaran Nair wrote:
> > >> >> >> On Tue, Feb 16, 2016 at 12:00 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >> >> >> > On Mon, Feb 15, 2016 at 02:56:38PM +0530, Jayachandran C wrote:
> > >> >> >> >> Handle two quirks of the PCI controller on Broadcom's Vulcan processor.
> > >> >> >> >>  - Mark internal bridges so that they are skipped during DMA alias
> > >> >> >> >>    search.
> > >> >> >> >>  - Skip BAR0 resource assignment for internal bridges. The BARs
> > >> >> >> >>    of internal bridges should not be assigned from the mem resource
> > >> >> >> >>    range.
> > >> >> >> >> ...
> > >> >> >> >>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
> > >> >> >> >>  1 file changed, 21 insertions(+)
> > >> >> >> >>
> > >> >> >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > >> >> >> >> index 0575a1e..afc186a 100644
> > >> >> >> >> --- a/drivers/pci/quirks.c
> > >> >> >> >> +++ b/drivers/pci/quirks.c
> > >> >> >> >> @@ -3705,6 +3705,27 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
> > >> >> >> >>  DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
> > >> >> >> >>
> > >> >> >> >>  /*
> > >> >> >> >> + * Two levels of bridges in Broadcom Vulcan are not real PCI or PCIe bridges.
> > >> >> >> >> + * These are internal bridges and should not be used for dma alias
> > >> >> >> >> + * calculations. Additionally, the BAR0 of thes bridges should not be
> > >> >> >> >> + * assigned with a mem resource from linux
> > >> >> >> >> + */
> > >> >> >> >> +static void quirk_bridge_brcm_vulcan_internal(struct pci_dev *pdev)
> > >> >> >> >> +{
> > >> >> >> >> +     struct resource *r = &pdev->resource[0];
> > >> >> >> >> +
> > >> >> >> >> +     /* skip from alias search */
> > >> >> >> >> +     pdev->dev_flags |= PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS;
> > >> >> >> >> +
> > >> >> >> >> +     /* clear BAR0, should not be used from Linux */
> > >> >> >> >> +     memset(r, 0, sizeof(*r));
> > >> >> >> >
> > >> >> >> > This definitely needs some explanation.  The whole point of the
> > >> >> >> > architected PCI config space header is so that generic OS code can
> > >> >> >> > manage the device without having to add device-specific code.
> > >> >> >> >
> > >> >> >> > Are you saying the register at 0x10 in config space is not actually a
> > >> >> >> > BAR at all?  Or it is a BAR, but you don't think anybody should need
> > >> >> >> > to use it?
> > >> >> >>
> > >> >> >> These is a proper BAR (64 bit pref) in the bridge's Type 1 header, but:
> > >> >> >> 1. Assigning a memory resource from the pci memory window to this
> > >> >> >>    BAR causes the PCIe system to fail (this is a bug). So we cannot
> > >> >> >>    expose this BAR to standard Linux code without even more quirks
> > >> >> >>    and hacks.
> > >> >> >
> > >> >> > Are you saying assigning space for this BAR exposes a Linux PCI bug?
> > >> >> > If so, I'd like to know more about that because we should fix it.
> > >> >> >
> > >> >> > Or does it expose a hardware bug in the Broadcom device?
> > >> >>
> > >> >> This is a hardware bug (or non-compliance). The BAR cannot be
> > >> >> assigned from the PCI MEM range. To use it we need to program
> > >> >> an address outside the areas assigned to PCI to the BAR. But like
> > >> >> I said it is better for us to ignore the BAR in linux and then the
> > >> >> device acts as a normal bridge.
> > >> >
> > >> > Your PCI host bridge has a window of address space that it forwards
> > >> > from the primary (CPU) side to the secondary (PCI) side.  I assume
> > >> > that's what you mean by the "PCI MEM range."  Apparently if the BAR is
> > >> > programmed inside that window, it causes some hardware error.  That
> > >> > still seems strange; there's no driver for the device, and we know the
> > >> > range is in use so it won't be assigned to any other device, so Linux
> > >> > should never *access* the range.
> > >>
> > >> This is correct. The write to this bridge BAR with a address from
> > >> the host bridge window will cause a hardware issue.
> > >>
> > >> > Apparently if you program the BAR *outside* the window, the hardware
> > >> > error does not happen, and the private registers are accessible.  But
> > >> > Linux currently doesn't know where this space is.
> > >> >
> > >> > If we ignore the BAR in Linux, apparently the hardware error does not
> > >> > happen.  But the BAR still contains some value, so this is really the
> > >> > same as having the BAR outside the window, presumably because it came
> > >> > out of reset that way, or maybe firmware programmed it.
> > >>
> > >> Yes.
> > >>
> > >> > It sounds to me like you should do the following:
> > >> >
> > >> > a) Have firmware program this BAR where you want it,
> > >> >
> > >> > b) Describe these private registers as internal PCI bridge registers,
> > >> >    using a DT "reg" property,
> > >>
> > >> Two ponts here:
> > >> - We have to support ACPI as well
> > >
> > > ACPI can do something similar.  This register space would be described
> > > in a _CRS method.  IIRC there is actually a bit in the _CRS descriptor
> > > that was intended to tell you whether the resource is (a) consumed
> > > directly by the device or (b) forwarded to downstream devices.  But I
> > > think BIOSes didn't use that bit correctly, so it's useless, so the
> > > _CRS method might have to be on a different device.
> > >
> > >> - There is no need to use the private registers in linux, so the
> > >>    whole thing will be pointless.
> > >>
> > >> > c) Describe the host bridge window (which doesn't include the above
> > >> >    registers) using the normal "ranges" property, and
> > >>
> > >> This is standard code (and ACPI works as well)..
> > >>
> > >> > d) Arrange for config writes to BAR 0 to be dropped and for reads to
> > >> >    return 0, maybe by checks in your config accessors.
> > >>
> > >> Here we are hiding the BAR from linux using checks in config read
> > >> write (if i understand correctly). This will need custom config accessors
> > >> for Vulcan both on device tree as well as in ACPI.
> > >
> > > Config accessors are usually not specific to DT or ACPI.
> > >
> > >> The patch (above) is trying to do it in a much much simpler way by
> > >> clearing the bar0 resource in DECLARE_PCI_FIXUP_HEADER quirk.
> > >> I am not able to see why this is not valid.
> > >
> > > I think there are two problems:
> > >
> > > 1) If the device responds to any address space, Linux needs to know
> > > about it.  If we don't know about it, we might assign that space to
> > > something else.
> >
> > If it is really needed I can mark the address space as reserved. And
> > Linux generally should not use physical address space outside the
> > areas specified by firmware of device tree.
>
> That's true, at least in theory.  Historically on x86, we only avoided
> areas marked as reserved.  We *should* have only used areas marked as
> available for MMIO, but Linux didn't pay much attention to the _CRS
> methods that tell us that.  We just assumed anything that wasn't RAM
> and wasn't marked as reserved in E820 was fair game for use by PCI.
>
> > > 2) The PCI core still reads and writes the BAR to size it, and we
> > > don't know whether this will trigger the hardware issue.
> >
> > The sizing read/write is done after disabling the BAR by writing
> > to CMD register - I don't see why you think this will be a problem.
>
> You're right; I forgot about that CMD enable bit.  There are a few
> cases where we don't disable decoding (see mmio_always_on), the most
> important being for PCI_CLASS_BRIDGE_HOST devices.  But I think your
> device is a PCI_CLASS_BRIDGE_PCI device, not a BRIDGE_HOST device, so
> we should disable decoding on your device.
>
> > > *You* might know that neither of these is an issue today, but special
> > > incidental knowledge like that is a maintenance problem because I
> > > can't tell what PCI core changes might make them an issue in the
> > > future.
> >
> > The worst case maintenance problem is that a future PCI change
> > might break our chip. This is not unexpected in Linux, and such
> > breakages can be easily fixed.
>
> It's certainly unexpected by *me* :)  And the burden of fixing such
> breakages doesn't scale well -- it usually involves triage by somebody
> other than the chip folks, e.g., me.
>
> Andi is doing something similar for some Broadwell-EP devices that
> have BARs that aren't really BARs.  I suggested custom config
> accessors, which is fairly clean from the core point of view, but it's
> definitely clunky on the device side.
>
> Andi's first proposals were to use a quirk to set a flag bit that told
> us not to touch the BAR.  I'm starting to think that's a cleaner
> approach.  I think it would be better to put the bit(s) in the
> pci_dev, so generic struct resource users wouldn't see them, set them
> via early quirks, then have __pci_read_base() check them and just
> leave the struct resource zeroed out.

I spent some more time working with our hardware team on the nature
of this issue. It looks like this is not a problem if we setup the PCI
configuration in firmware better (this has to do with the sequence for
configuring the PCI controller in firmware).

So I will drop the part in my patch that hides the BARs from linux, since
the standard Linux resource allocation code will work Sorry for taking up
your time on this.

Thanks,
JC.

Re: [RFC PATCH 2/2] PCI: Handle Broadcom Vulcan quirks

[Bjorn Helgaas]

On Sat, Feb 27, 2016 at 01:44:49PM +0530, Jayachandran Chandrashekaran Nair wrote:
> On Tue, Feb 23, 2016 at 8:42 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Tue, Feb 23, 2016 at 08:10:03PM +0530, Jayachandran Chandrashekaran Nair wrote:
> > > On Thu, Feb 18, 2016 at 9:19 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Wed, Feb 17, 2016 at 10:36:55PM +0530, Jayachandran Chandrashekaran Nair wrote:
> > > >> On Wed, Feb 17, 2016 at 2:33 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > >> > On Tue, Feb 16, 2016 at 11:39:41PM +0530, Jayachandran Chandrashekaran Nair wrote:
> > > >> >> On Tue, Feb 16, 2016 at 10:44 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > >> >> > On Tue, Feb 16, 2016 at 09:47:34PM +0530, Jayachandran Chandrashekaran Nair wrote:
> > > >> >> >> On Tue, Feb 16, 2016 at 12:00 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > >> >> >> > On Mon, Feb 15, 2016 at 02:56:38PM +0530, Jayachandran C wrote:
> > > >> >> >> >> Handle two quirks of the PCI controller on Broadcom's Vulcan processor.
> > > >> >> >> >>  - Mark internal bridges so that they are skipped during DMA alias
> > > >> >> >> >>    search.
> > > >> >> >> >>  - Skip BAR0 resource assignment for internal bridges. The BARs
> > > >> >> >> >>    of internal bridges should not be assigned from the mem resource
> > > >> >> >> >>    range.
> > > >> >> >> >> ...
> > > >> >> >> >>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
> > > >> >> >> >>  1 file changed, 21 insertions(+)
> > > >> >> >> >>
> > > >> >> >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > >> >> >> >> index 0575a1e..afc186a 100644
> > > >> >> >> >> --- a/drivers/pci/quirks.c
> > > >> >> >> >> +++ b/drivers/pci/quirks.c
> > > >> >> >> >> @@ -3705,6 +3705,27 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
> > > >> >> >> >>  DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
> > > >> >> >> >>
> > > >> >> >> >>  /*
> > > >> >> >> >> + * Two levels of bridges in Broadcom Vulcan are not real PCI or PCIe bridges.
> > > >> >> >> >> + * These are internal bridges and should not be used for dma alias
> > > >> >> >> >> + * calculations. Additionally, the BAR0 of thes bridges should not be
> > > >> >> >> >> + * assigned with a mem resource from linux
> > > >> >> >> >> + */
> > > >> >> >> >> +static void quirk_bridge_brcm_vulcan_internal(struct pci_dev *pdev)
> > > >> >> >> >> +{
> > > >> >> >> >> +     struct resource *r = &pdev->resource[0];
> > > >> >> >> >> +
> > > >> >> >> >> +     /* skip from alias search */
> > > >> >> >> >> +     pdev->dev_flags |= PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS;
> > > >> >> >> >> +
> > > >> >> >> >> +     /* clear BAR0, should not be used from Linux */
> > > >> >> >> >> +     memset(r, 0, sizeof(*r));
> > > >> >> >> >
> > > >> >> >> > This definitely needs some explanation.  The whole point of the
> > > >> >> >> > architected PCI config space header is so that generic OS code can
> > > >> >> >> > manage the device without having to add device-specific code.
> > > >> >> >> >
> > > >> >> >> > Are you saying the register at 0x10 in config space is not actually a
> > > >> >> >> > BAR at all?  Or it is a BAR, but you don't think anybody should need
> > > >> >> >> > to use it?
> > > >> >> >>
> > > >> >> >> These is a proper BAR (64 bit pref) in the bridge's Type 1 header, but:
> > > >> >> >> 1. Assigning a memory resource from the pci memory window to this
> > > >> >> >>    BAR causes the PCIe system to fail (this is a bug). So we cannot
> > > >> >> >>    expose this BAR to standard Linux code without even more quirks
> > > >> >> >>    and hacks.
> > > >> >> >
> > > >> >> > Are you saying assigning space for this BAR exposes a Linux PCI bug?
> > > >> >> > If so, I'd like to know more about that because we should fix it.
> > > >> >> >
> > > >> >> > Or does it expose a hardware bug in the Broadcom device?
> > > >> >>
> > > >> >> This is a hardware bug (or non-compliance). The BAR cannot be
> > > >> >> assigned from the PCI MEM range. To use it we need to program
> > > >> >> an address outside the areas assigned to PCI to the BAR. But like
> > > >> >> I said it is better for us to ignore the BAR in linux and then the
> > > >> >> device acts as a normal bridge.
> > > >> >
> > > >> > Your PCI host bridge has a window of address space that it forwards
> > > >> > from the primary (CPU) side to the secondary (PCI) side.  I assume
> > > >> > that's what you mean by the "PCI MEM range."  Apparently if the BAR is
> > > >> > programmed inside that window, it causes some hardware error.  That
> > > >> > still seems strange; there's no driver for the device, and we know the
> > > >> > range is in use so it won't be assigned to any other device, so Linux
> > > >> > should never *access* the range.
> > > >>
> > > >> This is correct. The write to this bridge BAR with a address from
> > > >> the host bridge window will cause a hardware issue.
> > > >>
> > > >> > Apparently if you program the BAR *outside* the window, the hardware
> > > >> > error does not happen, and the private registers are accessible.  But
> > > >> > Linux currently doesn't know where this space is.
> > > >> >
> > > >> > If we ignore the BAR in Linux, apparently the hardware error does not
> > > >> > happen.  But the BAR still contains some value, so this is really the
> > > >> > same as having the BAR outside the window, presumably because it came
> > > >> > out of reset that way, or maybe firmware programmed it.
> > > >>
> > > >> Yes.
> > > >>
> > > >> > It sounds to me like you should do the following:
> > > >> >
> > > >> > a) Have firmware program this BAR where you want it,
> > > >> >
> > > >> > b) Describe these private registers as internal PCI bridge registers,
> > > >> >    using a DT "reg" property,
> > > >>
> > > >> Two ponts here:
> > > >> - We have to support ACPI as well
> > > >
> > > > ACPI can do something similar.  This register space would be described
> > > > in a _CRS method.  IIRC there is actually a bit in the _CRS descriptor
> > > > that was intended to tell you whether the resource is (a) consumed
> > > > directly by the device or (b) forwarded to downstream devices.  But I
> > > > think BIOSes didn't use that bit correctly, so it's useless, so the
> > > > _CRS method might have to be on a different device.
> > > >
> > > >> - There is no need to use the private registers in linux, so the
> > > >>    whole thing will be pointless.
> > > >>
> > > >> > c) Describe the host bridge window (which doesn't include the above
> > > >> >    registers) using the normal "ranges" property, and
> > > >>
> > > >> This is standard code (and ACPI works as well)..
> > > >>
> > > >> > d) Arrange for config writes to BAR 0 to be dropped and for reads to
> > > >> >    return 0, maybe by checks in your config accessors.
> > > >>
> > > >> Here we are hiding the BAR from linux using checks in config read
> > > >> write (if i understand correctly). This will need custom config accessors
> > > >> for Vulcan both on device tree as well as in ACPI.
> > > >
> > > > Config accessors are usually not specific to DT or ACPI.
> > > >
> > > >> The patch (above) is trying to do it in a much much simpler way by
> > > >> clearing the bar0 resource in DECLARE_PCI_FIXUP_HEADER quirk.
> > > >> I am not able to see why this is not valid.
> > > >
> > > > I think there are two problems:
> > > >
> > > > 1) If the device responds to any address space, Linux needs to know
> > > > about it.  If we don't know about it, we might assign that space to
> > > > something else.
> > >
> > > If it is really needed I can mark the address space as reserved. And
> > > Linux generally should not use physical address space outside the
> > > areas specified by firmware of device tree.
> >
> > That's true, at least in theory.  Historically on x86, we only avoided
> > areas marked as reserved.  We *should* have only used areas marked as
> > available for MMIO, but Linux didn't pay much attention to the _CRS
> > methods that tell us that.  We just assumed anything that wasn't RAM
> > and wasn't marked as reserved in E820 was fair game for use by PCI.
> >
> > > > 2) The PCI core still reads and writes the BAR to size it, and we
> > > > don't know whether this will trigger the hardware issue.
> > >
> > > The sizing read/write is done after disabling the BAR by writing
> > > to CMD register - I don't see why you think this will be a problem.
> >
> > You're right; I forgot about that CMD enable bit.  There are a few
> > cases where we don't disable decoding (see mmio_always_on), the most
> > important being for PCI_CLASS_BRIDGE_HOST devices.  But I think your
> > device is a PCI_CLASS_BRIDGE_PCI device, not a BRIDGE_HOST device, so
> > we should disable decoding on your device.
> >
> > > > *You* might know that neither of these is an issue today, but special
> > > > incidental knowledge like that is a maintenance problem because I
> > > > can't tell what PCI core changes might make them an issue in the
> > > > future.
> > >
> > > The worst case maintenance problem is that a future PCI change
> > > might break our chip. This is not unexpected in Linux, and such
> > > breakages can be easily fixed.
> >
> > It's certainly unexpected by *me* :)  And the burden of fixing such
> > breakages doesn't scale well -- it usually involves triage by somebody
> > other than the chip folks, e.g., me.
> >
> > Andi is doing something similar for some Broadwell-EP devices that
> > have BARs that aren't really BARs.  I suggested custom config
> > accessors, which is fairly clean from the core point of view, but it's
> > definitely clunky on the device side.
> >
> > Andi's first proposals were to use a quirk to set a flag bit that told
> > us not to touch the BAR.  I'm starting to think that's a cleaner
> > approach.  I think it would be better to put the bit(s) in the
> > pci_dev, so generic struct resource users wouldn't see them, set them
> > via early quirks, then have __pci_read_base() check them and just
> > leave the struct resource zeroed out.
> 
> I spent some more time working with our hardware team on the nature
> of this issue. It looks like this is not a problem if we setup the PCI
> configuration in firmware better (this has to do with the sequence for
> configuring the PCI controller in firmware).
> 
> So I will drop the part in my patch that hides the BARs from linux, since
> the standard Linux resource allocation code will work Sorry for taking up
> your time on this.

No problem, this is the best possible outcome: the hardware isn't
broken, Linux isn't broken (at least in this tiny regard), we avoided
putting an unnecessary workaround into Linux, and we retained the
ability to use those debug registers if that's ever necessary.

Thanks a lot for chasing this down!

Bjorn

Re: [RFC PATCH 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS

[Bjorn Helgaas]

[+cc Alex, iommu list]

On Mon, Feb 15, 2016 at 03:35:00AM +0530, Jayachandran C wrote:
> Add a new flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS to indicate bridges
> that should not be considered during DMA alias search. This is
> to support hardware (in this case Broadcom Vulcan PCIe subsystem)
> that has internal bridges which have either missing or wrong PCIe
> capabilities.

This needs more explanation, like what exactly is wrong with this
device?  A missing PCIe capability might cause other problems.

What problem does this fix?  Without these patches, do we merely add
aliases that are unnecessary?  Do we crash because something goes
wrong in the pci_pcie_type() switch because of the incorrect
capability?

> Update the function pci_for_each_dma_alias() to skip bridges with
> this flag set.
> 
> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> ---
> 
> This patch is an RFC, if there is a better way to do this, please
> let me know.
> 
> Thanks,
> JC.
> 
>  drivers/pci/search.c | 2 ++
>  include/linux/pci.h  | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index a20ce7d..e5296aa 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -55,6 +55,8 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
>  			continue;
>  
>  		tmp = bus->self;
> +		if (tmp->dev_flags & PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS)
> +			continue;
>  
>  		/*
>  		 * PCIe-to-PCI/X bridges alias transactions from downstream
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 27df4a6..b4d8215 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -182,6 +182,8 @@ enum pci_dev_flags {
>  	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
>  	/* Get VPD from function 0 VPD */
>  	PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
> +	/* Bridge should be ignored for alias search  */
> +	PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS = (__force pci_dev_flags_t) (1 << 9),
>  };
>  
>  enum pci_irq_reroute_variant {
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS

[Alex Williamson]

On Mon, 15 Feb 2016 12:20:23 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:

> [+cc Alex, iommu list]
> 
> On Mon, Feb 15, 2016 at 03:35:00AM +0530, Jayachandran C wrote:
> > Add a new flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS to indicate bridges
> > that should not be considered during DMA alias search. This is
> > to support hardware (in this case Broadcom Vulcan PCIe subsystem)
> > that has internal bridges which have either missing or wrong PCIe
> > capabilities.  

I figured this would come at some point, the right answer is of course
to follow the PCIe spec and implement the required PCIe capability in
the hardware.

> 
> This needs more explanation, like what exactly is wrong with this
> device?  A missing PCIe capability might cause other problems.
> 
> What problem does this fix?  Without these patches, do we merely add
> aliases that are unnecessary?  Do we crash because something goes
> wrong in the pci_pcie_type() switch because of the incorrect
> capability?

The change takes the same code path as it would for a real PCIe bridge
port (downstream/upstream/root), which means they want to skip adding
this bridge as an alias of the device.  So we're adding in aliases that
don't exist, the bridge itself.

If anything I'd suggest a flag that actually tries to address the
problem rather than a symptom of the problem.  For example, maybe the
flag should be PCI_DEV_FLAGS_IS_PCIE.  Maybe pci_is_pcie() should even
take that into account.  That has some trickle through for
pci_pcie_type() and all the accessor functions, but maybe it's a
cleaner solution overall (or maybe it explodes further).  Thanks,

Alex

> > Update the function pci_for_each_dma_alias() to skip bridges with
> > this flag set.
> > 
> > Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> > ---
> > 
> > This patch is an RFC, if there is a better way to do this, please
> > let me know.
> > 
> > Thanks,
> > JC.
> > 
> >  drivers/pci/search.c | 2 ++
> >  include/linux/pci.h  | 2 ++
> >  2 files changed, 4 insertions(+)
> > 
> > diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> > index a20ce7d..e5296aa 100644
> > --- a/drivers/pci/search.c
> > +++ b/drivers/pci/search.c
> > @@ -55,6 +55,8 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
> >  			continue;
> >  
> >  		tmp = bus->self;
> > +		if (tmp->dev_flags & PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS)
> > +			continue;
> >  
> >  		/*
> >  		 * PCIe-to-PCI/X bridges alias transactions from downstream
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 27df4a6..b4d8215 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -182,6 +182,8 @@ enum pci_dev_flags {
> >  	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
> >  	/* Get VPD from function 0 VPD */
> >  	PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
> > +	/* Bridge should be ignored for alias search  */
> > +	PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS = (__force pci_dev_flags_t) (1 << 9),
> >  };
> >  
> >  enum pci_irq_reroute_variant {
> > -- 
> > 1.9.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  

Re: [RFC PATCH 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS

[Jayachandran Chandrashekaran Nair]

On Tue, Feb 16, 2016 at 1:09 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Mon, 15 Feb 2016 12:20:23 -0600
> Bjorn Helgaas <helgaas@kernel.org> wrote:
>
>> [+cc Alex, iommu list]
>>
>> On Mon, Feb 15, 2016 at 03:35:00AM +0530, Jayachandran C wrote:
>> > Add a new flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS to indicate bridges
>> > that should not be considered during DMA alias search. This is
>> > to support hardware (in this case Broadcom Vulcan PCIe subsystem)
>> > that has internal bridges which have either missing or wrong PCIe
>> > capabilities.
>
> I figured this would come at some point, the right answer is of course
> to follow the PCIe spec and implement the required PCIe capability in
> the hardware.

There are  PCIe controllers on the chip that follows the spec, the issue is
how it is integrated to the northbridge (equivalent) on the chip, I have
tried to explain it below.

>> This needs more explanation, like what exactly is wrong with this
>> device?  A missing PCIe capability might cause other problems.
>>
>> What problem does this fix?  Without these patches, do we merely add
>> aliases that are unnecessary?  Do we crash because something goes
>> wrong in the pci_pcie_type() switch because of the incorrect
>> capability?

Here's how (for example) how the PCI enumeration of a 2 node Vulcan
processor will look like:


[0] +-0.0.0--[1]---+--1.a.0----[2]-----2.0.0---[3]----3.0.0
    |              +--1.a.1----[4]-----4.0.0---[5]----5.0.0
    |              .
    |              ... etc...
    |
    +-0.0.1--[10]--+-10.a.0----[11]---11.0.0---[12]---12.0.0
                   +-10.a.1----[13]---13.0.0---[14]---14.0.0
                   .
                   ... etc...

The devices 0.0.x and x.a.x are glue bridges that are there to
bring the real PCIe controllers (pcie cap type 4) 2.0.0, 4.0.0,
11.0.0, 13.0.0 under a single PCI hierarchy. The x.a.x bridges
have a PCIe capability (type 8) and 0.0.x does not have any pcie
capability.

In case of Vulcan, both the GICv3 ITS driver code and the SMMUv3
driver code does dma alias walk to find the device id to use
in ITS and SMMU. In both cases it will ignore the x.0.0 bridges
since they are type root port, but will continue on up and end
up with incorrect device id.

The flag I have added is to make the pci_for_each_dma_alias()
ignore the last 2 levels of glue/internal bridges.

> The change takes the same code path as it would for a real PCIe bridge
> port (downstream/upstream/root), which means they want to skip adding
> this bridge as an alias of the device.  So we're adding in aliases that
> don't exist, the bridge itself.
>
> If anything I'd suggest a flag that actually tries to address the
> problem rather than a symptom of the problem.  For example, maybe the
> flag should be PCI_DEV_FLAGS_IS_PCIE.  Maybe pci_is_pcie() should even
> take that into account.  That has some trickle through for
> pci_pcie_type() and all the accessor functions, but maybe it's a
> cleaner solution overall (or maybe it explodes further).  Thanks,

I didn't really want to mark the glue bridges as PCIe or have fake
PCIe capability there, the obvious simple solution was to add
the flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS

Any suggestions/comments on how to do this better would be welcome.

Thanks,
JC.
[Using gmail due to IT transition, hope the ascii art makes it thru]

Re: [RFC PATCH 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS

[Alex Williamson]

On Wed, 17 Feb 2016 02:38:24 +0530
Jayachandran Chandrashekaran Nair
<jayachandran.chandrashekaran@broadcom.com> wrote:

> On Tue, Feb 16, 2016 at 1:09 AM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Mon, 15 Feb 2016 12:20:23 -0600
> > Bjorn Helgaas <helgaas@kernel.org> wrote:
> >  
> >> [+cc Alex, iommu list]
> >>
> >> On Mon, Feb 15, 2016 at 03:35:00AM +0530, Jayachandran C wrote:  
> >> > Add a new flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS to indicate bridges
> >> > that should not be considered during DMA alias search. This is
> >> > to support hardware (in this case Broadcom Vulcan PCIe subsystem)
> >> > that has internal bridges which have either missing or wrong PCIe
> >> > capabilities.  
> >
> > I figured this would come at some point, the right answer is of course
> > to follow the PCIe spec and implement the required PCIe capability in
> > the hardware.  
> 
> There are  PCIe controllers on the chip that follows the spec, the issue is
> how it is integrated to the northbridge (equivalent) on the chip, I have
> tried to explain it below.
> 
> >> This needs more explanation, like what exactly is wrong with this
> >> device?  A missing PCIe capability might cause other problems.
> >>
> >> What problem does this fix?  Without these patches, do we merely add
> >> aliases that are unnecessary?  Do we crash because something goes
> >> wrong in the pci_pcie_type() switch because of the incorrect
> >> capability?  
> 
> Here's how (for example) how the PCI enumeration of a 2 node Vulcan
> processor will look like:
> 
> 
> [0] +-0.0.0--[1]---+--1.a.0----[2]-----2.0.0---[3]----3.0.0
>     |              +--1.a.1----[4]-----4.0.0---[5]----5.0.0
>     |              .
>     |              ... etc...
>     |
>     +-0.0.1--[10]--+-10.a.0----[11]---11.0.0---[12]---12.0.0
>                    +-10.a.1----[13]---13.0.0---[14]---14.0.0
>                    .
>                    ... etc...

So we have:

    "Glue Bridge"---"Glue Bridges"---Root Ports---Endpoints
      (no pcie)      (pci/x-pcie)

> 
> The devices 0.0.x and x.a.x are glue bridges that are there to
> bring the real PCIe controllers (pcie cap type 4) 2.0.0, 4.0.0,
> 11.0.0, 13.0.0 under a single PCI hierarchy. The x.a.x bridges
> have a PCIe capability (type 8) and 0.0.x does not have any pcie
> capability.
> 
> In case of Vulcan, both the GICv3 ITS driver code and the SMMUv3
> driver code does dma alias walk to find the device id to use
> in ITS and SMMU. In both cases it will ignore the x.0.0 bridges
> since they are type root port, but will continue on up and end
> up with incorrect device id.
> 
> The flag I have added is to make the pci_for_each_dma_alias()
> ignore the last 2 levels of glue/internal bridges.

Per the PCIe spec, I'm not even sure what you have is a valid
hierarchy, root ports sit on root complexes, not behind a PCI-to-PCIe
bridge.  So really you're pretending that the downstream "glue bridge"
is your host bridge and therefore root complex, but the PCI topology
would clearly dictate that the top-most bus is conventional PCI and
therefore everything is an alias of everything else and the DMA alias
code is doing exactly what it should.

Also note that the caller of pci_for_each_dma_alias() owns the callback
function triggered for each alias, that function could choose to prune
out these "glue bridges" itself if that's the appropriate thing to do.
Where do the SMMU and ITS actually reside in the above diagram?  You
can use the callout function to stop the traversal anywhere you wish,
it's free to return a -errno to stop or positive number, which the
caller can interpret as error or non-failure stop condition.

You could even think about changing what Linux considers to be the host
bridge.  Maybe these glue bridges shouldn't even be visible to Linux,
would that also solve the issue with the broken BAR?

> > The change takes the same code path as it would for a real PCIe bridge
> > port (downstream/upstream/root), which means they want to skip adding
> > this bridge as an alias of the device.  So we're adding in aliases that
> > don't exist, the bridge itself.
> >
> > If anything I'd suggest a flag that actually tries to address the
> > problem rather than a symptom of the problem.  For example, maybe the
> > flag should be PCI_DEV_FLAGS_IS_PCIE.  Maybe pci_is_pcie() should even
> > take that into account.  That has some trickle through for
> > pci_pcie_type() and all the accessor functions, but maybe it's a
> > cleaner solution overall (or maybe it explodes further).  Thanks,  
> 
> I didn't really want to mark the glue bridges as PCIe or have fake
> PCIe capability there, the obvious simple solution was to add
> the flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS
> 
> Any suggestions/comments on how to do this better would be welcome.

I definitely don't think either flag idea is the right solution, I
think you've actually already got the tools you need to solve it by
putting the intelligence in the callback function or by going further
down the path of pretending the glue bridge is really a host bridge.
Thanks,

Alex

Re: [RFC PATCH 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS

[Jayachandran Chandrashekaran Nair]

On Wed, Feb 17, 2016 at 3:55 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Wed, 17 Feb 2016 02:38:24 +0530
> Jayachandran Chandrashekaran Nair
> <jayachandran.chandrashekaran@broadcom.com> wrote:
>
>> On Tue, Feb 16, 2016 at 1:09 AM, Alex Williamson
>> <alex.williamson@redhat.com> wrote:
>> > On Mon, 15 Feb 2016 12:20:23 -0600
>> > Bjorn Helgaas <helgaas@kernel.org> wrote:
>> >
>> >> [+cc Alex, iommu list]
>> >>
>> >> On Mon, Feb 15, 2016 at 03:35:00AM +0530, Jayachandran C wrote:
>> >> > Add a new flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS to indicate bridges
>> >> > that should not be considered during DMA alias search. This is
>> >> > to support hardware (in this case Broadcom Vulcan PCIe subsystem)
>> >> > that has internal bridges which have either missing or wrong PCIe
>> >> > capabilities.
>> >
>> > I figured this would come at some point, the right answer is of course
>> > to follow the PCIe spec and implement the required PCIe capability in
>> > the hardware.
>>
>> There are  PCIe controllers on the chip that follows the spec, the issue is
>> how it is integrated to the northbridge (equivalent) on the chip, I have
>> tried to explain it below.
>>
>> >> This needs more explanation, like what exactly is wrong with this
>> >> device?  A missing PCIe capability might cause other problems.
>> >>
>> >> What problem does this fix?  Without these patches, do we merely add
>> >> aliases that are unnecessary?  Do we crash because something goes
>> >> wrong in the pci_pcie_type() switch because of the incorrect
>> >> capability?
>>
>> Here's how (for example) how the PCI enumeration of a 2 node Vulcan
>> processor will look like:
>>
>>
>> [0] +-0.0.0--[1]---+--1.a.0----[2]-----2.0.0---[3]----3.0.0
>>     |              +--1.a.1----[4]-----4.0.0---[5]----5.0.0
>>     |              .
>>     |              ... etc...
>>     |
>>     +-0.0.1--[10]--+-10.a.0----[11]---11.0.0---[12]---12.0.0
>>                    +-10.a.1----[13]---13.0.0---[14]---14.0.0
>>                    .
>>                    ... etc...
>
> So we have:
>
>     "Glue Bridge"---"Glue Bridges"---Root Ports---Endpoints
>       (no pcie)      (pci/x-pcie)

The top level is one glue bridge per chip in a multi-chip board,
but otherwise this is accurate.

>>
>> The devices 0.0.x and x.a.x are glue bridges that are there to
>> bring the real PCIe controllers (pcie cap type 4) 2.0.0, 4.0.0,
>> 11.0.0, 13.0.0 under a single PCI hierarchy. The x.a.x bridges
>> have a PCIe capability (type 8) and 0.0.x does not have any pcie
>> capability.
>>
>> In case of Vulcan, both the GICv3 ITS driver code and the SMMUv3
>> driver code does dma alias walk to find the device id to use
>> in ITS and SMMU. In both cases it will ignore the x.0.0 bridges
>> since they are type root port, but will continue on up and end
>> up with incorrect device id.
>>
>> The flag I have added is to make the pci_for_each_dma_alias()
>> ignore the last 2 levels of glue/internal bridges.
>
> Per the PCIe spec, I'm not even sure what you have is a valid
> hierarchy, root ports sit on root complexes, not behind a PCI-to-PCIe
> bridge.  So really you're pretending that the downstream "glue bridge"
> is your host bridge and therefore root complex, but the PCI topology
> would clearly dictate that the top-most bus is conventional PCI and
> therefore everything is an alias of everything else and the DMA alias
> code is doing exactly what it should.

Yes, I am not arguing that there is any issue in the current code. I
am trying to figure out the correct way to implement the quirk. We
have to support the hardware we have, not the hardware we want to
have :)

> Also note that the caller of pci_for_each_dma_alias() owns the callback
> function triggered for each alias, that function could choose to prune
> out these "glue bridges" itself if that's the appropriate thing to do.

I had implemented this first, but moved to the current approach because
it is needed in multiple places. The problem is: "On vulcan, while
going up the pcie bus heirarchy for finding aliases, skip the glue bridges",
So the logical place to put the fix is in pci_for_each_dma_alias()

> Where do the SMMU and ITS actually reside in the above diagram?  You
> can use the callout function to stop the traversal anywhere you wish,
> it's free to return a -errno to stop or positive number, which the
> caller can interpret as error or non-failure stop condition.

The SMMU (for translation requests) and ITS (for MSI writes for
interrupts) are connected directly to the proper PCIe controller
(2/4/11/13.0.0 above)

> You could even think about changing what Linux considers to be the host
> bridge.  Maybe these glue bridges shouldn't even be visible to Linux,
> would that also solve the issue with the broken BAR?

The glue bridges have to seen by Linux for assigning resources like
bus ranges and memory windows. So these are required bridges in
the topology and will work with the standard linux code
(provided we skip them for aliases, and and ignore the BAR).

>> > The change takes the same code path as it would for a real PCIe bridge
>> > port (downstream/upstream/root), which means they want to skip adding
>> > this bridge as an alias of the device.  So we're adding in aliases that
>> > don't exist, the bridge itself.
>> >
>> > If anything I'd suggest a flag that actually tries to address the
>> > problem rather than a symptom of the problem.  For example, maybe the
>> > flag should be PCI_DEV_FLAGS_IS_PCIE.  Maybe pci_is_pcie() should even
>> > take that into account.  That has some trickle through for
>> > pci_pcie_type() and all the accessor functions, but maybe it's a
>> > cleaner solution overall (or maybe it explodes further).  Thanks,
>>
>> I didn't really want to mark the glue bridges as PCIe or have fake
>> PCIe capability there, the obvious simple solution was to add
>> the flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS
>>
>> Any suggestions/comments on how to do this better would be welcome.
>
> I definitely don't think either flag idea is the right solution, I
> think you've actually already got the tools you need to solve it by
> putting the intelligence in the callback function or by going further
> down the path of pretending the glue bridge is really a host bridge.

If I understand your suggestion, I need to fake the PCIe capability
for the glue bridges, which may be reasonable for the second level.
But for the first level, there is no pcie capability and faking that
becomes more difficult.

If the concern is adding a flag for just one platform, I can check
for the vendor ID/device ID in the pci_for_each_dma_alias()
function, which would be slightly uglier, but still a simple and
maintainable solution.

Another option is to break the pci_for_each_dma_alias when it
reaches to root port (for vulcan), this too can be done without
a flag.

Thanks,
JC.

Re: [RFC PATCH 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS

[Alex Williamson]

On Wed, 17 Feb 2016 17:15:09 +0530
Jayachandran Chandrashekaran Nair
<jayachandran.chandrashekaran@broadcom.com> wrote:

> On Wed, Feb 17, 2016 at 3:55 AM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Wed, 17 Feb 2016 02:38:24 +0530
> > Jayachandran Chandrashekaran Nair
> > <jayachandran.chandrashekaran@broadcom.com> wrote:
> >  
> >> On Tue, Feb 16, 2016 at 1:09 AM, Alex Williamson
> >> <alex.williamson@redhat.com> wrote:  
> >> > On Mon, 15 Feb 2016 12:20:23 -0600
> >> > Bjorn Helgaas <helgaas@kernel.org> wrote:
> >> >  
> >> >> [+cc Alex, iommu list]
> >> >>
> >> >> On Mon, Feb 15, 2016 at 03:35:00AM +0530, Jayachandran C wrote:  
> >> >> > Add a new flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS to indicate bridges
> >> >> > that should not be considered during DMA alias search. This is
> >> >> > to support hardware (in this case Broadcom Vulcan PCIe subsystem)
> >> >> > that has internal bridges which have either missing or wrong PCIe
> >> >> > capabilities.  
> >> >
> >> > I figured this would come at some point, the right answer is of course
> >> > to follow the PCIe spec and implement the required PCIe capability in
> >> > the hardware.  
> >>
> >> There are  PCIe controllers on the chip that follows the spec, the issue is
> >> how it is integrated to the northbridge (equivalent) on the chip, I have
> >> tried to explain it below.
> >>  
> >> >> This needs more explanation, like what exactly is wrong with this
> >> >> device?  A missing PCIe capability might cause other problems.
> >> >>
> >> >> What problem does this fix?  Without these patches, do we merely add
> >> >> aliases that are unnecessary?  Do we crash because something goes
> >> >> wrong in the pci_pcie_type() switch because of the incorrect
> >> >> capability?  
> >>
> >> Here's how (for example) how the PCI enumeration of a 2 node Vulcan
> >> processor will look like:
> >>
> >>
> >> [0] +-0.0.0--[1]---+--1.a.0----[2]-----2.0.0---[3]----3.0.0
> >>     |              +--1.a.1----[4]-----4.0.0---[5]----5.0.0
> >>     |              .
> >>     |              ... etc...
> >>     |
> >>     +-0.0.1--[10]--+-10.a.0----[11]---11.0.0---[12]---12.0.0
> >>                    +-10.a.1----[13]---13.0.0---[14]---14.0.0
> >>                    .
> >>                    ... etc...  
> >
> > So we have:
> >
> >     "Glue Bridge"---"Glue Bridges"---Root Ports---Endpoints
> >       (no pcie)      (pci/x-pcie)  
> 
> The top level is one glue bridge per chip in a multi-chip board,
> but otherwise this is accurate.
> 
> >>
> >> The devices 0.0.x and x.a.x are glue bridges that are there to
> >> bring the real PCIe controllers (pcie cap type 4) 2.0.0, 4.0.0,
> >> 11.0.0, 13.0.0 under a single PCI hierarchy. The x.a.x bridges
> >> have a PCIe capability (type 8) and 0.0.x does not have any pcie
> >> capability.
> >>
> >> In case of Vulcan, both the GICv3 ITS driver code and the SMMUv3
> >> driver code does dma alias walk to find the device id to use
> >> in ITS and SMMU. In both cases it will ignore the x.0.0 bridges
> >> since they are type root port, but will continue on up and end
> >> up with incorrect device id.
> >>
> >> The flag I have added is to make the pci_for_each_dma_alias()
> >> ignore the last 2 levels of glue/internal bridges.  
> >
> > Per the PCIe spec, I'm not even sure what you have is a valid
> > hierarchy, root ports sit on root complexes, not behind a PCI-to-PCIe
> > bridge.  So really you're pretending that the downstream "glue bridge"
> > is your host bridge and therefore root complex, but the PCI topology
> > would clearly dictate that the top-most bus is conventional PCI and
> > therefore everything is an alias of everything else and the DMA alias
> > code is doing exactly what it should.  
> 
> Yes, I am not arguing that there is any issue in the current code. I
> am trying to figure out the correct way to implement the quirk. We
> have to support the hardware we have, not the hardware we want to
> have :)
> 
> > Also note that the caller of pci_for_each_dma_alias() owns the callback
> > function triggered for each alias, that function could choose to prune
> > out these "glue bridges" itself if that's the appropriate thing to do.  
> 
> I had implemented this first, but moved to the current approach because
> it is needed in multiple places. The problem is: "On vulcan, while
> going up the pcie bus heirarchy for finding aliases, skip the glue bridges",
> So the logical place to put the fix is in pci_for_each_dma_alias()
> 
> > Where do the SMMU and ITS actually reside in the above diagram?  You
> > can use the callout function to stop the traversal anywhere you wish,
> > it's free to return a -errno to stop or positive number, which the
> > caller can interpret as error or non-failure stop condition.  
> 
> The SMMU (for translation requests) and ITS (for MSI writes for
> interrupts) are connected directly to the proper PCIe controller
> (2/4/11/13.0.0 above)


If the translation unit is on the root port then DMA aliases upstream of
the translation unit are irrelevant and the callback function should
stop the traversal at that point.

> > You could even think about changing what Linux considers to be the host
> > bridge.  Maybe these glue bridges shouldn't even be visible to Linux,
> > would that also solve the issue with the broken BAR?  
> 
> The glue bridges have to seen by Linux for assigning resources like
> bus ranges and memory windows. So these are required bridges in
> the topology and will work with the standard linux code
> (provided we skip them for aliases, and and ignore the BAR).
> 
> >> > The change takes the same code path as it would for a real PCIe bridge
> >> > port (downstream/upstream/root), which means they want to skip adding
> >> > this bridge as an alias of the device.  So we're adding in aliases that
> >> > don't exist, the bridge itself.
> >> >
> >> > If anything I'd suggest a flag that actually tries to address the
> >> > problem rather than a symptom of the problem.  For example, maybe the
> >> > flag should be PCI_DEV_FLAGS_IS_PCIE.  Maybe pci_is_pcie() should even
> >> > take that into account.  That has some trickle through for
> >> > pci_pcie_type() and all the accessor functions, but maybe it's a
> >> > cleaner solution overall (or maybe it explodes further).  Thanks,  
> >>
> >> I didn't really want to mark the glue bridges as PCIe or have fake
> >> PCIe capability there, the obvious simple solution was to add
> >> the flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS
> >>
> >> Any suggestions/comments on how to do this better would be welcome.  
> >
> > I definitely don't think either flag idea is the right solution, I
> > think you've actually already got the tools you need to solve it by
> > putting the intelligence in the callback function or by going further
> > down the path of pretending the glue bridge is really a host bridge.  
> 
> If I understand your suggestion, I need to fake the PCIe capability
> for the glue bridges, which may be reasonable for the second level.
> But for the first level, there is no pcie capability and faking that
> becomes more difficult.

That's not what I'm suggesting.
 
> If the concern is adding a flag for just one platform, I can check
> for the vendor ID/device ID in the pci_for_each_dma_alias()
> function, which would be slightly uglier, but still a simple and
> maintainable solution.
> 
> Another option is to break the pci_for_each_dma_alias when it
> reaches to root port (for vulcan), this too can be done without
> a flag.

Exactly, this can be done by the callback function provided on vulcan
systems and requires no core changes.  The IOMMU needs to provide a
callback function that makes sense for collecting aliases on the
specific platform.  If the translation unit is not on the root bus, the
callback function should stop the topology walk earlier.  Thanks,

Alex

Re: [RFC PATCH 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS

[Jayachandran Chandrashekaran Nair]

On Wed, Feb 17, 2016 at 8:58 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Wed, 17 Feb 2016 17:15:09 +0530
> Jayachandran Chandrashekaran Nair
> <jayachandran.chandrashekaran@broadcom.com> wrote:
>
>> On Wed, Feb 17, 2016 at 3:55 AM, Alex Williamson
>> <alex.williamson@redhat.com> wrote:
>> > On Wed, 17 Feb 2016 02:38:24 +0530
>> > Jayachandran Chandrashekaran Nair
>> > <jayachandran.chandrashekaran@broadcom.com> wrote:
>> >
>> >> On Tue, Feb 16, 2016 at 1:09 AM, Alex Williamson
>> >> <alex.williamson@redhat.com> wrote:
>> >> > On Mon, 15 Feb 2016 12:20:23 -0600
>> >> > Bjorn Helgaas <helgaas@kernel.org> wrote:
>> >> >
>> >> >> [+cc Alex, iommu list]
>> >> >>
>> >> >> On Mon, Feb 15, 2016 at 03:35:00AM +0530, Jayachandran C wrote:
>> >> >> > Add a new flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS to indicate bridges
>> >> >> > that should not be considered during DMA alias search. This is
>> >> >> > to support hardware (in this case Broadcom Vulcan PCIe subsystem)
>> >> >> > that has internal bridges which have either missing or wrong PCIe
>> >> >> > capabilities.
>> >> >
>> >> > I figured this would come at some point, the right answer is of course
>> >> > to follow the PCIe spec and implement the required PCIe capability in
>> >> > the hardware.
>> >>
>> >> There are  PCIe controllers on the chip that follows the spec, the issue is
>> >> how it is integrated to the northbridge (equivalent) on the chip, I have
>> >> tried to explain it below.
>> >>
>> >> >> This needs more explanation, like what exactly is wrong with this
>> >> >> device?  A missing PCIe capability might cause other problems.
>> >> >>
>> >> >> What problem does this fix?  Without these patches, do we merely add
>> >> >> aliases that are unnecessary?  Do we crash because something goes
>> >> >> wrong in the pci_pcie_type() switch because of the incorrect
>> >> >> capability?
>> >>
>> >> Here's how (for example) how the PCI enumeration of a 2 node Vulcan
>> >> processor will look like:
>> >>
>> >>
>> >> [0] +-0.0.0--[1]---+--1.a.0----[2]-----2.0.0---[3]----3.0.0
>> >>     |              +--1.a.1----[4]-----4.0.0---[5]----5.0.0
>> >>     |              .
>> >>     |              ... etc...
>> >>     |
>> >>     +-0.0.1--[10]--+-10.a.0----[11]---11.0.0---[12]---12.0.0
>> >>                    +-10.a.1----[13]---13.0.0---[14]---14.0.0
>> >>                    .
>> >>                    ... etc...
>> >
>> > So we have:
>> >
>> >     "Glue Bridge"---"Glue Bridges"---Root Ports---Endpoints
>> >       (no pcie)      (pci/x-pcie)
>>
>> The top level is one glue bridge per chip in a multi-chip board,
>> but otherwise this is accurate.
>>
>> >>
>> >> The devices 0.0.x and x.a.x are glue bridges that are there to
>> >> bring the real PCIe controllers (pcie cap type 4) 2.0.0, 4.0.0,
>> >> 11.0.0, 13.0.0 under a single PCI hierarchy. The x.a.x bridges
>> >> have a PCIe capability (type 8) and 0.0.x does not have any pcie
>> >> capability.
>> >>
>> >> In case of Vulcan, both the GICv3 ITS driver code and the SMMUv3
>> >> driver code does dma alias walk to find the device id to use
>> >> in ITS and SMMU. In both cases it will ignore the x.0.0 bridges
>> >> since they are type root port, but will continue on up and end
>> >> up with incorrect device id.
>> >>
>> >> The flag I have added is to make the pci_for_each_dma_alias()
>> >> ignore the last 2 levels of glue/internal bridges.
>> >
>> > Per the PCIe spec, I'm not even sure what you have is a valid
>> > hierarchy, root ports sit on root complexes, not behind a PCI-to-PCIe
>> > bridge.  So really you're pretending that the downstream "glue bridge"
>> > is your host bridge and therefore root complex, but the PCI topology
>> > would clearly dictate that the top-most bus is conventional PCI and
>> > therefore everything is an alias of everything else and the DMA alias
>> > code is doing exactly what it should.
>>
>> Yes, I am not arguing that there is any issue in the current code. I
>> am trying to figure out the correct way to implement the quirk. We
>> have to support the hardware we have, not the hardware we want to
>> have :)
>>
>> > Also note that the caller of pci_for_each_dma_alias() owns the callback
>> > function triggered for each alias, that function could choose to prune
>> > out these "glue bridges" itself if that's the appropriate thing to do.
>>
>> I had implemented this first, but moved to the current approach because
>> it is needed in multiple places. The problem is: "On vulcan, while
>> going up the pcie bus heirarchy for finding aliases, skip the glue bridges",
>> So the logical place to put the fix is in pci_for_each_dma_alias()
>>
>> > Where do the SMMU and ITS actually reside in the above diagram?  You
>> > can use the callout function to stop the traversal anywhere you wish,
>> > it's free to return a -errno to stop or positive number, which the
>> > caller can interpret as error or non-failure stop condition.
>>
>> The SMMU (for translation requests) and ITS (for MSI writes for
>> interrupts) are connected directly to the proper PCIe controller
>> (2/4/11/13.0.0 above)
>
>
> If the translation unit is on the root port then DMA aliases upstream of
> the translation unit are irrelevant and the callback function should
> stop the traversal at that point.
>
>> > You could even think about changing what Linux considers to be the host
>> > bridge.  Maybe these glue bridges shouldn't even be visible to Linux,
>> > would that also solve the issue with the broken BAR?
>>
>> The glue bridges have to seen by Linux for assigning resources like
>> bus ranges and memory windows. So these are required bridges in
>> the topology and will work with the standard linux code
>> (provided we skip them for aliases, and and ignore the BAR).
>>
>> >> > The change takes the same code path as it would for a real PCIe bridge
>> >> > port (downstream/upstream/root), which means they want to skip adding
>> >> > this bridge as an alias of the device.  So we're adding in aliases that
>> >> > don't exist, the bridge itself.
>> >> >
>> >> > If anything I'd suggest a flag that actually tries to address the
>> >> > problem rather than a symptom of the problem.  For example, maybe the
>> >> > flag should be PCI_DEV_FLAGS_IS_PCIE.  Maybe pci_is_pcie() should even
>> >> > take that into account.  That has some trickle through for
>> >> > pci_pcie_type() and all the accessor functions, but maybe it's a
>> >> > cleaner solution overall (or maybe it explodes further).  Thanks,
>> >>
>> >> I didn't really want to mark the glue bridges as PCIe or have fake
>> >> PCIe capability there, the obvious simple solution was to add
>> >> the flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS
>> >>
>> >> Any suggestions/comments on how to do this better would be welcome.
>> >
>> > I definitely don't think either flag idea is the right solution, I
>> > think you've actually already got the tools you need to solve it by
>> > putting the intelligence in the callback function or by going further
>> > down the path of pretending the glue bridge is really a host bridge.
>>
>> If I understand your suggestion, I need to fake the PCIe capability
>> for the glue bridges, which may be reasonable for the second level.
>> But for the first level, there is no pcie capability and faking that
>> becomes more difficult.
>
> That's not what I'm suggesting.
>
>> If the concern is adding a flag for just one platform, I can check
>> for the vendor ID/device ID in the pci_for_each_dma_alias()
>> function, which would be slightly uglier, but still a simple and
>> maintainable solution.
>>
>> Another option is to break the pci_for_each_dma_alias when it
>> reaches to root port (for vulcan), this too can be done without
>> a flag.
>
> Exactly, this can be done by the callback function provided on vulcan
> systems and requires no core changes.  The IOMMU needs to provide a
> callback function that makes sense for collecting aliases on the
> specific platform.  If the translation unit is not on the root bus, the
> callback function should stop the topology walk earlier.  Thanks,

Fixing every callback function is going to be painful and ugly.
I will have to change drivers/irqchip/irq-gic-v3-its-pci-msi.c,
drivers/pci/msi.c, and drivers/iommu/arm-smmu-v3.c with the
same check for vendor/device ids and return early.

I can update the patch to check the vendor/device id in the
case of ROOT_PORT in pci_for_each_dma_alias(). If you
think that will not be acceptable, I don't have any reasonable
options left.

Thanks,
JC.

Re: [RFC PATCH 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS

[Alex Williamson]

On Thu, 18 Feb 2016 18:57:12 +0530
Jayachandran Chandrashekaran Nair
<jayachandran.chandrashekaran@broadcom.com> wrote:

> On Wed, Feb 17, 2016 at 8:58 PM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Wed, 17 Feb 2016 17:15:09 +0530
> > Jayachandran Chandrashekaran Nair
> > <jayachandran.chandrashekaran@broadcom.com> wrote:
> >  
> >> On Wed, Feb 17, 2016 at 3:55 AM, Alex Williamson
> >> <alex.williamson@redhat.com> wrote:  
> >> > On Wed, 17 Feb 2016 02:38:24 +0530
> >> > Jayachandran Chandrashekaran Nair
> >> > <jayachandran.chandrashekaran@broadcom.com> wrote:
> >> >  
> >> >> On Tue, Feb 16, 2016 at 1:09 AM, Alex Williamson
> >> >> <alex.williamson@redhat.com> wrote:  
> >> >> > On Mon, 15 Feb 2016 12:20:23 -0600
> >> >> > Bjorn Helgaas <helgaas@kernel.org> wrote:
> >> >> >  
> >> >> >> [+cc Alex, iommu list]
> >> >> >>
> >> >> >> On Mon, Feb 15, 2016 at 03:35:00AM +0530, Jayachandran C wrote:  
> >> >> >> > Add a new flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS to indicate bridges
> >> >> >> > that should not be considered during DMA alias search. This is
> >> >> >> > to support hardware (in this case Broadcom Vulcan PCIe subsystem)
> >> >> >> > that has internal bridges which have either missing or wrong PCIe
> >> >> >> > capabilities.  
> >> >> >
> >> >> > I figured this would come at some point, the right answer is of course
> >> >> > to follow the PCIe spec and implement the required PCIe capability in
> >> >> > the hardware.  
> >> >>
> >> >> There are  PCIe controllers on the chip that follows the spec, the issue is
> >> >> how it is integrated to the northbridge (equivalent) on the chip, I have
> >> >> tried to explain it below.
> >> >>  
> >> >> >> This needs more explanation, like what exactly is wrong with this
> >> >> >> device?  A missing PCIe capability might cause other problems.
> >> >> >>
> >> >> >> What problem does this fix?  Without these patches, do we merely add
> >> >> >> aliases that are unnecessary?  Do we crash because something goes
> >> >> >> wrong in the pci_pcie_type() switch because of the incorrect
> >> >> >> capability?  
> >> >>
> >> >> Here's how (for example) how the PCI enumeration of a 2 node Vulcan
> >> >> processor will look like:
> >> >>
> >> >>
> >> >> [0] +-0.0.0--[1]---+--1.a.0----[2]-----2.0.0---[3]----3.0.0
> >> >>     |              +--1.a.1----[4]-----4.0.0---[5]----5.0.0
> >> >>     |              .
> >> >>     |              ... etc...
> >> >>     |
> >> >>     +-0.0.1--[10]--+-10.a.0----[11]---11.0.0---[12]---12.0.0
> >> >>                    +-10.a.1----[13]---13.0.0---[14]---14.0.0
> >> >>                    .
> >> >>                    ... etc...  
> >> >
> >> > So we have:
> >> >
> >> >     "Glue Bridge"---"Glue Bridges"---Root Ports---Endpoints
> >> >       (no pcie)      (pci/x-pcie)  
> >>
> >> The top level is one glue bridge per chip in a multi-chip board,
> >> but otherwise this is accurate.
> >>  
> >> >>
> >> >> The devices 0.0.x and x.a.x are glue bridges that are there to
> >> >> bring the real PCIe controllers (pcie cap type 4) 2.0.0, 4.0.0,
> >> >> 11.0.0, 13.0.0 under a single PCI hierarchy. The x.a.x bridges
> >> >> have a PCIe capability (type 8) and 0.0.x does not have any pcie
> >> >> capability.
> >> >>
> >> >> In case of Vulcan, both the GICv3 ITS driver code and the SMMUv3
> >> >> driver code does dma alias walk to find the device id to use
> >> >> in ITS and SMMU. In both cases it will ignore the x.0.0 bridges
> >> >> since they are type root port, but will continue on up and end
> >> >> up with incorrect device id.
> >> >>
> >> >> The flag I have added is to make the pci_for_each_dma_alias()
> >> >> ignore the last 2 levels of glue/internal bridges.  
> >> >
> >> > Per the PCIe spec, I'm not even sure what you have is a valid
> >> > hierarchy, root ports sit on root complexes, not behind a PCI-to-PCIe
> >> > bridge.  So really you're pretending that the downstream "glue bridge"
> >> > is your host bridge and therefore root complex, but the PCI topology
> >> > would clearly dictate that the top-most bus is conventional PCI and
> >> > therefore everything is an alias of everything else and the DMA alias
> >> > code is doing exactly what it should.  
> >>
> >> Yes, I am not arguing that there is any issue in the current code. I
> >> am trying to figure out the correct way to implement the quirk. We
> >> have to support the hardware we have, not the hardware we want to
> >> have :)
> >>  
> >> > Also note that the caller of pci_for_each_dma_alias() owns the callback
> >> > function triggered for each alias, that function could choose to prune
> >> > out these "glue bridges" itself if that's the appropriate thing to do.  
> >>
> >> I had implemented this first, but moved to the current approach because
> >> it is needed in multiple places. The problem is: "On vulcan, while
> >> going up the pcie bus heirarchy for finding aliases, skip the glue bridges",
> >> So the logical place to put the fix is in pci_for_each_dma_alias()
> >>  
> >> > Where do the SMMU and ITS actually reside in the above diagram?  You
> >> > can use the callout function to stop the traversal anywhere you wish,
> >> > it's free to return a -errno to stop or positive number, which the
> >> > caller can interpret as error or non-failure stop condition.  
> >>
> >> The SMMU (for translation requests) and ITS (for MSI writes for
> >> interrupts) are connected directly to the proper PCIe controller
> >> (2/4/11/13.0.0 above)  
> >
> >
> > If the translation unit is on the root port then DMA aliases upstream of
> > the translation unit are irrelevant and the callback function should
> > stop the traversal at that point.
> >  
> >> > You could even think about changing what Linux considers to be the host
> >> > bridge.  Maybe these glue bridges shouldn't even be visible to Linux,
> >> > would that also solve the issue with the broken BAR?  
> >>
> >> The glue bridges have to seen by Linux for assigning resources like
> >> bus ranges and memory windows. So these are required bridges in
> >> the topology and will work with the standard linux code
> >> (provided we skip them for aliases, and and ignore the BAR).
> >>  
> >> >> > The change takes the same code path as it would for a real PCIe bridge
> >> >> > port (downstream/upstream/root), which means they want to skip adding
> >> >> > this bridge as an alias of the device.  So we're adding in aliases that
> >> >> > don't exist, the bridge itself.
> >> >> >
> >> >> > If anything I'd suggest a flag that actually tries to address the
> >> >> > problem rather than a symptom of the problem.  For example, maybe the
> >> >> > flag should be PCI_DEV_FLAGS_IS_PCIE.  Maybe pci_is_pcie() should even
> >> >> > take that into account.  That has some trickle through for
> >> >> > pci_pcie_type() and all the accessor functions, but maybe it's a
> >> >> > cleaner solution overall (or maybe it explodes further).  Thanks,  
> >> >>
> >> >> I didn't really want to mark the glue bridges as PCIe or have fake
> >> >> PCIe capability there, the obvious simple solution was to add
> >> >> the flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS
> >> >>
> >> >> Any suggestions/comments on how to do this better would be welcome.  
> >> >
> >> > I definitely don't think either flag idea is the right solution, I
> >> > think you've actually already got the tools you need to solve it by
> >> > putting the intelligence in the callback function or by going further
> >> > down the path of pretending the glue bridge is really a host bridge.  
> >>
> >> If I understand your suggestion, I need to fake the PCIe capability
> >> for the glue bridges, which may be reasonable for the second level.
> >> But for the first level, there is no pcie capability and faking that
> >> becomes more difficult.  
> >
> > That's not what I'm suggesting.
> >  
> >> If the concern is adding a flag for just one platform, I can check
> >> for the vendor ID/device ID in the pci_for_each_dma_alias()
> >> function, which would be slightly uglier, but still a simple and
> >> maintainable solution.
> >>
> >> Another option is to break the pci_for_each_dma_alias when it
> >> reaches to root port (for vulcan), this too can be done without
> >> a flag.  
> >
> > Exactly, this can be done by the callback function provided on vulcan
> > systems and requires no core changes.  The IOMMU needs to provide a
> > callback function that makes sense for collecting aliases on the
> > specific platform.  If the translation unit is not on the root bus, the
> > callback function should stop the topology walk earlier.  Thanks,  
> 
> Fixing every callback function is going to be painful and ugly.
> I will have to change drivers/irqchip/irq-gic-v3-its-pci-msi.c,
> drivers/pci/msi.c, and drivers/iommu/arm-smmu-v3.c with the
> same check for vendor/device ids and return early.
> 
> I can update the patch to check the vendor/device id in the
> case of ROOT_PORT in pci_for_each_dma_alias(). If you
> think that will not be acceptable, I don't have any reasonable
> options left.

I've never known there to necessarily be a correlation between the
easiest solution and the correct one, but I think there's probably
still an easy and clean solution not far from your original approach.
pci_for_each_dma_alias() is meant to collect all the aliases for a
given device.  It stops at the root bus both because it can't go
further and because on many platforms, that's where the translation
unit lives.  If we want to have a common and easy way to stop it above
the root bus then we could flag a device as being the DMA alias root
and exit at that point.  I think that makes more sense architecturally
than some mystery devices flagged to skip reporting an alias.  Thanks,

Alex

Re: [RFC PATCH 1/2] PCI: Add PCI device flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS

[Jayachandran Chandrashekaran Nair]

Hi Alex,

On Thu, Feb 18, 2016 at 7:44 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Thu, 18 Feb 2016 18:57:12 +0530
> Jayachandran Chandrashekaran Nair
> <jayachandran.chandrashekaran@broadcom.com> wrote:
>
>> On Wed, Feb 17, 2016 at 8:58 PM, Alex Williamson
>> <alex.williamson@redhat.com> wrote:
>> > On Wed, 17 Feb 2016 17:15:09 +0530
>> > Jayachandran Chandrashekaran Nair
>> > <jayachandran.chandrashekaran@broadcom.com> wrote:
>> >
>> >> On Wed, Feb 17, 2016 at 3:55 AM, Alex Williamson
>> >> <alex.williamson@redhat.com> wrote:
>> >> > On Wed, 17 Feb 2016 02:38:24 +0530
>> >> > Jayachandran Chandrashekaran Nair
>> >> > <jayachandran.chandrashekaran@broadcom.com> wrote:
>> >> >
>> >> >> On Tue, Feb 16, 2016 at 1:09 AM, Alex Williamson
>> >> >> <alex.williamson@redhat.com> wrote:
>> >> >> > On Mon, 15 Feb 2016 12:20:23 -0600
>> >> >> > Bjorn Helgaas <helgaas@kernel.org> wrote:
>> >> >> >
>> >> >> >> [+cc Alex, iommu list]
>> >> >> >>
>> >> >> >> On Mon, Feb 15, 2016 at 03:35:00AM +0530, Jayachandran C wrote:
>> >> >> >> > Add a new flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS to indicate bridges
>> >> >> >> > that should not be considered during DMA alias search. This is
>> >> >> >> > to support hardware (in this case Broadcom Vulcan PCIe subsystem)
>> >> >> >> > that has internal bridges which have either missing or wrong PCIe
>> >> >> >> > capabilities.
>> >> >> >
>> >> >> > I figured this would come at some point, the right answer is of course
>> >> >> > to follow the PCIe spec and implement the required PCIe capability in
>> >> >> > the hardware.
>> >> >>
>> >> >> There are  PCIe controllers on the chip that follows the spec, the issue is
>> >> >> how it is integrated to the northbridge (equivalent) on the chip, I have
>> >> >> tried to explain it below.
>> >> >>
>> >> >> >> This needs more explanation, like what exactly is wrong with this
>> >> >> >> device?  A missing PCIe capability might cause other problems.
>> >> >> >>
>> >> >> >> What problem does this fix?  Without these patches, do we merely add
>> >> >> >> aliases that are unnecessary?  Do we crash because something goes
>> >> >> >> wrong in the pci_pcie_type() switch because of the incorrect
>> >> >> >> capability?
>> >> >>
>> >> >> Here's how (for example) how the PCI enumeration of a 2 node Vulcan
>> >> >> processor will look like:
>> >> >>
>> >> >>
>> >> >> [0] +-0.0.0--[1]---+--1.a.0----[2]-----2.0.0---[3]----3.0.0
>> >> >>     |              +--1.a.1----[4]-----4.0.0---[5]----5.0.0
>> >> >>     |              .
>> >> >>     |              ... etc...
>> >> >>     |
>> >> >>     +-0.0.1--[10]--+-10.a.0----[11]---11.0.0---[12]---12.0.0
>> >> >>                    +-10.a.1----[13]---13.0.0---[14]---14.0.0
>> >> >>                    .
>> >> >>                    ... etc...
>> >> >
>> >> > So we have:
>> >> >
>> >> >     "Glue Bridge"---"Glue Bridges"---Root Ports---Endpoints
>> >> >       (no pcie)      (pci/x-pcie)
>> >>
>> >> The top level is one glue bridge per chip in a multi-chip board,
>> >> but otherwise this is accurate.
>> >>
>> >> >>
>> >> >> The devices 0.0.x and x.a.x are glue bridges that are there to
>> >> >> bring the real PCIe controllers (pcie cap type 4) 2.0.0, 4.0.0,
>> >> >> 11.0.0, 13.0.0 under a single PCI hierarchy. The x.a.x bridges
>> >> >> have a PCIe capability (type 8) and 0.0.x does not have any pcie
>> >> >> capability.
>> >> >>
>> >> >> In case of Vulcan, both the GICv3 ITS driver code and the SMMUv3
>> >> >> driver code does dma alias walk to find the device id to use
>> >> >> in ITS and SMMU. In both cases it will ignore the x.0.0 bridges
>> >> >> since they are type root port, but will continue on up and end
>> >> >> up with incorrect device id.
>> >> >>
>> >> >> The flag I have added is to make the pci_for_each_dma_alias()
>> >> >> ignore the last 2 levels of glue/internal bridges.
>> >> >
>> >> > Per the PCIe spec, I'm not even sure what you have is a valid
>> >> > hierarchy, root ports sit on root complexes, not behind a PCI-to-PCIe
>> >> > bridge.  So really you're pretending that the downstream "glue bridge"
>> >> > is your host bridge and therefore root complex, but the PCI topology
>> >> > would clearly dictate that the top-most bus is conventional PCI and
>> >> > therefore everything is an alias of everything else and the DMA alias
>> >> > code is doing exactly what it should.
>> >>
>> >> Yes, I am not arguing that there is any issue in the current code. I
>> >> am trying to figure out the correct way to implement the quirk. We
>> >> have to support the hardware we have, not the hardware we want to
>> >> have :)
>> >>
>> >> > Also note that the caller of pci_for_each_dma_alias() owns the callback
>> >> > function triggered for each alias, that function could choose to prune
>> >> > out these "glue bridges" itself if that's the appropriate thing to do.
>> >>
>> >> I had implemented this first, but moved to the current approach because
>> >> it is needed in multiple places. The problem is: "On vulcan, while
>> >> going up the pcie bus heirarchy for finding aliases, skip the glue bridges",
>> >> So the logical place to put the fix is in pci_for_each_dma_alias()
>> >>
>> >> > Where do the SMMU and ITS actually reside in the above diagram?  You
>> >> > can use the callout function to stop the traversal anywhere you wish,
>> >> > it's free to return a -errno to stop or positive number, which the
>> >> > caller can interpret as error or non-failure stop condition.
>> >>
>> >> The SMMU (for translation requests) and ITS (for MSI writes for
>> >> interrupts) are connected directly to the proper PCIe controller
>> >> (2/4/11/13.0.0 above)
>> >
>> >
>> > If the translation unit is on the root port then DMA aliases upstream of
>> > the translation unit are irrelevant and the callback function should
>> > stop the traversal at that point.
>> >
>> >> > You could even think about changing what Linux considers to be the host
>> >> > bridge.  Maybe these glue bridges shouldn't even be visible to Linux,
>> >> > would that also solve the issue with the broken BAR?
>> >>
>> >> The glue bridges have to seen by Linux for assigning resources like
>> >> bus ranges and memory windows. So these are required bridges in
>> >> the topology and will work with the standard linux code
>> >> (provided we skip them for aliases, and and ignore the BAR).
>> >>
>> >> >> > The change takes the same code path as it would for a real PCIe bridge
>> >> >> > port (downstream/upstream/root), which means they want to skip adding
>> >> >> > this bridge as an alias of the device.  So we're adding in aliases that
>> >> >> > don't exist, the bridge itself.
>> >> >> >
>> >> >> > If anything I'd suggest a flag that actually tries to address the
>> >> >> > problem rather than a symptom of the problem.  For example, maybe the
>> >> >> > flag should be PCI_DEV_FLAGS_IS_PCIE.  Maybe pci_is_pcie() should even
>> >> >> > take that into account.  That has some trickle through for
>> >> >> > pci_pcie_type() and all the accessor functions, but maybe it's a
>> >> >> > cleaner solution overall (or maybe it explodes further).  Thanks,
>> >> >>
>> >> >> I didn't really want to mark the glue bridges as PCIe or have fake
>> >> >> PCIe capability there, the obvious simple solution was to add
>> >> >> the flag PCI_DEV_FLAGS_BRIDGE_SKIP_ALIAS
>> >> >>
>> >> >> Any suggestions/comments on how to do this better would be welcome.
>> >> >
>> >> > I definitely don't think either flag idea is the right solution, I
>> >> > think you've actually already got the tools you need to solve it by
>> >> > putting the intelligence in the callback function or by going further
>> >> > down the path of pretending the glue bridge is really a host bridge.
>> >>
>> >> If I understand your suggestion, I need to fake the PCIe capability
>> >> for the glue bridges, which may be reasonable for the second level.
>> >> But for the first level, there is no pcie capability and faking that
>> >> becomes more difficult.
>> >
>> > That's not what I'm suggesting.
>> >
>> >> If the concern is adding a flag for just one platform, I can check
>> >> for the vendor ID/device ID in the pci_for_each_dma_alias()
>> >> function, which would be slightly uglier, but still a simple and
>> >> maintainable solution.
>> >>
>> >> Another option is to break the pci_for_each_dma_alias when it
>> >> reaches to root port (for vulcan), this too can be done without
>> >> a flag.
>> >
>> > Exactly, this can be done by the callback function provided on vulcan
>> > systems and requires no core changes.  The IOMMU needs to provide a
>> > callback function that makes sense for collecting aliases on the
>> > specific platform.  If the translation unit is not on the root bus, the
>> > callback function should stop the topology walk earlier.  Thanks,
>>
>> Fixing every callback function is going to be painful and ugly.
>> I will have to change drivers/irqchip/irq-gic-v3-its-pci-msi.c,
>> drivers/pci/msi.c, and drivers/iommu/arm-smmu-v3.c with the
>> same check for vendor/device ids and return early.
>>
>> I can update the patch to check the vendor/device id in the
>> case of ROOT_PORT in pci_for_each_dma_alias(). If you
>> think that will not be acceptable, I don't have any reasonable
>> options left.
>
> I've never known there to necessarily be a correlation between the
> easiest solution and the correct one, but I think there's probably
> still an easy and clean solution not far from your original approach.
> pci_for_each_dma_alias() is meant to collect all the aliases for a
> given device.  It stops at the root bus both because it can't go
> further and because on many platforms, that's where the translation
> unit lives.  If we want to have a common and easy way to stop it above
> the root bus then we could flag a device as being the DMA alias root
> and exit at that point.  I think that makes more sense architecturally
> than some mystery devices flagged to skip reporting an alias.  Thanks,

Thanks for the suggestion, I will try to implement this approach and
submit a newer patchset.

JC.