| Message ID | 20210422161130.652779-18-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i386: KVM: expand Hyper-V features early | expand |
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? Wouldn't it be simpler to simply add a new HYPERV_FEAT_HYPERCALL_AVAILABLE bit to hyperv_features, and enabling it by default? 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. > --- > 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 >
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. > 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-vpindex,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. > > >> --- >> 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 >>
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 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. 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(). >> >> >> > 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. Strictly speaking, this bit has to be set or the guest can't use any of the Hyper-V features. It was proven that at least certain Windows versions don't even check it assuming it's always set. Moreover, we already set it for some very basic enlightenments ('hv-time') so there's going to be no change at all for real world configutations. > > >> >> > >> > >> >> --- >> >> 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 >>
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. > [...]
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] = {
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> --- target/i386/kvm/kvm.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)