Re: [Qemu-devel] [PATCH for-2.12 v2 2/2] i386/hyperv: error out if features requested but unsupported

[Eduardo Habkost <ehabkost@redhat.com>]

On Wed, Mar 28, 2018 at 06:30:24PM +0300, Roman Kagan wrote:
> In order to guarantee compatibility on migration, QEMU should have
> complete control over the features it announces to the guest via CPUID.
> 
> However, for a number of Hyper-V-related cpu properties, if the
> corresponding feature is not supported by the underlying KVM, the
> propery is silently ignored and the feature is not announced to the
> guest.
> 
> Refuse to start with an error instead.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>

Something I didn't consider before:

Will this block migration before it even starts, or will crash
the VM only after all migration data was sent to the destination?

I didn't test it, but kvm_arch_init_vcpu() seems to be too late
to block an invalid/unsupport configuration.

Maybe we can simply call hyperv_handle_properties() earlier,
inside x86_cpu_realizefn()?

(I know it's very late for this kind of intrusive change in
v2.12, but I still think it's a good idea to fix this as soon as
possible.)


> ---
> v1 -> v2:
>  - indicate what flag requested the feature that can't be enabled in the
>    error message
>  - fix a typo in the error message for VP_RUNTIME
> 
>  target/i386/kvm.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index b35623ae24..113926aff2 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -659,17 +659,41 @@ static int hyperv_handle_properties(CPUState *cs)
>          env->features[FEAT_HYPERV_EAX] |= HV_ACCESS_FREQUENCY_MSRS;
>          env->features[FEAT_HYPERV_EDX] |= HV_FREQUENCY_MSRS_AVAILABLE;
>      }
> -    if (cpu->hyperv_crash && has_msr_hv_crash) {
> +    if (cpu->hyperv_crash) {
> +        if (!has_msr_hv_crash) {
> +            fprintf(stderr, "Hyper-V crash MSRs "
> +                    "(requested by 'hv-crash' cpu flag) "
> +                    "are not supported by kernel\n");
> +            return -ENOSYS;
> +        }
>          env->features[FEAT_HYPERV_EDX] |= HV_GUEST_CRASH_MSR_AVAILABLE;
>      }
>      env->features[FEAT_HYPERV_EDX] |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE;
> -    if (cpu->hyperv_reset && has_msr_hv_reset) {
> +    if (cpu->hyperv_reset) {
> +        if (!has_msr_hv_reset) {
> +            fprintf(stderr, "Hyper-V reset MSR "
> +                    "(requested by 'hv-reset' cpu flag) "
> +                    "is not supported by kernel\n");
> +            return -ENOSYS;
> +        }
>          env->features[FEAT_HYPERV_EAX] |= HV_RESET_AVAILABLE;
>      }
> -    if (cpu->hyperv_vpindex && has_msr_hv_vpindex) {
> +    if (cpu->hyperv_vpindex) {
> +        if (!has_msr_hv_vpindex) {
> +            fprintf(stderr, "Hyper-V VP_INDEX MSR "
> +                    "(requested by 'hv-vpindex' cpu flag) "
> +                    "is not supported by kernel\n");
> +            return -ENOSYS;
> +        }
>          env->features[FEAT_HYPERV_EAX] |= HV_VP_INDEX_AVAILABLE;
>      }
> -    if (cpu->hyperv_runtime && has_msr_hv_runtime) {
> +    if (cpu->hyperv_runtime) {
> +        if (!has_msr_hv_runtime) {
> +            fprintf(stderr, "Hyper-V VP_RUNTIME MSR "
> +                    "(requested by 'hv-runtime' cpu flag) "
> +                    "is not supported by kernel\n");
> +            return -ENOSYS;
> +        }
>          env->features[FEAT_HYPERV_EAX] |= HV_VP_RUNTIME_AVAILABLE;
>      }
>      if (cpu->hyperv_synic) {
> -- 
> 2.14.3
> 

-- 
Eduardo