| Message ID | 20230615123703.4028156-1-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] powerpc/tpm: Create linux,sml-base/size as big endian | expand |
On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote: > There's code in prom_instantiate_sml() to do a "SML handover" (Stored > Measurement Log) from OF to Linux, before Linux shuts down Open > Firmware. > > This involves creating a buffer to hold the SML, and creating two device > tree properties to record its base address and size. The kernel then > later reads those properties from the device tree to find the SML. > > When the code was initially added in commit 4a727429abec ("PPC64: Add > support for instantiating SML from Open Firmware") the powerpc kernel > was always built big endian, so the properties were created big endian > by default. > > However since then little endian support was added to powerpc, and now > the code lacks conversions to big endian when creating the properties. > > This means on little endian kernels the device tree properties are > little endian, which is contrary to the device tree spec, and in > contrast to all other device tree properties. > > To cope with that a workaround was added in tpm_read_log_of() to skip > the endian conversion if the properties were created via the SML > handover. > > A better solution is to encode the properties as big endian as they > should be, and remove the workaround. > > Typically changing the encoding of a property like this would present > problems for kexec. However the SML is not propagated across kexec, so > changing the encoding of the properties is a non-issue. > > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log") > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > --- > arch/powerpc/kernel/prom_init.c | 8 ++++++-- > drivers/char/tpm/eventlog/of.c | 23 ++++------------------- > 2 files changed, 10 insertions(+), 21 deletions(-) Split into two patches (producer and consumer). BR, Jarkko > > v2: Add Stefan's reviewed-by. > > diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c > index d464ba412084..72fe306b6820 100644 > --- a/arch/powerpc/kernel/prom_init.c > +++ b/arch/powerpc/kernel/prom_init.c > @@ -1900,6 +1900,7 @@ static void __init prom_instantiate_sml(void) > u32 entry = 0, size = 0, succ = 0; > u64 base; > __be32 val; > + __be64 val64; > > prom_debug("prom_instantiate_sml: start...\n"); > > @@ -1956,10 +1957,13 @@ static void __init prom_instantiate_sml(void) > > reserve_mem(base, size); > > + val64 = cpu_to_be64(base); > prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base", > - &base, sizeof(base)); > + &val64, sizeof(val64)); > + > + val = cpu_to_be32(size); > prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size", > - &size, sizeof(size)); > + &val, sizeof(val)); > > prom_debug("sml base = 0x%llx\n", base); > prom_debug("sml size = 0x%x\n", size); > diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c > index 930fe43d5daf..0bc0cb6333c6 100644 > --- a/drivers/char/tpm/eventlog/of.c > +++ b/drivers/char/tpm/eventlog/of.c > @@ -51,8 +51,8 @@ static int tpm_read_log_memory_region(struct tpm_chip *chip) > int tpm_read_log_of(struct tpm_chip *chip) > { > struct device_node *np; > - const u32 *sizep; > - const u64 *basep; > + const __be32 *sizep; > + const __be64 *basep; > struct tpm_bios_log *log; > u32 size; > u64 base; > @@ -73,23 +73,8 @@ int tpm_read_log_of(struct tpm_chip *chip) > if (sizep == NULL || basep == NULL) > return -EIO; > > - /* > - * For both vtpm/tpm, firmware has log addr and log size in big > - * endian format. But in case of vtpm, there is a method called > - * sml-handover which is run during kernel init even before > - * device tree is setup. This sml-handover function takes care > - * of endianness and writes to sml-base and sml-size in little > - * endian format. For this reason, vtpm doesn't need conversion > - * but physical tpm needs the conversion. > - */ > - if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 && > - of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) { > - size = be32_to_cpup((__force __be32 *)sizep); > - base = be64_to_cpup((__force __be64 *)basep); > - } else { > - size = *sizep; > - base = *basep; > - } > + size = be32_to_cpup(sizep); > + base = be64_to_cpup(basep); > > if (size == 0) { > dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
On 7/10/23 17:23, Jarkko Sakkinen wrote: > On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote: >> There's code in prom_instantiate_sml() to do a "SML handover" (Stored >> Measurement Log) from OF to Linux, before Linux shuts down Open >> Firmware. >> >> This involves creating a buffer to hold the SML, and creating two device >> tree properties to record its base address and size. The kernel then >> later reads those properties from the device tree to find the SML. >> >> When the code was initially added in commit 4a727429abec ("PPC64: Add >> support for instantiating SML from Open Firmware") the powerpc kernel >> was always built big endian, so the properties were created big endian >> by default. >> >> However since then little endian support was added to powerpc, and now >> the code lacks conversions to big endian when creating the properties. >> >> This means on little endian kernels the device tree properties are >> little endian, which is contrary to the device tree spec, and in >> contrast to all other device tree properties. >> >> To cope with that a workaround was added in tpm_read_log_of() to skip >> the endian conversion if the properties were created via the SML >> handover. >> >> A better solution is to encode the properties as big endian as they >> should be, and remove the workaround. >> >> Typically changing the encoding of a property like this would present >> problems for kexec. However the SML is not propagated across kexec, so >> changing the encoding of the properties is a non-issue. >> >> Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log") >> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> >> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> >> --- >> arch/powerpc/kernel/prom_init.c | 8 ++++++-- >> drivers/char/tpm/eventlog/of.c | 23 ++++------------------- >> 2 files changed, 10 insertions(+), 21 deletions(-) > > Split into two patches (producer and consumer). I think this wouldn't be right since it would break the system when only one patch is applied since it would be reading the fields in the wrong endianess. Stefan > > BR, Jarkko > >> >> v2: Add Stefan's reviewed-by. >> >> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c >> index d464ba412084..72fe306b6820 100644 >> --- a/arch/powerpc/kernel/prom_init.c >> +++ b/arch/powerpc/kernel/prom_init.c >> @@ -1900,6 +1900,7 @@ static void __init prom_instantiate_sml(void) >> u32 entry = 0, size = 0, succ = 0; >> u64 base; >> __be32 val; >> + __be64 val64; >> >> prom_debug("prom_instantiate_sml: start...\n"); >> >> @@ -1956,10 +1957,13 @@ static void __init prom_instantiate_sml(void) >> >> reserve_mem(base, size); >> >> + val64 = cpu_to_be64(base); >> prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base", >> - &base, sizeof(base)); >> + &val64, sizeof(val64)); >> + >> + val = cpu_to_be32(size); >> prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size", >> - &size, sizeof(size)); >> + &val, sizeof(val)); >> >> prom_debug("sml base = 0x%llx\n", base); >> prom_debug("sml size = 0x%x\n", size); >> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c >> index 930fe43d5daf..0bc0cb6333c6 100644 >> --- a/drivers/char/tpm/eventlog/of.c >> +++ b/drivers/char/tpm/eventlog/of.c >> @@ -51,8 +51,8 @@ static int tpm_read_log_memory_region(struct tpm_chip *chip) >> int tpm_read_log_of(struct tpm_chip *chip) >> { >> struct device_node *np; >> - const u32 *sizep; >> - const u64 *basep; >> + const __be32 *sizep; >> + const __be64 *basep; >> struct tpm_bios_log *log; >> u32 size; >> u64 base; >> @@ -73,23 +73,8 @@ int tpm_read_log_of(struct tpm_chip *chip) >> if (sizep == NULL || basep == NULL) >> return -EIO; >> >> - /* >> - * For both vtpm/tpm, firmware has log addr and log size in big >> - * endian format. But in case of vtpm, there is a method called >> - * sml-handover which is run during kernel init even before >> - * device tree is setup. This sml-handover function takes care >> - * of endianness and writes to sml-base and sml-size in little >> - * endian format. For this reason, vtpm doesn't need conversion >> - * but physical tpm needs the conversion. >> - */ >> - if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 && >> - of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) { >> - size = be32_to_cpup((__force __be32 *)sizep); >> - base = be64_to_cpup((__force __be64 *)basep); >> - } else { >> - size = *sizep; >> - base = *basep; >> - } >> + size = be32_to_cpup(sizep); >> + base = be64_to_cpup(basep); >> >> if (size == 0) { >> dev_warn(&chip->dev, "%s: Event log area empty\n", __func__); >
On Tue, 2023-07-11 at 08:47 -0400, Stefan Berger wrote: > > On 7/10/23 17:23, Jarkko Sakkinen wrote: > > On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote: > > > There's code in prom_instantiate_sml() to do a "SML handover" (Stored > > > Measurement Log) from OF to Linux, before Linux shuts down Open > > > Firmware. > > > > > > This involves creating a buffer to hold the SML, and creating two device > > > tree properties to record its base address and size. The kernel then > > > later reads those properties from the device tree to find the SML. > > > > > > When the code was initially added in commit 4a727429abec ("PPC64: Add > > > support for instantiating SML from Open Firmware") the powerpc kernel > > > was always built big endian, so the properties were created big endian > > > by default. > > > > > > However since then little endian support was added to powerpc, and now > > > the code lacks conversions to big endian when creating the properties. > > > > > > This means on little endian kernels the device tree properties are > > > little endian, which is contrary to the device tree spec, and in > > > contrast to all other device tree properties. > > > > > > To cope with that a workaround was added in tpm_read_log_of() to skip > > > the endian conversion if the properties were created via the SML > > > handover. > > > > > > A better solution is to encode the properties as big endian as they > > > should be, and remove the workaround. > > > > > > Typically changing the encoding of a property like this would present > > > problems for kexec. However the SML is not propagated across kexec, so > > > changing the encoding of the properties is a non-issue. > > > > > > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log") > > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > > > --- > > > arch/powerpc/kernel/prom_init.c | 8 ++++++-- > > > drivers/char/tpm/eventlog/of.c | 23 ++++------------------- > > > 2 files changed, 10 insertions(+), 21 deletions(-) > > > > Split into two patches (producer and consumer). > > I think this wouldn't be right since it would break the system when only one patch is applied since it would be reading the fields in the wrong endianess. I think it would help if the commit message would better explain what is going on. It is somewhat difficult to decipher, if you don't have deep knowledge of the powerpc architecture. BR, Jarkko
Stefan Berger <stefanb@linux.ibm.com> writes: > On 7/10/23 17:23, Jarkko Sakkinen wrote: >> On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote: >>> There's code in prom_instantiate_sml() to do a "SML handover" (Stored >>> Measurement Log) from OF to Linux, before Linux shuts down Open >>> Firmware. >>> >>> This involves creating a buffer to hold the SML, and creating two device >>> tree properties to record its base address and size. The kernel then >>> later reads those properties from the device tree to find the SML. >>> >>> When the code was initially added in commit 4a727429abec ("PPC64: Add >>> support for instantiating SML from Open Firmware") the powerpc kernel >>> was always built big endian, so the properties were created big endian >>> by default. >>> >>> However since then little endian support was added to powerpc, and now >>> the code lacks conversions to big endian when creating the properties. >>> >>> This means on little endian kernels the device tree properties are >>> little endian, which is contrary to the device tree spec, and in >>> contrast to all other device tree properties. >>> >>> To cope with that a workaround was added in tpm_read_log_of() to skip >>> the endian conversion if the properties were created via the SML >>> handover. >>> >>> A better solution is to encode the properties as big endian as they >>> should be, and remove the workaround. >>> >>> Typically changing the encoding of a property like this would present >>> problems for kexec. However the SML is not propagated across kexec, so >>> changing the encoding of the properties is a non-issue. >>> >>> Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log") >>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> >>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> >>> --- >>> arch/powerpc/kernel/prom_init.c | 8 ++++++-- >>> drivers/char/tpm/eventlog/of.c | 23 ++++------------------- >>> 2 files changed, 10 insertions(+), 21 deletions(-) >> >> Split into two patches (producer and consumer). > > I think this wouldn't be right since it would break the system when > only one patch is applied since it would be reading the fields in the > wrong endianess. Yes that's right. Typically we would split the patch by subsystem, but in this case it will just create a known-broken point in the git history, which helps no one. cheers
Jarkko Sakkinen <jarkko@kernel.org> writes: > On Tue, 2023-07-11 at 08:47 -0400, Stefan Berger wrote: >> On 7/10/23 17:23, Jarkko Sakkinen wrote: >> > On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote: >> > > There's code in prom_instantiate_sml() to do a "SML handover" (Stored >> > > Measurement Log) from OF to Linux, before Linux shuts down Open >> > > Firmware. >> > > >> > > This involves creating a buffer to hold the SML, and creating two device >> > > tree properties to record its base address and size. The kernel then >> > > later reads those properties from the device tree to find the SML. >> > > >> > > When the code was initially added in commit 4a727429abec ("PPC64: Add >> > > support for instantiating SML from Open Firmware") the powerpc kernel >> > > was always built big endian, so the properties were created big endian >> > > by default. >> > > >> > > However since then little endian support was added to powerpc, and now >> > > the code lacks conversions to big endian when creating the properties. >> > > >> > > This means on little endian kernels the device tree properties are >> > > little endian, which is contrary to the device tree spec, and in >> > > contrast to all other device tree properties. >> > > >> > > To cope with that a workaround was added in tpm_read_log_of() to skip >> > > the endian conversion if the properties were created via the SML >> > > handover. >> > > >> > > A better solution is to encode the properties as big endian as they >> > > should be, and remove the workaround. >> > > >> > > Typically changing the encoding of a property like this would present >> > > problems for kexec. However the SML is not propagated across kexec, so >> > > changing the encoding of the properties is a non-issue. >> > > >> > > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log") >> > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> >> > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> >> > > --- >> > > arch/powerpc/kernel/prom_init.c | 8 ++++++-- >> > > drivers/char/tpm/eventlog/of.c | 23 ++++------------------- >> > > 2 files changed, 10 insertions(+), 21 deletions(-) >> > >> > Split into two patches (producer and consumer). >> >> I think this wouldn't be right since it would break the system when only one patch is applied since it would be reading the fields in the wrong endianess. > > I think it would help if the commit message would better explain > what is going on. It is somewhat difficult to decipher, if you > don't have deep knowledge of the powerpc architecture. I mean, it's already 8 paragraphs ¯\_(ツ)_/¯ But I'm happy to expand it. I just don't really know what extra detail is needed to make it clearer. cheers
On Wed Jul 12, 2023 at 12:39 PM UTC, Michael Ellerman wrote: > Jarkko Sakkinen <jarkko@kernel.org> writes: > > On Tue, 2023-07-11 at 08:47 -0400, Stefan Berger wrote: > >> On 7/10/23 17:23, Jarkko Sakkinen wrote: > >> > On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote: > >> > > There's code in prom_instantiate_sml() to do a "SML handover" (Stored > >> > > Measurement Log) from OF to Linux, before Linux shuts down Open > >> > > Firmware. > >> > > > >> > > This involves creating a buffer to hold the SML, and creating two device > >> > > tree properties to record its base address and size. The kernel then > >> > > later reads those properties from the device tree to find the SML. > >> > > > >> > > When the code was initially added in commit 4a727429abec ("PPC64: Add > >> > > support for instantiating SML from Open Firmware") the powerpc kernel > >> > > was always built big endian, so the properties were created big endian > >> > > by default. > >> > > > >> > > However since then little endian support was added to powerpc, and now > >> > > the code lacks conversions to big endian when creating the properties. > >> > > > >> > > This means on little endian kernels the device tree properties are > >> > > little endian, which is contrary to the device tree spec, and in > >> > > contrast to all other device tree properties. > >> > > > >> > > To cope with that a workaround was added in tpm_read_log_of() to skip > >> > > the endian conversion if the properties were created via the SML > >> > > handover. > >> > > > >> > > A better solution is to encode the properties as big endian as they > >> > > should be, and remove the workaround. > >> > > > >> > > Typically changing the encoding of a property like this would present > >> > > problems for kexec. However the SML is not propagated across kexec, so > >> > > changing the encoding of the properties is a non-issue. > >> > > > >> > > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log") > >> > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > >> > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > >> > > --- > >> > > arch/powerpc/kernel/prom_init.c | 8 ++++++-- > >> > > drivers/char/tpm/eventlog/of.c | 23 ++++------------------- > >> > > 2 files changed, 10 insertions(+), 21 deletions(-) > >> > > >> > Split into two patches (producer and consumer). > >> > >> I think this wouldn't be right since it would break the system when only one patch is applied since it would be reading the fields in the wrong endianess. > > > > I think it would help if the commit message would better explain > > what is going on. It is somewhat difficult to decipher, if you > > don't have deep knowledge of the powerpc architecture. > > I mean, it's already 8 paragraphs ¯\_(ツ)_/¯ > > But I'm happy to expand it. I just don't really know what extra detail > is needed to make it clearer. Adding more text is not the right way to clarify things. I'd start by explaining shortly SML and then move to the handover. It can't be that hard, right? Just adding new paragraphs would probably just make it even more confusing. BR, Jarkko
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c index d464ba412084..72fe306b6820 100644 --- a/arch/powerpc/kernel/prom_init.c +++ b/arch/powerpc/kernel/prom_init.c @@ -1900,6 +1900,7 @@ static void __init prom_instantiate_sml(void) u32 entry = 0, size = 0, succ = 0; u64 base; __be32 val; + __be64 val64; prom_debug("prom_instantiate_sml: start...\n"); @@ -1956,10 +1957,13 @@ static void __init prom_instantiate_sml(void) reserve_mem(base, size); + val64 = cpu_to_be64(base); prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base", - &base, sizeof(base)); + &val64, sizeof(val64)); + + val = cpu_to_be32(size); prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size", - &size, sizeof(size)); + &val, sizeof(val)); prom_debug("sml base = 0x%llx\n", base); prom_debug("sml size = 0x%x\n", size); diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c index 930fe43d5daf..0bc0cb6333c6 100644 --- a/drivers/char/tpm/eventlog/of.c +++ b/drivers/char/tpm/eventlog/of.c @@ -51,8 +51,8 @@ static int tpm_read_log_memory_region(struct tpm_chip *chip) int tpm_read_log_of(struct tpm_chip *chip) { struct device_node *np; - const u32 *sizep; - const u64 *basep; + const __be32 *sizep; + const __be64 *basep; struct tpm_bios_log *log; u32 size; u64 base; @@ -73,23 +73,8 @@ int tpm_read_log_of(struct tpm_chip *chip) if (sizep == NULL || basep == NULL) return -EIO; - /* - * For both vtpm/tpm, firmware has log addr and log size in big - * endian format. But in case of vtpm, there is a method called - * sml-handover which is run during kernel init even before - * device tree is setup. This sml-handover function takes care - * of endianness and writes to sml-base and sml-size in little - * endian format. For this reason, vtpm doesn't need conversion - * but physical tpm needs the conversion. - */ - if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 && - of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) { - size = be32_to_cpup((__force __be32 *)sizep); - base = be64_to_cpup((__force __be64 *)basep); - } else { - size = *sizep; - base = *basep; - } + size = be32_to_cpup(sizep); + base = be64_to_cpup(basep); if (size == 0) { dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
There's code in prom_instantiate_sml() to do a "SML handover" (Stored Measurement Log) from OF to Linux, before Linux shuts down Open Firmware. This involves creating a buffer to hold the SML, and creating two device tree properties to record its base address and size. The kernel then later reads those properties from the device tree to find the SML. When the code was initially added in commit 4a727429abec ("PPC64: Add support for instantiating SML from Open Firmware") the powerpc kernel was always built big endian, so the properties were created big endian by default. However since then little endian support was added to powerpc, and now the code lacks conversions to big endian when creating the properties. This means on little endian kernels the device tree properties are little endian, which is contrary to the device tree spec, and in contrast to all other device tree properties. To cope with that a workaround was added in tpm_read_log_of() to skip the endian conversion if the properties were created via the SML handover. A better solution is to encode the properties as big endian as they should be, and remove the workaround. Typically changing the encoding of a property like this would present problems for kexec. However the SML is not propagated across kexec, so changing the encoding of the properties is a non-issue. Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log") Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> --- arch/powerpc/kernel/prom_init.c | 8 ++++++-- drivers/char/tpm/eventlog/of.c | 23 ++++------------------- 2 files changed, 10 insertions(+), 21 deletions(-) v2: Add Stefan's reviewed-by.