Re: [PATCH 1/4] x86: remove unused x87 remnants of 32-bit days

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 14/10/15 14:57, Andrew Cooper wrote:
> On 14/10/15 14:29, Jan Beulich wrote:
>> x86-64 requires FXSR, XMM, and XMM2, so there's no point in hiding
>> respective code behind conditionals.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with two
> suggestions.
>
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1295,10 +1295,8 @@ void __init noreturn __start_xen(unsigne
>>  
>>      identify_cpu(&boot_cpu_data);
>>  
>> -    if ( cpu_has_fxsr )
>> -        set_in_cr4(X86_CR4_OSFXSR);
>> -    if ( cpu_has_xmm )
>> -        set_in_cr4(X86_CR4_OSXMMEXCPT);
>> +    set_in_cr4(X86_CR4_OSFXSR);
>> +    set_in_cr4(X86_CR4_OSXMMEXCPT);
> set_in_cr4() takes a mask, so could take both of these at once, to be
> slightly more efficient.
>
> If the bsp path gained an unconditional write_cr4(mmu_cr4_features) then
> it might be better to fold them into the initialiser for mmu_cr4_features.
>
>> --- a/xen/include/asm-x86/page.h
>> +++ b/xen/include/asm-x86/page.h
>> @@ -206,13 +206,10 @@ typedef struct { u64 pfn; } pagetable_t;
>>  #define pagetable_null()        pagetable_from_pfn(0)
>>  
>>  void clear_page_sse2(void *);
>> -#define clear_page(_p)      (cpu_has_xmm2 ?                             \
>> -                             clear_page_sse2((void *)(_p)) :            \
>> -                             (void)memset((void *)(_p), 0, PAGE_SIZE))
>>  void copy_page_sse2(void *, const void *);
>> -#define copy_page(_t,_f)    (cpu_has_xmm2 ?                             \
>> -                             copy_page_sse2(_t, _f) :                   \
>> -                             (void)memcpy(_t, _f, PAGE_SIZE))
>> +
>> +#define clear_page(_p)      clear_page_sse2(_p)
>> +#define copy_page(_t,_f)    copy_page_sse2(_t, _f)
> As you are changing this, an extra space following the comma.
>
> ~Andrew

A third suggestion to delete the trailing whitespace before fninit in
the following hunk.

--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -19,15 +19,12 @@
 
 static void fpu_init(void)
 {
-    unsigned long val;
+    uint32_t val = MXCSR_DEFAULT;
     
     asm volatile ( "fninit" );
-    if ( cpu_has_xmm )
-    {
-        /* load default value into MXCSR control/status register */
-        val = MXCSR_DEFAULT;
-        asm volatile ( "ldmxcsr %0" : : "m" (val) );
-    }
+
+    /* load default value into MXCSR control/status register */
+    asm volatile ( "ldmxcsr %0" : : "m" (val) );
 }
 

~Andrew