Re: [PATCH v3 3/8] platform/x86/intel: Add Primary to Sideband (P2SB) bridge support

[Bjorn Helgaas <helgaas@xxxxxxxxxx>]

On Fri, Jan 28, 2022 at 08:30:20PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 07, 2022 at 11:11:08AM -0600, Bjorn Helgaas wrote:
> > On Fri, Jan 07, 2022 at 04:56:42PM +0200, Andy Shevchenko wrote:
> 
> ...
> 
> > > The unhide/hide back has been tested and we have already users
> > > in the kernel (they have other issues though with the PCI rescan
> > > lock, but it doesn't mean it wasn't ever tested).
> > 
> > Does the firmware team that hid this device sign off on the OS
> > unhiding and using it?  How do we know that BIOS is not using the
> > device?
> 
> BIOS might use the device via OperationRegion() in ACPI, but that
> means that _CRS needs to have that region available. It seems not
> the case.
> 
> And as far I as see in the internal documentation the hide / unhide
> approach is not forbidden for OS side.

Unhiding is device-specific behavior, so generic PCI enumeration
cannot use it.  We have to know there's a P2SB device at some address
before we can safely do a config write to it.  PCI enumeration would
learn there's a P2SB device at an address by reading a Vendor/Device
ID.

> > My point is that the unhide is architecturally messed up.  The OS
> > runs on the platform as described by ACPI.  Devices that cannot be
> > enumerated are described in the ACPI namespace.
> 
> This device may or may not be _partially_ or _fully_ (due to being
> multifunctional) described in ACPI. I agree, that ideally the
> devices in question it has behind should be represented properly by
> firmware.  However, the firmwares in the wild for selected products
> / devices don't do that. We need to solve (work around) it in the
> software.
> 
> This is already done for a few devices. This series consolidates
> that and enables it for very known GPIO IPs.

Consolidating the code to unhide the device and size the BAR is fine.

I would prefer the PCI core to be involved as little as possible
because we're violating some key assumptions and we could trip over
those later.  We're assuming the existence of P2SB based on the fact
that we found some *other* device, we're assuming firmware isn't using
P2SB (may be true now, but impossible to verify), we're assuming the
P2SB BAR contains a valid address that's not used elsewhere but also
won't be assigned to anything.

> PCI core just provides a code that is very similar to what we need
> here. Are you specifically suggesting that we have to copy'n'paste
> that rather long function and maintain in parallel with PCI?

I think we're talking about __pci_read_base(), which is currently an
internal PCI interface.  This series adds pci_bus_info/warn/etc(),
reworks __pci_read_base() to operate on a struct pci_bus *, exports
it, and uses it via #include <../../../pci/pci.h>.

__pci_read_base() is fairly long, but you apparently don't need all
the functionality there because the core of the patch is this:

  -   pci_bus_read_config_dword(bus, spi, PCI_BASE_ADDRESS_0,
  -                             &spi_base);
  -   if (spi_base != ~0) {
  -           res->start = spi_base & 0xfffffff0;
  -           res->end = res->start + SPIBASE_APL_SZ - 1;
  -   }
  +   __pci_bus_read_base(bus, devfn, pci_bar_unknown, mem, PCI_BASE_ADDRESS_0, true)

I don't think it's worth all the __pci_read_base() changes to do that.
What if you made a library function that looks like this?

  int p2sb_bar(...)
  {
    pci_bus_write_config_byte(bus, devfn_p2sb, P2SBC_HIDE_BYTE, 0);
    pci_bus_read_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, &orig);
    if (orig) {
      pci_bus_write_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, ~0);
      pci_bus_read_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, &val);
      pci_bus_write_config_dword(bus, devfn, PCI_BASE_ADDRESS_0, orig);
      res->start = orig;
      res->end = res->start + (~val + 1);
    }
    pci_bus_write_config_byte(bus, devfn, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT);
  }