On Wed, 21 Dec 2016 12:35:09 +0000
Leif Lindholm <leif.lindh...@linaro.org> wrote:
> 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.
Just do 1 build for all targets before posting patches to avoid
such kind of errors.
>
> So a simpler, and more correct fix would rather be to change the
> #ifdef TARGET_I386
> in arch_init.c to
> #ifdef CONFIG_SMBIOS
it looks better to me than enumerating targets explicitly,
CCing Paolo for another opinion
> ... 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:
> >
>
