[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



Quoting Collin Walling (2018-11-09 10:44:41)
> 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.
> 
Thanks Collin,

The process stuff has been resubmitted in this patch series:
  [PATCH RFC 00/22] Move process code to qemu_process

The rest of this series can be resubmitted when the process changes are
solid.

> > 
> >> 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
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]