Re: What is TYPE_TPM_TIS_ISA? (Not an ISA Device)

[Markus Armbruster]

Stefan Berger <stefanb@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?