Re: [PATCH v10 13/13] tests/acpi: add DSDT.hpbrroot DSDT table blob to test global i440fx hotplug

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

On Wed, Sep 30, 2020 at 01:09:09PM +0530, Ani Sinha wrote:
> 
> 
> On Wed, Sep 30, 2020 at 1:06 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>     On Tue, Sep 29, 2020 at 06:03:00PM +0530, Ani Sinha wrote:
> 
>     > On Tue, Sep 29, 2020 at 5:05 PM Michael S. Tsirkin <mst@redhat.com>
>     wrote:
> 
>     > >
> 
>     > > On Tue, Sep 29, 2020 at 04:58:03PM +0530, Ani Sinha wrote:
> 
>     > > > On Tue, Sep 29, 2020 at 4:45 PM Michael S. Tsirkin <mst@redhat.com>
>     wrote:
> 
>     > > > >
> 
>     > > > > On Tue, Sep 29, 2020 at 04:35:50PM +0530, Ani Sinha wrote:
> 
>     > > > > > On Tue, Sep 29, 2020 at 4:25 PM Michael S. Tsirkin <
>     mst@redhat.com> wrote:
> 
>     > > > > > >
> 
>     > > > > > > On Tue, Sep 29, 2020 at 04:11:45PM +0530, Ani Sinha wrote:
> 
>     > > > > > > > On Tue, Sep 29, 2020 at 4:07 PM Michael S. Tsirkin <
>     mst@redhat.com> wrote:
> 
>     > > > > > > > >
> 
>     > > > > > > > > On Tue, Sep 29, 2020 at 04:02:07PM +0530, Ani Sinha wrote:
> 
>     > > > > > > > > > On Tue, Sep 29, 2020 at 4:00 PM Ani Sinha <
>     ani@anisinha.ca> wrote:
> 
>     > > > > > > > > > >
> 
>     > > > > > > > > > > In your pull request the following patch is completely
>     screwed up:
> 
>     > > > > > > > > > >
> 
>     > > > > > > > > > > commit cda2006eded0ed91974e1d9e7f9f288e65812a3e
> 
>     > > > > > > > > > > Author: Ani Sinha <ani@anisinha.ca>
> 
>     > > > > > > > > > > Date:   Tue Sep 29 03:22:52 2020 -0400
> 
>     > > > > > > > > > >
> 
>     > > > > > > > > > >     tests/acpi: update golden master DSDT binary table
>     blobs for q35
> 
>     > > > > > > > > > >
> 
>     > > > > > > > > > >
> 
>     > > > > > > > > > > This is not my patch. It has all sorts of changes which
>     does not
> 
>     > > > > > > > > > > belong there. Can you please check?
> 
>     > > > > > > > > >
> 
>     > > > > > > > > > See https://patchew.org/QEMU/
>     20200929071948.281157-1-mst@redhat.com/
>     20200929071948.281157-46-mst@redhat.com/
> 
>     > > > > > > > >
> 
>     > > > > > > > >
> 
>     > > > > > > > > I had to regenerate the binary, yes. That's par for the
>     course.
> 
>     > > > > > > > > But it looks like I added disasssembled files. Will fix up
>     and drop,
> 
>     > > > > > > > > thanks for noticing this.
> 
>     > > > > > >
> 
>     > > > > > > OK I pushed out a fixed variant. Pls take a look.
> 
>     > > > > >
> 
>     > > > > > OK I am not used to this workflow. How am I supposed to get it?
>     Which tag?
> 
>     > > > >
> 
>     > > > > New for_upstream tag - I just sent in a pull request.
> 
>     > > >
> 
>     > > > Can you please point me to your tree?
> 
>     > >
> 
>     > >
> 
>     > >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> 
>     >
> 
>     > I have sent the updated patches based on your pull request tag. I just
> 
>     > had to regenrated the blob for tests/data/acpi/pc/DSDT.hpbrroot.
> 
>     >
> 
>     > make && make check-qtest-x86_64 V=1 passes.
> 
>     >
> 
>     > The diff looks good.
> 
>     >
> 
>     > Can you please send a pull request with these two patches ASAP?
> 
> 
> 
> 
> 
>     Thanks, I will queue them and merge in the next pull request.
> 
> 
> I'm willing to get down on my knees begging you to just do one another pull
> request for these two patches. Were so close with my entire work merged.
> 
> Please let's not wait another week or so. 


OK it's not too much work but ... could you please add justification
about why adding this one unit test is needed so urgently?
That motivation would be quite helpful for the pull request.


> 
> 
> 
> 
> 
>     > >
> 
>     > >
> 
>     > >
> 
>     > > > >
> 
>     > > > > >
> 
>     > > > > > >
> 
>     > > > > > > > I think DSDT.hbridge is wrong. The checksum looks weird:
> 
>     > > > > > > >
> 
>     > > > > > > >
> 
>     > > > > > > > + *     Length           0x00000B89 (2953)
> 
>     > > > > > > >   *     Revision         0x01 **** 32-bit table (V1), no
>     64-bit math support
> 
>     > > > > > > > - *     Checksum         0x05
> 
>     > > > > > >
> 
>     > > > > > > What is weird about it?
> 
>     > > > > > >
> 
>     > > > > > > >
> 
>     > > > > > > > This file should be introduced just by one patch. my patch.
> 
>     > > > > > >
> 
>     > > > > > > I just re-run rebuild-expected-aml, no changes.
> 
>     > > > > > >
> 
>     > > > > > > I have this:
> 
>     > > > > > > commit 5e3a486211f02d9ecb18939ca21087515ec81883
> 
>     > > > > > > Author: Ani Sinha <ani@anisinha.ca>
> 
>     > > > > > > Date:   Fri Sep 18 14:11:05 2020 +0530
> 
>     > > > > > >
> 
>     > > > > > >     tests/acpi: unit test for
>     'acpi-pci-hotplug-with-bridge-support' bridge flag
> 
>     > > > > > >
> 
>     > > > > > >     This change adds a new unit test for the global flag
> 
>     > > > > > >     'acpi-pci-hotplug-with-bridge-support' which is available
>     for cold plugged pci
> 
>     > > > > > >     bridges in i440fx. The flag can be used to turn off ACPI
>     based hotplug support
> 
>     > > > > > >     on all pci bridges.
> 
>     > > > > > >
> 
>     > > > > > >
> 
>     > > > > > > Here is the full DSDT header, attached:
> 
>     > > > > > >
> 
>     > > > > > > /*
> 
>     > > > > > >  * Intel ACPI Component Architecture
> 
>     > > > > > >  * AML/ASL+ Disassembler version 20190509 (64-bit version)
> 
>     > > > > > >  * Copyright (c) 2000 - 2019 Intel Corporation
> 
>     > > > > > >  *
> 
>     > > > > > >  * Disassembling to symbolic ASL+ operators
> 
>     > > > > > >  *
> 
>     > > > > > >  * Disassembly of tests/data/acpi/pc/DSDT.hpbridge, Tue Sep 29
>     06:51:03 2020
> 
>     > > > > > >  *
> 
>     > > > > > >  * Original Table Header:
> 
>     > > > > > >  *     Signature        "DSDT"
> 
>     > > > > > >  *     Length           0x0000139D (5021)
> 
>     > > > > > >  *     Revision         0x01 **** 32-bit table (V1), no 64-bit
>     math support
> 
>     > > > > > >  *     Checksum         0x05
> 
>     > > > > > >  *     OEM ID           "BOCHS "
> 
>     > > > > > >  *     OEM Table ID     "BXPCDSDT"
> 
>     > > > > > >  *     OEM Revision     0x00000001 (1)
> 
>     > > > > > >  *     Compiler ID      "BXPC"
> 
>     > > > > > >  *     Compiler Version 0x00000001 (1)
> 
>     > > > > > >  */
> 
>     > > > > > > DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPCDSDT",
>     0x00000001)
> 
>     > > > > > >
> 
>     > > > > > > --
> 
>     > > > > > > MST
> 
>     > > > > > >
> 
>     > > > >
> 
>     > >
> 
> 
> 
>