| Message ID | 20210422161130.652779-2-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: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 Habkost <ehabkost@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 <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. > 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.
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); /* '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; } c = cpuid_find_entry(cpuid, HV_CPUID_INTERFACE, 0);
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. 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(-)