Re: [PATCH v6 17/19] i386: HV_HYPERCALL_AVAILABLE privilege bit is always needed

[Eduardo Habkost]

On Thu, May 27, 2021 at 09:37:59AM +0200, Vitaly Kuznetsov wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Mon, May 24, 2021 at 02:22:47PM +0200, Vitaly Kuznetsov wrote:
> >> Eduardo Habkost <ehabkost@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 <vkuznets@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