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?
>
>
> > 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
