[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifun
|
From: |
Igor Mammedov |
|
Subject: |
Re: [RFC PATCH] tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35 |
|
Date: |
Mon, 20 Sep 2021 08:03:33 +0200 |
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);
> > >
> > >
> >
>