1. Patchew
  2. QEMU
  3. [PATCH v2 0/2] tpm: Some fixes
  4. View patch

[PATCH v2 1/2] tpm: tpm_spapr: Exit on TPM backend failures

Stefan Berger posted 2 patches 5 years, 7 months ago
Maintainers: Stefan Berger <stefanb@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Laurent Vivier <lvivier@redhat.com>
There is a newer version of this series
[PATCH v2 1/2] tpm: tpm_spapr: Exit on TPM backend failures
Posted by Stefan Berger 5 years, 7 months ago 
Exit on TPM backend failures in the same way as the TPM CRB and TIS device
models do.

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

diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
index cb4dfd1e6a..8288ab0a15 100644
--- a/hw/tpm/tpm_spapr.c
+++ b/hw/tpm/tpm_spapr.c
@@ -306,7 +306,10 @@ static void tpm_spapr_reset(SpaprVioDevice *dev)
                             TPM_SPAPR_BUFFER_MAX);
 
     tpm_backend_reset(s->be_driver);
-    tpm_spapr_do_startup_tpm(s, s->be_buffer_size);
+
+    if (tpm_spapr_do_startup_tpm(s, s->be_buffer_size) < 0) {
+        exit(1);
+    }
 }
 
 static enum TPMVersion tpm_spapr_get_version(TPMIf *ti)
-- 
2.24.1
Re: [PATCH v2 1/2] tpm: tpm_spapr: Exit on TPM backend failures
Posted by Philippe Mathieu-Daudé 5 years, 7 months ago 
Hi Stefan,

On 7/7/20 6:05 AM, Stefan Berger wrote:
> Exit on TPM backend failures in the same way as the TPM CRB and TIS device
> models do.

Maybe the other models are not the best examples ;)

> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  hw/tpm/tpm_spapr.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
> index cb4dfd1e6a..8288ab0a15 100644
> --- a/hw/tpm/tpm_spapr.c
> +++ b/hw/tpm/tpm_spapr.c
> @@ -306,7 +306,10 @@ static void tpm_spapr_reset(SpaprVioDevice *dev)
>                              TPM_SPAPR_BUFFER_MAX);
>  
>      tpm_backend_reset(s->be_driver);
> -    tpm_spapr_do_startup_tpm(s, s->be_buffer_size);
> +
> +    if (tpm_spapr_do_startup_tpm(s, s->be_buffer_size) < 0) {

I don't see error reported, how users can know the cause of the exit?

> +        exit(1);

What about using this instead?

           qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);

> +    }
>  }
>  
>  static enum TPMVersion tpm_spapr_get_version(TPMIf *ti)
>
Re: [PATCH v2 1/2] tpm: tpm_spapr: Exit on TPM backend failures
Posted by David Gibson 5 years, 7 months ago 
On Tue, Jul 07, 2020 at 06:20:49AM +0200, Philippe Mathieu-Daudé wrote:
> Hi Stefan,
> 
> On 7/7/20 6:05 AM, Stefan Berger wrote:
> > Exit on TPM backend failures in the same way as the TPM CRB and TIS device
> > models do.
> 
> Maybe the other models are not the best examples ;)
> 
> > 
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > ---
> >  hw/tpm/tpm_spapr.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
> > index cb4dfd1e6a..8288ab0a15 100644
> > --- a/hw/tpm/tpm_spapr.c
> > +++ b/hw/tpm/tpm_spapr.c
> > @@ -306,7 +306,10 @@ static void tpm_spapr_reset(SpaprVioDevice *dev)
> >                              TPM_SPAPR_BUFFER_MAX);
> >  
> >      tpm_backend_reset(s->be_driver);
> > -    tpm_spapr_do_startup_tpm(s, s->be_buffer_size);
> > +
> > +    if (tpm_spapr_do_startup_tpm(s, s->be_buffer_size) < 0) {
> 
> I don't see error reported, how users can know the cause of the exit?
> 
> > +        exit(1);
> 
> What about using this instead?
> 
>            qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);

Hrm.  I'm not entirely convinced that's what we want.  But we
definitely need some sort of error reported.

> 
> > +    }
> >  }
> >  
> >  static enum TPMVersion tpm_spapr_get_version(TPMIf *ti)
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [PATCH v2 1/2] tpm: tpm_spapr: Exit on TPM backend failures
Posted by Stefan Berger 5 years, 7 months ago 
On 7/7/20 12:20 AM, Philippe Mathieu-Daudé wrote:
> Hi Stefan,
>
> On 7/7/20 6:05 AM, Stefan Berger wrote:
>> Exit on TPM backend failures in the same way as the TPM CRB and TIS device
>> models do.
> Maybe the other models are not the best examples ;)

At least they are known to report the error...


>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   hw/tpm/tpm_spapr.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
>> index cb4dfd1e6a..8288ab0a15 100644
>> --- a/hw/tpm/tpm_spapr.c
>> +++ b/hw/tpm/tpm_spapr.c
>> @@ -306,7 +306,10 @@ static void tpm_spapr_reset(SpaprVioDevice *dev)
>>                               TPM_SPAPR_BUFFER_MAX);
>>   
>>       tpm_backend_reset(s->be_driver);
>> -    tpm_spapr_do_startup_tpm(s, s->be_buffer_size);
>> +
>> +    if (tpm_spapr_do_startup_tpm(s, s->be_buffer_size) < 0) {
> I don't see error reported, how users can know the cause of the exit?


virt-manager does report the error then. It seems to be taking it from 
the last error message reported in the emulator backend when TPM_INIT 
fails with error code 0x101:

error: internal error: qemu unexpectedly closed the monitor: 
2020-07-07T12:49:28.333928Z qemu-system-ppc64: tpm-emulator: TPM result 
for CMD_INIT: 0x101 operation failed


>
>> +        exit(1);
> What about using this instead?
>
>             qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);

It doesn't have any effect, the VM just keeps on running. So the exit(1) 
is better and does report an error.
Re: [PATCH v2 1/2] tpm: tpm_spapr: Exit on TPM backend failures
Posted by Philippe Mathieu-Daudé 5 years, 7 months ago 
On 7/7/20 2:52 PM, Stefan Berger wrote:
> On 7/7/20 12:20 AM, Philippe Mathieu-Daudé wrote:
>> Hi Stefan,
>>
>> On 7/7/20 6:05 AM, Stefan Berger wrote:
>>> Exit on TPM backend failures in the same way as the TPM CRB and TIS
>>> device
>>> models do.
>> Maybe the other models are not the best examples ;)
> 
> At least they are known to report the error...
> 
> 
>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>> ---
>>>   hw/tpm/tpm_spapr.c | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
>>> index cb4dfd1e6a..8288ab0a15 100644
>>> --- a/hw/tpm/tpm_spapr.c
>>> +++ b/hw/tpm/tpm_spapr.c
>>> @@ -306,7 +306,10 @@ static void tpm_spapr_reset(SpaprVioDevice *dev)
>>>                               TPM_SPAPR_BUFFER_MAX);
>>>         tpm_backend_reset(s->be_driver);
>>> -    tpm_spapr_do_startup_tpm(s, s->be_buffer_size);
>>> +
>>> +    if (tpm_spapr_do_startup_tpm(s, s->be_buffer_size) < 0) {
>> I don't see error reported, how users can know the cause of the exit?
> 
> 
> virt-manager does report the error then. It seems to be taking it from
> the last error message reported in the emulator backend when TPM_INIT
> fails with error code 0x101:
> 
> error: internal error: qemu unexpectedly closed the monitor:
> 2020-07-07T12:49:28.333928Z qemu-system-ppc64: tpm-emulator: TPM result
> for CMD_INIT: 0x101 operation failed

Ah, good.

> 
>>
>>> +        exit(1);
>> What about using this instead?
>>
>>             qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);
> 
> It doesn't have any effect, the VM just keeps on running. So the exit(1)
> is better and does report an error.
> 

Hmm maybe something is missing or it was never totally implemented?

Anyway since virt-manager is notified, I'm not objecting to this patch
:)

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.