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