* [PATCH] tpm, tpm_crb: Handle 64-bit resource in crb_check_resource() @ 2016-12-19 4:20 Jiandi An 2016-12-19 13:56 ` Jarkko Sakkinen 2017-01-03 18:27 ` Jarkko Sakkinen 0 siblings, 2 replies; 7+ messages in thread From: Jiandi An @ 2016-12-19 4:20 UTC (permalink / raw) To: peterhuewe, tpmdd, jarkko.sakkinen, jgunthorpe, tpmdd-devel, linux-kernel Cc: Jiandi An crb_check_resource() in TPM CRB driver calls acpi_dev_resource_memory() which only handles 32-bit resources. Adding a call to acpi_dev_resource_address_space() in TPM CRB driver which handles 64-bit resources. Signed-off-by: Jiandi An <anjiandi@codeaurora.org> --- drivers/char/tpm/tpm_crb.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index 717b6b4..86f355b 100644 --- a/drivers/char/tpm/tpm_crb.c +++ b/drivers/char/tpm/tpm_crb.c @@ -264,10 +264,12 @@ static bool crb_req_canceled(struct tpm_chip *chip, u8 status) static int crb_check_resource(struct acpi_resource *ares, void *data) { struct resource *io_res = data; - struct resource res; + struct resource_win win; + struct resource *res = &(win.res); - if (acpi_dev_resource_memory(ares, &res)) { - *io_res = res; + if (acpi_dev_resource_memory(ares, res) || + acpi_dev_resource_address_space(ares, &win)) { + *io_res = *res; io_res->name = NULL; } -- Jiandi An Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] tpm, tpm_crb: Handle 64-bit resource in crb_check_resource() 2016-12-19 4:20 [PATCH] tpm, tpm_crb: Handle 64-bit resource in crb_check_resource() Jiandi An @ 2016-12-19 13:56 ` Jarkko Sakkinen 2016-12-19 16:22 ` Jason Gunthorpe 2016-12-20 6:19 ` [tpmdd-devel] " Jiandi An 2017-01-03 18:27 ` Jarkko Sakkinen 1 sibling, 2 replies; 7+ messages in thread From: Jarkko Sakkinen @ 2016-12-19 13:56 UTC (permalink / raw) To: Jiandi An; +Cc: peterhuewe, tpmdd, jgunthorpe, tpmdd-devel, linux-kernel On Sun, Dec 18, 2016 at 10:20:53PM -0600, Jiandi An wrote: > crb_check_resource() in TPM CRB driver calls > acpi_dev_resource_memory() which only handles 32-bit resources. > Adding a call to acpi_dev_resource_address_space() in TPM CRB > driver which handles 64-bit resources. > > Signed-off-by: Jiandi An <anjiandi@codeaurora.org> 1. Is there a platform in existence where this change fixes a problem? 2. What is difference between "memory" and "address space" conceptually? Just wondering why 32-bit stuff is "memory" and 64-bit stuff is "address space". Could there be a one function that would work both for 32-bit and 64-bit cases? Yeah, I do not know this API too well. That's why I'm asking. /Jarkko > --- > drivers/char/tpm/tpm_crb.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > index 717b6b4..86f355b 100644 > --- a/drivers/char/tpm/tpm_crb.c > +++ b/drivers/char/tpm/tpm_crb.c > @@ -264,10 +264,12 @@ static bool crb_req_canceled(struct tpm_chip *chip, u8 status) > static int crb_check_resource(struct acpi_resource *ares, void *data) > { > struct resource *io_res = data; > - struct resource res; > + struct resource_win win; > + struct resource *res = &(win.res); > > - if (acpi_dev_resource_memory(ares, &res)) { > - *io_res = res; > + if (acpi_dev_resource_memory(ares, res) || > + acpi_dev_resource_address_space(ares, &win)) { > + *io_res = *res; > io_res->name = NULL; > } > > -- > Jiandi An > Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. > Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tpm, tpm_crb: Handle 64-bit resource in crb_check_resource() 2016-12-19 13:56 ` Jarkko Sakkinen @ 2016-12-19 16:22 ` Jason Gunthorpe 2016-12-20 6:19 ` [tpmdd-devel] " Jiandi An 1 sibling, 0 replies; 7+ messages in thread From: Jason Gunthorpe @ 2016-12-19 16:22 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: Jiandi An, peterhuewe, tpmdd, tpmdd-devel, linux-kernel On Mon, Dec 19, 2016 at 03:56:24PM +0200, Jarkko Sakkinen wrote: > On Sun, Dec 18, 2016 at 10:20:53PM -0600, Jiandi An wrote: > > crb_check_resource() in TPM CRB driver calls > > acpi_dev_resource_memory() which only handles 32-bit resources. > > Adding a call to acpi_dev_resource_address_space() in TPM CRB > > driver which handles 64-bit resources. > > > > Signed-off-by: Jiandi An <anjiandi@codeaurora.org> > > 1. Is there a platform in existence where this change fixes a problem? > 2. What is difference between "memory" and "address space" conceptually? > Just wondering why 32-bit stuff is "memory" and 64-bit stuff is > "address space". Could there be a one function that would work both > for 32-bit and 64-bit cases? > > Yeah, I do not know this API too well. That's why I'm asking. If this is the right thing it also needs to be done in tpm_tis. I will point out that this driver only works with memory, so using a generic decoder without checking for IO maps may not be correct.. Jason ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [tpmdd-devel] [PATCH] tpm, tpm_crb: Handle 64-bit resource in crb_check_resource() 2016-12-19 13:56 ` Jarkko Sakkinen 2016-12-19 16:22 ` Jason Gunthorpe @ 2016-12-20 6:19 ` Jiandi An 2017-01-03 17:11 ` Jarkko Sakkinen 1 sibling, 1 reply; 7+ messages in thread From: Jiandi An @ 2016-12-20 6:19 UTC (permalink / raw) To: Jarkko Sakkinen, Jason Gunthorpe; +Cc: tpmdd-devel, linux-kernel On 12/19/16 07:56, Jarkko Sakkinen wrote: > On Sun, Dec 18, 2016 at 10:20:53PM -0600, Jiandi An wrote: >> crb_check_resource() in TPM CRB driver calls >> acpi_dev_resource_memory() which only handles 32-bit resources. >> Adding a call to acpi_dev_resource_address_space() in TPM CRB >> driver which handles 64-bit resources. >> >> Signed-off-by: Jiandi An <anjiandi@codeaurora.org> > > 1. Is there a platform in existence where this change fixes a problem? > 2. What is difference between "memory" and "address space" conceptually? > Just wondering why 32-bit stuff is "memory" and 64-bit stuff is > "address space". Could there be a one function that would work both > for 32-bit and 64-bit cases? > > Yeah, I do not know this API too well. That's why I'm asking. > > /Jarkko > > > If this is the right thing it also needs to be done in tpm_tis. > > I will point out that this driver only works with memory, so using a > generic decoder without checking for IO maps may not be correct.. > > Jason > Hi Jarkko and Jason, Thanks for your comments. I am a developer at Qualcomm and we are trying to enable TPM CRB driver on ARM64 for Qualcomm Centriq 2400. Spec changes to ACPI table for TPM 2.0 have been submitted and approved by TCG and are currently in the 60 day waiting period for release. I have a series of patches that do this TPM CRB driver enablement for ARM64 that I'll be submitting. But first, for our platform the control area buffer address is greater than 32-bit. It is memory with no IO effects. I ran into this issue first when I use QWordMemory in ACPI ASL to describe the resource. MemoryRangeType is specified as AddressRangeMemory. The QWordMemory macro evaluates to a buffer which contains a 64-bit memory resource descriptor, which describes a range of memory addresses. It is a QWord Address Space Descriptor. acpi_dev_resource_address_space() handles the 64-bit memory described using QWordMemory macro in ACPI ASL. The Memory32Fixed macro evaluates to a buffer which contains a 32-bit memory descriptor, which describes a fixed range of memory addresses. acpi_dev_resource_memory() handles the 32-bit memory described using Memory32Fixed in ACPI ASL. I did not see a specific acpi_dev_resource_ service that handles 64-bit resource address and doesn't use the generic acpi_decode_space() decoder. I do have a question about having to specify _CRS method in ACPI ASL. When I was doing early prototyping for this enablement work on ARM64 back during the time with 4.5 kernel, it was before the introduction of the following two patches: commit 1bd047be37d95bf65a219f4931215f71878ac060 tpm_crb: Use devm_ioremap_resource commit 51dd43dff74b0547ad844638f6910ca29c956819 tpm_tis: Use devm_ioremap_resource The control area buffer is specified in the TPM2.0 static ACPI table. TPM CRB driver maps the control area address and reads out cmd and rsp buffer addresses and maps them. There is no requirement in the TCG TPM ACPI spec for specifying _CRS to describe the control area buffer. When I rebased the early prototype for CRB driver ARM64 enablement to the latest kernel, I hit this issue where I have to specify _CRS method. So in _CRS method I specify the same control area address that's in the TPM2.0 static ACPI table. I see the _CRS requirement in the CRB driver is for resource synchronization between the TIS and CRB drivers to support force mode in TIS. Could I get some background on that so I could be more careful not breaking TIS while making changes to CRB driver for ARM64 enablement? Thanks in advance. >> --- >> drivers/char/tpm/tpm_crb.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c >> index 717b6b4..86f355b 100644 >> --- a/drivers/char/tpm/tpm_crb.c >> +++ b/drivers/char/tpm/tpm_crb.c >> @@ -264,10 +264,12 @@ static bool crb_req_canceled(struct tpm_chip *chip, u8 status) >> static int crb_check_resource(struct acpi_resource *ares, void *data) >> { >> struct resource *io_res = data; >> - struct resource res; >> + struct resource_win win; >> + struct resource *res = &(win.res); >> >> - if (acpi_dev_resource_memory(ares, &res)) { >> - *io_res = res; >> + if (acpi_dev_resource_memory(ares, res) || >> + acpi_dev_resource_address_space(ares, &win)) { >> + *io_res = *res; >> io_res->name = NULL; >> } >> >> -- >> Jiandi An >> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. >> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. >> > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > _______________________________________________ > tpmdd-devel mailing list > tpmdd-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel > -- Jiandi An Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [tpmdd-devel] [PATCH] tpm, tpm_crb: Handle 64-bit resource in crb_check_resource() 2016-12-20 6:19 ` [tpmdd-devel] " Jiandi An @ 2017-01-03 17:11 ` Jarkko Sakkinen 0 siblings, 0 replies; 7+ messages in thread From: Jarkko Sakkinen @ 2017-01-03 17:11 UTC (permalink / raw) To: Jiandi An; +Cc: Jason Gunthorpe, tpmdd-devel, linux-kernel On Tue, Dec 20, 2016 at 12:19:27AM -0600, Jiandi An wrote: > The control area buffer is specified in the TPM2.0 static ACPI table. TPM > CRB driver maps the control area address and reads out cmd and rsp buffer > addresses and maps them. There is no requirement in the TCG TPM ACPI spec > for specifying _CRS to describe the control area buffer. When I rebased > the early prototype for CRB driver ARM64 enablement to the latest kernel, > I hit this issue where I have to specify _CRS method. So in _CRS method > I specify the same control area address that's in the TPM2.0 static ACPI table. > > I see the _CRS requirement in the CRB driver is for resource synchronization > between the TIS and CRB drivers to support force mode in TIS. Could I get some > background on that so I could be more careful not breaking TIS while making > changes to CRB driver for ARM64 enablement? At least couple of reasons: - _CRS is required to access locality registers (check my patch set that is waiting for review/testing). - On x86 and PTT _CRS always exists and that's the only platform where this driver has been used so far. /Jarkko ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tpm, tpm_crb: Handle 64-bit resource in crb_check_resource() 2016-12-19 4:20 [PATCH] tpm, tpm_crb: Handle 64-bit resource in crb_check_resource() Jiandi An 2016-12-19 13:56 ` Jarkko Sakkinen @ 2017-01-03 18:27 ` Jarkko Sakkinen 2017-01-09 18:43 ` Jarkko Sakkinen 1 sibling, 1 reply; 7+ messages in thread From: Jarkko Sakkinen @ 2017-01-03 18:27 UTC (permalink / raw) To: Jiandi An; +Cc: peterhuewe, tpmdd, jgunthorpe, tpmdd-devel, linux-kernel On Sun, Dec 18, 2016 at 10:20:53PM -0600, Jiandi An wrote: > crb_check_resource() in TPM CRB driver calls > acpi_dev_resource_memory() which only handles 32-bit resources. > Adding a call to acpi_dev_resource_address_space() in TPM CRB > driver which handles 64-bit resources. > > Signed-off-by: Jiandi An <anjiandi@codeaurora.org> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko > --- > drivers/char/tpm/tpm_crb.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > index 717b6b4..86f355b 100644 > --- a/drivers/char/tpm/tpm_crb.c > +++ b/drivers/char/tpm/tpm_crb.c > @@ -264,10 +264,12 @@ static bool crb_req_canceled(struct tpm_chip *chip, u8 status) > static int crb_check_resource(struct acpi_resource *ares, void *data) > { > struct resource *io_res = data; > - struct resource res; > + struct resource_win win; > + struct resource *res = &(win.res); > > - if (acpi_dev_resource_memory(ares, &res)) { > - *io_res = res; > + if (acpi_dev_resource_memory(ares, res) || > + acpi_dev_resource_address_space(ares, &win)) { > + *io_res = *res; > io_res->name = NULL; > } > > -- > Jiandi An > Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. > Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tpm, tpm_crb: Handle 64-bit resource in crb_check_resource() 2017-01-03 18:27 ` Jarkko Sakkinen @ 2017-01-09 18:43 ` Jarkko Sakkinen 0 siblings, 0 replies; 7+ messages in thread From: Jarkko Sakkinen @ 2017-01-09 18:43 UTC (permalink / raw) To: Jiandi An; +Cc: peterhuewe, tpmdd, jgunthorpe, tpmdd-devel, linux-kernel On Tue, Jan 03, 2017 at 08:27:12PM +0200, Jarkko Sakkinen wrote: > On Sun, Dec 18, 2016 at 10:20:53PM -0600, Jiandi An wrote: > > crb_check_resource() in TPM CRB driver calls > > acpi_dev_resource_memory() which only handles 32-bit resources. > > Adding a call to acpi_dev_resource_address_space() in TPM CRB > > driver which handles 64-bit resources. > > > > Signed-off-by: Jiandi An <anjiandi@codeaurora.org> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-01-09 18:43 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-12-19 4:20 [PATCH] tpm, tpm_crb: Handle 64-bit resource in crb_check_resource() Jiandi An 2016-12-19 13:56 ` Jarkko Sakkinen 2016-12-19 16:22 ` Jason Gunthorpe 2016-12-20 6:19 ` [tpmdd-devel] " Jiandi An 2017-01-03 17:11 ` Jarkko Sakkinen 2017-01-03 18:27 ` Jarkko Sakkinen 2017-01-09 18:43 ` Jarkko Sakkinen
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).