Re: [Xen-devel] [PATCH 2/2] x86: drop cpu_has_sse{,2}

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/12/16 14:33, Jan Beulich wrote:
>>>> On 09.12.16 at 15:15, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 09/12/16 13:28, Jan Beulich wrote:
>>>>>> On 09.12.16 at 14:00, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> On 09/12/16 11:54, Jan Beulich wrote:
>>>>> Commit dc88221c97 ("x86: rename XMM* features to SSE*") pointlessly
>>>>> added them - these features are always available on 64-bit CPUs. (Let's
>>>>> not assume this for MMX though in at least the insn emulator.)
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>> This isn't necessarily true when compiled for 32bit in the userspace
>>>> harness.
>>> In the test harness vcpu_has_* == cpu_has_*, as also
>>> demonstrated by
>>>
>>> #define host_and_vcpu_must_have(feat) vcpu_must_have(feat)
>>>
>>> . The change (as its title is trying to say) really only affects the
>>> hypervisor.
>> Right, but this change is still contrary to the written requirement.
> I don't understand.
>
>> /*                                                                           
>>  * Note the difference between vcpu_must_have_<feature>() and                
>>                                                           
>>  * host_and_vcpu_must_have(<feature>): The latter needs to be used when      
>>                                                                    
>>  * emulation code is using the same instruction class for carrying out       
>>                                                                    
>>  * the actual operation.                                                     
>>               
>>  */
> This comment sits inside #ifdef __XEN__.

Yes, which means it applies to code running in the hypervisor.

>
>> We are using SSE and SSE2 instructions for carrying out that emulation,
>> so should still be using the host_and_vcpu check.
> Xen only running on x86-64, and x86-64 taking SSE and SSE2 as
> given (all math operations other than 80-bit ones are using these
> instead of the x87), there's no need to look at any flags here
> (and as you can see from the patch, the flags also already haven't
> been used anywhere outside the insn emulator, despite us using
> SSE2 insns, e.g. MOVNTI in the page copying and clearing code).

I agree that 64bit code may take SSE and SSE2 as a given, which is why I
have suggested this:

diff --git a/xen/include/asm-x86/cpufeature.h
b/xen/include/asm-x86/cpufeature.h
index c7c8520..c62dacd 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -59,8 +59,8 @@ XEN_CPUFEATURE(XEN_SMAP,        (FSCAPINTS+0)*32+ 11)
/* SMAP gets used by Xen i
 #define cpu_has_sep            boot_cpu_has(X86_FEATURE_SEP)
 #define cpu_has_mtrr           1
 #define cpu_has_mmx            1
-#define cpu_has_sse            boot_cpu_has(X86_FEATURE_SSE)
-#define cpu_has_sse2           boot_cpu_has(X86_FEATURE_SSE2)
+#define cpu_has_sse            1
+#define cpu_has_sse2           1
 #define cpu_has_sse3           boot_cpu_has(X86_FEATURE_SSE3)
 #define cpu_has_sse4_2         boot_cpu_has(X86_FEATURE_SSE4_2)
 #define cpu_has_htt            boot_cpu_has(X86_FEATURE_HTT)

This is fine, as we only support 64bit in the hypervisor.

>> Swapping the hypervisor defines to being 1 will cause the
>> generate_exception_if() clause to become dead and get dropped, turning
>> host_and_vcpu_must_have() into just vcpu_must_have_##feat
> Which is fine for the hypervisor. And for the test harness the
> vcpu_must_have() is sufficient, as explained before.

It is a layering violation to remove the host_has_* part of the check
from the emulator.

The purpose of the host_has_* part of the check is to be present when we
are using instructions from a certain instruction class, and we are
still using SSE/SSE2 instructions in the emulator.  Untill we drop the
32bit build of x86_emulate.c, it is wrong to drop the SSE/SSE2 check.

With the above diff, host_and_vcpu_must_have(SSE) will be simplified to
vcpu_must_have(SSE) by the compiler, which achieves your goal of
removing a tautological check.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel