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

[Michael S. Tsirkin]

On Wed, Sep 30, 2020 at 01:17:53PM +0530, Ani Sinha wrote:
> 
> 
> On Wed, Sep 30, 2020 at 1:14 PM Michael S. Tsirkin <m...@redhat.com> wrote:
> 
>     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 <m...@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 
> <m...@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 <
>     m...@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 <
> 
>     >     m...@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 
> <
> 
>     >     m...@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 <
> 
>     >     a...@anisinha.ca> wrote:
> 
>     >
> 
>     >     > > > > > > > > > >
> 
>     >
> 
>     >     > > > > > > > > > > In your pull request the following patch is
>     completely
> 
>     >     screwed up:
> 
>     >
> 
>     >     > > > > > > > > > >
> 
>     >
> 
>     >     > > > > > > > > > > commit 
> cda2006eded0ed91974e1d9e7f9f288e65812a3e
> 
>     >
> 
>     >     > > > > > > > > > > Author: Ani Sinha <a...@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-...@redhat.com/
> 
>     >     20200929071948.281157-46-...@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.
> 
> 
> A patch without unit test doesn't complete the patch work. A unit test makes
> sure that the change would not get broken by other changes that come in later.
> Typically all code changes are accompanied by unit test in the same patch.
> Hence since the main work has already been merged, the unit test should merge
> ASAP so that no breakage can happen in between.
> 
> Plus this completes an entire series of work which I've been working for a
> while. I really would love to see it all merged cleanly and fully completed.
> 

Absolutely, thanks for the great work!
I am not sure that's a good justification to rushing a pull request
though ... are you waiting to get paid and it hinges on the test, or are
under a deadline, or something like this? It's okay to say so if so.

> 
> 
> 
> 
> 
> 
>     >
> 
>     >
> 
>     >
> 
>     >
> 
>     >
> 
>     >     > >
> 
>     >
> 
>     >     > >
> 
>     >
> 
>     >     > >
> 
>     >
> 
>     >     > > > >
> 
>     >
> 
>     >     > > > > >
> 
>     >
> 
>     >     > > > > > >
> 
>     >
> 
>     >     > > > > > > > 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 <a...@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
> 
>     >
> 
>     >     > > > > > >
> 
>     >
> 
>     >     > > > >
> 
>     >
> 
>     >     > >
> 
>     >
> 
>     >
> 
>     >
> 
>     >
> 
> 
> 
>