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

[Igor Mammedov <imammedo@redhat.com>]

On Thu,  7 Oct 2021 19:27:49 +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 .
> 
> @@ -1,60 +1,38 @@
>  /*
>   * 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/q35/DSDT.multi-bridge, Thu Oct  7 18:56:05 2021
> + * Disassembly of /tmp/aml-AN0DA1, Thu Oct  7 18:56:05 2021
>   *
>   * Original Table Header:
>   *     Signature        "DSDT"
> - *     Length           0x000020FE (8446)
> + *     Length           0x00002187 (8583)
>   *     Revision         0x01 **** 32-bit table (V1), no 64-bit math support
> - *     Checksum         0xDE
> + *     Checksum         0x8D
>   *     OEM ID           "BOCHS "
>   *     OEM Table ID     "BXPC    "
>   *     OEM Revision     0x00000001 (1)
>   *     Compiler ID      "BXPC"
>   *     Compiler Version 0x00000001 (1)
>   */
>  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.S19_.PCNT, MethodObj)    // Warning: Unknown method, guessing 1 arguments
> -
>      Scope (\)
>      {
>          OperationRegion (DBG, SystemIO, 0x0402, One)
>          Field (DBG, ByteAcc, NoLock, Preserve)
>          {
>              DBGB,   8
>          }
> 
>          Method (DBUG, 1, NotSerialized)
>          {
>              ToHexString (Arg0, Local0)
>              ToBuffer (Local0, Local0)
>              Local1 = (SizeOf (Local0) - One)
>              Local2 = Zero
>              While ((Local2 < Local1))
>              {
> @@ -3322,24 +3300,60 @@
>                  Method (DVNT, 2, NotSerialized)
>                  {
>                      If ((Arg0 & One))
>                      {
>                          Notify (S00, Arg1)
>                      }
>                  }
> 
>                  Method (PCNT, 0, NotSerialized)
>                  {
>                      BNUM = One
>                      DVNT (PCIU, One)
>                      DVNT (PCID, 0x03)
>                  }
>              }
> 
> +            Device (S19)
> +            {
> +                Name (_ADR, 0x00030001)  // _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)
>              {
> -                ^S19.PCNT (^S10.PCNT ())
> +                ^S19.PCNT ()
> +                ^S10.PCNT ()
>              }
>          }
>      }
>  }
> 
> Signed-off-by: Ani Sinha <ani@anisinha.ca>

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

> ---
>  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..33107caeed 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("-device pcie-root-port,id=pcie-root-port-0,"
> +                  "multifunction=on,"
> +                  "port=0x0,chassis=1,addr=0x2,bus=pcie.0 "
> +                  "-device pcie-root-port,id=pcie-root-port-1,"
> +                  "port=0x1,chassis=2,addr=0x3.0x1,bus=pcie.0 "
> +                  "-device virtio-balloon,id=balloon0,"
> +                  "bus=pcie.0,addr=0x4.0x2",
> +                  &data);
> +    free_test_data(&data);
> +}
> +
>  static void test_acpi_q35_tcg_mmio64(void)
>  {
>      test_data data = {
> @@ -1534,6 +1551,7 @@ int main(int argc, char *argv[])
>                         test_acpi_piix4_no_acpi_pci_hotplug);
>          qtest_add_func("acpi/q35", test_acpi_q35_tcg);
>          qtest_add_func("acpi/q35/bridge", test_acpi_q35_tcg_bridge);
> +        qtest_add_func("acpi/q35/multif-bridge", test_acpi_q35_multif_bridge);
>          qtest_add_func("acpi/q35/mmio64", test_acpi_q35_tcg_mmio64);
>          qtest_add_func("acpi/piix4/ipmi", test_acpi_piix4_tcg_ipmi);
>          qtest_add_func("acpi/q35/ipmi", test_acpi_q35_tcg_ipmi);