diff mbox series

mos6522: fix linking error when CONFIG_MOS6522 is not set

Message ID 20220429233146.29662-1-muriloo@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series mos6522: fix linking error when CONFIG_MOS6522 is not set | expand

Commit Message

Murilo Opsfelder Araújo April 29, 2022, 11:31 p.m. UTC 
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(+)

Comments

Mark Cave-Ayland May 2, 2022, 9:43 a.m. UTC | #1 
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.

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?


ATB,

Mark.
Murilo Opsfelder Araújo May 2, 2022, 1:36 p.m. UTC | #2 
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
Fabiano Rosas May 3, 2022, 2:06 p.m. UTC | #3 
Murilo Opsfelder Araújo <muriloo@linux.ibm.com> writes:

> $ 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
>

<snip>

> $ ../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.

I think this particular problem you hit is due to the 64 bit devices
file including 32 bit as well:

$ cat configs/devices/ppc64-softmmu/default.mak 
# Default configuration for ppc64-softmmu

# Include all 32-bit boards
include ../ppc-softmmu/default.mak <----- here

# For PowerNV
CONFIG_POWERNV=y

# For pSeries
CONFIG_PSERIES=y
---

So ppc64-softmmu doesn't quite do what it says on the tin. I think in
the long run we need to revisit the conversation about whether to keep
the 32 & 64 bit builds separate. It is misleading that you're explicitly
excluding ppc-softmmu from the `--target-list`, but a some 32 bit stuff
still comes along implicitly.

> If hmp_info_via is generic enough and not device-specific, it could be moved out of mos6522.c to somewhere else.

Perhaps it would be easier to just have a stub for the 64 bits build? Do
HMP commands get in any way enabled/disabled depending on the emulated
hw that is available? It might be ok to have a hmp_info_via that is a
noop in 64bit.
Mark Cave-Ayland May 4, 2022, 7:10 a.m. UTC | #4 
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.


ATB,

Mark.
Mark Cave-Ayland May 4, 2022, 7:16 a.m. UTC | #5 
On 03/05/2022 15:06, Fabiano Rosas wrote:

> Murilo Opsfelder Araújo <muriloo@linux.ibm.com> writes:
> 
>> $ 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
>>
> 
> <snip>
> 
>> $ ../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.
> 
> I think this particular problem you hit is due to the 64 bit devices
> file including 32 bit as well:
> 
> $ cat configs/devices/ppc64-softmmu/default.mak
> # Default configuration for ppc64-softmmu
> 
> # Include all 32-bit boards
> include ../ppc-softmmu/default.mak <----- here
> 
> # For PowerNV
> CONFIG_POWERNV=y
> 
> # For pSeries
> CONFIG_PSERIES=y
> ---
> 
> So ppc64-softmmu doesn't quite do what it says on the tin. I think in
> the long run we need to revisit the conversation about whether to keep
> the 32 & 64 bit builds separate. It is misleading that you're explicitly
> excluding ppc-softmmu from the `--target-list`, but a some 32 bit stuff
> still comes along implicitly.

Unfortunately for historical reasons it isn't quite that simple: the mac99 machine in 
hw/ppc/mac_newworld.c is both a 32-bit and a 64-bit machine, but with a different PCI 
host bridge and a 970 CPU if run from qemu-system-ppc64. Unfortunately it pre-dates 
my time working on QEMU's PPC Mac machines but I believe it was (is?) capable of 
booting Linux, even though I doubt it accurately represents a real machine.

>> If hmp_info_via is generic enough and not device-specific, it could be moved out of mos6522.c to somewhere else.
> 
> Perhaps it would be easier to just have a stub for the 64 bits build? Do
> HMP commands get in any way enabled/disabled depending on the emulated
> hw that is available? It might be ok to have a hmp_info_via that is a
> noop in 64bit.

I've provided a pointer back to the original thread in my previous reply which I hope 
will give some background here.


ATB,

Mark.
Murilo Opsfelder Araújo May 4, 2022, 1:16 p.m. UTC | #6 
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.

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.

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?

Thank you!
Fabiano Rosas May 4, 2022, 2:26 p.m. UTC | #7 
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:

> On 03/05/2022 15:06, Fabiano Rosas wrote:
>
>> Murilo Opsfelder Araújo <muriloo@linux.ibm.com> writes:
>> 
>>> $ 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
>>>
>> 
>> <snip>
>> 
>>> $ ../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.
>> 
>> I think this particular problem you hit is due to the 64 bit devices
>> file including 32 bit as well:
>> 
>> $ cat configs/devices/ppc64-softmmu/default.mak
>> # Default configuration for ppc64-softmmu
>> 
>> # Include all 32-bit boards
>> include ../ppc-softmmu/default.mak <----- here
>> 
>> # For PowerNV
>> CONFIG_POWERNV=y
>> 
>> # For pSeries
>> CONFIG_PSERIES=y
>> ---
>> 
>> So ppc64-softmmu doesn't quite do what it says on the tin. I think in
>> the long run we need to revisit the conversation about whether to keep
>> the 32 & 64 bit builds separate. It is misleading that you're explicitly
>> excluding ppc-softmmu from the `--target-list`, but a some 32 bit stuff
>> still comes along implicitly.
>
> Unfortunately for historical reasons it isn't quite that simple: the mac99 machine in 
> hw/ppc/mac_newworld.c is both a 32-bit and a 64-bit machine, but with a different PCI 
> host bridge and a 970 CPU if run from qemu-system-ppc64. Unfortunately it pre-dates 
> my time working on QEMU's PPC Mac machines but I believe it was (is?) capable of 
> booting Linux, even though I doubt it accurately represents a real machine.

Well, what you describe is fine in my view, the 64-bit machine uses
qemu-system-ppc64. If there is shared code with 32-bit, that is ok.

What I would like to understand is what is the community's direction
with this, do we want:

1) the 64-bit build to include all of the code and have it run all
   machines, or;

2) the 64-bit build to run only 64-bit machines and the 32-bit build to
   run only 32-bit machines.

There are things such as 'target_ulong' that go against #1, and this
thread shows that we're not doing #2 as well.

I know there have been discussions about this in the past but I couldn't
find them in the archives.
Mark Cave-Ayland May 4, 2022, 2:32 p.m. UTC | #8 
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. 
Perhaps CONFIG_MOS6522 is missing from ppc64-rh-devices?

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.

> 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. 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.

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.


ATB,

Mark.
Mark Cave-Ayland May 4, 2022, 2:48 p.m. UTC | #9 
On 04/05/2022 15:26, Fabiano Rosas wrote:

> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:
> 
>> On 03/05/2022 15:06, Fabiano Rosas wrote:
>>
>>> Murilo Opsfelder Araújo <muriloo@linux.ibm.com> writes:
>>>
>>>> $ 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
>>>>
>>>
>>> <snip>
>>>
>>>> $ ../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.
>>>
>>> I think this particular problem you hit is due to the 64 bit devices
>>> file including 32 bit as well:
>>>
>>> $ cat configs/devices/ppc64-softmmu/default.mak
>>> # Default configuration for ppc64-softmmu
>>>
>>> # Include all 32-bit boards
>>> include ../ppc-softmmu/default.mak <----- here
>>>
>>> # For PowerNV
>>> CONFIG_POWERNV=y
>>>
>>> # For pSeries
>>> CONFIG_PSERIES=y
>>> ---
>>>
>>> So ppc64-softmmu doesn't quite do what it says on the tin. I think in
>>> the long run we need to revisit the conversation about whether to keep
>>> the 32 & 64 bit builds separate. It is misleading that you're explicitly
>>> excluding ppc-softmmu from the `--target-list`, but a some 32 bit stuff
>>> still comes along implicitly.
>>
>> Unfortunately for historical reasons it isn't quite that simple: the mac99 machine in
>> hw/ppc/mac_newworld.c is both a 32-bit and a 64-bit machine, but with a different PCI
>> host bridge and a 970 CPU if run from qemu-system-ppc64. Unfortunately it pre-dates
>> my time working on QEMU's PPC Mac machines but I believe it was (is?) capable of
>> booting Linux, even though I doubt it accurately represents a real machine.
> 
> Well, what you describe is fine in my view, the 64-bit machine uses
> qemu-system-ppc64. If there is shared code with 32-bit, that is ok.
> 
> What I would like to understand is what is the community's direction
> with this, do we want:
> 
> 1) the 64-bit build to include all of the code and have it run all
>     machines, or;
> 
> 2) the 64-bit build to run only 64-bit machines and the 32-bit build to
>     run only 32-bit machines.
> 
> There are things such as 'target_ulong' that go against #1, and this
> thread shows that we're not doing #2 as well.
> 
> I know there have been discussions about this in the past but I couldn't
> find them in the archives.

Certainly if a 64-bit Mac machine were to be added today, I'd be inclined to copy 
mac_newworld.c into a separate file and give it a separate machine name for ppc64 to 
make a clear distinction between the two machines (and allow them to evolve 
separately). Unfortunately I have no idea as to what the reference machine for the 
PPC64 Mac machine was supposed to be which makes it harder to decide what to do :(

In my mind it feels like qemu-system-ppc is for 32-bit guests and qemu-system-ppc64 
is for 64-bit guests which I believe is consistent with how it currently works with 
MIPS and ARM (someone feel free to correct me here).


ATB,

Mark.
Murilo Opsfelder Araújo May 5, 2022, 1:24 a.m. UTC | #10 
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.
Mark Cave-Ayland May 5, 2022, 8:19 a.m. UTC | #11 
On 05/05/2022 02:24, Murilo Opsfelder Araújo wrote:

> 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.

Ah so that's the problem then - you can't guarantee the configuration from a 
vendor-customised build will work with upstream, particularly when the build system 
itself has been patched. More explanation below.

>> 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.

In an upstream build the default boards for each target are listed in the configs/ 
directory and Kconfig specifies the dependencies such that for ppc and ppc64 
CONFIG_MOS6522 is **always** defined. However what happens in the .spec file you 
linked to is that the device lists **are being overridden** by the provided 
ppc64-rh-devices file which **doesn't** contain CONFIG_MOS6522. It seems to me that 
the .spec file can only work with that vendor-specific ppc64-rh-devices file if it 
also patches the build system to prevent this error occurring.

The question to ask is: what are you trying to do? If you want to test the latest and 
greatest code on a regular basis, then you want to be building from vanilla upstream 
to ensure that none of the vendor patches are responsible for any errors that you see.

If you're taking upstream and applying a vendor-specific configuration then equally 
that isn't a valid test, since your builds aren't integrating the vendor-specific 
patches which can potentially cause regressions/differences or different behaviours 
that don't exist in upstream.

Do you get a working build if you run rpmbuild on the SRPM containing the .spec file 
you linked to? Updating that .spec file to use a tarball of the latest code seems to 
be the only way to get a build that is as close as possible to RHEL/CentOS stream.

>> 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.


ATB,

Mark.
Murilo Opsfelder Araújo May 6, 2022, 11:44 p.m. UTC | #12 
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.

I've sent a v2 of this patch that addresses this, i.e.: the CONFIG_* options are available in hmp-commands-info.hx:

     https://lore.kernel.org/qemu-devel/20220506011632.183257-1-muriloo@linux.ibm.com/

I hope it can resolve the build issue in the short-term.
I'd appreciate if you or anyone on this thread could review it.

Thank you, Mark, for the discussion and knowledge sharing!

Cheers!
Mark Cave-Ayland May 8, 2022, 9:30 a.m. UTC | #13 
On 07/05/2022 00:44, Murilo Opsfelder Araújo wrote:

> 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.
> 
> I've sent a v2 of this patch that addresses this, i.e.: the CONFIG_* options are 
> available in hmp-commands-info.hx:
> 
>      https://lore.kernel.org/qemu-devel/20220506011632.183257-1-muriloo@linux.ibm.com/
> 
> I hope it can resolve the build issue in the short-term.
> I'd appreciate if you or anyone on this thread could review it.
> 
> Thank you, Mark, for the discussion and knowledge sharing!

No worries. I suspect there must be a reason why this wasn't suggested when I 
submitted the original patch, but that knowledge would clearly lie with the HMP and 
QMP maintainers.


ATB,

Mark.
Thomas Huth May 10, 2022, 8:03 a.m. UTC | #14 
On 04/05/2022 16.48, Mark Cave-Ayland wrote:
> On 04/05/2022 15:26, Fabiano Rosas wrote:
> 
>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:
>>
>>> On 03/05/2022 15:06, Fabiano Rosas wrote:
>>>
>>>> Murilo Opsfelder Araújo <muriloo@linux.ibm.com> writes:
[...]
>>>> So ppc64-softmmu doesn't quite do what it says on the tin. I think in
>>>> the long run we need to revisit the conversation about whether to keep
>>>> the 32 & 64 bit builds separate. It is misleading that you're explicitly
>>>> excluding ppc-softmmu from the `--target-list`, but a some 32 bit stuff
>>>> still comes along implicitly.
>>>
>>> Unfortunately for historical reasons it isn't quite that simple: the 
>>> mac99 machine in
>>> hw/ppc/mac_newworld.c is both a 32-bit and a 64-bit machine, but with a 
>>> different PCI
>>> host bridge and a 970 CPU if run from qemu-system-ppc64. Unfortunately it 
>>> pre-dates
>>> my time working on QEMU's PPC Mac machines but I believe it was (is?) 
>>> capable of
>>> booting Linux, even though I doubt it accurately represents a real machine.
>>
>> Well, what you describe is fine in my view, the 64-bit machine uses
>> qemu-system-ppc64. If there is shared code with 32-bit, that is ok.
>>
>> What I would like to understand is what is the community's direction
>> with this, do we want:
>>
>> 1) the 64-bit build to include all of the code and have it run all
>>     machines, or;
>>
>> 2) the 64-bit build to run only 64-bit machines and the 32-bit build to
>>     run only 32-bit machines.
>>
>> There are things such as 'target_ulong' that go against #1, and this
>> thread shows that we're not doing #2 as well.
>>
>> I know there have been discussions about this in the past but I couldn't
>> find them in the archives.
> 
> Certainly if a 64-bit Mac machine were to be added today, I'd be inclined to 
> copy mac_newworld.c into a separate file and give it a separate machine name 
> for ppc64 to make a clear distinction between the two machines (and allow 
> them to evolve separately). Unfortunately I have no idea as to what the 
> reference machine for the PPC64 Mac machine was supposed to be which makes 
> it harder to decide what to do :(
> 
> In my mind it feels like qemu-system-ppc is for 32-bit guests and 
> qemu-system-ppc64 is for 64-bit guests which I believe is consistent with 
> how it currently works with MIPS and ARM (someone feel free to correct me 
> here).

For CPUs that have both, 32-bit and 64-bit variants, we have mixed approaches:

1) For x86_64/i386, aarch64/arm, mips64/mips and ppc64/ppc, the 64-bit 
variants are a superset of the 32-bit variants, i.e. if you build the 64-bit 
version, you normally don't need the 32-bit version anymore (see below for 
the KVM-case where you still might need it).

2) For sparc64/sparc and riscv64/riscv32, the set of machines is distinct 
between the 64-bit and 32-bit versions (well, riscv has some machines 
shared, and some machines are different).

I once suggested in the past already that we should maybe get rid of the 
32-bit variants in case the 64-bit variant is a full superset, so we can 
save compile- and test times (which is quite a bit for QEMU), but I've been 
told that the 32-bit variants are mostly still required for supporting KVM 
on 32-bit host machines.

But in the long run, I think we rather want to converge everything into one 
binary (to decrease testing and compilation time) instead of separating 
stuff into multiple binaries, so IMHO we should not start separating the 
32-bit machines into qemu-system-ppc and the 64-bit-only machines into 
qemu-system-ppc64 now.

  Thomas
Alistair Francis May 10, 2022, 8:26 a.m. UTC | #15 
On Tue, May 10, 2022 at 10:07 AM Thomas Huth <thuth@redhat.com> wrote:
>
> On 04/05/2022 16.48, Mark Cave-Ayland wrote:
> > On 04/05/2022 15:26, Fabiano Rosas wrote:
> >
> >> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:
> >>
> >>> On 03/05/2022 15:06, Fabiano Rosas wrote:
> >>>
> >>>> Murilo Opsfelder Araújo <muriloo@linux.ibm.com> writes:
> [...]
> >>>> So ppc64-softmmu doesn't quite do what it says on the tin. I think in
> >>>> the long run we need to revisit the conversation about whether to keep
> >>>> the 32 & 64 bit builds separate. It is misleading that you're explicitly
> >>>> excluding ppc-softmmu from the `--target-list`, but a some 32 bit stuff
> >>>> still comes along implicitly.
> >>>
> >>> Unfortunately for historical reasons it isn't quite that simple: the
> >>> mac99 machine in
> >>> hw/ppc/mac_newworld.c is both a 32-bit and a 64-bit machine, but with a
> >>> different PCI
> >>> host bridge and a 970 CPU if run from qemu-system-ppc64. Unfortunately it
> >>> pre-dates
> >>> my time working on QEMU's PPC Mac machines but I believe it was (is?)
> >>> capable of
> >>> booting Linux, even though I doubt it accurately represents a real machine.
> >>
> >> Well, what you describe is fine in my view, the 64-bit machine uses
> >> qemu-system-ppc64. If there is shared code with 32-bit, that is ok.
> >>
> >> What I would like to understand is what is the community's direction
> >> with this, do we want:
> >>
> >> 1) the 64-bit build to include all of the code and have it run all
> >>     machines, or;
> >>
> >> 2) the 64-bit build to run only 64-bit machines and the 32-bit build to
> >>     run only 32-bit machines.
> >>
> >> There are things such as 'target_ulong' that go against #1, and this
> >> thread shows that we're not doing #2 as well.
> >>
> >> I know there have been discussions about this in the past but I couldn't
> >> find them in the archives.
> >
> > Certainly if a 64-bit Mac machine were to be added today, I'd be inclined to
> > copy mac_newworld.c into a separate file and give it a separate machine name
> > for ppc64 to make a clear distinction between the two machines (and allow
> > them to evolve separately). Unfortunately I have no idea as to what the
> > reference machine for the PPC64 Mac machine was supposed to be which makes
> > it harder to decide what to do :(
> >
> > In my mind it feels like qemu-system-ppc is for 32-bit guests and
> > qemu-system-ppc64 is for 64-bit guests which I believe is consistent with
> > how it currently works with MIPS and ARM (someone feel free to correct me
> > here).
>
> For CPUs that have both, 32-bit and 64-bit variants, we have mixed approaches:
>
> 1) For x86_64/i386, aarch64/arm, mips64/mips and ppc64/ppc, the 64-bit
> variants are a superset of the 32-bit variants, i.e. if you build the 64-bit
> version, you normally don't need the 32-bit version anymore (see below for
> the KVM-case where you still might need it).
>
> 2) For sparc64/sparc and riscv64/riscv32, the set of machines is distinct
> between the 64-bit and 32-bit versions (well, riscv has some machines
> shared, and some machines are different).

For RISC-V we are slowly moving towards riscv64 being a superset of
riscv32, similar to the other architectures

>
> I once suggested in the past already that we should maybe get rid of the
> 32-bit variants in case the 64-bit variant is a full superset, so we can
> save compile- and test times (which is quite a bit for QEMU), but I've been
> told that the 32-bit variants are mostly still required for supporting KVM
> on 32-bit host machines.

That was the eventual long term plan for RISC-V, then we can have a
single binary for everything

>
> But in the long run, I think we rather want to converge everything into one
> binary (to decrease testing and compilation time) instead of separating
> stuff into multiple binaries, so IMHO we should not start separating the
> 32-bit machines into qemu-system-ppc and the 64-bit-only machines into
> qemu-system-ppc64 now.

Agreed!

Alistair

>
>   Thomas
>
>
Thomas Huth May 10, 2022, 8:40 a.m. UTC | #16 
On 05/05/2022 10.19, Mark Cave-Ayland wrote:
> On 05/05/2022 02:24, Murilo Opsfelder Araújo wrote:
> 
>> 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:
[...]
>>>>> 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.
> 
> Ah so that's the problem then - you can't guarantee the configuration from a 
> vendor-customised build will work with upstream, particularly when the build 
> system itself has been patched. More explanation below.
> 
>>> 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.
> 
> In an upstream build the default boards for each target are listed in the 
> configs/ directory and Kconfig specifies the dependencies such that for ppc 
> and ppc64 CONFIG_MOS6522 is **always** defined. However what happens in the 
> .spec file you linked to is that the device lists **are being overridden** 
> by the provided ppc64-rh-devices file which **doesn't** contain 
> CONFIG_MOS6522. It seems to me that the .spec file can only work with that 
> vendor-specific ppc64-rh-devices file if it also patches the build system to 
> prevent this error occurring.

Well, yes and no. QEMU was quite monolitic for many years (that's where most 
of these downstream patches have their origin from), but the Kconfig stuff 
that was introduced a while ago (which required quite a lot of patches to 
untangle many parts of the build) was indeed meant for giving more 
flexibility here, so that downstream vendors could build QEMU more easily 
with only a subset of the machines (which is not only a desire by Red Hat, 
by the way - do you remember the "nemu" fork a while ago for example?). We 
still have quite a bit on the TODO list for full flexibility (e.g. mips 
config has not been fully switched to Kconfig yet, I think), but for most 
architectures, it should already work that you only select a subset of the 
available machines.

However, as you already noticed the big downside is still that this is not 
tested automatically in the upstream CI environment, so regressions like 
this one here with the mos6522 are expected. In the long run, I think we 
certainly want to test e.g. this cut-down CentOS-stream configuration in the 
CI, too ... it's just a big bunch of work that nobody started to tangle with 
yet.

  Thomas
Markus Armbruster May 10, 2022, 8:54 a.m. UTC | #17 
Thomas Huth <thuth@redhat.com> writes:

[...]

> I once suggested in the past already that we should maybe get rid of
> the 32-bit variants in case the 64-bit variant is a full superset, so
> we can save compile- and test times (which is quite a bit for QEMU),
> but I've been told that the 32-bit variants are mostly still required
> for supporting KVM on 32-bit host machines.

Do we still care for 32-bit host machines?

[...]
Thomas Huth May 10, 2022, 9:01 a.m. UTC | #18 
On 10/05/2022 10.54, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
> [...]
> 
>> I once suggested in the past already that we should maybe get rid of
>> the 32-bit variants in case the 64-bit variant is a full superset, so
>> we can save compile- and test times (which is quite a bit for QEMU),
>> but I've been told that the 32-bit variants are mostly still required
>> for supporting KVM on 32-bit host machines.
> 
> Do we still care for 32-bit host machines?

As long as the Linux kernel still supports 32-bit KVM virtualization, I 
think we have to keep the userspace around for that, too.

But I wonder why we're keeping qemu-system-arm around? 32-bit KVM support 
for ARM has been removed with Linux kernel 5.7 as far as I know, so I think 
we could likely drop the qemu-system-arm nowadays, too? Peter, Richard, 
what's your opinion on this?

  Thomas
Peter Maydell May 10, 2022, 9:14 a.m. UTC | #19 
On Tue, 10 May 2022 at 10:01, Thomas Huth <thuth@redhat.com> wrote:
>
> On 10/05/2022 10.54, Markus Armbruster wrote:
> > Thomas Huth <thuth@redhat.com> writes:
> >
> > [...]
> >
> >> I once suggested in the past already that we should maybe get rid of
> >> the 32-bit variants in case the 64-bit variant is a full superset, so
> >> we can save compile- and test times (which is quite a bit for QEMU),
> >> but I've been told that the 32-bit variants are mostly still required
> >> for supporting KVM on 32-bit host machines.
> >
> > Do we still care for 32-bit host machines?
>
> As long as the Linux kernel still supports 32-bit KVM virtualization, I
> think we have to keep the userspace around for that, too.
>
> But I wonder why we're keeping qemu-system-arm around? 32-bit KVM support
> for ARM has been removed with Linux kernel 5.7 as far as I know, so I think
> we could likely drop the qemu-system-arm nowadays, too? Peter, Richard,
> what's your opinion on this?

Two main reasons, I think:
 * command-line compatibility (ie there are lots of
   command lines out there using that binary name)
 * nobody has yet cared enough to come up with a plan for what
   we want to do differently for these 32-bit architectures,
   so the default is "keep doing what we always have"

In particular, I don't want to get rid of qemu-system-arm as the
*only* 32-bit target binary we drop. Either we stick with what
we have or we have a larger plan for sorting this out consistently
across target architectures.

-- PMM
Dr. David Alan Gilbert May 10, 2022, 9:22 a.m. UTC | #20 
* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Tue, 10 May 2022 at 10:01, Thomas Huth <thuth@redhat.com> wrote:
> >
> > On 10/05/2022 10.54, Markus Armbruster wrote:
> > > Thomas Huth <thuth@redhat.com> writes:
> > >
> > > [...]
> > >
> > >> I once suggested in the past already that we should maybe get rid of
> > >> the 32-bit variants in case the 64-bit variant is a full superset, so
> > >> we can save compile- and test times (which is quite a bit for QEMU),
> > >> but I've been told that the 32-bit variants are mostly still required
> > >> for supporting KVM on 32-bit host machines.
> > >
> > > Do we still care for 32-bit host machines?
> >
> > As long as the Linux kernel still supports 32-bit KVM virtualization, I
> > think we have to keep the userspace around for that, too.
> >
> > But I wonder why we're keeping qemu-system-arm around? 32-bit KVM support
> > for ARM has been removed with Linux kernel 5.7 as far as I know, so I think
> > we could likely drop the qemu-system-arm nowadays, too? Peter, Richard,
> > what's your opinion on this?
> 
> Two main reasons, I think:
>  * command-line compatibility (ie there are lots of
>    command lines out there using that binary name)
>  * nobody has yet cared enough to come up with a plan for what
>    we want to do differently for these 32-bit architectures,
>    so the default is "keep doing what we always have"
> 
> In particular, I don't want to get rid of qemu-system-arm as the
> *only* 32-bit target binary we drop. Either we stick with what
> we have or we have a larger plan for sorting this out consistently
> across target architectures.

To my mind, qemu-system-arm makes a lot of sense, and I'd rather see the
32 bit guests disappear from qemu-system-aarch64.
It's difficult to justify to someone running their aarch virt stack why
their binary has the security footprint that includes a camera or PDA.

ARM is a lot cleaner than x86; you don't suddenly find a little Cortex-M
machine with a big 64 bit core in it; yet on x86 our machines are
frankenstinian mixes with 25 year old chipsets and modern CPUs.

Dave

> -- PMM
>
Thomas Huth May 10, 2022, 9:31 a.m. UTC | #21 
On 10/05/2022 11.22, Dr. David Alan Gilbert wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> On Tue, 10 May 2022 at 10:01, Thomas Huth <thuth@redhat.com> wrote:
>>>
>>> On 10/05/2022 10.54, Markus Armbruster wrote:
>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>
>>>> [...]
>>>>
>>>>> I once suggested in the past already that we should maybe get rid of
>>>>> the 32-bit variants in case the 64-bit variant is a full superset, so
>>>>> we can save compile- and test times (which is quite a bit for QEMU),
>>>>> but I've been told that the 32-bit variants are mostly still required
>>>>> for supporting KVM on 32-bit host machines.
>>>>
>>>> Do we still care for 32-bit host machines?
>>>
>>> As long as the Linux kernel still supports 32-bit KVM virtualization, I
>>> think we have to keep the userspace around for that, too.
>>>
>>> But I wonder why we're keeping qemu-system-arm around? 32-bit KVM support
>>> for ARM has been removed with Linux kernel 5.7 as far as I know, so I think
>>> we could likely drop the qemu-system-arm nowadays, too? Peter, Richard,
>>> what's your opinion on this?
>>
>> Two main reasons, I think:
>>   * command-line compatibility (ie there are lots of
>>     command lines out there using that binary name)
>>   * nobody has yet cared enough to come up with a plan for what
>>     we want to do differently for these 32-bit architectures,
>>     so the default is "keep doing what we always have"
>>
>> In particular, I don't want to get rid of qemu-system-arm as the
>> *only* 32-bit target binary we drop. Either we stick with what
>> we have or we have a larger plan for sorting this out consistently
>> across target architectures.
> 
> To my mind, qemu-system-arm makes a lot of sense, and I'd rather see the
> 32 bit guests disappear from qemu-system-aarch64.
> It's difficult to justify to someone running their aarch virt stack why
> their binary has the security footprint that includes a camera or PDA.

I'm not very familiar with KVM on ARM - but is it possible to use KVM there 
with an arbitrary machine? If that's the case, a user might want to use KVM 
on their 64-bit host to run a 32-bit guest machine, and then you need to 
keep the 32-bit machines in the -aarch64 binary.

Something like that is at least theoretically possible with ppc64, I think: 
Using KVM-PR, it should be possible to run a g3beige (i.e. 32-bit) machine 
on a 64-bit host. Not sure whether anybody has tried that in recent times 
(afaik KVM-PR is in a rather bad shape nowadays), but it might have been 
possible at one point in time in the past. (PPC folks, please correct me if 
I'm wrong)

  Thomas
Peter Maydell May 10, 2022, 9:47 a.m. UTC | #22 
On Tue, 10 May 2022 at 10:31, Thomas Huth <thuth@redhat.com> wrote:
> On 10/05/2022 11.22, Dr. David Alan Gilbert wrote:
> > To my mind, qemu-system-arm makes a lot of sense, and I'd rather see the
> > 32 bit guests disappear from qemu-system-aarch64.
> > It's difficult to justify to someone running their aarch virt stack why
> > their binary has the security footprint that includes a camera or PDA.

There's a lot of stuff you would want to chop out if you want
a stripped-down "just for KVM" binary, though, and not all of
it is 32-bit-related.  And unless you're chopping non-KVM
machine types and devices out of qemu-system-aarch64 anyway,
you need the 32-bit CPU support in there: we have a machine
type which has both 64-bit and 32-bit CPUs in it.

> I'm not very familiar with KVM on ARM - but is it possible to use KVM there
> with an arbitrary machine? If that's the case, a user might want to use KVM
> on their 64-bit host to run a 32-bit guest machine, and then you need to
> keep the 32-bit machines in the -aarch64 binary.

No, Arm KVM is pretty restrictive -- effectively you can only really
use it with the 'virt' board. You can do a 32-bit guest, but you
do that with qemu-system-aarch64 and telling the 64-bit vcpu
"actually start with EL1 (guest kernel) in 32-bit mode".

thanks
-- PMM
BALATON Zoltan May 10, 2022, 10:14 a.m. UTC | #23 
On Tue, 10 May 2022, Thomas Huth wrote:
> On 10/05/2022 11.22, Dr. David Alan Gilbert wrote:
>> * Peter Maydell (peter.maydell@linaro.org) wrote:
>>> On Tue, 10 May 2022 at 10:01, Thomas Huth <thuth@redhat.com> wrote:
>>>> 
>>>> On 10/05/2022 10.54, Markus Armbruster wrote:
>>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>> 
>>>>> [...]
>>>>> 
>>>>>> I once suggested in the past already that we should maybe get rid of
>>>>>> the 32-bit variants in case the 64-bit variant is a full superset, so
>>>>>> we can save compile- and test times (which is quite a bit for QEMU),
>>>>>> but I've been told that the 32-bit variants are mostly still required
>>>>>> for supporting KVM on 32-bit host machines.
>>>>> 
>>>>> Do we still care for 32-bit host machines?
>>>> 
>>>> As long as the Linux kernel still supports 32-bit KVM virtualization, I
>>>> think we have to keep the userspace around for that, too.
>>>> 
>>>> But I wonder why we're keeping qemu-system-arm around? 32-bit KVM support
>>>> for ARM has been removed with Linux kernel 5.7 as far as I know, so I 
>>>> think
>>>> we could likely drop the qemu-system-arm nowadays, too? Peter, Richard,
>>>> what's your opinion on this?
>>> 
>>> Two main reasons, I think:
>>>   * command-line compatibility (ie there are lots of
>>>     command lines out there using that binary name)
>>>   * nobody has yet cared enough to come up with a plan for what
>>>     we want to do differently for these 32-bit architectures,
>>>     so the default is "keep doing what we always have"
>>> 
>>> In particular, I don't want to get rid of qemu-system-arm as the
>>> *only* 32-bit target binary we drop. Either we stick with what
>>> we have or we have a larger plan for sorting this out consistently
>>> across target architectures.
>> 
>> To my mind, qemu-system-arm makes a lot of sense, and I'd rather see the
>> 32 bit guests disappear from qemu-system-aarch64.
>> It's difficult to justify to someone running their aarch virt stack why
>> their binary has the security footprint that includes a camera or PDA.
>
> I'm not very familiar with KVM on ARM - but is it possible to use KVM there 
> with an arbitrary machine? If that's the case, a user might want to use KVM 
> on their 64-bit host to run a 32-bit guest machine, and then you need to keep 
> the 32-bit machines in the -aarch64 binary.
>
> Something like that is at least theoretically possible with ppc64, I think: 
> Using KVM-PR, it should be possible to run a g3beige (i.e. 32-bit) machine on 
> a 64-bit host. Not sure whether anybody has tried that in recent times (afaik 
> KVM-PR is in a rather bad shape nowadays), but it might have been possible at 
> one point in time in the past. (PPC folks, please correct me if I'm wrong)

https://www.talospace.com/2018/08/making-your-talos-ii-into-power-mac.html

Regards,
BALATON Zoltan
Gerd Hoffmann May 10, 2022, 12:20 p.m. UTC | #24 
Hi,

> I'm not very familiar with KVM on ARM - but is it possible to use KVM there
> with an arbitrary machine? If that's the case, a user might want to use KVM
> on their 64-bit host to run a 32-bit guest machine, and then you need to
> keep the 32-bit machines in the -aarch64 binary.

32-bit guest is 'qemu-system-aarch64 -machine virt -cpu host,aarch64=off'
(and a aarch64 CPU which actually supports the armv7 instructions).

take care,
  Gerd
Peter Maydell May 10, 2022, 12:25 p.m. UTC | #25 
On Tue, 10 May 2022 at 13:21, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > I'm not very familiar with KVM on ARM - but is it possible to use KVM there
> > with an arbitrary machine? If that's the case, a user might want to use KVM
> > on their 64-bit host to run a 32-bit guest machine, and then you need to
> > keep the 32-bit machines in the -aarch64 binary.
>
> 32-bit guest is 'qemu-system-aarch64 -machine virt -cpu host,aarch64=off'
> (and a aarch64 CPU which actually supports the armv7 instructions).

It's still armv8 even when it's 32-bit :-) The requirement is AArch32
support at EL1. (Some hardware has AArch32 support at EL0 only,
ie userspace but not kernel, which obviously isn't enough for 32-bit
KVM guests.)

-- PMM
diff mbox series

Patch

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``