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

[Juergen Gross <jgross@xxxxxxxx>]

On 26/03/2019 15:07, Jan Beulich wrote:
>>>> On 26.03.19 at 14:11, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 26/03/2019 11:52, Jan Beulich wrote:
>>>>>> On 21.03.19 at 13:21, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> Also introduce constants for the vendor strings in CPUID leaf 0.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> albeit I'd appreciate if this was committed together with an actual
>>> user (other than the testsuite one) of the new function, and
>>> despite ...
>>>
>>>> --- a/xen/include/xen/lib/x86/cpuid.h
>>>> +++ b/xen/include/xen/lib/x86/cpuid.h
>>>> @@ -65,6 +65,12 @@ static inline void cpuid_count_leaf(
>>>>  #undef BX_CON
>>>>  #undef XCHG
>>>>  
>>>> +/**
>>>> + * Given the vendor id from CPUID leaf 0, look up Xen's internal integer
>>>> + * vendor ID.  Returns X86_VENDOR_UNKNOWN for any unknown vendor.
>>>> + */
>>>> +unsigned int x86_cpuid_lookup_vendor(uint32_t ebx, uint32_t ecx, uint32_t 
>>>> edx);
>>> ... the undesirable (imo; I think I know you think otherwise) use of
>>> fixed width types here.
>>
>> Please, for the benefit of everyone, stop making snide remarks like
>> this.  It comes across as rude, and is off-putting to contributors.
> 
> It coming across as rude was certainly not my intention - apologies.
> 
>> You are complaining that I didn't write code in way you would have
>> done.  Just because you dislike-but-don't-object-to how the code look
>> doesn't make the code wrong, or worthy of comment.
> 
> That's your way of looking at it. My basic desire for consistency in
> how code overall looks like still made me think it was worthwhile to
> point this out once again (and I'm afraid I'm not going to be willing
> to be uniformly silent on such matters). This is because if you
> yourself follow what you wrote above, you'd not complain if a
> patch of mine was introducing a sibling function using all unsigned
> int (I probably wouldn't, again for consistency's sake, but I might
> in a somewhat more remote area of code). The end result would
> be a total mixture of fixed width types and basic ones, which no-
> one could make sense of by looking at, or even by looking at just
> some recent commits (in an attempt to get a feel for where we're
> trying to move).
> 
>> Your judgement of when to use which types is, in my opinion, very
>> inconsistent.  By my judgement, I am conforming to your expectation of
>> using fixed width types when the ABI calls for it, which is the case
>> here - the ABI is that of the CPUID instruction.
> 
> I don't think I've ever said anything like this, and we've had the same
> dispute over CPUID in the past. Instead I think I've been pretty
> consistently asking to use fixed width types only where strictly
> needed (or where e.g. improving generated code quality). In all cases
> where (following the example here) unsigned int is fine, it should be
> preferred over uint32_t (due to our assumption that
> sizeof(unsigned int) >= 4). The only ABI relevance I can see here is
> wrt the public interface - there fixed width types should indeed be
> used (almost) everywhere, to make the interfaces sufficiently portable.

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.


Juergen

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