[PATCH] x86/SVM: correct boot time cpu_data[] handling

[PATCH] x86/SVM: correct boot time cpu_data[] handling

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2535 bytes --]

start_svm() already runs after cpu_data[] was set up, so it shouldn't
modify it anymore (at least not directly). Constify the involved
pointers.

Furthermore LMSLE feature detection was broken by 566ddbe833 ("x86:
Fail CPU bringup cleanly if it cannot initialise HVM"), as Andrew
Cooper has pointed out: c couldn't possibly equal &boot_cpu_data
anymore.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/svm/asid.c
+++ b/xen/arch/x86/hvm/svm/asid.c
@@ -22,7 +22,7 @@
 #include <asm/amd.h>
 #include <asm/hvm/nestedhvm.h>
 
-void svm_asid_init(struct cpuinfo_x86 *c)
+void svm_asid_init(const struct cpuinfo_x86 *c)
 {
     int nasids = 0;
 
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1330,7 +1330,7 @@ static int svm_cpu_up_prepare(unsigned i
     return 0;
 }
 
-static void svm_init_erratum_383(struct cpuinfo_x86 *c)
+static void svm_init_erratum_383(const struct cpuinfo_x86 *c)
 {
     uint64_t msr_content;
 
@@ -1365,11 +1365,12 @@ static int svm_handle_osvw(struct vcpu *
     return 0;
 }
 
-static int svm_cpu_up(void)
+static int _svm_cpu_up(bool bsp)
 {
     uint64_t msr_content;
-    int rc, cpu = smp_processor_id();
-    struct cpuinfo_x86 *c = &cpu_data[cpu];
+    int rc;
+    unsigned int cpu = smp_processor_id();
+    const struct cpuinfo_x86 *c = &cpu_data[cpu];
  
     /* Check whether SVM feature is disabled in BIOS */
     rdmsrl(MSR_K8_VM_CR, msr_content);
@@ -1402,7 +1403,7 @@ static int svm_cpu_up(void)
         rdmsrl(MSR_EFER, msr_content);
     if ( msr_content & EFER_LMSLE )
     {
-        if ( c == &boot_cpu_data )
+        if ( bsp )
             cpu_has_lmsl = 1;
         wrmsrl(MSR_EFER, msr_content ^ EFER_LMSLE);
     }
@@ -1419,13 +1420,18 @@ static int svm_cpu_up(void)
     return 0;
 }
 
+static int svm_cpu_up(void)
+{
+    return _svm_cpu_up(false);
+}
+
 const struct hvm_function_table * __init start_svm(void)
 {
     bool_t printed = 0;
 
     svm_host_osvw_reset();
 
-    if ( svm_cpu_up() )
+    if ( _svm_cpu_up(true) )
     {
         printk("SVM: failed to initialise.\n");
         return NULL;
--- a/xen/include/asm-x86/hvm/svm/asid.h
+++ b/xen/include/asm-x86/hvm/svm/asid.h
@@ -22,7 +22,7 @@
 #include <asm/hvm/asid.h>
 #include <asm/processor.h>
 
-void svm_asid_init(struct cpuinfo_x86 *c);
+void svm_asid_init(const struct cpuinfo_x86 *c);
 
 static inline void svm_asid_g_invlpg(struct vcpu *v, unsigned long g_vaddr)
 {




[-- Attachment #2: SVM-cpu_data.patch --]
[-- Type: text/plain, Size: 2579 bytes --]

x86/SVM: correct boot time cpu_data[] handling

start_svm() already runs after cpu_data[] was set up, so it shouldn't
modify it anymore (at least not directly). Constify the involved
pointers.

Furthermore LMSLE feature detection was broken by 566ddbe833 ("x86:
Fail CPU bringup cleanly if it cannot initialise HVM"), as Andrew
Cooper has pointed out: c couldn't possibly equal &boot_cpu_data
anymore.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/svm/asid.c
+++ b/xen/arch/x86/hvm/svm/asid.c
@@ -22,7 +22,7 @@
 #include <asm/amd.h>
 #include <asm/hvm/nestedhvm.h>
 
-void svm_asid_init(struct cpuinfo_x86 *c)
+void svm_asid_init(const struct cpuinfo_x86 *c)
 {
     int nasids = 0;
 
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1330,7 +1330,7 @@ static int svm_cpu_up_prepare(unsigned i
     return 0;
 }
 
-static void svm_init_erratum_383(struct cpuinfo_x86 *c)
+static void svm_init_erratum_383(const struct cpuinfo_x86 *c)
 {
     uint64_t msr_content;
 
@@ -1365,11 +1365,12 @@ static int svm_handle_osvw(struct vcpu *
     return 0;
 }
 
-static int svm_cpu_up(void)
+static int _svm_cpu_up(bool bsp)
 {
     uint64_t msr_content;
-    int rc, cpu = smp_processor_id();
-    struct cpuinfo_x86 *c = &cpu_data[cpu];
+    int rc;
+    unsigned int cpu = smp_processor_id();
+    const struct cpuinfo_x86 *c = &cpu_data[cpu];
  
     /* Check whether SVM feature is disabled in BIOS */
     rdmsrl(MSR_K8_VM_CR, msr_content);
@@ -1402,7 +1403,7 @@ static int svm_cpu_up(void)
         rdmsrl(MSR_EFER, msr_content);
     if ( msr_content & EFER_LMSLE )
     {
-        if ( c == &boot_cpu_data )
+        if ( bsp )
             cpu_has_lmsl = 1;
         wrmsrl(MSR_EFER, msr_content ^ EFER_LMSLE);
     }
@@ -1419,13 +1420,18 @@ static int svm_cpu_up(void)
     return 0;
 }
 
+static int svm_cpu_up(void)
+{
+    return _svm_cpu_up(false);
+}
+
 const struct hvm_function_table * __init start_svm(void)
 {
     bool_t printed = 0;
 
     svm_host_osvw_reset();
 
-    if ( svm_cpu_up() )
+    if ( _svm_cpu_up(true) )
     {
         printk("SVM: failed to initialise.\n");
         return NULL;
--- a/xen/include/asm-x86/hvm/svm/asid.h
+++ b/xen/include/asm-x86/hvm/svm/asid.h
@@ -22,7 +22,7 @@
 #include <asm/hvm/asid.h>
 #include <asm/processor.h>
 
-void svm_asid_init(struct cpuinfo_x86 *c);
+void svm_asid_init(const struct cpuinfo_x86 *c);
 
 static inline void svm_asid_g_invlpg(struct vcpu *v, unsigned long g_vaddr)
 {

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH] x86/SVM: correct boot time cpu_data[] handling

[Boris Ostrovsky]

On 03/02/2017 08:08 AM, Jan Beulich wrote:
> start_svm() already runs after cpu_data[] was set up, so it shouldn't
> modify it anymore (at least not directly). Constify the involved
> pointers.
>
> Furthermore LMSLE feature detection was broken by 566ddbe833 ("x86:
> Fail CPU bringup cleanly if it cannot initialise HVM"), as Andrew
> Cooper has pointed out: c couldn't possibly equal &boot_cpu_data
> anymore.

I can't find Andrew's email --- why is this not possible?

-boris


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

Re: [PATCH] x86/SVM: correct boot time cpu_data[] handling

[Jan Beulich]

>>> On 02.03.17 at 15:58, <boris.ostrovsky@oracle.com> wrote:
> On 03/02/2017 08:08 AM, Jan Beulich wrote:
>> start_svm() already runs after cpu_data[] was set up, so it shouldn't
>> modify it anymore (at least not directly). Constify the involved
>> pointers.
>>
>> Furthermore LMSLE feature detection was broken by 566ddbe833 ("x86:
>> Fail CPU bringup cleanly if it cannot initialise HVM"), as Andrew
>> Cooper has pointed out: c couldn't possibly equal &boot_cpu_data
>> anymore.
> 
> I can't find Andrew's email --- why is this not possible?

Oh, that was in a private irc chat. The issue is that prior to cited
commit, &boot_cpu_data would have come into the function. The
change done results in the check effectively having become

	&cpu_data[0] == &boot_cpu_data

which is never true.

Jan


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

Re: [PATCH] x86/SVM: correct boot time cpu_data[] handling

[Andrew Cooper]

On 02/03/17 13:08, Jan Beulich wrote:
> start_svm() already runs after cpu_data[] was set up, so it shouldn't
> modify it anymore (at least not directly). Constify the involved
> pointers.
>
> Furthermore LMSLE feature detection was broken by 566ddbe833 ("x86:
> Fail CPU bringup cleanly if it cannot initialise HVM"), as Andrew
> Cooper has pointed out: c couldn't possibly equal &boot_cpu_data
> anymore.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I was hoping to avoid fixing LMSLE this until I had enough CPUID
infrastructure in place to make it migration safe.

OTOH, I can't really object to the patch either.

Would you mind if we #if 0'd the LMSLE bit for now, to avoid introducing
a window where it definitely isn't migration safe?

~Andrew

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

Re: [PATCH] x86/SVM: correct boot time cpu_data[] handling

[Boris Ostrovsky]

On 03/02/2017 10:02 AM, Jan Beulich wrote:
>>>> On 02.03.17 at 15:58, <boris.ostrovsky@oracle.com> wrote:
>> On 03/02/2017 08:08 AM, Jan Beulich wrote:
>>> start_svm() already runs after cpu_data[] was set up, so it shouldn't
>>> modify it anymore (at least not directly). Constify the involved
>>> pointers.
>>>
>>> Furthermore LMSLE feature detection was broken by 566ddbe833 ("x86:
>>> Fail CPU bringup cleanly if it cannot initialise HVM"), as Andrew
>>> Cooper has pointed out: c couldn't possibly equal &boot_cpu_data
>>> anymore.
>> I can't find Andrew's email --- why is this not possible?
> Oh, that was in a private irc chat. The issue is that prior to cited
> commit, &boot_cpu_data would have come into the function. The
> change done results in the check effectively having become
>
> 	&cpu_data[0] == &boot_cpu_data
>
> which is never true.


Can't we use 'cpu==0' as BSP test?

-boris

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

Re: [PATCH] x86/SVM: correct boot time cpu_data[] handling

[Jan Beulich]

>>> On 02.03.17 at 16:19, <boris.ostrovsky@oracle.com> wrote:
> On 03/02/2017 10:02 AM, Jan Beulich wrote:
>>>>> On 02.03.17 at 15:58, <boris.ostrovsky@oracle.com> wrote:
>>> On 03/02/2017 08:08 AM, Jan Beulich wrote:
>>>> start_svm() already runs after cpu_data[] was set up, so it shouldn't
>>>> modify it anymore (at least not directly). Constify the involved
>>>> pointers.
>>>>
>>>> Furthermore LMSLE feature detection was broken by 566ddbe833 ("x86:
>>>> Fail CPU bringup cleanly if it cannot initialise HVM"), as Andrew
>>>> Cooper has pointed out: c couldn't possibly equal &boot_cpu_data
>>>> anymore.
>>> I can't find Andrew's email --- why is this not possible?
>> Oh, that was in a private irc chat. The issue is that prior to cited
>> commit, &boot_cpu_data would have come into the function. The
>> change done results in the check effectively having become
>>
>> 	&cpu_data[0] == &boot_cpu_data
>>
>> which is never true.
> 
> Can't we use 'cpu==0' as BSP test?

While a lot of code does so, it's conceptually wrong, and hence
I'm trying to avoid introducing new instance (also during patch
review): Eventually we will want CPU0 to be offlineable as well,
and with that it could of course also be brought back online.
Linux has gone through this a year or two back, iirc.

Jan


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

Re: [PATCH] x86/SVM: correct boot time cpu_data[] handling

[Jan Beulich]

>>> On 02.03.17 at 16:06, <andrew.cooper3@citrix.com> wrote:
> On 02/03/17 13:08, Jan Beulich wrote:
>> start_svm() already runs after cpu_data[] was set up, so it shouldn't
>> modify it anymore (at least not directly). Constify the involved
>> pointers.
>>
>> Furthermore LMSLE feature detection was broken by 566ddbe833 ("x86:
>> Fail CPU bringup cleanly if it cannot initialise HVM"), as Andrew
>> Cooper has pointed out: c couldn't possibly equal &boot_cpu_data
>> anymore.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I was hoping to avoid fixing LMSLE this until I had enough CPUID
> infrastructure in place to make it migration safe.
> 
> OTOH, I can't really object to the patch either.
> 
> Would you mind if we #if 0'd the LMSLE bit for now, to avoid introducing
> a window where it definitely isn't migration safe?

I can do this, albeit a little reluctantly. I'd prefer "if ( 0 && ...)"
though, to keep the code getting seen by the parser.

Jan


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

Re: [PATCH] x86/SVM: correct boot time cpu_data[] handling

[Andrew Cooper]

On 02/03/17 15:33, Jan Beulich wrote:
>>>> On 02.03.17 at 16:06, <andrew.cooper3@citrix.com> wrote:
>> On 02/03/17 13:08, Jan Beulich wrote:
>>> start_svm() already runs after cpu_data[] was set up, so it shouldn't
>>> modify it anymore (at least not directly). Constify the involved
>>> pointers.
>>>
>>> Furthermore LMSLE feature detection was broken by 566ddbe833 ("x86:
>>> Fail CPU bringup cleanly if it cannot initialise HVM"), as Andrew
>>> Cooper has pointed out: c couldn't possibly equal &boot_cpu_data
>>> anymore.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> I was hoping to avoid fixing LMSLE this until I had enough CPUID
>> infrastructure in place to make it migration safe.
>>
>> OTOH, I can't really object to the patch either.
>>
>> Would you mind if we #if 0'd the LMSLE bit for now, to avoid introducing
>> a window where it definitely isn't migration safe?
> I can do this, albeit a little reluctantly. I'd prefer "if ( 0 && ...)"
> though, to keep the code getting seen by the parser.

Fine by me.  (Along with a TODO, which I will eventually get around to
taking back out).

~Andrew

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