Re: [PATCH v3 2/3] tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35

[Ani Sinha]

On Thu, 7 Oct 2021, Igor Mammedov wrote:

> On Thu, 7 Oct 2021 13:45:36 +0530 (IST)
> Ani Sinha <ani@anisinha.ca> wrote:
>
> > On Thu, 7 Oct 2021, Igor Mammedov wrote:
> >
> > > On Mon, 20 Sep 2021 12:30:46 +0530
> > > Ani Sinha <ani@anisinha.ca> wrote:
> > >
> > > > commit d7346e614f4ec ("acpi: x86: pcihp: add support hotplug on 
> > > > multifunction bridges")
> > > > added ACPI hotplug descriptions for cold plugged bridges for functions 
> > > > other
> > > > than 0. For all other devices, the ACPI hotplug descriptions are 
> > > > limited to
> > > > function 0 only. This change adds unit tests for this feature.
> > > >
> > > > This test adds the following devices to qemu and then checks the changes
> > > > introduced in the DSDT table due to the addition of the following 
> > > > devices:
> > > >
> > > > (a) a multifunction bridge device
> > > > (b) a bridge device with function 1
> > > > (c) a non-bridge device with function 2
> > > >
> > > > In the DSDT table, we should see AML hotplug descriptions for (a) and 
> > > > (b).
> > > > For (a) we should find a hotplug AML description for function 0.
> > > >
> > > > The following diff compares the DSDT table AML with the new unit test 
> > > > before
> > > > and after the change d7346e614f4ec is introduced. In other words,
> > > > this diff reflects the changes that occurs in the DSDT table due to the 
> > > > change
> > > > d7346e614f4ec .
> > > >
> > > > @@ -5,13 +5,13 @@
> > > >   *
> > > >   * Disassembling to symbolic ASL+ operators
> > > >   *
> > > > - * Disassembly of /tmp/aml-7A7890, Sat Sep 18 13:13:29 2021
> > > > + * Disassembly of /tmp/aml-PE4S90, Sat Sep 18 13:08:54 2021
> > > >   *
> > > >   * Original Table Header:
> > > >   *     Signature        "DSDT"
> > > > - *     Length           0x0000206A (8298)
> > > > + *     Length           0x000020F3 (8435)
> > > >   *     Revision         0x01 **** 32-bit table (V1), no 64-bit math 
> > > > support
> > > > - *     Checksum         0x59
> > > > + *     Checksum         0x1B
> > > >   *     OEM ID           "BOCHS "
> > > >   *     OEM Table ID     "BXPC    "
> > > >   *     OEM Revision     0x00000001 (1)
> > > > @@ -20,28 +20,6 @@
> > > >   */
> > > >  DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPC    ", 0x00000001)
> > > >  {
> > > > -    /*
> > > > -     * iASL Warning: There was 1 external control method found during
> > > > -     * disassembly, but only 0 were resolved (1 unresolved). Additional
> > > > -     * ACPI tables may be required to properly disassemble the code. 
> > > > This
> > > > -     * resulting disassembler output file may not compile because the
> > > > -     * disassembler did not know how many arguments to assign to the
> > > > -     * unresolved methods. Note: SSDTs can be dynamically loaded at
> > > > -     * runtime and may or may not be available via the host OS.
> > > > -     *
> > > > -     * In addition, the -fe option can be used to specify a file 
> > > > containing
> > > > -     * control method external declarations with the associated method
> > > > -     * argument counts. Each line of the file must be of the form:
> > > > -     *     External (<method pathname>, MethodObj, <argument count>)
> > > > -     * Invocation:
> > > > -     *     iasl -fe refs.txt -d dsdt.aml
> > > > -     *
> > > > -     * The following methods were unresolved and many not compile 
> > > > properly
> > > > -     * because the disassembler had to guess at the number of arguments
> > > > -     * required for each:
> > > > -     */
> > > > -    External (_SB_.PCI0.S09_.PCNT, MethodObj)    // Warning: Unknown 
> > > > method, guessing 1 arguments
> > > > -
> > > >      Scope (\)
> > > >      {
> > > >          OperationRegion (DBG, SystemIO, 0x0402, One)
> > > > @@ -3280,9 +3258,45 @@
> > > >                  }
> > > >              }
> > > >
> > > > +            Device (S09)
> > > > +            {
> > > > +                Name (_ADR, 0x00010001)  // _ADR: Address
> > > > +                Name (BSEL, Zero)
> > > > +                Device (S00)
> > > > +                {
> > > > +                    Name (_SUN, Zero)  // _SUN: Slot User Number
> > > > +                    Name (_ADR, Zero)  // _ADR: Address
> > > > +                    Method (_EJ0, 1, NotSerialized)  // _EJx: Eject 
> > > > Device, x=0-9
> > > > +                    {
> > > > +                        PCEJ (BSEL, _SUN)
> > > > +                    }
> > > > +
> > > > +                    Method (_DSM, 4, Serialized)  // _DSM: 
> > > > Device-Specific Method
> > > > +                    {
> > > > +                        Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, 
> > > > _SUN))
> > > > +                    }
> > > > +                }
> > > > +
> > > > +                Method (DVNT, 2, NotSerialized)
> > > > +                {
> > > > +                    If ((Arg0 & One))
> > > > +                    {
> > > > +                        Notify (S00, Arg1)
> > > > +                    }
> > > > +                }
> > > > +
> > > > +                Method (PCNT, 0, NotSerialized)
> > > > +                {
> > > > +                    BNUM = Zero
> > > > +                    DVNT (PCIU, One)
> > > > +                    DVNT (PCID, 0x03)
> > > > +                }
> > > > +            }
> > > > +
> > > >              Method (PCNT, 0, NotSerialized)
> > > >              {
> > > > -                ^S09.PCNT (^S08.PCNT ())
> > > > +                ^S09.PCNT ()
> > > > +                ^S08.PCNT ()
> > > >              }
> > > >          }
> > > >      }
> > > >
> > > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > > ---
> > > >  tests/qtest/bios-tables-test.c | 18 ++++++++++++++++++
> > > >  1 file changed, 18 insertions(+)
> > > >
> > > > diff --git a/tests/qtest/bios-tables-test.c 
> > > > b/tests/qtest/bios-tables-test.c
> > > > index 4f11d03055..d4cd77ea02 100644
> > > > --- a/tests/qtest/bios-tables-test.c
> > > > +++ b/tests/qtest/bios-tables-test.c
> > > > @@ -859,6 +859,23 @@ static void test_acpi_q35_tcg_bridge(void)
> > > >      free_test_data(&data);
> > > >  }
> > > >
> > > > +static void test_acpi_q35_multif_bridge(void)
> > > > +{
> > > > +    test_data data = {
> > > > +        .machine = MACHINE_Q35,
> > > > +        .variant = ".multi-bridge",
> > > > +    };
> > > > +    test_acpi_one("-nodefaults -device 
> > > > pcie-root-port,id=pcie-root-port-0,"
> > >
> > > what's the reason for using "-nodefaults" here?
> >
> > I have tried to use the same commandline that commit d7346e614f4ec
> > mentions as the repro case for the issue.
>
> if test can work without 'nodefaults', it would be better as it
> introduces not related diff hunk in the following patch.
>
> Other than that patch looks good to me.

yes that is a fair comment. I have sent a v3 with nodefaults removed. I
had to adjust the test a bit because we now have VGA and other devices
occupying certain slots.