Re: [PATCH] mos6522: fix linking error when CONFIG_MOS6522 is not set

[Murilo Opsfelder Araújo]

Hi, Mark.

On 5/4/22 11:32, Mark Cave-Ayland wrote:
> On 04/05/2022 14:16, Murilo Opsfelder Araújo wrote:
> > Hi, Mark.
> >
> > On 5/4/22 04:10, Mark Cave-Ayland wrote:
> > > On 02/05/2022 14:36, Murilo Opsfelder Araújo wrote:
> > >
> > > > Hi, Mark.
> > > >
> > > > Thanks for reviewing.  Comments below.
> > > >
> > > > On 5/2/22 06:43, Mark Cave-Ayland wrote:
> > > > > On 30/04/2022 00:31, Murilo Opsfelder Araujo wrote:
> > > > >
> > > > > > When CONFIG_MOS6522 is not set, building ppc64-softmmu target fails:
> > > > > >
> > > > > >      /usr/bin/ld: libqemu-ppc64-softmmu.fa.p/monitor_misc.c.o:(.data+0x1158): 
> > > > > > undefined reference to `hmp_info_via'
> > > > > >      clang-13: error: linker command failed with exit code 1 (use -v to see 
> > > > > > invocation)
> > > > > >
> > > > > > Add CONFIG_MOS6522 check for hmp_info_via in hmp-commands-info.hx to fix
> > > > > > such linking error.
> > > > > >
> > > > > > Fixes: 409e9f7131e5 (mos6522: add "info via" HMP command for debugging)
> > > > > > Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
> > > > > > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > > > > Cc: Fabiano Rosas <farosas@linux.ibm.com>
> > > > > > ---
> > > > > >   hmp-commands-info.hx | 2 ++
> > > > > >   1 file changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > > > > > index adfa085a9b..9ad784dd9f 100644
> > > > > > --- a/hmp-commands-info.hx
> > > > > > +++ b/hmp-commands-info.hx
> > > > > > @@ -881,6 +881,7 @@ SRST
> > > > > >   ERST
> > > > > >   #if defined(TARGET_M68K) || defined(TARGET_PPC)
> > > > > > +#if defined(CONFIG_MOS6522)
> > > > > >       {
> > > > > >           .name         = "via",
> > > > > >           .args_type    = "",
> > > > > > @@ -889,6 +890,7 @@ ERST
> > > > > >           .cmd          = hmp_info_via,
> > > > > >       },
> > > > > >   #endif
> > > > > > +#endif
> > > > > >   SRST
> > > > > >     ``info via``
> > > > >
> > > > > Hmmm. The patch in its proposed form isn't correct, since device CONFIG_* 
> > > > > defines aren't declared when processing hmp-commands-info.hx. This was 
> > > > > something that was discovered and discussed in the original thread for which 
> > > > > the current workaround is to use the per-target TARGET_* defines instead.
> > > >
> > > > So my proposed fix worked just by coincidence.  Thanks for providing the 
> > > > background.
> > > >
> > > > > Given that the g3beige and mac99 machines are included by default in 
> > > > > qemu-system-ppc64 which both contain the MOS6522 device, I can't quite 
> > > > > understand how CONFIG_MOS6522 isn't being selected.
> > > > >
> > > > > Can you give more information about how you are building QEMU including your 
> > > > > configure command line?
> > > >
> > > > Here is a reproducer adapted from CentOS 9 Stream qemu-kvm[0] package
> > > > (build failed on c9s ppc64le with QEMU at commit 
> > > > f5643914a9e8f79c606a76e6a9d7ea82a3fc3e65):
> > > >
> > > > $ cat > configs/devices/rh-virtio.mak <<"EOF"
> > > > CONFIG_VIRTIO=y
> > > > CONFIG_VIRTIO_BALLOON=y
> > > > CONFIG_VIRTIO_BLK=y
> > > > CONFIG_VIRTIO_GPU=y
> > > > CONFIG_VIRTIO_INPUT=y
> > > > CONFIG_VIRTIO_INPUT_HOST=y
> > > > CONFIG_VIRTIO_NET=y
> > > > CONFIG_VIRTIO_RNG=y
> > > > CONFIG_VIRTIO_SCSI=y
> > > > CONFIG_VIRTIO_SERIAL=y
> > > > EOF
> > > >
> > > > $ cat > configs/devices/ppc64-softmmu/ppc64-rh-devices.mak <<"EOF"
> > > > include ../rh-virtio.mak
> > > > CONFIG_DIMM=y
> > > > CONFIG_MEM_DEVICE=y
> > > > CONFIG_NVDIMM=y
> > > > CONFIG_PCI=y
> > > > CONFIG_PCI_DEVICES=y
> > > > CONFIG_PCI_TESTDEV=y
> > > > CONFIG_PCI_EXPRESS=y
> > > > CONFIG_PSERIES=y
> > > > CONFIG_SCSI=y
> > > > CONFIG_SPAPR_VSCSI=y
> > > > CONFIG_TEST_DEVICES=y
> > > > CONFIG_USB=y
> > > > CONFIG_USB_OHCI=y
> > > > CONFIG_USB_OHCI_PCI=y
> > > > CONFIG_USB_SMARTCARD=y
> > > > CONFIG_USB_STORAGE_CORE=y
> > > > CONFIG_USB_STORAGE_CLASSIC=y
> > > > CONFIG_USB_XHCI=y
> > > > CONFIG_USB_XHCI_NEC=y
> > > > CONFIG_USB_XHCI_PCI=y
> > > > CONFIG_VFIO=y
> > > > CONFIG_VFIO_PCI=y
> > > > CONFIG_VGA=y
> > > > CONFIG_VGA_PCI=y
> > > > CONFIG_VHOST_USER=y
> > > > CONFIG_VIRTIO_PCI=y
> > > > CONFIG_VIRTIO_VGA=y
> > > > CONFIG_WDT_IB6300ESB=y
> > > > CONFIG_XICS=y
> > > > CONFIG_XIVE=y
> > > > CONFIG_TPM=y
> > > > CONFIG_TPM_SPAPR=y
> > > > CONFIG_TPM_EMULATOR=y
> > > > EOF
> > > >
> > > > $ mkdir build
> > > > $ cd build
> > > >
> > > > $ ../configure --cc=clang --cxx=/bin/false --prefix=/usr --libdir=/usr/lib64 --datadir=/usr/share --sysconfdir=/etc --interp-prefix=/usr/qemu-%M --localstatedir=/var --docdir=/usr/share/doc --libexecdir=/usr/libexec '--extra-ldflags=-Wl,-z,relro -Wl,--as-needed  -Wl,-z,now   ' '--extra-cflags=-O2 -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS --config /usr/lib/rpm/redhat/redhat-hardened-clang.cfg -fstack-protector-strong   -m64 -mcpu=power9 -mtune=power9 -fasynchronous-unwind-tables -fstack-clash-protection -Wno-string-plus-int' --with-pkgversion=qemu-kvm-7.0.0-1.el9 --with-suffix=qemu-kvm --firmwarepath=/usr/share/qemu-firmware:/usr/share/ipxe/qemu:/usr/share/seavgabios:/usr/share/seabios --meson=internal --enable-trace-backend=dtrace --with-coroutine=ucontext --with-git=git --tls-priority=@QEMU,SYSTEM --audio-drv-list= --disable-alsa --disable-attr --disable-auth-pam --disable-avx2 
> > > > --disable-avx512f --disable-block-drv-whitelist-in-tools --disable-bochs --disable-bpf --disable-brlapi --disable-bsd-user --disable-bzip2 --disable-cap-ng --disable-capstone --disable-cfi --disable-cfi-debug --disable-cloop --disable-cocoa --disable-coreaudio --disable-coroutine-pool --disable-crypto-afalg --disable-curl --disable-curses --disable-dbus-display --disable-debug-info --disable-debug-mutex --disable-debug-tcg --disable-dmg --disable-docs --disable-dsound --disable-fdt --disable-fuse --disable-fuse-lseek --disable-gcrypt --disable-gettext --disable-gio --disable-glusterfs --disable-gnutls --disable-gtk --disable-guest-agent --disable-guest-agent-msi --disable-hax --disable-hvf --disable-iconv --disable-jack --disable-kvm --disable-l2tpv3 --disable-libdaxctl --disable-libiscsi --disable-libnfs --disable-libpmem --disable-libssh --disable-libudev --disable-libusb --disable-linux-aio --disable-linux-io-uring --disable-linux-user --disable-live-block-migration 
> > > > --disable-lto --disable-lzfse --disable-lzo --disable-malloc-trim --disable-membarrier --disable-modules --disable-module-upgrades --disable-mpath --disable-multiprocess --disable-netmap --disable-nettle --disable-numa --disable-nvmm --disable-opengl --disable-oss --disable-pa --disable-parallels --disable-pie --disable-pvrdma --disable-qcow1 --disable-qed --disable-qga-vss --disable-qom-cast-debug --disable-rbd --disable-rdma --disable-replication --disable-rng-none --disable-safe-stack --disable-sanitizers --disable-sdl --disable-sdl-image --disable-seccomp --disable-selinux --disable-slirp --disable-slirp-smbd --disable-smartcard --disable-snappy --disable-sparse --disable-spice --disable-spice-protocol --disable-strip --disable-system --disable-tcg --disable-tools --disable-tpm --disable-u2f --disable-usb-redir --disable-user --disable-vde --disable-vdi --disable-vhost-crypto --disable-vhost-kernel --disable-vhost-net --disable-vhost-scsi --disable-vhost-user 
> > > > --disable-vhost-user-blk-server --disable-vhost-vdpa --disable-vhost-vsock --disable-virglrenderer --disable-virtfs --disable-virtiofsd --disable-vnc --disable-vnc-jpeg --disable-vnc-sasl --disable-vte --disable-vvfat --disable-werror --disable-whpx --disable-xen --disable-xen-pci-passthrough --disable-xkbcommon --disable-zstd --with-git-submodules=ignore --without-default-devices --with-devices-ppc64=ppc64-rh-devices --target-list=ppc64-softmmu --block-drv-rw-whitelist=qcow2,raw,file,host_device,nbd,iscsi,rbd,blkdebug,luks,null-co,nvme,copy-on-read,throttle,compress --block-drv-ro-whitelist=vdi,vmdk,vhdx,vpc,https --enable-attr --enable-cap-ng --enable-capstone=internal --enable-coroutine-pool --enable-curl --enable-debug-info --enable-docs --enable-fdt=system --enable-gnutls --enable-guest-agent --enable-iconv --enable-kvm --enable-libusb --enable-libudev --enable-linux-aio --enable-lzo --enable-malloc-trim --enable-modules --enable-mpath --enable-numa --enable-pa 
> > > > --enable-pie --enable-rbd --enable-rdma --enable-seccomp --enable-selinux --enable-slirp=system --enable-snappy --enable-spice-protocol --enable-system --enable-tcg --enable-tools --enable-tpm --enable-vdi --enable-virtiofsd --enable-vhost-kernel --enable-vhost-net --enable-vhost-user --enable-vhost-user-blk-server --enable-vhost-vdpa --enable-vhost-vsock --enable-vnc --enable-vnc-sasl --enable-werror --enable-xkbcommon
> > > >
> > > > $ make -O -j$(nproc) V=1 VERBOSE=1
> > > > ...
> > > > /usr/bin/ld: libqemu-ppc64-softmmu.fa.p/monitor_misc.c.o:(.data+0x1158): 
> > > > undefined reference to `hmp_info_via'
> > > > clang-13: error: linker command failed with exit code 1 (use -v to see 
> > > > invocation)
> > > >
> > > > I have figured that it also fails with this minimal set of configure options
> > > > (in addition to the devices CONFIG_* options above):
> > > >
> > > > $ ../configure --without-default-devices --with-devices-ppc64=ppc64-rh-devices 
> > > > --target-list=ppc64-softmmu
> > > > $ make -j
> > > > ...
> > > > /usr/bin/ld: libqemu-ppc64-softmmu.fa.p/monitor_misc.c.o:(.data.rel+0x3228): 
> > > > undefined reference to `hmp_info_via'
> > > > collect2: error: ld returned 1 exit status
> > > >
> > > > Since TARGET_PPC is defined when building target ppc64-softmmu, the 
> > > > hmp_info_via will be referenced when processing the hmp-commands-info.hx.
> > > > However, hmp_info_via implementation resides on hw/misc/mos6522.c, which is 
> > > > built only if CONFIG_MOS6522 is defined, as per hw/misc/meson.build.
> > > >
> > > > If hmp_info_via is generic enough and not device-specific, it could be moved 
> > > > out of mos6522.c to somewhere else.
> > > >
> > > > What do you think?
> > > >
> > > > [0] 
> > > > https://gitlab.com/redhat/centos-stream/rpms/qemu-kvm/-/blob/c9s/qemu-kvm.spec#L686
> > >
> > > It's probably easier if I post a link to the original thread at 
> > > https://lore.kernel.org/all/20220127205405.23499-9-mark.cave-ayland@ilande.co.uk/
> > >  for reference but the main takeaway is:
> > >
> > > - Device CONFIG_* defines aren't present when building hmp-commands-info.hx
> > >
> > > - The TARGET_* defines are poisoned in qapi/qapi-commands-misc-target.h
> > >
> > > - The long-term solution is to implement a DeviceClass function callback that 
> > > allows
> > >    "info debug <foo>" QMP-wrapped monitor commands to be registered dynamically 
> > > by
> > >    devices. But that needs someone with the time and ability to implement it.
> > >
> > > The compromise was to accept the command wrapped by TARGET_ defines where it is 
> > > used, but of course that won't work in this case where you're generating a 
> > > custom configuration as above.
> > >
> > > Certainly QEMU could do better here, but then if you are already patching the 
> > > build to generate a custom configuration as above, you might as well just patch 
> > > out the relevant part of hmp-commands-info.hx at the same time until proper 
> > > per-device HMP/QMP support is added.
> >
> > We are not patching the build.  We are just configuring it.
>
> That's not true though: the spec file linked above contains 20 patches to the 
> vanilla QEMU source, including feeding custom device lists into the build 
> system via 
> https://gitlab.com/redhat/centos-stream/rpms/qemu-kvm/-/blob/c9s/0005-Enable-disable-devices-for-RHEL.patch.

I'm sorry.  I think I wasn't clear enough.

The reproducer I sent in the email was *adapted* from CentOS/RHEL qemu-kvm.spec.
Meaning that we configured the devices in the same way that the CentOS/RHEL 
package is configuring but used the unmodified QEMU source tree from upstream.

I did that because I wanted to mimic its configuration (devices and configure 
options) against the upstream code to determine if the failure was a downstream 
or upstream issue.
It turns out it's an upstream regression.

> Perhaps CONFIG_MOS6522 is missing from ppc64-rh-devices?

I don't think so.  Since the CONFIG_MOS6522 is available, one can build without 
it and code should cope with that.

> If you need to generate a build within a short timeframe then patching out the 
> part of the build that fails for you is going to be the quickest solution.
>
> > QEMU at tag v6.2.0 works with the exact same configuration.
> > QEMU 7.0.0 does not.
> > This is a regression in QEMU source code.
>
> I've just tried a plain "./configure --target-list=ppc64-softmmu" build here on 
> my x86_64 host using git master and everything builds fine, and of course it passes 
> gitlab-CI since that is a pre-requisite for merging. The only thing I can think is that 
> your RHEL ppc64 build is being patched to remove the Mac machines which is why you see 
> the failure, in which case you should also update the patch to remove the part 
> referencing hmp_info_via.

You need to configure the devices in order to reproduce the build issue.

Let me paste again the commands here in case you want to give them a try:

cat > configs/devices/rh-virtio.mak <<"EOF"
CONFIG_VIRTIO=y
CONFIG_VIRTIO_BALLOON=y
CONFIG_VIRTIO_BLK=y
CONFIG_VIRTIO_GPU=y
CONFIG_VIRTIO_INPUT=y
CONFIG_VIRTIO_INPUT_HOST=y
CONFIG_VIRTIO_NET=y
CONFIG_VIRTIO_RNG=y
CONFIG_VIRTIO_SCSI=y
CONFIG_VIRTIO_SERIAL=y
EOF

cat > configs/devices/ppc64-softmmu/ppc64-rh-devices.mak <<"EOF"
include ../rh-virtio.mak
CONFIG_DIMM=y
CONFIG_MEM_DEVICE=y
CONFIG_NVDIMM=y
CONFIG_PCI=y
CONFIG_PCI_DEVICES=y
CONFIG_PCI_TESTDEV=y
CONFIG_PCI_EXPRESS=y
CONFIG_PSERIES=y
CONFIG_SCSI=y
CONFIG_SPAPR_VSCSI=y
CONFIG_TEST_DEVICES=y
CONFIG_USB=y
CONFIG_USB_OHCI=y
CONFIG_USB_OHCI_PCI=y
CONFIG_USB_SMARTCARD=y
CONFIG_USB_STORAGE_CORE=y
CONFIG_USB_STORAGE_CLASSIC=y
CONFIG_USB_XHCI=y
CONFIG_USB_XHCI_NEC=y
CONFIG_USB_XHCI_PCI=y
CONFIG_VFIO=y
CONFIG_VFIO_PCI=y
CONFIG_VGA=y
CONFIG_VGA_PCI=y
CONFIG_VHOST_USER=y
CONFIG_VIRTIO_PCI=y
CONFIG_VIRTIO_VGA=y
CONFIG_WDT_IB6300ESB=y
CONFIG_XICS=y
CONFIG_XIVE=y
CONFIG_TPM=y
CONFIG_TPM_SPAPR=y
CONFIG_TPM_EMULATOR=y
EOF

mkdir build && cd build

../configure --without-default-devices --with-devices-ppc64=ppc64-rh-devices 
--target-list=ppc64-softmmu
make -j

> > Is my ideia of moving the hmp_info_via implementation out of mos6522.c an 
> > acceptable way to have this fixed in the short-term?
>
> The current solution was agreed after discussions with David and Daniel (the 
> HMP and QMP maintainers) so they are the people who can advise you as to the 
> best approach here.

David, Daniel - any recommendation to fix this regression?

> Unfortunately QEMU changes can involve testing an N x M matrix which gets even 
> bigger when different accelerators are involved, so occassionally issues like 
> this can still occur.

Indeed.  We can't predict what downstream or users will specify for device and 
configure options.

> If you feel strongly that upstream should support this particular RHEL 
> configuration then please consider adding it to gitlab-CI so that build issues 
> like this can be detected before merge.

That's certainly a good idea.

--
Murilo