Re: [Qemu-devel] [PATCH] i386/kvm: expose HV_CPUID_ENLIGHTMENT_INFO.EAX and HV_CPUID_NESTED_FEATURES.EAX as feature words

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Nov 29, 2018 at 12:51:55PM +0100, Vitaly Kuznetsov wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>> > On 26/11/18 14:59, Vitaly Kuznetsov wrote:
>> >> It was found that QMP users of QEMU (e.g. libvirt) may need
>> >> HV_CPUID_ENLIGHTMENT_INFO.EAX/HV_CPUID_NESTED_FEATURES.EAX information. In
>> >> particular, 'hv_tlbflush' and 'hv_evmcs' enlightenments are only exposed in
>> >> HV_CPUID_ENLIGHTMENT_INFO.EAX.
>> >> 
>> >> HV_CPUID_NESTED_FEATURES.EAX is exposed for two reasons: convenience
>> >> (we don't need to export it from hyperv_handle_properties() and as
>> >> future-proof for Enlightened MSR-Bitmap, PV EPT invalidation and
>> >> direct virtual flush features.
>> >> 
>> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> >
>> > Can you add a comment to feature_word_info, explaining why the
>> > feat_names are not set?
>> 
>> I had to do some code archeology to make sure I understand, I think it
>> goes back to
>> 
>> http://lists.gnu.org/archive/html/qemu-devel/2016-06/msg06579.html
>> 
>> So the comment (probably added before FEAT_HYPERV_EAX definition) would
>> be
>> 
>> ".feat_names are commented out for Hyper-V enlightenments because we
>> don't want to have two different ways for enabling them on QEMU command
>> line. Some features (e.g. "hyperv_time", "hyperv_vapic", ...) require
>> enabling several feature bits simultaneously, exposing these bits
>> individually may just confuse guests."
>> 
>> Would do?
>
> That's an accurate description.
>

Thanks, I'll send v2 out with it.

> But note that we might still be able to move the existing
> "hyperv_*" features to feature_word_info[].feat_names.  We just
> need to keep the same semantics (e.g. enable
> HV_HYPERCALL_AVAILABLE automatically when some features are set).
>
> Maybe we can make some of the feature properties read-only.  This
> way we can give them meaningful names for debugging and error
> messages, even if we don't want to make them configurable
> directly.

I'd suggest (if there are no objections of course) we do this separately
from this patch. Some time ago when merging direct mode stimers for KVM
Paolo suggested we stop adding capabilities to KVM for each individulat
feature and replace them with something like KVM_GET_SUPPORTED_HV_CPUID
ioctl returning all Hyper-V related feature words. When this is done we
can reconsider how Qemu discoveres Hyper-V related KVM features and as
part of this work we can take a closer look at feature words and
feat_names.

-- 
Vitaly