Re: [PATCH v6 01/19] i386: keep hyperv_vendor string up-to-date

[Eduardo Habkost]

On Thu, Apr 22, 2021 at 06:11:12PM +0200, Vitaly Kuznetsov wrote:
> When cpu->hyperv_vendor is not set manually we default to "Microsoft Hv"
> and in 'hv_passthrough' mode we get the information from the host. This
> information is stored in cpu->hyperv_vendor_id[] array but we don't update
> cpu->hyperv_vendor string so e.g. QMP's query-cpu-model-expansion output
> is incorrect.

I was confused for a while because this can't happen until patch
15/19 is applied.  Probably worth a note in the commit message
indicating that hyperv_handle_properties() will be called by
x86_cpu_expand_features() in the future.

> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  target/i386/cpu.c     | 19 +++++++++----------
>  target/i386/kvm/kvm.c |  5 +++++
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ad99cad0e7ce..2d05df232329 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6665,17 +6665,16 @@ static void x86_cpu_hyperv_realize(X86CPU *cpu)
>  
>      /* Hyper-V vendor id */
>      if (!cpu->hyperv_vendor) {
> -        memcpy(cpu->hyperv_vendor_id, "Microsoft Hv", 12);
> -    } else {
> -        len = strlen(cpu->hyperv_vendor);
> -
> -        if (len > 12) {
> -            warn_report("hv-vendor-id truncated to 12 characters");
> -            len = 12;
> -        }
> -        memset(cpu->hyperv_vendor_id, 0, 12);
> -        memcpy(cpu->hyperv_vendor_id, cpu->hyperv_vendor, len);
> +        object_property_set_str(OBJECT(cpu), "hv-vendor-id", "Microsoft Hv",
> +                                &error_abort);
> +    }
> +    len = strlen(cpu->hyperv_vendor);
> +    if (len > 12) {
> +        warn_report("hv-vendor-id truncated to 12 characters");
> +        len = 12;
>      }
> +    memset(cpu->hyperv_vendor_id, 0, 12);
> +    memcpy(cpu->hyperv_vendor_id, cpu->hyperv_vendor, len);

Existing issue: hardcoded 12 as the size of hyperv_vendor_id here
(compare with the code you add below using sizeof()).  I don't
think this should hold the whole series, so it can be fixed in a
follow up patch if necessary.

>  
>      /* 'Hv#1' interface identification*/
>      cpu->hyperv_interface_id[0] = 0x31237648;
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 7fe9f527103c..ab073a5e9c44 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -1215,6 +1215,11 @@ static int hyperv_handle_properties(CPUState *cs,
>              cpu->hyperv_vendor_id[0] = c->ebx;
>              cpu->hyperv_vendor_id[1] = c->ecx;
>              cpu->hyperv_vendor_id[2] = c->edx;
> +            cpu->hyperv_vendor = g_realloc(cpu->hyperv_vendor,
> +                                           sizeof(cpu->hyperv_vendor_id) + 
> 1);
> +            memcpy(cpu->hyperv_vendor, cpu->hyperv_vendor_id,
> +                   sizeof(cpu->hyperv_vendor_id));
> +            cpu->hyperv_vendor[sizeof(cpu->hyperv_vendor_id)] = 0;

I don't like having to do manual g_realloc() + memcpy() here
(calling object_property_set_str() would be simpler), but I
believe it will be easier to clean this up after this whole
series is applied.

Reluctantly:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

>          }
>  
>          c = cpuid_find_entry(cpuid, HV_CPUID_INTERFACE, 0);
> -- 
> 2.30.2
> 

-- 
Eduardo