On Fri, 6 Aug 2021 23:16:42 +0530
Ani Sinha <a...@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.
>
> The diff of ACPI DSDT table before and after the change d7346e614f4e with the
> same newly added unit test is provided below:
ASL below should be updated to match actual diff it's spewing out
(I get more than it mentioned below)
> @@ -5,13 +5,13 @@
> *
> * Disassembling to symbolic ASL+ operators
> *
> - * Disassembly of /tmp/aml-35UR70, Fri Aug 6 21:00:03 2021
> + * Disassembly of /tmp/aml-GY8760, Fri Aug 6 21:10:31 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 <a...@anisinha.ca>
> ---
> tests/qtest/bios-tables-test.c | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 51d3a4e239..c92b70e8b8 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -859,6 +859,33 @@ 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",
> + .required_struct_types = base_required_struct_types,
> + .required_struct_types_len = ARRAY_SIZE(base_required_struct_types)
do we care, i.e. why is this here?
> + };
> + /*
> + * lets try three things:
s/try .../test following configuration/
> + * (a) a multifunction bridge device
> + * (b) a bridge device with function 1
> + * (c) a non-bridge device with function 2
> + * We should see AML hotplug descriptions for (a) and (b) in DSDT.
> + * For (a) it should have a hotplug AML description for function 0.
> + */
A little bit hard to parse this comment, maybe explain a bit more
what is being tested
also I'd move this comment into commit message
> + test_acpi_one("-nodefaults -device pcie-root-port,id=pcie-root-port-0,"
> + "multifunction=on,"
> + "port=0x0,chassis=1,addr=0x1,bus=pcie.0 "
> + "-device pcie-root-port,id=pcie-root-port-1,"
> + "port=0x1,chassis=2,addr=0x1.0x1,bus=pcie.0 "
> + "-device virtio-balloon,id=balloon0,"
> + "bus=pcie.0,addr=0x1.0x2",
> + &data);
> + free_test_data(&data);
> +}
> +
> static void test_acpi_q35_tcg_mmio64(void)
> {
> test_data data = {
> @@ -1528,6 +1555,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);
