xen: vcpu_info reinit error after 'xl save -c' & 'xl restore' on PVOPS VM which has multi-cpu

If a PVOPS VM has multi-cpu the vcpu_info of cpu0 is the member of the structure HYPERVISOR_shared_info, and the others is not, but after 'xl save -c/restore' the vcpu_info will be reinitialized, the vcpu_info of all the vcpus will be considered as the member of HYPERVISOR_shared_info. This will cause the cpu1 and other cpu keep receiving interrupts, and the cpu0 is waiting them to finish the job.
So we do not reinit the vcpu_info when PVOPS vm is doing 'xl save -c/restore'.

Signed-off-by Charles Ouyang <[email protected]>

---
  arch/x86/xen/suspend.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/xen/suspend.c +0 -0

index d949769..b2bed45 100644

Quoted 17 lines · Ouyang Zhaowei (Charles)

If a PVOPS VM has multi-cpu the vcpu_info of cpu0 is the member of the structure HYPERVISOR_shared_info, and the others is not, but after 'xl save -c/restore' the vcpu_info will be reinitialized, the vcpu_info of all the vcpus will be considered as the member of HYPERVISOR_shared_info. This will cause the cpu1 and other cpu keep receiving interrupts, and the cpu0 is waiting them to finish the job.
So we do not reinit the vcpu_info when PVOPS vm is doing 'xl save -c/restore'.

Signed-off-by Charles Ouyang <[email protected]>

---
  arch/x86/xen/suspend.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/xen/suspend.c +0 -0

index d949769..b2bed45 100644

arch/x86/xen/suspend.c static void xen_hvm_post_suspend(int suspend_cancelled) · line 39

Quoted 9 lines

arch/x86/xen/suspend.c +2 -1

@@ -32,7 +32,8 @@ static void xen_hvm_post_suspend(int suspend_cancelled)
  {
  #ifdef CONFIG_XEN_PVHVM
         int cpu;
-       xen_hvm_init_shared_info();
+       if (!suspend_cancelled)
+               xen_hvm_init_shared_info();
         xen_callback_vector();
         xen_unplug_emulated_devices();
         if (xen_feature(XENFEAT_hvm_safe_pvclock)) {

Do we need to call other routines if suspend is canceled?

Also, if suspend is canceled then we don't do xen_irq_resume() if that's what you meant by "vcpu_info will be reinitialized". Were you referring some other re-initialization?

(The patch itself looks like the right thing to do though).

-boris

.

Quoted 29 lines · Boris Ostrovsky

Quoted 17 lines · Ouyang Zhaowei (Charles)

If a PVOPS VM has multi-cpu the vcpu_info of cpu0 is the member of the structure HYPERVISOR_shared_info, and the others is not, but after 'xl save -c/restore' the vcpu_info will be reinitialized, the vcpu_info of all the vcpus will be considered as the member of HYPERVISOR_shared_info. This will cause the cpu1 and other cpu keep receiving interrupts, and the cpu0 is waiting them to finish the job.
So we do not reinit the vcpu_info when PVOPS vm is doing 'xl save -c/restore'.

Signed-off-by Charles Ouyang <[email protected]>

---
   arch/x86/xen/suspend.c | 3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/xen/suspend.c +0 -0

index d949769..b2bed45 100644

arch/x86/xen/suspend.c static void xen_hvm_post_suspend(int suspend_cancelled) · line 39

Quoted 9 lines

arch/x86/xen/suspend.c +2 -1

@@ -32,7 +32,8 @@ static void xen_hvm_post_suspend(int suspend_cancelled)
   {
   #ifdef CONFIG_XEN_PVHVM
          int cpu;
-       xen_hvm_init_shared_info();
+       if (!suspend_cancelled)
+               xen_hvm_init_shared_info();
          xen_callback_vector();
          xen_unplug_emulated_devices();
          if (xen_feature(XENFEAT_hvm_safe_pvclock)) {

Do we need to call other routines if suspend is canceled?

Quoted 2 lines

Also, if suspend is canceled then we don't do xen_irq_resume() if that's what you meant by "vcpu_info will be reinitialized". Were you referring some other re-initialization?

Hi Boris,

Sorry I didn't make myself clear.

About the "vcpu_info reinitialize", I mean in the function "xen_hvm_init_shared_info()" the pointer "xen_vcpu" will be reset and all point to HYPERVISOR_shared_info->vcpu_info[cpu].

void __ref xen_hvm_init_shared_info(void)
----

1702          * When xen_hvm_init_shared_info is run at boot time only vcpu 0 is
1703          * online but xen_hvm_init_shared_info is run at resume time too and
1704          * in that case multiple vcpus might be online. */

1705 for_each_online_cpu(cpu) {
1706 /* Leave it to be NULL. */
1707 if (cpu >= MAX_VIRT_CPUS)
1708 continue;
1709 per_cpu(xen_vcpu, cpu) = &HYPERVISOR_shared_info->vcpu_info[cpu];
1710 }
1711 }

But on Xen boot the init function "xen_start_kernel" only set the cpu0 to point to HYPERVISOR_shared_info->vcpu_info[0]

asmlinkage __visible void __init xen_start_kernel(void)

----
1563 /* Don't do the full vcpu_info placement stuff until we have a
1564 possible map and a non-dummy shared_info. */
1565 per_cpu(xen_vcpu, 0) = &HYPERVISOR_shared_info->vcpu_info[0];
1566
1567 local_irq_disable();

Other cpus are set to point to "xen_vcpu_info" in function xen_vcpu_setup().

So after xl save -c/restore, the pointer xen_vcpu will be reset in function "xen_hvm_init_shared_info" and point to a wrong place. This may cause all the cpus cannot handle irqs except cpu0, so IMHO it's not necessary to call xen_hvm_init_shared_info again if suspend is canceled.

Quoted 6 lines

(The patch itself looks like the right thing to do though).

-boris

.

Quoted 54 lines · Ouyang Zhaowei (Charles)

Quoted 29 lines · Boris Ostrovsky

Quoted 17 lines · Ouyang Zhaowei (Charles)

If a PVOPS VM has multi-cpu the vcpu_info of cpu0 is the member of the structure HYPERVISOR_shared_info, and the others is not, but after 'xl save -c/restore' the vcpu_info will be reinitialized, the vcpu_info of all the vcpus will be considered as the member of HYPERVISOR_shared_info. This will cause the cpu1 and other cpu keep receiving interrupts, and the cpu0 is waiting them to finish the job.
So we do not reinit the vcpu_info when PVOPS vm is doing 'xl save -c/restore'.

Signed-off-by Charles Ouyang <[email protected]>

---
   arch/x86/xen/suspend.c | 3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/xen/suspend.c +0 -0

index d949769..b2bed45 100644

arch/x86/xen/suspend.c static void xen_hvm_post_suspend(int suspend_cancelled) · line 39

Quoted 9 lines

arch/x86/xen/suspend.c +2 -1

@@ -32,7 +32,8 @@ static void xen_hvm_post_suspend(int suspend_cancelled)
   {
   #ifdef CONFIG_XEN_PVHVM
          int cpu;
-       xen_hvm_init_shared_info();
+       if (!suspend_cancelled)
+               xen_hvm_init_shared_info();
          xen_callback_vector();
          xen_unplug_emulated_devices();
          if (xen_feature(XENFEAT_hvm_safe_pvclock)) {

Do we need to call other routines if suspend is canceled?

Quoted 2 lines

Also, if suspend is canceled then we don't do xen_irq_resume() if that's what you meant by "vcpu_info will be reinitialized". Were you referring some other re-initialization?

Hi Boris,

Sorry I didn't make myself clear.

About the "vcpu_info reinitialize", I mean in the function "xen_hvm_init_shared_info()" the pointer "xen_vcpu" will be reset and all point to HYPERVISOR_shared_info->vcpu_info[cpu].

void __ref xen_hvm_init_shared_info(void)
----

1702          * When xen_hvm_init_shared_info is run at boot time only vcpu 0 is
1703          * online but xen_hvm_init_shared_info is run at resume time too and
1704          * in that case multiple vcpus might be online. */

1705 for_each_online_cpu(cpu) {
1706 /* Leave it to be NULL. */
1707 if (cpu >= MAX_VIRT_CPUS)
1708 continue;
1709 per_cpu(xen_vcpu, cpu) = &HYPERVISOR_shared_info->vcpu_info[cpu];
1710 }
1711 }

Quoted 3 lines

But on Xen boot the init function "xen_start_kernel" only set the cpu0 to point to HYPERVISOR_shared_info->vcpu_info[0]

asmlinkage __visible void __init xen_start_kernel(void)

We are talking about HVM guests here and xen_start_kernel is only called for PV. But even if it was, xen_vcpu pointers for other VCPUs are set in xen_vcpu_setup(), which is called when non-boot VCPUs are coming up.

And I wonder whether the actual problem is that we don't call xen_vcpu_setup() on canceled suspend (as we don't need to, really) and therefore if we call xen_hvm_init_shared_info() then per_cpu(xen_vcpu,cpu) for *non-boot* cpus is will become wrong.

-boris

Quoted 7 lines

----
1563 /* Don't do the full vcpu_info placement stuff until we have a
1564 possible map and a non-dummy shared_info. */
1565 per_cpu(xen_vcpu, 0) = &HYPERVISOR_shared_info->vcpu_info[0];
1566
1567 local_irq_disable();

Quoted 12 lines

Other cpus are set to point to "xen_vcpu_info" in function xen_vcpu_setup().

So after xl save -c/restore, the pointer xen_vcpu will be reset in function "xen_hvm_init_shared_info" and point to a wrong place. This may cause all the cpus cannot handle irqs except cpu0, so IMHO it's not necessary to call xen_hvm_init_shared_info again if suspend is canceled.

Quoted 6 lines

(The patch itself looks like the right thing to do though).

-boris

.

.

Quoted 54 lines · Boris Ostrovsky

Quoted 53 lines · Ouyang Zhaowei (Charles)

Quoted 29 lines · Boris Ostrovsky

Quoted 17 lines · Ouyang Zhaowei (Charles)

If a PVOPS VM has multi-cpu the vcpu_info of cpu0 is the member of the structure HYPERVISOR_shared_info, and the others is not, but after 'xl save -c/restore' the vcpu_info will be reinitialized, the vcpu_info of all the vcpus will be considered as the member of HYPERVISOR_shared_info. This will cause the cpu1 and other cpu keep receiving interrupts, and the cpu0 is waiting them to finish the job.
So we do not reinit the vcpu_info when PVOPS vm is doing 'xl save -c/restore'.

Signed-off-by Charles Ouyang <[email protected]>

---
    arch/x86/xen/suspend.c | 3 ++-
    1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/xen/suspend.c +0 -0

index d949769..b2bed45 100644

arch/x86/xen/suspend.c static void xen_hvm_post_suspend(int suspend_cancelled) · line 39

Quoted 9 lines

arch/x86/xen/suspend.c +2 -1

@@ -32,7 +32,8 @@ static void xen_hvm_post_suspend(int suspend_cancelled)
    {
    #ifdef CONFIG_XEN_PVHVM
           int cpu;
-       xen_hvm_init_shared_info();
+       if (!suspend_cancelled)
+               xen_hvm_init_shared_info();
           xen_callback_vector();
           xen_unplug_emulated_devices();
           if (xen_feature(XENFEAT_hvm_safe_pvclock)) {

Do we need to call other routines if suspend is canceled?

Quoted 2 lines

Also, if suspend is canceled then we don't do xen_irq_resume() if that's what you meant by "vcpu_info will be reinitialized". Were you referring some other re-initialization?

Hi Boris,

Sorry I didn't make myself clear.

About the "vcpu_info reinitialize", I mean in the function "xen_hvm_init_shared_info()" the pointer "xen_vcpu" will be reset and all point to HYPERVISOR_shared_info->vcpu_info[cpu].

void __ref xen_hvm_init_shared_info(void)
----

1702          * When xen_hvm_init_shared_info is run at boot time only vcpu 0 is
1703          * online but xen_hvm_init_shared_info is run at resume time too and
1704          * in that case multiple vcpus might be online. */

1705 for_each_online_cpu(cpu) {
1706 /* Leave it to be NULL. */
1707 if (cpu >= MAX_VIRT_CPUS)
1708 continue;
1709 per_cpu(xen_vcpu, cpu) = &HYPERVISOR_shared_info->vcpu_info[cpu];
1710 }
1711 }

Quoted 8 lines

Quoted 3 lines

But on Xen boot the init function "xen_start_kernel" only set the cpu0 to point to HYPERVISOR_shared_info->vcpu_info[0]

asmlinkage __visible void __init xen_start_kernel(void)

We are talking about HVM guests here and xen_start_kernel is only called for PV. But even if it was, xen_vcpu pointers for other VCPUs are set in xen_vcpu_setup(), which is called when non-boot VCPUs are coming up.

And I wonder whether the actual problem is that we don't call xen_vcpu_setup() on canceled suspend (as we don't need to, really) and therefore if we call xen_hvm_init_shared_info() then per_cpu(xen_vcpu,cpu) for *non-boot* cpus is will become wrong.

Yes, you are right, in xen_vcpu_setup() non-boot VCPUs is set to point to xen_vcpu_info

static void xen_vcpu_setup(int cpu)
----
208 vcpup = &per_cpu(xen_vcpu_info, cpu);
...
227 /* This cpu is using the registered vcpu info, even if
228 later ones fail to. */
229 per_cpu(xen_vcpu, cpu) = vcpup;

But on canceled suspend if we call xen_hvm_init_shared_info(), the non-boot VCPUS will be reset to point to HYPERVISOR_shared_info->vcpu_info[cpu] which is a wrong address. So I suggest we don't call xen_hvm_init_shared_info() when suspend is canceled.

Quoted 25 lines

-boris

Quoted 7 lines

----
1563 /* Don't do the full vcpu_info placement stuff until we have a
1564 possible map and a non-dummy shared_info. */
1565 per_cpu(xen_vcpu, 0) = &HYPERVISOR_shared_info->vcpu_info[0];
1566
1567 local_irq_disable();

Quoted 12 lines

Other cpus are set to point to "xen_vcpu_info" in function xen_vcpu_setup().

So after xl save -c/restore, the pointer xen_vcpu will be reset in function "xen_hvm_init_shared_info" and point to a wrong place. This may cause all the cpus cannot handle irqs except cpu0, so IMHO it's not necessary to call xen_hvm_init_shared_info again if suspend is canceled.

Quoted 6 lines

(The patch itself looks like the right thing to do though).

-boris

.

.

Quoted 74 lines · Ouyang Zhaowei (Charles)

Quoted 54 lines · Boris Ostrovsky

Quoted 53 lines · Ouyang Zhaowei (Charles)

Quoted 29 lines · Boris Ostrovsky

Quoted 17 lines · Ouyang Zhaowei (Charles)

If a PVOPS VM has multi-cpu the vcpu_info of cpu0 is the member of the structure HYPERVISOR_shared_info, and the others is not, but after 'xl save -c/restore' the vcpu_info will be reinitialized, the vcpu_info of all the vcpus will be considered as the member of HYPERVISOR_shared_info. This will cause the cpu1 and other cpu keep receiving interrupts, and the cpu0 is waiting them to finish the job.
So we do not reinit the vcpu_info when PVOPS vm is doing 'xl save -c/restore'.

Signed-off-by Charles Ouyang <[email protected]>

---
    arch/x86/xen/suspend.c | 3 ++-
    1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/xen/suspend.c +0 -0

index d949769..b2bed45 100644

arch/x86/xen/suspend.c static void xen_hvm_post_suspend(int suspend_cancelled) · line 39

Quoted 9 lines

arch/x86/xen/suspend.c +2 -1

@@ -32,7 +32,8 @@ static void xen_hvm_post_suspend(int suspend_cancelled)
    {
    #ifdef CONFIG_XEN_PVHVM
           int cpu;
-       xen_hvm_init_shared_info();
+       if (!suspend_cancelled)
+               xen_hvm_init_shared_info();
           xen_callback_vector();
           xen_unplug_emulated_devices();
           if (xen_feature(XENFEAT_hvm_safe_pvclock)) {

Do we need to call other routines if suspend is canceled?

Quoted 2 lines

Also, if suspend is canceled then we don't do xen_irq_resume() if that's what you meant by "vcpu_info will be reinitialized". Were you referring some other re-initialization?

Hi Boris,

Sorry I didn't make myself clear.

About the "vcpu_info reinitialize", I mean in the function "xen_hvm_init_shared_info()" the pointer "xen_vcpu" will be reset and all point to HYPERVISOR_shared_info->vcpu_info[cpu].

void __ref xen_hvm_init_shared_info(void)
----

1702          * When xen_hvm_init_shared_info is run at boot time only vcpu 0 is
1703          * online but xen_hvm_init_shared_info is run at resume time too and
1704          * in that case multiple vcpus might be online. */

1705 for_each_online_cpu(cpu) {
1706 /* Leave it to be NULL. */
1707 if (cpu >= MAX_VIRT_CPUS)
1708 continue;
1709 per_cpu(xen_vcpu, cpu) = &HYPERVISOR_shared_info->vcpu_info[cpu];
1710 }
1711 }

Quoted 8 lines

Quoted 3 lines

But on Xen boot the init function "xen_start_kernel" only set the cpu0 to point to HYPERVISOR_shared_info->vcpu_info[0]

asmlinkage __visible void __init xen_start_kernel(void)

We are talking about HVM guests here and xen_start_kernel is only called for PV. But even if it was, xen_vcpu pointers for other VCPUs are set in xen_vcpu_setup(), which is called when non-boot VCPUs are coming up.

And I wonder whether the actual problem is that we don't call xen_vcpu_setup() on canceled suspend (as we don't need to, really) and therefore if we call xen_hvm_init_shared_info() then per_cpu(xen_vcpu,cpu) for *non-boot* cpus is will become wrong.

Yes, you are right, in xen_vcpu_setup() non-boot VCPUs is set to point to xen_vcpu_info

static void xen_vcpu_setup(int cpu)
----
208 vcpup = &per_cpu(xen_vcpu_info, cpu);
...
227 /* This cpu is using the registered vcpu info, even if
228 later ones fail to. */
229 per_cpu(xen_vcpu, cpu) = vcpup;

Quoted 2 lines

But on canceled suspend if we call xen_hvm_init_shared_info(), the non-boot VCPUS will be reset to point to HYPERVISOR_shared_info->vcpu_info[cpu] which is a wrong address. So I suggest we don't call xen_hvm_init_shared_info() when suspend is canceled.

Right, so can you resubmit the patch with updated commit message? (Just note there that the hypervisor continues assuming that vcpu_info is stored in per-cpu data which was set up by xen_vcpu_setup(), while the call to xen_hvm_init_shared_info() will now make the guest think that vcpu_info is in the shared page).

Thanks.
-boris

Quoted 19 lines

Quoted 19 lines

-boris

Quoted 15 lines

----
1563 /* Don't do the full vcpu_info placement stuff until we have a
1564 possible map and a non-dummy shared_info. */
1565 per_cpu(xen_vcpu, 0) = &HYPERVISOR_shared_info->vcpu_info[0];
1566
1567 local_irq_disable();

Other cpus are set to point to "xen_vcpu_info" in function xen_vcpu_setup().

So after xl save -c/restore, the pointer xen_vcpu will be reset in function "xen_hvm_init_shared_info" and point to a wrong place. This may cause all the cpus cannot handle irqs except cpu0, so IMHO it's not necessary to call xen_hvm_init_shared_info again if suspend is canceled.

Quoted 2 lines

(The patch itself looks like the right thing to do though).

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

Quoted 4 lines

-boris

.

.

.