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

[Valeriy Vdovin]

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.