On 11/14/18 8:24 AM, Vitaly Kuznetsov wrote: > John Ferlan <jferlan@xxxxxxxxxx> writes: > >> On 11/14/18 4:04 AM, Andrea Bolognani wrote: >>> On Tue, 2018-11-13 at 18:53 +0100, Vitaly Kuznetsov wrote: >>>> The upcoming Qemu-3.1 release will bring us two new Hyper-V enlightenments: >>>> hv_ipi and hv_evmcs. Support these in libvirt. >>>> >>>> Vitaly Kuznetsov (2): >>>> conf: qemu: add support for Hyper-V PV IPIs >>>> conf: qemu: add support for Hyper-V Enlightened VMCS >>> >>> I have pointed out a few very minor issues with the patches, as >>> well as a couple areas where pre-existing issues could be fixed, >>> although I don't consider the latter a requirement for merging >>> the series ;) >>> >>> One thing that I would like to see, though, is the change being >>> documented properly in the release notes (docs/news.xml). Please >>> include that and respin. >>> >> >> One other thing that I've really tried to see done is splitting the >> "conf" and "qemu" patches. >> >> That way one deals with conf, docs, conf, util, xml2xmltest, while the >> other deals with the qemu specific changes qemu, xml2argv. >> >> Sometimes it's "harder" to do that - such as may be the case with >> various switches when a new case is added; however, in that case the >> added case in the conf/docs/etc patch would go with the "default:" or >> "last" case and then be moved with the subsequent qemu patch. >> > > I have to admit my overall knowledge of libvirt pretty limited, probably > because of that I'm failing to see benefits of doing such split for > Qemu/KVM-only features (as the whole Hyper-V emulation is Generally speaking it's to keep conf/docs/XML changes separate from hypervisor specific to make bisecting easier and patches smaller for reviews. I see I didn't ask the last time (commit f4c39db736 for PV TLB flush support in Aug 2018), so either I had a lapse there or I was just feeling it wasn't that important since things are so tightly coupled. I look at a lot of patches and just try to be consistent in asks. > Qemu/KVM-only). How is the first patch going to be tested? Compile-only? Since you've updated tests/qemuxml2xmloutdata/hyperv.xml (and the output). That means the XML parsing is being tested. So that's testing that the newly added XML/RNG field can be parsed and output as expected. There are cases I've reviewed where the formatting was wrong, but there was no xml2xml to test that. > Also, which of these patches should src/cpu/* hunks go to? I would think the conf/docs/xml2xml since it seems they are defining supported bits. > > Would realy appreciate some guidance here) Thanks! > In the long run it may not matter much - I'm not here to NAK something because the split isn't done. Again, I'm trying to be consistent in how I look at things. John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
