[PATCH] x86: tpm: Remove a busy bit of the NVS area for supporting AMD's fTPM

[PATCH] x86: tpm: Remove a busy bit of the NVS area for supporting AMD's fTPM

[Seunghun Han]

I'm Seunghun Han and work at the Affiliated Institute of ETRI. 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 e820 map and 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 out that a busy bit was set to
the ACPI NVS area, and the Linux kernel didn't allow the TPM CRB driver
to assign CRB regions in it.

To support AMD's fTPM, I removed the busy bit from the ACPI NVS area like
the reserved area so that AMD's fTPM regions could be assigned in it.

Signed-off-by: Seunghun Han <kkamagui@gmail.com>
---
 arch/x86/kernel/e820.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 7da2bcd2b8eb..0d721df8900e 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -1085,11 +1085,12 @@ static bool __init do_mark_busy(enum e820_type type, struct resource *res)
 	case E820_TYPE_RESERVED:
 	case E820_TYPE_PRAM:
 	case E820_TYPE_PMEM:
+	/* AMD's fTPM regions are in the ACPI NVS area */
+	case E820_TYPE_NVS:
 		return false;
 	case E820_TYPE_RESERVED_KERN:
 	case E820_TYPE_RAM:
 	case E820_TYPE_ACPI:
-	case E820_TYPE_NVS:
 	case E820_TYPE_UNUSABLE:
 	default:
 		return true;
-- 
2.21.0

Re: [PATCH] x86: tpm: Remove a busy bit of the NVS area for supporting AMD's fTPM

[Matthew Garrett]

On Mon, Aug 26, 2019 at 1:18 AM Seunghun Han <kkamagui@gmail.com> wrote:
> To support AMD's fTPM, I removed the busy bit from the ACPI NVS area like
> the reserved area so that AMD's fTPM regions could be assigned in it.

drivers/acpi/nvs.c saves and restores the contents of NVS regions, and
if other drivers use these regions without any awareness of this then
things may break. I'm reluctant to say that just unilaterally marking
these regions as available is a good thing, but it's clearly what's
expected by AMD's implementation. One approach would be to have a
callback into the nvs code to indicate that a certain region should be
handed off to a driver, which would ensure that we can handle this on
a case by case basis?

Re: [PATCH] x86: tpm: Remove a busy bit of the NVS area for supporting AMD's fTPM

[Seunghun Han]

>
> On Mon, Aug 26, 2019 at 1:18 AM Seunghun Han <kkamagui@gmail.com> wrote:
> > To support AMD's fTPM, I removed the busy bit from the ACPI NVS area like
> > the reserved area so that AMD's fTPM regions could be assigned in it.
>
> drivers/acpi/nvs.c saves and restores the contents of NVS regions, and
> if other drivers use these regions without any awareness of this then
> things may break. I'm reluctant to say that just unilaterally marking
> these regions as available is a good thing, but it's clearly what's
> expected by AMD's implementation. One approach would be to have a
> callback into the nvs code to indicate that a certain region should be
> handed off to a driver, which would ensure that we can handle this on
> a case by case basis?

If the regions allocated in the NVS region need to be handled by a
driver, the callback mechanism is good for it. However, this case
doesn't need it because the regions allocated in NVS are just I/O
regions.

In my opinion, if the driver wants to handle the region in the NVS
while suspending or hibernating, it has to use register_pm_notifier()
function and handle the event. We already had the mechanism that could
ensure that the cases you worried about would be handled, so it seems
to me that removing the busy bit from the NVS region is fine.

Seunghun

Re: [PATCH] x86: tpm: Remove a busy bit of the NVS area for supporting AMD's fTPM

[Matthew Garrett]

On Tue, Aug 27, 2019 at 1:23 AM Seunghun Han <kkamagui@gmail.com> wrote:
> If the regions allocated in the NVS region need to be handled by a
> driver, the callback mechanism is good for it. However, this case
> doesn't need it because the regions allocated in NVS are just I/O
> regions.
>
> In my opinion, if the driver wants to handle the region in the NVS
> while suspending or hibernating, it has to use register_pm_notifier()
> function and handle the event. We already had the mechanism that could
> ensure that the cases you worried about would be handled, so it seems
> to me that removing the busy bit from the NVS region is fine.

No. The NVS regions are regions that need to be saved and restored
over hibernation, but which aren't otherwise handled by a driver -
that's why the NVS code exists. If drivers are allowed to bind to NVS
regions without explicit handling, they risk conflicting with that.

Re: [PATCH] x86: tpm: Remove a busy bit of the NVS area for supporting AMD's fTPM

[Seunghun Han]

> On Tue, Aug 27, 2019 at 1:23 AM Seunghun Han <kkamagui@gmail.com> wrote:
> > If the regions allocated in the NVS region need to be handled by a
> > driver, the callback mechanism is good for it. However, this case
> > doesn't need it because the regions allocated in NVS are just I/O
> > regions.
> >
> > In my opinion, if the driver wants to handle the region in the NVS
> > while suspending or hibernating, it has to use register_pm_notifier()
> > function and handle the event. We already had the mechanism that could
> > ensure that the cases you worried about would be handled, so it seems
> > to me that removing the busy bit from the NVS region is fine.
>
> No. The NVS regions are regions that need to be saved and restored
> over hibernation, but which aren't otherwise handled by a driver -
> that's why the NVS code exists. If drivers are allowed to bind to NVS
> regions without explicit handling, they risk conflicting with that.

I got your point. Is there any problem if some regions which don't
need to be handled in NVS area are saved and restored? If there is a
problem, how about adding code for ignoring the regions in NVS area to
the nvs.c file like Jarkko said? If we add the code, we can save and
restore NVS area without driver's interaction.

Seunghun

Re: [PATCH] x86: tpm: Remove a busy bit of the NVS area for supporting AMD's fTPM

[Matthew Garrett]

On Wed, Aug 28, 2019 at 01:36:30AM +0900, Seunghun Han wrote:

> I got your point. Is there any problem if some regions which don't
> need to be handled in NVS area are saved and restored? If there is a
> problem, how about adding code for ignoring the regions in NVS area to
> the nvs.c file like Jarkko said? If we add the code, we can save and
> restore NVS area without driver's interaction.

The only thing that knows which regions should be skipped by the NVS 
driver is the hardware specific driver, so the TPM driver needs to ask 
the NVS driver to ignore that region and grant control to the TPM 
driver.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] x86: tpm: Remove a busy bit of the NVS area for supporting AMD's fTPM

[Seunghun Han]

>
> On Wed, Aug 28, 2019 at 01:36:30AM +0900, Seunghun Han wrote:
>
> > I got your point. Is there any problem if some regions which don't
> > need to be handled in NVS area are saved and restored? If there is a
> > problem, how about adding code for ignoring the regions in NVS area to
> > the nvs.c file like Jarkko said? If we add the code, we can save and
> > restore NVS area without driver's interaction.
>
> The only thing that knows which regions should be skipped by the NVS
> driver is the hardware specific driver, so the TPM driver needs to ask
> the NVS driver to ignore that region and grant control to the TPM
> driver.
>
> --
> Matthew Garrett | mjg59@srcf.ucam.org

Thank you, Matthew and Jarkko.
It seems that the TPM driver needs to handle the specific case that
TPM regions are in the NVS. I would make a patch that removes TPM
regions from the ACPI NVS by requesting to the NVS driver soon.

Jarkko,
I would like to get some advice on it. What do you think about
removing TPM regions from the ACPI NVS in TPM CRB driver? If you don't
mind, I would make the patch about it.

Seunghun

Re: [PATCH] x86: tpm: Remove a busy bit of the NVS area for supporting AMD's fTPM

[Jarkko Sakkinen]

On Wed, Aug 28, 2019 at 06:36:04PM +0900, Seunghun Han wrote:
> >
> > On Wed, Aug 28, 2019 at 01:36:30AM +0900, Seunghun Han wrote:
> >
> > > I got your point. Is there any problem if some regions which don't
> > > need to be handled in NVS area are saved and restored? If there is a
> > > problem, how about adding code for ignoring the regions in NVS area to
> > > the nvs.c file like Jarkko said? If we add the code, we can save and
> > > restore NVS area without driver's interaction.
> >
> > The only thing that knows which regions should be skipped by the NVS
> > driver is the hardware specific driver, so the TPM driver needs to ask
> > the NVS driver to ignore that region and grant control to the TPM
> > driver.
> >
> > --
> > Matthew Garrett | mjg59@srcf.ucam.org
> 
> Thank you, Matthew and Jarkko.
> It seems that the TPM driver needs to handle the specific case that
> TPM regions are in the NVS. I would make a patch that removes TPM
> regions from the ACPI NVS by requesting to the NVS driver soon.
> 
> Jarkko,
> I would like to get some advice on it. What do you think about
> removing TPM regions from the ACPI NVS in TPM CRB driver? If you don't
> mind, I would make the patch about it.

I'm not sure if ignoring is right call. Then the hibernation behaviour
for TPM regions would break.

Thus, should be "ask access" rather than "grant control".

/Jarkko

Re: [PATCH] x86: tpm: Remove a busy bit of the NVS area for supporting AMD's fTPM

[Jarkko Sakkinen]

On Thu, Aug 29, 2019 at 06:34:37PM +0300, Jarkko Sakkinen wrote:
> On Wed, Aug 28, 2019 at 06:36:04PM +0900, Seunghun Han wrote:
> > >
> > > On Wed, Aug 28, 2019 at 01:36:30AM +0900, Seunghun Han wrote:
> > >
> > > > I got your point. Is there any problem if some regions which don't
> > > > need to be handled in NVS area are saved and restored? If there is a
> > > > problem, how about adding code for ignoring the regions in NVS area to
> > > > the nvs.c file like Jarkko said? If we add the code, we can save and
> > > > restore NVS area without driver's interaction.
> > >
> > > The only thing that knows which regions should be skipped by the NVS
> > > driver is the hardware specific driver, so the TPM driver needs to ask
> > > the NVS driver to ignore that region and grant control to the TPM
> > > driver.
> > >
> > > --
> > > Matthew Garrett | mjg59@srcf.ucam.org
> > 
> > Thank you, Matthew and Jarkko.
> > It seems that the TPM driver needs to handle the specific case that
> > TPM regions are in the NVS. I would make a patch that removes TPM
> > regions from the ACPI NVS by requesting to the NVS driver soon.
> > 
> > Jarkko,
> > I would like to get some advice on it. What do you think about
> > removing TPM regions from the ACPI NVS in TPM CRB driver? If you don't
> > mind, I would make the patch about it.
> 
> I'm not sure if ignoring is right call. Then the hibernation behaviour
> for TPM regions would break.
> 
> Thus, should be "ask access" rather than "grant control".

Or "reserve access" as NVS driver does not have intelligence to do any
policy based decision here.

A function that gets region and then checks if NVS driver has matching
one and returns true/false based on that should be good enough. Then
you raw ioremap() in the TPM driver.

/Jarkko

Re: [PATCH] x86: tpm: Remove a busy bit of the NVS area for supporting AMD's fTPM

[Seunghun Han]

>
> On Thu, Aug 29, 2019 at 06:34:37PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Aug 28, 2019 at 06:36:04PM +0900, Seunghun Han wrote:
> > > >
> > > > On Wed, Aug 28, 2019 at 01:36:30AM +0900, Seunghun Han wrote:
> > > >
> > > > > I got your point. Is there any problem if some regions which don't
> > > > > need to be handled in NVS area are saved and restored? If there is a
> > > > > problem, how about adding code for ignoring the regions in NVS area to
> > > > > the nvs.c file like Jarkko said? If we add the code, we can save and
> > > > > restore NVS area without driver's interaction.
> > > >
> > > > The only thing that knows which regions should be skipped by the NVS
> > > > driver is the hardware specific driver, so the TPM driver needs to ask
> > > > the NVS driver to ignore that region and grant control to the TPM
> > > > driver.
> > > >
> > > > --
> > > > Matthew Garrett | mjg59@srcf.ucam.org
> > >
> > > Thank you, Matthew and Jarkko.
> > > It seems that the TPM driver needs to handle the specific case that
> > > TPM regions are in the NVS. I would make a patch that removes TPM
> > > regions from the ACPI NVS by requesting to the NVS driver soon.
> > >
> > > Jarkko,
> > > I would like to get some advice on it. What do you think about
> > > removing TPM regions from the ACPI NVS in TPM CRB driver? If you don't
> > > mind, I would make the patch about it.
> >
> > I'm not sure if ignoring is right call. Then the hibernation behaviour
> > for TPM regions would break.
> >
> > Thus, should be "ask access" rather than "grant control".

I agree with your idea. It seems to make trouble. So, I would like to
do like your idea below.

> Or "reserve access" as NVS driver does not have intelligence to do any
> policy based decision here.
>
> A function that gets region and then checks if NVS driver has matching
> one and returns true/false based on that should be good enough. Then
> you raw ioremap() in the TPM driver.
>
> /Jarkko

This solution is great and clear to me. I will make a new patch on
your advice and test it in my machine. After that, I will send it
again soon.
I really appreciate it.

Seunghun

Re: [PATCH] x86: tpm: Remove a busy bit of the NVS area for supporting AMD's fTPM

[Seunghun Han]

> > On Thu, Aug 29, 2019 at 06:34:37PM +0300, Jarkko Sakkinen wrote:
> > > On Wed, Aug 28, 2019 at 06:36:04PM +0900, Seunghun Han wrote:
> > > > >
> > > > > On Wed, Aug 28, 2019 at 01:36:30AM +0900, Seunghun Han wrote:
> > > > >
> > > > > > I got your point. Is there any problem if some regions which don't
> > > > > > need to be handled in NVS area are saved and restored? If there is a
> > > > > > problem, how about adding code for ignoring the regions in NVS area to
> > > > > > the nvs.c file like Jarkko said? If we add the code, we can save and
> > > > > > restore NVS area without driver's interaction.
> > > > >
> > > > > The only thing that knows which regions should be skipped by the NVS
> > > > > driver is the hardware specific driver, so the TPM driver needs to ask
> > > > > the NVS driver to ignore that region and grant control to the TPM
> > > > > driver.
> > > > >
> > > > > --
> > > > > Matthew Garrett | mjg59@srcf.ucam.org
> > > >
> > > > Thank you, Matthew and Jarkko.
> > > > It seems that the TPM driver needs to handle the specific case that
> > > > TPM regions are in the NVS. I would make a patch that removes TPM
> > > > regions from the ACPI NVS by requesting to the NVS driver soon.
> > > >
> > > > Jarkko,
> > > > I would like to get some advice on it. What do you think about
> > > > removing TPM regions from the ACPI NVS in TPM CRB driver? If you don't
> > > > mind, I would make the patch about it.
> > >
> > > I'm not sure if ignoring is right call. Then the hibernation behaviour
> > > for TPM regions would break.
> > >
> > > Thus, should be "ask access" rather than "grant control".
>
> I agree with your idea. It seems to make trouble. So, I would like to
> do like your idea below.
>
> > Or "reserve access" as NVS driver does not have intelligence to do any
> > policy based decision here.
> >
> > A function that gets region and then checks if NVS driver has matching
> > one and returns true/false based on that should be good enough. Then
> > you raw ioremap() in the TPM driver.
> >
> > /Jarkko
>
> This solution is great and clear to me. I will make a new patch on
> your advice and test it in my machine. After that, I will send it
> again soon.
> I really appreciate it.
>
> Seunghun

I have made and sent patches on your advice.
The patch links are below and please review them.
[PATCH 0/2] https://lkml.org/lkml/2019/8/30/372
[PATCH 1/2] https://lkml.org/lkml/2019/8/30/373
[PATCH 2/2] https://lkml.org/lkml/2019/8/30/374

Thank you again for your sincere advice.

Seunghun

Re: [PATCH] x86: tpm: Remove a busy bit of the NVS area for supporting AMD's fTPM

[Jarkko Sakkinen]

On Mon, Aug 26, 2019 at 10:40:25AM -0700, Matthew Garrett wrote:
> On Mon, Aug 26, 2019 at 1:18 AM Seunghun Han <kkamagui@gmail.com> wrote:
> > To support AMD's fTPM, I removed the busy bit from the ACPI NVS area like
> > the reserved area so that AMD's fTPM regions could be assigned in it.
> 
> drivers/acpi/nvs.c saves and restores the contents of NVS regions, and
> if other drivers use these regions without any awareness of this then
> things may break. I'm reluctant to say that just unilaterally marking
> these regions as available is a good thing, but it's clearly what's
> expected by AMD's implementation. One approach would be to have a
> callback into the nvs code to indicate that a certain region should be
> handed off to a driver, which would ensure that we can handle this on
> a case by case basis?

What if E820 would just have a small piece of code just for fTPM's e.g.
it would check the ACPI tree for fTPM's and ignore TPM regions.

/Jarkko

Re: [PATCH] x86: tpm: Remove a busy bit of the NVS area for supporting AMD's fTPM

[Seunghun Han]

>
> On Mon, Aug 26, 2019 at 10:40:25AM -0700, Matthew Garrett wrote:
> > On Mon, Aug 26, 2019 at 1:18 AM Seunghun Han <kkamagui@gmail.com> wrote:
> > > To support AMD's fTPM, I removed the busy bit from the ACPI NVS area like
> > > the reserved area so that AMD's fTPM regions could be assigned in it.
> >
> > drivers/acpi/nvs.c saves and restores the contents of NVS regions, and
> > if other drivers use these regions without any awareness of this then
> > things may break. I'm reluctant to say that just unilaterally marking
> > these regions as available is a good thing, but it's clearly what's
> > expected by AMD's implementation. One approach would be to have a
> > callback into the nvs code to indicate that a certain region should be
> > handed off to a driver, which would ensure that we can handle this on
> > a case by case basis?
>
> What if E820 would just have a small piece of code just for fTPM's e.g.
> it would check the ACPI tree for fTPM's and ignore TPM regions.
>
> /Jarkko

It seems that it is possible. However, the memory layout is set by
enumerating e820 table and ACPI table in order, and the memory regions
are typically added and overlapped to the existing memory layout. I
also worry about the direct interaction between the e820 table and
ACPI table. As I know, they have no straightforward interface or
relationship. So, if we make the code for identifying fTPM regions in
ACPI table and write it to e820 code, we would meet other problems
like "acpi=off" kernel option.

In my view, it is natural that ACPI NVS allows device drivers to
assign some regions in it if the hardware reports the regions there.

Seunghun