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

[Richard Henderson]

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.

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

> > +#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