| Message ID | 20200707040522.4013885-2-stefanb@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] tpm: tpm_spapr: Exit on TPM backend failures | expand |
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) >
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) > > >
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.
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 :)
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)
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(-)