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

[Eduardo Habkost]

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?

> 
> 
> > We don't necessarily need to make it configurable by the user,
> > but probably it would be a good idea to keep the bit unset by
> > default on older machine types.  Even if guests don't mind seeing
> > the bit changing under their feet, it would make it easier for
> > automated test cases that check for unexpected changes in raw
> > CPUID data.
> 
> I see current situation as a bug. While most likely nobody runs with
> a configuration like 'hv-vpindexem,hv-synic' it is still valid. And if KVM
> was enforcing the features (not yet), Windows would've just crashed in
> early boot. Normal configurations will likely always include at least
> 'hv-time' which has HYPERV_FEAT_HYPERCALL_AVAILABLE enabled.
> 
> That being said, I'm not sure we need to maintain 'bug compatibility'
> even for older machine types. I'm also not aware of any specific tests
> for such 'crazy' configurations out there. The last patch of the series
> adds a very simple test to qtest but this is about it.

If you are 100% sure the CPUID change can't crash or confuse a
guest, then that's OK.  I agree that bug compatibility is not a
must if the bit is simply ignored by most guests and by KVM
emulation code.


> 
> >
> >
> >> ---
> >>  target/i386/kvm/kvm.c | 15 +++++++++------
> >>  1 file changed, 9 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> >> index 2c1a77f9b00f..d81451276cd8 100644
> >> --- a/target/i386/kvm/kvm.c
> >> +++ b/target/i386/kvm/kvm.c
> >> @@ -835,6 +835,8 @@ static struct {
> >>      [HYPERV_FEAT_CRASH] = {
> >>          .desc = "crash MSRs (hv-crash)",
> >>          .flags = {
> >> +            {.func = HV_CPUID_FEATURES, .reg = R_EAX,
> >> +             .bits = HV_HYPERCALL_AVAILABLE},
> >>              {.func = HV_CPUID_FEATURES, .reg = R_EDX,
> >>               .bits = HV_GUEST_CRASH_MSR_AVAILABLE}
> >>          }
> >> @@ -843,28 +845,28 @@ static struct {
> >>          .desc = "reset MSR (hv-reset)",
> >>          .flags = {
> >>              {.func = HV_CPUID_FEATURES, .reg = R_EAX,
> >> -             .bits = HV_RESET_AVAILABLE}
> >> +             .bits = HV_HYPERCALL_AVAILABLE | HV_RESET_AVAILABLE}
> >>          }
> >>      },
> >>      [HYPERV_FEAT_VPINDEX] = {
> >>          .desc = "VP_INDEX MSR (hv-vpindex)",
> >>          .flags = {
> >>              {.func = HV_CPUID_FEATURES, .reg = R_EAX,
> >> -             .bits = HV_VP_INDEX_AVAILABLE}
> >> +             .bits = HV_HYPERCALL_AVAILABLE | HV_VP_INDEX_AVAILABLE}
> >>          }
> >>      },
> >>      [HYPERV_FEAT_RUNTIME] = {
> >>          .desc = "VP_RUNTIME MSR (hv-runtime)",
> >>          .flags = {
> >>              {.func = HV_CPUID_FEATURES, .reg = R_EAX,
> >> -             .bits = HV_VP_RUNTIME_AVAILABLE}
> >> +             .bits = HV_HYPERCALL_AVAILABLE | HV_VP_RUNTIME_AVAILABLE}
> >>          }
> >>      },
> >>      [HYPERV_FEAT_SYNIC] = {
> >>          .desc = "synthetic interrupt controller (hv-synic)",
> >>          .flags = {
> >>              {.func = HV_CPUID_FEATURES, .reg = R_EAX,
> >> -             .bits = HV_SYNIC_AVAILABLE}
> >> +             .bits = HV_HYPERCALL_AVAILABLE | HV_SYNIC_AVAILABLE}
> >>          }
> >>      },
> >>      [HYPERV_FEAT_STIMER] = {
> >> @@ -879,7 +881,7 @@ static struct {
> >>          .desc = "frequency MSRs (hv-frequencies)",
> >>          .flags = {
> >>              {.func = HV_CPUID_FEATURES, .reg = R_EAX,
> >> -             .bits = HV_ACCESS_FREQUENCY_MSRS},
> >> +             .bits = HV_HYPERCALL_AVAILABLE | HV_ACCESS_FREQUENCY_MSRS},
> >>              {.func = HV_CPUID_FEATURES, .reg = R_EDX,
> >>               .bits = HV_FREQUENCY_MSRS_AVAILABLE}
> >>          }
> >> @@ -888,7 +890,8 @@ static struct {
> >>          .desc = "reenlightenment MSRs (hv-reenlightenment)",
> >>          .flags = {
> >>              {.func = HV_CPUID_FEATURES, .reg = R_EAX,
> >> -             .bits = HV_ACCESS_REENLIGHTENMENTS_CONTROL}
> >> +             .bits = HV_HYPERCALL_AVAILABLE |
> >> +             HV_ACCESS_REENLIGHTENMENTS_CONTROL}
> >>          }
> >>      },
> >>      [HYPERV_FEAT_TLBFLUSH] = {
> >> -- 
> >> 2.30.2
> >> 
> 
> -- 
> Vitaly
> 

-- 
Eduardo