* [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction
@ 2021-08-06 17:46 Ani Sinha
2021-08-06 17:46 ` [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35 Ani Sinha
0 siblings, 1 reply; 10+ messages in thread
From: Ani Sinha @ 2021-08-06 17:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Ani Sinha, imammedo, Michael S. Tsirkin
Hi Igor :
This is the new unit test which I wanted to add for your change related to adding multifunction hotplug support.
This is not the final patchset. I wanted to run this by you first to make sure things are looking
good before sending out the patch series for this unit test with blobs etc added as per the rules.
Please let me know if we need to make any changes to the test arguments or add/remove anything.
Thanks
Ani
^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35
2021-08-06 17:46 [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction Ani Sinha
@ 2021-08-06 17:46 ` Ani Sinha
2021-08-16 15:44 ` Ani Sinha
2021-09-17 13:32 ` Igor Mammedov
0 siblings, 2 replies; 10+ messages in thread
From: Ani Sinha @ 2021-08-06 17:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Ani Sinha, imammedo, Michael S. Tsirkin
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:
@@ -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 <ani@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)
+ };
+ /*
+ * lets try three things:
+ * (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.
+ */
+ 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);
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35
2021-08-06 17:46 ` [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35 Ani Sinha
@ 2021-08-16 15:44 ` Ani Sinha
2021-09-01 12:18 ` Ani Sinha
2021-09-17 13:32 ` Igor Mammedov
1 sibling, 1 reply; 10+ messages in thread
From: Ani Sinha @ 2021-08-16 15:44 UTC (permalink / raw)
To: Ani Sinha; +Cc: imammedo, qemu-devel, Michael S. Tsirkin
Ping ...
Igor, if this looks ok, I will send the entire patch series with the blobs
updated.
On Fri, 6 Aug 2021, Ani Sinha 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:
>
> @@ -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 <ani@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)
> + };
> + /*
> + * lets try three things:
> + * (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.
> + */
> + 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);
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35
2021-08-16 15:44 ` Ani Sinha
@ 2021-09-01 12:18 ` Ani Sinha
0 siblings, 0 replies; 10+ messages in thread
From: Ani Sinha @ 2021-09-01 12:18 UTC (permalink / raw)
To: Ani Sinha; +Cc: Igor Mammedov, QEMU Developers, Michael S. Tsirkin
Ping ...
On Mon, Aug 16, 2021 at 9:14 PM Ani Sinha <ani@anisinha.ca> wrote:
>
> Ping ...
>
> Igor, if this looks ok, I will send the entire patch series with the blobs
> updated.
>
> On Fri, 6 Aug 2021, Ani Sinha 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:
> >
> > @@ -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 <ani@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)
> > + };
> > + /*
> > + * lets try three things:
> > + * (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.
> > + */
> > + 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);
> > --
> > 2.25.1
> >
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35
2021-08-06 17:46 ` [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35 Ani Sinha
2021-08-16 15:44 ` Ani Sinha
@ 2021-09-17 13:32 ` Igor Mammedov
2021-09-19 2:49 ` Ani Sinha
1 sibling, 1 reply; 10+ messages in thread
From: Igor Mammedov @ 2021-09-17 13:32 UTC (permalink / raw)
To: Ani Sinha; +Cc: qemu-devel, Michael S. Tsirkin
On Fri, 6 Aug 2021 23:16:42 +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.
>
> 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 <ani@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);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35
2021-09-17 13:32 ` Igor Mammedov
@ 2021-09-19 2:49 ` Ani Sinha
2021-09-19 2:59 ` Ani Sinha
0 siblings, 1 reply; 10+ messages in thread
From: Ani Sinha @ 2021-09-19 2:49 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Ani Sinha, qemu-devel, Michael S. Tsirkin
On Fri, 17 Sep 2021, Igor Mammedov wrote:
> On Fri, 6 Aug 2021 23:16:42 +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.
> >
> > 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)
No. this diff is correct. This is the diff of the DSDT table before and
after appplying your change with the same unit test. So this diff shows
what effectively changes in the DSDT table when your fix
d7346e614f4ec ("acpi: x86: pcihp: add support hotplug on multifunction
bridges")
is applied. So I think it is important to capture this data. I will
clarify the diff more clearly in the commit log in the next version.
>
> > @@ -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 <ani@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?
This verifies the smbios struct. It seems most of the other tests uses it.
So I left it in this test also.
Which of the tests should not be testing smbios? Maybe we can remove this
from other tests (even the ones that I added earlier)? I wasnt' sure so
maybe you can clarify.
>
> > + };
> > + /*
> > + * 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
OK will do in next revision.
>
> > + 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);
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35
2021-09-19 2:49 ` Ani Sinha
@ 2021-09-19 2:59 ` Ani Sinha
2021-09-19 11:29 ` Ani Sinha
2021-09-20 6:03 ` Igor Mammedov
0 siblings, 2 replies; 10+ messages in thread
From: Ani Sinha @ 2021-09-19 2:59 UTC (permalink / raw)
To: Ani Sinha; +Cc: Igor Mammedov, qemu-devel, Michael S. Tsirkin
On Sun, 19 Sep 2021, Ani Sinha wrote:
>
>
> On Fri, 17 Sep 2021, Igor Mammedov wrote:
>
> > On Fri, 6 Aug 2021 23:16:42 +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.
> > >
> > > 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)
>
> No. this diff is correct. This is the diff of the DSDT table before and
> after appplying your change with the same unit test. So this diff shows
> what effectively changes in the DSDT table when your fix
>
> d7346e614f4ec ("acpi: x86: pcihp: add support hotplug on multifunction
> bridges")
>
> is applied. So I think it is important to capture this data. I will
> clarify the diff more clearly in the commit log in the next version.
>
>
> >
> > > @@ -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 <ani@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?
>
> This verifies the smbios struct. It seems most of the other tests uses it.
> So I left it in this test also.
> Which of the tests should not be testing smbios?
Right now smbios is only tested for non-uefi firmware. There are lots
of tests that does not use uefi yet exercize the smbios struct tests.
For example:
test_acpi_piix4_tcg
test_acpi_piix4_tcg_bridge
test_acpi_piix4_no_root_hotplug
test_acpi_piix4_no_bridge_hotplug
test_acpi_piix4_no_acpi_pci_hotplug
test_acpi_q35_tcg
test_acpi_q35_tcg_bridge
test_acpi_q35_tcg_mmio64
test_acpi_q35_tcg_ipmi
test_acpi_piix4_tcg_ipmi
Should the smbios struct verification tests be removed from all of them?
> Maybe we can remove this
> from other tests (even the ones that I added earlier)? I wasnt' sure so
> maybe you can clarify.
>
>
> >
> > > + };
> > > + /*
> > > + * 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
>
> OK will do in next revision.
>
> >
> > > + 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);
> >
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35
2021-09-19 2:59 ` Ani Sinha
@ 2021-09-19 11:29 ` Ani Sinha
2021-09-20 6:03 ` Igor Mammedov
1 sibling, 0 replies; 10+ messages in thread
From: Ani Sinha @ 2021-09-19 11:29 UTC (permalink / raw)
To: Ani Sinha; +Cc: Igor Mammedov, qemu-devel, Michael S. Tsirkin
On Sun, 19 Sep 2021, Ani Sinha wrote:
> > > > +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?
> >
> > This verifies the smbios struct. It seems most of the other tests uses it.
> > So I left it in this test also.
> > Which of the tests should not be testing smbios?
>
> Right now smbios is only tested for non-uefi firmware. There are lots
> of tests that does not use uefi yet exercize the smbios struct tests.
> For example:
>
> test_acpi_piix4_tcg
> test_acpi_piix4_tcg_bridge
> test_acpi_piix4_no_root_hotplug
> test_acpi_piix4_no_bridge_hotplug
> test_acpi_piix4_no_acpi_pci_hotplug
> test_acpi_q35_tcg
> test_acpi_q35_tcg_bridge
> test_acpi_q35_tcg_mmio64
> test_acpi_q35_tcg_ipmi
> test_acpi_piix4_tcg_ipmi
>
> Should the smbios struct verification tests be removed from all of them?
To answer my own question ...
> test_acpi_q35_tcg
This uses smbios options directly. So we need to keep smbios specific
verifications.
> test_acpi_piix4_no_root_hotplug
> test_acpi_piix4_no_bridge_hotplug
> test_acpi_piix4_no_acpi_pci_hotplug
I think smbios specific struct tests can be removed from all of the above.
They just deal with DSDT tables.
Out of the rest,
> test_acpi_piix4_tcg
> test_acpi_piix4_tcg_bridge
> test_acpi_q35_tcg_bridge
I am not sure for the above three. I am not sure if the options passed
indirectly affect smbios. Probably not?
> test_acpi_q35_tcg_mmio64
> test_acpi_q35_tcg_ipmi
> test_acpi_piix4_tcg_ipmi
For the above three, we probably need the smbios checks. Again I am not
100% certain.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35
2021-09-19 2:59 ` Ani Sinha
2021-09-19 11:29 ` Ani Sinha
@ 2021-09-20 6:03 ` Igor Mammedov
2021-09-20 7:04 ` Ani Sinha
1 sibling, 1 reply; 10+ messages in thread
From: Igor Mammedov @ 2021-09-20 6:03 UTC (permalink / raw)
To: Ani Sinha; +Cc: qemu-devel, Michael S. Tsirkin
On Sun, 19 Sep 2021 08:29:51 +0530 (IST)
Ani Sinha <ani@anisinha.ca> wrote:
> On Sun, 19 Sep 2021, Ani Sinha wrote:
>
> >
> >
> > On Fri, 17 Sep 2021, Igor Mammedov wrote:
> >
> > > On Fri, 6 Aug 2021 23:16:42 +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.
> > > >
> > > > 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)
> >
> > No. this diff is correct. This is the diff of the DSDT table before and
> > after appplying your change with the same unit test. So this diff shows
> > what effectively changes in the DSDT table when your fix
> >
> > d7346e614f4ec ("acpi: x86: pcihp: add support hotplug on multifunction
> > bridges")
> >
> > is applied. So I think it is important to capture this data. I will
> > clarify the diff more clearly in the commit log in the next version.
> >
> >
> > >
> > > > @@ -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 <ani@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?
> >
> > This verifies the smbios struct. It seems most of the other tests uses it.
> > So I left it in this test also.
> > Which of the tests should not be testing smbios?
>
> Right now smbios is only tested for non-uefi firmware. There are lots
> of tests that does not use uefi yet exercize the smbios struct tests.
> For example:
>
> test_acpi_piix4_tcg
> test_acpi_piix4_tcg_bridge
> test_acpi_piix4_no_root_hotplug
> test_acpi_piix4_no_bridge_hotplug
> test_acpi_piix4_no_acpi_pci_hotplug
> test_acpi_q35_tcg
> test_acpi_q35_tcg_bridge
> test_acpi_q35_tcg_mmio64
> test_acpi_q35_tcg_ipmi
> test_acpi_piix4_tcg_ipmi
>
> Should the smbios struct verification tests be removed from all of them?
I'd leave them alone, and just remove smbios testing from this patch.
>
>
> > Maybe we can remove this
> > from other tests (even the ones that I added earlier)? I wasnt' sure so
> > maybe you can clarify.
>
>
> >
> >
> > >
> > > > + };
> > > > + /*
> > > > + * 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
> >
> > OK will do in next revision.
> >
> > >
> > > > + 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);
> > >
> > >
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35
2021-09-20 6:03 ` Igor Mammedov
@ 2021-09-20 7:04 ` Ani Sinha
0 siblings, 0 replies; 10+ messages in thread
From: Ani Sinha @ 2021-09-20 7:04 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Ani Sinha, qemu-devel, Michael S. Tsirkin
On Mon, 20 Sep 2021, Igor Mammedov wrote:
> > > > > 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?
> > >
> > > This verifies the smbios struct. It seems most of the other tests uses it.
> > > So I left it in this test also.
> > > Which of the tests should not be testing smbios?
> >
> > Right now smbios is only tested for non-uefi firmware. There are lots
> > of tests that does not use uefi yet exercize the smbios struct tests.
> > For example:
> >
> > test_acpi_piix4_tcg
> > test_acpi_piix4_tcg_bridge
> > test_acpi_piix4_no_root_hotplug
> > test_acpi_piix4_no_bridge_hotplug
> > test_acpi_piix4_no_acpi_pci_hotplug
> > test_acpi_q35_tcg
> > test_acpi_q35_tcg_bridge
> > test_acpi_q35_tcg_mmio64
> > test_acpi_q35_tcg_ipmi
> > test_acpi_piix4_tcg_ipmi
> >
> > Should the smbios struct verification tests be removed from all of them?
>
> I'd leave them alone, and just remove smbios testing from this patch.
>
I have sent a v3. Please ignore v2.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-09-20 7:05 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-06 17:46 [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction Ani Sinha
2021-08-06 17:46 ` [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35 Ani Sinha
2021-08-16 15:44 ` Ani Sinha
2021-09-01 12:18 ` Ani Sinha
2021-09-17 13:32 ` Igor Mammedov
2021-09-19 2:49 ` Ani Sinha
2021-09-19 2:59 ` Ani Sinha
2021-09-19 11:29 ` Ani Sinha
2021-09-20 6:03 ` Igor Mammedov
2021-09-20 7:04 ` Ani Sinha
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.