On Wed, Dec 21, 2016 at 11:51:02AM +0100, Igor Mammedov wrote:
> On Fri, 16 Dec 2016 15:23:19 +0000
> > Verified on ARM mach-virt with UEFI shell "smbiosview" command and QEMU
> > command line parameter -smbios type=0,version=foobar.
> >
> > arch_init.c | 6 +++---
> > include/hw/smbios/smbios.h | 2 ++
> > vl.c | 2 ++
> > 3 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch_init.c b/arch_init.c
> > index 5cc58b2..d4e28c0 100644
> > --- a/arch_init.c
> > +++ b/arch_init.c
> > @@ -250,9 +250,9 @@ void do_acpitable_option(const QemuOpts *opts)
> >
> > void do_smbios_option(QemuOpts *opts)
> > {
> > -#ifdef TARGET_I386
> extending above condition to make it compiled for ARM
> would do what you need
Yes, but given how tedious that was to track down without a decent
grasp of the source tree structure I had hoped to improve the
experience for future newcomers.
If that's not considered important, sure, I could hack together a v2
consisting only of that.
> > - smbios_entry_add(opts);
> > -#endif
> > + if (smbios_override) {
> > + smbios_entry_add(opts);
> > + }
> > }
> >
> > int kvm_available(void)
> > diff --git a/include/hw/smbios/smbios.h b/include/hw/smbios/smbios.h
> > index 1cd53cc..2a3dca2 100644
> > --- a/include/hw/smbios/smbios.h
> > +++ b/include/hw/smbios/smbios.h
> > @@ -267,4 +267,6 @@ void smbios_get_tables(const struct
> > smbios_phys_mem_area *mem_array,
> > const unsigned int mem_array_size,
> > uint8_t **tables, size_t *tables_len,
> > uint8_t **anchor, size_t *anchor_len);
> > +
> > +extern int smbios_override;
> Adding global variables generally is not welcomed and one
> should try to avoid it if it could be helped.
That is certainly a fair comment. Not being too familiar with the
codebase I was slavishly trying to follow the style of the surrounding
code. Will avoid that in future.
> > #endif /* QEMU_SMBIOS_H */
> > diff --git a/vl.c b/vl.c
> > index d77dd86..8e71b06 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -159,6 +159,7 @@ int smp_cpus = 1;
> > int max_cpus = 1;
> > int smp_cores = 1;
> > int smp_threads = 1;
> > +int smbios_override = 0;
> > int acpi_enabled = 1;
> > int no_hpet = 0;
> > int fd_bootchk = 1;
> > @@ -3711,6 +3712,7 @@ int main(int argc, char **argv, char **envp)
> > if (!opts) {
> > exit(1);
> > }
> > + smbios_override = 1;
> ^^^ practically turns follow up call into unconditional
> smbios_entry_add(opts) call,
Which is what it already is for TARGET_I386.
> so what's the point for adding 'smbios_override' at all?
Apparently a misunderstanding of the underlying command line handling
mechanics.
> Also this patch would break build for targets that don't link smbios.c
> (i.e. which don't have CONFIG_SMBIOS=y)
Ah, I hadn't spotted that - apologies.
So a simpler, and more correct fix would rather be to change the
#ifdef TARGET_I386
in arch_init.c to
#ifdef CONFIG_SMBIOS
?
... if there had been a conveniently available CONFIG_SMBIOS.
Would it be acceptable to add one to config-target.h or is that
reserved for host-specific options?
Regards,
Leif
> > do_smbios_option(opts);
> > break;
> > case QEMU_OPTION_fwcfg:
>
