Re: [PATCH 1/4] libx86: Introduce x86_cpuid_lookup_vendor()

From: "Jan Beulich" <JBeulich@suse.com>
To: Juergen Gross <jgross@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	xen-devel@lists.xen.org, Sergey Dyasli <sergey.dyasli@citrix.com>,
	Wei Liu <wei.liu2@citrix.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH 1/4] libx86: Introduce x86_cpuid_lookup_vendor()
Date: Tue, 26 Mar 2019 09:23:29 -0600	[thread overview]
Message-ID: <5C9A43F10200007800221F45@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <750355ca-cf00-92aa-3417-1c582f504828@suse.com>

>>> On 26.03.19 at 15:47, <jgross@suse.com> wrote:
> On 26/03/2019 15:39, Jan Beulich wrote:
>>>>> On 26.03.19 at 15:23, <jgross@suse.com> wrote:
>>> IMO especially in the CPUID case it is desirable to explicitly specify
>>> the width of the data. Looking at nodes 0x80000002 and following this
>>> should be rather clear (and I even think get_model_name() should be
>>> modified to use a pointer to uint32_t instead of unsigned int). Using
>>> a type with size >= 4 doesn't fit really well. You want size == 4.
>> 
>> Why? Fixed width types only introduce unnecessary restrictions
>> when wanting to re-use code in other environments. And I don't
>> see why CPUID nodes 0x8000000[234] would be any better (or
>> worse) as an example here. If anything they tell us that neither
>> uint32_t nor unsigned int are right, and it should be char[4] or
>> uint8_t[4] instead (depending on whether we want to tie
>> ourselves to CHAR_BIT == 8, which clearly is more restrictive
>> than sizeof(int) >= 4, but otoh is also less likely to get in the
>> way).
> 
> You are using a little endian specific point of view.

Of course - it's x86 code when we talk about x86 CPUID. For the
architecture independent aspect I agree.

> That's the only
> reason the current code would work with a non-fixed width specification.
> 
> The char[4] variant would work only with a union where the other member
> would need to be a fixed width type like uint32_t.

I think we're talking of slightly different things (and I agree that
get_model_name() would need switching to uint32_t, if we
wanted to allow for sizeof(int) > 4). My original point though was
towards cases where values (not pointers to values) obtained
from CPUID get passed around as function arguments. I don't
see why the respective function parameters would need to be
uint32_t, irrespective of endianness.

For functions taking pointers moving away from uint32_t could
be doable as well, but might then require more care at certain
call sites (like get_model_name()), e.g. by needing to go through
intermediate variables. Whether that's beneficial or harmful to
code overall would need to be determined on a case by case
basis, unless maximum consistency was the primary goal.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel