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

[Igor Mammedov]

On Wed, 19 Apr 2023 02:48:55 +0000
"zhangying (AZ)" <zhangying134@huawei.com> wrote:

> > On Tue, 18 Apr 2023 09:06:30 +0000
> > "zhangying (AZ)" via <qemu-devel@nongnu.org> 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?
> > > >
> > > > 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.
> > 
> > that's expected,
> > by upgrading machine type you basically change hardware.
> >   
> > >
> > > 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?
> > 
> > Upgrading qemu itself is fine as long as you keep using 4.1 machine type.
> > 
> > If you wish to upgrade machine type,
> > you will have to reconfigure manually configured 'Network connection's 
> > (nothing
> > else would help you)
> > 
> > From admin point of view it would be better to use DHPC (and if you need 
> > fixed
> > IPs, pin them on DHCP server side to VMs MAC addresses)
> >   
> > > 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?
> > 
> > that's what we already do by flipping UID value based on machine type 
> > version.
> > Risk is that by staying on old machine type you might not get some fixes or
> > changed defaults/behavior. You can analyze changes by looking at version-ed
> > FOO_machine_options() and hw_compat_x.y pc_compat_x.y property list and
> > decide if old machine type is good enough for you.  
> 
> 
> Thanks for the answer, but there are still some questions：
> 1. Because our services do not involve macOS, we want to roll back the _UID 
> of all qemu versions from 1 to 0. However, we do not know whether there are 
> potential risks. Do you suggest this?

you'll have to maintain downstream patch on your own

> 2. Flipping UID value based on machine type version, This will cause multiple 
> VMs of different machine types to run on the same host. We want to run VMs of 
> only one machine type on one host. If the machine type remains unchanged, and 
> a new parameter is added to notify the VM, If the VM is upgraded from 4.1, 
> the UID is set to 1. If the VM is newly created, the UID is set to 0. 
> However, this will cause different operations for the same machine type 
> version. Is this appropriate? Will there be some potential problems?

frankly speaking you lost me between 'if-s'

even after upgrading qemu to the latest you can still use old machine type 
(pc-q35-4.1) for exiting and new VMs to have the same behavior as qemu-4.1

> 3. QEMU is upgraded from 4.1 to 6.2. Is there any other problem about the 
> _UID except that the IP address of Windows is lost?

as for risks, no one would be able to tell you.
Common sense tells me "Check Changelogs to decide if you need a new version and 
do some testing before rolling out changes."

> 
> 
> > > 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_s
> > > > timer,hv
> > > > _synic,hv_time,hv_vapic,hv_vpindex,+kvm_pv_eoi,+kvm_pv_unhalt,+md-cl
> > > > ear,
> > > > +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=dr
> > > > ive-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,i
> > > > d=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=0x1
> > > > 2,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  
> > >  
> >   
>