[PATCH v5 0/3] tpm: Add missing ACPI device identification objects

[PATCH v5 0/3] tpm: Add missing ACPI device identification objects

[Stefan Berger]

This series of patches adds missing ACPI device identification objects _STR
and _UID to TPM 1.2 and TPM 2 ACPI tables.

   Stefan

v5:
 - Changed Fixes: tag to Resolves:
 - Mentioned in 2/3 that choice of uid '1' is based on inspection of hardware
   TPMs' sysfs entries

v4:
 - Rebased on current master: regenerated binaries in patch 3

v3:
 - Dropped replacement of ACPI tables with empty files in 1/3.
 - Reduced ignored files

Stefan Berger (3):
  tests: acpi: prepare for updated TPM related tables
  acpi: tpm: Add missing device identification objects
  tests: acpi: Add updated TPM related tables

 hw/arm/virt-acpi-build.c           |   1 +
 hw/i386/acpi-build.c               |   7 +++++++
 tests/data/acpi/q35/DSDT.tis.tpm12 | Bin 8894 -> 8900 bytes
 tests/data/acpi/q35/DSDT.tis.tpm2  | Bin 8894 -> 8921 bytes
 4 files changed, 8 insertions(+)

-- 
2.31.1

[PATCH v5 1/3] tests: acpi: prepare for updated TPM related tables

[Stefan Berger]

Replace existing TPM related tables, that are about to change, with
empty files.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Ani Sinha <ani@anisinha.ca>
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Acked-by: Ani Sinha <ani@anisinha.ca>
Message-id: 20211223022310.575496-2-stefanb@linux.ibm.com
---
 tests/qtest/bios-tables-test-allowed-diff.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..5d80e408d4 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,3 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/q35/DSDT.tis.tpm12",
+"tests/data/acpi/q35/DSDT.tis.tpm2",
-- 
2.31.1

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

[Stefan Berger]

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'.

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);
 
-- 
2.31.1

[PATCH v5 3/3] tests: acpi: Add updated TPM related tables

[Stefan Berger]

The updated TPM related tables have the following additions:

   Device (TPM)
   {
       Name (_HID, "MSFT0101" /* TPM 2.0 Security Device */)  // _HID: Hardware ID
+      Name (_STR, "TPM 2.0 Device")  // _STR: Description String
+      Name (_UID, One)  // _UID: Unique ID
       Name (_STA, 0x0F)  // _STA: Status
       Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Ani Sinha <ani@anisinha.ca>
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Acked-by: Ani Sinha <ani@anisinha.ca>
Message-id: 20211223022310.575496-4-stefanb@linux.ibm.com
---
 tests/data/acpi/q35/DSDT.tis.tpm12          | Bin 8894 -> 8900 bytes
 tests/data/acpi/q35/DSDT.tis.tpm2           | Bin 8894 -> 8921 bytes
 tests/qtest/bios-tables-test-allowed-diff.h |   2 --
 3 files changed, 2 deletions(-)

diff --git a/tests/data/acpi/q35/DSDT.tis.tpm12 b/tests/data/acpi/q35/DSDT.tis.tpm12
index 0ebdf6fbd77967f1ab5d5337b7b1fed314cfaca8..fb9dd1f0599afd6b555ea570ecd00a3bb227aa84 100644
GIT binary patch
delta 50
zcmdnzdc>8>CD<k8h!O(><KvB7q6(a@S~2m#PVoZ1lQk6FnOs#T7b=LdgnGI#Zf;Sq
GVgdkr91X<)

delta 45
zcmX@&y3du%CD<iopArKDqxwcJQ3Xza&6xOLr+5MP$r=joO#Uj93l&5+_b6B}0RSYz
B3@!iw

diff --git a/tests/data/acpi/q35/DSDT.tis.tpm2 b/tests/data/acpi/q35/DSDT.tis.tpm2
index dcbb7f0af377425db53130e8ba1c62c09c22e006..00d732e46f5d9d056e557bd026fa30f9db3b8c30 100644
GIT binary patch
delta 70
zcmdnzdefE5CD<k8rV;}KBgaNAQ3Wn9?U?vrr+5J;?a7)7ZcJWklM5BZ#e;Z50(=#W
a^b8bSQp+-vQyDnoLp@y>H@7HQF#!OXcoHoD

delta 46
zcmccVy3du%CD<iopArKD<D-pSq6%F8nlbUgPVoZnnv*pZ+?f1TCKoD*Z(gim#smOL
C=M6sq

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 5d80e408d4..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,3 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/q35/DSDT.tis.tpm12",
-"tests/data/acpi/q35/DSDT.tis.tpm2",
-- 
2.31.1

Re: [PATCH v5 0/3] tpm: Add missing ACPI device identification objects

[Daniel P. Berrangé]

On Tue, Jan 04, 2022 at 12:58:03PM -0500, Stefan Berger wrote:
> This series of patches adds missing ACPI device identification objects _STR
> and _UID to TPM 1.2 and TPM 2 ACPI tables.

What was the practical impact on guests (if any) from these ACPI
objects being missing ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v5 0/3] tpm: Add missing ACPI device identification objects

[Stefan Berger]

On 1/4/22 13:09, Daniel P. Berrangé wrote:
> On Tue, Jan 04, 2022 at 12:58:03PM -0500, Stefan Berger wrote:
>> This series of patches adds missing ACPI device identification objects _STR
>> and _UID to TPM 1.2 and TPM 2 ACPI tables.
> What was the practical impact on guests (if any) from these ACPI
> objects being missing ?

As discussed in v4: I don't know if there's software relying on this or 
just a user who found these to be missing:

https://gitlab.com/qemu-project/qemu/-/issues/708

I can only say that these entries were missing, so I added them. It 
makes TPM ACPI more complete, for sure. Maybe someone else knows more if 
there's something out there that actually needs this. I'd be curious as 
well.

    Stefan

>
> Regards,
> Daniel

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

[Igor Mammedov]

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)?


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

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

[Michael S. Tsirkin]

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

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

[Stefan Berger]

On 1/6/22 03:36, 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)?
>
I smoke tested it with the posted patches applied to v6.2.0 and started 
3 VMs with it:

- Linux shows uid = 1 and the description "TPM 2.0 Device" in sysfs

- Win 10 and Win 11 tpm.msc tool are both showing that the TPM is 'ready 
for use'

     Stefan

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

[Michael S. Tsirkin]

On Thu, Jan 06, 2022 at 08:53:00AM -0500, Stefan Berger wrote:
> 
> On 1/6/22 03:36, 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)?
> > 
> I smoke tested it with the posted patches applied to v6.2.0 and started 3
> VMs with it:
> 
> - Linux shows uid = 1 and the description "TPM 2.0 Device" in sysfs
> 
> - Win 10 and Win 11 tpm.msc tool are both showing that the TPM is 'ready for
> use'
> 
>     Stefan
> 

Just to make sure, what Igor was concerned about is issues like
we had with e.g. network devices, when changing UID makes
windows think it's a new device and lose configuration
created on old qemu on boot with a new qemu.
Not sure what can be configured with a TPM device though ...

-- 
MST

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

[Stefan Berger]

On 1/6/22 08:56, Michael S. Tsirkin wrote:
> On Thu, Jan 06, 2022 at 08:53:00AM -0500, Stefan Berger wrote:
>> On 1/6/22 03:36, 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)?
>>>
>> I smoke tested it with the posted patches applied to v6.2.0 and started 3
>> VMs with it:
>>
>> - Linux shows uid = 1 and the description "TPM 2.0 Device" in sysfs
>>
>> - Win 10 and Win 11 tpm.msc tool are both showing that the TPM is 'ready for
>> use'
>>
>>      Stefan
>>
> Just to make sure, what Igor was concerned about is issues like
> we had with e.g. network devices, when changing UID makes
> windows think it's a new device and lose configuration
> created on old qemu on boot with a new qemu.
> Not sure what can be configured with a TPM device though ...

The VMs were all created on an old qemu and booted into the patched 
qemu. They hadn't seen the new ACPI entries before, for sure not when 
they were installed.



>

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

[Igor Mammedov]

On Thu, 6 Jan 2022 09:01:36 -0500
Stefan Berger <stefanb@linux.ibm.com> wrote:

> On 1/6/22 08:56, Michael S. Tsirkin wrote:
> > On Thu, Jan 06, 2022 at 08:53:00AM -0500, Stefan Berger wrote:  
> >> On 1/6/22 03:36, 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)?
> >>>  
> >> I smoke tested it with the posted patches applied to v6.2.0 and started 3
> >> VMs with it:
> >>
> >> - Linux shows uid = 1 and the description "TPM 2.0 Device" in sysfs
> >>
> >> - Win 10 and Win 11 tpm.msc tool are both showing that the TPM is 'ready for
> >> use'
> >>
> >>      Stefan
> >>  
> > Just to make sure, what Igor was concerned about is issues like
> > we had with e.g. network devices, when changing UID makes
> > windows think it's a new device and lose configuration
> > created on old qemu on boot with a new qemu.
> > Not sure what can be configured with a TPM device though ...  
> 
> The VMs were all created on an old qemu and booted into the patched 
> qemu. They hadn't seen the new ACPI entries before, for sure not when 
> they were installed. 

In that case I would not bother with compat machinery

(my stance on APCI and compat knobs haven't changed and it
is avoid it if possible, sometimes that backfires but overall
keeps code simpler, otherwise it would be unreadable mess
(it's already complex enough))

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Re: [PATCH v5 1/3] tests: acpi: prepare for updated TPM related tables

[Igor Mammedov]

On Tue,  4 Jan 2022 12:58:04 -0500
Stefan Berger <stefanb@linux.ibm.com> wrote:

> Replace existing TPM related tables, that are about to change, with
> empty files.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Ani Sinha <ani@anisinha.ca>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Acked-by: Ani Sinha <ani@anisinha.ca>
> Message-id: 20211223022310.575496-2-stefanb@linux.ibm.com


Acked-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  tests/qtest/bios-tables-test-allowed-diff.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index dfb8523c8b..5d80e408d4 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1 +1,3 @@
>  /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/q35/DSDT.tis.tpm12",
> +"tests/data/acpi/q35/DSDT.tis.tpm2",

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

[Ani Sinha]

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

On Thu, Jan 6, 2022 at 19:31 Stefan Berger <stefanb@linux.ibm.com> wrote:

>
> >>> 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)?
>
>
> The VMs were all created on an old qemu and booted into the patched
> qemu.


Stupid question - should we also check the other way as well? Install on
patches qemu and try to boot on the old unpatched qemu?

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

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

[Stefan Berger]

On 1/6/22 11:55, Igor Mammedov wrote:
> On Thu, 6 Jan 2022 09:01:36 -0500
> Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>> On 1/6/22 08:56, Michael S. Tsirkin wrote:
>>> On Thu, Jan 06, 2022 at 08:53:00AM -0500, Stefan Berger wrote:
>>>> On 1/6/22 03:36, 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)?
>>>>>   
>>>> I smoke tested it with the posted patches applied to v6.2.0 and started 3
>>>> VMs with it:
>>>>
>>>> - Linux shows uid = 1 and the description "TPM 2.0 Device" in sysfs
>>>>
>>>> - Win 10 and Win 11 tpm.msc tool are both showing that the TPM is 'ready for
>>>> use'
>>>>
>>>>       Stefan
>>>>   
>>> Just to make sure, what Igor was concerned about is issues like
>>> we had with e.g. network devices, when changing UID makes
>>> windows think it's a new device and lose configuration
>>> created on old qemu on boot with a new qemu.
>>> Not sure what can be configured with a TPM device though ...
>> The VMs were all created on an old qemu and booted into the patched
>> qemu. They hadn't seen the new ACPI entries before, for sure not when
>> they were installed.
> In that case I would not bother with compat machinery
>
> (my stance on APCI and compat knobs haven't changed and it
> is avoid it if possible, sometimes that backfires but overall
> keeps code simpler, otherwise it would be unreadable mess
> (it's already complex enough))
>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>
Another test I did was to remove the TPM 2, boot the win 10 and 11 VMs 
ran tpm.msc (no TPM there), shut them down, added the TPM 2 and ran 
tpm.msc again. Same result: TPM is 'ready for use'.