* [PATCH v2 0/2] Enhance support for the AMD's fTPM
@ 2019-09-09 9:09 Seunghun Han
2019-09-09 9:09 ` [PATCH v2 1/2] tpm: tpm_crb: enhance command and response buffer size calculation code Seunghun Han
2019-09-09 9:09 ` [PATCH v2 2/2] tpm: tpm_crb: enhance resource mapping mechanism for supporting AMD's fTPM Seunghun Han
0 siblings, 2 replies; 11+ messages in thread
From: Seunghun Han @ 2019-09-09 9:09 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Peter Huewe, Matthew Garrett, open list:TPM DEVICE DRIVER,
linux-kernel, Seunghun Han
This patch series enhances the support for the AMD's fTPM.
The AMD system assigned a command buffer and response buffer
independently in ACPI NVS region. ACPI NVS region allowed nothing to
assign a resource in it.
For supporting AMD's fTPM, I made a patch to enhance the code of command
and response buffer size calculation. I also made a patch to detect TPM
regions in ACPI NVS and work around it.
Changes in v2:
- fix a warning of kbuild test robot. The link is below.
https://lkml.org/lkml/2019/8/31/217
Seunghun Han (2):
tpm: tpm_crb: enhance command and response buffer size calculation
code
tpm: tpm_crb: enhance resource mapping mechanism for supporting AMD's
fTPM
drivers/char/tpm/tpm_crb.c | 69 ++++++++++++++++++++++++++++++++------
1 file changed, 59 insertions(+), 10 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v2 1/2] tpm: tpm_crb: enhance command and response buffer size calculation code 2019-09-09 9:09 [PATCH v2 0/2] Enhance support for the AMD's fTPM Seunghun Han @ 2019-09-09 9:09 ` Seunghun Han 2019-09-10 12:34 ` Jarkko Sakkinen 2019-09-09 9:09 ` [PATCH v2 2/2] tpm: tpm_crb: enhance resource mapping mechanism for supporting AMD's fTPM Seunghun Han 1 sibling, 1 reply; 11+ messages in thread From: Seunghun Han @ 2019-09-09 9:09 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Peter Huewe, Matthew Garrett, open list:TPM DEVICE DRIVER, linux-kernel, Seunghun Han The purpose of crb_fixup_cmd_size() function is to work around broken BIOSes and get the trustable size between the ACPI region and register. When the TPM has a command buffer and response buffer independently, the crb_map_io() function calls crb_fixup_cmd_size() twice to calculate each buffer size. However, the current implementation of it considers one of two buffers. To support independent command and response buffers, I changed crb_check_resource() function for storing ACPI TPB regions to a list. I also changed crb_fixup_cmd_size() to use the list for calculating each buffer size. Signed-off-by: Seunghun Han <kkamagui@gmail.com> --- Changes in v2: same as v1. drivers/char/tpm/tpm_crb.c | 44 +++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index e59f1f91d7f3..14f486c23af2 100644 --- a/drivers/char/tpm/tpm_crb.c +++ b/drivers/char/tpm/tpm_crb.c @@ -442,6 +442,9 @@ static int crb_check_resource(struct acpi_resource *ares, void *data) acpi_dev_resource_address_space(ares, &win)) { *io_res = *res; io_res->name = NULL; + + /* Add this TPM CRB resource to the list */ + return 0; } return 1; @@ -471,7 +474,7 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv, * region vs the registers. Trust the ACPI region. Such broken systems * probably cannot send large TPM commands since the buffer will be truncated. */ -static u64 crb_fixup_cmd_size(struct device *dev, struct resource *io_res, +static u64 __crb_fixup_cmd_size(struct device *dev, struct resource *io_res, u64 start, u64 size) { if (io_res->start > start || io_res->end < start) @@ -487,6 +490,26 @@ static u64 crb_fixup_cmd_size(struct device *dev, struct resource *io_res, return io_res->end - start + 1; } +static u64 crb_fixup_cmd_size(struct device *dev, struct list_head *resources, + u64 start, u64 size) +{ + struct resource_entry *pos; + struct resource *cur_res; + u64 ret = size; + + /* Check all TPM CRB resources with the start and size values */ + resource_list_for_each_entry(pos, resources) { + cur_res = pos->res; + + ret = __crb_fixup_cmd_size(dev, cur_res, start, size); + /* Broken BIOS is detected. Trust the ACPI region. */ + if (ret < size) + break; + } + + return ret; +} + static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, struct acpi_table_tpm2 *buf) { @@ -506,16 +529,18 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, &io_res); if (ret < 0) return ret; - acpi_dev_free_resource_list(&resources); if (resource_type(&io_res) != IORESOURCE_MEM) { dev_err(dev, FW_BUG "TPM2 ACPI table does not define a memory resource\n"); - return -EINVAL; + ret = -EINVAL; + goto out_early; } priv->iobase = devm_ioremap_resource(dev, &io_res); - if (IS_ERR(priv->iobase)) - return PTR_ERR(priv->iobase); + if (IS_ERR(priv->iobase)) { + ret = PTR_ERR(priv->iobase); + goto out_early; + } /* 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 @@ -532,7 +557,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, ret = __crb_request_locality(dev, priv, 0); if (ret) - return ret; + goto out_early; priv->regs_t = crb_map_res(dev, priv, &io_res, buf->control_address, sizeof(struct crb_regs_tail)); @@ -552,7 +577,7 @@ 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, + cmd_size = crb_fixup_cmd_size(dev, &resources, cmd_pa, ioread32(&priv->regs_t->ctrl_cmd_size)); dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n", @@ -566,7 +591,7 @@ 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, + rsp_size = crb_fixup_cmd_size(dev, &resources, rsp_pa, ioread32(&priv->regs_t->ctrl_rsp_size)); if (cmd_pa != rsp_pa) { @@ -596,6 +621,9 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, __crb_relinquish_locality(dev, priv, 0); +out_early: + acpi_dev_free_resource_list(&resources); + return ret; } -- 2.21.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] tpm: tpm_crb: enhance command and response buffer size calculation code 2019-09-09 9:09 ` [PATCH v2 1/2] tpm: tpm_crb: enhance command and response buffer size calculation code Seunghun Han @ 2019-09-10 12:34 ` Jarkko Sakkinen 2019-09-10 15:12 ` Seunghun Han 0 siblings, 1 reply; 11+ messages in thread From: Jarkko Sakkinen @ 2019-09-10 12:34 UTC (permalink / raw) To: Seunghun Han Cc: Peter Huewe, Matthew Garrett, open list:TPM DEVICE DRIVER, linux-kernel On Mon, Sep 09, 2019 at 06:09:05PM +0900, Seunghun Han wrote: > The purpose of crb_fixup_cmd_size() function is to work around broken > BIOSes and get the trustable size between the ACPI region and register. > When the TPM has a command buffer and response buffer independently, > the crb_map_io() function calls crb_fixup_cmd_size() twice to calculate > each buffer size. However, the current implementation of it considers > one of two buffers. > > To support independent command and response buffers, I changed > crb_check_resource() function for storing ACPI TPB regions to a list. > I also changed crb_fixup_cmd_size() to use the list for calculating each > buffer size. > > Signed-off-by: Seunghun Han <kkamagui@gmail.com> I think as far as the tpm_crb goes I focus on getting Vanya's change landed because it is better structured, more mature and the first version was sent couple of weeks earlier. You are welcome to make your remarks on that patch. /Jarkko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] tpm: tpm_crb: enhance command and response buffer size calculation code 2019-09-10 12:34 ` Jarkko Sakkinen @ 2019-09-10 15:12 ` Seunghun Han 0 siblings, 0 replies; 11+ messages in thread From: Seunghun Han @ 2019-09-10 15:12 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Peter Huewe, Matthew Garrett, open list:TPM DEVICE DRIVER, Linux Kernel Mailing List > > On Mon, Sep 09, 2019 at 06:09:05PM +0900, Seunghun Han wrote: > > The purpose of crb_fixup_cmd_size() function is to work around broken > > BIOSes and get the trustable size between the ACPI region and register. > > When the TPM has a command buffer and response buffer independently, > > the crb_map_io() function calls crb_fixup_cmd_size() twice to calculate > > each buffer size. However, the current implementation of it considers > > one of two buffers. > > > > To support independent command and response buffers, I changed > > crb_check_resource() function for storing ACPI TPB regions to a list. > > I also changed crb_fixup_cmd_size() to use the list for calculating each > > buffer size. > > > > Signed-off-by: Seunghun Han <kkamagui@gmail.com> > > I think as far as the tpm_crb goes I focus on getting Vanya's change > landed because it is better structured, more mature and the first > version was sent couple of weeks earlier. You are welcome to make > your remarks on that patch. Thank you for your review. I already knew Vanya's patch, https://lkml.org/lkml/2019/8/11/151, and this patch didn't work for me. I also couldn't agree on some points like memory allocating inside the ACPI walker and changing many parts of TPM driver. I would like to support AMD's fTPM with the smallest changes since this is a workaround as you know. I didn't understand clearly what your point is. Do you want me to change my patches structurally like Vanya's patch and make patch v3? or want me to give some advice to Vanya? > > /Jarkko ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] tpm: tpm_crb: enhance resource mapping mechanism for supporting AMD's fTPM 2019-09-09 9:09 [PATCH v2 0/2] Enhance support for the AMD's fTPM Seunghun Han 2019-09-09 9:09 ` [PATCH v2 1/2] tpm: tpm_crb: enhance command and response buffer size calculation code Seunghun Han @ 2019-09-09 9:09 ` Seunghun Han 2019-09-10 14:42 ` Jarkko Sakkinen 1 sibling, 1 reply; 11+ messages in thread From: Seunghun Han @ 2019-09-09 9:09 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Peter Huewe, Matthew Garrett, open list:TPM DEVICE DRIVER, linux-kernel, Seunghun Han I got an AMD system which had a Ryzen Threadripper 1950X and MSI mainboard, and I had a problem with AMD's fTPM. My machine showed an error message below, and the fTPM didn't work because of it. [ 5.732084] tpm_crb MSFT0101:00: can't request region for resource [mem 0x79b4f000-0x79b4ffff] [ 5.732089] tpm_crb: probe of MSFT0101:00 failed with error -16 When I saw the iomem, I found two fTPM regions were in the ACPI NVS area. The regions are below. 79a39000-79b6afff : ACPI Non-volatile Storage 79b4b000-79b4bfff : MSFT0101:00 79b4f000-79b4ffff : MSFT0101:00 After analyzing this issue, I found that crb_map_io() function called devm_ioremap_resource() and it failed. The ACPI NVS didn't allow the TPM CRB driver to assign a resource in it because a busy bit was set to the ACPI NVS area. To support AMD's fTPM, I added a function to check intersects between the TPM region and ACPI NVS before it mapped the region. If some intersects are detected, the function just calls devm_ioremap() for a workaround. If there is no intersect, it calls devm_ioremap_resource(). Signed-off-by: Seunghun Han <kkamagui@gmail.com> --- Changes in v2: fix a warning of kbuild test robot. The link is below. https://lkml.org/lkml/2019/8/31/217 drivers/char/tpm/tpm_crb.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index 14f486c23af2..6b98a3a995b7 100644 --- a/drivers/char/tpm/tpm_crb.c +++ b/drivers/char/tpm/tpm_crb.c @@ -450,6 +450,27 @@ static int crb_check_resource(struct acpi_resource *ares, void *data) return 1; } +static void __iomem *crb_ioremap_resource(struct device *dev, + const struct resource *res) +{ + int rc; + resource_size_t size = resource_size(res); + + /* Broken BIOS assigns command and response buffers in ACPI NVS region. + * Check intersections between a resource and ACPI NVS for W/A. + */ + rc = region_intersects(res->start, size, IORESOURCE_MEM | + IORESOURCE_BUSY, IORES_DESC_ACPI_NV_STORAGE); + if (rc != REGION_DISJOINT) { + dev_err(dev, + FW_BUG "Resource overlaps with a ACPI NVS. %pr\n", + res); + return devm_ioremap(dev, res->start, size); + } + + return devm_ioremap_resource(dev, res); +} + static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv, struct resource *io_res, u64 start, u32 size) { @@ -464,7 +485,7 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv, return (void __iomem *) ERR_PTR(-EINVAL); if (!resource_contains(io_res, &new_res)) - return devm_ioremap_resource(dev, &new_res); + return crb_ioremap_resource(dev, &new_res); return priv->iobase + (new_res.start - io_res->start); } @@ -536,7 +557,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, goto out_early; } - priv->iobase = devm_ioremap_resource(dev, &io_res); + priv->iobase = crb_ioremap_resource(dev, &io_res); if (IS_ERR(priv->iobase)) { ret = PTR_ERR(priv->iobase); goto out_early; -- 2.21.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] tpm: tpm_crb: enhance resource mapping mechanism for supporting AMD's fTPM 2019-09-09 9:09 ` [PATCH v2 2/2] tpm: tpm_crb: enhance resource mapping mechanism for supporting AMD's fTPM Seunghun Han @ 2019-09-10 14:42 ` Jarkko Sakkinen 2019-09-10 15:06 ` Jarkko Sakkinen 0 siblings, 1 reply; 11+ messages in thread From: Jarkko Sakkinen @ 2019-09-10 14:42 UTC (permalink / raw) To: Seunghun Han Cc: Peter Huewe, Matthew Garrett, open list:TPM DEVICE DRIVER, linux-kernel, Vanya Lazeev On Mon, Sep 09, 2019 at 06:09:06PM +0900, Seunghun Han wrote: > I got an AMD system which had a Ryzen Threadripper 1950X and MSI > mainboard, and I had a problem with AMD's fTPM. My machine showed an error > message below, and the fTPM didn't work because of it. > > [ 5.732084] tpm_crb MSFT0101:00: can't request region for resource > [mem 0x79b4f000-0x79b4ffff] > [ 5.732089] tpm_crb: probe of MSFT0101:00 failed with error -16 > > When I saw the iomem, I found two fTPM regions were in the ACPI NVS area. > The regions are below. > > 79a39000-79b6afff : ACPI Non-volatile Storage > 79b4b000-79b4bfff : MSFT0101:00 > 79b4f000-79b4ffff : MSFT0101:00 > > After analyzing this issue, I found that crb_map_io() function called > devm_ioremap_resource() and it failed. The ACPI NVS didn't allow the TPM > CRB driver to assign a resource in it because a busy bit was set to > the ACPI NVS area. > > To support AMD's fTPM, I added a function to check intersects between > the TPM region and ACPI NVS before it mapped the region. If some > intersects are detected, the function just calls devm_ioremap() for > a workaround. If there is no intersect, it calls devm_ioremap_resource(). > > Signed-off-by: Seunghun Han <kkamagui@gmail.com> This problem is still valid and not addressed by Vanya's patch (and should not be as it is a disjoint issue). However, calling devm_ioremap() is somewhat racy as the NVS driver is not aware of that. My take is that this should be fixed in the code that assigns regions to the NVS driver e.g. it could look up the regions assigned to the MSFT0101 and ignore those regions. In the end linux-acpi maintainers have the say on this but this would be the angle that I'd take to implement such patch probably. /Jarkko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] tpm: tpm_crb: enhance resource mapping mechanism for supporting AMD's fTPM 2019-09-10 14:42 ` Jarkko Sakkinen @ 2019-09-10 15:06 ` Jarkko Sakkinen 2019-09-10 15:28 ` Seunghun Han 0 siblings, 1 reply; 11+ messages in thread From: Jarkko Sakkinen @ 2019-09-10 15:06 UTC (permalink / raw) To: Seunghun Han Cc: Peter Huewe, Matthew Garrett, open list:TPM DEVICE DRIVER, linux-kernel, Vanya Lazeev On Tue, Sep 10, 2019 at 03:42:15PM +0100, Jarkko Sakkinen wrote: > On Mon, Sep 09, 2019 at 06:09:06PM +0900, Seunghun Han wrote: > > I got an AMD system which had a Ryzen Threadripper 1950X and MSI > > mainboard, and I had a problem with AMD's fTPM. My machine showed an error > > message below, and the fTPM didn't work because of it. > > > > [ 5.732084] tpm_crb MSFT0101:00: can't request region for resource > > [mem 0x79b4f000-0x79b4ffff] > > [ 5.732089] tpm_crb: probe of MSFT0101:00 failed with error -16 > > > > When I saw the iomem, I found two fTPM regions were in the ACPI NVS area. > > The regions are below. > > > > 79a39000-79b6afff : ACPI Non-volatile Storage > > 79b4b000-79b4bfff : MSFT0101:00 > > 79b4f000-79b4ffff : MSFT0101:00 > > > > After analyzing this issue, I found that crb_map_io() function called > > devm_ioremap_resource() and it failed. The ACPI NVS didn't allow the TPM > > CRB driver to assign a resource in it because a busy bit was set to > > the ACPI NVS area. > > > > To support AMD's fTPM, I added a function to check intersects between > > the TPM region and ACPI NVS before it mapped the region. If some > > intersects are detected, the function just calls devm_ioremap() for > > a workaround. If there is no intersect, it calls devm_ioremap_resource(). > > > > Signed-off-by: Seunghun Han <kkamagui@gmail.com> > > This problem is still valid and not addressed by Vanya's patch (and > should not be as it is a disjoint issue). However, calling > devm_ioremap() is somewhat racy as the NVS driver is not aware of that. > > My take is that this should be fixed in the code that assigns regions to > the NVS driver e.g. it could look up the regions assigned to the > MSFT0101 and ignore those regions. In the end linux-acpi maintainers > have the say on this but this would be the angle that I'd take to > implement such patch probably. Matthew pointed out that having a hook in NVS driver is better solution because it is nil functionality if the TPM driver is loaded. We need functions to: 1. Request a region from the NVS driver (when tpm_crb loads) 2. Release a region back to the NVS Driver (when tpm_crb unloads). My proposal would unnecessarily duplicate code and also leave a side-effect when TPM is not used in the first place. I see this as the overally best solution. If you can come up with a patch for the NVS side and changes to CRB drivers to utilize the new hooks, then combined with Vanya's changes we have a sustainable solution for AMD fTPM. /Jarkko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] tpm: tpm_crb: enhance resource mapping mechanism for supporting AMD's fTPM 2019-09-10 15:06 ` Jarkko Sakkinen @ 2019-09-10 15:28 ` Seunghun Han 2019-09-13 13:12 ` Jarkko Sakkinen 0 siblings, 1 reply; 11+ messages in thread From: Seunghun Han @ 2019-09-10 15:28 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Peter Huewe, Matthew Garrett, open list:TPM DEVICE DRIVER, Linux Kernel Mailing List, Vanya Lazeev > > On Tue, Sep 10, 2019 at 03:42:15PM +0100, Jarkko Sakkinen wrote: > > On Mon, Sep 09, 2019 at 06:09:06PM +0900, Seunghun Han wrote: > > > I got an AMD system which had a Ryzen Threadripper 1950X and MSI > > > mainboard, and I had a problem with AMD's fTPM. My machine showed an error > > > message below, and the fTPM didn't work because of it. > > > > > > [ 5.732084] tpm_crb MSFT0101:00: can't request region for resource > > > [mem 0x79b4f000-0x79b4ffff] > > > [ 5.732089] tpm_crb: probe of MSFT0101:00 failed with error -16 > > > > > > When I saw the iomem, I found two fTPM regions were in the ACPI NVS area. > > > The regions are below. > > > > > > 79a39000-79b6afff : ACPI Non-volatile Storage > > > 79b4b000-79b4bfff : MSFT0101:00 > > > 79b4f000-79b4ffff : MSFT0101:00 > > > > > > After analyzing this issue, I found that crb_map_io() function called > > > devm_ioremap_resource() and it failed. The ACPI NVS didn't allow the TPM > > > CRB driver to assign a resource in it because a busy bit was set to > > > the ACPI NVS area. > > > > > > To support AMD's fTPM, I added a function to check intersects between > > > the TPM region and ACPI NVS before it mapped the region. If some > > > intersects are detected, the function just calls devm_ioremap() for > > > a workaround. If there is no intersect, it calls devm_ioremap_resource(). > > > > > > Signed-off-by: Seunghun Han <kkamagui@gmail.com> > > > > This problem is still valid and not addressed by Vanya's patch (and > > should not be as it is a disjoint issue). However, calling > > devm_ioremap() is somewhat racy as the NVS driver is not aware of that. > > > > My take is that this should be fixed in the code that assigns regions to > > the NVS driver e.g. it could look up the regions assigned to the > > MSFT0101 and ignore those regions. In the end linux-acpi maintainers > > have the say on this but this would be the angle that I'd take to > > implement such patch probably. > > Matthew pointed out that having a hook in NVS driver is better solution > because it is nil functionality if the TPM driver is loaded. We need > functions to: > > 1. Request a region from the NVS driver (when tpm_crb loads) > 2. Release a region back to the NVS Driver (when tpm_crb unloads). > > My proposal would unnecessarily duplicate code and also leave a > side-effect when TPM is not used in the first place. > > I see this as the overally best solution. If you can come up with a > patch for the NVS side and changes to CRB drivers to utilize the new > hooks, then combined with Vanya's changes we have a sustainable solution > for AMD fTPM. It's a great solution. I will update this patch on your advice and send it to you soon. By the way, I have a question about your advice. If we handle the NVS region with NVS driver, calling devm_ioremap() function is fine like crb_ioremap_resource() function in this patch? > > /Jarkko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] tpm: tpm_crb: enhance resource mapping mechanism for supporting AMD's fTPM 2019-09-10 15:28 ` Seunghun Han @ 2019-09-13 13:12 ` Jarkko Sakkinen 2019-09-16 8:18 ` Seunghun Han 0 siblings, 1 reply; 11+ messages in thread From: Jarkko Sakkinen @ 2019-09-13 13:12 UTC (permalink / raw) To: Seunghun Han Cc: Peter Huewe, Matthew Garrett, open list:TPM DEVICE DRIVER, Linux Kernel Mailing List, Vanya Lazeev On Wed, Sep 11, 2019 at 12:28:18AM +0900, Seunghun Han wrote: > > Matthew pointed out that having a hook in NVS driver is better solution > > because it is nil functionality if the TPM driver is loaded. We need > > functions to: > > > > 1. Request a region from the NVS driver (when tpm_crb loads) > > 2. Release a region back to the NVS Driver (when tpm_crb unloads). > > > > My proposal would unnecessarily duplicate code and also leave a > > side-effect when TPM is not used in the first place. > > > > I see this as the overally best solution. If you can come up with a > > patch for the NVS side and changes to CRB drivers to utilize the new > > hooks, then combined with Vanya's changes we have a sustainable solution > > for AMD fTPM. > > It's a great solution. I will update this patch on your advice and > send it to you soon. > > By the way, I have a question about your advice. > If we handle the NVS region with NVS driver, calling devm_ioremap() > function is fine like crb_ioremap_resource() function in this patch? No, you should reclaim the resource that conflicts and return it back when tpm_crb is unregistered (e.g. rmmod tpm_crb). I would try something like enumerating iomem resources with walk_iomem_res_desc(). I would advice to peek at arch/x86/kernel/crash.c for an example how to use this for NVS regions (IORES_DESC_ACPI_NV_STORAGE). E.g. you could use a callback for it along the lines of: static int crb_find_iomem_res_cb(struct resource *res, void *io_res_ptr) { struct resource *io_res = io_res_ptr; if (res->start == io_res->start && res->end == io_res->end) { /* * Backup all resource data so that it can be inserted * later on with the flags it had etc. */ *io_res = *res; return 1; } return 0; } Then you could __release_region() to unallocate the source. When tpm_crb is removed you can then allocate and insert a resource with data matching it had. /Jarkko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] tpm: tpm_crb: enhance resource mapping mechanism for supporting AMD's fTPM 2019-09-13 13:12 ` Jarkko Sakkinen @ 2019-09-16 8:18 ` Seunghun Han 2019-09-16 8:42 ` Seunghun Han 0 siblings, 1 reply; 11+ messages in thread From: Seunghun Han @ 2019-09-16 8:18 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Peter Huewe, Matthew Garrett, open list:TPM DEVICE DRIVER, Linux Kernel Mailing List, Vanya Lazeev > > > Matthew pointed out that having a hook in NVS driver is better solution > > > because it is nil functionality if the TPM driver is loaded. We need > > > functions to: > > > > > > 1. Request a region from the NVS driver (when tpm_crb loads) > > > 2. Release a region back to the NVS Driver (when tpm_crb unloads). > > > > > > My proposal would unnecessarily duplicate code and also leave a > > > side-effect when TPM is not used in the first place. > > > > > > I see this as the overally best solution. If you can come up with a > > > patch for the NVS side and changes to CRB drivers to utilize the new > > > hooks, then combined with Vanya's changes we have a sustainable solution > > > for AMD fTPM. > > > > It's a great solution. I will update this patch on your advice and > > send it to you soon. > > > > By the way, I have a question about your advice. > > If we handle the NVS region with NVS driver, calling devm_ioremap() > > function is fine like crb_ioremap_resource() function in this patch? > > No, you should reclaim the resource that conflicts and return it back > when tpm_crb is unregistered (e.g. rmmod tpm_crb). > > I would try something like enumerating iomem resources with > walk_iomem_res_desc(). I would advice to peek at arch/x86/kernel/crash.c > for an example how to use this for NVS regions > > Then you could __release_region() to unallocate the source. When tpm_crb > is removed you can then allocate and insert a resource with data > matching it had. Thank you for your sincere advice, and I have some questions about it. As you know, the core reason of this ACPI NVS problem is that a busy bit is set to the ACPI NVS area. So, devm_ioremap_resource() function fails because of it. If we want to call devm_ioremap_resource() for this case, we maybe need to rearrange the existing memory layout from the child relationship to the sibling relationship below. We also need to get back when tpm_crb unloads. [ ACPI NVS (parent) [ TPM CMD buffer (child of NVS) ] [ TPM RSP buffer (child of NVS) ] ] <---> [ ACPI NVS head ] [ CMD buffer ] [ NVS middle ] [ RSP buffer ] [ NVS tails ] Our concern is a race condition between NVS driver and TPM CRB driver. In my view, we could solve this problem if we only make and call the functions you said, requesting and releasing a region from NVS driver. NVS driver doesn't rely on iomem layout, and it relies on internal nvs_list data. Therefore, I added some details to your guide. How about this sequence? 1) When tpm_crb driver loads, the driver checks if command/response buffers are in ACPI NVS area. If so, it requests (or removes) the buffer regions from NVS driver's nvs_list (with suspend_nvs_unregister() function I will add to the nvs.c driver). 2) If command/response buffers are in ACPI NVS area, tpm_crb driver calls devm_ioremap() instead of devm_ioremap_resource() like this patch. 3) When tpm_crb driver unloads, the driver releases (or adds) the buffer regions to NVS driver's nvs_list (with existing suspend_nvs_register() function in the nvs.c driver). I think the sequence could solve the problem we concerned. What do you think about the sequence? Seunghun ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] tpm: tpm_crb: enhance resource mapping mechanism for supporting AMD's fTPM 2019-09-16 8:18 ` Seunghun Han @ 2019-09-16 8:42 ` Seunghun Han 0 siblings, 0 replies; 11+ messages in thread From: Seunghun Han @ 2019-09-16 8:42 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Peter Huewe, Matthew Garrett, open list:TPM DEVICE DRIVER, Linux Kernel Mailing List, Vanya Lazeev Sorry for my mistake. I misunderstood some functions in nvs.c. So I have fixed it and sent my email again. My email is below. > > > Matthew pointed out that having a hook in NVS driver is better solution > > > because it is nil functionality if the TPM driver is loaded. We need > > > functions to: > > > > > > 1. Request a region from the NVS driver (when tpm_crb loads) > > > 2. Release a region back to the NVS Driver (when tpm_crb unloads). > > > > > > My proposal would unnecessarily duplicate code and also leave a > > > side-effect when TPM is not used in the first place. > > > > > > I see this as the overally best solution. If you can come up with a > > > patch for the NVS side and changes to CRB drivers to utilize the new > > > hooks, then combined with Vanya's changes we have a sustainable solution > > > for AMD fTPM. > > > > It's a great solution. I will update this patch on your advice and > > send it to you soon. > > > > By the way, I have a question about your advice. > > If we handle the NVS region with NVS driver, calling devm_ioremap() > > function is fine like crb_ioremap_resource() function in this patch? > > No, you should reclaim the resource that conflicts and return it back > when tpm_crb is unregistered (e.g. rmmod tpm_crb). > > I would try something like enumerating iomem resources with > walk_iomem_res_desc(). I would advice to peek at arch/x86/kernel/crash.c > for an example how to use this for NVS regions > > Then you could __release_region() to unallocate the source. When tpm_crb > is removed you can then allocate and insert a resource with data > matching it had. Thank you for your sincere advice, and I have some questions about it. As you know, the core reason of this ACPI NVS problem is that a busy bit is set to the ACPI NVS area. So, devm_ioremap_resource() function fails because of it. If we want to call devm_ioremap_resource() for this case, we maybe need to rearrange the existing memory layout from the child relationship to the sibling relationship below. We also need to get back when tpm_crb unloads. [ ACPI NVS (parent) [ TPM CMD buffer (child of NVS) ] [ TPM RSP buffer (child of NVS) ] ] <---> [ ACPI NVS head ] [ CMD buffer ] [ NVS middle ] [ RSP buffer ] [ NVS tails ] Our concern is a race condition between NVS driver and TPM CRB driver. In my view, we could solve this problem if we only make and call the functions you said, requesting and releasing a region from NVS driver. NVS driver doesn't rely on iomem layout, and it relies on internal nvs_region_list data. Therefore, I added some details to your guide. How about this sequence? 1) When tpm_crb driver loads, the driver checks if command/response buffers are in ACPI NVS area. If so, it requests (or removes) the buffer regions from NVS driver's nvs_region_list (with acpi_nvs_unregister() function I will add to the nvs.c driver). 2) If command/response buffers are in ACPI NVS area, tpm_crb driver calls devm_ioremap() instead of devm_ioremap_resource() like this patch. 3) When tpm_crb driver unloads, the driver releases (or adds) the buffer regions to NVS driver's nvs_region_list (with existing acpi_nvs_register() function in the nvs.c driver). I think the sequence could solve the problem we concerned. What do you think about the sequence? Seunghun ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-09-16 8:42 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-09 9:09 [PATCH v2 0/2] Enhance support for the AMD's fTPM Seunghun Han 2019-09-09 9:09 ` [PATCH v2 1/2] tpm: tpm_crb: enhance command and response buffer size calculation code Seunghun Han 2019-09-10 12:34 ` Jarkko Sakkinen 2019-09-10 15:12 ` Seunghun Han 2019-09-09 9:09 ` [PATCH v2 2/2] tpm: tpm_crb: enhance resource mapping mechanism for supporting AMD's fTPM Seunghun Han 2019-09-10 14:42 ` Jarkko Sakkinen 2019-09-10 15:06 ` Jarkko Sakkinen 2019-09-10 15:28 ` Seunghun Han 2019-09-13 13:12 ` Jarkko Sakkinen 2019-09-16 8:18 ` Seunghun Han 2019-09-16 8:42 ` Seunghun Han
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).