[2/3] x86/boot: Cache cpu_has_hypervisor very early on boot
diff mbox series

Message ID 20191101202502.31750-3-andrew.cooper3@citrix.com
State New
Headers show
Series
  • Fix PV shim ballooning problems
Related show

Commit Message

Andrew Cooper Nov. 1, 2019, 8:25 p.m. UTC
We cache Long Mode and No Execute early on boot, so take the opportunity to
cache HYPERVISOR early as well.

Replace opencoded early access to the feature bit.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/apic.c         | 2 +-
 xen/arch/x86/boot/head.S    | 4 ++++
 xen/arch/x86/efi/efi-boot.h | 6 ++++--
 xen/arch/x86/guest/xen.c    | 6 +-----
 xen/arch/x86/mm.c           | 3 +--
 5 files changed, 11 insertions(+), 10 deletions(-)

Comments

Jan Beulich Nov. 4, 2019, 1:32 p.m. UTC | #1
On 01.11.2019 21:25, Andrew Cooper wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -630,6 +630,10 @@ trampoline_setup:
>  
>  1:
>          /* Interrogate CPU extended features via CPUID. */
> +        mov     $1, %eax
> +        cpuid
> +        mov     %ecx, sym_fs(boot_cpu_data) + CPUINFO_FEATURE_OFFSET(X86_FEATURE_HYPERVISOR)

I understand the ECX output is all we need right now. But wouldn't
it be better to then store EDX as well (and similarly ECX of
0x80000001 output)?

Also, along the lines of a question back to Sergey on his
standalone patch, wouldn't it be better to take the opportunity
and check here that CPUID leaf 1 is actually valid?

Of course the question on the (intended) effect of
"cpuid=no-hypervisor" also remains. As said before, I think this
should be honored by all of our code that possibly can (i.e. at
least everything following command line parsing).

Jan
Andrew Cooper Nov. 4, 2019, 3:31 p.m. UTC | #2
On 04/11/2019 13:32, Jan Beulich wrote:
> On 01.11.2019 21:25, Andrew Cooper wrote:
>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -630,6 +630,10 @@ trampoline_setup:
>>  
>>  1:
>>          /* Interrogate CPU extended features via CPUID. */
>> +        mov     $1, %eax
>> +        cpuid
>> +        mov     %ecx, sym_fs(boot_cpu_data) + CPUINFO_FEATURE_OFFSET(X86_FEATURE_HYPERVISOR)
> I understand the ECX output is all we need right now. But wouldn't
> it be better to then store EDX as well (and similarly ECX of
> 0x80000001 output)?

No - I don't think so.

I did debate applying an and mask for the features we only intend to be
usable this early, to avoid getting buggy code which checks for
unexpected features this early.

> Also, along the lines of a question back to Sergey on his
> standalone patch, wouldn't it be better to take the opportunity
> and check here that CPUID leaf 1 is actually valid?

There is no such thing as a 64-bit capable CPU without leaf 1.

> Of course the question on the (intended) effect of
> "cpuid=no-hypervisor" also remains. As said before, I think this
> should be honored by all of our code that possibly can (i.e. at
> least everything following command line parsing).

There is no chance of making that work in a sane way - we use
cpu_has_hypervisor for quite a few things before the command line gets
parsed.

If you're booting under a hypervisor, your hypervisor ought to have a
way to turn that off.

~Andrew
Jan Beulich Nov. 4, 2019, 3:41 p.m. UTC | #3
On 04.11.2019 16:31, Andrew Cooper wrote:
> On 04/11/2019 13:32, Jan Beulich wrote:
>> On 01.11.2019 21:25, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -630,6 +630,10 @@ trampoline_setup:
>>>  
>>>  1:
>>>          /* Interrogate CPU extended features via CPUID. */
>>> +        mov     $1, %eax
>>> +        cpuid
>>> +        mov     %ecx, sym_fs(boot_cpu_data) + CPUINFO_FEATURE_OFFSET(X86_FEATURE_HYPERVISOR)
>> I understand the ECX output is all we need right now. But wouldn't
>> it be better to then store EDX as well (and similarly ECX of
>> 0x80000001 output)?
> 
> No - I don't think so.
> 
> I did debate applying an and mask for the features we only intend to be
> usable this early, to avoid getting buggy code which checks for
> unexpected features this early.

Indeed doing so would seem a good reason to not also store the EDX
value here.

>> Also, along the lines of a question back to Sergey on his
>> standalone patch, wouldn't it be better to take the opportunity
>> and check here that CPUID leaf 1 is actually valid?
> 
> There is no such thing as a 64-bit capable CPU without leaf 1.

About anything can be constructed under a hypervisor. But well, I
guess I'll stop mumbling on this aspect.

>> Of course the question on the (intended) effect of
>> "cpuid=no-hypervisor" also remains. As said before, I think this
>> should be honored by all of our code that possibly can (i.e. at
>> least everything following command line parsing).
> 
> There is no chance of making that work in a sane way - we use
> cpu_has_hypervisor for quite a few things before the command line gets
> parsed.

You said so the other day, but iirc when checking I wasn't able to
identify any such case, let alone "quite a few".

Anyway, feel free to add
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan
Sergey Dyasli Nov. 5, 2019, 11:08 a.m. UTC | #4
On 01/11/2019 20:25, Andrew Cooper wrote:
> We cache Long Mode and No Execute early on boot, so take the opportunity to
> cache HYPERVISOR early as well.

Either:
1. the description needs clarifying that the whole 1c featureset is
   being stored, or
2. a mask should be applied to store only the hypervisor bit
   (this would be a safer option)

> Replace opencoded early access to the feature bit.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

--
Thanks,
Sergey

Patch
diff mbox series

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index a5f7b05d5a..a8ee18636f 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1156,7 +1156,7 @@  static void __init check_deadline_errata(void)
     const struct x86_cpu_id *m;
     unsigned int rev;
 
-    if ( boot_cpu_has(X86_FEATURE_HYPERVISOR) )
+    if ( cpu_has_hypervisor )
         return;
 
     m = x86_match_cpu(deadline_match);
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 77309e3c82..8d0ffbd1b0 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -630,6 +630,10 @@  trampoline_setup:
 
 1:
         /* Interrogate CPU extended features via CPUID. */
+        mov     $1, %eax
+        cpuid
+        mov     %ecx, sym_fs(boot_cpu_data) + CPUINFO_FEATURE_OFFSET(X86_FEATURE_HYPERVISOR)
+
         mov     $0x80000000,%eax
         cpuid
         shld    $16,%eax,%ecx
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index cde193a771..232972eedf 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -637,11 +637,13 @@  static void __init efi_arch_handle_module(struct file *file, const CHAR16 *name,
 static void __init efi_arch_cpu(void)
 {
     uint32_t eax = cpuid_eax(0x80000000);
+    uint32_t *caps = boot_cpu_data.x86_capability;
+
+    caps[cpufeat_word(X86_FEATURE_HYPERVISOR)] = cpuid_ecx(1);
 
     if ( (eax >> 16) == 0x8000 && eax > 0x80000000 )
     {
-        boot_cpu_data.x86_capability[cpufeat_word(X86_FEATURE_SYSCALL)]
-            = cpuid_edx(0x80000001);
+        caps[cpufeat_word(X86_FEATURE_SYSCALL)] = cpuid_edx(0x80000001);
 
         if ( cpu_has_nx )
             trampoline_efer |= EFER_NX;
diff --git a/xen/arch/x86/guest/xen.c b/xen/arch/x86/guest/xen.c
index 7b7a5badab..a329e7c886 100644
--- a/xen/arch/x86/guest/xen.c
+++ b/xen/arch/x86/guest/xen.c
@@ -69,11 +69,7 @@  static void __init find_xen_leaves(void)
 
 void __init probe_hypervisor(void)
 {
-    if ( xen_guest )
-        return;
-
-    /* Too early to use cpu_has_hypervisor */
-    if ( !(cpuid_ecx(1) & cpufeat_mask(X86_FEATURE_HYPERVISOR)) )
+    if ( xen_guest || !cpu_has_hypervisor )
         return;
 
     find_xen_leaves();
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 57f22775ac..bd8182f40f 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -6112,8 +6112,7 @@  const struct platform_bad_page *__init get_platform_badpages(unsigned int *array
     case 0x000506e0: /* errata SKL167 / SKW159 */
     case 0x000806e0: /* erratum KBL??? */
     case 0x000906e0: /* errata KBL??? / KBW114 / CFW103 */
-        *array_size = (cpuid_eax(0) >= 7 &&
-                       !(cpuid_ecx(1) & cpufeat_mask(X86_FEATURE_HYPERVISOR)) &&
+        *array_size = (cpuid_eax(0) >= 7 && !cpu_has_hypervisor &&
                        (cpuid_count_ebx(7, 0) & cpufeat_mask(X86_FEATURE_HLE)));
         return &hle_bad_page;
     }