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
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list