[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5] cutils: Rewrite x86 buffer zero checking
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH v5] cutils: Rewrite x86 buffer zero checking |
Date: |
Wed, 14 Sep 2016 10:56:56 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 |
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
>
>
>