Re: [PATCH 1/2] i386/acpi: fix inconsistent QEMU/OVMF device paths

[zhangying (AZ)]

> On 30.07.20 17:58, Michael S. Tsirkin wrote:
> > 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 ACPI 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. This is due to the _UID assigned to it by build_dsdt in
> > hw/i386/acpi-build.c Which does not correspond to the primary PCI
> > identifier given by pcibus_num in hw/pci/pci.c
> >
> > Reference with the device paths, OVMF startup logs, and ACPI table
> > dumps (SysReport):
> > https://github.com/acidanthera/bugtracker/issues/1050
> >
> > 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.
> >
> > Suggested-by: Laszlo Ersek <lersek@redhat.com>
> > Tested-by: vit9696 <vit9696@protonmail.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/i386/acpi-build.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index
> > b7bcbbbb2a..7a5a8b3521 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1497,7 +1497,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >          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);
> >
> > @@ -1512,7 +1512,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >          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);
> >
> 
> This "breaks" Windows guests created/installed before this change in the sense
> of Windows gets confused and declares that most of the devices changed and
> thus it has new entries for them in the device manager where settings of the 
> old
> one do not apply anymore.
> 
> We were made aware of this by our users when making QEMU 5.2.0 available on
> a more used repository of us. Users complained that their static network
> configuration got thrown out in Windows 2016 or 2019 server VMs, and Windows
> tried to use DHCP (which was not available in their environments) and thus 
> their
> Windows VMs had no network connectivity at all anymore.
> 
> It's currently not yet quite 100% clear to me with what QEMU version the
> Windows VM must be installed with, from reading the patch I have to believe it
> must be before that, but we got mixed reports and a colleague could not
> replicate it from upgrade of 4.0 to 5.2 (I did /not/ confirm that one). 
> Anyway, just
> writing this all to avoid people seeing different results and brushing this 
> off.
> 
> So here's my personal reproducer, as said, I think that one should be able to 
> just
> use QEMU 5.1 to install a Windows guest and start it with 5.2 afterwards to 
> see
> this issue, but YMMV.
> 
> Note. I always used the exact same QEMU command (see below) for installation,
> reproducing and bisect.
> 
> 1. Installed Windows 2016 1616 VM using QEMU 3.0.1
>    - VirtIO net/scsi driver from VirtIO win 190 2. Setup static network in 
> the VM
> and shutdown 3. Started VM with 5.2.0 -> Network gone, new "Ethernet #2"
> adapter shows up instead
> 
> Starting the  "Device Manager" and enabling "View -> Show hidden devices"
> showed me a greyed out device duplicate for basically anything attached, SCSI
> disk, Basic Display Adapter, CDROM device, ..., and the Network device.
> 
> The first difference I could find was the "Device instance path" one can find 
> in the
> "Details" tab of the devices' "Properties" window.
> 
> # old, from initial installation on QEMU 3.0.1
> PCI\VEN_1AF4&DEV_1000&SUBSYS_00011AF4&REV_00\3&13C0B0C5&0&90
> 
> # new, from boot with QEMU 5.2
> PCI\VEN_1AF4&DEV_1000&SUBSYS_00011AF4&REV_00\3&267A616A&0&90
> 
> They match until almost the end, not sure how important that is, but it 
> caught my
> eye (I'm really no windows guy since a decade so please excuse my terrible
> debugging/exploring skills there. The rest of those properties looked pretty
> much identical.
> 
> I then started a bisect, always just restarting the guest with the new QEMU 
> build
> and checking "Device Manager" and network settings to see if good/bad. That
> worked pretty well and I came to this commit. See the bisect log attached at 
> the
> end of this mail.
> 
> So, from reading the commit message I figure that this change is wanted, what
> are the implications of just reverting it? (which works out in bringing back 
> the old
> state in Windows + working static network config again).
> 
> Or any other way/idea to address this in a sane way so that those picky 
> Windows
> guests can be handled more graciously?
> 
> I guess also that there could be more subtle effects from this patch here, the
> network one may have just had quite visible effects to pop up as first 
> issue...
> 
> Thanks if you read so far!
> 
> cheers,
> Thomas
> 

We have a similar problem and want to solve it further.

Description of problem:

When QEMU is upgraded from 4.1 to 6.2, if the machine type is not fixed as 4.1 
and NIC was configured with static IP address, Windows will make original 
'network connection' inactive and create a new one (which is not configured as 
desired). As result guest looses network connectivity.

Test 1: 
Steps to test the guest loses the network connection:
1. on QEMU 4.1 install Windows Server 2019 guest with virtio NIC
2. configure NIC with static IP and shutdown guest
3. start guest on qemu-6.2 with machine version as qemu 6.2

Test 2: 
Steps to test the guest does not lose the network connection:
1. on QEMU 4.1 install Windows Server 2019 guest with virtio NIC
2. configure NIC with static IP and shutdown guest
3. start guest on qemu-6.2 with machine version as qemu 4.1

Here's a historical analysis of the problem:

Commit af1b80ae56c9 ("i386/acpi: fix inconsistent QEMU/OVMF device paths") 
fixed UID of PCI root bridge in ACPI tables for all pc/q35 machine 
versions.however it was discovered that this change updates Windows 
configuration in an incompatible way causing network configuration failure 
unless DHCP is used.

And Commit 0a343a5add75 ("i386/acpi: restore device paths for pre-5.1 vms") 
reverts the _UID update from 1 to 0 for q35 and i440fx VMs before version 5.2 
to maintain the original behaviour when upgrading. This requires that the same 
machine type be used after the QEMU upgrade to 5.2 or later.

We want all VMs to be able to use the features of the new qemu 6.2 version 
After upgrade, we'd like to ask some questions:

1. When the QEMU is upgraded from 4.1 to 6.2, is there any method to ensure 
that the guest does not lose the network connection for Test 1? could anyone 
give some suggestions?
2. If no other method is available, reverts the _UID update from 1 to 0 for all 
QEMU versions(the prerequisite is that macOS is not used). Is there any risk? 
Is it recommended to do this?
3. When the QEMU is upgraded from 4.1 to 6.2, set _UID to 1 for versions 
earlier than QEMU 5.1 and to 0 for versions later than QEMU 5.2 Through 
Parameters, but start guest on qemu-6.2 with machine version as qemu 6.2 to 
solve Test 1's problem. Is this solution feasible? Is there any risk?

Thanks.

> = QEMU Command =
> 
> (This was generated by our (Proxmox VE) stack, I only cleaned it up a bit to 
> allow
> easier manual running it)
> 
> ./qemu-system-x86_64 \
>   -name win2016 \
>   -chardev
> 'socket,id=qmp,path=/var/run/qemu-server/11765.qmp,server,nowait' \
>   -mon 'chardev=qmp,mode=control' \
>   -smbios 'type=1,uuid=6324fb28-e98a-44cf-85db-694d1b3405f5' \
>   -smp '2,sockets=1,cores=2,maxcpus=2' \
>   -nodefaults \
>   -boot 'menu=on,strict=on,reboot-timeout=1000' \
>   -vnc unix:/var/run/qemu-server/11765.vnc,password \
>   -no-hpet \
>   -cpu
> 'host,hv_ipi,hv_relaxed,hv_reset,hv_runtime,hv_spinlocks=0x1fff,hv_stimer,hv
> _synic,hv_time,hv_vapic,hv_vpindex,+kvm_pv_eoi,+kvm_pv_unhalt,+md-clear,
> +pcid,+spec-ctrl' \
>   -m 2048 \
>   -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
>   -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f' \
>   -device 'vmgenid,guid=2e56e6ca-2cf8-4f1d-8cc3-9b19a2510c01' \
>   -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
>   -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
>   -device 'VGA,id=vga,bus=pci.0,addr=0x2,edid=off' \
>   -chardev
> 'socket,path=/var/run/qemu-server/11765.qga,server,nowait,id=qga0' \
>   -device 'virtio-serial,id=qga0,bus=pci.0,addr=0x8' \
>   -device 'virtserialport,chardev=qga0,name=org.qemu.guest_agent.0' \
>   -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3' \
>   -iscsi 'initiator-name=iqn.1993-08.org.debian:01:468faae9322b' \
>   -drive
> 'file=/mnt/pve/iso/template/iso/virtio-win-0.1.190.iso,if=none,id=drive-ide0,me
> dia=cdrom,aio=threads' \
>   -device 'ide-cd,bus=ide.0,unit=0,drive=drive-ide0,id=ide0,bootindex=200' \
>   -drive
> 'file=/mnt/pve/iso/template/iso/Win2016-1616-evaluation.ISO,if=none,id=drive-
> ide2,media=cdrom,aio=threads' \
>   -device 'ide-cd,bus=ide.1,unit=0,drive=drive-ide2,id=ide2,bootindex=201' \
>   -device 'virtio-scsi-pci,id=scsihw0,bus=pci.0,addr=0x5' \
>   -drive
> 'file=/dev/WDnvme/vm-11765-disk-0,if=none,id=drive-scsi0,format=raw,cache=
> none,aio=native,detect-zeroes=on' \
>   -device
> 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,rotati
> on_rate=1,bootindex=100' \
>   -netdev
> 'type=tap,id=net0,ifname=tap11765i0,script=/var/lib/qemu-server/pve-bridge,d
> ownscript=/var/lib/qemu-server/pve-bridgedown,vhost=on' \
>   -device
> 'virtio-net-pci,mac=02:98:90:43:42:1D,netdev=net0,bus=pci.0,addr=0x12,id=net0,
> bootindex=300' \
>   -rtc 'driftfix=slew,base=localtime' \
>   -machine 'type=pc' \
>   -global 'kvm-pit.lost_tick_policy=discard'
> 
> 
> = bisect log =
> 
> git bisect start
> # bad: [553032db17440f8de011390e5a1cfddd13751b0b] Update version for v5.2.0
> release git bisect bad 553032db17440f8de011390e5a1cfddd13751b0b
> # good: [d0ed6a69d399ae193959225cdeaa9382746c91cc] Update version for
> v5.1.0 release git bisect good d0ed6a69d399ae193959225cdeaa9382746c91cc
> # bad: [ed799805d00ccdda45eb8441c7d929624d9e98a6] qom: Add kernel-doc
> markup to introduction doc comment git bisect bad
> ed799805d00ccdda45eb8441c7d929624d9e98a6
> # bad: [e4d8b7c1a95fffcfa4bdab9aa7ffd1cf590cdcf5] Merge remote-tracking
> branch 'remotes/nvme/tags/pull-nvme-20200902' into staging git bisect bad
> e4d8b7c1a95fffcfa4bdab9aa7ffd1cf590cdcf5
> # bad: [af1dfe1ec0864e6700237a43cc36018176f9eba9] acpi: update expected
> DSDT files with _UID changes git bisect bad
> af1dfe1ec0864e6700237a43cc36018176f9eba9
> # good: [d7df0ceee0fd2e512cd214a9074ebeeb40da3099] Merge remote-tracking
> branch 'remotes/philmd-gitlab/tags/sd-next-20200821' into staging git bisect
> good d7df0ceee0fd2e512cd214a9074ebeeb40da3099
> # good: [df82aa7fe10e46b675678977999d49bd586538f8] Merge remote-tracking
> branch 'remotes/edgar/tags/edgar/xilinx-next-2020-08-24.for-upstream' into
> staging git bisect good df82aa7fe10e46b675678977999d49bd586538f8
> # good: [e39a8320b088dd5efc9ebaafe387e52b3d962665] target/riscv: Support
> the Virtual Instruction fault git bisect good
> e39a8320b088dd5efc9ebaafe387e52b3d962665
> # good: [a6841a2de66fa44fe52ed996b70f9fb9f7bd6ca7] qcow2: Add subcluster
> support to qcow2_co_pwrite_zeroes() git bisect good
> a6841a2de66fa44fe52ed996b70f9fb9f7bd6ca7
> # good: [2f8cd515477edab1cbf38ecbdbfa2cac13ce1550] hw/display/artist: Fix
> invalidation of lines near screen border git bisect good
> 2f8cd515477edab1cbf38ecbdbfa2cac13ce1550
> # good: [a5d3cfa2dc775e5d99f013703b8508f1d989d588] iotests: Add tests for
> qcow2 images with extended L2 entries git bisect good
> a5d3cfa2dc775e5d99f013703b8508f1d989d588
> # good: [8e49197ca5e76fdb8928833b2649ef13fc5aab2f] Merge remote-tracking
> branch 'remotes/hdeller/tags/target-hppa-v3-pull-request' into staging git 
> bisect
> good 8e49197ca5e76fdb8928833b2649ef13fc5aab2f
> # bad: [af1b80ae56c9495999e8ccf7b70ef894378de642] i386/acpi: fix inconsistent
> QEMU/OVMF device paths git bisect bad
> af1b80ae56c9495999e8ccf7b70ef894378de642
> # good: [42a62c20925e02aef0d849f92a0e9540888e79ae] acpi: allow DSDT changes
> git bisect good 42a62c20925e02aef0d849f92a0e9540888e79ae
> # first bad commit: [af1b80ae56c9495999e8ccf7b70ef894378de642] i386/acpi: fix
> inconsistent QEMU/OVMF device paths