From 17a6d9446e79ab7f0cfef1b0212920ab5b069ddc Mon Sep 17 00:00:00 2001 From: Huihuang Shi Date: Wed, 28 Nov 2018 10:24:26 +0800 Subject: [PATCH] hv: guest: fix "Procedure has more than one exit point" IEC 61508,ISO 26262 standards highly recommend single-exit rule. Reduce the count of the "return entries". Fix the violations which is comply with the cases list below: 1.Function has 2 return entries. 2.The first return entry is used to return the error code of checking variable whether is valid. Fix the violations in "if else" format. Tracked-On: #861 Signed-off-by: Huihuang Shi Acked-by: Eddie Dong --- hypervisor/arch/x86/guest/instr_emul.c | 151 +++++++++++++------------ hypervisor/arch/x86/guest/instr_emul.h | 2 +- hypervisor/arch/x86/guest/pm.c | 84 +++++++------- hypervisor/arch/x86/guest/vm.c | 107 +++++++++--------- 4 files changed, 174 insertions(+), 170 deletions(-) diff --git a/hypervisor/arch/x86/guest/instr_emul.c b/hypervisor/arch/x86/guest/instr_emul.c index 82d943e4a..5014c7df4 100644 --- a/hypervisor/arch/x86/guest/instr_emul.c +++ b/hypervisor/arch/x86/guest/instr_emul.c @@ -1462,13 +1462,11 @@ static int emulate_sub(struct acrn_vcpu *vcpu, const struct instr_emul_vie *vie) break; } - if (error != 0) { - return error; + if (error == 0) { + rflags2 = getcc(size, val1, val2); + vie_update_rflags(vcpu, rflags2, RFLAGS_STATUS_BITS); } - rflags2 = getcc(size, val1, val2); - vie_update_rflags(vcpu, rflags2, RFLAGS_STATUS_BITS); - return error; } @@ -1494,11 +1492,12 @@ static int emulate_group1(struct acrn_vcpu *vcpu, const struct instr_emul_vie *v return error; } -static int emulate_bittest(struct acrn_vcpu *vcpu, const struct instr_emul_vie *vie) +static int32_t emulate_bittest(struct acrn_vcpu *vcpu, const struct instr_emul_vie *vie) { uint64_t val, rflags, bitmask; uint64_t bitoff; uint8_t size; + int32_t ret; /* * 0F BA is a Group 8 extended opcode. @@ -1506,31 +1505,32 @@ static int emulate_bittest(struct acrn_vcpu *vcpu, const struct instr_emul_vie * * Currently we only emulate the 'Bit Test' instruction which is * identified by a ModR/M:reg encoding of 100b. */ - if ((vie->reg & 7U) != 4U) { - return -EINVAL; - } + if ((vie->reg & 7U) == 4U) { + rflags = vm_get_register(vcpu, CPU_REG_RFLAGS); - rflags = vm_get_register(vcpu, CPU_REG_RFLAGS); + vie_mmio_read(vcpu, &val); - vie_mmio_read(vcpu, &val); + /* + * Intel SDM, Vol 2, Table 3-2: + * "Range of Bit Positions Specified by Bit Offset Operands" + */ + bitmask = ((uint64_t)vie->opsize * 8UL) - 1UL; + bitoff = (uint64_t)vie->immediate & bitmask; - /* - * Intel SDM, Vol 2, Table 3-2: - * "Range of Bit Positions Specified by Bit Offset Operands" - */ - bitmask = ((uint64_t)vie->opsize * 8UL) - 1UL; - bitoff = (uint64_t)vie->immediate & bitmask; - - /* Copy the bit into the Carry flag in %rflags */ - if ((val & (1UL << bitoff)) != 0U) { - rflags |= PSL_C; + /* Copy the bit into the Carry flag in %rflags */ + if ((val & (1UL << bitoff)) != 0U) { + rflags |= PSL_C; + } else { + rflags &= ~PSL_C; + } + size = 8U; + vie_update_register(vcpu, CPU_REG_RFLAGS, rflags, size); + ret = 0; } else { - rflags &= ~PSL_C; + ret = -EINVAL; } - size = 8U; - vie_update_register(vcpu, CPU_REG_RFLAGS, rflags, size); - return 0; + return ret; } static int vmm_emulate_instruction(struct instr_emul_ctxt *ctxt) @@ -1539,48 +1539,50 @@ static int vmm_emulate_instruction(struct instr_emul_ctxt *ctxt) struct acrn_vcpu *vcpu = ctxt->vcpu; int error; - if (vie->decoded == 0U) { - return -EINVAL; - } - switch (vie->op.op_type) { - case VIE_OP_TYPE_GROUP1: - error = emulate_group1(vcpu, vie); - break; - case VIE_OP_TYPE_CMP: - error = emulate_cmp(vcpu, vie); - break; - case VIE_OP_TYPE_MOV: - error = emulate_mov(vcpu, vie); - break; - case VIE_OP_TYPE_MOVSX: - case VIE_OP_TYPE_MOVZX: - error = emulate_movx(vcpu, vie); - break; - case VIE_OP_TYPE_MOVS: - error = emulate_movs(vcpu, vie); - break; - case VIE_OP_TYPE_STOS: - error = emulate_stos(vcpu, vie); - break; - case VIE_OP_TYPE_AND: - error = emulate_and(vcpu, vie); - break; - case VIE_OP_TYPE_TEST: - error = emulate_test(vcpu, vie); - break; - case VIE_OP_TYPE_OR: - error = emulate_or(vcpu, vie); - break; - case VIE_OP_TYPE_SUB: - error = emulate_sub(vcpu, vie); - break; - case VIE_OP_TYPE_BITTEST: - error = emulate_bittest(vcpu, vie); - break; - default: + if (vie->decoded != 0U) { + switch (vie->op.op_type) { + case VIE_OP_TYPE_GROUP1: + error = emulate_group1(vcpu, vie); + break; + case VIE_OP_TYPE_CMP: + error = emulate_cmp(vcpu, vie); + break; + case VIE_OP_TYPE_MOV: + error = emulate_mov(vcpu, vie); + break; + case VIE_OP_TYPE_MOVSX: + case VIE_OP_TYPE_MOVZX: + error = emulate_movx(vcpu, vie); + break; + case VIE_OP_TYPE_MOVS: + error = emulate_movs(vcpu, vie); + break; + case VIE_OP_TYPE_STOS: + error = emulate_stos(vcpu, vie); + break; + case VIE_OP_TYPE_AND: + error = emulate_and(vcpu, vie); + break; + case VIE_OP_TYPE_TEST: + error = emulate_test(vcpu, vie); + break; + case VIE_OP_TYPE_OR: + error = emulate_or(vcpu, vie); + break; + case VIE_OP_TYPE_SUB: + error = emulate_sub(vcpu, vie); + break; + case VIE_OP_TYPE_BITTEST: + error = emulate_bittest(vcpu, vie); + break; + default: + error = -EINVAL; + break; + } + } else { error = -EINVAL; - break; } + return error; } @@ -2141,19 +2143,21 @@ static int local_decode_instruction(enum vm_cpu_mode cpu_mode, } /* for instruction MOVS/STO, check the gva gotten from DI/SI. */ -static int instr_check_di(struct acrn_vcpu *vcpu, struct instr_emul_ctxt *emul_ctxt) +static int32_t instr_check_di(struct acrn_vcpu *vcpu, struct instr_emul_ctxt *emul_ctxt) { - int ret; + int32_t ret; struct instr_emul_vie *vie = &emul_ctxt->vie; uint64_t gva; ret = get_gva_di_check(vcpu, vie, vie->addrsize, &gva); if (ret < 0) { - return -EFAULT; + ret = -EFAULT; + } else { + ret = 0; } - return 0; + return ret; } static int instr_check_gva(struct acrn_vcpu *vcpu, struct instr_emul_ctxt *emul_ctxt, @@ -2305,14 +2309,17 @@ int decode_instruction(struct acrn_vcpu *vcpu) return (int)(emul_ctxt->vie.opsize); } -int emulate_instruction(const struct acrn_vcpu *vcpu) +int32_t emulate_instruction(const struct acrn_vcpu *vcpu) { struct instr_emul_ctxt *ctxt = &per_cpu(g_inst_ctxt, vcpu->pcpu_id); + int32_t ret; if (ctxt == NULL) { pr_err("%s: Failed to get instr_emul_ctxt", __func__); - return -1; + ret = -1; + } else { + ret = vmm_emulate_instruction(ctxt); } - return vmm_emulate_instruction(ctxt); + return ret; } diff --git a/hypervisor/arch/x86/guest/instr_emul.h b/hypervisor/arch/x86/guest/instr_emul.h index 60398f266..362f164f9 100644 --- a/hypervisor/arch/x86/guest/instr_emul.h +++ b/hypervisor/arch/x86/guest/instr_emul.h @@ -193,7 +193,7 @@ struct instr_emul_ctxt { struct acrn_vcpu *vcpu; }; -int emulate_instruction(const struct acrn_vcpu *vcpu); +int32_t emulate_instruction(const struct acrn_vcpu *vcpu); int decode_instruction(struct acrn_vcpu *vcpu); #endif diff --git a/hypervisor/arch/x86/guest/pm.c b/hypervisor/arch/x86/guest/pm.c index de1731614..f5c56450d 100644 --- a/hypervisor/arch/x86/guest/pm.c +++ b/hypervisor/arch/x86/guest/pm.c @@ -39,21 +39,18 @@ static void vm_setup_cpu_px(struct acrn_vm *vm) (void)memset(vm->pm.px_data, 0U, MAX_PSTATE * sizeof(struct cpu_px_data)); - if ((boot_cpu_data.state_info.px_cnt == 0U) - || (boot_cpu_data.state_info.px_data == NULL)) { - return; + if ((boot_cpu_data.state_info.px_cnt != 0U) + && (boot_cpu_data.state_info.px_data != NULL)) { + ASSERT ((boot_cpu_data.state_info.px_cnt <= MAX_PSTATE), + "failed to setup cpu px"); + + vm->pm.px_cnt = boot_cpu_data.state_info.px_cnt; + + px_data_size = ((uint32_t)vm->pm.px_cnt) * sizeof(struct cpu_px_data); + + (void)memcpy_s(vm->pm.px_data, px_data_size, + boot_cpu_data.state_info.px_data, px_data_size); } - - ASSERT ((boot_cpu_data.state_info.px_cnt <= MAX_PSTATE), - "failed to setup cpu px"); - - vm->pm.px_cnt = boot_cpu_data.state_info.px_cnt; - - px_data_size = ((uint32_t)vm->pm.px_cnt) * sizeof(struct cpu_px_data); - - (void)memcpy_s(vm->pm.px_data, px_data_size, - boot_cpu_data.state_info.px_data, px_data_size); - } static void vm_setup_cpu_cx(struct acrn_vm *vm) @@ -64,24 +61,21 @@ static void vm_setup_cpu_cx(struct acrn_vm *vm) (void)memset(vm->pm.cx_data, 0U, MAX_CSTATE * sizeof(struct cpu_cx_data)); - if ((boot_cpu_data.state_info.cx_cnt == 0U) - || (boot_cpu_data.state_info.cx_data == NULL)) { - return; + if ((boot_cpu_data.state_info.cx_cnt != 0U) + && (boot_cpu_data.state_info.cx_data != NULL)) { + ASSERT ((boot_cpu_data.state_info.cx_cnt <= MAX_CX_ENTRY), + "failed to setup cpu cx"); + + vm->pm.cx_cnt = boot_cpu_data.state_info.cx_cnt; + + cx_data_size = ((uint32_t)vm->pm.cx_cnt) * sizeof(struct cpu_cx_data); + + /* please note pm.cx_data[0] is a empty space holder, + * pm.cx_data[1...MAX_CX_ENTRY] would be used to store cx entry datas. + */ + (void)memcpy_s(vm->pm.cx_data + 1, cx_data_size, + boot_cpu_data.state_info.cx_data, cx_data_size); } - - ASSERT ((boot_cpu_data.state_info.cx_cnt <= MAX_CX_ENTRY), - "failed to setup cpu cx"); - - vm->pm.cx_cnt = boot_cpu_data.state_info.cx_cnt; - - cx_data_size = ((uint32_t)vm->pm.cx_cnt) * sizeof(struct cpu_cx_data); - - /* please note pm.cx_data[0] is a empty space holder, - * pm.cx_data[1...MAX_CX_ENTRY] would be used to store cx entry datas. - */ - (void)memcpy_s(vm->pm.cx_data + 1, cx_data_size, - boot_cpu_data.state_info.cx_data, cx_data_size); - } static inline void init_cx_port(struct acrn_vm *vm) @@ -192,22 +186,20 @@ register_gas_io_handler(struct acrn_vm *vm, uint32_t pio_idx, const struct acpi_ uint8_t io_len[5] = {0, 1, 2, 4, 8}; struct vm_io_range gas_io; - if ((gas->address == 0UL) - || (gas->space_id != SPACE_SYSTEM_IO) - || (gas->access_size == 0U) - || (gas->access_size > 4U)) { - return; + if ((gas->address != 0UL) + && (gas->space_id == SPACE_SYSTEM_IO) + && (gas->access_size != 0U) + && (gas->access_size <= 4U)) { + gas_io.flags = IO_ATTR_RW; + gas_io.base = (uint16_t)gas->address; + gas_io.len = io_len[gas->access_size]; + + register_io_emulation_handler(vm, pio_idx, &gas_io, + &pm1ab_io_read, &pm1ab_io_write); + + pr_dbg("Enable PM1A trap for VM %d, port 0x%x, size %d\n", + vm->vm_id, gas_io.base, gas_io.len); } - - gas_io.flags = IO_ATTR_RW; - gas_io.base = (uint16_t)gas->address; - gas_io.len = io_len[gas->access_size]; - - register_io_emulation_handler(vm, pio_idx, &gas_io, - &pm1ab_io_read, &pm1ab_io_write); - - pr_dbg("Enable PM1A trap for VM %d, port 0x%x, size %d\n", - vm->vm_id, gas_io.base, gas_io.len); } void register_pm1ab_handler(struct acrn_vm *vm) diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c index b2a8965cb..6c647462f 100644 --- a/hypervisor/arch/x86/guest/vm.c +++ b/hypervisor/arch/x86/guest/vm.c @@ -45,11 +45,15 @@ static inline bool is_vm_valid(uint16_t vm_id) */ struct acrn_vm *get_vm_from_vmid(uint16_t vm_id) { + struct acrn_vm *ret; + if (is_vm_valid(vm_id)) { - return &vm_array[vm_id]; + ret = &vm_array[vm_id]; } else { - return NULL; + ret = NULL; } + + return ret; } /** @@ -188,41 +192,42 @@ err: /* * @pre vm != NULL */ -int shutdown_vm(struct acrn_vm *vm) +int32_t shutdown_vm(struct acrn_vm *vm) { - int status = 0; uint16_t i; struct acrn_vcpu *vcpu = NULL; + int32_t ret; pause_vm(vm); /* Only allow shutdown paused vm */ - if (vm->state != VM_PAUSED) { - return -EINVAL; + if (vm->state == VM_PAUSED) { + foreach_vcpu(i, vm, vcpu) { + reset_vcpu(vcpu); + offline_vcpu(vcpu); + } + + ptdev_release_all_entries(vm); + + /* Free EPT allocated resources assigned to VM */ + destroy_ept(vm); + + /* Free iommu */ + if (vm->iommu != NULL) { + destroy_iommu_domain(vm->iommu); + } + + /* Free vm id */ + free_vm_id(vm); + + vpci_cleanup(vm); + ret = 0; + } else { + ret = -EINVAL; } - foreach_vcpu(i, vm, vcpu) { - reset_vcpu(vcpu); - offline_vcpu(vcpu); - } - - ptdev_release_all_entries(vm); - - /* Free EPT allocated resources assigned to VM */ - destroy_ept(vm); - - /* Free iommu */ - if (vm->iommu != NULL) { - destroy_iommu_domain(vm->iommu); - } - - /* Free vm id */ - free_vm_id(vm); - - vpci_cleanup(vm); - /* Return status to caller */ - return status; + return ret; } /** @@ -244,29 +249,31 @@ int start_vm(struct acrn_vm *vm) /** * * @pre vm != NULL */ -int reset_vm(struct acrn_vm *vm) +int32_t reset_vm(struct acrn_vm *vm) { uint16_t i; struct acrn_vcpu *vcpu = NULL; + int32_t ret; - if (vm->state != VM_PAUSED) { - return -1; + if (vm->state == VM_PAUSED) { + foreach_vcpu(i, vm, vcpu) { + reset_vcpu(vcpu); + } + + if (is_vm0(vm)) { + (void )vm_sw_loader(vm); + } + + reset_vm_ioreqs(vm); + vioapic_reset(vm_ioapic(vm)); + destroy_secure_world(vm, false); + vm->sworld_control.flag.active = 0UL; + ret = 0; + } else { + ret = -1; } - foreach_vcpu(i, vm, vcpu) { - reset_vcpu(vcpu); - } - - if (is_vm0(vm)) { - (void )vm_sw_loader(vm); - } - - reset_vm_ioreqs(vm); - vioapic_reset(vm_ioapic(vm)); - destroy_secure_world(vm, false); - vm->sworld_control.flag.active = 0UL; - - return 0; + return ret; } /** @@ -277,14 +284,12 @@ void pause_vm(struct acrn_vm *vm) uint16_t i; struct acrn_vcpu *vcpu = NULL; - if (vm->state == VM_PAUSED) { - return; - } + if (vm->state != VM_PAUSED) { + vm->state = VM_PAUSED; - vm->state = VM_PAUSED; - - foreach_vcpu(i, vm, vcpu) { - pause_vcpu(vcpu, VCPU_ZOMBIE); + foreach_vcpu(i, vm, vcpu) { + pause_vcpu(vcpu, VCPU_ZOMBIE); + } } }