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

[Igor Mammedov]

On Sun, 28 Feb 2021 15:43:40 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Sat, Feb 27, 2021 at 08:41:11PM +0100, Thomas Lamprecht wrote:
> > 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?  
> 
> Sure. The way to do that is to tie old behaviour to old machine
> versions. We'll need it in stable too ...
> 
> Igor want to cook up a patch?

It might be too late for that,
I mean VMs installed on qemu-5.2 will use new ACPI tables on all
machine types and reverting behavior back for old machine types
will cause the same headache for them.

The difference is that probably there are a lot less new
Windows installations than the old ones (especially with static IP assignment),
so it could be better to restore bug for old machine types to
avoid guest reconfiguration.

How about:
 * buggy ACPI for 5.1 machine types and older
 * fixed ACPI for 5.2 and newer?


> > 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
> > 
> > 
> > = 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,media=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,rotation_rate=1,bootindex=100'
> >  \
> >   -netdev 
> > 'type=tap,id=net0,ifname=tap11765i0,script=/var/lib/qemu-server/pve-bridge,downscript=/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  
> 
>