xen: vcpu_info would be reset to wrong place on canceled suspend on PVOPS VM which has multi-cpu

PATCH RESEND Superseded Ouyang Zhaowei (Charles) 2 participants 5 messages May 4, 2015 – May 6, 2015

Subsystems (2)

x86 Xen hypervisor x86

lore

Share & tools

lore

Ouyang Zhaowei (Charles)

author

2015-05-04 02:56 UTC #0 lore raw

PATCH RESEND 1/1 xen: vcpu_info would be reset to wrong place on canceled suspend on PVOPS VM which has multi-cpu

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

Reviewed-by Boris Ostrovsky <[email protected]>

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

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

index d949769..b2bed45 100644
@@ -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)) {

David Vrabel

2015-05-05 09:11 UTC #1 lore raw

Quoted 3 lines · Ouyang Zhaowei (Charles)

The hypervisor continues assuming that vcpu_info is stored in per-cpu data which was set up by xen_vcpu_setup(), while on canceled suspend, the call to xen_hvm_init_shared_info() will now make the guest think that vcpu_info is in the shared page, so we do not call xen_hvm_init_shared_info() on suspend canceled.

I don't understand what the impact of this fix is. Can you include this in the commit message?

David

Quoted 21 lines

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

Reviewed-by Boris Ostrovsky <[email protected]>

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

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

index d949769..b2bed45 100644
@@ -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)) {

Ouyang Zhaowei (Charles)

author

2015-05-05 13:20 UTC #2 lore raw

Quoted 9 lines · David Vrabel

Quoted 3 lines · Ouyang Zhaowei (Charles)
The hypervisor continues assuming that vcpu_info is stored in per-cpu data which was set up by xen_vcpu_setup(), while on canceled suspend, the call to xen_hvm_init_shared_info() will now make the guest think that vcpu_info is in the shared page, so we do not call xen_hvm_init_shared_info() on suspend canceled.
I don't understand what the impact of this fix is. Can you include this in the commit message?
David

Sure, please let me know if there is anything not clear enough.

vcpu_info would be reset to wrong place on canceled suspend on PVOPS VM which has multi-cpu

The hypervisor continues assuming that vcpu_info is stored in per-cpu data which was set up by xen_vcpu_setup(), while on canceled suspend, the call to xen_hvm_init_shared_info() will make the guest think that vcpu_info is in the shared page. This will cause non-boot cpus keep checking a wrong vcpu_info address and can not handle irqs any more after suspend canceled, for example after we do "xl save -c/restore", PVOPS VM can not be resumed as the cpus can not finish their irqs.

Quoted 26 lines

Quoted 13 lines

Signed-off-by Charles Ouyang <[email protected]>
Reviewed-by Boris Ostrovsky <[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)) {

David Vrabel

2015-05-05 17:21 UTC #3 lore raw

Quoted 15 lines · Ouyang Zhaowei (Charles)

Quoted 9 lines · David Vrabel
Quoted 3 lines · Ouyang Zhaowei (Charles)
The hypervisor continues assuming that vcpu_info is stored in per-cpu data which was set up by xen_vcpu_setup(), while on canceled suspend, the call to xen_hvm_init_shared_info() will now make the guest think that vcpu_info is in the shared page, so we do not call xen_hvm_init_shared_info() on suspend canceled.
I don't understand what the impact of this fix is. Can you include this in the commit message?
David
Sure, please let me know if there is anything not clear enough.

Quoted 7 lines

vcpu_info would be reset to wrong place on canceled suspend on PVOPS VM which has multi-cpu
The hypervisor continues assuming that vcpu_info is stored in per-cpu data which was set up by xen_vcpu_setup(), while on canceled suspend, the call to xen_hvm_init_shared_info() will make the guest think that vcpu_info is in the shared page. This will cause non-boot cpus keep checking a wrong vcpu_info address and can not handle irqs any more after suspend canceled, for example after we do "xl save -c/restore", PVOPS VM can not be resumed as the cpus can not finish their irqs.

Ok, so it's only cancelled suspends that are affected. Can you make the clear in the commit and resubmit. This looks like something that should be tagged for stable as well.

David

Quoted 28 lines

Quoted 27 lines
Quoted 14 lines
Signed-off-by Charles Ouyang <[email protected]>
Reviewed-by Boris Ostrovsky <[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)) {
.

Ouyang Zhaowei (Charles)

author

2015-05-06 01:41 UTC #4 lore raw

Quoted 24 lines · David Vrabel

Quoted 15 lines · Ouyang Zhaowei (Charles)
Quoted 9 lines · David Vrabel
Quoted 3 lines · Ouyang Zhaowei (Charles)
The hypervisor continues assuming that vcpu_info is stored in per-cpu data which was set up by xen_vcpu_setup(), while on canceled suspend, the call to xen_hvm_init_shared_info() will now make the guest think that vcpu_info is in the shared page, so we do not call xen_hvm_init_shared_info() on suspend canceled.
I don't understand what the impact of this fix is. Can you include this in the commit message?
David
Sure, please let me know if there is anything not clear enough.
Quoted 7 lines
vcpu_info would be reset to wrong place on canceled suspend on PVOPS VM which has multi-cpu
The hypervisor continues assuming that vcpu_info is stored in per-cpu data which was set up by xen_vcpu_setup(), while on canceled suspend, the call to xen_hvm_init_shared_info() will make the guest think that vcpu_info is in the shared page. This will cause non-boot cpus keep checking a wrong vcpu_info address and can not handle irqs any more after suspend canceled, for example after we do "xl save -c/restore", PVOPS VM can not be resumed as the cpus can not finish their irqs.

Quoted 5 lines

Ok, so it's only cancelled suspends that are affected. Can you make the clear in the commit and resubmit. This looks like something that should be tagged for stable as well.
David

OK, thanks

Quoted 33 lines

Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Signed-off-by Charles Ouyang <[email protected]>
Reviewed-by Boris Ostrovsky <[email protected]>
---
 arch/x86/xen/suspend.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/xen/suspend.c +2 -1
index d949769..b2bed45 100644
@@ -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();
Quoted 9 lines
Quoted 8 lines
arch/x86/xen/suspend.c static void xen_hvm_post_suspend(int suspend_cancelled) · line 39
Quoted 4 lines
arch/x86/xen/suspend.c +0 -0
        xen_callback_vector();
        xen_unplug_emulated_devices();
        if (xen_feature(XENFEAT_hvm_safe_pvclock)) {
.
.