Re: [Qemu-devel] [PATCH] Introduce query-cpu-max QMP command and cpu_max HMP counterpart

[Michal Novotny <minovotn@redhat.com>]

On 03/19/2013 02:27 PM, Andreas Färber wrote:
> Am 19.03.2013 13:28, schrieb Markus Armbruster:
>> Please cc: Luiz and me on QMP work in the future.
>>
>> Michal Novotny <minovotn@redhat.com> writes:
>>
>>> This is the patch to introduce the query-cpu-max QMP command to get
>>> the maximum number of CPUs supported by the currently running emulator
>>> instance. This may differ machine from machine as defined by -machine
>>> settings and max_cpus member of QEMUMachine structure.
>> Humor me: don't start commit message bodies with "This patch" or
>> variations thereof, and don't repeat the subject.  Suggest:
>>
>> QMP command query-cpu-max returns the maximum number of CPUs supported
>> by the currently running emulator instance, as defined in its
>> QEMUMachine struct.
>>
>>> It's been tested both using QMP/qmp utility and telnet session on
>>> the QEMU session.
>> What's a QMP/qmp utility?
> Our ./QMP/qmp script, I guess.
>
>>  Let's drop this sentence.  In the future,
>> feel free to put testing info below the "---" line.
>>
>>> The HMP counterpart called cpu_max has been introduced by this patch
>>> too.
>> Grammar nit: s/has been/is/.  Even better, avoid passive voice.
>>
>> Hmm, I just rewrote most of your commit message, so why not rewrite all
>> of it:
>>
>>     New QMP command query-cpu-max and HMP command cpu_max
>>
>>     These commands return the maximum number of CPUs supported by the
>>     currently running emulator instance, as defined in its QEMUMachine
>>     struct.
>>
>> Perhaps Luiz can fix up the commit message commit, if you don't mind.
>>
>> Patch looks good.
>>
>> Should query commands for machine properties multiply, we should
>> consider creating a single command returning all of them.  I'm not
>> asking you to do that now.
>>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> From my CPU perspective I am wondering if this info has future as-is?
> IIUC this QEMUMachine value differs from the value exposed to
> SeaBIOS/KVM already since recently, as it ignores CPU topology.
>
> I'm guessing the libvirt use case is just to detect users specifying too
> many -cpu options, so I won't veto this, but want to caution that we may
> need to change this as we proceed with CPU remodelling.

Yeah, this patch was written for libvirt as one of the libvirt
developers was not happy qemu doesn't support that yet.

Michal
>
> Regards,
> Andreas
>

-- 
Michal Novotny <minovotn@redhat.com>, RHCE, Red Hat
Virtualization | libvirt-php bindings | php-virt-control.org