Re: [Qemu-devel] [PATCH v5] cutils: Rewrite x86 buffer zero checking

[Paolo Bonzini]

On 14/09/2016 03:11, Richard Henderson wrote:
> On 09/13/2016 04:21 PM, Paolo Bonzini wrote:
>>
>>
>> On 13/09/2016 22:57, Richard Henderson wrote:
>>> -#if defined(CONFIG_AVX2_OPT) || (defined(CONFIG_CPUID_H) &&
>>> defined(__SSE2__))
>>> -#include <cpuid.h>
>>> -
>>> +#if (defined(CONFIG_AVX2_OPT) && defined(CONFIG_CPUID_H)) ||
>>> defined(__SSE2__)
>>
>> Your __SSE2__ version is better than mine which required cpuid.h just to
>> simplify the logic a bit.  On the other hand, CONFIG_CPUID_H is not
>> needed in CONFIG_AVX2_OPT, because the test already requires cpuid.h.
> 
> Hmm, it does, although it needn't -- the test case would compile without
> it.
> 
> Although I bet there's no situation in which the pragmas are supported
> and cpuid.h isn't, I think it's cleaner not to infer stuff like this.

Yeah, I agree.  But we can change the test to not look at cpuid.h
separately.

>>> +#ifdef CONFIG_CPUID_H
>>> +# define INIT_CACHE
>>> +# define INIT_ACCEL
>>> +#else
>>> +# ifndef __SSE2__
>>> +#  error "ISA selection confusion"
>>> +# endif
>>> +# define INIT_CACHE  = CACHE_SSE2
>>> +# define INIT_ACCEL  = buffer_zero_sse2
>>>  #endif
>>
>> This is ugly, any reason not to initialize INIT_CACHE/INIT_ACCEL to
>> respectively 0 and NULL, or 0 and buffer_zero_int in the #ifdef
>> CONFIG_CPUID_H case?
> 
> I was hoping to avoid an extra RELATIVE relocation in the (normal) PIE
> case.

There would be no relocation for 0 and NULL, right?  GCC would actually
put them in bss, IIRC.

Paolo

>>> +#undef INIT_CACHE
>>> +#undef INIT_ACCEL
>>
>> The #undef is not really necessary since this file hardly has anything
>> after the toplevel #endif.
> 
> Fair enough.
> 
> 
> r~
> 
>>
>> Just tell me which changes you agree with, I can make them locally.
>>
>> Paolo
> 
> 
>