Re: [PATCH] mfd: Export LPC attributes for the system SPI chip

[Mika Westerberg]

On Thu, May 14, 2020 at 01:53:23PM +0100, Richard Hughes wrote:
> On Thu, 14 May 2020 at 13:15, Mika Westerberg
> <mika.westerberg@xxxxxxxxxxxxxxx> wrote:
> > > +What:                /sys/kernel/security/firmware/bioswe
> > Should this still be "firmware_protections" or similar. Plain "firmware"
> > sounds again too generic. Maybe its just me..
> 
> It's not always going to be protections provided by the firmware; it
> might also be restrictions put on the firmware. My first choice was
> /sys/kernel/security/firmware_security/ but having the double
> 'security' just looked redundant.

OK.

> > > +     LPC_SPT,        /* Sunrise Point */
> > > +     LPC_KBL,        /* Kaby Lake */
> > > +     LPC_TGL,        /* Tiger Lake */
> >
> > These all have the SPI-NOR controller as separate PCI device (as ICL and
> > others).
> 
> For Sunrise Point I see:
> 
> 00:1f.0 ISA bridge [0601]: Intel Corporation CM236 Chipset LPC/eSPI
> Controller [8086:a150] (rev 31)
> 00:1f.0 ISA bridge [0601]: Intel Corporation Sunrise Point LPC
> Controller/eSPI Controller [8086:9d4e] (rev 21)
> 
> For Kaby Lake I see:
> 
> 00:1f.0 ISA bridge [0601]: Intel Corporation HM175 Chipset LPC/eSPI
> Controller [8086:a152] (rev 31)",

Yes, both of these have LPC device (1f.0) but the SPI-NOR controller is
separate PCI device and most likely hidden.

> You're indeed correct about Tiger Lake, my apologies.
> 
> > > +     [LPC_SPT] = {
> > > +             .name = "Sunrise Point",
> > > +             .spi_type = INTEL_SPI_LPC,
> > > +     },
> >
> > So all of these have LCP/eSPI controller but the SPI-NOR controller is
> > not accessible through it - it is a separate PCI device.
> 
> I have a Sunrise Point system here -- the lspci is here:
> https://people.freedesktop.org/~hughsient/temp/lspci.txt
> 
> Is the SPI-NOR controller perhaps hidden? If I read the BCR @ 0xdc
> from the 00:1f.0 ISB bridge I get the expected BIOS_WE, BLE and
> SMM_BWP results.

OK, I checked datasheet of KBL and indeed the LPC still has the BIOS
Control (BC) register at 0xdc so that should work. Incidently the same
register is part of the SPI-NOR controller register set.

> > Like you said, Evolution seems to mangle these.
> 
> I'll use git for future patches, thanks.
> 
> > > +             pci_read_config_dword(dev, BCR, &bcr);
> > > +             info->writeable = !!(bcr & BCR_WPD);
> > > +             break;
> > > +
> > > +     case INTEL_SPI_LPC:
> >
> > So instead of this, you can add the security attributes to the existing
> > entries where we are sure there is SPI-NOR controller behind LPC. Here
> > it is not the case and further..
> 
> Sooo I'd use INTEL_SPI_LPT? On my system RCBA isn't set, and so "if
> (!res->start)" bails out with  return -ENODEV;"

I think the INTEL_SPI_LPC is slightly misleading because the SPI is not
accessible through LPC. Instead what if we read the BIOS control
register first in lpc_ich_init_spi() and then bail out since .spi_type
is not set?

Probably we can rename the function lpc_ich_init_spi() to
lpc_ich_init_security_and_spi() or something like that.