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 <vkuzn...@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 <ehabk...@redhat.com>
> }
>
> c = cpuid_find_entry(cpuid, HV_CPUID_INTERFACE, 0);
> --
> 2.30.2
>
--
Eduardo
