[PATCH v3] HV: fix "missing for discarded return value" for vm related api


Victor Sun
 

- add handler if prepare_vm0() failed;

- remove assert in start_vm() and return -1 if failed;

changelog:
v2 -> v3: replace panic with pr_fatal and return directly
if prepare_vm0() failed;
v1 -> v2: panic if prepare_vm0() failed instead of reboot system;

Signed-off-by: Victor Sun <victor.sun@...>
---
hypervisor/arch/x86/cpu.c | 9 ++++++---
hypervisor/arch/x86/guest/vm.c | 7 ++++---
hypervisor/common/hypercall.c | 3 +--
3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 02cbaec..6944592 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -563,9 +563,12 @@ static void bsp_boot_post(void)

exec_vmxon_instr(BOOT_CPU_ID);

- prepare_vm(BOOT_CPU_ID);
-
- default_idle();
+ if (prepare_vm0() == 0) {
+ default_idle();
+ } else {
+ pr_fatal("Prepare VM0 failed!");
+ return;
+ }

/* Control should not come here */
cpu_dead(BOOT_CPU_ID);
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index 09ceadb..09d9fc0 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -331,7 +331,9 @@ int start_vm(struct vm *vm)

/* Only start BSP (vid = 0) and let BSP start other APs */
vcpu = vcpu_from_vid(vm, 0U);
- ASSERT(vcpu != NULL, "vm%d, vcpu0", vm->vm_id);
+ if (vcpu == NULL) {
+ return -1;
+ }
schedule_vcpu(vcpu);

return 0;
@@ -358,8 +360,7 @@ int reset_vm(struct vm *vm)

vioapic_reset(vm->arch_vm.virt_ioapic);

- start_vm(vm);
- return 0;
+ return (start_vm(vm));
}

/**
diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index f8efb35..7489b99 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -280,8 +280,7 @@ int32_t hcall_reset_vm(uint16_t vmid)
if ((target_vm == NULL) || is_vm0(target_vm))
return -1;

- reset_vm(target_vm);
- return 0;
+ return (reset_vm(target_vm));
}

int32_t hcall_assert_irqline(struct vm *vm, uint16_t vmid, uint64_t param)
--
2.7.4


Victor Sun
 

Hi Eddie/Anthony,

Do you have any comments on this one?

On 8/10/2018 11:31 AM, Victor Sun wrote:
- add handler if prepare_vm0() failed;

- remove assert in start_vm() and return -1 if failed;

changelog:
v2 -> v3: replace panic with pr_fatal and return directly
if prepare_vm0() failed;
v1 -> v2: panic if prepare_vm0() failed instead of reboot system;

Signed-off-by: Victor Sun <victor.sun@...>
---
hypervisor/arch/x86/cpu.c | 9 ++++++---
hypervisor/arch/x86/guest/vm.c | 7 ++++---
hypervisor/common/hypercall.c | 3 +--
3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 02cbaec..6944592 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -563,9 +563,12 @@ static void bsp_boot_post(void)
exec_vmxon_instr(BOOT_CPU_ID);
- prepare_vm(BOOT_CPU_ID);
-
- default_idle();
+ if (prepare_vm0() == 0) {
+ default_idle();
+ } else {
+ pr_fatal("Prepare VM0 failed!");
+ return;
+ }
/* Control should not come here */
cpu_dead(BOOT_CPU_ID);
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index 09ceadb..09d9fc0 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -331,7 +331,9 @@ int start_vm(struct vm *vm)
/* Only start BSP (vid = 0) and let BSP start other APs */
vcpu = vcpu_from_vid(vm, 0U);
- ASSERT(vcpu != NULL, "vm%d, vcpu0", vm->vm_id);
+ if (vcpu == NULL) {
+ return -1;
+ }
schedule_vcpu(vcpu);
return 0;
@@ -358,8 +360,7 @@ int reset_vm(struct vm *vm)
vioapic_reset(vm->arch_vm.virt_ioapic);
- start_vm(vm);
- return 0;
+ return (start_vm(vm));
}
/**
diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index f8efb35..7489b99 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -280,8 +280,7 @@ int32_t hcall_reset_vm(uint16_t vmid)
if ((target_vm == NULL) || is_vm0(target_vm))
return -1;
- reset_vm(target_vm);
- return 0;
+ return (reset_vm(target_vm));
}
int32_t hcall_assert_irqline(struct vm *vm, uint16_t vmid, uint64_t param)


Eddie Dong
 

-----Original Message-----
From: acrn-dev@...
[mailto:acrn-dev@...] On Behalf Of Victor Sun
Sent: Friday, August 10, 2018 11:32 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v3] HV: fix "missing for discarded return value"
for vm related api

- add handler if prepare_vm0() failed;

- remove assert in start_vm() and return -1 if failed;

changelog:
v2 -> v3: replace panic with pr_fatal and return directly
if prepare_vm0() failed;
v1 -> v2: panic if prepare_vm0() failed instead of reboot system;

Signed-off-by: Victor Sun <victor.sun@...>
---
hypervisor/arch/x86/cpu.c | 9 ++++++---
hypervisor/arch/x86/guest/vm.c | 7 ++++---
hypervisor/common/hypercall.c | 3 +--
3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c index
02cbaec..6944592 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -563,9 +563,12 @@ static void bsp_boot_post(void)

exec_vmxon_instr(BOOT_CPU_ID);

- prepare_vm(BOOT_CPU_ID);
-
- default_idle();
+ if (prepare_vm0() == 0) {
Why you want to change from prepare_vm to prepare_vm0? This is obvious issue.

+ default_idle();
+ } else {
+ pr_fatal("Prepare VM0 failed!");
+ return;
+ }

/* Control should not come here */
cpu_dead(BOOT_CPU_ID);
diff --git a/hypervisor/arch/x86/guest/vm.c
b/hypervisor/arch/x86/guest/vm.c index 09ceadb..09d9fc0 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -331,7 +331,9 @@ int start_vm(struct vm *vm)

/* Only start BSP (vid = 0) and let BSP start other APs */
vcpu = vcpu_from_vid(vm, 0U);
- ASSERT(vcpu != NULL, "vm%d, vcpu0", vm->vm_id);
+ if (vcpu == NULL) {
+ return -1;
Why -1 ?

+ }
schedule_vcpu(vcpu);

return 0;
@@ -358,8 +360,7 @@ int reset_vm(struct vm *vm)

vioapic_reset(vm->arch_vm.virt_ioapic);

- start_vm(vm);
- return 0;
+ return (start_vm(vm));
I remember FuSa wants to split them into 2 lines of instructions.
}

/**
diff --git a/hypervisor/common/hypercall.c
b/hypervisor/common/hypercall.c index f8efb35..7489b99 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -280,8 +280,7 @@ int32_t hcall_reset_vm(uint16_t vmid)
if ((target_vm == NULL) || is_vm0(target_vm))
return -1;

- reset_vm(target_vm);
- return 0;
+ return (reset_vm(target_vm));
}

int32_t hcall_assert_irqline(struct vm *vm, uint16_t vmid, uint64_t param)
--
2.7.4



Victor Sun
 

On 8/13/2018 5:58 PM, Eddie Dong wrote:

-----Original Message-----
From: acrn-dev@...
[mailto:acrn-dev@...] On Behalf Of Victor Sun
Sent: Friday, August 10, 2018 11:32 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v3] HV: fix "missing for discarded return value"
for vm related api

- add handler if prepare_vm0() failed;

- remove assert in start_vm() and return -1 if failed;

changelog:
v2 -> v3: replace panic with pr_fatal and return directly
if prepare_vm0() failed;
v1 -> v2: panic if prepare_vm0() failed instead of reboot system;

Signed-off-by: Victor Sun <victor.sun@...>
---
hypervisor/arch/x86/cpu.c | 9 ++++++---
hypervisor/arch/x86/guest/vm.c | 7 ++++---
hypervisor/common/hypercall.c | 3 +--
3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c index
02cbaec..6944592 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -563,9 +563,12 @@ static void bsp_boot_post(void)

exec_vmxon_instr(BOOT_CPU_ID);

- prepare_vm(BOOT_CPU_ID);
-
- default_idle();
+ if (prepare_vm0() == 0) {
Why you want to change from prepare_vm to prepare_vm0? This is obvious issue.
OK, this is my fault. I am not aware the code here has been changed to prepare_vm() recently.
I will fix it in my next version.


+ default_idle();
+ } else {
+ pr_fatal("Prepare VM0 failed!");
+ return;
+ }

/* Control should not come here */
cpu_dead(BOOT_CPU_ID);
diff --git a/hypervisor/arch/x86/guest/vm.c
b/hypervisor/arch/x86/guest/vm.c index 09ceadb..09d9fc0 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -331,7 +331,9 @@ int start_vm(struct vm *vm)

/* Only start BSP (vid = 0) and let BSP start other APs */
vcpu = vcpu_from_vid(vm, 0U);
- ASSERT(vcpu != NULL, "vm%d, vcpu0", vm->vm_id);
+ if (vcpu == NULL) {
+ return -1;
Why -1 ?
The purpose here is want to return a failure.
Is it better to return a error code of "-EFAULT"?

+ }
schedule_vcpu(vcpu);

return 0;
@@ -358,8 +360,7 @@ int reset_vm(struct vm *vm)

vioapic_reset(vm->arch_vm.virt_ioapic);

- start_vm(vm);
- return 0;
+ return (start_vm(vm));
I remember FuSa wants to split them into 2 lines of instructions.
Anthony also has such concern. I already checked with Junjie on this and Junjie confirmed it is safe.
Please refer his comments on patch V2.

BR,
Victor
}

/**
diff --git a/hypervisor/common/hypercall.c
b/hypervisor/common/hypercall.c index f8efb35..7489b99 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -280,8 +280,7 @@ int32_t hcall_reset_vm(uint16_t vmid)
if ((target_vm == NULL) || is_vm0(target_vm))
return -1;

- reset_vm(target_vm);
- return 0;
+ return (reset_vm(target_vm));
}

int32_t hcall_assert_irqline(struct vm *vm, uint16_t vmid, uint64_t param)
--
2.7.4