What is TYPE_TPM_TIS_ISA? (Not an ISA Device)

What is TYPE_TPM_TIS_ISA? (Not an ISA Device)

[Philippe Mathieu-Daudé]

Hi Stefan,

I'm trying to understand what is modelling the
TYPE_TPM_TIS_ISA device.

It inherits from TYPE_ISA_DEVICE, so I expected
to see an ISA device, but then I noticed:

1/ it doesn't use the ISA I/O space, it directly
maps the device in the system memory at a fixed
address that is not addressable by the ISA bus:

#define TPM_TIS_ADDR_BASE           0xFED40000

2/ it is not plugged to an ISA BUS (ISABus*)

3/ no machine plug it using isa_register_ioport()
   (it is not registered to the ISA memory space)

4/ the only thing slightly related to ISA is it
checks the IRQ number is < ISA_NUM_IRQS


So it seems this is a plain SysBusDevice. But then
there is TYPE_TPM_TIS_SYSBUS... What is the difference?

Thanks,

Phil.

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

[Stefan Berger]

On 7/21/20 12:02 PM, Philippe Mathieu-Daudé wrote:
> Hi Stefan,
>
> I'm trying to understand what is modelling the
> TYPE_TPM_TIS_ISA device.
>
> It inherits from TYPE_ISA_DEVICE, so I expected
> to see an ISA device, but then I noticed:
>
> 1/ it doesn't use the ISA I/O space, it directly
> maps the device in the system memory at a fixed
> address that is not addressable by the ISA bus:
>
> #define TPM_TIS_ADDR_BASE           0xFED40000
>
> 2/ it is not plugged to an ISA BUS (ISABus*)
>
> 3/ no machine plug it using isa_register_ioport()
>     (it is not registered to the ISA memory space)
>
> 4/ the only thing slightly related to ISA is it
> checks the IRQ number is < ISA_NUM_IRQS
>
>
> So it seems this is a plain SysBusDevice. But then
> there is TYPE_TPM_TIS_SYSBUS... What is the difference?

The TIS is modeled as an ISA device as an alternative to being connected 
to the LPC bus, to which it would typically be connected to. I believe 
we also have the ISA.TPM because of Windows only accepting it. Maybe 
this was not quite correct, but does it cause issues?

                 } else {
                     dev = aml_device("ISA.TPM");
                     aml_append(dev, aml_name_decl("_HID",
aml_eisaid("PNP0C31")));
                 }




>
> Thanks,
>
> Phil.
>

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

[Peter Maydell]

On Tue, 21 Jul 2020 at 17:07, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Hi Stefan,
>
> I'm trying to understand what is modelling the
> TYPE_TPM_TIS_ISA device.
>
> It inherits from TYPE_ISA_DEVICE, so I expected
> to see an ISA device, but then I noticed:
>
> 1/ it doesn't use the ISA I/O space, it directly
> maps the device in the system memory at a fixed
> address that is not addressable by the ISA bus:
>
> #define TPM_TIS_ADDR_BASE           0xFED40000

Why do you think this is mapping to the system memory?
tpm_tis_isa_realizefn() does:

    memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
                                TPM_TIS_ADDR_BASE, &s->mmio);

which puts it into the ISA memory address space.

The weird thing about this is not which AS it's
going in but the fact that the TPM_TIS_ADDR_BASE
is way higher than an actual ISA bus can address
(so for instance it's out of range of the size of
the ISA memory window on the Jazz board).

> 2/ it is not plugged to an ISA BUS (ISABus*)

Won't it autoplug into the ISA bus if you say "-device tpm-tis",
the same as any other ISA device ?

> 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

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

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 21 Jul 2020 at 17:07, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Hi Stefan,
>>
>> I'm trying to understand what is modelling the
>> TYPE_TPM_TIS_ISA device.
>>
>> It inherits from TYPE_ISA_DEVICE, so I expected
>> to see an ISA device, but then I noticed:
>>
>> 1/ it doesn't use the ISA I/O space, it directly
>> maps the device in the system memory at a fixed
>> address that is not addressable by the ISA bus:
>>
>> #define TPM_TIS_ADDR_BASE           0xFED40000
>
> Why do you think this is mapping to the system memory?
> tpm_tis_isa_realizefn() does:
>
>     memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
>                                 TPM_TIS_ADDR_BASE, &s->mmio);
>
> which puts it into the ISA memory address space.
>
> The weird thing about this is not which AS it's
> going in but the fact that the TPM_TIS_ADDR_BASE
> is way higher than an actual ISA bus can address
> (so for instance it's out of range of the size of
> the ISA memory window on the Jazz board).
>
>> 2/ it is not plugged to an ISA BUS (ISABus*)
>
> Won't it autoplug into the ISA bus if you say "-device tpm-tis",
> the same as any other ISA device ?

Yup:

    $ qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -chardev socket,id=chrtpm,path=tpm/swtpm-sock -tpmdev emulator,id=tpm0,chardev=chrtpm -device tpm-tis,tpmdev=tpm0
    QEMU 5.0.90 monitor - type 'help' for more information
    (qemu) info qtree
    bus: main-system-bus
      type System
      [...]
      dev: i440FX-pcihost, id ""
        [...]
        bus: pci.0
          type PCI
            [...]
          dev: PIIX3, id ""
            [...]
            bus: isa.0
              type ISA
              dev: tpm-tis, id ""
                irq = 5 (0x5)
                tpmdev = "tpm0"
                ppi = true
                isa irq 5
              [...]

This is with

    $ swtpm 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.

Different tack: could -tpmdev be made sugar for -object?  I'm asking
because other kinds of backends use -object instead of their very own
option.

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

[Stefan Berger]

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);
+    }
  }

  /*

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?


    Stefan

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?