Re: [Qemu-devel] [PATCH] smbios: stop ignoring command line options for TARGET_ARM

[Igor Mammedov]

On Wed, 21 Dec 2016 12:35:09 +0000
Leif Lindholm <address@hidden> 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:  
> >   
>