diff mbox series

[v12] qapi: introduce 'query-x86-cpuid' QMP command.

Message ID 20210728125402.2496-1-valeriy.vdovin@virtuozzo.com
State New
Headers show
Series [v12] qapi: introduce 'query-x86-cpuid' QMP command. | expand

Commit Message

Valeriy Vdovin July 28, 2021, 12:54 p.m. UTC
Introducing new QMP command 'query-x86-cpuid'. This command can be used to
get virtualized cpu model info generated by QEMU during VM initialization in
the form of cpuid representation.

Diving into more details about virtual CPU generation: QEMU first parses '-cpu'
command line option. From there it takes the name of the model as the basis for
feature set of the new virtual CPU. After that it uses trailing '-cpu' options,
that state if additional cpu features should be present on the virtual CPU or
excluded from it (tokens '+'/'-' or '=on'/'=off').
After that QEMU checks if the host's cpu can actually support the derived
feature set and applies host limitations to it.
After this initialization procedure, virtual CPU has it's model and
vendor names, and a working feature set and is ready for identification
instructions such as CPUID.

To learn exactly how virtual CPU is presented to the guest machine via CPUID
instruction, new QMP command can be used. By calling 'query-x86-cpuid'
command, one can get a full listing of all CPUID leaves with subleaves which are
supported by the initialized virtual CPU.

Other than debug, the command is useful in cases when we would like to
utilize QEMU's virtual CPU initialization routines and put the retrieved
values into kernel CPUID overriding mechanics for more precise control
over how various processes perceive its underlying hardware with
container processes as a good example.

The command is specific to x86. It is currenly only implemented for KVM acceleator.

Output format:
The output is a plain list of leaf/subleaf argument combinations, that
return 4 words in registers EAX, EBX, ECX, EDX.

Use example:
qmp_request: {
  "execute": "query-x86-cpuid"
}

qmp_response: {
  "return": [
    {
      "eax": 1073741825,
      "edx": 77,
      "in-eax": 1073741824,
      "ecx": 1447775574,
      "ebx": 1263359563
    },
    {
      "eax": 16777339,
      "edx": 0,
      "in-eax": 1073741825,
      "ecx": 0,
      "ebx": 0
    },
    {
      "eax": 13,
      "edx": 1231384169,
      "in-eax": 0,
      "ecx": 1818588270,
      "ebx": 1970169159
    },
    {
      "eax": 198354,
      "edx": 126614527,
      "in-eax": 1,
      "ecx": 2176328193,
      "ebx": 2048
    },
    ....
    {
      "eax": 12328,
      "edx": 0,
      "in-eax": 2147483656,
      "ecx": 0,
      "ebx": 0
    }
  ]
}

Signed-off-by: Valeriy Vdovin <valeriy.vdovin@virtuozzo.com>
---
This patch is next version (v12) of a patch with a changed subject
Previous subject: [PATCH v11] qapi: introduce 'query-kvm-cpuid' QMP command
https://patchew.org/QEMU/20210617122723.8670-1-valeriy.vdovin@virtuozzo.com/
Supersedes: 20210617122723.8670-1-valeriy.vdovin@virtuozzo.com

v2: - Removed leaf/subleaf iterators.
    - Modified cpu_x86_cpuid to return false in cases when count is
      greater than supported subleaves.
v3: - Fixed structure name coding style.
    - Added more comments
    - Ensured buildability for non-x86 targets.
v4: - Fixed cpu_x86_cpuid return value logic and handling of 0xA leaf.
    - Fixed comments.
    - Removed target check in qmp_query_cpu_model_cpuid.
v5: - Added error handling code in qmp_query_cpu_model_cpuid
v6: - Fixed error handling code. Added method to query_error_class
v7: - Changed implementation in favor of cached cpuid_data for
      KVM_SET_CPUID2
v8: - Renamed qmp method to query-kvm-cpuid and some fields in response.
    - Modified documentation to qmp method
    - Removed helper struct declaration
v9: - Renamed 'in_eax' / 'in_ecx' fields to 'in-eax' / 'in-ecx'
    - Pasted more complete response to commit message.
v10:
    - Subject changed
    - Fixes in commit message
    - Small fixes in QMP command docs
v11:
    - Added explanation about CONFIG_KVM to the commit message.
v12:
    - Changed title from query-kvm-cpuid to query-x86-cpuid
    - Removed CONFIG_KVM ifdefs
    - Added detailed error messages for some stub/unimplemented cases.

 qapi/machine-target.json   | 44 ++++++++++++++++++++++++++++++++
 softmmu/cpus.c             |  2 +-
 target/i386/kvm/kvm-stub.c | 10 ++++++++
 target/i386/kvm/kvm.c      | 51 ++++++++++++++++++++++++++++++++++++++
 tests/qtest/qmp-cmd-test.c |  1 +
 5 files changed, 107 insertions(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé July 28, 2021, 1:25 p.m. UTC | #1
On 7/28/21 2:54 PM, Valeriy Vdovin wrote:
> Introducing new QMP command 'query-x86-cpuid'. This command can be used to
> get virtualized cpu model info generated by QEMU during VM initialization in
> the form of cpuid representation.
> 
> Diving into more details about virtual CPU generation: QEMU first parses '-cpu'
> command line option. From there it takes the name of the model as the basis for
> feature set of the new virtual CPU. After that it uses trailing '-cpu' options,
> that state if additional cpu features should be present on the virtual CPU or
> excluded from it (tokens '+'/'-' or '=on'/'=off').
> After that QEMU checks if the host's cpu can actually support the derived
> feature set and applies host limitations to it.
> After this initialization procedure, virtual CPU has it's model and
> vendor names, and a working feature set and is ready for identification
> instructions such as CPUID.
> 
> To learn exactly how virtual CPU is presented to the guest machine via CPUID
> instruction, new QMP command can be used. By calling 'query-x86-cpuid'
> command, one can get a full listing of all CPUID leaves with subleaves which are
> supported by the initialized virtual CPU.
> 
> Other than debug, the command is useful in cases when we would like to
> utilize QEMU's virtual CPU initialization routines and put the retrieved
> values into kernel CPUID overriding mechanics for more precise control
> over how various processes perceive its underlying hardware with
> container processes as a good example.
> 
> The command is specific to x86. It is currenly only implemented for KVM acceleator.
> 
> Output format:
> The output is a plain list of leaf/subleaf argument combinations, that
> return 4 words in registers EAX, EBX, ECX, EDX.
> 
...

> +##
> +# @query-x86-cpuid:
> +#
> +# Returns raw data from the emulated CPUID table for the first VCPU.
> +# The emulated CPUID table defines the response to the CPUID
> +# instruction when executed by the guest operating system.
> +#
> +# Returns: a list of CpuidEntry
> +#
> +# Since: 6.1
> +##

Unfortunately too late for 6.1, so you'll have to update that to
"6.2" (also in @CpuidEntry).
Markus Armbruster Aug. 7, 2021, 2:22 p.m. UTC | #2
I'm afraid this needs a rebase now.  Reviewing anyway.

Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes:

> Introducing new QMP command 'query-x86-cpuid'. This command can be used to
> get virtualized cpu model info generated by QEMU during VM initialization in
> the form of cpuid representation.
>
> Diving into more details about virtual CPU generation: QEMU first parses '-cpu'
> command line option. From there it takes the name of the model as the basis for
> feature set of the new virtual CPU. After that it uses trailing '-cpu' options,
> that state if additional cpu features should be present on the virtual CPU or
> excluded from it (tokens '+'/'-' or '=on'/'=off').
> After that QEMU checks if the host's cpu can actually support the derived
> feature set and applies host limitations to it.
> After this initialization procedure, virtual CPU has it's model and
> vendor names, and a working feature set and is ready for identification
> instructions such as CPUID.
>
> To learn exactly how virtual CPU is presented to the guest machine via CPUID
> instruction, new QMP command can be used. By calling 'query-x86-cpuid'
> command, one can get a full listing of all CPUID leaves with subleaves which are
> supported by the initialized virtual CPU.
>
> Other than debug, the command is useful in cases when we would like to
> utilize QEMU's virtual CPU initialization routines and put the retrieved
> values into kernel CPUID overriding mechanics for more precise control
> over how various processes perceive its underlying hardware with
> container processes as a good example.
>
> The command is specific to x86. It is currenly only implemented for KVM acceleator.
>
> Output format:
> The output is a plain list of leaf/subleaf argument combinations, that
> return 4 words in registers EAX, EBX, ECX, EDX.
>
> Use example:
> qmp_request: {
>   "execute": "query-x86-cpuid"
> }
>
> qmp_response: {
>   "return": [
>     {
>       "eax": 1073741825,
>       "edx": 77,
>       "in-eax": 1073741824,
>       "ecx": 1447775574,
>       "ebx": 1263359563
>     },
>     {
>       "eax": 16777339,
>       "edx": 0,
>       "in-eax": 1073741825,
>       "ecx": 0,
>       "ebx": 0
>     },
>     {
>       "eax": 13,
>       "edx": 1231384169,
>       "in-eax": 0,
>       "ecx": 1818588270,
>       "ebx": 1970169159
>     },
>     {
>       "eax": 198354,
>       "edx": 126614527,
>       "in-eax": 1,
>       "ecx": 2176328193,
>       "ebx": 2048
>     },
>     ....
>     {
>       "eax": 12328,
>       "edx": 0,
>       "in-eax": 2147483656,
>       "ecx": 0,
>       "ebx": 0
>     }
>   ]
> }
>
> Signed-off-by: Valeriy Vdovin <valeriy.vdovin@virtuozzo.com>
> ---

[...]

> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index e7811654b7..db906c9240 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -329,3 +329,47 @@
>  ##
>  { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
>    'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
> +
> +##
> +# @CpuidEntry:
> +#
> +# A single entry of a CPUID response.
> +#
> +# One entry holds full set of information (leaf) returned to the guest
> +# in response to it calling a CPUID instruction with eax, ecx used as
> +# the agruments to that instruction. ecx is an optional argument as

Typo: arguments

> +# not all of the leaves support it.
> +#
> +# @in-eax: CPUID argument in eax
> +# @in-ecx: CPUID argument in ecx
> +# @eax: CPUID result in eax
> +# @ebx: CPUID result in ebx
> +# @ecx: CPUID result in ecx
> +# @edx: CPUID result in edx
> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'CpuidEntry',
> +  'data': { 'in-eax' : 'uint32',
> +            '*in-ecx' : 'uint32',
> +            'eax' : 'uint32',
> +            'ebx' : 'uint32',
> +            'ecx' : 'uint32',
> +            'edx' : 'uint32'
> +          },
> +  'if': 'defined(TARGET_I386)' }
> +
> +##
> +# @query-x86-cpuid:
> +#
> +# Returns raw data from the emulated CPUID table for the first VCPU.
> +# The emulated CPUID table defines the response to the CPUID
> +# instruction when executed by the guest operating system.
> +#
> +# Returns: a list of CpuidEntry
> +#
> +# Since: 6.1
> +##
> +{ 'command': 'query-x86-cpuid',
> +  'returns': ['CpuidEntry'],
> +  'if': 'defined(TARGET_I386)' }

I understand this fails when the acceleator isn't KVM.  I think that's
worth documenting.

Is this intended to be a stable interface?  Interfaces intended just for
debugging usually aren't.

Eduardo, what's your take on this version?
Eduardo Habkost Aug. 10, 2021, 6:56 p.m. UTC | #3
On Sat, Aug 07, 2021 at 04:22:42PM +0200, Markus Armbruster wrote:
> Is this intended to be a stable interface?  Interfaces intended just for
> debugging usually aren't.

I don't think we need to make it a stable interface, but I won't
mind if we declare it stable.

> 
> Eduardo, what's your take on this version?
> 

I sent my comments as reply to v14:
https://lore.kernel.org/qemu-devel/20210810185245.kivvmrmvew6e5xtr@habkost.net/
Thomas Huth Aug. 11, 2021, 6:09 a.m. UTC | #4
On 10/08/2021 20.56, Eduardo Habkost wrote:
> On Sat, Aug 07, 2021 at 04:22:42PM +0200, Markus Armbruster wrote:
>> Is this intended to be a stable interface?  Interfaces intended just for
>> debugging usually aren't.
> 
> I don't think we need to make it a stable interface, but I won't
> mind if we declare it stable.

If we don't feel 100% certain yet, it's maybe better to introduce this with 
a "x-" prefix first, isn't it? I.e. "x-query-x86-cpuid" ... then it's clear 
that this is only experimental/debugging/not-stable yet. Just my 0.02 €.

  Thomas
Eduardo Habkost Aug. 11, 2021, 1:40 p.m. UTC | #5
On Wed, Aug 11, 2021 at 2:10 AM Thomas Huth <thuth@redhat.com> wrote:
>
> On 10/08/2021 20.56, Eduardo Habkost wrote:
> > On Sat, Aug 07, 2021 at 04:22:42PM +0200, Markus Armbruster wrote:
> >> Is this intended to be a stable interface?  Interfaces intended just for
> >> debugging usually aren't.
> >
> > I don't think we need to make it a stable interface, but I won't
> > mind if we declare it stable.
>
> If we don't feel 100% certain yet, it's maybe better to introduce this with
> a "x-" prefix first, isn't it? I.e. "x-query-x86-cpuid" ... then it's clear
> that this is only experimental/debugging/not-stable yet. Just my 0.02 €.

That would be my expectation. Is this a documented policy?
Thomas Huth Aug. 11, 2021, 1:44 p.m. UTC | #6
On 11/08/2021 15.40, Eduardo Habkost wrote:
> On Wed, Aug 11, 2021 at 2:10 AM Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 10/08/2021 20.56, Eduardo Habkost wrote:
>>> On Sat, Aug 07, 2021 at 04:22:42PM +0200, Markus Armbruster wrote:
>>>> Is this intended to be a stable interface?  Interfaces intended just for
>>>> debugging usually aren't.
>>>
>>> I don't think we need to make it a stable interface, but I won't
>>> mind if we declare it stable.
>>
>> If we don't feel 100% certain yet, it's maybe better to introduce this with
>> a "x-" prefix first, isn't it? I.e. "x-query-x86-cpuid" ... then it's clear
>> that this is only experimental/debugging/not-stable yet. Just my 0.02 €.
> 
> That would be my expectation. Is this a documented policy?
> 

According to docs/interop/qmp-spec.txt :

  Any command or member name beginning with "x-" is deemed
  experimental, and may be withdrawn or changed in an incompatible
  manner in a future release.

  Thomas
Eduardo Habkost Aug. 11, 2021, 1:58 p.m. UTC | #7
On Wed, Aug 11, 2021 at 9:44 AM Thomas Huth <thuth@redhat.com> wrote:
>
> On 11/08/2021 15.40, Eduardo Habkost wrote:
> > On Wed, Aug 11, 2021 at 2:10 AM Thomas Huth <thuth@redhat.com> wrote:
> >>
> >> On 10/08/2021 20.56, Eduardo Habkost wrote:
> >>> On Sat, Aug 07, 2021 at 04:22:42PM +0200, Markus Armbruster wrote:
> >>>> Is this intended to be a stable interface?  Interfaces intended just for
> >>>> debugging usually aren't.
> >>>
> >>> I don't think we need to make it a stable interface, but I won't
> >>> mind if we declare it stable.
> >>
> >> If we don't feel 100% certain yet, it's maybe better to introduce this with
> >> a "x-" prefix first, isn't it? I.e. "x-query-x86-cpuid" ... then it's clear
> >> that this is only experimental/debugging/not-stable yet. Just my 0.02 €.
> >
> > That would be my expectation. Is this a documented policy?
> >
>
> According to docs/interop/qmp-spec.txt :
>
>   Any command or member name beginning with "x-" is deemed
>   experimental, and may be withdrawn or changed in an incompatible
>   manner in a future release.

Thanks! I had looked at other QMP docs, but not qmp-spec.txt.

In my reply above, please read "make it a stable interface" as
"declare it as supported by not using the 'x-' prefix".

I don't think we have to make it stable, but I won't argue against it
if the current proposal is deemed acceptable by other maintainers.

Personally, I'm still frustrated by the complexity of the current
proposal, but I don't want to block it just because of my frustration.
Markus Armbruster Aug. 23, 2021, 1:35 p.m. UTC | #8
Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, Aug 11, 2021 at 9:44 AM Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 11/08/2021 15.40, Eduardo Habkost wrote:
>> > On Wed, Aug 11, 2021 at 2:10 AM Thomas Huth <thuth@redhat.com> wrote:
>> >>
>> >> On 10/08/2021 20.56, Eduardo Habkost wrote:
>> >>> On Sat, Aug 07, 2021 at 04:22:42PM +0200, Markus Armbruster wrote:
>> >>>> Is this intended to be a stable interface?  Interfaces intended just for
>> >>>> debugging usually aren't.
>> >>>
>> >>> I don't think we need to make it a stable interface, but I won't
>> >>> mind if we declare it stable.
>> >>
>> >> If we don't feel 100% certain yet, it's maybe better to introduce this with
>> >> a "x-" prefix first, isn't it? I.e. "x-query-x86-cpuid" ... then it's clear
>> >> that this is only experimental/debugging/not-stable yet. Just my 0.02 €.
>> >
>> > That would be my expectation. Is this a documented policy?
>> >
>>
>> According to docs/interop/qmp-spec.txt :
>>
>>   Any command or member name beginning with "x-" is deemed
>>   experimental, and may be withdrawn or changed in an incompatible
>>   manner in a future release.
>
> Thanks! I had looked at other QMP docs, but not qmp-spec.txt.
>
> In my reply above, please read "make it a stable interface" as
> "declare it as supported by not using the 'x-' prefix".
>
> I don't think we have to make it stable, but I won't argue against it
> if the current proposal is deemed acceptable by other maintainers.
>
> Personally, I'm still frustrated by the complexity of the current
> proposal, but I don't want to block it just because of my frustration.

Is this a case of "there must be a simpler way", or did you actually
propose a simpler way?  I don't remember...
Eduardo Habkost Aug. 23, 2021, 3:55 p.m. UTC | #9
On Mon, Aug 23, 2021 at 9:35 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > On Wed, Aug 11, 2021 at 9:44 AM Thomas Huth <thuth@redhat.com> wrote:
> >>
> >> On 11/08/2021 15.40, Eduardo Habkost wrote:
> >> > On Wed, Aug 11, 2021 at 2:10 AM Thomas Huth <thuth@redhat.com> wrote:
> >> >>
> >> >> On 10/08/2021 20.56, Eduardo Habkost wrote:
> >> >>> On Sat, Aug 07, 2021 at 04:22:42PM +0200, Markus Armbruster wrote:
> >> >>>> Is this intended to be a stable interface?  Interfaces intended just for
> >> >>>> debugging usually aren't.
> >> >>>
> >> >>> I don't think we need to make it a stable interface, but I won't
> >> >>> mind if we declare it stable.
> >> >>
> >> >> If we don't feel 100% certain yet, it's maybe better to introduce this with
> >> >> a "x-" prefix first, isn't it? I.e. "x-query-x86-cpuid" ... then it's clear
> >> >> that this is only experimental/debugging/not-stable yet. Just my 0.02 €.
> >> >
> >> > That would be my expectation. Is this a documented policy?
> >> >
> >>
> >> According to docs/interop/qmp-spec.txt :
> >>
> >>   Any command or member name beginning with "x-" is deemed
> >>   experimental, and may be withdrawn or changed in an incompatible
> >>   manner in a future release.
> >
> > Thanks! I had looked at other QMP docs, but not qmp-spec.txt.
> >
> > In my reply above, please read "make it a stable interface" as
> > "declare it as supported by not using the 'x-' prefix".
> >
> > I don't think we have to make it stable, but I won't argue against it
> > if the current proposal is deemed acceptable by other maintainers.
> >
> > Personally, I'm still frustrated by the complexity of the current
> > proposal, but I don't want to block it just because of my frustration.
>
> Is this a case of "there must be a simpler way", or did you actually
> propose a simpler way?  I don't remember...
>

I did propose a simpler way at
https://lore.kernel.org/qemu-devel/20210810195053.6vsjadglrexf6jwy@habkost.net/

--
Eduardo
Markus Armbruster Aug. 24, 2021, 6:48 a.m. UTC | #10
Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, Aug 23, 2021 at 9:35 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>
>> > On Wed, Aug 11, 2021 at 9:44 AM Thomas Huth <thuth@redhat.com> wrote:
>> >>
>> >> On 11/08/2021 15.40, Eduardo Habkost wrote:
>> >> > On Wed, Aug 11, 2021 at 2:10 AM Thomas Huth <thuth@redhat.com> wrote:
>> >> >>
>> >> >> On 10/08/2021 20.56, Eduardo Habkost wrote:
>> >> >>> On Sat, Aug 07, 2021 at 04:22:42PM +0200, Markus Armbruster wrote:
>> >> >>>> Is this intended to be a stable interface?  Interfaces intended just for
>> >> >>>> debugging usually aren't.
>> >> >>>
>> >> >>> I don't think we need to make it a stable interface, but I won't
>> >> >>> mind if we declare it stable.
>> >> >>
>> >> >> If we don't feel 100% certain yet, it's maybe better to introduce this with
>> >> >> a "x-" prefix first, isn't it? I.e. "x-query-x86-cpuid" ... then it's clear
>> >> >> that this is only experimental/debugging/not-stable yet. Just my 0.02 €.
>> >> >
>> >> > That would be my expectation. Is this a documented policy?
>> >> >
>> >>
>> >> According to docs/interop/qmp-spec.txt :
>> >>
>> >>   Any command or member name beginning with "x-" is deemed
>> >>   experimental, and may be withdrawn or changed in an incompatible
>> >>   manner in a future release.
>> >
>> > Thanks! I had looked at other QMP docs, but not qmp-spec.txt.
>> >
>> > In my reply above, please read "make it a stable interface" as
>> > "declare it as supported by not using the 'x-' prefix".
>> >
>> > I don't think we have to make it stable, but I won't argue against it
>> > if the current proposal is deemed acceptable by other maintainers.
>> >
>> > Personally, I'm still frustrated by the complexity of the current
>> > proposal, but I don't want to block it just because of my frustration.
>>
>> Is this a case of "there must be a simpler way", or did you actually
>> propose a simpler way?  I don't remember...
>>
>
> I did propose a simpler way at
> https://lore.kernel.org/qemu-devel/20210810195053.6vsjadglrexf6jwy@habkost.net/

Valeriy, would the simpler way still work for you?

If no, please explain why.  If you already did, just provide a pointer.

If yes, we need to choose between the complex solution we have and the
simpler solution we still need to code up.  The latter is extra work,
but having to carry more complex code is going to be extra work, too.
Valeriy Vdovin Sept. 3, 2021, 9:33 a.m. UTC | #11
On Tue, 24 Aug 2021 08:48:31 +0200 Marcus Armbruster <armbru@redhat.com> wrote:

>Eduardo Habkost <ehabkost@redhat.com> writes:
>
>> On Mon, Aug 23, 2021 at 9:35 AM Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>>
>>> > On Wed, Aug 11, 2021 at 9:44 AM Thomas Huth <thuth@redhat.com> wrote:
>>> >>
>>> >> On 11/08/2021 15.40, Eduardo Habkost wrote:
>>> >> > On Wed, Aug 11, 2021 at 2:10 AM Thomas Huth <thuth@redhat.com> wrote:
>>> >> >>
>>> >> >> On 10/08/2021 20.56, Eduardo Habkost wrote:
>>> >> >>> On Sat, Aug 07, 2021 at 04:22:42PM +0200, Markus Armbruster wrote:
>>> >> >>>> Is this intended to be a stable interface?  Interfaces intended just
>>> >> >>>> for
>>> >> >>>> debugging usually aren't.
>>> >> >>>
>>> >> >>> I don't think we need to make it a stable interface, but I won't
>>> >> >>> mind if we declare it stable.
>>> >> >>
>>> >> >> If we don't feel 100% certain yet, it's maybe better to introduce this
>>> >> >> with
>>> >> >> a "x-" prefix first, isn't it? I.e. "x-query-x86-cpuid" ... then it's
>>> >> >> clear
>>> >> >> that this is only experimental/debugging/not-stable yet. Just my 0.02
>>> >> >> €.
>>> >> >
>>> >> > That would be my expectation. Is this a documented policy?
>>> >> >
>>> >>
>>> >> According to docs/interop/qmp-spec.txt :
>>> >>
>>> >>   Any command or member name beginning with "x-" is deemed
>>> >>   experimental, and may be withdrawn or changed in an incompatible
>>> >>   manner in a future release.
>>> >
>>> > Thanks! I had looked at other QMP docs, but not qmp-spec.txt.
>>> >
>>> > In my reply above, please read "make it a stable interface" as
>>> > "declare it as supported by not using the 'x-' prefix".
>>> >
>>> > I don't think we have to make it stable, but I won't argue against it
>>> > if the current proposal is deemed acceptable by other maintainers.
>>> >
>>> > Personally, I'm still frustrated by the complexity of the current
>>> > proposal, but I don't want to block it just because of my frustration.
>>>
>>> Is this a case of "there must be a simpler way", or did you actually
>>> propose a simpler way?  I don't remember...
>>>
>>
>> I did propose a simpler way at
>> 20210810195053.6vsjadglrexf6jwy@habkost.net/">https://lore.kernel.org/qemu-devel/20210810195053.6vsjadglrexf6jwy@habkost.net/
>

Hi. Sorry for the late response to that.

Also sorry for possible technical email header errors in my reply mail, as I was switching the e-mail accounts that I use to
communicate with this maillist, so hope technically all went well.

>Valeriy, would the simpler way still work for you?
>
>If no, please explain why.  If you already did, just provide a pointer.
>
Yes, I remember your proposal of using just 5 lines of code. To be exact here are
those proposed lines:

>>    for start in (0, 0x40000000, 0x80000000, 0xC0000000):
>>        leaf = query_cpuid(qom_path, start)
>>        for eax in range(start, leaf.max_eax + 1):
>>            for ecx in range(0, leaf.get('max_ecx', 0) + 1):
>>                all_leaves.append(query_cpuid(qom_path, eax, ecx))


It looks cool and short, but this is only a pseudocode with not only variable declarations omitted, but
with some logic omitted as well. It does not become obvious until you start typing the code and then review it.
In fact the patch, to which you have done this suggestion back then already had the same concept at it's basis, but
it has grown quickly to somewhat more complex code than it's conceptual pseudo-brother above.

I'm sure that this current patch (which is the most recent in v15 email) is the most simple and shortest of possible.
This is iteration 3 patch, with first iteration being the one to which you've made that suggestion, but then we also tried one
more version by trying to do this via KVM ioctls, but it did not work quite smooth. So this last iteration at which we are currently
looking at is really the product of thought and is the simplest.

I suggest that we stick to it and start converging towards it's submission instead of going to another round of coding and discussion.
v15 - is the result of fine-tunes and rebases, that has already covered a lot of comments. Please let's review it to the end and give it a go.


>If yes, we need to choose between the complex solution we have and the
>simpler solution we still need to code up.  The latter is extra work,
>but having to carry more complex code is going to be extra work, too.

I agree to the idea that we MUST minimize support effort in priority to the commiting effort, but here I do not see direct dependency
between the two. This is already the simplest solution. All the code we have here is mostly
to service the QMP machinery, which has to be in any version of the patch. The payload code is minimal.

Thanks.
diff mbox series

Patch

diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index e7811654b7..db906c9240 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -329,3 +329,47 @@ 
 ##
 { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
   'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
+
+##
+# @CpuidEntry:
+#
+# A single entry of a CPUID response.
+#
+# One entry holds full set of information (leaf) returned to the guest
+# in response to it calling a CPUID instruction with eax, ecx used as
+# the agruments to that instruction. ecx is an optional argument as
+# not all of the leaves support it.
+#
+# @in-eax: CPUID argument in eax
+# @in-ecx: CPUID argument in ecx
+# @eax: CPUID result in eax
+# @ebx: CPUID result in ebx
+# @ecx: CPUID result in ecx
+# @edx: CPUID result in edx
+#
+# Since: 6.1
+##
+{ 'struct': 'CpuidEntry',
+  'data': { 'in-eax' : 'uint32',
+            '*in-ecx' : 'uint32',
+            'eax' : 'uint32',
+            'ebx' : 'uint32',
+            'ecx' : 'uint32',
+            'edx' : 'uint32'
+          },
+  'if': 'defined(TARGET_I386)' }
+
+##
+# @query-x86-cpuid:
+#
+# Returns raw data from the emulated CPUID table for the first VCPU.
+# The emulated CPUID table defines the response to the CPUID
+# instruction when executed by the guest operating system.
+#
+# Returns: a list of CpuidEntry
+#
+# Since: 6.1
+##
+{ 'command': 'query-x86-cpuid',
+  'returns': ['CpuidEntry'],
+  'if': 'defined(TARGET_I386)' }
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index a7ee431187..74fa6b9af4 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -128,7 +128,7 @@  void hw_error(const char *fmt, ...)
 /*
  * The chosen accelerator is supposed to register this.
  */
-static const AccelOpsClass *cpus_accel;
+const AccelOpsClass *cpus_accel;
 
 void cpu_synchronize_all_states(void)
 {
diff --git a/target/i386/kvm/kvm-stub.c b/target/i386/kvm/kvm-stub.c
index 92f49121b8..27305fc458 100644
--- a/target/i386/kvm/kvm-stub.c
+++ b/target/i386/kvm/kvm-stub.c
@@ -12,6 +12,7 @@ 
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "kvm_i386.h"
+#include "qapi/error.h"
 
 #ifndef __OPTIMIZE__
 bool kvm_has_smm(void)
@@ -39,3 +40,12 @@  bool kvm_hv_vpindex_settable(void)
 {
     return false;
 }
+
+typedef struct CpuidEntryList CpuidEntryList;
+CpuidEntryList *qmp_query_x86_cpuid(Error **errp);
+
+CpuidEntryList *qmp_query_x86_cpuid(Error **errp)
+{
+    error_setg(errp, "Not implemented in --disable-kvm configuration");
+    return NULL;
+}
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 7fe9f52710..114ed76493 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -20,10 +20,12 @@ 
 
 #include <linux/kvm.h>
 #include "standard-headers/asm-x86/kvm_para.h"
+#include "qapi/qapi-commands-machine-target.h"
 
 #include "cpu.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/hw_accel.h"
+#include "sysemu/accel-ops.h"
 #include "sysemu/kvm_int.h"
 #include "sysemu/runstate.h"
 #include "kvm_i386.h"
@@ -1464,6 +1466,51 @@  static Error *invtsc_mig_blocker;
 
 #define KVM_MAX_CPUID_ENTRIES  100
 
+struct kvm_cpuid2 *cpuid_data_cached;
+extern const AccelOpsClass *cpus_accel;
+
+static inline int is_kvm_accel(AccelOpsClass *class)
+{
+    ObjectClass *parent_class;
+
+    parent_class = &class->parent_class;
+    return strcmp(object_class_get_name(parent_class),
+        "kvm-accel-ops") == 0;
+}
+
+CpuidEntryList *qmp_query_x86_cpuid(Error **errp)
+{
+    int i;
+    struct kvm_cpuid_entry2 *kvm_entry;
+    CpuidEntryList *head = NULL, **tail = &head;
+    CpuidEntry *entry;
+
+    if (!cpuid_data_cached) {
+         if (cpus_accel && !is_kvm_accel((AccelOpsClass *)cpus_accel))
+             error_setg(errp, "Not implemented for non-kvm accel");
+         else
+             error_setg(errp, "VCPU was not initialized yet");
+         return NULL;
+    }
+
+    for (i = 0; i < cpuid_data_cached->nent; ++i) {
+        kvm_entry = &cpuid_data_cached->entries[i];
+        entry = g_malloc0(sizeof(*entry));
+        entry->in_eax = kvm_entry->function;
+        if (kvm_entry->flags & KVM_CPUID_FLAG_SIGNIFCANT_INDEX) {
+            entry->in_ecx = kvm_entry->index;
+            entry->has_in_ecx = true;
+        }
+        entry->eax = kvm_entry->eax;
+        entry->ebx = kvm_entry->ebx;
+        entry->ecx = kvm_entry->ecx;
+        entry->edx = kvm_entry->edx;
+        QAPI_LIST_APPEND(tail, entry);
+    }
+
+    return head;
+}
+
 int kvm_arch_init_vcpu(CPUState *cs)
 {
     struct {
@@ -1833,6 +1880,10 @@  int kvm_arch_init_vcpu(CPUState *cs)
     if (r) {
         goto fail;
     }
+    if (!cpuid_data_cached) {
+        cpuid_data_cached = g_malloc0(sizeof(cpuid_data));
+        memcpy(cpuid_data_cached, &cpuid_data, sizeof(cpuid_data));
+    }
 
     if (has_xsave) {
         env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index c98b78d033..bd883f7f52 100644
--- a/tests/qtest/qmp-cmd-test.c
+++ b/tests/qtest/qmp-cmd-test.c
@@ -46,6 +46,7 @@  static int query_error_class(const char *cmd)
         { "query-balloon", ERROR_CLASS_DEVICE_NOT_ACTIVE },
         { "query-hotpluggable-cpus", ERROR_CLASS_GENERIC_ERROR },
         { "query-vm-generation-id", ERROR_CLASS_GENERIC_ERROR },
+        { "query-x86-cpuid", ERROR_CLASS_GENERIC_ERROR },
         { NULL, -1 }
     };
     int i;