On Wed, Jan 15, 2025 at 6:23 PM Igor Mammedov <imamm...@redhat.com> wrote:
>
> Current versions of Windows call _DSM(func=7) regardless
> of whether it is supported or not. It leads to NICs having bogus
> 'PCI Label Id = 0', where none should be set at all.
>
> Also presence of 'PCI Label Id' triggers another Windows bug
> on localized versions that leads to hangs. The later bug is fixed
> in latest updates for 'Windows Server' but not in consumer
> versions of Windows (and there is no plans to fix it
> as far as I'm aware).
>
> Given it's easy, implement Microsoft suggested workaround
> (return invalid Package) so that affected Windows versions
> could boot on QEMU.
> This would effectvely remove bogus 'PCI Label Id's on NICs,
> but MS teem confirmed that flipping 'PCI Label Id' should not
> change 'Network Connection' ennumeration, so it should be safe
> for QEMU to change _DSM without any compat code.
>
> Smoke tested with WinXP and WS2022
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/774
> Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> ---
> hw/i386/acpi-build.c | 33 +++++++++++++++++++++++----------
> 1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 733b8f0851..1311a0d4f3 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -654,6 +654,7 @@ static Aml *aml_pci_pdsm(void)
> Aml *acpi_index = aml_local(2);
> Aml *zero = aml_int(0);
> Aml *one = aml_int(1);
> + Aml *not_supp = aml_int(0xFFFFFFFF);
> Aml *func = aml_arg(2);
> Aml *params = aml_arg(4);
> Aml *bnum = aml_derefof(aml_index(params, aml_int(0)));
> @@ -678,7 +679,7 @@ static Aml *aml_pci_pdsm(void)
> */
> ifctx1 = aml_if(aml_lnot(
> aml_or(aml_equal(acpi_index, zero),
> - aml_equal(acpi_index, aml_int(0xFFFFFFFF)), NULL)
> + aml_equal(acpi_index, not_supp), NULL)
> ));
> {
> /* have supported functions */
> @@ -704,18 +705,30 @@ static Aml *aml_pci_pdsm(void)
> {
> Aml *pkg = aml_package(2);
>
> - aml_append(pkg, zero);
> - /*
> - * optional, if not impl. should return null string
> - */
> - aml_append(pkg, aml_string("%s", ""));
> - aml_append(ifctx, aml_store(pkg, ret));
> -
> aml_append(ifctx, aml_store(aml_call2("AIDX", bnum, sunum),
> acpi_index));
> + aml_append(ifctx, aml_store(pkg, ret));
> /*
> - * update acpi-index to actual value
> + * Windows calls func=7 without checking if it's available,
> + * as workaround Microsoft has suggested to return invalid for func7
> + * Package, so return 2 elements package but only initialize elements
> + * when acpi_index is supported and leave them uninitialized, which
> + * leads elements to being Uninitialized ObjectType and should trip
> + * Windows into discarding result as an unexpected and prevent setting
> + * bogus 'PCI Label' on the device.
This comment is very confusing!
> */
> - aml_append(ifctx, aml_store(acpi_index, aml_index(ret, zero)));
> + ifctx1 = aml_if(aml_lnot(aml_lor(
> + aml_equal(acpi_index, zero), aml_equal(acpi_index,
> not_supp)
> + )));
So this conditional checks if the acpi index is supported (because its
aml_lnot()).
> + {
> + aml_append(ifctx1, aml_store(acpi_index, aml_index(ret, zero)));
> + /*
> + * optional, if not impl. should return null string
> + */
I know this comes from the existing code but I am still confused. Why
is this appending "return null string" logic to "if acpi index is
supprted" conditional?
> + aml_append(ifctx1, aml_store(aml_string("%s", ""),
> + aml_index(ret, one)));
> + }
> + aml_append(ifctx, ifctx1);
> +
> aml_append(ifctx, aml_return(ret));
> }
>
> --
> 2.43.0
>
