Re: [PATCH] hvf: guard xgetbv call.

[Roman Bolshakov <r.bolshakov@yadro.com>]

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