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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> If you feel strongly, then please draft a coherent and simple set of
> rules for CODING_STYLE.

https://lists.xenproject.org/archives/html/xen-devel/2018-11/msg02853.html

Note how the half sentence in parentheses is already something
I added without being wholeheartedly convinced: I don't think
values read from or written to registers strongly have such a
requirement. This may be needed for the variables directly
handed to asm()-s, but not for values which have originally come
from a register (like CPUID output), but then get handed on.

I've also made attempts in other directions. They've all been either
completely ignored (like the one above) or turned down. Seeing
how we disagree here, I don't think it's worth my time making
another attempt, just to see you veto or everyone ignore it.

Jan



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