Re: [PATCH v2 2/7] i386/cpu: Mark CPUID 0x80000008 ECX bits[0:7] & [12:15] as reserved for Intel/Zhaoxin

[Zhao Liu <zhao1.liu@intel.com>]

> >           if (threads_per_pkg > 1) {
> >               /*
> > -             * Bits 15:12 is "The number of bits in the initial
> > -             * Core::X86::Apic::ApicId[ApicId] value that indicate
> > -             * thread ID within a package".
> > -             * Bits 7:0 is "The number of threads in the package is NC+1"
> > +             * Don't emulate Bits [7:0] & Bits [15:12] for Intel/Zhaoxin, since
> > +             * they're using 0x1f leaf.
> >                */
> > -            *ecx = (apicid_pkg_offset(topo_info) << 12) |
> > -                   (threads_per_pkg - 1);
> > +            if (cpu->vendor_cpuid_only_v2 &&
> > +                (IS_INTEL_CPU(env) || IS_ZHAOXIN_CPU(env))) {
> > +                    *ecx = 0;
> > +            } else {
> > +                /*
> > +                 * Bits 15:12 is "The number of bits in the initial
> > +                 * Core::X86::Apic::ApicId[ApicId] value that indicate
> > +                 * thread ID within a package".
> > +                 * Bits 7:0 is "The number of threads in the package is NC+1"
> > +                 */
> > +                *ecx = (apicid_pkg_offset(topo_info) << 12) |
> > +                       (threads_per_pkg - 1);
> > +            }
> >           } else {
> >               *ecx = 0;
> >           }
> 
> I prefer below:
> 
> 	if ((cpu->vendor_cpuid_only_v2 &&
> 	    (IS_INTEL_CPU(env) || IS_ZHAOXIN_CPU(env))) ||
> 	    threads_per_pkg < 2) {
> 		*ecx = 0;
> 	} else {
> 		...
> 	}

Yes, this works, but I would like to keep the vendor-related checks
separate from other logic - to avoid mixing them together.

Then it makes the code logic clearer and makes it easier for future
changes.

Thanks for your review!
Zhao