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

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

[Leif Lindholm]

Commit c30e1565 ("smbios: implement smbios support for mach-virt")
enabled automatic generation of SMBIOS tables for TARGET_ARM, and
actually provides data for the "virt" machine.

However, do_smbios_option() still had an #ifdef TARGET_I386, preventing
any -smbios command line options from being parsed for any non-x86
targets.
Change this to use a status variable instead of compile-time filtering.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---

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
-    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;
 #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;
                 do_smbios_option(opts);
                 break;
             case QEMU_OPTION_fwcfg:
-- 
2.10.2

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

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH] smbios: stop ignoring command line options for TARGET_ARM
Message-id: 20161216152319.12494-1-leif.lindholm@linaro.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/148190196925.25504.12830466137601571123.stgit@bahia -> patchew/148190196925.25504.12830466137601571123.stgit@bahia
 * [new tag]         patchew/20161216152319.12494-1-leif.lindholm@linaro.org -> patchew/20161216152319.12494-1-leif.lindholm@linaro.org
Switched to a new branch 'test'
f9025dc smbios: stop ignoring command line options for TARGET_ARM

=== OUTPUT BEGIN ===
Checking PATCH 1/1: smbios: stop ignoring command line options for TARGET_ARM...
ERROR: do not initialise globals to 0 or NULL
#54: FILE: vl.c:162:
+int smbios_override = 0;

total: 1 errors, 0 warnings, 32 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

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

[Leif Lindholm]

So, in addition to the style issue I was automatically notified of, I
also neglected to CC the appropriate people - adding them here.

As for the style issue - is it more important to adhere to
checkpatch.pl or to surrounding definitions?

Regards,

Leif

On Fri, Dec 16, 2016 at 03:23:19PM +0000, Leif Lindholm wrote:
> Commit c30e1565 ("smbios: implement smbios support for mach-virt")
> enabled automatic generation of SMBIOS tables for TARGET_ARM, and
> actually provides data for the "virt" machine.
> 
> However, do_smbios_option() still had an #ifdef TARGET_I386, preventing
> any -smbios command line options from being parsed for any non-x86
> targets.
> Change this to use a status variable instead of compile-time filtering.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> 
> 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
> -    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;
>  #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;
>                  do_smbios_option(opts);
>                  break;
>              case QEMU_OPTION_fwcfg:
> -- 
> 2.10.2
> 

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

[Igor Mammedov]

On Fri, 16 Dec 2016 15:23:19 +0000
Leif Lindholm <leif.lindholm@linaro.org> wrote:

> Commit c30e1565 ("smbios: implement smbios support for mach-virt")
> enabled automatic generation of SMBIOS tables for TARGET_ARM, and
> actually provides data for the "virt" machine.
> 
> However, do_smbios_option() still had an #ifdef TARGET_I386, preventing
> any -smbios command line options from being parsed for any non-x86
> targets.
> Change this to use a status variable instead of compile-time filtering.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> 
> 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

> -    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.

>  #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,
so what's the point for adding 'smbios_override' at all?

Also this patch would break build for targets that don't link smbios.c
(i.e. which don't have CONFIG_SMBIOS=y)

>                  do_smbios_option(opts);
>                  break;
>              case QEMU_OPTION_fwcfg:

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

[Leif Lindholm]

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:
> 

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 <leif.lindholm@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:  
> >   
> 

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

[Paolo Bonzini]

On 21/12/2016 14:59, Igor Mammedov wrote:
>> 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

I don't think CONFIG_SMBIOS is visible from C, is it?

However, the solution is to:

1) add a smbios-stub.c file to hw/smbios, containing a dummy
implementation of smbios_entry_add.  For the Makefile magic see
hw/pci/Makefile.objs.

2) add an Error * argument to smbios_entry_add, and make the stub
version fail

3) remove do_smbios_option altogether, and make vl.c call
smbios_entry_add directly.

Paolo

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

[Leif Lindholm]

On Wed, Dec 21, 2016 at 06:58:44PM +0100, Paolo Bonzini wrote:
> On 21/12/2016 14:59, Igor Mammedov wrote:
> >> 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
> 
> I don't think CONFIG_SMBIOS is visible from C, is it?

No.
(A bit of context that was drop asked:
"... 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?"
But that doesn't matter now.)

> However, the solution is to:
> 
> 1) add a smbios-stub.c file to hw/smbios, containing a dummy
> implementation of smbios_entry_add.  For the Makefile magic see
> hw/pci/Makefile.objs.
> 
> 2) add an Error * argument to smbios_entry_add, and make the stub
> version fail
> 
> 3) remove do_smbios_option altogether, and make vl.c call
> smbios_entry_add directly.

Many thanks for the pointers, I've done just that and am sending it
out (as a new patch rather than a v2 because ... it bears no
resemblance to the first one).

Regards,

Leif