your patch "crypto: sha1 - SSSE3 based SHA1 implementation for x86-64" vs xsave

your patch "crypto: sha1 - SSSE3 based SHA1 implementation for x86-64" vs xsave

[Jan Beulich]

Mathias,

in avx_usable() you use cpu_has_osxsave to determine whether AVX can
actually be used, but I can't see how that conditional would ever evaluate
to true. Checking CPUID.OSXSAVE is actually a user land requirement,
whereas in the kernel you should really look at kernel internal variables
to determine whether xsave was enabled (or, if you really want to stay
with using cpu_has_osxsave you'd have to force re-execution of
get_cpu_cap() after xsave got enabled on individual CPUs), namely
mmu_cr4_features having X86_CR4_OSXSAVE set.

Additionally, under a hypervisor, CPUID.OSXSAVE may be set (due to
the hypervisor having enabled xsave), while the kernel may be running
with xsave disabled (e.g. due to a command line option saying so).

Thanks, Jan

Re: your patch "crypto: sha1 - SSSE3 based SHA1 implementation for x86-64" vs xsave

[Mathias Krause]

On Thu, Nov 17, 2011 at 11:33 AM, Jan Beulich <JBeulich@suse.com> wrote:
> Mathias,
>
> in avx_usable() you use cpu_has_osxsave to determine whether AVX can
> actually be used, but I can't see how that conditional would ever evaluate
> to true.

Well, it does, at least in my setup, a Core i7-2620M:

  [   44.902613] sha1_ssse3: Using AVX optimized SHA-1 implementation

> Checking CPUID.OSXSAVE is actually a user land requirement,
> whereas in the kernel you should really look at kernel internal variables
> to determine whether xsave was enabled

That's a valid point. I actually ported the check from the initial
assembler implementation which was written with userland code in mind.
Albeit this test can also be used in kernel code because the kernel
only sets this flag after xsave was enabled in
arch/x86/kernel/xsave.c:xstate_enable_boot_cpu/xstate_enable

> (or, if you really want to stay
> with using cpu_has_osxsave you'd have to force re-execution of
> get_cpu_cap() after xsave got enabled on individual CPUs), namely
> mmu_cr4_features having X86_CR4_OSXSAVE set.

I don't see where X86_CR4_OSXSAVE would get reset after xsave init.
Can you give me a pointer to that code?

> Additionally, under a hypervisor, CPUID.OSXSAVE may be set (due to
> the hypervisor having enabled xsave), while the kernel may be running
> with xsave disabled (e.g. due to a command line option saying so).

When noxsave is given on the kernel command line, X86_FEATURE_XSAVE
will be cleared and xsave_init() won't call xstate_enable(), so not
setting X86_CR4_OSXSAVE. All fine.


Thanks,
Mathias

Re: your patch "crypto: sha1 - SSSE3 based SHA1 implementation for x86-64" vs xsave

[Jan Beulich]

>>> On 17.11.11 at 13:36, Mathias Krause <minipli@googlemail.com> wrote:
> On Thu, Nov 17, 2011 at 11:33 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> Mathias,
>>
>> in avx_usable() you use cpu_has_osxsave to determine whether AVX can
>> actually be used, but I can't see how that conditional would ever evaluate
>> to true.
> 
> Well, it does, at least in my setup, a Core i7-2620M:
> 
>   [   44.902613] sha1_ssse3: Using AVX optimized SHA-1 implementation
> 
>> Checking CPUID.OSXSAVE is actually a user land requirement,
>> whereas in the kernel you should really look at kernel internal variables
>> to determine whether xsave was enabled
> 
> That's a valid point. I actually ported the check from the initial
> assembler implementation which was written with userland code in mind.
> Albeit this test can also be used in kernel code because the kernel
> only sets this flag after xsave was enabled in
> arch/x86/kernel/xsave.c:xstate_enable_boot_cpu/xstate_enable

Indeed, I got this the wrong way round.

>> (or, if you really want to stay
>> with using cpu_has_osxsave you'd have to force re-execution of
>> get_cpu_cap() after xsave got enabled on individual CPUs), namely
>> mmu_cr4_features having X86_CR4_OSXSAVE set.
> 
> I don't see where X86_CR4_OSXSAVE would get reset after xsave init.
> Can you give me a pointer to that code?

I didn't say it would get cleared. I said it should be used in favor of
cpu_has_osxsave.

>> Additionally, under a hypervisor, CPUID.OSXSAVE may be set (due to
>> the hypervisor having enabled xsave), while the kernel may be running
>> with xsave disabled (e.g. due to a command line option saying so).
> 
> When noxsave is given on the kernel command line, X86_FEATURE_XSAVE
> will be cleared and xsave_init() won't call xstate_enable(), so not
> setting X86_CR4_OSXSAVE. All fine.

But you're not looking at this bit, you're looking at cpu_has_osxsave,
which is a CPUID bit (not a CR4 one).

Jan

Re: your patch "crypto: sha1 - SSSE3 based SHA1 implementation for x86-64" vs xsave

[Mathias Krause]

On Thu, Nov 17, 2011 at 2:09 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> I don't see where X86_CR4_OSXSAVE would get reset after xsave init.
>> Can you give me a pointer to that code?
>
> I didn't say it would get cleared. I said it should be used in favor of
> cpu_has_osxsave.
>
>>> Additionally, under a hypervisor, CPUID.OSXSAVE may be set (due to
>>> the hypervisor having enabled xsave), while the kernel may be running
>>> with xsave disabled (e.g. due to a command line option saying so).
>>
>> When noxsave is given on the kernel command line, X86_FEATURE_XSAVE
>> will be cleared and xsave_init() won't call xstate_enable(), so not
>> setting X86_CR4_OSXSAVE. All fine.
>
> But you're not looking at this bit, you're looking at cpu_has_osxsave,
> which is a CPUID bit (not a CR4 one).

You're right. But that's no problem at all, as the cpu_has_* tests are
only to error out early. They also ensure it is save to execute the
next instruction -- xsetbv(). This instruction actually does the xsave
runtime support check by looking at the extended control register XCR0
and the enabled features in there.
So you're right that the cpu_has_osxsave might be superfluous here but
it doesn't hurt either.

Mathias