Stefan Berger <stef...@linux.ibm.com> writes: > On 7/22/20 1:55 AM, Markus Armbruster wrote: >> pm socket --tpmstate dir=tpm --ctrl type=unixio,path=tpm/swtpm-soc >> running in another terminal. >> >>>> 3/ no machine plug it using isa_register_ioport() >>>> (it is not registered to the ISA memory space) >>> There's no requirement for an ISA device to have IO ports... >>> >>> thanks >>> -- PMM >> Thread hijack! Since I didn't have swtpm installed, I tried to take a >> shortcut: >> >> $ 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 >> >> That a null chardev doesn't work is fine. But the error handling looks >> broken: QEMU diagnoses and reports the problem, then continues. The >> final error message indicates that it continued without creating the >> backend "tpm0". That's wrong. > > > This issue can be solve via the following change that then displays > this error: > > $ x86_64-softmmu/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 > > > diff --git a/tpm.c b/tpm.c > index 358566cb10..857a861e69 100644 > --- a/tpm.c > +++ b/tpm.c > @@ -170,8 +170,10 @@ void tpm_cleanup(void) > */ > void 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, &error_fatal)) { > + exit(1); > + } > } > > /*
Interesting. > We had something like this before this patch here was applied: > https://github.com/qemu/qemu/commit/d10e05f15d5c3dd5e5cc59c5dfff460d89d48580#diff-0ec5df49c6751cb2dc9fa18ed5cf9f0e > > > Do we now want to partially revert this patch or call the exit(1) as > shown here? Let's have a closer look. qemu_opts_foreach()'s contract: * For each member of @list, call @func(@opaque, member, @errp). * Call it with the current location temporarily set to the member's. * @func() may store an Error through @errp, but must return non-zero then. * When @func() returns non-zero, break the loop and return that value. * Return zero when the loop completes. When qemu_opts_foreach(list, func, opaque, &error_fatal) returns, then func() did not set an error (If it did, we'd have died due to &error_fatal). Therefore, func() must have returned non-zero without setting an error. That's wrong. Let's look for this in tpm_init_tpmdev(): static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp) { [...] drv = be->create(opts); if (!drv) { return 1; Bingo! When I did commit d10e05f15d5, I missed this error path. } drv->id = g_strdup(id); QLIST_INSERT_HEAD(&tpm_backends, drv, list); return 0; } Two possible fixes: 1. Revert d10e05f15d5, live with the "error_report() in a function that takes an Error ** argument" code smell. Bearable, because it's confined to tpm.c. I'd recommend a comment explaining the non-use of @errp in tpm_init_tpmdev(). 2. Convert the ->create() to Error: tpm_passthrough_create(), tpm_emulator_create(), and their helpers. I think this would leave us in a better state, but I'm not sure the improvement is worth the effort right now. Spotted while writing this: ->tpm_startup() methods can fail. They appear to run in DeviceClass method reset(), which can't fail. Awkward. Could some failures be moved to realize() somehow?