[PATCH v7 5/5] HV: Reset physical core of lapic_pt vm when shutdown


Kaige Fu
 

The physical core of lapic_pt vm should be reset for security and
correctness when shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/guest/vm.c | 20 +++++++++++++++++++-
hypervisor/include/lib/errno.h | 2 ++
2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index cebbc9cc..8a66973c 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -446,9 +446,10 @@ int32_t create_vm(uint16_t vm_id, struct acrn_vm_config *vm_config, struct acrn_
int32_t shutdown_vm(struct acrn_vm *vm)
{
uint16_t i;
+ uint64_t mask = 0UL;
struct acrn_vcpu *vcpu = NULL;
struct acrn_vm_config *vm_config = NULL;
- int32_t ret;
+ int32_t ret = 0;

pause_vm(vm);

@@ -459,6 +460,23 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ if (is_lapic_pt(vm)) {
+ bitmap_set_nolock(vcpu->pcpu_id, &mask);
+ }
+ }
+
+ if (is_lapic_pt(vm) && !stop_cpus(mask)) {
+ pr_fatal("Failed to stop all cpus in mask(0x%llx)", mask);
+ }
+
+ /*
+ * No matter whether there are cpus failed to offline, try to start all
+ * cpus in mask anyway.
+ */
+ if (is_lapic_pt(vm) && !start_cpus(mask)) {
+ pr_fatal("Failed to start all cpus in mask(0x%llx)", mask);
+ ret = -ETIMEDOUT;
}

vm_config = get_vm_config(vm->vm_id);
diff --git a/hypervisor/include/lib/errno.h b/hypervisor/include/lib/errno.h
index b97112f5..bc8c0db7 100644
--- a/hypervisor/include/lib/errno.h
+++ b/hypervisor/include/lib/errno.h
@@ -23,5 +23,7 @@
#define ENODEV 19
/** Indicates that argument is not valid. */
#define EINVAL 22
+/** Indicates that timeout occurs. */
+#define ETIMEDOUT 110

#endif /* ERRNO_H */
--
2.20.0


Eddie Dong
 

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Kaige Fu
Sent: Friday, April 19, 2019 2:11 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v7 5/5] HV: Reset physical core of lapic_pt vm
when shutdown

The physical core of lapic_pt vm should be reset for security and correctness
when shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/guest/vm.c | 20 +++++++++++++++++++-
hypervisor/include/lib/errno.h | 2 ++
2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index cebbc9cc..8a66973c 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -446,9 +446,10 @@ int32_t create_vm(uint16_t vm_id, struct
acrn_vm_config *vm_config, struct acrn_ int32_t shutdown_vm(struct
acrn_vm *vm) {
uint16_t i;
+ uint64_t mask = 0UL;
struct acrn_vcpu *vcpu = NULL;
struct acrn_vm_config *vm_config = NULL;
- int32_t ret;
+ int32_t ret = 0;

pause_vm(vm);

@@ -459,6 +460,23 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ if (is_lapic_pt(vm)) {
+ bitmap_set_nolock(vcpu->pcpu_id, &mask);
3rd time comment: We should call make_pcpu_offline(..) here.
If the comments are simply ignored, I don't need to review further.


+ }
+ }
+
+ if (is_lapic_pt(vm) && !stop_cpus(mask)) {
+ pr_fatal("Failed to stop all cpus in mask(0x%llx)", mask);
+ }
+
+ /*
+ * No matter whether there are cpus failed to offline, try to start all
+ * cpus in mask anyway.
+ */
+ if (is_lapic_pt(vm) && !start_cpus(mask)) {
+ pr_fatal("Failed to start all cpus in mask(0x%llx)", mask);
+ ret = -ETIMEDOUT;
}

vm_config = get_vm_config(vm->vm_id); diff --git
a/hypervisor/include/lib/errno.h b/hypervisor/include/lib/errno.h index
b97112f5..bc8c0db7 100644
--- a/hypervisor/include/lib/errno.h
+++ b/hypervisor/include/lib/errno.h
@@ -23,5 +23,7 @@
#define ENODEV 19
/** Indicates that argument is not valid. */
#define EINVAL 22
+/** Indicates that timeout occurs. */
+#define ETIMEDOUT 110

#endif /* ERRNO_H */
--
2.20.0



Kaige Fu
 

Hi Eddie,

On 04-18 Thu 13:23, Eddie Dong wrote:


-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Kaige Fu
Sent: Friday, April 19, 2019 2:11 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v7 5/5] HV: Reset physical core of lapic_pt vm
when shutdown

The physical core of lapic_pt vm should be reset for security and correctness
when shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/guest/vm.c | 20 +++++++++++++++++++-
hypervisor/include/lib/errno.h | 2 ++
2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index cebbc9cc..8a66973c 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -446,9 +446,10 @@ int32_t create_vm(uint16_t vm_id, struct
acrn_vm_config *vm_config, struct acrn_ int32_t shutdown_vm(struct
acrn_vm *vm) {
uint16_t i;
+ uint64_t mask = 0UL;
struct acrn_vcpu *vcpu = NULL;
struct acrn_vm_config *vm_config = NULL;
- int32_t ret;
+ int32_t ret = 0;

pause_vm(vm);

@@ -459,6 +460,23 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ if (is_lapic_pt(vm)) {
+ bitmap_set_nolock(vcpu->pcpu_id, &mask);
3rd time comment: We should call make_pcpu_offline(..) here.
If the comments are simply ignored, I don't need to review further.
I got your comment. But here is what I think: I refine the stop_cpus to take one more parameter
'mask'. So, we can reuse the stop_cpus here directly instead of doing like the following:

1. make_pcpu_offline()
2. wait the pcpus offline.

These above logic is already in stop_cpus. IMHO, it is better to reuse stop_cpus. Otherwise, we have duplicated code here.

If you prefer make_pcpu_offline and wait here, I will do it in next version. Sorry for confusing.


+ }
+ }
+
+ if (is_lapic_pt(vm) && !stop_cpus(mask)) {
+ pr_fatal("Failed to stop all cpus in mask(0x%llx)", mask);
+ }
+
+ /*
+ * No matter whether there are cpus failed to offline, try to start all
+ * cpus in mask anyway.
+ */
+ if (is_lapic_pt(vm) && !start_cpus(mask)) {
+ pr_fatal("Failed to start all cpus in mask(0x%llx)", mask);
+ ret = -ETIMEDOUT;
}

vm_config = get_vm_config(vm->vm_id); diff --git
a/hypervisor/include/lib/errno.h b/hypervisor/include/lib/errno.h index
b97112f5..bc8c0db7 100644
--- a/hypervisor/include/lib/errno.h
+++ b/hypervisor/include/lib/errno.h
@@ -23,5 +23,7 @@
#define ENODEV 19
/** Indicates that argument is not valid. */
#define EINVAL 22
+/** Indicates that timeout occurs. */
+#define ETIMEDOUT 110

#endif /* ERRNO_H */
--
2.20.0





Kaige Fu
 

Hi Eddie,

In current stop_cpus, it do like this:

1) make_pcpu_offline (all APs)
2) Wait for all APs offlined
3) If timeout occurs, dead loop

For reset_cpus in shutdown_vm, you commented that we should put the make_pcpu_offline and wait action in the shutdown_vm,

1) make_pcpu_offline (all cpus of lapic_pt vm)
2) Wait for all cpus offlined
3) if timeout occurs, mark cpu as INVALID
4) start_cpus.

Step 1/2/3 of stop_cpus and reset_cpus is similar. So, what I think is refine the stop_cpus to take one parameter 'mask'
like start_cpus. Then we can reuse the stop_cpus when reseting cpu of lapic_pt vm.

What is your opinion?

BTW, sorry for not making myself clear and leave you an impression that I simply ignored your comment.

On 04-19 Fri 16:44, Kaige Fu wrote:
Hi Eddie,

On 04-18 Thu 13:23, Eddie Dong wrote:


-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Kaige Fu
Sent: Friday, April 19, 2019 2:11 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v7 5/5] HV: Reset physical core of lapic_pt vm
when shutdown

The physical core of lapic_pt vm should be reset for security and correctness
when shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/guest/vm.c | 20 +++++++++++++++++++-
hypervisor/include/lib/errno.h | 2 ++
2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index cebbc9cc..8a66973c 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -446,9 +446,10 @@ int32_t create_vm(uint16_t vm_id, struct
acrn_vm_config *vm_config, struct acrn_ int32_t shutdown_vm(struct
acrn_vm *vm) {
uint16_t i;
+ uint64_t mask = 0UL;
struct acrn_vcpu *vcpu = NULL;
struct acrn_vm_config *vm_config = NULL;
- int32_t ret;
+ int32_t ret = 0;

pause_vm(vm);

@@ -459,6 +460,23 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ if (is_lapic_pt(vm)) {
+ bitmap_set_nolock(vcpu->pcpu_id, &mask);
3rd time comment: We should call make_pcpu_offline(..) here.
If the comments are simply ignored, I don't need to review further.
I got your comment. But here is what I think: I refine the stop_cpus to take one more parameter
'mask'. So, we can reuse the stop_cpus here directly instead of doing like the following:

1. make_pcpu_offline()
2. wait the pcpus offline.

These above logic is already in stop_cpus. IMHO, it is better to reuse stop_cpus. Otherwise, we have duplicated code here.

If you prefer make_pcpu_offline and wait here, I will do it in next version. Sorry for confusing.


+ }
+ }
+
+ if (is_lapic_pt(vm) && !stop_cpus(mask)) {
+ pr_fatal("Failed to stop all cpus in mask(0x%llx)", mask);
+ }
+
+ /*
+ * No matter whether there are cpus failed to offline, try to start all
+ * cpus in mask anyway.
+ */
+ if (is_lapic_pt(vm) && !start_cpus(mask)) {
+ pr_fatal("Failed to start all cpus in mask(0x%llx)", mask);
+ ret = -ETIMEDOUT;
}

vm_config = get_vm_config(vm->vm_id); diff --git
a/hypervisor/include/lib/errno.h b/hypervisor/include/lib/errno.h index
b97112f5..bc8c0db7 100644
--- a/hypervisor/include/lib/errno.h
+++ b/hypervisor/include/lib/errno.h
@@ -23,5 +23,7 @@
#define ENODEV 19
/** Indicates that argument is not valid. */
#define EINVAL 22
+/** Indicates that timeout occurs. */
+#define ETIMEDOUT 110

#endif /* ERRNO_H */
--
2.20.0






Eddie Dong
 


I got your comment. But here is what I think: I refine the stop_cpus to take one
more parameter 'mask'. So, we can reuse the stop_cpus here directly instead
of doing like the following:

1. make_pcpu_offline()
2. wait the pcpus offline.

These above logic is already in stop_cpus. IMHO, it is better to reuse stop_cpus.
Otherwise, we have duplicated code here.

If you prefer make_pcpu_offline and wait here, I will do it in next version. Sorry
for confusing.
We don't need to call stop_cpus again. It is just one line of code that can work as stop_cpus did.
Wait pcpus offline can be a separate API that can be shared.


Kaige Fu
 

On 04-19 Fri 03:40, Eddie Dong wrote:


I got your comment. But here is what I think: I refine the stop_cpus to take one
more parameter 'mask'. So, we can reuse the stop_cpus here directly instead
of doing like the following:

1. make_pcpu_offline()
2. wait the pcpus offline.

These above logic is already in stop_cpus. IMHO, it is better to reuse stop_cpus.
Otherwise, we have duplicated code here.

If you prefer make_pcpu_offline and wait here, I will do it in next version. Sorry
for confusing.
We don't need to call stop_cpus again. It is just one line of code that can work as stop_cpus did.
Wait pcpus offline can be a separate API that can be shared.
Got it. Will do in next version.