Re: [libvirt] [PATCH 0/2] conf: qemu: support new Hyper-V enlightenments in Qemu-3.1
On 11/14/18 8:24 AM, Vitaly Kuznetsov wrote:
John Ferlan <jferlan(a)redhat.com> 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