Re: [PATCH trivial for-9.0] hw/i386/fw_cfg.c: fix non-legacy smbios build

[Michael Tokarev]

25.03.2024 18:20, Igor Mammedov wrote
> On Mon, 25 Mar 2024 16:09:20 +0300
> Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> > When building qemu with smbios but not legacy mode (eg minimal microvm build),
> > link fails with:
> >
> >    hw/i386/fw_cfg.c:74: undefined reference to `smbios_get_table_legacy'
> >
> > This is because fw_cfg interface can call this function if CONFIG_SMBIOS
> > is defined.  Made this code block to depend on CONFIG_SMBIOS_LEGACY.
>
> stub supposedly should have handled that
> what configure options do you use to build 'minimal microvm'?

This is a custom build, not only configure options but also custom
devices.mak: 
https://salsa.debian.org/qemu-team/qemu/-/blob/master/debian/microvm-devices.mak

================== cut ==========================
# see configs/devices/i386-softmmu/default.mak
# for additional devices which can be disabled
#
CONFIG_PCI_DEVICES=n
# we can't disable all machine types (boards) as of 6.1
# since the resulting binary fails to link
#CONFIG_ISAPC=y
#CONFIG_I440FX=y
CONFIG_Q35=y
CONFIG_MICROVM=y
CONFIG_VIRTIO_BLK=y
CONFIG_VIRTIO_SERIAL=y
CONFIG_VIRTIO_INPUT=y
CONFIG_VIRTIO_INPUT_HOST=y
CONFIG_VHOST_USER_INPUT=y
CONFIG_VIRTIO_NET=y
CONFIG_VIRTIO_SCSI=y
CONFIG_VIRTIO_RNG=y
CONFIG_VIRTIO_CRYPTO=y
CONFIG_VIRTIO_BALLOON=y
CONFIG_VIRTIO_MEM=y
CONFIG_VIRTIO_PMEM=y
CONFIG_VIRTIO_GPU=y
CONFIG_VHOST_USER_GPU=y
================== cut ==========================

Relevant configure options:
https://salsa.debian.org/qemu-team/qemu/-/blob/master/debian/rules#L293-308

                ../../configure ${common_configure_opts} \
                --extra-cflags="${extra-cflags} -DCONFIG_MICROVM_DEFAULT=1" \
                --without-default-features \
                --target-list=x86_64-softmmu --enable-kvm --disable-tcg \
                --enable-pixman --enable-vnc \
                --enable-attr \
                --enable-coroutine-pool \
                --audio-drv-list="" \
                --without-default-devices \
                --with-devices-x86_64=microvm \
                --enable-vhost-kernel --enable-vhost-net \
                --enable-vhost-vdpa \
                --enable-vhost-user --enable-vhost-user-blk-server \
                --enable-vhost-crypto \

I dunno how relevant these are, - it come from ubuntu and I haven't
looked there for a long time.  The idea was to have a build especially
for microvm with minimal footprint, as light as possible, for fastest
startup time etc.

Enabling (selecting) CONFIG_SMBIOS does not help since it is already
enabled by something, but not SMBIOS_LEGACY (which should not be
enabled in this case).

I still think it is better to avoid pcmc->smbios_legacy_mode variable
(field) entirely.

Thanks,

/mjt

> > Fixes: b42b0e4daaa5 "smbios: build legacy mode code only for 'pc' machine"
> > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> > ---
> >   hw/i386/fw_cfg.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> > index d802d2787f..d5e78a9183 100644
> > --- a/hw/i386/fw_cfg.c
> > +++ b/hw/i386/fw_cfg.c
> > @@ -70,6 +70,7 @@ void fw_cfg_build_smbios(PCMachineState *pcms, FWCfgState 
> > *fw_cfg,
> >       /* tell smbios about cpuid version and features */
> >       smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
> >   
> > +#ifdef CONFIG_SMBIOS_LEGACY
> >       if (pcmc->smbios_legacy_mode) {
> >           smbios_tables = smbios_get_table_legacy(&smbios_tables_len,
> >                                                   &error_fatal);
> > @@ -77,6 +78,7 @@ void fw_cfg_build_smbios(PCMachineState *pcms, FWCfgState 
> > *fw_cfg,
> >                            smbios_tables, smbios_tables_len);
> >           return;
> >       }
> > +#endif
> >   
> >       /* build the array of physical mem area from e820 table */
> >       mem_array = g_malloc0(sizeof(*mem_array) * e820_get_num_entries());