| Message ID | 20210920070047.3937292-3-ani@anisinha.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35 | expand |
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? > + "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 = { > @@ -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);
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. > > > + "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 = { > > @@ -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); > >
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. > > > > > > > + "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 = { > > > @@ -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); > > > > >
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.
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," + "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 = { @@ -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);
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(+)