Re: [Qemu-devel] [PATCH] target-i386: make x86_def_t.vendor[1, 2, 3] a string and use cpuid_vendor union in CPUX86State

[Igor Mammedov]

On Tue, 15 Jan 2013 15:51:18 -0200
Eduardo Habkost <address@hidden> wrote:

> On Tue, Jan 15, 2013 at 01:06:28PM +0100, Igor Mammedov wrote:
[..]
> >   - replace cpuid_vendor[1.2.3] words in CPUX86State with union
> >     to simplify vendor property setter and pack/unpack procedures
> >   - add x86_cpu_vendor_words2str() to make for() cycle reusable
> >   - convert words in cpuid_vendor union to little-endian when
> >     returning them to guest in cpuid instruction emulation, since
> >     they are not packed manualy anymore
> 
> What about all other cases where CPUID_VENDOR_*_[123] constants are
> used?
NACK patch, it is probably horribly broken on big-endian acrh.

> 
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > v4:
> >  - use union for cpuid_vendor to simplify convertions string<=>registers
> > v3:
> >  - replace x86cpu_vendor_words2str() with x86_cpu_vendor_words2str()
> >    due to renaming of the last in previous patch
> >  - rebased with "target-i386: move out CPU features initialization
> >    in separate func" patch dropped
> > v2:
> >   - restore deleted host_cpuid() call in kvm_cpu_fill_host()
> >      Spotted-By: Eduardo Habkost <address@hidden>
> > ---
> >  target-i386/cpu.c       |  180 
> > ++++++++++++++---------------------------------
> >  target-i386/cpu.h       |   17 +++--
> >  target-i386/translate.c |    4 +-
> >  3 files changed, 67 insertions(+), 134 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 333745b..882da50 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -45,6 +45,18 @@
> >  #include "hw/apic_internal.h"
> >  #endif
> >  
> > +static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> > +                                     uint32_t vendor2, uint32_t vendor3)
> > +{
> > +    int i;
> > +    for (i = 0; i < 4; i++) {
> > +        dst[i] = vendor1 >> (8 * i);
> > +        dst[i + 4] = vendor2 >> (8 * i);
> > +        dst[i + 8] = vendor3 >> (8 * i);
> > +    }
> > +    dst[CPUID_VENDOR_SZ] = '\0';
> > +}
> 
> Why this function still exists, if we have the union?
to copy words from host_cpuid(0, 0, NULL, &ebx, &ecx, &edx); to
x86_cpu_def->vendor when vendor_override is removed.
but never mind.

> 
> Anyway: even with the union, we can't avoid having explicit conversion
> code because of endianness. It doesn't matter which option we choose:
> 
> - if we store vendor[123], we need to convert to string on the vendor
>   property getter/setter;
> - if we store vendor string, we need to convert to register values on
>   cpu_x86_cpuid();
> - if we store it in an union we need endianness conversion code (inline
>   or in a function) every time we read vendor[123].
> 
> That said, I don't see the point of the union.
Agreed, union patch gets more intrusive as we would need to care about
endiannes in every place words are consumed.

Original patch looks much less intrusive and touches only words=>string
conversion. +1 in favor of original patch.

> 
[...]
> 
> -- 
> Eduardo


-- 
Regards,
  Igor