On Sun, Jan 10, 2021 at 08:38:36AM -1000, Richard Henderson wrote: > On 1/10/21 8:34 AM, Richard Henderson wrote: > > On 1/9/21 3:46 PM, Roman Bolshakov wrote: > >> +static int xgetbv(uint32_t cpuid_ecx, uint32_t idx, uint64_t *xcr) > >> { > >> - uint32_t eax, edx; > >> + uint32_t xcrl, xcrh; > >> > >> - __asm__ volatile ("xgetbv" > >> - : "=a" (eax), "=d" (edx) > >> - : "c" (xcr)); > >> + if (cpuid_ecx && CPUID_EXT_OSXSAVE) { > >> + /* The xgetbv instruction is not available to older versions of > >> + * the assembler, so we encode the instruction manually. > >> + */ > >> + asm(".byte 0x0f, 0x01, 0xd0" : "=a" (xcrl), "=d" (xcrh) : "c" > >> (idx)); > >> > >> - return (((uint64_t)edx) << 32) | eax; > >> + *xcr = (((uint64_t)xcrh) << 32) | xcrl; > >> + return 0; > >> + } > >> + > >> + return 1; > >> } > > > > Not to bikeshed too much, but this looks like it should return bool, and > > true > > on success, not the other way around. >
I agree, it'd better to comprehend (and Hill has already sent v2 with this). > Also, if we're going to put this some place common, forcing the caller to do > the cpuid that feeds this, then we should probably make all of the startup > cpuid stuff common as well. > I proposed the version because all callers of xgetbv instruction already call cpuid before invoking inline xgetbv. > Note that we'd probably have to use constructor priorities to get that right > for util/bufferiszero.c. > Please correct me if I read this wrong. What you're saying is we should initialize cpuid in constructors and then use cached cpuid ecx in xgetbv() (and drop one argument, respectively)? Thanks, Roman