From: Jonathan Yong <[email protected]>
There is already one and at least one more user is coming which
requires an access to Primary to Sideband bridge (P2SB) in order to
get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
for x86 devices.
Signed-off-by: Jonathan Yong <[email protected]>
Co-developed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/pci/Kconfig | 8 ++++
drivers/pci/Makefile | 1 +
drivers/pci/pci-p2sb.c | 83 ++++++++++++++++++++++++++++++++++++++++
include/linux/pci-p2sb.h | 28 ++++++++++++++
4 files changed, 120 insertions(+)
create mode 100644 drivers/pci/pci-p2sb.c
create mode 100644 include/linux/pci-p2sb.h
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 0c473d75e625..740e5b30d6fd 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -252,6 +252,14 @@ config PCIE_BUS_PEER2PEER
endchoice
+config PCI_P2SB
+ bool "Primary to Sideband (P2SB) bridge access support"
+ depends on PCI && X86
+ help
+ The Primary to Sideband bridge is an interface to some PCI
+ devices connected through it. In particular, SPI NOR
+ controller in Intel Apollo Lake SoC is one of such devices.
+
source "drivers/pci/hotplug/Kconfig"
source "drivers/pci/controller/Kconfig"
source "drivers/pci/endpoint/Kconfig"
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index d62c4ac4ae1b..eee8d5dda7d9 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_PCI_IOV) += iov.o
obj-$(CONFIG_PCI_BRIDGE_EMUL) += pci-bridge-emul.o
obj-$(CONFIG_PCI_LABEL) += pci-label.o
obj-$(CONFIG_X86_INTEL_MID) += pci-mid.o
+obj-$(CONFIG_PCI_P2SB) += pci-p2sb.o
obj-$(CONFIG_PCI_SYSCALL) += syscall.o
obj-$(CONFIG_PCI_STUB) += pci-stub.o
obj-$(CONFIG_PCI_PF_STUB) += pci-pf-stub.o
diff --git a/drivers/pci/pci-p2sb.c b/drivers/pci/pci-p2sb.c
new file mode 100644
index 000000000000..68d7dad48cdb
--- /dev/null
+++ b/drivers/pci/pci-p2sb.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Primary to Sideband bridge (P2SB) access support
+ *
+ * Copyright (c) 2017, 2021 Intel Corporation.
+ *
+ * Authors: Andy Shevchenko <[email protected]>
+ * Jonathan Yong <[email protected]>
+ */
+
+#include <linux/bitops.h>
+#include <linux/export.h>
+#include <linux/pci-p2sb.h>
+
+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>
+
+#include "pci.h"
+
+#define P2SBC_HIDE_BYTE 0xe1
+#define P2SBC_HIDE_BIT BIT(0)
+
+static const struct x86_cpu_id p2sb_cpu_ids[] = {
+ X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT, PCI_DEVFN(13, 0)),
+ {}
+};
+
+static int pci_p2sb_devfn(unsigned int *devfn)
+{
+ const struct x86_cpu_id *id;
+
+ id = x86_match_cpu(p2sb_cpu_ids);
+ if (!id)
+ return -ENODEV;
+
+ *devfn = (unsigned int)id->driver_data;
+ return 0;
+}
+
+/**
+ * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR
+ * @pdev: PCI device to get a PCI bus to communicate with
+ * @devfn: PCI slot and function to communicate with
+ * @mem: memory resource to be filled in
+ *
+ * The BIOS prevents the P2SB device from being enumerated by the PCI
+ * subsystem, so we need to unhide and hide it back to lookup the BAR.
+ *
+ * Caller must provide a valid pointer to @mem.
+ *
+ * Locking is handled by pci_rescan_remove_lock mutex.
+ *
+ * Return:
+ * 0 on success or appropriate errno value on error.
+ */
+int pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct resource *mem)
+{
+ struct pci_bus *bus = pdev->bus;
+ unsigned int df;
+ int ret;
+
+ /* Get devfn for P2SB device itself */
+ ret = pci_p2sb_devfn(&df);
+ if (ret)
+ return ret;
+
+ pci_lock_rescan_remove();
+
+ /* Unhide the P2SB device */
+ pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, 0);
+
+ /* Read the first BAR of the device in question */
+ __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem, PCI_BASE_ADDRESS_0, true);
+
+ /* Hide the P2SB device */
+ pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT);
+
+ pci_unlock_rescan_remove();
+
+ pci_bus_info(bus, devfn, "BAR: %pR\n", mem);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(pci_p2sb_bar);
diff --git a/include/linux/pci-p2sb.h b/include/linux/pci-p2sb.h
new file mode 100644
index 000000000000..15dd42737c84
--- /dev/null
+++ b/include/linux/pci-p2sb.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Primary to Sideband bridge (P2SB) access support
+ */
+
+#ifndef _PCI_P2SB_H
+#define _PCI_P2SB_H
+
+#include <linux/errno.h>
+
+struct pci_dev;
+struct resource;
+
+#if IS_BUILTIN(CONFIG_PCI_P2SB)
+
+int pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct resource *mem);
+
+#else /* CONFIG_PCI_P2SB is not set */
+
+static inline
+int pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct resource *mem)
+{
+ return -ENODEV;
+}
+
+#endif /* CONFIG_PCI_P2SB */
+
+#endif /* _PCI_P2SB_H */
--
2.30.1
On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> From: Jonathan Yong <[email protected]>
>
> There is already one and at least one more user is coming which
> requires an access to Primary to Sideband bridge (P2SB) in order to
> get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
> for x86 devices.
Can you include a spec reference? I'm trying to figure out why this
belongs in drivers/pci/. It looks very device-specific.
> Signed-off-by: Jonathan Yong <[email protected]>
> Co-developed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/pci/Kconfig | 8 ++++
> drivers/pci/Makefile | 1 +
> drivers/pci/pci-p2sb.c | 83 ++++++++++++++++++++++++++++++++++++++++
> include/linux/pci-p2sb.h | 28 ++++++++++++++
> 4 files changed, 120 insertions(+)
> create mode 100644 drivers/pci/pci-p2sb.c
> create mode 100644 include/linux/pci-p2sb.h
>
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 0c473d75e625..740e5b30d6fd 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -252,6 +252,14 @@ config PCIE_BUS_PEER2PEER
>
> endchoice
>
> +config PCI_P2SB
> + bool "Primary to Sideband (P2SB) bridge access support"
> + depends on PCI && X86
> + help
> + The Primary to Sideband bridge is an interface to some PCI
> + devices connected through it. In particular, SPI NOR
> + controller in Intel Apollo Lake SoC is one of such devices.
This doesn't sound like a "bridge". If it's a bridge, what's on the
primary (upstream) side? What's on the secondary side? What
resources are passed through the bridge, i.e., what transactions does
it transfer from one side to the other?
> source "drivers/pci/hotplug/Kconfig"
> source "drivers/pci/controller/Kconfig"
> source "drivers/pci/endpoint/Kconfig"
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index d62c4ac4ae1b..eee8d5dda7d9 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_PCI_IOV) += iov.o
> obj-$(CONFIG_PCI_BRIDGE_EMUL) += pci-bridge-emul.o
> obj-$(CONFIG_PCI_LABEL) += pci-label.o
> obj-$(CONFIG_X86_INTEL_MID) += pci-mid.o
> +obj-$(CONFIG_PCI_P2SB) += pci-p2sb.o
> obj-$(CONFIG_PCI_SYSCALL) += syscall.o
> obj-$(CONFIG_PCI_STUB) += pci-stub.o
> obj-$(CONFIG_PCI_PF_STUB) += pci-pf-stub.o
> diff --git a/drivers/pci/pci-p2sb.c b/drivers/pci/pci-p2sb.c
> new file mode 100644
> index 000000000000..68d7dad48cdb
> --- /dev/null
> +++ b/drivers/pci/pci-p2sb.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Primary to Sideband bridge (P2SB) access support
> + *
> + * Copyright (c) 2017, 2021 Intel Corporation.
> + *
> + * Authors: Andy Shevchenko <[email protected]>
> + * Jonathan Yong <[email protected]>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/export.h>
> +#include <linux/pci-p2sb.h>
> +
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +
> +#include "pci.h"
> +
> +#define P2SBC_HIDE_BYTE 0xe1
> +#define P2SBC_HIDE_BIT BIT(0)
> +
> +static const struct x86_cpu_id p2sb_cpu_ids[] = {
> + X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT, PCI_DEVFN(13, 0)),
> + {}
> +};
> +
> +static int pci_p2sb_devfn(unsigned int *devfn)
> +{
> + const struct x86_cpu_id *id;
> +
> + id = x86_match_cpu(p2sb_cpu_ids);
> + if (!id)
> + return -ENODEV;
> +
> + *devfn = (unsigned int)id->driver_data;
> + return 0;
> +}
> +
> +/**
> + * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR
> + * @pdev: PCI device to get a PCI bus to communicate with
> + * @devfn: PCI slot and function to communicate with
> + * @mem: memory resource to be filled in
> + *
> + * The BIOS prevents the P2SB device from being enumerated by the PCI
> + * subsystem, so we need to unhide and hide it back to lookup the BAR.
> + *
> + * Caller must provide a valid pointer to @mem.
> + *
> + * Locking is handled by pci_rescan_remove_lock mutex.
> + *
> + * Return:
> + * 0 on success or appropriate errno value on error.
> + */
> +int pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct resource *mem)
> +{
> + struct pci_bus *bus = pdev->bus;
> + unsigned int df;
> + int ret;
> +
> + /* Get devfn for P2SB device itself */
> + ret = pci_p2sb_devfn(&df);
> + if (ret)
> + return ret;
> +
> + pci_lock_rescan_remove();
> +
> + /* Unhide the P2SB device */
> + pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, 0);
> +
> + /* Read the first BAR of the device in question */
> + __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem, PCI_BASE_ADDRESS_0, true);
I don't get this. Apparently this normally hidden device is consuming
PCI address space. The PCI core needs to know about this. If it
doesn't, the PCI core may assign this space to another device.
> + /* Hide the P2SB device */
> + pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT);
> +
> + pci_unlock_rescan_remove();
> +
> + pci_bus_info(bus, devfn, "BAR: %pR\n", mem);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_p2sb_bar);
> diff --git a/include/linux/pci-p2sb.h b/include/linux/pci-p2sb.h
> new file mode 100644
> index 000000000000..15dd42737c84
> --- /dev/null
> +++ b/include/linux/pci-p2sb.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Primary to Sideband bridge (P2SB) access support
> + */
> +
> +#ifndef _PCI_P2SB_H
> +#define _PCI_P2SB_H
> +
> +#include <linux/errno.h>
> +
> +struct pci_dev;
> +struct resource;
> +
> +#if IS_BUILTIN(CONFIG_PCI_P2SB)
> +
> +int pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct resource *mem);
> +
> +#else /* CONFIG_PCI_P2SB is not set */
> +
> +static inline
> +int pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct resource *mem)
> +{
> + return -ENODEV;
> +}
> +
> +#endif /* CONFIG_PCI_P2SB */
> +
> +#endif /* _PCI_P2SB_H */
> --
> 2.30.1
>
On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> > From: Jonathan Yong <[email protected]>
> >
> > There is already one and at least one more user is coming which
> > requires an access to Primary to Sideband bridge (P2SB) in order to
> > get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
> > for x86 devices.
>
> Can you include a spec reference?
I'm not sure I have a public link to the spec. It's the 100 Series PCH [1].
The document number to look for is 546955 [2] and there actually a bit of
information about this.
> I'm trying to figure out why this
> belongs in drivers/pci/. It looks very device-specific.
Because it's all about access to PCI configuration spaces of the (hidden)
devices.
[1]: https://ark.intel.com/content/www/us/en/ark/products/series/98456/intel-100-series-desktop-chipsets.html
[2]: https://medium.com/@jacksonchen_43335/bios-gpio-p2sb-70e9b829b403
...
> > +config PCI_P2SB
> > + bool "Primary to Sideband (P2SB) bridge access support"
> > + depends on PCI && X86
> > + help
> > + The Primary to Sideband bridge is an interface to some PCI
> > + devices connected through it. In particular, SPI NOR
> > + controller in Intel Apollo Lake SoC is one of such devices.
>
> This doesn't sound like a "bridge". If it's a bridge, what's on the
> primary (upstream) side? What's on the secondary side? What
> resources are passed through the bridge, i.e., what transactions does
> it transfer from one side to the other?
It's a confusion terminology here. It's a Bridge according to the spec, but
it is *not* a PCI Bridge as you may had a first impression.
...
> > + /* Unhide the P2SB device */
> > + pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, 0);
> > +
> > + /* Read the first BAR of the device in question */
> > + __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem, PCI_BASE_ADDRESS_0, true);
>
> I don't get this. Apparently this normally hidden device is consuming
> PCI address space. The PCI core needs to know about this. If it
> doesn't, the PCI core may assign this space to another device.
Right, it returns all 1:s to any request so PCI core *thinks* it's plugged off
(like D3cold or so).
> > + /* Hide the P2SB device */
> > + pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT);
--
With Best Regards,
Andy Shevchenko
On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> > > From: Jonathan Yong <[email protected]>
> > >
> > > There is already one and at least one more user is coming which
> > > requires an access to Primary to Sideband bridge (P2SB) in order to
> > > get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
> > > for x86 devices.
> >
> > Can you include a spec reference?
>
> I'm not sure I have a public link to the spec. It's the 100 Series PCH [1].
> The document number to look for is 546955 [2] and there actually a bit of
> information about this.
This link, found by googling for "p2sb bridge", looks like it might
have relevant public links:
https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/
I'd prefer if you could dig out the relevant sections because I really
don't know how to identify them.
> > I'm trying to figure out why this
> > belongs in drivers/pci/. It looks very device-specific.
>
> Because it's all about access to PCI configuration spaces of the (hidden)
> devices.
The PCI core generally doesn't deal with device-specific config
registers.
> [1]: https://ark.intel.com/content/www/us/en/ark/products/series/98456/intel-100-series-desktop-chipsets.html
> [2]: https://medium.com/@jacksonchen_43335/bios-gpio-p2sb-70e9b829b403
>
> ...
>
> > > +config PCI_P2SB
> > > + bool "Primary to Sideband (P2SB) bridge access support"
> > > + depends on PCI && X86
> > > + help
> > > + The Primary to Sideband bridge is an interface to some PCI
> > > + devices connected through it. In particular, SPI NOR
> > > + controller in Intel Apollo Lake SoC is one of such devices.
> >
> > This doesn't sound like a "bridge". If it's a bridge, what's on the
> > primary (upstream) side? What's on the secondary side? What
> > resources are passed through the bridge, i.e., what transactions does
> > it transfer from one side to the other?
>
> It's a confusion terminology here. It's a Bridge according to the spec, but
> it is *not* a PCI Bridge as you may had a first impression.
The code suggests that a register on this device controls whether a
different device is visible in config space. I think it will be
better if we can describe what's happening.
> ...
>
> > > + /* Unhide the P2SB device */
> > > + pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, 0);
> > > +
> > > + /* Read the first BAR of the device in question */
> > > + __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem, PCI_BASE_ADDRESS_0, true);
> >
> > I don't get this. Apparently this normally hidden device is consuming
> > PCI address space. The PCI core needs to know about this. If it
> > doesn't, the PCI core may assign this space to another device.
>
> Right, it returns all 1:s to any request so PCI core *thinks* it's
> plugged off (like D3cold or so).
I'm asking about the MMIO address space. The BAR is a register in
config space. AFAICT, clearing P2SBC_HIDE_BYTE makes that BAR
visible. The BAR describes a region of PCI address space. It looks
like setting P2SBC_HIDE_BIT makes the BAR disappear from config space,
but it sounds like the PCI address space *described* by the BAR is
still claimed by the device. If the device didn't respond to that
MMIO space, you would have no reason to read the BAR at all.
So what keeps the PCI core from assigning that MMIO space to another
device?
This all sounds quite irregular from the point of view of the PCI
core. If a device responds to address space that is not described by
a standard PCI BAR, or by an EA capability, or by one of the legacy
VGA or IDE exceptions, we have a problem. That space must be
described *somehow* in a generic way, e.g., ACPI or similar.
What happens if CONFIG_PCI_P2SB is unset? The device doesn't know
that, and if it is still consuming MMIO address space that we don't
know about, that's a problem.
> > > + /* Hide the P2SB device */
> > > + pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT);
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Am Mon, 8 Mar 2021 19:42:21 -0600
schrieb Bjorn Helgaas <[email protected]>:
> On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> > > > From: Jonathan Yong <[email protected]>
> > > >
> > > > There is already one and at least one more user is coming which
> > > > requires an access to Primary to Sideband bridge (P2SB) in
> > > > order to get IO or MMIO bar hidden by BIOS. Create a library to
> > > > access P2SB for x86 devices.
> > >
> > > Can you include a spec reference?
> >
> > I'm not sure I have a public link to the spec. It's the 100 Series
> > PCH [1]. The document number to look for is 546955 [2] and there
> > actually a bit of information about this.
>
> This link, found by googling for "p2sb bridge", looks like it might
> have relevant public links:
>
> https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/
>
> I'd prefer if you could dig out the relevant sections because I really
> don't know how to identify them.
>
> > > I'm trying to figure out why this
> > > belongs in drivers/pci/. It looks very device-specific.
> >
> > Because it's all about access to PCI configuration spaces of the
> > (hidden) devices.
>
> The PCI core generally doesn't deal with device-specific config
> registers.
>
> > [1]:
> > https://ark.intel.com/content/www/us/en/ark/products/series/98456/intel-100-series-desktop-chipsets.html
> > [2]:
> > https://medium.com/@jacksonchen_43335/bios-gpio-p2sb-70e9b829b403
> >
> > ...
> >
> > > > +config PCI_P2SB
> > > > + bool "Primary to Sideband (P2SB) bridge access support"
> > > > + depends on PCI && X86
> > > > + help
> > > > + The Primary to Sideband bridge is an interface to
> > > > some PCI
> > > > + devices connected through it. In particular, SPI NOR
> > > > + controller in Intel Apollo Lake SoC is one of such
> > > > devices.
> > >
> > > This doesn't sound like a "bridge". If it's a bridge, what's on
> > > the primary (upstream) side? What's on the secondary side? What
> > > resources are passed through the bridge, i.e., what transactions
> > > does it transfer from one side to the other?
> >
> > It's a confusion terminology here. It's a Bridge according to the
> > spec, but it is *not* a PCI Bridge as you may had a first
> > impression.
>
> The code suggests that a register on this device controls whether a
> different device is visible in config space. I think it will be
> better if we can describe what's happening.
>
> > ...
> >
> > > > + /* Unhide the P2SB device */
> > > > + pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, 0);
> > > > +
> > > > + /* Read the first BAR of the device in question */
> > > > + __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem,
> > > > PCI_BASE_ADDRESS_0, true);
> > >
> > > I don't get this. Apparently this normally hidden device is
> > > consuming PCI address space. The PCI core needs to know about
> > > this. If it doesn't, the PCI core may assign this space to
> > > another device.
> >
> > Right, it returns all 1:s to any request so PCI core *thinks* it's
> > plugged off (like D3cold or so).
>
> I'm asking about the MMIO address space. The BAR is a register in
> config space. AFAICT, clearing P2SBC_HIDE_BYTE makes that BAR
> visible. The BAR describes a region of PCI address space. It looks
> like setting P2SBC_HIDE_BIT makes the BAR disappear from config space,
> but it sounds like the PCI address space *described* by the BAR is
> still claimed by the device. If the device didn't respond to that
> MMIO space, you would have no reason to read the BAR at all.
>
> So what keeps the PCI core from assigning that MMIO space to another
> device?
The device will respond to MMIO while being hidden. I am afraid nothing
stops a collision, except for the assumption that the BIOS is always
right and PCI devices never get remapped. But just guessing here.
I have seen devices with coreboot having the P2SB visible, and most
likely relocatable. Making it visible in Linux and not hiding it again
might work, but probably only as long as Linux will not relocate it.
Which i am afraid might seriously upset the BIOS, depending on what a
device does with those GPIOs and which parts are implemented in the
BIOS.
regards,
Henning
> This all sounds quite irregular from the point of view of the PCI
> core. If a device responds to address space that is not described by
> a standard PCI BAR, or by an EA capability, or by one of the legacy
> VGA or IDE exceptions, we have a problem. That space must be
> described *somehow* in a generic way, e.g., ACPI or similar.
>
> What happens if CONFIG_PCI_P2SB is unset? The device doesn't know
> that, and if it is still consuming MMIO address space that we don't
> know about, that's a problem.
>
> > > > + /* Hide the P2SB device */
> > > > + pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE,
> > > > P2SBC_HIDE_BIT);
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
Am Mon, 8 Mar 2021 14:20:16 +0200
schrieb Andy Shevchenko <[email protected]>:
> From: Jonathan Yong <[email protected]>
>
> There is already one and at least one more user is coming which
> requires an access to Primary to Sideband bridge (P2SB) in order to
> get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
> for x86 devices.
>
> Signed-off-by: Jonathan Yong <[email protected]>
> Co-developed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/pci/Kconfig | 8 ++++
> drivers/pci/Makefile | 1 +
> drivers/pci/pci-p2sb.c | 83
> ++++++++++++++++++++++++++++++++++++++++ include/linux/pci-p2sb.h |
> 28 ++++++++++++++ 4 files changed, 120 insertions(+)
> create mode 100644 drivers/pci/pci-p2sb.c
> create mode 100644 include/linux/pci-p2sb.h
>
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 0c473d75e625..740e5b30d6fd 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -252,6 +252,14 @@ config PCIE_BUS_PEER2PEER
>
> endchoice
>
> +config PCI_P2SB
> + bool "Primary to Sideband (P2SB) bridge access support"
> + depends on PCI && X86
> + help
> + The Primary to Sideband bridge is an interface to some PCI
> + devices connected through it. In particular, SPI NOR
> + controller in Intel Apollo Lake SoC is one of such devices.
> +
> source "drivers/pci/hotplug/Kconfig"
> source "drivers/pci/controller/Kconfig"
> source "drivers/pci/endpoint/Kconfig"
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index d62c4ac4ae1b..eee8d5dda7d9 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_PCI_IOV) += iov.o
> obj-$(CONFIG_PCI_BRIDGE_EMUL) += pci-bridge-emul.o
> obj-$(CONFIG_PCI_LABEL) += pci-label.o
> obj-$(CONFIG_X86_INTEL_MID) += pci-mid.o
> +obj-$(CONFIG_PCI_P2SB) += pci-p2sb.o
> obj-$(CONFIG_PCI_SYSCALL) += syscall.o
> obj-$(CONFIG_PCI_STUB) += pci-stub.o
> obj-$(CONFIG_PCI_PF_STUB) += pci-pf-stub.o
> diff --git a/drivers/pci/pci-p2sb.c b/drivers/pci/pci-p2sb.c
> new file mode 100644
> index 000000000000..68d7dad48cdb
> --- /dev/null
> +++ b/drivers/pci/pci-p2sb.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Primary to Sideband bridge (P2SB) access support
> + *
> + * Copyright (c) 2017, 2021 Intel Corporation.
> + *
> + * Authors: Andy Shevchenko <[email protected]>
> + * Jonathan Yong <[email protected]>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/export.h>
> +#include <linux/pci-p2sb.h>
> +
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +
> +#include "pci.h"
> +
> +#define P2SBC_HIDE_BYTE 0xe1
> +#define P2SBC_HIDE_BIT BIT(0)
> +
> +static const struct x86_cpu_id p2sb_cpu_ids[] = {
> + X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT,
> PCI_DEVFN(13, 0)),
> + {}
> +};
> +
> +static int pci_p2sb_devfn(unsigned int *devfn)
> +{
> + const struct x86_cpu_id *id;
> +
> + id = x86_match_cpu(p2sb_cpu_ids);
> + if (!id)
> + return -ENODEV;
> +
> + *devfn = (unsigned int)id->driver_data;
> + return 0;
> +}
> +
> +/**
> + * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR
> + * @pdev: PCI device to get a PCI bus to communicate with
> + * @devfn: PCI slot and function to communicate with
> + * @mem: memory resource to be filled in
Do we really need that many arguments to it?
Before i had, in a platform driver that never had its own pci_dev or bus
res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
if (res-start == 0)
return -ENODEV;
So helper only asked for the devfn, returned base and no dedicated
error code.
With this i need
struct pci_bus *bus = pci_find_bus(0, 0);
struct pci_dev *pci_dev = bus->self;
unsigned int magic_i_do_not_want = PCI_DEVFN(13, 0);
> + * The BIOS prevents the P2SB device from being enumerated by the PCI
> + * subsystem, so we need to unhide and hide it back to lookup the
> BAR.
> + *
> + * Caller must provide a valid pointer to @mem.
> + *
> + * Locking is handled by pci_rescan_remove_lock mutex.
> + *
> + * Return:
> + * 0 on success or appropriate errno value on error.
> + */
> +int pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct
> resource *mem) +{
> + struct pci_bus *bus = pdev->bus;
if (!pdev)
bus = pci_find_bus(0, 0);
Or can we drop the whole arg?
> + unsigned int df;
> + int ret;
> +
> + /* Get devfn for P2SB device itself */
> + ret = pci_p2sb_devfn(&df);
> + if (ret)
> + return ret;
if (!devfn)
devfn = df;
I guess that second devfn is for devices behind that bridge. So
unhiding it might reveal several devices? But when caring about that
p2sb do i really need to know its devfn. If so i would like to get
EXPORT_SYMBOL(pci_p2sb_devfn);
regards,
Henning
> +
> + pci_lock_rescan_remove();
> +
> + /* Unhide the P2SB device */
> + pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, 0);
> +
> + /* Read the first BAR of the device in question */
> + __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem,
> PCI_BASE_ADDRESS_0, true); +
> + /* Hide the P2SB device */
> + pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE,
> P2SBC_HIDE_BIT); +
> + pci_unlock_rescan_remove();
> +
> + pci_bus_info(bus, devfn, "BAR: %pR\n", mem);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_p2sb_bar);
> diff --git a/include/linux/pci-p2sb.h b/include/linux/pci-p2sb.h
> new file mode 100644
> index 000000000000..15dd42737c84
> --- /dev/null
> +++ b/include/linux/pci-p2sb.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Primary to Sideband bridge (P2SB) access support
> + */
> +
> +#ifndef _PCI_P2SB_H
> +#define _PCI_P2SB_H
> +
> +#include <linux/errno.h>
> +
> +struct pci_dev;
> +struct resource;
> +
> +#if IS_BUILTIN(CONFIG_PCI_P2SB)
> +
> +int pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct
> resource *mem); +
> +#else /* CONFIG_PCI_P2SB is not set */
> +
> +static inline
> +int pci_p2sb_bar(struct pci_dev *pdev, unsigned int devfn, struct
> resource *mem) +{
> + return -ENODEV;
> +}
> +
> +#endif /* CONFIG_PCI_P2SB */
> +
> +#endif /* _PCI_P2SB_H */
On Thu, Apr 01, 2021 at 06:45:02PM +0300, Andy Shevchenko wrote:
> On Tue, Mar 09, 2021 at 09:42:52AM +0100, Henning Schild wrote:
> > Am Mon, 8 Mar 2021 19:42:21 -0600
> > schrieb Bjorn Helgaas <[email protected]>:
> > > On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> > > > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
>
> ...
>
> > > > > > + /* Read the first BAR of the device in question */
> > > > > > + __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem,
> > > > > > PCI_BASE_ADDRESS_0, true);
> > > > >
> > > > > I don't get this. Apparently this normally hidden device is
> > > > > consuming PCI address space. The PCI core needs to know
> > > > > about this. If it doesn't, the PCI core may assign this
> > > > > space to another device.
> > > >
> > > > Right, it returns all 1:s to any request so PCI core *thinks*
> > > > it's plugged off (like D3cold or so).
> > >
> > > I'm asking about the MMIO address space. The BAR is a register
> > > in config space. AFAICT, clearing P2SBC_HIDE_BYTE makes that
> > > BAR visible. The BAR describes a region of PCI address space.
> > > It looks like setting P2SBC_HIDE_BIT makes the BAR disappear
> > > from config space, but it sounds like the PCI address space
> > > *described* by the BAR is still claimed by the device. If the
> > > device didn't respond to that MMIO space, you would have no
> > > reason to read the BAR at all.
> > >
> > > So what keeps the PCI core from assigning that MMIO space to
> > > another device?
> >
> > The device will respond to MMIO while being hidden. I am afraid
> > nothing stops a collision, except for the assumption that the BIOS
> > is always right and PCI devices never get remapped. But just
> > guessing here.
> >
> > I have seen devices with coreboot having the P2SB visible, and
> > most likely relocatable. Making it visible in Linux and not hiding
> > it again might work, but probably only as long as Linux will not
> > relocate it. Which i am afraid might seriously upset the BIOS,
> > depending on what a device does with those GPIOs and which parts
> > are implemented in the BIOS.
>
> So the question is, do we have knobs in PCI core to mark device
> fixes in terms of BARs, no relocation must be applied, no other
> devices must have the region?
I think the closest thing is the IORESOURCE_PCI_FIXED bit that we use
for things that must not be moved. Generally PCI resources are
associated with a pci_dev, and we set IORESOURCE_PCI_FIXED for BARs,
e.g., dev->resource[n]. We do that for IDE legacy regions (see
LEGACY_IO_RESOURCE), Langwell devices (pci_fixed_bar_fixup()),
"enhanced allocation" (pci_ea_flags()), and some quirks (quirk_io()).
In your case, the device is hidden so it doesn't respond to config
accesses, so there is no pci_dev for it.
Maybe you could do some sort of quirk that allocates its own struct
resource, fills it in, sets IORESOURCE_PCI_FIXED, and does something
similar to pci_claim_resource()?
Bjorn
On Thu, Apr 01, 2021 at 06:43:11PM +0300, Andy Shevchenko wrote:
> On Sat, Mar 13, 2021 at 10:45:57AM +0100, Henning Schild wrote:
> > Am Mon, 8 Mar 2021 14:20:16 +0200
> > schrieb Andy Shevchenko <[email protected]>:
>
> ...
>
> > > + * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR
> > > + * @pdev: PCI device to get a PCI bus to communicate with
> > > + * @devfn: PCI slot and function to communicate with
> > > + * @mem: memory resource to be filled in
> >
> > Do we really need that many arguments to it?
> >
> > Before i had, in a platform driver that never had its own pci_dev or bus
> >
> > res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
> > if (res-start == 0)
> > return -ENODEV;
> >
> > So helper only asked for the devfn, returned base and no dedicated
> > error code.
> >
> > With this i need
> >
> > struct pci_bus *bus = pci_find_bus(0, 0);
> > struct pci_dev *pci_dev = bus->self;
> > unsigned int magic_i_do_not_want = PCI_DEVFN(13, 0);
>
> What confuses me is the use for SPI NOR controller on Broxton. And I think
> we actually can indeed hide all this under the hood by exposing P2SB to the OS.
>
> Mika, what do you think?
Not sure I follow. Do you mean we force unhide P2SB and then bind (MFD)
driver to that?
On Thu, Apr 01, 2021 at 11:42:56AM -0500, Bjorn Helgaas wrote:
> On Thu, Apr 01, 2021 at 06:45:02PM +0300, Andy Shevchenko wrote:
> > On Tue, Mar 09, 2021 at 09:42:52AM +0100, Henning Schild wrote:
> > > Am Mon, 8 Mar 2021 19:42:21 -0600
> > > schrieb Bjorn Helgaas <[email protected]>:
> > > > On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > > > > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> > > > > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> >
> > ...
> >
> > > > > > > + /* Read the first BAR of the device in question */
> > > > > > > + __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem,
> > > > > > > PCI_BASE_ADDRESS_0, true);
> > > > > >
> > > > > > I don't get this. Apparently this normally hidden device is
> > > > > > consuming PCI address space. The PCI core needs to know
> > > > > > about this. If it doesn't, the PCI core may assign this
> > > > > > space to another device.
> > > > >
> > > > > Right, it returns all 1:s to any request so PCI core *thinks*
> > > > > it's plugged off (like D3cold or so).
> > > >
> > > > I'm asking about the MMIO address space. The BAR is a register
> > > > in config space. AFAICT, clearing P2SBC_HIDE_BYTE makes that
> > > > BAR visible. The BAR describes a region of PCI address space.
> > > > It looks like setting P2SBC_HIDE_BIT makes the BAR disappear
> > > > from config space, but it sounds like the PCI address space
> > > > *described* by the BAR is still claimed by the device. If the
> > > > device didn't respond to that MMIO space, you would have no
> > > > reason to read the BAR at all.
> > > >
> > > > So what keeps the PCI core from assigning that MMIO space to
> > > > another device?
> > >
> > > The device will respond to MMIO while being hidden. I am afraid
> > > nothing stops a collision, except for the assumption that the BIOS
> > > is always right and PCI devices never get remapped. But just
> > > guessing here.
> > >
> > > I have seen devices with coreboot having the P2SB visible, and
> > > most likely relocatable. Making it visible in Linux and not hiding
> > > it again might work, but probably only as long as Linux will not
> > > relocate it. Which i am afraid might seriously upset the BIOS,
> > > depending on what a device does with those GPIOs and which parts
> > > are implemented in the BIOS.
> >
> > So the question is, do we have knobs in PCI core to mark device
> > fixes in terms of BARs, no relocation must be applied, no other
> > devices must have the region?
>
> I think the closest thing is the IORESOURCE_PCI_FIXED bit that we use
> for things that must not be moved. Generally PCI resources are
> associated with a pci_dev, and we set IORESOURCE_PCI_FIXED for BARs,
> e.g., dev->resource[n]. We do that for IDE legacy regions (see
> LEGACY_IO_RESOURCE), Langwell devices (pci_fixed_bar_fixup()),
> "enhanced allocation" (pci_ea_flags()), and some quirks (quirk_io()).
>
> In your case, the device is hidden so it doesn't respond to config
> accesses, so there is no pci_dev for it.
Yes, and the idea is to unhide it on the early stage.
Would it be possible to quirk it to fix the IO resources?
> Maybe you could do some sort of quirk that allocates its own struct
> resource, fills it in, sets IORESOURCE_PCI_FIXED, and does something
> similar to pci_claim_resource()?
--
With Best Regards,
Andy Shevchenko
On Thu, Apr 01, 2021 at 09:06:17PM +0300, Mika Westerberg wrote:
> On Thu, Apr 01, 2021 at 06:43:11PM +0300, Andy Shevchenko wrote:
> > On Sat, Mar 13, 2021 at 10:45:57AM +0100, Henning Schild wrote:
> > > Am Mon, 8 Mar 2021 14:20:16 +0200
> > > schrieb Andy Shevchenko <[email protected]>:
> >
> > ...
> >
> > > > + * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR
> > > > + * @pdev: PCI device to get a PCI bus to communicate with
> > > > + * @devfn: PCI slot and function to communicate with
> > > > + * @mem: memory resource to be filled in
> > >
> > > Do we really need that many arguments to it?
> > >
> > > Before i had, in a platform driver that never had its own pci_dev or bus
> > >
> > > res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
> > > if (res-start == 0)
> > > return -ENODEV;
> > >
> > > So helper only asked for the devfn, returned base and no dedicated
> > > error code.
> > >
> > > With this i need
> > >
> > > struct pci_bus *bus = pci_find_bus(0, 0);
> > > struct pci_dev *pci_dev = bus->self;
> > > unsigned int magic_i_do_not_want = PCI_DEVFN(13, 0);
> >
> > What confuses me is the use for SPI NOR controller on Broxton. And I think
> > we actually can indeed hide all this under the hood by exposing P2SB to the OS.
> >
> > Mika, what do you think?
>
> Not sure I follow. Do you mean we force unhide P2SB and then bind (MFD)
> driver to that?
Not MFD, SPI NOR (if I understood correctly the code in MFD driver for SPI NOR
in regards to P2SB case).
--
With Best Regards,
Andy Shevchenko
On Tue, Mar 09, 2021 at 09:42:52AM +0100, Henning Schild wrote:
> Am Mon, 8 Mar 2021 19:42:21 -0600
> schrieb Bjorn Helgaas <[email protected]>:
> > On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> > > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
...
> > > > > + /* Read the first BAR of the device in question */
> > > > > + __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem,
> > > > > PCI_BASE_ADDRESS_0, true);
> > > >
> > > > I don't get this. Apparently this normally hidden device is
> > > > consuming PCI address space. The PCI core needs to know about
> > > > this. If it doesn't, the PCI core may assign this space to
> > > > another device.
> > >
> > > Right, it returns all 1:s to any request so PCI core *thinks* it's
> > > plugged off (like D3cold or so).
> >
> > I'm asking about the MMIO address space. The BAR is a register in
> > config space. AFAICT, clearing P2SBC_HIDE_BYTE makes that BAR
> > visible. The BAR describes a region of PCI address space. It looks
> > like setting P2SBC_HIDE_BIT makes the BAR disappear from config space,
> > but it sounds like the PCI address space *described* by the BAR is
> > still claimed by the device. If the device didn't respond to that
> > MMIO space, you would have no reason to read the BAR at all.
> >
> > So what keeps the PCI core from assigning that MMIO space to another
> > device?
>
> The device will respond to MMIO while being hidden. I am afraid nothing
> stops a collision, except for the assumption that the BIOS is always
> right and PCI devices never get remapped. But just guessing here.
>
> I have seen devices with coreboot having the P2SB visible, and most
> likely relocatable. Making it visible in Linux and not hiding it again
> might work, but probably only as long as Linux will not relocate it.
> Which i am afraid might seriously upset the BIOS, depending on what a
> device does with those GPIOs and which parts are implemented in the
> BIOS.
So the question is, do we have knobs in PCI core to mark device fixes in terms
of BARs, no relocation must be applied, no other devices must have the region?
--
With Best Regards,
Andy Shevchenko
On Sat, Mar 13, 2021 at 10:45:57AM +0100, Henning Schild wrote:
> Am Mon, 8 Mar 2021 14:20:16 +0200
> schrieb Andy Shevchenko <[email protected]>:
...
> > + * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR
> > + * @pdev: PCI device to get a PCI bus to communicate with
> > + * @devfn: PCI slot and function to communicate with
> > + * @mem: memory resource to be filled in
>
> Do we really need that many arguments to it?
>
> Before i had, in a platform driver that never had its own pci_dev or bus
>
> res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
> if (res-start == 0)
> return -ENODEV;
>
> So helper only asked for the devfn, returned base and no dedicated
> error code.
>
> With this i need
>
> struct pci_bus *bus = pci_find_bus(0, 0);
> struct pci_dev *pci_dev = bus->self;
> unsigned int magic_i_do_not_want = PCI_DEVFN(13, 0);
What confuses me is the use for SPI NOR controller on Broxton. And I think
we actually can indeed hide all this under the hood by exposing P2SB to the OS.
Mika, what do you think?
> I guess that second devfn is for devices behind that bridge. So
> unhiding it might reveal several devices?
Good question. I need a device where actually this happens (hidden P2SB stuff
with let's say SPI NOR there) to see how it looks like in such case. I only
understood the GPIO part. But I'm not so sure anymore.
> But when caring about that
> p2sb do i really need to know its devfn. If so i would like to get
--
With Best Regards,
Andy Shevchenko
On Thu, Apr 01, 2021 at 09:22:02PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 01, 2021 at 09:06:17PM +0300, Mika Westerberg wrote:
> > On Thu, Apr 01, 2021 at 06:43:11PM +0300, Andy Shevchenko wrote:
> > > On Sat, Mar 13, 2021 at 10:45:57AM +0100, Henning Schild wrote:
> > > > Am Mon, 8 Mar 2021 14:20:16 +0200
> > > > schrieb Andy Shevchenko <[email protected]>:
> > >
> > > ...
> > >
> > > > > + * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR
> > > > > + * @pdev: PCI device to get a PCI bus to communicate with
> > > > > + * @devfn: PCI slot and function to communicate with
> > > > > + * @mem: memory resource to be filled in
> > > >
> > > > Do we really need that many arguments to it?
> > > >
> > > > Before i had, in a platform driver that never had its own pci_dev or bus
> > > >
> > > > res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
> > > > if (res-start == 0)
> > > > return -ENODEV;
> > > >
> > > > So helper only asked for the devfn, returned base and no dedicated
> > > > error code.
> > > >
> > > > With this i need
> > > >
> > > > struct pci_bus *bus = pci_find_bus(0, 0);
> > > > struct pci_dev *pci_dev = bus->self;
> > > > unsigned int magic_i_do_not_want = PCI_DEVFN(13, 0);
> > >
> > > What confuses me is the use for SPI NOR controller on Broxton. And I think
> > > we actually can indeed hide all this under the hood by exposing P2SB to the OS.
> > >
> > > Mika, what do you think?
> >
> > Not sure I follow. Do you mean we force unhide P2SB and then bind (MFD)
> > driver to that?
>
> Not MFD, SPI NOR (if I understood correctly the code in MFD driver for SPI NOR
> in regards to P2SB case).
I mean a new MFD driver that binds to the P2SB and that one then exposes
the stuff needed by the SPI-NOR driver.
On Thu, Apr 01, 2021 at 09:23:04PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 01, 2021 at 11:42:56AM -0500, Bjorn Helgaas wrote:
> > On Thu, Apr 01, 2021 at 06:45:02PM +0300, Andy Shevchenko wrote:
> > > On Tue, Mar 09, 2021 at 09:42:52AM +0100, Henning Schild wrote:
> > > > Am Mon, 8 Mar 2021 19:42:21 -0600
> > > > schrieb Bjorn Helgaas <[email protected]>:
> > > > > On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > > > > > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> > > > > > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> > >
> > > ...
> > >
> > > > > > > > + /* Read the first BAR of the device in question */
> > > > > > > > + __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem,
> > > > > > > > PCI_BASE_ADDRESS_0, true);
> > > > > > >
> > > > > > > I don't get this. Apparently this normally hidden device is
> > > > > > > consuming PCI address space. The PCI core needs to know
> > > > > > > about this. If it doesn't, the PCI core may assign this
> > > > > > > space to another device.
> > > > > >
> > > > > > Right, it returns all 1:s to any request so PCI core *thinks*
> > > > > > it's plugged off (like D3cold or so).
> > > > >
> > > > > I'm asking about the MMIO address space. The BAR is a register
> > > > > in config space. AFAICT, clearing P2SBC_HIDE_BYTE makes that
> > > > > BAR visible. The BAR describes a region of PCI address space.
> > > > > It looks like setting P2SBC_HIDE_BIT makes the BAR disappear
> > > > > from config space, but it sounds like the PCI address space
> > > > > *described* by the BAR is still claimed by the device. If the
> > > > > device didn't respond to that MMIO space, you would have no
> > > > > reason to read the BAR at all.
> > > > >
> > > > > So what keeps the PCI core from assigning that MMIO space to
> > > > > another device?
> > > >
> > > > The device will respond to MMIO while being hidden. I am afraid
> > > > nothing stops a collision, except for the assumption that the BIOS
> > > > is always right and PCI devices never get remapped. But just
> > > > guessing here.
> > > >
> > > > I have seen devices with coreboot having the P2SB visible, and
> > > > most likely relocatable. Making it visible in Linux and not hiding
> > > > it again might work, but probably only as long as Linux will not
> > > > relocate it. Which i am afraid might seriously upset the BIOS,
> > > > depending on what a device does with those GPIOs and which parts
> > > > are implemented in the BIOS.
> > >
> > > So the question is, do we have knobs in PCI core to mark device
> > > fixes in terms of BARs, no relocation must be applied, no other
> > > devices must have the region?
> >
> > I think the closest thing is the IORESOURCE_PCI_FIXED bit that we use
> > for things that must not be moved. Generally PCI resources are
> > associated with a pci_dev, and we set IORESOURCE_PCI_FIXED for BARs,
> > e.g., dev->resource[n]. We do that for IDE legacy regions (see
> > LEGACY_IO_RESOURCE), Langwell devices (pci_fixed_bar_fixup()),
> > "enhanced allocation" (pci_ea_flags()), and some quirks (quirk_io()).
> >
> > In your case, the device is hidden so it doesn't respond to config
> > accesses, so there is no pci_dev for it.
>
> Yes, and the idea is to unhide it on the early stage.
> Would it be possible to quirk it to fix the IO resources?
If I read your current patch right, it unhides the device, reads the
BAR, then hides the device again. I didn't see that it would create a
pci_dev for it.
If you unhide it and then enumerate it normally (and mark the BAR as
IORESOURCE_PCI_FIXED to make sure we never move it), that might work.
Then there should be a pci_dev for it, and it would then show up in
sysfs, lspci, etc. And we should insert the BAR in iomem_resource, so
we should see it in /proc/iomem and we won't accidentally put
something else on top of it.
> > Maybe you could do some sort of quirk that allocates its own struct
> > resource, fills it in, sets IORESOURCE_PCI_FIXED, and does something
> > similar to pci_claim_resource()?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On 09.03.21 09:42, Henning Schild wrote:
> The device will respond to MMIO while being hidden. I am afraid nothing
> stops a collision, except for the assumption that the BIOS is always
> right and PCI devices never get remapped. But just guessing here.
What could go wrong if it is remapped, except that this driver would
write to the wrong mmio space ?
If it's unhidden, pci-core should see it and start the usual probing,
right ?
--mtx
--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
[email protected] -- +49-151-27565287
Am Fri, 2 Apr 2021 15:09:12 +0200
schrieb "Enrico Weigelt, metux IT consult" <[email protected]>:
> On 09.03.21 09:42, Henning Schild wrote:
>
> > The device will respond to MMIO while being hidden. I am afraid
> > nothing stops a collision, except for the assumption that the BIOS
> > is always right and PCI devices never get remapped. But just
> > guessing here.
>
> What could go wrong if it is remapped, except that this driver would
> write to the wrong mmio space ?
>
> If it's unhidden, pci-core should see it and start the usual probing,
> right ?
I have seen this guy exposed to Linux on coreboot machines. No issues.
But i can imagine BIOSs that somehow make use of the device and assume
it wont move. So we would at least need a parameter to allow keeping
that device hidden, or "fixed" in memory.
Henning
>
> --mtx
>
On Tue, Apr 06, 2021 at 03:40:01PM +0200, Henning Schild wrote:
> Am Fri, 2 Apr 2021 15:09:12 +0200
> schrieb "Enrico Weigelt, metux IT consult" <[email protected]>:
>
> > On 09.03.21 09:42, Henning Schild wrote:
> >
> > > The device will respond to MMIO while being hidden. I am afraid
> > > nothing stops a collision, except for the assumption that the BIOS
> > > is always right and PCI devices never get remapped. But just
> > > guessing here.
> >
> > What could go wrong if it is remapped, except that this driver would
> > write to the wrong mmio space ?
> >
> > If it's unhidden, pci-core should see it and start the usual probing,
> > right ?
>
> I have seen this guy exposed to Linux on coreboot machines. No issues.
> But i can imagine BIOSs that somehow make use of the device and assume
> it wont move. So we would at least need a parameter to allow keeping
> that device hidden, or "fixed" in memory.
I'm wondering if they have pin control device described in the ACPI.
If so, how in that case you prevent double initialisation?
We would need to check both: P2SB and ACPI tables. Basically if we enable P2SB
as a PCI device we may create a corresponding driver (somewhere under
drivers/pci or PDx86) and check in its probe that ACPI device is also present
and functional.
--
With Best Regards,
Andy Shevchenko
On Thu, Apr 01, 2021 at 09:32:36PM +0300, Mika Westerberg wrote:
> On Thu, Apr 01, 2021 at 09:22:02PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 01, 2021 at 09:06:17PM +0300, Mika Westerberg wrote:
> > > On Thu, Apr 01, 2021 at 06:43:11PM +0300, Andy Shevchenko wrote:
> > > > On Sat, Mar 13, 2021 at 10:45:57AM +0100, Henning Schild wrote:
> > > > > Am Mon, 8 Mar 2021 14:20:16 +0200
> > > > > schrieb Andy Shevchenko <[email protected]>:
> > > >
> > > > ...
> > > >
> > > > > > + * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR
> > > > > > + * @pdev: PCI device to get a PCI bus to communicate with
> > > > > > + * @devfn: PCI slot and function to communicate with
> > > > > > + * @mem: memory resource to be filled in
> > > > >
> > > > > Do we really need that many arguments to it?
> > > > >
> > > > > Before i had, in a platform driver that never had its own pci_dev or bus
> > > > >
> > > > > res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
> > > > > if (res-start == 0)
> > > > > return -ENODEV;
> > > > >
> > > > > So helper only asked for the devfn, returned base and no dedicated
> > > > > error code.
> > > > >
> > > > > With this i need
> > > > >
> > > > > struct pci_bus *bus = pci_find_bus(0, 0);
> > > > > struct pci_dev *pci_dev = bus->self;
> > > > > unsigned int magic_i_do_not_want = PCI_DEVFN(13, 0);
> > > >
> > > > What confuses me is the use for SPI NOR controller on Broxton. And I think
> > > > we actually can indeed hide all this under the hood by exposing P2SB to the OS.
> > > >
> > > > Mika, what do you think?
> > >
> > > Not sure I follow. Do you mean we force unhide P2SB and then bind (MFD)
> > > driver to that?
> >
> > Not MFD, SPI NOR (if I understood correctly the code in MFD driver for SPI NOR
> > in regards to P2SB case).
>
> I mean a new MFD driver that binds to the P2SB and that one then exposes
> the stuff needed by the SPI-NOR driver.
But as far as I understood it doesn't binds to P2SB since we do not have that
device present at PCI enumeration stage.
Maybe at the end of the day the P2SB driver should be located in drivers/mfd
and take over the SPI NOR enumeration as well on platforms in question?
--
With Best Regards,
Andy Shevchenko
On Thu, Apr 01, 2021 at 01:44:46PM -0500, Bjorn Helgaas wrote:
> On Thu, Apr 01, 2021 at 09:23:04PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 01, 2021 at 11:42:56AM -0500, Bjorn Helgaas wrote:
> > > On Thu, Apr 01, 2021 at 06:45:02PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Mar 09, 2021 at 09:42:52AM +0100, Henning Schild wrote:
> > > > > Am Mon, 8 Mar 2021 19:42:21 -0600
> > > > > schrieb Bjorn Helgaas <[email protected]>:
> > > > > > On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > > > > > > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> > > > > > > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> > > >
> > > > ...
> > > >
> > > > > > > > > + /* Read the first BAR of the device in question */
> > > > > > > > > + __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem,
> > > > > > > > > PCI_BASE_ADDRESS_0, true);
> > > > > > > >
> > > > > > > > I don't get this. Apparently this normally hidden device is
> > > > > > > > consuming PCI address space. The PCI core needs to know
> > > > > > > > about this. If it doesn't, the PCI core may assign this
> > > > > > > > space to another device.
> > > > > > >
> > > > > > > Right, it returns all 1:s to any request so PCI core *thinks*
> > > > > > > it's plugged off (like D3cold or so).
> > > > > >
> > > > > > I'm asking about the MMIO address space. The BAR is a register
> > > > > > in config space. AFAICT, clearing P2SBC_HIDE_BYTE makes that
> > > > > > BAR visible. The BAR describes a region of PCI address space.
> > > > > > It looks like setting P2SBC_HIDE_BIT makes the BAR disappear
> > > > > > from config space, but it sounds like the PCI address space
> > > > > > *described* by the BAR is still claimed by the device. If the
> > > > > > device didn't respond to that MMIO space, you would have no
> > > > > > reason to read the BAR at all.
> > > > > >
> > > > > > So what keeps the PCI core from assigning that MMIO space to
> > > > > > another device?
> > > > >
> > > > > The device will respond to MMIO while being hidden. I am afraid
> > > > > nothing stops a collision, except for the assumption that the BIOS
> > > > > is always right and PCI devices never get remapped. But just
> > > > > guessing here.
> > > > >
> > > > > I have seen devices with coreboot having the P2SB visible, and
> > > > > most likely relocatable. Making it visible in Linux and not hiding
> > > > > it again might work, but probably only as long as Linux will not
> > > > > relocate it. Which i am afraid might seriously upset the BIOS,
> > > > > depending on what a device does with those GPIOs and which parts
> > > > > are implemented in the BIOS.
> > > >
> > > > So the question is, do we have knobs in PCI core to mark device
> > > > fixes in terms of BARs, no relocation must be applied, no other
> > > > devices must have the region?
> > >
> > > I think the closest thing is the IORESOURCE_PCI_FIXED bit that we use
> > > for things that must not be moved. Generally PCI resources are
> > > associated with a pci_dev, and we set IORESOURCE_PCI_FIXED for BARs,
> > > e.g., dev->resource[n]. We do that for IDE legacy regions (see
> > > LEGACY_IO_RESOURCE), Langwell devices (pci_fixed_bar_fixup()),
> > > "enhanced allocation" (pci_ea_flags()), and some quirks (quirk_io()).
> > >
> > > In your case, the device is hidden so it doesn't respond to config
> > > accesses, so there is no pci_dev for it.
> >
> > Yes, and the idea is to unhide it on the early stage.
> > Would it be possible to quirk it to fix the IO resources?
>
> If I read your current patch right, it unhides the device, reads the
> BAR, then hides the device again. I didn't see that it would create a
> pci_dev for it.
>
> If you unhide it and then enumerate it normally (and mark the BAR as
> IORESOURCE_PCI_FIXED to make sure we never move it), that might work.
> Then there should be a pci_dev for it, and it would then show up in
> sysfs, lspci, etc. And we should insert the BAR in iomem_resource, so
> we should see it in /proc/iomem and we won't accidentally put
> something else on top of it.
If the PCI device is present and we have ACPI description for the one or more
devices (currently pin control), wouldn't be a conflicting resources issue?
When would be the suitable place to avoid that?
> > > resource, fills it in, sets IORESOURCE_PCI_FIXED, and does something
> > > similar to pci_claim_resource()?
--
With Best Regards,
Andy Shevchenko
On Mon, Jul 12, 2021 at 03:15:17PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 01, 2021 at 01:44:46PM -0500, Bjorn Helgaas wrote:
> > On Thu, Apr 01, 2021 at 09:23:04PM +0300, Andy Shevchenko wrote:
> > > On Thu, Apr 01, 2021 at 11:42:56AM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Apr 01, 2021 at 06:45:02PM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Mar 09, 2021 at 09:42:52AM +0100, Henning Schild wrote:
> > > > > > Am Mon, 8 Mar 2021 19:42:21 -0600
> > > > > > schrieb Bjorn Helgaas <[email protected]>:
> > > > > > > On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > > > > > > > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> > > > > > > > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> > > > >
> > > > > ...
> > > > >
> > > > > > > > > > + /* Read the first BAR of the device in question */
> > > > > > > > > > + __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem,
> > > > > > > > > > PCI_BASE_ADDRESS_0, true);
> > > > > > > > >
> > > > > > > > > I don't get this. Apparently this normally hidden device is
> > > > > > > > > consuming PCI address space. The PCI core needs to know
> > > > > > > > > about this. If it doesn't, the PCI core may assign this
> > > > > > > > > space to another device.
> > > > > > > >
> > > > > > > > Right, it returns all 1:s to any request so PCI core *thinks*
> > > > > > > > it's plugged off (like D3cold or so).
> > > > > > >
> > > > > > > I'm asking about the MMIO address space. The BAR is a register
> > > > > > > in config space. AFAICT, clearing P2SBC_HIDE_BYTE makes that
> > > > > > > BAR visible. The BAR describes a region of PCI address space.
> > > > > > > It looks like setting P2SBC_HIDE_BIT makes the BAR disappear
> > > > > > > from config space, but it sounds like the PCI address space
> > > > > > > *described* by the BAR is still claimed by the device. If the
> > > > > > > device didn't respond to that MMIO space, you would have no
> > > > > > > reason to read the BAR at all.
> > > > > > >
> > > > > > > So what keeps the PCI core from assigning that MMIO space to
> > > > > > > another device?
> > > > > >
> > > > > > The device will respond to MMIO while being hidden. I am afraid
> > > > > > nothing stops a collision, except for the assumption that the BIOS
> > > > > > is always right and PCI devices never get remapped. But just
> > > > > > guessing here.
> > > > > >
> > > > > > I have seen devices with coreboot having the P2SB visible, and
> > > > > > most likely relocatable. Making it visible in Linux and not hiding
> > > > > > it again might work, but probably only as long as Linux will not
> > > > > > relocate it. Which i am afraid might seriously upset the BIOS,
> > > > > > depending on what a device does with those GPIOs and which parts
> > > > > > are implemented in the BIOS.
> > > > >
> > > > > So the question is, do we have knobs in PCI core to mark device
> > > > > fixes in terms of BARs, no relocation must be applied, no other
> > > > > devices must have the region?
> > > >
> > > > I think the closest thing is the IORESOURCE_PCI_FIXED bit that we use
> > > > for things that must not be moved. Generally PCI resources are
> > > > associated with a pci_dev, and we set IORESOURCE_PCI_FIXED for BARs,
> > > > e.g., dev->resource[n]. We do that for IDE legacy regions (see
> > > > LEGACY_IO_RESOURCE), Langwell devices (pci_fixed_bar_fixup()),
> > > > "enhanced allocation" (pci_ea_flags()), and some quirks (quirk_io()).
> > > >
> > > > In your case, the device is hidden so it doesn't respond to config
> > > > accesses, so there is no pci_dev for it.
> > >
> > > Yes, and the idea is to unhide it on the early stage.
> > > Would it be possible to quirk it to fix the IO resources?
> >
> > If I read your current patch right, it unhides the device, reads the
> > BAR, then hides the device again. I didn't see that it would create a
> > pci_dev for it.
> >
> > If you unhide it and then enumerate it normally (and mark the BAR as
> > IORESOURCE_PCI_FIXED to make sure we never move it), that might work.
> > Then there should be a pci_dev for it, and it would then show up in
> > sysfs, lspci, etc. And we should insert the BAR in iomem_resource, so
> > we should see it in /proc/iomem and we won't accidentally put
> > something else on top of it.
>
> If the PCI device is present and we have ACPI description for the one or more
> devices (currently pin control), wouldn't be a conflicting resources issue?
>
> When would be the suitable place to avoid that?
Given another thought on that and I think we can't unhide entire P2SB due to
possible ACPI tables present which may or may not fully or partially describe
devices behind that bridge, so, I would stick with current approach.
> > > > resource, fills it in, sets IORESOURCE_PCI_FIXED, and does something
> > > > similar to pci_claim_resource()?
--
With Best Regards,
Andy Shevchenko
On Mon, Mar 08, 2021 at 07:42:21PM -0600, Bjorn Helgaas wrote:
> On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> > > > From: Jonathan Yong <[email protected]>
> > > >
> > > > There is already one and at least one more user is coming which
> > > > requires an access to Primary to Sideband bridge (P2SB) in order to
> > > > get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
> > > > for x86 devices.
> > >
> > > Can you include a spec reference?
> >
> > I'm not sure I have a public link to the spec. It's the 100 Series PCH [1].
> > The document number to look for is 546955 [2] and there actually a bit of
> > information about this.
>
> This link, found by googling for "p2sb bridge", looks like it might
> have relevant public links:
>
> https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/
>
> I'd prefer if you could dig out the relevant sections because I really
> don't know how to identify them.
I'm not sure I understand what you would like to see. The information about
P2SB here has confidential tag. I probably can use the document number and cite
couple of paragraphs from it. Would it be sufficient?
> > > I'm trying to figure out why this
> > > belongs in drivers/pci/. It looks very device-specific.
> >
> > Because it's all about access to PCI configuration spaces of the (hidden)
> > devices.
>
> The PCI core generally doesn't deal with device-specific config
> registers.
The location is purely based on practical reason, after the fact that P2SB
is a PCI device, of deduplicating BAR decoding code. I can easily move this
outside of PCI subsystem, but I will need to export this function instead.
Would it work for you?
> > [1]: https://ark.intel.com/content/www/us/en/ark/products/series/98456/intel-100-series-desktop-chipsets.html
> > [2]: https://medium.com/@jacksonchen_43335/bios-gpio-p2sb-70e9b829b403
...
> The code suggests that a register on this device controls whether a
> different device is visible in config space. I think it will be
> better if we can describe what's happening.
Actually it seems incorrect assumption (while it works by some reason).
I have to double test this.
From the doc:
"The P2SB is enumerated as a normal PCI device. ...
Writing a 1 to the P2SBC.HIDE field in the P2SB PCI Configuration space
hides the device; writing a 0 to this field, unhides the device."
It clearly states the P2SB PCI configuration space.
Also it looks like Henning pointed out to this by asking why we need too many
parameters to the function.
...
> > > > + /* Unhide the P2SB device */
> > > > + pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, 0);
> > > > +
> > > > + /* Read the first BAR of the device in question */
> > > > + __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem, PCI_BASE_ADDRESS_0, true);
> > >
> > > I don't get this. Apparently this normally hidden device is consuming
> > > PCI address space. The PCI core needs to know about this. If it
> > > doesn't, the PCI core may assign this space to another device.
> >
> > Right, it returns all 1:s to any request so PCI core *thinks* it's
> > plugged off (like D3cold or so).
>
> I'm asking about the MMIO address space.
> The BAR is a register in
> config space. AFAICT, clearing P2SBC_HIDE_BYTE makes that BAR
> visible. The BAR describes a region of PCI address space. It looks
> like setting P2SBC_HIDE_BIT makes the BAR disappear from config space,
> but it sounds like the PCI address space *described* by the BAR is
> still claimed by the device. If the device didn't respond to that
> MMIO space, you would have no reason to read the BAR at all.
>
> So what keeps the PCI core from assigning that MMIO space to another
> device?
>
> This all sounds quite irregular from the point of view of the PCI
> core. If a device responds to address space that is not described by
> a standard PCI BAR, or by an EA capability, or by one of the legacy
> VGA or IDE exceptions, we have a problem. That space must be
> described *somehow* in a generic way, e.g., ACPI or similar.
>
> What happens if CONFIG_PCI_P2SB is unset? The device doesn't know
> that, and if it is still consuming MMIO address space that we don't
> know about, that's a problem.
Yeah, Henning already answered on this and I believe that nothing prevents OS
to try that addresses for other PCI devices, except the memory region
reservation (by ACPI and / or e820 meaning). It means that we rely on firmware
to do the right thing if it hides the P2SB bar.
And at the same time P2SB bar is used as a part of telling OS where the *fixed*
16Mb region of MMIO is located.
> > > > + /* Hide the P2SB device */
> > > > + pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT);
--
With Best Regards,
Andy Shevchenko
On Fri, Nov 26, 2021 at 05:38:46PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 08, 2021 at 07:42:21PM -0600, Bjorn Helgaas wrote:
> > On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> > > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> > > > > From: Jonathan Yong <[email protected]>
> > > > >
> > > > > There is already one and at least one more user is coming which
> > > > > requires an access to Primary to Sideband bridge (P2SB) in order to
> > > > > get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
> > > > > for x86 devices.
> > > >
> > > > Can you include a spec reference?
> > >
> > > I'm not sure I have a public link to the spec. It's the 100 Series PCH [1].
> > > The document number to look for is 546955 [2] and there actually a bit of
> > > information about this.
> >
> > This link, found by googling for "p2sb bridge", looks like it might
> > have relevant public links:
> >
> > https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/
> >
> > I'd prefer if you could dig out the relevant sections because I really
> > don't know how to identify them.
>
> I'm not sure I understand what you would like to see. The
> information about P2SB here has confidential tag. I probably can use
> the document number and cite couple of paragraphs from it. Would it
> be sufficient?
This patch proposes to add drivers/pci/pci-p2sb.c. Things in
drivers/pci/ should generally be documented via PCI-SIG specs to make
them maintainable. pci-p2sb.c is clearly x86- and Intel-specific.
Maybe arch/x86/pci/ would be a better place for it?
If I were to maintain it under drivers/pci/, I would want some
description about P2SBC_HIDE_BYTE, which device's config space it is
in (apparently 0d.0 of some CPU), and which device it hides/unhides
(apparently some device X other than the CPU).
> > The code suggests that a register on this device controls whether a
> > different device is visible in config space. I think it will be
> > better if we can describe what's happening.
>
> Actually it seems incorrect assumption (while it works by some reason).
> I have to double test this.
>
> From the doc:
>
> "The P2SB is enumerated as a normal PCI device. ...
> Writing a 1 to the P2SBC.HIDE field in the P2SB PCI Configuration space
> hides the device; writing a 0 to this field, unhides the device."
>
> It clearly states the P2SB PCI configuration space.
It clearly says P2SBC.HIDE is in P2SB config space. But I think it's
talking about hiding/unhiding a device other than P2SB.
This patch suggests the P2SB device is at PCI_DEVFN(13, 0), and the
lpc_ich_init_spi() patch suggests there's a SPI controller at
PCI_DEVFN(13, 2). And apparently P2SBC_HIDE_BIT in PCI_DEVFN(13, 0)
determines whether PCI_DEVFN(13, 2) appears in config space.
So it sounds like P2SB is always visible in config space and is not
itself a "bridge". The SPI controller, which *is* a bridge from PCI
to SPI, appears at PCI_DEVFN(13, 2) in config space when
P2SBC_HIDE_BIT is cleared.
> > This all sounds quite irregular from the point of view of the PCI
> > core. If a device responds to address space that is not described by
> > a standard PCI BAR, or by an EA capability, or by one of the legacy
> > VGA or IDE exceptions, we have a problem. That space must be
> > described *somehow* in a generic way, e.g., ACPI or similar.
> >
> > What happens if CONFIG_PCI_P2SB is unset? The device doesn't know
> > that, and if it is still consuming MMIO address space that we don't
> > know about, that's a problem.
>
> Yeah, Henning already answered on this and I believe that nothing
> prevents OS to try that addresses for other PCI devices, except the
> memory region reservation (by ACPI and / or e820 meaning). It means
> that we rely on firmware to do the right thing if it hides the P2SB
> bar.
>
> And at the same time P2SB bar is used as a part of telling OS where
> the *fixed* 16Mb region of MMIO is located.
If the SPI controller consumes PCI address space, that space must be
discoverable by standard PCI enumeration and BAR sizing.
If the BIOS hides the device, I assume that means it does not respond
in config space and does not consume MMIO space. That's fine.
If the BIOS hides the device and the OS unhides it, the unhide should
happen before PCI device enumeration, so the PCI core will find the
device and learn about the MMIO space it consumes.
Bjorn
On Mon, Nov 29, 2021 at 03:07:05PM -0600, Bjorn Helgaas wrote:
> On Fri, Nov 26, 2021 at 05:38:46PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 08, 2021 at 07:42:21PM -0600, Bjorn Helgaas wrote:
> > > On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> > > > > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> > > > > > From: Jonathan Yong <[email protected]>
> > > > > >
> > > > > > There is already one and at least one more user is coming which
> > > > > > requires an access to Primary to Sideband bridge (P2SB) in order to
> > > > > > get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
> > > > > > for x86 devices.
> > > > >
> > > > > Can you include a spec reference?
> > > >
> > > > I'm not sure I have a public link to the spec. It's the 100 Series PCH [1].
> > > > The document number to look for is 546955 [2] and there actually a bit of
> > > > information about this.
> > >
> > > This link, found by googling for "p2sb bridge", looks like it might
> > > have relevant public links:
> > >
> > > https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/
> > >
> > > I'd prefer if you could dig out the relevant sections because I really
> > > don't know how to identify them.
> >
> > I'm not sure I understand what you would like to see. The
> > information about P2SB here has confidential tag. I probably can use
> > the document number and cite couple of paragraphs from it. Would it
> > be sufficient?
>
> This patch proposes to add drivers/pci/pci-p2sb.c. Things in
> drivers/pci/ should generally be documented via PCI-SIG specs to make
> them maintainable. pci-p2sb.c is clearly x86- and Intel-specific.
> Maybe arch/x86/pci/ would be a better place for it?
It's not CPU core, so x86 maintainers for sure be happy to reject this.
The only fit I see for that is PDx86.
> If I were to maintain it under drivers/pci/, I would want some
> description about P2SBC_HIDE_BYTE, which device's config space it is
> in (apparently 0d.0 of some CPU), and which device it hides/unhides
> (apparently some device X other than the CPU).
Since there is publicly (leaked?) available descriptions of it, it's good
to add documentation to the series wherever it will land to.
> > > The code suggests that a register on this device controls whether a
> > > different device is visible in config space. I think it will be
> > > better if we can describe what's happening.
> >
> > Actually it seems incorrect assumption (while it works by some reason).
> > I have to double test this.
> >
> > From the doc:
> >
> > "The P2SB is enumerated as a normal PCI device. ...
> > Writing a 1 to the P2SBC.HIDE field in the P2SB PCI Configuration space
> > hides the device; writing a 0 to this field, unhides the device."
> >
> > It clearly states the P2SB PCI configuration space.
>
> It clearly says P2SBC.HIDE is in P2SB config space. But I think it's
> talking about hiding/unhiding a device other than P2SB.
> This patch suggests the P2SB device is at PCI_DEVFN(13, 0), and the
> lpc_ich_init_spi() patch suggests there's a SPI controller at
> PCI_DEVFN(13, 2). And apparently P2SBC_HIDE_BIT in PCI_DEVFN(13, 0)
> determines whether PCI_DEVFN(13, 2) appears in config space.
Ha, now I can answer to this. P2SB is function 0 of the device, which means
that when it's absent the whole device functions are absent. When you enable
function 0, it enables all devices behind it (which is according to the PCI
requirements).
> So it sounds like P2SB is always visible in config space
Determine "visible" here. In terms of PCI software protocol communication it's
not ("hidden"). In terms of hardware access, the writes to this device are
filtered based on this "hidden"-"unhidden" policy, which declares that one bit
in one register is always available, that's it.
> and is not
> itself a "bridge".
It's not.
> The SPI controller, which *is* a bridge from PCI
> to SPI, appears at PCI_DEVFN(13, 2) in config space when
> P2SBC_HIDE_BIT is cleared.
Yes, as PCI talks about. No function can be available (enabled) until function
0 is enabled. It's a side effect here.
> > > This all sounds quite irregular from the point of view of the PCI
> > > core. If a device responds to address space that is not described by
> > > a standard PCI BAR, or by an EA capability, or by one of the legacy
> > > VGA or IDE exceptions, we have a problem. That space must be
> > > described *somehow* in a generic way, e.g., ACPI or similar.
> > >
> > > What happens if CONFIG_PCI_P2SB is unset? The device doesn't know
> > > that, and if it is still consuming MMIO address space that we don't
> > > know about, that's a problem.
> >
> > Yeah, Henning already answered on this and I believe that nothing
> > prevents OS to try that addresses for other PCI devices, except the
> > memory region reservation (by ACPI and / or e820 meaning). It means
> > that we rely on firmware to do the right thing if it hides the P2SB
> > bar.
> >
> > And at the same time P2SB bar is used as a part of telling OS where
> > the *fixed* 16Mb region of MMIO is located.
>
> If the SPI controller consumes PCI address space, that space must be
> discoverable by standard PCI enumeration and BAR sizing.
I have checked that memory is even supplied as reserved even without device
being visible. Yes, agree on this.
> If the BIOS hides the device, I assume that means it does not respond
> in config space and does not consume MMIO space. That's fine.
Not really. That MMIO space (behind P2SB, I'm not talking about this
SPI special case) is kinda hard coded to the devices. I haven't clarified
yet if the high part of the BAR can be relocated.
> If the BIOS hides the device and the OS unhides it, the unhide should
> happen before PCI device enumeration, so the PCI core will find the
> device and learn about the MMIO space it consumes.
Yep.
--
With Best Regards,
Andy Shevchenko