Eduardo Habkost <ehabk...@redhat.com> writes: > 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. >
Agreed, adding a patch to change '12' to sizeof(). >> >> /* '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. The problem here is that object_property_set_str() only works with NULL-terminated strings and 'hyperv_vendor_id' doesn't have it (that's why I explicitly do 'cpu->hyperv_vendor[sizeof(cpu->hyperv_vendor_id)] = 0'). We could've converted 'hyperv_vendor_id' to a char[13] array to accomodate for '\0', but cpu->hyperv_vendor_id[0] = c->ebx; cpu->hyperv_vendor_id[1] = c->ecx; cpu->hyperv_vendor_id[2] = c->edx; assignments will have to be converted to something and this won't look natural. -- Vitaly