| Message ID | 20190409184958.7476-2-sashal@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ftpm: a firmware based TPM driver | expand |
On Tue, Apr 9, 2019 at 1:50 PM Sasha Levin <sashal@kernel.org> wrote: > > The parameters are similar to the ones used by IBM's vTPM and the > various I2C tpm drivers. Bindings describe h/w (or firmware interfaces in this case), not drivers. > > Signed-off-by: Sasha Levin <sashal@kernel.org> > --- > .../bindings/security/tpm/tpm_ftpm_tee.txt | 13 +++++++++++++ > .../devicetree/bindings/vendor-prefixes.txt | 1 + > 2 files changed, 14 insertions(+) > create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt > > diff --git a/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt > new file mode 100644 > index 000000000000..20fca67a56c4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt > @@ -0,0 +1,13 @@ > +Required properties: > +- compatible: should be "microsoft,ftpm" > +- linux,sml-base: 64-bit base address of the reserved memory allocated > + for the firmware event log > +- linux,sml-size: size of the memory allocated for the firmware event log Firmware is defining linux specific properties? What if I want to run BSD? We should use 'reg' here instead. What memory is used here? This should be under /reserved-memory if it is part of "main" memory. Really, I'd prefer to not see this in DT at all. Make the firmware discoverable. Why repeat the mistakes of non-discoverable h/w in s/w interfaces? OP-Tee at least has defined a mechanism to enumerate TEE functions IIRC. Rob
On Tue, Apr 09, 2019 at 04:18:29PM -0500, Rob Herring wrote: >On Tue, Apr 9, 2019 at 1:50 PM Sasha Levin <sashal@kernel.org> wrote: >> >> The parameters are similar to the ones used by IBM's vTPM and the >> various I2C tpm drivers. > >Bindings describe h/w (or firmware interfaces in this case), not drivers. > >> >> Signed-off-by: Sasha Levin <sashal@kernel.org> >> --- >> .../bindings/security/tpm/tpm_ftpm_tee.txt | 13 +++++++++++++ >> .../devicetree/bindings/vendor-prefixes.txt | 1 + >> 2 files changed, 14 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt >> >> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt >> new file mode 100644 >> index 000000000000..20fca67a56c4 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt >> @@ -0,0 +1,13 @@ >> +Required properties: >> +- compatible: should be "microsoft,ftpm" >> +- linux,sml-base: 64-bit base address of the reserved memory allocated >> + for the firmware event log >> +- linux,sml-size: size of the memory allocated for the firmware event log > >Firmware is defining linux specific properties? What if I want to run >BSD? We should use 'reg' here instead. This is based on already existing code that defines these names, see tpm_read_log_of() in drivers/char/tpm/eventlog/of.c . These properties were described similarily by other interfaces (see Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt or Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt for example). We could rename them all if you'd like, I was just trying to follow the existing code. >What memory is used here? This should be under /reserved-memory if it >is part of "main" memory. That's my understanding, yes. >Really, I'd prefer to not see this in DT at all. Make the firmware >discoverable. Why repeat the mistakes of non-discoverable h/w in s/w >interfaces? OP-Tee at least has defined a mechanism to enumerate TEE >functions IIRC. Sadly the firmware already exists as-is on live hardware, there is a paper describing it back from 2016 and we're stuck having to support that. -- Thanks, Sasha
On Wed, Apr 10, 2019 at 11:19 AM Sasha Levin <sashal@kernel.org> wrote: > > On Tue, Apr 09, 2019 at 04:18:29PM -0500, Rob Herring wrote: > >On Tue, Apr 9, 2019 at 1:50 PM Sasha Levin <sashal@kernel.org> wrote: > >> > >> The parameters are similar to the ones used by IBM's vTPM and the > >> various I2C tpm drivers. > > > >Bindings describe h/w (or firmware interfaces in this case), not drivers. > > > >> > >> Signed-off-by: Sasha Levin <sashal@kernel.org> > >> --- > >> .../bindings/security/tpm/tpm_ftpm_tee.txt | 13 +++++++++++++ > >> .../devicetree/bindings/vendor-prefixes.txt | 1 + > >> 2 files changed, 14 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt > >> > >> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt > >> new file mode 100644 > >> index 000000000000..20fca67a56c4 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt > >> @@ -0,0 +1,13 @@ > >> +Required properties: > >> +- compatible: should be "microsoft,ftpm" > >> +- linux,sml-base: 64-bit base address of the reserved memory allocated > >> + for the firmware event log > >> +- linux,sml-size: size of the memory allocated for the firmware event log > > > >Firmware is defining linux specific properties? What if I want to run > >BSD? We should use 'reg' here instead. > > This is based on already existing code that defines these names, see > tpm_read_log_of() in drivers/char/tpm/eventlog/of.c . BTW, that probably needs updating to handle endianness correctly. > These properties were described similarily by other interfaces (see > Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt or > Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt for example). > > We could rename them all if you'd like, I was just trying to follow the > existing code. > > >What memory is used here? This should be under /reserved-memory if it > >is part of "main" memory. > > That's my understanding, yes. > > >Really, I'd prefer to not see this in DT at all. Make the firmware > >discoverable. Why repeat the mistakes of non-discoverable h/w in s/w > >interfaces? OP-Tee at least has defined a mechanism to enumerate TEE > >functions IIRC. > > Sadly the firmware already exists as-is on live hardware, there is a > paper describing it back from 2016 and we're stuck having to support > that. Does the firmware depend on this binding or the DT just gets statically populated with the address/size the firmware hardcodes? I'd prefer not to propagate the IBM binding that we were kind of stuck with. I had similar comments on it... Are there other parts of the TEE firmware that need to be described in DT? Rob
On Wed, Apr 10, 2019 at 12:01:37PM -0500, Rob Herring wrote: > On Wed, Apr 10, 2019 at 11:19 AM Sasha Levin <sashal@kernel.org> wrote: > > > > On Tue, Apr 09, 2019 at 04:18:29PM -0500, Rob Herring wrote: > > >On Tue, Apr 9, 2019 at 1:50 PM Sasha Levin <sashal@kernel.org> wrote: > > >> > > >> The parameters are similar to the ones used by IBM's vTPM and the > > >> various I2C tpm drivers. > > > > > >Bindings describe h/w (or firmware interfaces in this case), not drivers. > > > > > >> > > >> Signed-off-by: Sasha Levin <sashal@kernel.org> > > >> .../bindings/security/tpm/tpm_ftpm_tee.txt | 13 +++++++++++++ > > >> .../devicetree/bindings/vendor-prefixes.txt | 1 + > > >> 2 files changed, 14 insertions(+) > > >> create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt > > >> > > >> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt > > >> new file mode 100644 > > >> index 000000000000..20fca67a56c4 > > >> +++ b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt > > >> @@ -0,0 +1,13 @@ > > >> +Required properties: > > >> +- compatible: should be "microsoft,ftpm" > > >> +- linux,sml-base: 64-bit base address of the reserved memory allocated > > >> + for the firmware event log > > >> +- linux,sml-size: size of the memory allocated for the firmware event log > > > > > >Firmware is defining linux specific properties? What if I want to run > > >BSD? We should use 'reg' here instead. > > > > This is based on already existing code that defines these names, see > > tpm_read_log_of() in drivers/char/tpm/eventlog/of.c . > > BTW, that probably needs updating to handle endianness correctly. IIRC this legacy IBM code has broken endianness in the firmware.. All that stuff in read_log_of, and the related DT stuff, is historical IBM special case-ness and should not be copied into new things. Jason
On Wed, Apr 10, 2019 at 02:03:16PM -0300, Jason Gunthorpe wrote: >On Wed, Apr 10, 2019 at 12:01:37PM -0500, Rob Herring wrote: >> On Wed, Apr 10, 2019 at 11:19 AM Sasha Levin <sashal@kernel.org> wrote: >> > >> > On Tue, Apr 09, 2019 at 04:18:29PM -0500, Rob Herring wrote: >> > >On Tue, Apr 9, 2019 at 1:50 PM Sasha Levin <sashal@kernel.org> wrote: >> > >> >> > >> The parameters are similar to the ones used by IBM's vTPM and the >> > >> various I2C tpm drivers. >> > > >> > >Bindings describe h/w (or firmware interfaces in this case), not drivers. >> > > >> > >> >> > >> Signed-off-by: Sasha Levin <sashal@kernel.org> >> > >> .../bindings/security/tpm/tpm_ftpm_tee.txt | 13 +++++++++++++ >> > >> .../devicetree/bindings/vendor-prefixes.txt | 1 + >> > >> 2 files changed, 14 insertions(+) >> > >> create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt >> > >> >> > >> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt >> > >> new file mode 100644 >> > >> index 000000000000..20fca67a56c4 >> > >> +++ b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt >> > >> @@ -0,0 +1,13 @@ >> > >> +Required properties: >> > >> +- compatible: should be "microsoft,ftpm" >> > >> +- linux,sml-base: 64-bit base address of the reserved memory allocated >> > >> + for the firmware event log >> > >> +- linux,sml-size: size of the memory allocated for the firmware event log >> > > >> > >Firmware is defining linux specific properties? What if I want to run >> > >BSD? We should use 'reg' here instead. >> > >> > This is based on already existing code that defines these names, see >> > tpm_read_log_of() in drivers/char/tpm/eventlog/of.c . >> >> BTW, that probably needs updating to handle endianness correctly. > >IIRC this legacy IBM code has broken endianness in the firmware.. > >All that stuff in read_log_of, and the related DT stuff, is historical >IBM special case-ness and should not be copied into new things. The fTPM driver does not use it on it's own, so I guess I can just drop this patch then? -- Thanks, Sasha
On Wed, Apr 10, 2019 at 01:53:59PM -0400, Sasha Levin wrote: > On Wed, Apr 10, 2019 at 02:03:16PM -0300, Jason Gunthorpe wrote: > > On Wed, Apr 10, 2019 at 12:01:37PM -0500, Rob Herring wrote: > > > On Wed, Apr 10, 2019 at 11:19 AM Sasha Levin <sashal@kernel.org> wrote: > > > > > > > > On Tue, Apr 09, 2019 at 04:18:29PM -0500, Rob Herring wrote: > > > > >On Tue, Apr 9, 2019 at 1:50 PM Sasha Levin <sashal@kernel.org> wrote: > > > > >> > > > > >> The parameters are similar to the ones used by IBM's vTPM and the > > > > >> various I2C tpm drivers. > > > > > > > > > >Bindings describe h/w (or firmware interfaces in this case), not drivers. > > > > > > > > > >> > > > > >> Signed-off-by: Sasha Levin <sashal@kernel.org> > > > > >> .../bindings/security/tpm/tpm_ftpm_tee.txt | 13 +++++++++++++ > > > > >> .../devicetree/bindings/vendor-prefixes.txt | 1 + > > > > >> 2 files changed, 14 insertions(+) > > > > >> create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt > > > > >> > > > > >> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt > > > > >> new file mode 100644 > > > > >> index 000000000000..20fca67a56c4 > > > > >> +++ b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt > > > > >> @@ -0,0 +1,13 @@ > > > > >> +Required properties: > > > > >> +- compatible: should be "microsoft,ftpm" > > > > >> +- linux,sml-base: 64-bit base address of the reserved memory allocated > > > > >> + for the firmware event log > > > > >> +- linux,sml-size: size of the memory allocated for the firmware event log > > > > > > > > > >Firmware is defining linux specific properties? What if I want to run > > > > >BSD? We should use 'reg' here instead. > > > > > > > > This is based on already existing code that defines these names, see > > > > tpm_read_log_of() in drivers/char/tpm/eventlog/of.c . > > > > > > BTW, that probably needs updating to handle endianness correctly. > > > > IIRC this legacy IBM code has broken endianness in the firmware.. > > > > All that stuff in read_log_of, and the related DT stuff, is historical > > IBM special case-ness and should not be copied into new things. > > The fTPM driver does not use it on it's own, so I guess I can just drop > this patch then? If your ARM system boots via EFI then it should pass the firmware event log through EFI mechanisms, IIRC. Otherwise maybe you could make a case for using this, but only if the old IBM stuff is perfectly emulated, bugs and all. Do you even have an event log? Jason
> -----Original Message----- > From: Jason Gunthorpe <jgg@ziepe.ca> > Sent: Wednesday, April 10, 2019 10:57 AM > To: Sasha Levin <sashal@kernel.org> > Cc: Rob Herring <robh+dt@kernel.org>; Peter Huewe <peterhuewe@gmx.de>; > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; Mark Rutland > <mark.rutland@arm.com>; Jonathan Corbet <corbet@lwn.net>; Arnd Bergmann > <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux- > kernel@vger.kernel.org; Linux Doc Mailing List <linux-doc@vger.kernel.org>; > linux-integrity@vger.kernel.org; Microsoft Linux Kernel List <linux- > kernel@microsoft.com>; Thirupathaiah Annapureddy <thiruan@microsoft.com>; > Bryan Kelly (CSI) <bryankel@microsoft.com> > Subject: Re: [PATCH v2 1/3] ftpm: dt-binding: add dts documentation for > fTPM driver > > On Wed, Apr 10, 2019 at 01:53:59PM -0400, Sasha Levin wrote: > > On Wed, Apr 10, 2019 at 02:03:16PM -0300, Jason Gunthorpe wrote: > > > On Wed, Apr 10, 2019 at 12:01:37PM -0500, Rob Herring wrote: > > > > On Wed, Apr 10, 2019 at 11:19 AM Sasha Levin <sashal@kernel.org> > wrote: > > > > > > > > > > On Tue, Apr 09, 2019 at 04:18:29PM -0500, Rob Herring wrote: > > > > > >On Tue, Apr 9, 2019 at 1:50 PM Sasha Levin <sashal@kernel.org> > wrote: > > > > > >> > > > > > >> The parameters are similar to the ones used by IBM's vTPM and > the > > > > > >> various I2C tpm drivers. > > > > > > > > > > > >Bindings describe h/w (or firmware interfaces in this case), not > drivers. > > > > > > > > > > > >> > > > > > >> Signed-off-by: Sasha Levin <sashal@kernel.org> > > > > > >> .../bindings/security/tpm/tpm_ftpm_tee.txt | 13 > +++++++++++++ > > > > > >> .../devicetree/bindings/vendor-prefixes.txt | 1 + > > > > > >> 2 files changed, 14 insertions(+) > > > > > >> create mode 100644 > Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt > > > > > >> > > > > > >> diff --git > a/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt > b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt > > > > > >> new file mode 100644 > > > > > >> index 000000000000..20fca67a56c4 > > > > > >> +++ > b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt > > > > > >> @@ -0,0 +1,13 @@ > > > > > >> +Required properties: > > > > > >> +- compatible: should be "microsoft,ftpm" > > > > > >> +- linux,sml-base: 64-bit base address of the reserved memory > allocated > > > > > >> + for the firmware event log > > > > > >> +- linux,sml-size: size of the memory allocated for the firmware > event log > > > > > > > > > > > >Firmware is defining linux specific properties? What if I want to > run > > > > > >BSD? We should use 'reg' here instead. > > > > > > > > > > This is based on already existing code that defines these names, > see > > > > > tpm_read_log_of() in drivers/char/tpm/eventlog/of.c . > > > > > > > > BTW, that probably needs updating to handle endianness correctly. > > > > > > IIRC this legacy IBM code has broken endianness in the firmware.. > > > > > > All that stuff in read_log_of, and the related DT stuff, is historical > > > IBM special case-ness and should not be copied into new things. > > > > The fTPM driver does not use it on it's own, so I guess I can just drop > > this patch then? > > If your ARM system boots via EFI then it should pass the firmware > event log through EFI mechanisms, IIRC. > > Otherwise maybe you could make a case for using this, but only if the > old IBM stuff is perfectly emulated, bugs and all. > > Do you even have an event log? > Our ARM system is booting using U-boot and Device Tree. So the ACPI/EFI table mechanism to pass binary boot measurements won't be applicable. We have event logs working and we are able to successfully read them using /sys/kernel/security/tpm0/binary_bios_measurements and parse them --Thiru > Jason
diff --git a/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt new file mode 100644 index 000000000000..20fca67a56c4 --- /dev/null +++ b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt @@ -0,0 +1,13 @@ +Required properties: +- compatible: should be "microsoft,ftpm" +- linux,sml-base: 64-bit base address of the reserved memory allocated + for the firmware event log +- linux,sml-size: size of the memory allocated for the firmware event log + +Example: + +tpm@0 { + compatible = "microsoft,ftpm"; + linux,sml-base = <0x0 0xC0000000>; + linux,sml-size = <0x10000>; +}; diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt index 8162b0eb4b50..902798bcb9a5 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.txt +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt @@ -249,6 +249,7 @@ micrel Micrel Inc. microchip Microchip Technology Inc. microcrystal Micro Crystal AG micron Micron Technology Inc. +microsoft Microsoft Corporation mikroe MikroElektronika d.o.o. minix MINIX Technology Ltd. miramems MiraMEMS Sensing Technology Co., Ltd.
The parameters are similar to the ones used by IBM's vTPM and the various I2C tpm drivers. Signed-off-by: Sasha Levin <sashal@kernel.org> --- .../bindings/security/tpm/tpm_ftpm_tee.txt | 13 +++++++++++++ .../devicetree/bindings/vendor-prefixes.txt | 1 + 2 files changed, 14 insertions(+) create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt