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

[Richard Hughes]

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.

> > +     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)",

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.

> 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;"

> Otherwise this looks good, nice work :)

I thank you for your patience so far, what I've got the hang of this I
promise to start being more useful.

Richard.