On Thu, May 27, 2021 at 09:37:59AM +0200, Vitaly Kuznetsov wrote: > Eduardo Habkost <ehabk...@redhat.com> writes: > > > On Mon, May 24, 2021 at 02:22:47PM +0200, Vitaly Kuznetsov wrote: > >> Eduardo Habkost <ehabk...@redhat.com> writes: > >> > >> > On Thu, Apr 22, 2021 at 06:11:28PM +0200, Vitaly Kuznetsov wrote: > >> >> According to TLFS, Hyper-V guest is supposed to check > >> >> HV_HYPERCALL_AVAILABLE privilege bit before accessing > >> >> HV_X64_MSR_GUEST_OS_ID/HV_X64_MSR_HYPERCALL MSRs but at least some > >> >> Windows versions ignore that. As KVM is very permissive and allows > >> >> accessing these MSRs unconditionally, no issue is observed. We may, > >> >> however, want to tighten the checks eventually. Conforming to the > >> >> spec is probably also a good idea. > >> >> > >> >> Add HV_HYPERCALL_AVAILABLE to all 'leaf' features with no dependencies. > >> >> > >> >> Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com> > >> > > >> > Are all VMs being created with HV_HYPERCALL_AVAILABLE unset, > >> > today? > >> > > >> > >> No, we have HV_HYPERCALL_AVAILABLE encoded in 'hv-relaxed','hv-vapic' > >> and 'hv-time' features but not > >> > >> > >> > Wouldn't it be simpler to simply add a new > >> > HYPERV_FEAT_HYPERCALL_AVAILABLE bit to hyperv_features, and > >> > enabling it by default? > >> > > >> > >> We could do that but as I note above, we already have it for three > >> features. > > > > Do we have any cases where we do not want to enable > > HV_HYPERCALL_AVAILABLE? > > > > Would it be OK to just hardcoded it in hyperv_fill_cpuids() like > > we do with HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE? > > > > struct kvm_hyperv_properties[] serves two purposes: > 1) Set corresponding guest visible CPUID bits when certain features are > enabled. > > 2) Check, that KVM supports certain features before we expose them to the > guest.
Oh, you're right. > > Whatever we hardcode in hyperv_fill_cpuids() gives us 1) but not 2). For > this particular bit it probably doesn't matter as even the oldest > supported kernel (v4.5) has it. That said, I'm OK with moving this to > hyperv_fill_cpuids(). I'm only worried about the risk of somebody forgetting to hardcode the HV_HYPERCALL_AVAILABLE bit in new kvm_hyperv_expand_features[] entries in the future. A new HYPERV_FEAT_HYPERCALL_AVAILABLE bit (hardcoded to 1 at kvm_hyperv_expand_features()) would give us feature checking. But if you're OK with hardcoding it at hyperv_fill_cpuids(), it's probably the simplest solution. > [...] -- Eduardo