Re: OVMF and PCI0 UID

* Re: OVMF and PCI0 UID
       [not found] <56E4DCD4-DBA1-4A41-8568-1CBBB37ED320@protonmail.com>
@ 2020-07-20 21:25 ` Laszlo Ersek
  2020-07-21  6:58   ` Michael S. Tsirkin
  0 siblings, 1 reply; 5+ messages in thread
From: Laszlo Ersek @ 2020-07-20 21:25 UTC (permalink / raw)
  To: vit9696; +Cc: Michael Tsirkin, Igor Mammedov, mhaeuser, qemu devel list

Hi Vitaly,

adding Igor, Michael, Marcel, and qemu-devel.

On 07/20/20 11:06, vit9696 wrote:
> Hello,
>
> I discovered an issue with inconsistent QEMU/OVMF device paths, and
> while I am unsure whether directing this e-mail is appropriate to you,
> I believe that you likely have the contacts you could forward this
> e-mail to.
>
> macOS uses ACPI UIDs to build the DevicePath for NVRAM boot options,
> while OVMF firmware gets them via an internal channel through QEMU.
> Due to a bug in QEMU (or OVMF) currently UEFI firmware and ACPI have
> different values, and this makes the underlying operating system
> unable to report its boot option.
>
> The particular node in question is the primary PciRoot (PCI0 in ACPI),
> which for some reason gets assigned 1 in ACPI UID and 0 in the
> DevicePath. To me this looks like a bug here:
> https://github.com/qemu/qemu/blob/8f06f22/hw/i386/acpi-build.c#L1511-L1515
> Which does not correspond to the primary PCI identifier here:
> https://github.com/qemu/qemu/blob/5a79d10/hw/pci/pci.c#L160-L162
>
> Reference with the device paths, OVMF startup logs, and ACPI table
> dumps (SysReport):
> https://github.com/acidanthera/bugtracker/issues/1050
>
> Would you be able to forward this to the right people or perhaps keep
> an eye on the issue itself?

I think you are right.

In UEFI v2.8, section "10.4.2 Rules with ACPI _HID and _UID" ends with
the paragraph,

    Root PCI bridges will use the plug and play ID of PNP0A03, This will
    be stored in the ACPI Device Path _HID field, or in the Expanded
    ACPI Device Path _CID field to match the ACPI name space. The _UID
    in the ACPI Device Path structure must match the _UID in the ACPI
    name space.

(See especially the last sentence.)

Considering *extra* root bridges / root buses (with bus number > 0),
QEMU's ACPI generator actually does the right thing; since QEMU commit
c96d9286a6d7 ("i386/acpi-build: more traditional _UID and _HID for PXB
root buses", 2015-06-11).

However, the _UID values for root bridge zero (on both i440fx and q35)
have always been "wrong" (from UEFI perspective), going back in QEMU to
commit 74523b850189 ("i386: add ACPI table files from seabios",
2013-10-14).

Even in SeaBIOS, these _UID values have always been 1; see commit
a4d357638c57 ("Port rombios32 code from bochs-bios.", 2008-03-08) for
i440fx, and commit ecbe3fd61511 ("seabios: q35: add dsdt", 2012-12-01)
for q35.

Does the following patch work for you? (I can see you proposed the same
in
<https://github.com/acidanthera/bugtracker/issues/1050#issuecomment-660734139>)

> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b7bcbbbb2a35..7a5a8b3521b0 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1496,9 +1496,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          sb_scope = aml_scope("_SB");
>          dev = aml_device("PCI0");
>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
>          aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> -        aml_append(dev, aml_name_decl("_UID", aml_int(1)));
> +        aml_append(dev, aml_name_decl("_UID", aml_int(0)));
>          aml_append(sb_scope, dev);
>          aml_append(dsdt, sb_scope);
>
>          build_hpet_aml(dsdt);
> @@ -1511,9 +1511,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          dev = aml_device("PCI0");
>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
>          aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
>          aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> -        aml_append(dev, aml_name_decl("_UID", aml_int(1)));
> +        aml_append(dev, aml_name_decl("_UID", aml_int(0)));
>          aml_append(dev, build_q35_osc_method());
>          aml_append(sb_scope, dev);
>          aml_append(dsdt, sb_scope);

If it does, I suggest submitting the above patch to qemu-devel, and/or
filing a bug for upstream QEMU at <https://bugs.launchpad.net/qemu/>.

(Note: I didn't even compile the above change.)

Thanks
Laszlo

^ permalink raw reply	[flat|nested] 5+ messages in thread