On Mon, Aug 12, 2019 at 10:10:03AM -0300, Jason Gunthorpe wrote: > On Sun, Aug 11, 2019 at 09:52:18PM +0300, ivan.lazeev@xxxxxxxxx wrote: > > From: Vanya Lazeev <ivan.lazeev@xxxxxxxxx> > > > > The patch is an attempt to make fTPM on AMD Zen CPUs work. > > Bug link: https://bugzilla.kernel.org/show_bug.cgi?id=195657 > > > > The problem seems to be that tpm_crb driver doesn't expect tpm command > > and response memory regions to belong to different ACPI resources. > > > > Tested on Asrock 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. > > Hopefully, the patch will be useful. > > > > Changes from v1: > > - use list_for_each_safe > > > > Signed-off-by: Vanya Lazeev <ivan.lazeev@xxxxxxxxx> > > drivers/char/tpm/tpm_crb.c | 146 ++++++++++++++++++++++++++++--------- > > 1 file changed, 110 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > > index e59f1f91d..b0e797464 100644 > > +++ 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,13 @@ struct tpm2_crb_smc { > > u32 smc_func_id; > > }; > > > > +struct crb_resource { > > + struct resource io_res; > > + void __iomem *iobase; > > + > > + struct list_head link; > > +}; > > + > > static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value, > > unsigned long timeout) > > { > > @@ -432,23 +438,69 @@ static const struct tpm_class_ops tpm_crb = { > > .req_complete_val = CRB_DRV_STS_COMPLETE, > > }; > > > > +static void crb_free_resource_list(struct list_head *resources) > > +{ > > + struct list_head *position, *tmp; > > + > > + list_for_each_safe(position, tmp, resources) > > + kfree(list_entry(position, struct crb_resource, link)); > > +} > > + > > +/** > > + * Checks if resource @io_res contains range with the specified @start and @size > > + * completely or, when @strict is false, at least it's beginning. > > + * Non-strict match is needed to work around broken BIOSes that return > > + * inconsistent values from ACPI regions vs registers. > > + */ > > +static inline bool crb_resource_contains(const struct resource *io_res, > > + u64 start, u32 size, bool strict) > > +{ > > + return start >= io_res->start && > > + (start + size - 1 <= io_res->end || > > + (!strict && start <= io_res->end)); > > +} > > + > > +static struct crb_resource *crb_containing_resource( > > + const struct list_head *resources, > > + u64 start, u32 size, bool strict) > > +{ > > + struct list_head *pos; > > + > > + list_for_each(pos, resources) { > > + struct crb_resource *cres; > > + > > + cres = list_entry(pos, struct crb_resource, link); > > + if (crb_resource_contains(&cres->io_res, start, size, strict)) > > + return cres; > > + } > > + > > + return NULL; > > +} > > This flow seems very strange, why isn't this part of crb_map_res? > fTPM on Zen+ not only needs multiple mappings, it can also return inconsistent with ACPI values for range sizes (as for me and mikajhe from the bug thread), so results of crb_containing_resource are also used to fix the inconsistencies with crb_fixup_cmd_size. > > static int crb_check_resource(struct acpi_resource *ares, void *data) > > { > > - struct resource *io_res = data; > > + struct list_head *list = data; > > + struct crb_resource *cres; > > struct resource_win win; > > struct resource *res = &(win.res); > > > > if (acpi_dev_resource_memory(ares, res) || > > acpi_dev_resource_address_space(ares, &win)) { > > - *io_res = *res; > > - io_res->name = NULL; > > + cres = kzalloc(sizeof(*cres), GFP_KERNEL); > > + if (!cres) > > + return -ENOMEM; > > + > > + cres->io_res = *res; > > + cres->io_res.name = NULL; > > + > > + list_add_tail(&cres->link, list); > > And why is this allocating memory inside the acpi table walker? It > seems to me like the memory should be allocated once the mapping is > made. > Yes, this looks bad. Letting the walker build the list and then using it is, probably, a better idea. > Maybe all the mappings should be created from the ACPI ranges right > away? > I don't know if it's a good idea to just map them all instead of doing so only for relevant ones. Maybe it is safe, here I need an advice from a more knowledgeable person. > > @@ -460,10 +512,16 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv, > > if (start != new_res.start) > > return (void __iomem *) ERR_PTR(-EINVAL); > > > > - if (!resource_contains(io_res, &new_res)) > > + if (!cres) > > return devm_ioremap_resource(dev, &new_res); > > > > - return priv->iobase + (new_res.start - io_res->start); > > + if (!cres->iobase) { > > + cres->iobase = devm_ioremap_resource(dev, &cres->io_res); > > + if (IS_ERR(cres->iobase)) > > + return cres->iobase; > > + } > > It sounds likethe real bug is that the crb_map_res only considers a > single active mapping, while these AMD chips need more than one? > Yes, this seems to be the issue.
