From: Ivan Lazeev <[email protected]>
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.
Memory configuration on ASRock x470 ITX:
db0a0000-dc59efff : Reserved
dc57e000-dc57efff : MSFT0101:00
dc582000-dc582fff : MSFT0101:00
Work around the issue by storing ACPI regions declared for the
device in struct crb_resources. It contains fixed arrays of
copies of ACPI resourses (iores field) and pointers
to a possibly allocated with devm_ioremap_resource memory region
(iobase field), along with number of entries (num field).
This data was previously held for a single resource
in struct crb_priv (iobase field) and local variable io_res in
crb_map_io function. ACPI resources array is used to find index of
corresponding region for each buffer with crb_containing_res_idx,
make the buffer size consistent with region's length and map it at
most once, storing pointer to the allocated resource in iobase
array at the same index.
Signed-off-by: Ivan Lazeev <[email protected]>
---
Changes in v5:
- prefix new symbols with tpm_crb_
- get rid of dynamic allocation in ACPI walker
drivers/char/tpm/tpm_crb.c | 129 +++++++++++++++++++++++++++----------
1 file changed, 96 insertions(+), 33 deletions(-)
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index e59f1f91d7f3..3c2485c71260 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -22,6 +22,7 @@
#include "tpm.h"
#define ACPI_SIG_TPM2 "TPM2"
+#define TPM_CRB_MAX_RESOURCES 3
static const guid_t crb_acpi_start_guid =
GUID_INIT(0x6BBF6CAB, 0x5463, 0x4714,
@@ -91,7 +92,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 +108,12 @@ struct tpm2_crb_smc {
u32 smc_func_id;
};
+struct tpm_crb_resources {
+ struct resource iores[TPM_CRB_MAX_RESOURCES];
+ void __iomem *iobase[TPM_CRB_MAX_RESOURCES];
+ int num;
+};
+
static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
unsigned long timeout)
{
@@ -432,38 +438,75 @@ static const struct tpm_class_ops tpm_crb = {
.req_complete_val = CRB_DRV_STS_COMPLETE,
};
+static bool tpm_crb_resource_contains(struct resource *iores,
+ u64 address)
+{
+ return address >= iores->start && address <= iores->end;
+}
+
+static int tpm_crb_containing_res_idx(struct tpm_crb_resources *resources,
+ u64 address)
+{
+ int res_idx;
+
+ for (res_idx = 0; res_idx < resources->num; ++res_idx) {
+ if (tpm_crb_resource_contains(&resources->iores[res_idx],
+ address))
+ return res_idx;
+ }
+
+ return -1;
+}
+
static int crb_check_resource(struct acpi_resource *ares, void *data)
{
- struct resource *io_res = data;
+ struct tpm_crb_resources *resources = data;
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;
+ if (resources->num < TPM_CRB_MAX_RESOURCES) {
+ resources->iores[resources->num] = *res;
+ resources->iores[resources->num].name = NULL;
+ }
+ resources->num += 1;
}
return 1;
}
-static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
- struct resource *io_res, u64 start, u32 size)
+static void __iomem *crb_map_res(struct device *dev,
+ struct tpm_crb_resources *resources,
+ int res_idx,
+ u64 start, u32 size)
{
struct resource new_res = {
.start = start,
.end = start + size - 1,
.flags = IORESOURCE_MEM,
};
+ struct resource *iores;
+ void __iomem *iobase;
/* Detect a 64 bit address on a 32 bit system */
if (start != new_res.start)
return (void __iomem *) ERR_PTR(-EINVAL);
- if (!resource_contains(io_res, &new_res))
+ if (res_idx < 0)
return devm_ioremap_resource(dev, &new_res);
- return priv->iobase + (new_res.start - io_res->start);
+ iores = &resources->iores[res_idx];
+ iobase = resources->iobase[res_idx];
+ if (!iobase) {
+ iobase = devm_ioremap_resource(dev, iores);
+ if (IS_ERR(iobase))
+ return iobase;
+
+ resources->iobase[res_idx] = iobase;
+ }
+
+ return iobase + (new_res.start - iores->start);
}
/*
@@ -490,9 +533,10 @@ static u64 crb_fixup_cmd_size(struct device *dev, struct resource *io_res,
static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
struct acpi_table_tpm2 *buf)
{
- struct list_head resources;
- struct resource io_res;
+ struct list_head acpi_resources;
struct device *dev = &device->dev;
+ struct tpm_crb_resources resources;
+ int res_idx;
u32 pa_high, pa_low;
u64 cmd_pa;
u32 cmd_size;
@@ -501,21 +545,36 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
u32 rsp_size;
int ret;
- INIT_LIST_HEAD(&resources);
- ret = acpi_dev_get_resources(device, &resources, crb_check_resource,
- &io_res);
+ INIT_LIST_HEAD(&acpi_resources);
+ memset(&resources, 0, sizeof(resources));
+ ret = acpi_dev_get_resources(device, &acpi_resources,
+ crb_check_resource, &resources);
if (ret < 0)
return ret;
- acpi_dev_free_resource_list(&resources);
+ acpi_dev_free_resource_list(&acpi_resources);
- if (resource_type(&io_res) != IORESOURCE_MEM) {
+ if (resources.num == 0) {
dev_err(dev, FW_BUG "TPM2 ACPI table does not define a memory resource\n");
return -EINVAL;
+ } else if (resources.num > TPM_CRB_MAX_RESOURCES) {
+ dev_warn(dev, FW_BUG "TPM2 ACPI table defines too many memory resources\n");
+ resources.num = TPM_CRB_MAX_RESOURCES;
}
- priv->iobase = devm_ioremap_resource(dev, &io_res);
- if (IS_ERR(priv->iobase))
- return PTR_ERR(priv->iobase);
+ res_idx = tpm_crb_containing_res_idx(&resources, buf->control_address);
+
+ if (res_idx >= 0 &&
+ !tpm_crb_resource_contains(&resources.iores[res_idx],
+ buf->control_address +
+ sizeof(struct crb_regs_tail) - 1))
+ res_idx = -1;
+
+ priv->regs_t = crb_map_res(dev, &resources, res_idx,
+ buf->control_address,
+ sizeof(struct crb_regs_tail));
+
+ if (IS_ERR(priv->regs_t))
+ return PTR_ERR(priv->regs_t);
/* The ACPI IO region starts at the head area and continues to include
* the control area, as one nice sane region except for some older
@@ -523,9 +582,10 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
*/
if ((priv->sm == ACPI_TPM2_COMMAND_BUFFER) ||
(priv->sm == ACPI_TPM2_MEMORY_MAPPED)) {
- if (buf->control_address == io_res.start +
+ if (res_idx >= 0 &&
+ buf->control_address == resources.iores[res_idx].start +
sizeof(*priv->regs_h))
- priv->regs_h = priv->iobase;
+ priv->regs_h = resources.iobase[res_idx];
else
dev_warn(dev, FW_BUG "Bad ACPI memory layout");
}
@@ -534,13 +594,6 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
if (ret)
return ret;
- priv->regs_t = crb_map_res(dev, priv, &io_res, buf->control_address,
- sizeof(struct crb_regs_tail));
- if (IS_ERR(priv->regs_t)) {
- ret = PTR_ERR(priv->regs_t);
- goto out_relinquish_locality;
- }
-
/*
* PTT HW bug w/a: wake up the device to access
* possibly not retained registers.
@@ -552,13 +605,17 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
pa_low = ioread32(&priv->regs_t->ctrl_cmd_pa_low);
cmd_pa = ((u64)pa_high << 32) | pa_low;
- cmd_size = crb_fixup_cmd_size(dev, &io_res, cmd_pa,
- ioread32(&priv->regs_t->ctrl_cmd_size));
+ cmd_size = ioread32(&priv->regs_t->ctrl_cmd_size);
+
+ res_idx = tpm_crb_containing_res_idx(&resources, cmd_pa);
+ if (res_idx >= 0)
+ cmd_size = crb_fixup_cmd_size(dev, &resources.iores[res_idx],
+ cmd_pa, cmd_size);
dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n",
pa_high, pa_low, cmd_size);
- priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size);
+ priv->cmd = crb_map_res(dev, &resources, res_idx, cmd_pa, cmd_size);
if (IS_ERR(priv->cmd)) {
ret = PTR_ERR(priv->cmd);
goto out;
@@ -566,11 +623,17 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
memcpy_fromio(&__rsp_pa, &priv->regs_t->ctrl_rsp_pa, 8);
rsp_pa = le64_to_cpu(__rsp_pa);
- rsp_size = crb_fixup_cmd_size(dev, &io_res, rsp_pa,
- ioread32(&priv->regs_t->ctrl_rsp_size));
+ rsp_size = ioread32(&priv->regs_t->ctrl_rsp_size);
+
+ res_idx = tpm_crb_containing_res_idx(&resources, rsp_pa);
+
+ if (res_idx >= 0)
+ rsp_size = crb_fixup_cmd_size(dev, &resources.iores[res_idx],
+ rsp_pa, rsp_size);
if (cmd_pa != rsp_pa) {
- priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size);
+ priv->rsp = crb_map_res(dev, &resources, res_idx,
+ rsp_pa, rsp_size);
ret = PTR_ERR_OR_ZERO(priv->rsp);
goto out;
}
--
2.20.1
On Thu, Sep 26, 2019 at 12:56:46AM +0300, [email protected] wrote:
> +struct tpm_crb_resources {
> + struct resource iores[TPM_CRB_MAX_RESOURCES];
> + void __iomem *iobase[TPM_CRB_MAX_RESOURCES];
> + int num;
> +};
Do not add a new struct.
> +
> static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
> unsigned long timeout)
> {
> @@ -432,38 +438,75 @@ static const struct tpm_class_ops tpm_crb = {
> .req_complete_val = CRB_DRV_STS_COMPLETE,
> };
>
> +static bool tpm_crb_resource_contains(struct resource *iores,
> + u64 address)
> +{
> + return address >= iores->start && address <= iores->end;
> +}
Open code this. Makes the code only more unreadable.
> +static int tpm_crb_containing_res_idx(struct tpm_crb_resources *resources,
> + u64 address)
> +{
> + int res_idx;
Use "int i;"
> +
> + for (res_idx = 0; res_idx < resources->num; ++res_idx) {
> + if (tpm_crb_resource_contains(&resources->iores[res_idx],
> + address))
> + return res_idx;
> + }
> +
> + return -1;
> +}
Open code this. Two call sites do not make the function worth it.
> +
> static int crb_check_resource(struct acpi_resource *ares, void *data)
> {
> - struct resource *io_res = data;
> + struct tpm_crb_resources *resources = data;
> 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;
> + if (resources->num < TPM_CRB_MAX_RESOURCES) {
> + resources->iores[resources->num] = *res;
> + resources->iores[resources->num].name = NULL;
> + }
> + resources->num += 1;
> }
>
> return 1;
> }
>
> -static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
> - struct resource *io_res, u64 start, u32 size)
> +static void __iomem *crb_map_res(struct device *dev,
> + struct tpm_crb_resources *resources,
> + int res_idx,
> + u64 start, u32 size)
> {
> struct resource new_res = {
> .start = start,
> .end = start + size - 1,
> .flags = IORESOURCE_MEM,
> };
> + struct resource *iores;
> + void __iomem *iobase;
>
> /* Detect a 64 bit address on a 32 bit system */
> if (start != new_res.start)
> return (void __iomem *) ERR_PTR(-EINVAL);
>
> - if (!resource_contains(io_res, &new_res))
> + if (res_idx < 0)
> return devm_ioremap_resource(dev, &new_res);
>
> - return priv->iobase + (new_res.start - io_res->start);
> + iores = &resources->iores[res_idx];
> + iobase = resources->iobase[res_idx];
> + if (!iobase) {
> + iobase = devm_ioremap_resource(dev, iores);
> + if (IS_ERR(iobase))
> + return iobase;
> +
> + resources->iobase[res_idx] = iobase;
> + }
> +
> + return iobase + (new_res.start - iores->start);
> }
>
> /*
> @@ -490,9 +533,10 @@ static u64 crb_fixup_cmd_size(struct device *dev, struct resource *io_res,
> static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
> struct acpi_table_tpm2 *buf)
> {
> - struct list_head resources;
> - struct resource io_res;
> + struct list_head acpi_resources;
> struct device *dev = &device->dev;
> + struct tpm_crb_resources resources;
> + int res_idx;
> u32 pa_high, pa_low;
> u64 cmd_pa;
> u32 cmd_size;
> @@ -501,21 +545,36 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
> u32 rsp_size;
> int ret;
>
> - INIT_LIST_HEAD(&resources);
> - ret = acpi_dev_get_resources(device, &resources, crb_check_resource,
> - &io_res);
> + INIT_LIST_HEAD(&acpi_resources);
> + memset(&resources, 0, sizeof(resources));
> + ret = acpi_dev_get_resources(device, &acpi_resources,
> + crb_check_resource, &resources);
> if (ret < 0)
> return ret;
> - acpi_dev_free_resource_list(&resources);
> + acpi_dev_free_resource_list(&acpi_resources);
>
> - if (resource_type(&io_res) != IORESOURCE_MEM) {
> + if (resources.num == 0) {
> dev_err(dev, FW_BUG "TPM2 ACPI table does not define a memory resource\n");
> return -EINVAL;
> + } else if (resources.num > TPM_CRB_MAX_RESOURCES) {
> + dev_warn(dev, FW_BUG "TPM2 ACPI table defines too many memory resources\n");
> + resources.num = TPM_CRB_MAX_RESOURCES;
Remove FW_BUG as this would not be a bug.
/Jarkko