diff mbox

qapi: add 'query-target' command to return target arch/bit size

Message ID 1345473098-26299-1-git-send-email-berrange@redhat.com
State New
Headers show

Commit Message

Daniel P. Berrangé Aug. 20, 2012, 2:31 p.m. UTC
From: "Daniel P. Berrange" <berrange@redhat.com>

Add a 'query-target' QAPI command to allow management applications
to determine what target architecture a QEMU binary is emulating
without having to parse the binary name or -help output

  $ qmp-shell -p /tmp/qemu
  (QEMU) query-target
  {   u'return': {   u'arch': u'x86_64', u'bits': 64}}

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 arch_init.c      | 11 +++++++++++
 qapi-schema.json | 25 +++++++++++++++++++++++++
 qmp-commands.hx  |  5 +++++
 3 files changed, 41 insertions(+)

Comments

Eric Blake Aug. 20, 2012, 3:47 p.m. UTC | #1
On 08/20/2012 08:31 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> Add a 'query-target' QAPI command to allow management applications
> to determine what target architecture a QEMU binary is emulating
> without having to parse the binary name or -help output
> 
>   $ qmp-shell -p /tmp/qemu
>   (QEMU) query-target
>   {   u'return': {   u'arch': u'x86_64', u'bits': 64}}
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  arch_init.c      | 11 +++++++++++
>  qapi-schema.json | 25 +++++++++++++++++++++++++
>  qmp-commands.hx  |  5 +++++
>  3 files changed, 41 insertions(+)

Adding the for 1.2 marking in the subject, to make it clear that this is
a "bug fix" for the fact that Anthony's QMP additions for libvirt were
incomplete, and should be pulled in now rather than delayed to 1.3.

> +##
> +# @TargetInfo:
> +#
> +# Information describing the QEMU target.
> +#
> +# @arch: the name of the target architecture (eg "x86_64", "i686", etc)

s/eg/e.g./ (it is an abbreviation of the two-word phrase exempli gratia)

Or even better, since mixing 'e.g.' and 'etc.' in the same sentence
sounds stupid, just go with the much simpler:

@arch: the name of the target architecture (such as "x86_64" or "i686")

> +# @bits: number of bits in physical address (eg 32 or 64)

Ditto.
Luiz Capitulino Aug. 20, 2012, 7:02 p.m. UTC | #2
On Mon, 20 Aug 2012 15:31:38 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> Add a 'query-target' QAPI command to allow management applications
> to determine what target architecture a QEMU binary is emulating
> without having to parse the binary name or -help output
> 
>   $ qmp-shell -p /tmp/qemu
>   (QEMU) query-target
>   {   u'return': {   u'arch': u'x86_64', u'bits': 64}}
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  arch_init.c      | 11 +++++++++++
>  qapi-schema.json | 25 +++++++++++++++++++++++++
>  qmp-commands.hx  |  5 +++++
>  3 files changed, 41 insertions(+)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 9b46bfc..095672d 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -1080,3 +1080,14 @@ int xen_available(void)
>      return 0;
>  #endif
>  }
> +
> +
> +TargetInfo *qmp_query_target(Error **errp)
> +{
> +    TargetInfo *info = g_malloc0(sizeof(*info));
> +
> +    info->arch = g_strdup(TARGET_ARCH);
> +    info->bits = TARGET_PHYS_ADDR_BITS;
> +
> +    return info;
> +}
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3d2b2d1..f0e3fe0 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2454,3 +2454,28 @@
>  #
>  ##
>  { 'command': 'query-fdsets', 'returns': ['FdsetInfo'] }
> +
> +##
> +# @TargetInfo:
> +#
> +# Information describing the QEMU target.
> +#
> +# @arch: the name of the target architecture (eg "x86_64", "i686", etc)

Should be an enum, otherwise looks good.

> +#
> +# @bits: number of bits in physical address (eg 32 or 64)
> +#
> +# Since: 1.2.0
> +##
> +{ 'type': 'TargetInfo',
> +  'data': { 'arch': 'str', 'bits': 'int' } }
> +
> +##
> +# @query-target:
> +#
> +# Return information about the target for this QEMU
> +#
> +# Returns: TargetInfo
> +#
> +# Since: 1.2.0
> +##
> +{ 'command': 'query-target', 'returns': 'TargetInfo' }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2ce4ce6..00d798f 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2509,3 +2509,8 @@ EQMP
>          .mhandler.cmd_new = qmp_marshal_input_query_cpu_definitions,
>      },
>  
> +    {
> +        .name       = "query-target",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_query_target,
> +    },
Anthony Liguori Aug. 20, 2012, 9:48 p.m. UTC | #3
"Daniel P. Berrange" <berrange@redhat.com> writes:

> From: "Daniel P. Berrange" <berrange@redhat.com>
>
> Add a 'query-target' QAPI command to allow management applications
> to determine what target architecture a QEMU binary is emulating
> without having to parse the binary name or -help output
>
>   $ qmp-shell -p /tmp/qemu
>   (QEMU) query-target
>   {   u'return': {   u'arch': u'x86_64', u'bits': 64}}

"bits" is really ambiguous.  What it means in QEMU (specifically the
value you are returning) is probably not what you expect it to mean.

We're going to most likely fix TARGET_PHYS_ADDR_BITS to 64 real soon.

Why did you include this field?  What information are you looking to get
from QEMU and what decisions do you plan to make with it?

Regards,

Anthony Liguori

>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  arch_init.c      | 11 +++++++++++
>  qapi-schema.json | 25 +++++++++++++++++++++++++
>  qmp-commands.hx  |  5 +++++
>  3 files changed, 41 insertions(+)
>
> diff --git a/arch_init.c b/arch_init.c
> index 9b46bfc..095672d 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -1080,3 +1080,14 @@ int xen_available(void)
>      return 0;
>  #endif
>  }
> +
> +
> +TargetInfo *qmp_query_target(Error **errp)
> +{
> +    TargetInfo *info = g_malloc0(sizeof(*info));
> +
> +    info->arch = g_strdup(TARGET_ARCH);
> +    info->bits = TARGET_PHYS_ADDR_BITS;
> +
> +    return info;
> +}
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3d2b2d1..f0e3fe0 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2454,3 +2454,28 @@
>  #
>  ##
>  { 'command': 'query-fdsets', 'returns': ['FdsetInfo'] }
> +
> +##
> +# @TargetInfo:
> +#
> +# Information describing the QEMU target.
> +#
> +# @arch: the name of the target architecture (eg "x86_64", "i686", etc)
> +#
> +# @bits: number of bits in physical address (eg 32 or 64)
> +#
> +# Since: 1.2.0
> +##
> +{ 'type': 'TargetInfo',
> +  'data': { 'arch': 'str', 'bits': 'int' } }
> +
> +##
> +# @query-target:
> +#
> +# Return information about the target for this QEMU
> +#
> +# Returns: TargetInfo
> +#
> +# Since: 1.2.0
> +##
> +{ 'command': 'query-target', 'returns': 'TargetInfo' }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2ce4ce6..00d798f 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2509,3 +2509,8 @@ EQMP
>          .mhandler.cmd_new = qmp_marshal_input_query_cpu_definitions,
>      },
>  
> +    {
> +        .name       = "query-target",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_query_target,
> +    },
> -- 
> 1.7.11.2
Daniel P. Berrangé Aug. 21, 2012, 10:05 a.m. UTC | #4
On Mon, Aug 20, 2012 at 04:48:24PM -0500, Anthony Liguori wrote:
> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
> > From: "Daniel P. Berrange" <berrange@redhat.com>
> >
> > Add a 'query-target' QAPI command to allow management applications
> > to determine what target architecture a QEMU binary is emulating
> > without having to parse the binary name or -help output
> >
> >   $ qmp-shell -p /tmp/qemu
> >   (QEMU) query-target
> >   {   u'return': {   u'arch': u'x86_64', u'bits': 64}}
> 
> "bits" is really ambiguous.  What it means in QEMU (specifically the
> value you are returning) is probably not what you expect it to mean.

My intent was to indicate the pointer word size for the architecture.
eg 64 for x86_64, ppc64, etc, and 32 and i686, ppc, etc. Probably
should have called it 'wordsize' or something like that

> We're going to most likely fix TARGET_PHYS_ADDR_BITS to 64 real soon.

Hmm, when I looked at the header in my checkout it already
*is* 64 or 32 as I'd expect for the architecture in question.

$ grep PHYS_ADDR_BITS */config-target.mak
alpha-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
arm-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
cris-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=32
i386-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
lm32-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=32
m68k-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=32
microblazeel-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=32
microblaze-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=32
mips64el-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
mips64-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
mipsel-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
mips-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
ppc64-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
ppcemb-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
ppc-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
s390x-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
sh4eb-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=32
sh4-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=32
sparc64-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
sparc-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
x86_64-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
xtensaeb-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=32
xtensa-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=32

> Why did you include this field?  What information are you looking to get
> from QEMU and what decisions do you plan to make with it?

When libvirt reports the host capabilities it includes the
architecture name and wordsize, amongst other things:

  # virsh capabilities
  ....snip...
  <guest>
    <os_type>hvm</os_type>
    <arch name='arm'>
      <wordsize>32</wordsize>
      <emulator>/bin/qemu-system-arm</emulator>
  ...

Currently we just have a table of arch name -> wordsize mapping
data in libvirt. I figured if I was adding a 'query-target' command
to QEMU, we might as well include this info too. It is not critical
though if you'd rather we omitted it though.

Regards,
Daniel
Daniel P. Berrangé Aug. 21, 2012, 10:07 a.m. UTC | #5
On Mon, Aug 20, 2012 at 04:02:39PM -0300, Luiz Capitulino wrote:
> On Mon, 20 Aug 2012 15:31:38 +0100
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > From: "Daniel P. Berrange" <berrange@redhat.com>
> > 
> > Add a 'query-target' QAPI command to allow management applications
> > to determine what target architecture a QEMU binary is emulating
> > without having to parse the binary name or -help output
> > 
> >   $ qmp-shell -p /tmp/qemu
> >   (QEMU) query-target
> >   {   u'return': {   u'arch': u'x86_64', u'bits': 64}}
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  arch_init.c      | 11 +++++++++++
> >  qapi-schema.json | 25 +++++++++++++++++++++++++
> >  qmp-commands.hx  |  5 +++++
> >  3 files changed, 41 insertions(+)
> > 
> > diff --git a/arch_init.c b/arch_init.c
> > index 9b46bfc..095672d 100644
> > --- a/arch_init.c
> > +++ b/arch_init.c
> > @@ -1080,3 +1080,14 @@ int xen_available(void)
> >      return 0;
> >  #endif
> >  }
> > +
> > +
> > +TargetInfo *qmp_query_target(Error **errp)
> > +{
> > +    TargetInfo *info = g_malloc0(sizeof(*info));
> > +
> > +    info->arch = g_strdup(TARGET_ARCH);
> > +    info->bits = TARGET_PHYS_ADDR_BITS;
> > +
> > +    return info;
> > +}
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 3d2b2d1..f0e3fe0 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -2454,3 +2454,28 @@
> >  #
> >  ##
> >  { 'command': 'query-fdsets', 'returns': ['FdsetInfo'] }
> > +
> > +##
> > +# @TargetInfo:
> > +#
> > +# Information describing the QEMU target.
> > +#
> > +# @arch: the name of the target architecture (eg "x86_64", "i686", etc)
> 
> Should be an enum, otherwise looks good.

Really ? It feels a little bit odd to make this an enum IMHO.

Daniel
Peter Maydell Aug. 21, 2012, 10:17 a.m. UTC | #6
On 21 August 2012 11:05, Daniel P. Berrange <berrange@redhat.com> wrote:
> On Mon, Aug 20, 2012 at 04:48:24PM -0500, Anthony Liguori wrote:
>> "bits" is really ambiguous.  What it means in QEMU (specifically the
>> value you are returning) is probably not what you expect it to mean.
>
> My intent was to indicate the pointer word size for the architecture.
> eg 64 for x86_64, ppc64, etc, and 32 and i686, ppc, etc. Probably
> should have called it 'wordsize' or something like that

This is not the same as the physical address size...

> Hmm, when I looked at the header in my checkout it already
> *is* 64 or 32 as I'd expect for the architecture in question.
>
> $ grep PHYS_ADDR_BITS */config-target.mak
> alpha-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
> arm-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64

...eg for ARM we have a 32 bit pointer size but 40 bit physical
addresses (on some cores) and we set TARGET_PHYS_ADDR_BITS
to 64 in all cases.

> i386-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64

i386 is the other obvious "pointers are 32 bit but we
set TARGET_PHYS_ADDR_BITS wider", for about the same reasons.

-- PMM
Daniel P. Berrangé Aug. 21, 2012, 10:24 a.m. UTC | #7
On Tue, Aug 21, 2012 at 11:17:40AM +0100, Peter Maydell wrote:
> On 21 August 2012 11:05, Daniel P. Berrange <berrange@redhat.com> wrote:
> > On Mon, Aug 20, 2012 at 04:48:24PM -0500, Anthony Liguori wrote:
> >> "bits" is really ambiguous.  What it means in QEMU (specifically the
> >> value you are returning) is probably not what you expect it to mean.
> >
> > My intent was to indicate the pointer word size for the architecture.
> > eg 64 for x86_64, ppc64, etc, and 32 and i686, ppc, etc. Probably
> > should have called it 'wordsize' or something like that
> 
> This is not the same as the physical address size...
> 
> > Hmm, when I looked at the header in my checkout it already
> > *is* 64 or 32 as I'd expect for the architecture in question.
> >
> > $ grep PHYS_ADDR_BITS */config-target.mak
> > alpha-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
> > arm-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
> 
> ...eg for ARM we have a 32 bit pointer size but 40 bit physical
> addresses (on some cores) and we set TARGET_PHYS_ADDR_BITS
> to 64 in all cases.
> 
> > i386-softmmu/config-target.mak:TARGET_PHYS_ADDR_BITS=64
> 
> i386 is the other obvious "pointers are 32 bit but we
> set TARGET_PHYS_ADDR_BITS wider", for about the same reasons.

Ok, so I'll just respin this patch & remove the 'bits' field entirely
so we avoid the confusion.

Daniel
Luiz Capitulino Aug. 21, 2012, 12:53 p.m. UTC | #8
On Tue, 21 Aug 2012 11:07:50 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Mon, Aug 20, 2012 at 04:02:39PM -0300, Luiz Capitulino wrote:
> > On Mon, 20 Aug 2012 15:31:38 +0100
> > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > 
> > > From: "Daniel P. Berrange" <berrange@redhat.com>
> > > 
> > > Add a 'query-target' QAPI command to allow management applications
> > > to determine what target architecture a QEMU binary is emulating
> > > without having to parse the binary name or -help output
> > > 
> > >   $ qmp-shell -p /tmp/qemu
> > >   (QEMU) query-target
> > >   {   u'return': {   u'arch': u'x86_64', u'bits': 64}}
> > > 
> > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > > ---
> > >  arch_init.c      | 11 +++++++++++
> > >  qapi-schema.json | 25 +++++++++++++++++++++++++
> > >  qmp-commands.hx  |  5 +++++
> > >  3 files changed, 41 insertions(+)
> > > 
> > > diff --git a/arch_init.c b/arch_init.c
> > > index 9b46bfc..095672d 100644
> > > --- a/arch_init.c
> > > +++ b/arch_init.c
> > > @@ -1080,3 +1080,14 @@ int xen_available(void)
> > >      return 0;
> > >  #endif
> > >  }
> > > +
> > > +
> > > +TargetInfo *qmp_query_target(Error **errp)
> > > +{
> > > +    TargetInfo *info = g_malloc0(sizeof(*info));
> > > +
> > > +    info->arch = g_strdup(TARGET_ARCH);
> > > +    info->bits = TARGET_PHYS_ADDR_BITS;
> > > +
> > > +    return info;
> > > +}
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index 3d2b2d1..f0e3fe0 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -2454,3 +2454,28 @@
> > >  #
> > >  ##
> > >  { 'command': 'query-fdsets', 'returns': ['FdsetInfo'] }
> > > +
> > > +##
> > > +# @TargetInfo:
> > > +#
> > > +# Information describing the QEMU target.
> > > +#
> > > +# @arch: the name of the target architecture (eg "x86_64", "i686", etc)
> > 
> > Should be an enum, otherwise looks good.
> 
> Really ? It feels a little bit odd to make this an enum IMHO.

I don't think it's odd. We should avoid using free-form strings when
the set of possible values a command returns is limited and known.

The only small issue I see though, is that TARGET_ARCH is defined in
configure, so you'll have to add something like CONFIG_TARGET_ENUM there too
(although we could probably kill TARGET_ARCH and use the strings generated
by the qapi, but this might end resulting too much work for the hard-freeze).
Daniel P. Berrangé Aug. 21, 2012, 1:07 p.m. UTC | #9
On Tue, Aug 21, 2012 at 09:53:55AM -0300, Luiz Capitulino wrote:
> On Tue, 21 Aug 2012 11:07:50 +0100
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > On Mon, Aug 20, 2012 at 04:02:39PM -0300, Luiz Capitulino wrote:
> > > On Mon, 20 Aug 2012 15:31:38 +0100
> > > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > > 
> > > > From: "Daniel P. Berrange" <berrange@redhat.com>
> > > > 
> > > > Add a 'query-target' QAPI command to allow management applications
> > > > to determine what target architecture a QEMU binary is emulating
> > > > without having to parse the binary name or -help output
> > > > 
> > > >   $ qmp-shell -p /tmp/qemu
> > > >   (QEMU) query-target
> > > >   {   u'return': {   u'arch': u'x86_64', u'bits': 64}}
> > > > 
> > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > > > ---
> > > >  arch_init.c      | 11 +++++++++++
> > > >  qapi-schema.json | 25 +++++++++++++++++++++++++
> > > >  qmp-commands.hx  |  5 +++++
> > > >  3 files changed, 41 insertions(+)
> > > > 
> > > > diff --git a/arch_init.c b/arch_init.c
> > > > index 9b46bfc..095672d 100644
> > > > --- a/arch_init.c
> > > > +++ b/arch_init.c
> > > > @@ -1080,3 +1080,14 @@ int xen_available(void)
> > > >      return 0;
> > > >  #endif
> > > >  }
> > > > +
> > > > +
> > > > +TargetInfo *qmp_query_target(Error **errp)
> > > > +{
> > > > +    TargetInfo *info = g_malloc0(sizeof(*info));
> > > > +
> > > > +    info->arch = g_strdup(TARGET_ARCH);
> > > > +    info->bits = TARGET_PHYS_ADDR_BITS;
> > > > +
> > > > +    return info;
> > > > +}
> > > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > > index 3d2b2d1..f0e3fe0 100644
> > > > --- a/qapi-schema.json
> > > > +++ b/qapi-schema.json
> > > > @@ -2454,3 +2454,28 @@
> > > >  #
> > > >  ##
> > > >  { 'command': 'query-fdsets', 'returns': ['FdsetInfo'] }
> > > > +
> > > > +##
> > > > +# @TargetInfo:
> > > > +#
> > > > +# Information describing the QEMU target.
> > > > +#
> > > > +# @arch: the name of the target architecture (eg "x86_64", "i686", etc)
> > > 
> > > Should be an enum, otherwise looks good.
> > 
> > Really ? It feels a little bit odd to make this an enum IMHO.
> 
> I don't think it's odd. We should avoid using free-form strings when
> the set of possible values a command returns is limited and known.
> 
> The only small issue I see though, is that TARGET_ARCH is defined in
> configure, so you'll have to add something like CONFIG_TARGET_ENUM there too

Well the (slightly inefficient) way is to just iterate over the enum
strings until you find the matching index. Not a good idea for large
enums, but I figure it'd be acceptable for the small number of arches.

Daniel
Anthony Liguori Aug. 21, 2012, 1:21 p.m. UTC | #10
"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Mon, Aug 20, 2012 at 04:48:24PM -0500, Anthony Liguori wrote:
>> "Daniel P. Berrange" <berrange@redhat.com> writes:
>> 
>> > From: "Daniel P. Berrange" <berrange@redhat.com>
>> >
>> > Add a 'query-target' QAPI command to allow management applications
>> > to determine what target architecture a QEMU binary is emulating
>> > without having to parse the binary name or -help output
>> >
>> >   $ qmp-shell -p /tmp/qemu
>> >   (QEMU) query-target
>> >   {   u'return': {   u'arch': u'x86_64', u'bits': 64}}
>> 
>> "bits" is really ambiguous.  What it means in QEMU (specifically the
>> value you are returning) is probably not what you expect it to mean.
>
> My intent was to indicate the pointer word size for the architecture.
> eg 64 for x86_64, ppc64, etc, and 32 and i686, ppc, etc. Probably
> should have called it 'wordsize' or something like that

So this is physical address size which doesn't necessarily correspond to
whether you can run a "64-bit" OS or not.  As Peter mentioned, i386 can
have a larger physical address size even though it's limited to 32-bit
guests.

We really don't need to use a physical address size < 64-bit anymore.
It's just a matter of time before someone comes in and fixes
TARGET_PHYS_ADDR_BITS to 64 which ought to significantly reduce the
build time since we don't need to duplicate objects anymore.

So yeah, removing 'bits' is probably a good idea.

>   # virsh capabilities
>   ....snip...
>   <guest>
>     <os_type>hvm</os_type>
>     <arch name='arm'>
>       <wordsize>32</wordsize>
>       <emulator>/bin/qemu-system-arm</emulator>
>   ...
>
> Currently we just have a table of arch name -> wordsize mapping
> data in libvirt. I figured if I was adding a 'query-target' command
> to QEMU, we might as well include this info too. It is not critical
> though if you'd rather we omitted it though.

Does anyone actually use wordsize that libvirt reports?  What decisions
are made with that information?

I'd much rather QEMU provide this kind of information to libvirt but
understanding how it's used is important to figure out what to expose.

Regards,

Anthony Liguori

>
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
Anthony Liguori Aug. 22, 2012, 1:20 p.m. UTC | #11
"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Tue, Aug 21, 2012 at 09:53:55AM -0300, Luiz Capitulino wrote:
>> On Tue, 21 Aug 2012 11:07:50 +0100
>> "Daniel P. Berrange" <berrange@redhat.com> wrote:
>> 
>> > On Mon, Aug 20, 2012 at 04:02:39PM -0300, Luiz Capitulino wrote:
>> > > On Mon, 20 Aug 2012 15:31:38 +0100
>> > > "Daniel P. Berrange" <berrange@redhat.com> wrote:
>> > > 
>> > > > From: "Daniel P. Berrange" <berrange@redhat.com>
>> > > > 
>> > > > Add a 'query-target' QAPI command to allow management applications
>> > > > to determine what target architecture a QEMU binary is emulating
>> > > > without having to parse the binary name or -help output
>> > > > 
>> > > >   $ qmp-shell -p /tmp/qemu
>> > > >   (QEMU) query-target
>> > > >   {   u'return': {   u'arch': u'x86_64', u'bits': 64}}
>> > > > 
>> > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> > > > ---
>> > > >  arch_init.c      | 11 +++++++++++
>> > > >  qapi-schema.json | 25 +++++++++++++++++++++++++
>> > > >  qmp-commands.hx  |  5 +++++
>> > > >  3 files changed, 41 insertions(+)
>> > > > 
>> > > > diff --git a/arch_init.c b/arch_init.c
>> > > > index 9b46bfc..095672d 100644
>> > > > --- a/arch_init.c
>> > > > +++ b/arch_init.c
>> > > > @@ -1080,3 +1080,14 @@ int xen_available(void)
>> > > >      return 0;
>> > > >  #endif
>> > > >  }
>> > > > +
>> > > > +
>> > > > +TargetInfo *qmp_query_target(Error **errp)
>> > > > +{
>> > > > +    TargetInfo *info = g_malloc0(sizeof(*info));
>> > > > +
>> > > > +    info->arch = g_strdup(TARGET_ARCH);
>> > > > +    info->bits = TARGET_PHYS_ADDR_BITS;
>> > > > +
>> > > > +    return info;
>> > > > +}
>> > > > diff --git a/qapi-schema.json b/qapi-schema.json
>> > > > index 3d2b2d1..f0e3fe0 100644
>> > > > --- a/qapi-schema.json
>> > > > +++ b/qapi-schema.json
>> > > > @@ -2454,3 +2454,28 @@
>> > > >  #
>> > > >  ##
>> > > >  { 'command': 'query-fdsets', 'returns': ['FdsetInfo'] }
>> > > > +
>> > > > +##
>> > > > +# @TargetInfo:
>> > > > +#
>> > > > +# Information describing the QEMU target.
>> > > > +#
>> > > > +# @arch: the name of the target architecture (eg "x86_64", "i686", etc)
>> > > 
>> > > Should be an enum, otherwise looks good.
>> > 
>> > Really ? It feels a little bit odd to make this an enum IMHO.
>> 
>> I don't think it's odd. We should avoid using free-form strings when
>> the set of possible values a command returns is limited and known.
>> 
>> The only small issue I see though, is that TARGET_ARCH is defined in
>> configure, so you'll have to add something like CONFIG_TARGET_ENUM there too
>
> Well the (slightly inefficient) way is to just iterate over the enum
> strings until you find the matching index. Not a good idea for large
> enums, but I figure it'd be acceptable for the small number of arches.

We can create a new CONFIG_ #define that gets set directly to the
enumerator value.   No need for string comparison.

Regards,

Anthony Liguori

>
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
diff mbox

Patch

diff --git a/arch_init.c b/arch_init.c
index 9b46bfc..095672d 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1080,3 +1080,14 @@  int xen_available(void)
     return 0;
 #endif
 }
+
+
+TargetInfo *qmp_query_target(Error **errp)
+{
+    TargetInfo *info = g_malloc0(sizeof(*info));
+
+    info->arch = g_strdup(TARGET_ARCH);
+    info->bits = TARGET_PHYS_ADDR_BITS;
+
+    return info;
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index 3d2b2d1..f0e3fe0 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2454,3 +2454,28 @@ 
 #
 ##
 { 'command': 'query-fdsets', 'returns': ['FdsetInfo'] }
+
+##
+# @TargetInfo:
+#
+# Information describing the QEMU target.
+#
+# @arch: the name of the target architecture (eg "x86_64", "i686", etc)
+#
+# @bits: number of bits in physical address (eg 32 or 64)
+#
+# Since: 1.2.0
+##
+{ 'type': 'TargetInfo',
+  'data': { 'arch': 'str', 'bits': 'int' } }
+
+##
+# @query-target:
+#
+# Return information about the target for this QEMU
+#
+# Returns: TargetInfo
+#
+# Since: 1.2.0
+##
+{ 'command': 'query-target', 'returns': 'TargetInfo' }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 2ce4ce6..00d798f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2509,3 +2509,8 @@  EQMP
         .mhandler.cmd_new = qmp_marshal_input_query_cpu_definitions,
     },
 
+    {
+        .name       = "query-target",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_query_target,
+    },