| Message ID | 20240325130920.349521-1-mjt@tls.msk.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [trivial,for-9.0] hw/i386/fw_cfg.c: fix non-legacy smbios build | expand |
Hi Michael, On 25/3/24 14:09, Michael Tokarev 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. > > 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) { But then having pcmc->smbios_legacy_mode == true without CONFIG_SMBIOS_LEGACY would be a bug. IMHO what we want is: -- >8 -- diff --git a/hw/smbios/smbios_legacy_stub.c b/hw/smbios/smbios_legacy_stub.c index f29b15316c..7d593dca98 100644 --- a/hw/smbios/smbios_legacy_stub.c +++ b/hw/smbios/smbios_legacy_stub.c @@ -13,3 +13,8 @@ void smbios_add_usr_blob_size(size_t size) { } + +uint8_t *smbios_get_table_legacy(size_t *length, Error **errp) +{ + g_assert_not_reached(); +} --- > 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());
25.03.2024 16:40, Philippe Mathieu-Daudé пишет: > Hi Michael, > > On 25/3/24 14:09, Michael Tokarev 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. >> >> 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) { > > But then having pcmc->smbios_legacy_mode == true without > CONFIG_SMBIOS_LEGACY would be a bug. IMHO what we want is: How about this instead (in addition to original patch): diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 27a68071d7..4e3c220b85 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -109,7 +109,9 @@ struct PCMachineClass { /* SMBIOS compat: */ bool smbios_defaults; +#ifdef CONFIG_SMBIOS_LEGACY bool smbios_legacy_mode; +#endif bool smbios_uuid_encoded; SmbiosEntryPointType default_smbios_ep_type; ? /mjt
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'? > > 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());
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. > > Fixes: b42b0e4daaa5 "smbios: build legacy mode code only for 'pc' machine" > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> hmh, it looks like MICROVM doesn't select SMBIOS nor FW_CFG_DMA which looks broken to me, does following help: diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig index a6ee052f9a..54c77b5bcc 100644 --- a/hw/i386/Kconfig +++ b/hw/i386/Kconfig @@ -119,6 +119,8 @@ config MICROVM select PCI_EXPRESS_GENERIC_BRIDGE select USB_XHCI_SYSBUS select I8254 + select SMBIOS + select FW_CFG_DMA config X86_IOMMU bool > --- > 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());
On Mon, 25 Mar 2024 14:40:21 +0100 Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > Hi Michael, > > On 25/3/24 14:09, Michael Tokarev 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. > > > > 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) { > > But then having pcmc->smbios_legacy_mode == true without > CONFIG_SMBIOS_LEGACY would be a bug. IMHO what we want is: > > -- >8 -- > diff --git a/hw/smbios/smbios_legacy_stub.c b/hw/smbios/smbios_legacy_stub.c > index f29b15316c..7d593dca98 100644 > --- a/hw/smbios/smbios_legacy_stub.c > +++ b/hw/smbios/smbios_legacy_stub.c > @@ -13,3 +13,8 @@ > void smbios_add_usr_blob_size(size_t size) > { > } > + > +uint8_t *smbios_get_table_legacy(size_t *length, Error **errp) > +{ > + g_assert_not_reached(); > +} Agreed Philippe, shall I post a patch or will you do this? > --- > > > 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()); >
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()); > >
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()); > > > > >
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());
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. 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(+)