Re: [PATCH trivial for-9.0] hw/i386/fw_cfg.c: fix non-legacy smbios build
On Mon, 25 Mar 2024 21:01:42 +0300
Michael Tokarev wrote:
> 25.03.2024 18:20, Igor Mammedov wrote
> > On Mon, 25 Mar 2024 16:09:20 +0300
> > 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.
> >
> > 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
> >> ---
> >> 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(_tables_len,
> >> _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());
> >
> >
>
Re: [PATCH trivial for-9.0] hw/i386/fw_cfg.c: fix non-legacy smbios build
25.03.2024 18:20, Igor Mammedov wrote
On Mon, 25 Mar 2024 16:09:20 +0300
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.
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
---
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(_tables_len,
_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());
Re: [PATCH trivial for-9.0] hw/i386/fw_cfg.c: fix non-legacy smbios build
On Mon, 25 Mar 2024 14:40:21 +0100
Philippe Mathieu-Daudé 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
> > ---
> > 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(_tables_len,
> > _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());
>
Re: [PATCH trivial for-9.0] hw/i386/fw_cfg.c: fix non-legacy smbios build
On Mon, 25 Mar 2024 16:09:20 +0300
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
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(_tables_len,
> _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());
Re: [PATCH trivial for-9.0] hw/i386/fw_cfg.c: fix non-legacy smbios build
On Mon, 25 Mar 2024 16:09:20 +0300
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.
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
> ---
> 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(_tables_len,
> _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());
Re: [PATCH trivial for-9.0] hw/i386/fw_cfg.c: fix non-legacy smbios build
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
---
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
Re: [PATCH trivial for-9.0] hw/i386/fw_cfg.c: fix non-legacy smbios build
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
---
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(_tables_len,
_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());
[PATCH trivial for-9.0] hw/i386/fw_cfg.c: fix non-legacy smbios build
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
---
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(_tables_len,
_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());
--
2.39.2
