Re: [PATCH v5 2/3] acpi: tpm: Add missing device identification objects

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jan 06, 2022 at 09:36:36AM +0100, Igor Mammedov wrote:
> On Tue,  4 Jan 2022 12:58:05 -0500
> Stefan Berger <stefanb@linux.ibm.com> wrote:
> 
> > Add missing TPM device identification objects _STR and _UID. They will
> > appear as files 'description' and 'uid' under Linux sysfs.
> > 
> > Following inspection of sysfs entries for hardware TPMs we chose
> > uid '1'.
> 
> My guess would be that buy default (in case of missing UID), OSPM
> will start enumerate from 0. So I think 0 is more safer choice
> when it comes to compatibility.
> 
> Can you smoke test TPM with Windows, and check if adding UID doesn't
> break anything if VM actually uses TMP (though I'm not sure how to
> check it on Windows, maybe install Windows 11 without this patch
> and then see if it still boots pre-installed VM and nothing is broken
> after this patch)?

Given out experience with these things, I would add compat
machinery and avoid changing things for existing machine types.
Should be sufficient to address these concerns right Igor?

> 
> > Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: Ani Sinha <ani@anisinha.ca>
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/708
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > Reviewed-by: Shannon Zhao <shannon.zhaosl@gmail.com>
> > Message-id: 20211223022310.575496-3-stefanb@linux.ibm.com
> > ---
> >  hw/arm/virt-acpi-build.c | 1 +
> >  hw/i386/acpi-build.c     | 7 +++++++
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index d0f4867fdf..f2514ce77c 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -229,6 +229,7 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
> >  
> >      Aml *dev = aml_device("TPM0");
> >      aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
> > +    aml_append(dev, aml_name_decl("_STR", aml_string("TPM 2.0 Device")));
> >      aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> >  
> >      Aml *crs = aml_resource_template();
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 8383b83ee3..05740b7f15 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1812,11 +1812,15 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >                      dev = aml_device("TPM");
> >                      aml_append(dev, aml_name_decl("_HID",
> >                                                    aml_string("MSFT0101")));
> > +                    aml_append(dev,
> > +                               aml_name_decl("_STR",
> > +                                             aml_string("TPM 2.0 Device")));
> >                  } else {
> >                      dev = aml_device("ISA.TPM");
> >                      aml_append(dev, aml_name_decl("_HID",
> >                                                    aml_eisaid("PNP0C31")));
> >                  }
> > +                aml_append(dev, aml_name_decl("_UID", aml_int(1)));
> >  
> >                  aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
> >                  crs = aml_resource_template();
> > @@ -1844,12 +1848,15 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >      if (TPM_IS_CRB(tpm)) {
> >          dev = aml_device("TPM");
> >          aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
> > +        aml_append(dev, aml_name_decl("_STR",
> > +                                      aml_string("TPM 2.0 Device")));
> >          crs = aml_resource_template();
> >          aml_append(crs, aml_memory32_fixed(TPM_CRB_ADDR_BASE,
> >                                             TPM_CRB_ADDR_SIZE, AML_READ_WRITE));
> >          aml_append(dev, aml_name_decl("_CRS", crs));
> >  
> >          aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
> > +        aml_append(dev, aml_name_decl("_UID", aml_int(1)));
> >  
> >          tpm_build_ppi_acpi(tpm, dev);
> >