genodelabs / genode

Genode OS Framework
https://genode.org/
Other
1.05k stars 249 forks source link

base-hw: missing XSDT-attribute in platform_info on UP Xtreme Edge (UPX-EDGEI7-A10-1664-F01) #5332

Open rite opened 2 weeks ago

rite commented 2 weeks ago

When using base-nova on this x86_64 system, the platform_info-ROM looks as follows (note the xsdt-attribute of the acpi-node):

<platform_info>
    <kernel name="nova" acpi="true" msi="true" iommu="true"/>
    <efi-system-table address="0x8cb72018"/>
    <acpi revision="2" rsdt="0x8c3dd028" xsdt="0x8c3dd0b8"/>
    <affinity-space width="4" height="2"/>
    <boot/>
    <hardware>
        <features svm="false" vmx="true"/>
        <tsc invariant="true" freq_khz="1992000"/>
        <cpus>
            <cpu xpos="0" ypos="0" id="0" package="0" core="0" thread="0" family="0x6" model="0x8e" stepping="0xc" platform="0x7" patch="0xc6"/>
            <cpu xpos="0" ypos="1" id="4" package="0" core="0" thread="1" family="0x6" model="0x8e" stepping="0xc" platform="0x7" patch="0xc6"/>
            <cpu xpos="1" ypos="0" id="1" package="0" core="1" thread="0" family="0x6" model="0x8e" stepping="0xc" platform="0x7" patch="0xc6"/>
            <cpu xpos="1" ypos="1" id="5" package="0" core="1" thread="1" family="0x6" model="0x8e" stepping="0xc" platform="0x7" patch="0xc6"/>
            <cpu xpos="2" ypos="0" id="2" package="0" core="2" thread="0" family="0x6" model="0x8e" stepping="0xc" platform="0x7" patch="0xc6"/>
            <cpu xpos="2" ypos="1" id="6" package="0" core="2" thread="1" family="0x6" model="0x8e" stepping="0xc" platform="0x7" patch="0xc6"/>
            <cpu xpos="3" ypos="0" id="3" package="0" core="3" thread="0" family="0x6" model="0x8e" stepping="0xc" platform="0x7" patch="0xc6"/>
            <cpu xpos="3" ypos="1" id="7" package="0" core="3" thread="1" family="0x6" model="0x8e" stepping="0xc" platform="0x7" patch="0xc6"/>
        </cpus>
    </hardware>
</platform_info>

The xsdt-attribute is missing when using base-hw:

<platform_info>
    <kernel name="hw" acpi="true" msi="true"/>
    <efi-system-table address="0x8cb72018"/>
    <acpi revision="2" rsdt="0x8c3dd028"/>
    <boot>
        <framebuffer phys="0x0" width="0" height="0" bpp="0" type="0" pitch="0"/>
    </boot>
    <hardware>
        <features svm="false" vmx="true"/>
        <tsc invariant="false" freq_khz="2000000"/>
    </hardware>
    <affinity-space width="8" height="1"/>
</platform_info>

I suspect that this is the reason why the acpi_drv reports for example the fTPM only with base-nova, but not with base-hw.

rite commented 2 weeks ago

I verified that the behavior described above applies to the current master (Release 24.08).

alex-ab commented 2 weeks ago

Please try to instrument repos/base-hw/src/bootstrap/spec/x86_64/platform.cc. There are potential two places where acpi_rsdp is created, which contains the xsdt pointer. On my local test machine (which is not UPX) the xsdt pointer is available.

rite commented 2 weeks ago

Thanks @alex-ab for the pointer. acpi_rsdp is assigned in if (__initial_ax == Multiboot2_info::MAGIC):

--- a/repos/base-hw/src/bootstrap/spec/x86_64/platform.cc
+++ b/repos/base-hw/src/bootstrap/spec/x86_64/platform.cc
@@ -145,6 +145,8 @@ Bootstrap::Platform::Board::Board()
                },
                [&] (Hw::Acpi_rsdp const &rsdp) {
                        /* prefer higher acpi revisions */
+                       log(__FILE__, ":", __LINE__, ", acpi_rsdp.valid: ",    acpi_rsdp.valid(),
+                                                    ", acpi_rsdp.revision: ", acpi_rsdp.revision);
                        if (!acpi_rsdp.valid() || acpi_rsdp.revision < rsdp.revision)
                                acpi_rsdp = rsdp;
                },

...produces the following log output:

repos/base-hw/src/bootstrap/spec/x86_64/platform.cc:148, acpi_rsdp.valid: 0, acpi_rsdp.revision: 0
repos/base-hw/src/bootstrap/spec/x86_64/platform.cc:148, acpi_rsdp.valid: 1, acpi_rsdp.revision: 2

I'm still struggling to understand why the XSDT pointer is not available, even tough the same RSDP is determined with both kernels – on the UPX, that is... 😳

alex-ab commented 2 weeks ago

Thanks, and both acpi_rsdp.xsdt are 0 ? (just print it). Maybe the parsing code is wrong in multiboot2.h. Please have a look for ACPI_RSDP_V1 and ACPI_RSDP_V2. There are some size checks, maybe something is wrong how I did this. Just for test invoke ever acpi_fn() and print the acpi_rsdt.xsdt

rite commented 2 weeks ago

both acpi_rsdp.xsdt are 0 ?

Yes, both are 0.

There are some size checks [...] in multiboot2.h.

🎯

With the following crude hack, the platform_info-ROM now contains the xsdt-attribute (0x8c3dd0b8, as on base-nova):

--- a/repos/base-hw/src/bootstrap/spec/x86_64/multiboot2.h
+++ b/repos/base-hw/src/bootstrap/spec/x86_64/multiboot2.h
@@ -108,11 +108,13 @@ class Genode::Multiboot2_info : Mmio<0x8>

                                        Hw::Acpi_rsdp * rsdp = reinterpret_cast<Hw::Acpi_rsdp *>(rsdp_addr);

-                                       if (tag.read<Tag::Size>() - sizeof_tag == 20) {
+                                       log("tag.read<Tag::Size>() - sizeof_tag: ", tag.read<Tag::Size>() - sizeof_tag);
+
+                                       if (tag.read<Tag::Size>() - sizeof_tag == 36) {
                                                /* ACPI RSDP v1 is 20 byte solely */
                                                Hw::Acpi_rsdp rsdp_v1;
                                                memset (&rsdp_v1, 0, sizeof(rsdp_v1));
-                                               memcpy (&rsdp_v1, rsdp, 20);
+                                               memcpy (&rsdp_v1, rsdp, 36);
                                                acpi_fn(rsdp_v1);
                                        }
                                        if (sizeof(*rsdp) <= tag.read<Tag::Size>() - sizeof_tag)

For the sake of completeness, the debug log output is:

repos/base-hw/src/bootstrap/spec/x86_64/multiboot2.h:111, tag.read<Tag::Size>() - sizeof_tag: 20
repos/base-hw/src/bootstrap/spec/x86_64/multiboot2.h:111, tag.read<Tag::Size>() - sizeof_tag: 36
repos/base-hw/src/bootstrap/spec/x86_64/platform.cc:149, acpi_rsdp.valid: 0, acpi_rsdp.revision: 0, acpi_rsdp.xsdt: 0
repos/base-hw/src/bootstrap/spec/x86_64/platform.cc:149, acpi_rsdp.valid: 1, acpi_rsdp.revision: 2, acpi_rsdp.xsdt: 2352861368
rite commented 2 weeks ago

I suspect that this is the reason why the acpi_drv reports for example the fTPM only with base-nova, but not with base-hw.

btw with the above hack, the ACPI driver now indeed reports the fTPM also with base-hw.

alex-ab commented 2 weeks ago

Currently I have no clue, why this helps. The second acpi_fn(*rsdp) should be invoked, operating on the raw acpi_rsdp value, right (without your change) ? With your change, you make a partial copy and the first acpi_fn is taken. Have to think about it or you demystify it.

rite commented 2 weeks ago

After getting a bit more familiar with this topic, I refined the instrumentation in platform.cc (using the unchanged multiboot2.h).

First invocation of acpi_fn(*rsdp):

repos/base-hw/src/bootstrap/spec/x86_64/platform.cc:149, acpi_rsdp.valid: 0, acpi_rsdp.revision: 0
repos/base-hw/src/bootstrap/spec/x86_64/platform.cc:151, rsdp.valid: 1, rsdp.revision: 2

Second invocation of acpi_fn(*rsdp):

repos/base-hw/src/bootstrap/spec/x86_64/platform.cc:149, acpi_rsdp.valid: 1, acpi_rsdp.revision: 2
repos/base-hw/src/bootstrap/spec/x86_64/platform.cc:151, rsdp.valid: 1, rsdp.revision: 2

Now it's obvious that acpi_rsdp is only updated in the first invocation of acpi_fn(*rsdp) but not the second, because there is already a valid RSDP version 2. I verified that the platform_info-ROM contains the XSDT pointer with the following patch:

--- a/repos/base-hw/src/bootstrap/spec/x86_64/platform.cc
+++ b/repos/base-hw/src/bootstrap/spec/x86_64/platform.cc
@@ -145,7 +145,7 @@ Bootstrap::Platform::Board::Board()
                },
                [&] (Hw::Acpi_rsdp const &rsdp) {
                        /* prefer higher acpi revisions */
-                       if (!acpi_rsdp.valid() || acpi_rsdp.revision < rsdp.revision)
+                       if (!acpi_rsdp.valid() || acpi_rsdp.revision <= rsdp.revision)
                                acpi_rsdp = rsdp;
                },
                [&] (Hw::Framebuffer const &fb) {
alex-ab commented 1 week ago

@rite: Thanks for your detailed analysis. Can you please give f2bd5f6a18e5f8dccafc660e2d011f53a5397b73 a spin ?

Even so the patch with the comparison helps, we may get bite again, in case we find a machine where multiboot2 would put the rsdp_v2 version before rsdp_v1 in the multiboot2 list. In such a case, we would override with the <= patch the rsdp_v2 version (which would contain the xsdt).

Because of that I added to the commit above an explicit rsdp_v2 functor, to make clear that this is what we actually want. The v1 case is just for older machines or emulators, e.g. Qemu.

alex-ab commented 1 week ago

@chelmuth: please check, we may implement it differently. I refrained to iterate 2x times over the multiboot2, which we shortly discussed.

chelmuth commented 1 week ago

I'm fine with the commit that fixes the issue at hand - merged to staging.

rite commented 4 days ago

I confirm that 778406d fixes the issue for the UPX, thank you!