From d64072c0ac6f73413026695bb33a4aa232e69617 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 23 Jul 2020 13:58:44 +0200 Subject: Revert "tpm: Clean up error reporting in tpm_init_tpmdev()" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit d10e05f15d5c3dd5e5cc59c5dfff460d89d48580. We report some -tpmdev failures, but then continue as if all was fine. Reproducer: $ qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -chardev null,id=tpm0 -tpmdev emulator,id=tpm0,chardev=chrtpm -device tpm-tis,tpmdev=tpm0 qemu-system-x86_64: -tpmdev emulator,id=tpm0,chardev=chrtpm: tpm-emulator: tpm chardev 'chrtpm' not found. qemu-system-x86_64: -tpmdev emulator,id=tpm0,chardev=chrtpm: tpm-emulator: Could not cleanly shutdown the TPM: No such file or directory QEMU 5.0.90 monitor - type 'help' for more information (qemu) qemu-system-x86_64: -device tpm-tis,tpmdev=tpm0: Property 'tpm-tis.tpmdev' can't find value 'tpm0' $ echo $? 1 This is a regression caused by commit d10e05f15d "tpm: Clean up error reporting in tpm_init_tpmdev()". It's incomplete: be->create(opts) continues to use error_report(), and we don't set an error when it fails. I figure converting the create() methods to Error would make some sense, but I'm not sure it's worth the effort right now. Revert the broken commit instead, and add a comment to tpm_init_tpmdev(). Straightforward conflict in tpm.c resolved. Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Stefan Berger Signed-off-by: Stefan Berger --- include/sysemu/tpm.h | 2 +- softmmu/vl.c | 4 +++- stubs/tpm.c | 3 ++- tpm.c | 30 +++++++++++++++++++++--------- 4 files changed, 27 insertions(+), 12 deletions(-) diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h index 03fb25941c..730c61ac97 100644 --- a/include/sysemu/tpm.h +++ b/include/sysemu/tpm.h @@ -16,7 +16,7 @@ #include "qom/object.h" int tpm_config_parse(QemuOptsList *opts_list, const char *optarg); -void tpm_init(void); +int tpm_init(void); void tpm_cleanup(void); typedef enum TPMVersion { diff --git a/softmmu/vl.c b/softmmu/vl.c index 3416241557..660537a709 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -4258,7 +4258,9 @@ void qemu_init(int argc, char **argv, char **envp) user_creatable_add_opts_foreach, object_create_delayed, &error_fatal); - tpm_init(); + if (tpm_init() < 0) { + exit(1); + } blk_mig_init(); ram_mig_init(); diff --git a/stubs/tpm.c b/stubs/tpm.c index 66c99d667d..9bded191d9 100644 --- a/stubs/tpm.c +++ b/stubs/tpm.c @@ -10,8 +10,9 @@ #include "sysemu/tpm.h" #include "hw/acpi/tpm.h" -void tpm_init(void) +int tpm_init(void) { + return 0; } void tpm_cleanup(void) diff --git a/tpm.c b/tpm.c index fe03b24858..f6045bb6da 100644 --- a/tpm.c +++ b/tpm.c @@ -81,26 +81,33 @@ TPMBackend *qemu_find_tpm_be(const char *id) static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp) { + /* + * Use of error_report() in a function with an Error ** parameter + * is suspicious. It is okay here. The parameter only exists to + * make the function usable with qemu_opts_foreach(). It is not + * actually used. + */ const char *value; const char *id; const TPMBackendClass *be; TPMBackend *drv; + Error *local_err = NULL; int i; if (!QLIST_EMPTY(&tpm_backends)) { - error_setg(errp, "Only one TPM is allowed."); + error_report("Only one TPM is allowed."); return 1; } id = qemu_opts_id(opts); if (id == NULL) { - error_setg(errp, QERR_MISSING_PARAMETER, "id"); + error_report(QERR_MISSING_PARAMETER, "id"); return 1; } value = qemu_opt_get(opts, "type"); if (!value) { - error_setg(errp, QERR_MISSING_PARAMETER, "type"); + error_report(QERR_MISSING_PARAMETER, "type"); tpm_display_backend_drivers(); return 1; } @@ -108,14 +115,15 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp) i = qapi_enum_parse(&TpmType_lookup, value, -1, NULL); be = i >= 0 ? tpm_be_find_by_type(i) : NULL; if (be == NULL) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", - "a TPM backend type"); + error_report(QERR_INVALID_PARAMETER_VALUE, + "type", "a TPM backend type"); tpm_display_backend_drivers(); return 1; } /* validate backend specific opts */ - if (!qemu_opts_validate(opts, be->opts, errp)) { + if (!qemu_opts_validate(opts, be->opts, &local_err)) { + error_report_err(local_err); return 1; } @@ -148,10 +156,14 @@ void tpm_cleanup(void) * Initialize the TPM. Process the tpmdev command line options describing the * TPM backend. */ -void tpm_init(void) +int tpm_init(void) { - qemu_opts_foreach(qemu_find_opts("tpmdev"), - tpm_init_tpmdev, NULL, &error_fatal); + if (qemu_opts_foreach(qemu_find_opts("tpmdev"), + tpm_init_tpmdev, NULL, NULL)) { + return -1; + } + + return 0; } /* -- cgit v1.2.3 From 5a6791c3d4a7719e8d0797afe0e2822a54bda6a2 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 23 Jul 2020 13:58:45 +0200 Subject: tpm: Improve help on TPM types when none are available MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Help is a bit awkward when no TPM types are built into QEMU: $ qemu-system-x86_64 -tpmdev nonexistent,id=tpm0 qemu-system-x86_64: -tpmdev nonexistent,id=tpm0: Parameter 'type' expects a TPM backend type Supported TPM types (choose only one): Improve to qemu-system-x86_64: -tpmdev nonexistent,id=tpm0: Parameter 'type' expects a TPM backend type No TPM backend types are available Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Stefan Berger Signed-off-by: Stefan Berger --- tpm.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/tpm.c b/tpm.c index f6045bb6da..cab206355a 100644 --- a/tpm.c +++ b/tpm.c @@ -47,18 +47,23 @@ tpm_be_find_by_type(enum TpmType type) */ static void tpm_display_backend_drivers(void) { + bool got_one = false; int i; - fprintf(stderr, "Supported TPM types (choose only one):\n"); - for (i = 0; i < TPM_TYPE__MAX; i++) { const TPMBackendClass *bc = tpm_be_find_by_type(i); if (!bc) { continue; } - fprintf(stderr, "%12s %s\n", TpmType_str(i), bc->desc); + if (!got_one) { + error_printf("Supported TPM types (choose only one):\n"); + got_one = true; + } + error_printf("%12s %s\n", TpmType_str(i), bc->desc); + } + if (!got_one) { + error_printf("No TPM backend types are available\n"); } - fprintf(stderr, "\n"); } /* -- cgit v1.2.3 From 88f830745721ba8c9e9d2831c01045a6f130c1a6 Mon Sep 17 00:00:00 2001 From: Stefan Berger Date: Fri, 24 Jul 2020 08:57:26 -0400 Subject: tpm_emulator: Report an error if chardev is missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch fixes the odd error reporting when trying to send a file descriptor to the TPM emulator if one has not passed a valid chardev. $ x86_64-softmmu/qemu-system-x86_64 -tpmdev emulator,id=tpm0 qemu-system-x86_64: -tpmdev emulator,id=tpm0: tpm-emulator: Failed to send CMD_SET_DATAFD: Success qemu-system-x86_64: -tpmdev emulator,id=tpm0: tpm-emulator: Could not cleanly shutdown the TPM: Success This is the new error report: $ x86_64-softmmu/qemu-system-x86_64 -tpmdev emulator,id=tpm0 qemu-system-x86_64: -tpmdev emulator,id=tpm0: tpm-emulator: parameter 'chardev' is missing This change does not hide the display of supported TPM types if a non-existent type is passed: $ x86_64-softmmu/qemu-system-x86_64 -tpmdev nonexistent,id=tpm0 qemu-system-x86_64: -tpmdev nonexistent,id=tpm0: Parameter 'type' expects a TPM backend type Supported TPM types (choose only one): passthrough Passthrough TPM backend driver emulator TPM emulator backend driver Signed-off-by: Stefan Berger Reviewed-by: Marc-André Lureau Reviewed-by: Markus Armbruster --- backends/tpm/tpm_emulator.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c index 9605339f93..a9b0f55e67 100644 --- a/backends/tpm/tpm_emulator.c +++ b/backends/tpm/tpm_emulator.c @@ -549,27 +549,30 @@ err_exit: static int tpm_emulator_handle_device_opts(TPMEmulator *tpm_emu, QemuOpts *opts) { const char *value; + Error *err = NULL; + Chardev *dev; value = qemu_opt_get(opts, "chardev"); - if (value) { - Error *err = NULL; - Chardev *dev = qemu_chr_find(value); - - if (!dev) { - error_report("tpm-emulator: tpm chardev '%s' not found.", value); - goto err; - } + if (!value) { + error_report("tpm-emulator: parameter 'chardev' is missing"); + goto err; + } - if (!qemu_chr_fe_init(&tpm_emu->ctrl_chr, dev, &err)) { - error_prepend(&err, "tpm-emulator: No valid chardev found at '%s':", - value); - error_report_err(err); - goto err; - } + dev = qemu_chr_find(value); + if (!dev) { + error_report("tpm-emulator: tpm chardev '%s' not found", value); + goto err; + } - tpm_emu->options->chardev = g_strdup(value); + if (!qemu_chr_fe_init(&tpm_emu->ctrl_chr, dev, &err)) { + error_prepend(&err, "tpm-emulator: No valid chardev found at '%s':", + value); + error_report_err(err); + goto err; } + tpm_emu->options->chardev = g_strdup(value); + if (tpm_emulator_prepare_data_fd(tpm_emu) < 0) { goto err; } @@ -925,6 +928,11 @@ static void tpm_emulator_shutdown(TPMEmulator *tpm_emu) { ptm_res res; + if (!tpm_emu->options->chardev) { + /* was never properly initialized */ + return; + } + if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SHUTDOWN, &res, 0, sizeof(res)) < 0) { error_report("tpm-emulator: Could not cleanly shutdown the TPM: %s", strerror(errno)); -- cgit v1.2.3