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

[Igor Mammedov]

On Mon, 25 Mar 2024 21:01:42 +0300
Michael Tokarev <mjt@tls.msk.ru> wrote:

> 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'll look into what pulls in fwcfg and SMBIOS into microvm only config

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

Defines are usually frowned upon in QEMU and stubs
are typically preferred way to go.

I've just sent a patch adding a missing stub,
fill free to take it via your tree if time permits.

> 
> 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());  
> > 
> >   
>