[PATCH 0/1] Adding TPM support for ARM SBSA-Ref machine

[PATCH 0/1] Adding TPM support for ARM SBSA-Ref machine

[Kun Qin]

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2625

This change intends to support an ARM64 server like platform that has TPM
support to unlock more power for sbsa_ref.

The idea is to add a TPM create routine during sbsa machine initialization.
The backend can be the same as the rest of TPM support, by using swtpm.

Sign-off-by: Kun Qin <kuqin12@gmail.com>

Kun Qin (1):
  hw/arm/sbsa-ref: Adding TPM support for ARM SBSA-Ref machine

 hw/arm/sbsa-ref.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

-- 
2.43.0

[PATCH 1/1] hw/arm/sbsa-ref: Adding TPM support for ARM SBSA-Ref machine

[Kun Qin]

From: Kun Qin <kuqin@microsoft.com>

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.

Signed-off-by: Kun Qin <kuqin12@gmail.com>
---
 hw/arm/sbsa-ref.c | 24 ++++++++++++++++++++++++
 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)
+{
+    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);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, sbsa_ref_memmap[SBSA_TPM].base);
+}
+
 static void create_pcie(SBSAMachineState *sms)
 {
     hwaddr base_ecam = sbsa_ref_memmap[SBSA_PCIE_ECAM].base;
@@ -686,6 +708,8 @@ static void create_pcie(SBSAMachineState *sms)
     pci_create_simple(pci->bus, -1, "bochs-display");
 
     create_smmu(sms, pci->bus);
+
+    create_tpm(sms, pci->bus);
 }
 
 static void *sbsa_ref_dtb(const struct arm_boot_info *binfo, int *fdt_size)
-- 
2.43.0

Re: [PATCH 1/1] hw/arm/sbsa-ref: Adding TPM support for ARM SBSA-Ref machine

[Graeme Gregory]

On 25/02/2025 07:41, Kun Qin wrote:
> From: Kun Qin <kuqin@microsoft.com>
>
> 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.

This looks sensible to me.

Reviewed-by: Graeme Gregory <graeme@xora.org.uk>

> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> ---
>   hw/arm/sbsa-ref.c | 24 ++++++++++++++++++++++++
>   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)
> +{
> +    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);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, sbsa_ref_memmap[SBSA_TPM].base);
> +}
> +
>   static void create_pcie(SBSAMachineState *sms)
>   {
>       hwaddr base_ecam = sbsa_ref_memmap[SBSA_PCIE_ECAM].base;
> @@ -686,6 +708,8 @@ static void create_pcie(SBSAMachineState *sms)
>       pci_create_simple(pci->bus, -1, "bochs-display");
>   
>       create_smmu(sms, pci->bus);
> +
> +    create_tpm(sms, pci->bus);
>   }
>   
>   static void *sbsa_ref_dtb(const struct arm_boot_info *binfo, int *fdt_size)

Re: [PATCH 1/1] hw/arm/sbsa-ref: Adding TPM support for ARM SBSA-Ref machine

[Kun Qin]

[-- Attachment #1: Type: text/plain, Size: 3299 bytes --]

Hi Graeme,

Thank you for your review. Could you please let me know if there is
anything else I need to do or wait on before merging the change?

Any input is appreciated.

Regards,
Kun



On Thu, Feb 27, 2025 at 7:16 AM Graeme Gregory <graeme@xora.org.uk> wrote:

>
> On 25/02/2025 07:41, Kun Qin wrote:
> > From: Kun Qin <kuqin@microsoft.com>
> >
> > 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.
>
> This looks sensible to me.
>
> Reviewed-by: Graeme Gregory <graeme@xora.org.uk>
>
> > Signed-off-by: Kun Qin <kuqin12@gmail.com>
> > ---
> >   hw/arm/sbsa-ref.c | 24 ++++++++++++++++++++++++
> >   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)
> > +{
> > +    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);
> > +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
> sbsa_ref_memmap[SBSA_TPM].base);
> > +}
> > +
> >   static void create_pcie(SBSAMachineState *sms)
> >   {
> >       hwaddr base_ecam = sbsa_ref_memmap[SBSA_PCIE_ECAM].base;
> > @@ -686,6 +708,8 @@ static void create_pcie(SBSAMachineState *sms)
> >       pci_create_simple(pci->bus, -1, "bochs-display");
> >
> >       create_smmu(sms, pci->bus);
> > +
> > +    create_tpm(sms, pci->bus);
> >   }
> >
> >   static void *sbsa_ref_dtb(const struct arm_boot_info *binfo, int
> *fdt_size)
>

[-- Attachment #2: Type: text/html, Size: 4707 bytes --]

Re: [PATCH 1/1] hw/arm/sbsa-ref: Adding TPM support for ARM SBSA-Ref machine

[Peter Maydell]

On Tue, 25 Feb 2025 at 07:41, Kun Qin <kuqin12@gmail.com> wrote:
>
> From: Kun Qin <kuqin@microsoft.com>
>
> 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 <kuqin12@gmail.com>

(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

Re: [PATCH 1/1] hw/arm/sbsa-ref: Adding TPM support for ARM SBSA-Ref machine

[Leif Lindholm]

Doh! Add the lists back in. (No idea how I dropped them off.)

On Mon, 3 Mar 2025 at 17:02, Leif Lindholm
<leif.lindholm@oss.qualcomm.com> wrote:
>
> Hi Kun,
>
> Apologies for delay in responding - I was out last week.
> I agree with this addition, since a TPM is a requirement for servers.
>
> However, to help simplify review, could you add some detail in the
> commit message
> as to which SystemReady requirements this resolves and whether this
> implementation
> fulfills all requirements across BSA/SBSA/BBSA?
>
> I agree with Peter that since this is a non-discoverable component, it
> would make sense
> to step the machine minor version number. A major version bump would
> not be required
> since simply adding this component will not break any existing
> firmware (which will have
> no way of knowing it even exists).
>
> Regards,
>
> Leif
>
> On Tue, 25 Feb 2025 at 07:41, Kun Qin <kuqin12@gmail.com> wrote:
> >
> > From: Kun Qin <kuqin@microsoft.com>
> >
> > 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.
> >
> > Signed-off-by: Kun Qin <kuqin12@gmail.com>
> > ---
> >  hw/arm/sbsa-ref.c | 24 ++++++++++++++++++++++++
> >  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)
> > +{
> > +    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);
> > +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, sbsa_ref_memmap[SBSA_TPM].base);
> > +}
> > +
> >  static void create_pcie(SBSAMachineState *sms)
> >  {
> >      hwaddr base_ecam = sbsa_ref_memmap[SBSA_PCIE_ECAM].base;
> > @@ -686,6 +708,8 @@ static void create_pcie(SBSAMachineState *sms)
> >      pci_create_simple(pci->bus, -1, "bochs-display");
> >
> >      create_smmu(sms, pci->bus);
> > +
> > +    create_tpm(sms, pci->bus);
> >  }
> >
> >  static void *sbsa_ref_dtb(const struct arm_boot_info *binfo, int *fdt_size)
> > --
> > 2.43.0
> >

Re: [PATCH 1/1] hw/arm/sbsa-ref: Adding TPM support for ARM SBSA-Ref machine

[Kun Qin]

[-- Attachment #1: Type: text/plain, Size: 4260 bytes --]

Hi Leif & Peter,

Thanks for the comments. I will address them in a v2 patch.

Regards,
Kun

On Mon, Mar 3, 2025 at 12:44 PM Leif Lindholm <
leif.lindholm@oss.qualcomm.com> wrote:

> Doh! Add the lists back in. (No idea how I dropped them off.)
>
> On Mon, 3 Mar 2025 at 17:02, Leif Lindholm
> <leif.lindholm@oss.qualcomm.com> wrote:
> >
> > Hi Kun,
> >
> > Apologies for delay in responding - I was out last week.
> > I agree with this addition, since a TPM is a requirement for servers.
> >
> > However, to help simplify review, could you add some detail in the
> > commit message
> > as to which SystemReady requirements this resolves and whether this
> > implementation
> > fulfills all requirements across BSA/SBSA/BBSA?
> >
> > I agree with Peter that since this is a non-discoverable component, it
> > would make sense
> > to step the machine minor version number. A major version bump would
> > not be required
> > since simply adding this component will not break any existing
> > firmware (which will have
> > no way of knowing it even exists).
> >
> > Regards,
> >
> > Leif
> >
> > On Tue, 25 Feb 2025 at 07:41, Kun Qin <kuqin12@gmail.com> wrote:
> > >
> > > From: Kun Qin <kuqin@microsoft.com>
> > >
> > > 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.
> > >
> > > Signed-off-by: Kun Qin <kuqin12@gmail.com>
> > > ---
> > >  hw/arm/sbsa-ref.c | 24 ++++++++++++++++++++++++
> > >  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)
> > > +{
> > > +    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);
> > > +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
> sbsa_ref_memmap[SBSA_TPM].base);
> > > +}
> > > +
> > >  static void create_pcie(SBSAMachineState *sms)
> > >  {
> > >      hwaddr base_ecam = sbsa_ref_memmap[SBSA_PCIE_ECAM].base;
> > > @@ -686,6 +708,8 @@ static void create_pcie(SBSAMachineState *sms)
> > >      pci_create_simple(pci->bus, -1, "bochs-display");
> > >
> > >      create_smmu(sms, pci->bus);
> > > +
> > > +    create_tpm(sms, pci->bus);
> > >  }
> > >
> > >  static void *sbsa_ref_dtb(const struct arm_boot_info *binfo, int
> *fdt_size)
> > > --
> > > 2.43.0
> > >
>

[-- Attachment #2: Type: text/html, Size: 6039 bytes --]

Re: [PATCH 1/1] hw/arm/sbsa-ref: Adding TPM support for ARM SBSA-Ref machine

[Philippe Mathieu-Daudé]

Hi Kun,

On 3/3/25 23:55, Kun Qin wrote:
> Hi Leif & Peter,
> 
> Thanks for the comments. I will address them in a v2 patch.

Please also Cc me in your v2 :)

Regards,

Phil.

> 
> Regards,
> Kun
> 
> On Mon, Mar 3, 2025 at 12:44 PM Leif Lindholm 
> <leif.lindholm@oss.qualcomm.com <mailto:leif.lindholm@oss.qualcomm.com>> 
> wrote:
> 
>     Doh! Add the lists back in. (No idea how I dropped them off.)
> 
>     On Mon, 3 Mar 2025 at 17:02, Leif Lindholm
>     <leif.lindholm@oss.qualcomm.com
>     <mailto:leif.lindholm@oss.qualcomm.com>> wrote:
>      >
>      > Hi Kun,
>      >
>      > Apologies for delay in responding - I was out last week.
>      > I agree with this addition, since a TPM is a requirement for servers.
>      >
>      > However, to help simplify review, could you add some detail in the
>      > commit message
>      > as to which SystemReady requirements this resolves and whether this
>      > implementation
>      > fulfills all requirements across BSA/SBSA/BBSA?
>      >
>      > I agree with Peter that since this is a non-discoverable
>     component, it
>      > would make sense
>      > to step the machine minor version number. A major version bump would
>      > not be required
>      > since simply adding this component will not break any existing
>      > firmware (which will have
>      > no way of knowing it even exists).
>      >
>      > Regards,
>      >
>      > Leif
>      >
>      > On Tue, 25 Feb 2025 at 07:41, Kun Qin <kuqin12@gmail.com
>     <mailto:kuqin12@gmail.com>> wrote:
>      > >
>      > > From: Kun Qin <kuqin@microsoft.com <mailto:kuqin@microsoft.com>>
>      > >
>      > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2625
>     <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.
>      > >
>      > > Signed-off-by: Kun Qin <kuqin12@gmail.com
>     <mailto:kuqin12@gmail.com>>
>      > > ---
>      > >  hw/arm/sbsa-ref.c | 24 ++++++++++++++++++++++++
>      > >  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)
>      > > +{
>      > > +    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);
>      > > +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
>     sbsa_ref_memmap[SBSA_TPM].base);
>      > > +}
>      > > +
>      > >  static void create_pcie(SBSAMachineState *sms)
>      > >  {
>      > >      hwaddr base_ecam = sbsa_ref_memmap[SBSA_PCIE_ECAM].base;
>      > > @@ -686,6 +708,8 @@ static void create_pcie(SBSAMachineState *sms)
>      > >      pci_create_simple(pci->bus, -1, "bochs-display");
>      > >
>      > >      create_smmu(sms, pci->bus);
>      > > +
>      > > +    create_tpm(sms, pci->bus);
>      > >  }
>      > >
>      > >  static void *sbsa_ref_dtb(const struct arm_boot_info *binfo,
>     int *fdt_size)
>      > > --
>      > > 2.43.0
>      > >
> 

Re: [PATCH 0/1] Adding TPM support for ARM SBSA-Ref machine

[Peter Maydell]

On Tue, 25 Feb 2025 at 07:41, Kun Qin <kuqin12@gmail.com> wrote:
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2625
>
> This change intends to support an ARM64 server like platform that has TPM
> support to unlock more power for sbsa_ref.
>
> The idea is to add a TPM create routine during sbsa machine initialization.
> The backend can be the same as the rest of TPM support, by using swtpm.
>
> Sign-off-by: Kun Qin <kuqin12@gmail.com>
>
> Kun Qin (1):
>   hw/arm/sbsa-ref: Adding TPM support for ARM SBSA-Ref machine
>
>  hw/arm/sbsa-ref.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)

By the way, single patches don't need a separate cover letter
email; only multi-patch series should have the cover letter.
This isn't a big deal, but the cover letter on a single patch
seems to confuse some of our tooling, unfortunately, e.g.
patchew fails to find the patch email:
https://patchew.org/QEMU/20250225074133.6827-1-kuqin12@gmail.com/

-- PMM