diff mbox

smbios: stop ignoring command line options for TARGET_ARM

Message ID 20161216152319.12494-1-leif.lindholm@linaro.org
State New
Headers show

Commit Message

Leif Lindholm Dec. 16, 2016, 3:23 p.m. UTC 
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(-)

-- 
2.10.2

Comments

no-reply@patchew.org Dec. 16, 2016, 3:31 p.m. UTC | #1 
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
Leif Lindholm Dec. 19, 2016, 7:33 p.m. UTC | #2 
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

>
Igor Mammedov Dec. 21, 2016, 10:51 a.m. UTC | #3 
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:
Leif Lindholm Dec. 21, 2016, 12:35 p.m. UTC | #4 
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:

>
Igor Mammedov Dec. 21, 2016, 1:59 p.m. UTC | #5 
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:  

> >   

>
Paolo Bonzini Dec. 21, 2016, 5:58 p.m. UTC | #6 
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
Leif Lindholm Dec. 22, 2016, 3:18 p.m. UTC | #7 
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
diff mbox

Patch

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: