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