On Tue, 25 Feb 2025 at 07:41, Kun Qin <[email protected]> wrote: > > From: Kun Qin <[email protected]> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2625 > > This change aims to add a TPM device for SBSA ref machine. > > The implementation adds a TPM create routine during machine > initialization. > > The backend can be the same as the rest of TPM support, by using swtpm.
sbsa-ref is a standard reference platform, so my main question here is: does the firmware and the rest of the reference platform expect and correctly handle the new device we're adding here ? Changes to the QEMU model I would expect are typically done in concert with changes to the software stack. I think a new device also would merit at least a bumping of the machine-version-minor number. Depending on the behaviour of the software stack it might need a major version bump. This kind of thing is a question that hopefully Radoslaw and/or Leif can help with. I'd like to see review by one of them before I merge this patch. I also have some more minor comments below on the code changes. > Signed-off-by: Kun Qin <[email protected]> (minor commit message style thing: typically the "Resolves:" line and similar "foo:" standard lines go immediately above the "Signed-off-by:" line.) > --- > hw/arm/sbsa-ref.c | 24 ++++++++++++++++++++++++ docs/system/arm/sbsa.rst also needs to be updated. > 1 file changed, 24 insertions(+) > > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c > index e720de306419..93eb3d1e363b 100644 > --- a/hw/arm/sbsa-ref.c > +++ b/hw/arm/sbsa-ref.c > @@ -28,6 +28,8 @@ > #include "system/numa.h" > #include "system/runstate.h" > #include "system/system.h" > +#include "system/tpm.h" > +#include "system/tpm_backend.h" > #include "exec/hwaddr.h" > #include "kvm_arm.h" > #include "hw/arm/boot.h" > @@ -94,6 +96,7 @@ enum { > SBSA_SECURE_MEM, > SBSA_AHCI, > SBSA_XHCI, > + SBSA_TPM, > }; > > struct SBSAMachineState { > @@ -132,6 +135,7 @@ static const MemMapEntry sbsa_ref_memmap[] = { > /* Space here reserved for more SMMUs */ > [SBSA_AHCI] = { 0x60100000, 0x00010000 }, > [SBSA_XHCI] = { 0x60110000, 0x00010000 }, > + [SBSA_TPM] = { 0x60120000, 0x00010000 }, > /* Space here reserved for other devices */ > [SBSA_PCIE_PIO] = { 0x7fff0000, 0x00010000 }, > /* 32-bit address PCIE MMIO space */ > @@ -629,6 +633,24 @@ static void create_smmu(const SBSAMachineState *sms, > PCIBus *bus) > } > } > > +static void create_tpm(SBSAMachineState *sbsa, PCIBus *bus) Why do you pass in a pointer to the PCI bus when this isn't a PCI device? It looks like the 'bus' argument is unused. > +{ > + Error *errp = NULL; > + DeviceState *dev; > + > + TPMBackend *be = qemu_find_tpm_be("tpm0"); > + if (be == NULL) { > + error_report("Couldn't find tmp0 backend"); > + return; > + } > + > + dev = qdev_new(TYPE_TPM_TIS_SYSBUS); > + object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &errp); > + object_property_set_str(OBJECT(dev), "tpmdev", be->id, &errp); > + sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); Why use &errp here twice and &error_fatal once? Passing in an errp and then not doing anything with it is effectively throwing away the error report. If you know that the function you're calling can't in practice fail, you can use &error_abort (which is like an assert() that the function didn't fail). If the function might fail and you want to print an error message and exit(1) then you can use &error_fatal. If you want to pass the error status up to a calling function, or do more complicated things in the failure case, you can use an Error* variable. The comments in include/qapi/error.h have an extended discussion with various standard usage patterns. In this case I think &error_fatal on all three lines is probably what you want. > + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, sbsa_ref_memmap[SBSA_TPM].base); > +} thanks -- PMM
