Re: [PATCH] tpm_crb: fix fTPM on AMD Zen+ CPUs

[Jarkko Sakkinen]

On Wed, 2019-09-04 at 22:03 +0300, ivan.lazeev@xxxxxxxxx wrote:
> From: Ivan Lazeev <ivan.lazeev@xxxxxxxxx>
> 
> Bug link: https://bugzilla.kernel.org/show_bug.cgi?id=195657
> 
> cmd/rsp buffers are expected to be in the same ACPI region.
> For Zen+ CPUs BIOS's might report two different regions, some of
> them also report region sizes inconsistent with values from TPM
> registers.
> 
> Work around the issue by storing ACPI regions declared for the
> device in a list, then using it to find entry corresponding
> to each buffer. Use information from the entry to map each resource
> at most once and make buffer size consistent with the length of the
> region.
> 
> Signed-off-by: Ivan Lazeev <ivan.lazeev@xxxxxxxxx>

Can you add the relevant pieces of /proc/iomem output to the commit
message so we can see the memory configuration? I don't have the
hardware available where this kind of situation occurs.

> ---
> 
> Tested on ASRock x470 ITX motherboard with Ryzen 2600X CPU.
> However, I don't have any other hardware to test the changes on and no
> expertise to be sure that other TPMs won't break as a result.

You can still ask yourself that if the hardware describes the memory
with only one region do the code flows reduce mostly to do the same as
before this patch. It is not the same as testing with such hardware but
it is still a good mental exercise.

To summarize, I do get "not having hardware" part but completely
disqualify "not having expertise" part as the expertise needed here has
nothing specific to do with TPMs :-)

> 
>  drivers/char/tpm/tpm_crb.c | 137 +++++++++++++++++++++++++++----------
>  1 file changed, 101 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index e59f1f91d7f3..0bcfe52db5d6 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -91,7 +91,6 @@ enum crb_status {
>  struct crb_priv {
>  	u32 sm;
>  	const char *hid;
> -	void __iomem *iobase;
>  	struct crb_regs_head __iomem *regs_h;
>  	struct crb_regs_tail __iomem *regs_t;
>  	u8 __iomem *cmd;
> @@ -108,6 +107,12 @@ struct tpm2_crb_smc {
>  	u32 smc_func_id;
>  };
>  
> +struct crb_resource {
> +	struct resource io_res;
> +	void __iomem *iobase;
> +	struct list_head node;
> +};

Please rename 'node' as 'list' for the sake of coherency with the rest
of the kernel and io_res as iores for the sake of coherency with the
other fields.

I think it would be cleaner to do the following:

1. Start with empty resource list.
2. Everytime crb_map_res() is called it does check the existing
   resources and does a ACPI walk on need basis at most adding
   one new entry.

In other words, crb_map_res() gets the list and appends one entry when
necessary.

Overhead of doing multiple walks is irrelevant here. The simplicity and
clarity win.

/Jarkko