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

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

On Tue, Nov 09, 2021 at 08:26:11PM +0530, Ani Sinha wrote:
> On Tue, Nov 9, 2021 at 8:00 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Tue, Nov 09, 2021 at 09:14:25AM -0500, Michael S. Tsirkin wrote:
> > > On Tue, Nov 09, 2021 at 09:01:52AM -0500, Stefan Berger wrote:
> > > > 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>
> > > > ---
> > > >  tests/data/acpi/q35/DSDT.tis.tpm12          | Bin 0 -> 8900 bytes
> > > >  tests/data/acpi/q35/DSDT.tis.tpm2           | Bin 0 -> 8921 bytes
> > > >  tests/data/acpi/q35/TCPA.tis.tpm12          | Bin 0 -> 50 bytes
> > > >  tests/data/acpi/q35/TPM2.tis.tpm2           | Bin 0 -> 76 bytes
> > > >  tests/qtest/bios-tables-test-allowed-diff.h |  11 -----------
> > > >  5 files changed, 11 deletions(-)
> > >
> > > A disadvantage to doing it like this is that git thinks
> > > it's ok to replace any empty file with this, so if acpi
> > > changed in any way git will happily resolve it
> > > replacing it with this version.
> >
> > Do we actually need to be storing these binary files in git
> > at all ?
> >
> > IIUC, the test will do two things
> >
> >  - memcmp the expected binary we store, against the new binary
> >    we generated.
> >  - if they differ, then disassemble both and report the
> >    differences in a user friendly-ish way
> >
> > What if we only stored the sha256 checksum of the binary *and*
> > the dissasembled output in git, never the full binary.
> 
> If you are going down that path, why need the sha256 at all? The test
> can disassemble the tables from qemu and only compare the disassembled
> ASL.

The output isn't stable unfortunately, so it can't replace the binary..
However, the process of documenting what changed is quite fiddly, so I
am open to including both the binary and the dis-assembled file.
An issue is that iasl does not support all platforms that qemu
supports, in particular the disassembler is broken on BE platforms.
Not sure how big a deal that is ...

> >
> > IIUC, that would give us the same level of diagnostic output
> > from the test failures. The dissasembled output would then
> > give us meaningful patches for reviewers to look at. The
> > author wouldn't have to describe the difference in the
> > commit message as Stefan has (helpfully) done here.
> >
> >
> >
> > 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 :|
> >