* What is TYPE_TPM_TIS_ISA? (Not an ISA Device) @ 2020-07-21 16:02 Philippe Mathieu-Daudé 2020-07-21 16:38 ` Stefan Berger 2020-07-21 16:40 ` Peter Maydell 0 siblings, 2 replies; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2020-07-21 16:02 UTC (permalink / raw) To: Stefan Berger, qemu-devel, Eric Auger Cc: Marc-André Lureau, David Safford 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: What is TYPE_TPM_TIS_ISA? (Not an ISA Device) 2020-07-21 16:02 What is TYPE_TPM_TIS_ISA? (Not an ISA Device) Philippe Mathieu-Daudé @ 2020-07-21 16:38 ` Stefan Berger 2020-07-21 16:40 ` Peter Maydell 1 sibling, 0 replies; 6+ messages in thread From: Stefan Berger @ 2020-07-21 16:38 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel, Eric Auger Cc: Marc-André Lureau 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. > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: What is TYPE_TPM_TIS_ISA? (Not an ISA Device) 2020-07-21 16:02 What is TYPE_TPM_TIS_ISA? (Not an ISA Device) Philippe Mathieu-Daudé 2020-07-21 16:38 ` Stefan Berger @ 2020-07-21 16:40 ` Peter Maydell 2020-07-22 5:55 ` Markus Armbruster 1 sibling, 1 reply; 6+ messages in thread From: Peter Maydell @ 2020-07-21 16:40 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Eric Auger, David Safford, qemu-devel, Marc-André Lureau, Stefan Berger 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: What is TYPE_TPM_TIS_ISA? (Not an ISA Device) 2020-07-21 16:40 ` Peter Maydell @ 2020-07-22 5:55 ` Markus Armbruster 2020-07-22 22:07 ` Stefan Berger 0 siblings, 1 reply; 6+ messages in thread From: Markus Armbruster @ 2020-07-22 5:55 UTC (permalink / raw) To: Peter Maydell Cc: David Safford, qemu-devel, Eric Auger, Marc-André Lureau, Philippe Mathieu-Daudé, Stefan Berger 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: What is TYPE_TPM_TIS_ISA? (Not an ISA Device) 2020-07-22 5:55 ` Markus Armbruster @ 2020-07-22 22:07 ` Stefan Berger 2020-07-23 9:10 ` Markus Armbruster 0 siblings, 1 reply; 6+ messages in thread From: Stefan Berger @ 2020-07-22 22:07 UTC (permalink / raw) To: Markus Armbruster Cc: Eric Auger, Philippe Mathieu-Daudé, qemu-devel, Marc-André Lureau 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: What is TYPE_TPM_TIS_ISA? (Not an ISA Device) 2020-07-22 22:07 ` Stefan Berger @ 2020-07-23 9:10 ` Markus Armbruster 0 siblings, 0 replies; 6+ messages in thread From: Markus Armbruster @ 2020-07-23 9:10 UTC (permalink / raw) To: Stefan Berger Cc: Eric Auger, Philippe Mathieu-Daudé, qemu-devel, Marc-André Lureau 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-23 9:11 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-21 16:02 What is TYPE_TPM_TIS_ISA? (Not an ISA Device) Philippe Mathieu-Daudé 2020-07-21 16:38 ` Stefan Berger 2020-07-21 16:40 ` Peter Maydell 2020-07-22 5:55 ` Markus Armbruster 2020-07-22 22:07 ` Stefan Berger 2020-07-23 9:10 ` Markus Armbruster
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.