[libvirt] [PATCH v4 00/37] BaselineHypervisorCPU using QEMU QMP exchanges

Fri Nov 9 16:44:41 UTC 2018

Hi Chris,

On 11/9/18 11:27 AM, Chris Venteicher wrote:
> Quoting Chris Venteicher (2018-11-02 22:13:01)
>> Some architectures (S390) depend on QEMU to compute baseline CPU model and
>> expand a models feature set.
>>
>> Interacting with QEMU requires starting the QEMU process and completing one or
>> more query-cpu-model-baseline QMP exchanges with QEMU in addition to a
>> query-cpu-model-expansion QMP exchange to expand all features in the model.
>>
>> See "s390x CPU models: exposing features" patch set on Qemu-devel for discussion
>> of QEMU aspects.
>>
>> This is part of resolution of: https://bugzilla.redhat.com/show_bug.cgi?id=1511999
>>
>> Version 4 attempts to address all issues from V1,2,3 including making the
>> QEMU process activation for QMP Queries generic (not specific to capabilities)
>> and moving that code from qemu_capabilities to qemu_process.
>>
>> Version 4 greatly expands the number of patches to make the set easier
>> to review.
>>
>> The patches to do hypervisor baseline using QEMU are primarily in
>> qemu_driver.c
>>
> Patches 1-2 create the cpumodel to/from Json conversion code.
> Patches 3-4 modify the cpu expansion interface.
> Patches 28-36 are the actual baseline changes.
> 
>> The patches to make the process code generic (not capabilities specific)
>> are mostly in qemu_process.c
> 
> Patches 5 -> 27 move the process code from qemu_capabilities to
> qemu_process.
> 
> A lot of these are code move or rename patches so the patches with real
> implementation changes are easy to identify.
> 
> I might have ended up with too many patches though.
> Want to mention... the whole "process" block could be moved to it's own
> series if that would be easier to review.

I've been meaning to provide an in-depth review of these patches, but other things
have been holding me up -- my apologies.

At a quick glance, a lot of your patches involve a few short patches that don't
necessarily provide much context on their own. A lot of patches in this series can
definitely be merged.

I think moving the "qemu process" code into its own series would be helpful. I would
include something in the cover that they will benefit the hypervisor CPU baseline and 
comparison patches. If you feel confident with your qemu process patches, then you 
could also send your baseline patch series at the same time, and state that your 
baseline patches rely on your QEMU process patches (make sure to include a mailing 
list archive link in that header so reviewers can easily reference the other patch 
series).

Also, make sure you CC the authors and contributors of the respective files that you
touch. Most of them are listed at the top of the file. (I think Jiri might be interested
in this series as well.)

So let's start there. Repost your qemu_process code as it's own series, and I'd recommend
tagging it with "RFC" (Request For Comments) instead of giving it a version number. This
will prompt reviewers that you're mainly looking for areas of improvement and some guidance.

> 
>> Many of the patches are cut / paste of existing code.  The patches that
>> change functionality are as modular and minimal as possible to make
>> reviewing easier.
>>
>> I attempted to make the new qemu_process functions
>> consistent with the existing domain activation qemu_process functions.
>>
>> A thread safe library function creates a unique directory under libDir for each QEMU
>> process (for QMP messaging) to decouple processes in terms of sockets and
>> file system footprint.
>>
>> The last patch is based on past discussion of QEMU cpu
>> expansion only returning migratable features except for one x86 case where
>> non-migratable features are explicitly requested.  The patch records that features
>> are migratable based on QEMU only returning migratable features.  The main result
>> is the CPU xml for S390 CPU's contains the same migratability info the X86 currently
>> does.  The testcases were updated to reflect the change.
>>
>> Every patch should compile independently if applied in sequence.
>>
>> Thanks,
>> Chris
>>
>>
>> Chris Venteicher (37):
>>   qemu_monitor: Introduce qemuMonitorCPUModelInfoNew
>>   qemu_monitor: Introduce qemuMonitorCPUModelInfo / JSON conversion
>>   qemu_capabilities: Introduce virQEMuCapsMigratablePropsDiff
>>   qemu_monitor: qemuMonitorGetCPUModelExpansion inputs and outputs
>>     CPUModelInfo
>>   qemu_process: Move process code from qemu_capabilities to qemu_process
>>   qemu_process: Use qemuProcess prefix
>>   qemu_process: Limit qemuProcessNew to const input strings
>>   qemu_process: Refer to proc not cmd in process code
>>   qemu_process: Use consistent name for stop process function
>>   qemu_capabilities: Stop QEMU process before freeing
>>   qemu_process: Use qemuProcess struct for a single process
>>   qemu_process: Persist stderr in qemuProcess struct
>>   qemu_capabilities: Detect caps probe failure by checking monitor ptr
>>   qemu_process: Introduce qemuProcessStartQmp
>>   qemu_process: Add debug message in qemuProcessLaunchQmp
>>   qemu_process: Collect monitor code in single function
>>   qemu_process: Store libDir in qemuProcess struct
>>   qemu_process: Setup paths within qemuProcessInitQmp
>>   qemu_process: Stop retaining Qemu Monitor config in qemuProcess
>>   qemu_process: Don't open monitor if process failed
>>   qemu_process: Cleanup qemuProcess alloc function
>>   qemu_process: Cleanup qemuProcessStopQmp function
>>   qemu_process: Catch process free before process stop
>>   qemu_monitor: Make monitor callbacks optional
>>   qemu_process: Enter QMP command mode when starting QEMU Process
>>   qemu_process: Use unique directories for QMP processes
>>   qemu_process: Stop locking QMP process monitor immediately
>>   qemu_capabilities: Introduce CPUModelInfo to virCPUDef function
>>   qemu_capabilities: Introduce virCPUDef to CPUModelInfo function
>>   qemu_monitor: Support query-cpu-model-baseline QMP command
>>   qemu_driver: Consolidate code to baseline using libvirt
>>   qemu_driver: Decouple code for baseline using libvirt
>>   qemu_driver: Identify using libvirt as a distinct way to compute
>>     baseline
>>   qemu_driver: Support baseline calculation using QEMU
>>   qemu_driver: Support feature expansion via QEMU when baselining cpu
>>   qemu_driver: Remove unsupported props in expanded hypervisor baseline
>>     output
>>   qemu_monitor: Default props to migratable when expanding cpu model
>>
>>  src/qemu/qemu_capabilities.c                  | 567 ++++++++----------
>>  src/qemu/qemu_capabilities.h                  |   4 +
>>  src/qemu/qemu_driver.c                        | 219 ++++++-
>>  src/qemu/qemu_monitor.c                       | 184 +++++-
>>  src/qemu/qemu_monitor.h                       |  29 +-
>>  src/qemu/qemu_monitor_json.c                  | 226 +++++--
>>  src/qemu/qemu_monitor_json.h                  |  12 +-
>>  src/qemu/qemu_process.c                       | 359 +++++++++++
>>  src/qemu/qemu_process.h                       |  37 ++
>>  tests/cputest.c                               |  11 +-
>>  .../caps_2.10.0.s390x.xml                     |  60 +-
>>  .../caps_2.11.0.s390x.xml                     |  58 +-
>>  .../caps_2.12.0.s390x.xml                     |  56 +-
>>  .../qemucapabilitiesdata/caps_2.8.0.s390x.xml |  32 +-
>>  .../qemucapabilitiesdata/caps_2.9.0.s390x.xml |  34 +-
>>  .../qemucapabilitiesdata/caps_3.0.0.s390x.xml |  64 +-
>>  tests/qemucapabilitiestest.c                  |   7 +
>>  17 files changed, 1388 insertions(+), 571 deletions(-)
>>
>> -- 
>> 2.17.1
>>
> 

-- 
Respectfully,
- Collin Walling