[Qemu-devel] [PATCH 0/2] tpm: Handle failure mode of backend device better

[Qemu-devel] [PATCH 0/2] tpm: Handle failure mode of backend device better

[Stefan Berger]

The following two patches fix the case that the backend device, e.g.,
tpm_emulator, could not be initialized and the TIS frontend needs to
go into failure mode.

   Stefan

Stefan Berger (2):
  tpm_tis: Return TPM_VERSION_UNSPEC in case of BE failure
  tpm_tis: Return 0 for every register in case of failure mode

 hw/tpm/tpm_tis.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.5.5

[Qemu-devel] [PATCH 1/2] tpm_tis: Return TPM_VERSION_UNSPEC in case of BE failure

[Stefan Berger]

In case the backend has a failure, such as the tpm_emulator's CMD_INIT
failing, the TIS goes into failure mode and does not respond to reads
or writes to MMIO registers. In this case we need to prevent the ACPI
table from being added and the straight-forward way is to indicate that
there's no known TPM version being used.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 hw/tpm/tpm_tis.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index c0a0204..eca3374 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -970,6 +970,10 @@ static enum TPMVersion tpm_tis_get_tpm_version(TPMIf *ti)
 {
     TPMState *s = TPM(ti);
 
+    if (tpm_backend_had_startup_error(s->be_driver)) {
+        return TPM_VERSION_UNSPEC;
+    }
+
     return tpm_backend_get_tpm_version(s->be_driver);
 }
 
-- 
2.5.5

Re: [Qemu-devel] [PATCH 1/2] tpm_tis: Return TPM_VERSION_UNSPEC in case of BE failure

[Marc-André Lureau]

On Sat, Nov 11, 2017 at 4:39 AM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> In case the backend has a failure, such as the tpm_emulator's CMD_INIT
> failing, the TIS goes into failure mode and does not respond to reads
> or writes to MMIO registers. In this case we need to prevent the ACPI
> table from being added and the straight-forward way is to indicate that
> there's no known TPM version being used.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  hw/tpm/tpm_tis.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index c0a0204..eca3374 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -970,6 +970,10 @@ static enum TPMVersion tpm_tis_get_tpm_version(TPMIf *ti)
>  {
>      TPMState *s = TPM(ti);
>
> +    if (tpm_backend_had_startup_error(s->be_driver)) {
> +        return TPM_VERSION_UNSPEC;
> +    }
> +
>      return tpm_backend_get_tpm_version(s->be_driver);
>  }
>
> --
> 2.5.5
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 1/2] tpm_tis: Return TPM_VERSION_UNSPEC in case of BE failure

[Stefan Berger]

On 12/13/2017 05:23 AM, Marc-André Lureau wrote:
> On Sat, Nov 11, 2017 at 4:39 AM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> In case the backend has a failure, such as the tpm_emulator's CMD_INIT
>> failing, the TIS goes into failure mode and does not respond to reads
>> or writes to MMIO registers. In this case we need to prevent the ACPI
>> table from being added and the straight-forward way is to indicate that
>> there's no known TPM version being used.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Resending an old response?

https://patchwork.kernel.org/patch/10058405/

>
>
>> ---
>>   hw/tpm/tpm_tis.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
>> index c0a0204..eca3374 100644
>> --- a/hw/tpm/tpm_tis.c
>> +++ b/hw/tpm/tpm_tis.c
>> @@ -970,6 +970,10 @@ static enum TPMVersion tpm_tis_get_tpm_version(TPMIf *ti)
>>   {
>>       TPMState *s = TPM(ti);
>>
>> +    if (tpm_backend_had_startup_error(s->be_driver)) {
>> +        return TPM_VERSION_UNSPEC;
>> +    }
>> +
>>       return tpm_backend_get_tpm_version(s->be_driver);
>>   }
>>
>> --
>> 2.5.5
>>
>>
>
>

[Qemu-devel] [PATCH 2/2] tpm_tis: Return 0 for every register in case of failure mode

[Stefan Berger]

Rather than returning ~0, return 0 for every register in case of
failure mode. The '0' is better to indicate that there's no device
there.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 hw/tpm/tpm_tis.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index eca3374..d0bdd96 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -519,7 +519,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
     uint8_t v;
 
     if (tpm_backend_had_startup_error(s->be_driver)) {
-        return val;
+        return 0;
     }
 
     switch (offset) {
-- 
2.5.5

Re: [Qemu-devel] [PATCH 2/2] tpm_tis: Return 0 for every register in case of failure mode

[Marc-André Lureau]

Hi Stefan

On Sat, Nov 11, 2017 at 4:39 AM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Rather than returning ~0, return 0 for every register in case of
> failure mode. The '0' is better to indicate that there's no device
> there.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  hw/tpm/tpm_tis.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index eca3374..d0bdd96 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -519,7 +519,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
>      uint8_t v;
>
>      if (tpm_backend_had_startup_error(s->be_driver)) {
> -        return val;
> +        return 0;
>      }

it's not obvious to me why it's better. Could you detail a bit more the reason?

why not change the initialization of val instead?

>
>      switch (offset) {
> --
> 2.5.5
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 2/2] tpm_tis: Return 0 for every register in case of failure mode

[Stefan Berger]

On 12/13/2017 05:26 AM, Marc-André Lureau wrote:
> Hi Stefan
>
> On Sat, Nov 11, 2017 at 4:39 AM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> Rather than returning ~0, return 0 for every register in case of
>> failure mode. The '0' is better to indicate that there's no device
>> there.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>   hw/tpm/tpm_tis.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
>> index eca3374..d0bdd96 100644
>> --- a/hw/tpm/tpm_tis.c
>> +++ b/hw/tpm/tpm_tis.c
>> @@ -519,7 +519,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
>>       uint8_t v;
>>
>>       if (tpm_backend_had_startup_error(s->be_driver)) {
>> -        return val;
>> +        return 0;
>>       }
> it's not obvious to me why it's better. Could you detail a bit more the reason?
>
> why not change the initialization of val instead?

http://patchwork.ozlabs.org/patch/838042/