Re: [Qemu-devel] [PATCH 2/5] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[Eduardo Habkost]

On Thu, Jan 17, 2013 at 04:16:31PM +0100, Igor Mammedov wrote:
> Vendor property setter takes string as vendor value but cpudefs
> use uint32_t vendor[123] fields to define vendor value. It makes it
> difficult to unify and use property setter for values from cpudefs.
> 
> Simplify code by using vendor property setter, vendor[123] fields
> are converted into vendor[13] array to keep its value. And vendor
> property setter is used to access/set value on CPU.
> 
>  - Make for() cycle reusable for the next patch by adding
>    x86_cpu_vendor_words2str()
> 
> Intel's CPUID spec[1] says:
> "
> 5.1.1 ...
> These registers contain the ASCII string: GenuineIntel
> ...
> "
> 
> List[2] of known vendor values shows that they all are 12 ASCII
> characters long, padded where necessary with space
> 
> Current supported values are all ASCII characters packed in
> ebx, edx, ecx. So lets state that qemu supports 12 ASCII characters
> packed in ebx, edx, ecx registers for cpuid(0) instruction.

Nit: I guess you mean ASCII _printable_ characters. NUL is an ASCII
character as well, but it won't be supported.

> 
> *1 - http://www.intel.com/Assets/PDF/appnote/241618.pdf
> *2 - http://en.wikipedia.org/wiki/CPUID#EAX.3D0:_Get_vendor_ID
> 
> Signed-off-by: Igor Mammedov <address@hidden>

Reviewed-by: Eduardo Habkost <address@hidden>

Minor comment below, feel free to ignore.

> ---
> v4:
>  - add to commit msg that supported vendor should be 12 ASCII
>    characters long string
>  - squash in "add x86_cpu_vendor_words2str()" patch
>  - use strncmp() instead of strcmp()
>     Suggested-By: Andreas Färber <address@hidden>
>  - style fix: align args on next line properly
> 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 |  148 
> ++++++++++++++++-------------------------------------
>  target-i386/cpu.h |    6 +-
>  2 files changed, 48 insertions(+), 106 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ce914da..ab80dbe 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
[...]
> @@ -975,9 +937,8 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>      x86_cpu_def->vendor_override = 0;
>  
>      /* Call Centaur's CPUID instruction. */
> -    if (x86_cpu_def->vendor1 == CPUID_VENDOR_VIA_1 &&
> -        x86_cpu_def->vendor2 == CPUID_VENDOR_VIA_2 &&
> -        x86_cpu_def->vendor3 == CPUID_VENDOR_VIA_3) {
> +    if (!strncmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA,
> +        sizeof(x86_cpu_def->vendor))) {

We don't need strncmp() if we are sure the vendor field is always
NUL-terminated.

>          host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
>          eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
>          if (eax >= 0xC0000001) {
[...]

-- 
Eduardo