From de487fff2b5f43521b6e3b063b5bfdc0c27eac8e Mon Sep 17 00:00:00 2001 From: Mingqiang Chi Date: Tue, 14 Aug 2018 18:54:31 +0800 Subject: [PATCH] hv:fix return value violations for vpic/vioapic -- Change these APIs to void type, add pre-conditions, and move parameter-check to upper-layer functions. handle_vpic_irqline vpic_set_irqstate vpic_assert_irq vpic_deassert_irq vpic_pulse_irq vpic_get_irq_trigger handle_vioapic_irqline vioapic_assert_irq vioapic_deassert_irq vioapic_pulse_irq -- Remove dead code vpic_set_irq_trigger v1-->v2: add cleanup vpic change some APIs to void type, add pre-conditions, and move the parameter-check to upper-layer functions. Signed-off-by: Mingqiang Chi Acked-by: Anthony Xu --- hypervisor/arch/x86/assign.c | 3 +- hypervisor/common/hypercall.c | 61 +++++++------ hypervisor/dm/vioapic.c | 40 ++++----- hypervisor/dm/vpic.c | 99 +++++---------------- hypervisor/include/arch/x86/guest/vioapic.h | 6 +- hypervisor/include/arch/x86/guest/vpic.h | 12 ++- 6 files changed, 83 insertions(+), 138 deletions(-) diff --git a/hypervisor/arch/x86/assign.c b/hypervisor/arch/x86/assign.c index 353baa663..424703475 100644 --- a/hypervisor/arch/x86/assign.c +++ b/hypervisor/arch/x86/assign.c @@ -896,7 +896,8 @@ int ptdev_add_intx_remapping(struct vm *vm, { struct ptdev_remapping_info *entry; - if (vm == NULL) { + if (vm == NULL || (!pic_pin && virt_pin >= vioapic_pincount(vm)) + || (pic_pin && virt_pin >= vpic_pincount())) { pr_err("ptdev_add_intx_remapping fails!\n"); return -EINVAL; } diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c index 35e0d90ac..d585e80dc 100644 --- a/hypervisor/common/hypercall.c +++ b/hypervisor/common/hypercall.c @@ -68,24 +68,22 @@ int32_t hcall_get_api_version(struct vm *vm, uint64_t param) return 0; } -static int32_t +/** + * @pre vm != NULL + */ +static void handle_vpic_irqline(struct vm *vm, uint32_t irq, enum irq_mode mode) { - int32_t ret = -1; - - if (vm == NULL) { - return ret; - } switch (mode) { case IRQ_ASSERT: - ret = vpic_assert_irq(vm, irq); + vpic_assert_irq(vm, irq); break; case IRQ_DEASSERT: - ret = vpic_deassert_irq(vm, irq); + vpic_deassert_irq(vm, irq); break; case IRQ_PULSE: - ret = vpic_pulse_irq(vm, irq); + vpic_pulse_irq(vm, irq); default: /* * In this switch statement, mode shall either be IRQ_ASSERT or @@ -94,28 +92,23 @@ handle_vpic_irqline(struct vm *vm, uint32_t irq, enum irq_mode mode) */ break; } - - return ret; } -static int32_t +/** + * @pre vm != NULL + */ +static void handle_vioapic_irqline(struct vm *vm, uint32_t irq, enum irq_mode mode) { - int32_t ret = -1; - - if (vm == NULL) { - return ret; - } - switch (mode) { case IRQ_ASSERT: - ret = vioapic_assert_irq(vm, irq); + vioapic_assert_irq(vm, irq); break; case IRQ_DEASSERT: - ret = vioapic_deassert_irq(vm, irq); + vioapic_deassert_irq(vm, irq); break; case IRQ_PULSE: - ret = vioapic_pulse_irq(vm, irq); + vioapic_pulse_irq(vm, irq); break; default: /* @@ -125,7 +118,6 @@ handle_vioapic_irqline(struct vm *vm, uint32_t irq, enum irq_mode mode) */ break; } - return ret; } static int32_t @@ -136,8 +128,21 @@ handle_virt_irqline(struct vm *vm, uint16_t target_vmid, uint32_t intr_type; struct vm *target_vm = get_vm_from_vmid(target_vmid); - if ((vm == NULL) || (param == NULL)) { - return -1; + if ((vm == NULL) || (param == NULL) || target_vm == NULL) { + return -EINVAL; + } + + /* Check valid irq */ + if (param->intr_type == ACRN_INTR_TYPE_IOAPIC + && param->ioapic_irq >= vioapic_pincount(vm)) { + return -EINVAL; + } + + if (param->intr_type == ACRN_INTR_TYPE_ISA + && (param->pic_irq >= vpic_pincount() + || (param->ioapic_irq != (~0U) + && param->ioapic_irq >= vioapic_pincount(vm)))) { + return -EINVAL; } intr_type = param->intr_type; @@ -145,24 +150,24 @@ handle_virt_irqline(struct vm *vm, uint16_t target_vmid, switch (intr_type) { case ACRN_INTR_TYPE_ISA: /* Call vpic for pic injection */ - ret = handle_vpic_irqline(target_vm, param->pic_irq, mode); + handle_vpic_irqline(target_vm, param->pic_irq, mode); /* call vioapic for ioapic injection if ioapic_irq != ~0U*/ if (param->ioapic_irq != (~0U)) { /* handle IOAPIC irqline */ - ret = handle_vioapic_irqline(target_vm, + handle_vioapic_irqline(target_vm, param->ioapic_irq, mode); } break; case ACRN_INTR_TYPE_IOAPIC: /* handle IOAPIC irqline */ - ret = handle_vioapic_irqline(target_vm, + handle_vioapic_irqline(target_vm, param->ioapic_irq, mode); break; default: dev_dbg(ACRN_DBG_HYCALL, "vINTR inject failed. type=%d", intr_type); - ret = -1; + ret = -EINVAL; } return ret; } diff --git a/hypervisor/dm/vioapic.c b/hypervisor/dm/vioapic.c index 389c8d2ab..a08dd6752 100644 --- a/hypervisor/dm/vioapic.c +++ b/hypervisor/dm/vioapic.c @@ -67,17 +67,15 @@ vm_ioapic(struct vm *vm) return (struct vioapic *)vm->arch_vm.virt_ioapic; } +/** + * @pre pin < vioapic_pincount(vm) + */ static void vioapic_send_intr(struct vioapic *vioapic, uint32_t pin) { uint32_t vector, dest, delmode; union ioapic_rte rte; bool level, phys; - uint32_t pincount = vioapic_pincount(vioapic->vm); - - if (pin >= pincount) { - pr_err("vioapic_send_intr: invalid pin number %hhu", pin); - } rte = vioapic->rtbl[pin]; @@ -105,16 +103,14 @@ vioapic_send_intr(struct vioapic *vioapic, uint32_t pin) delmode, vector, false); } +/** + * @pre pin < vioapic_pincount(vm) + */ static void vioapic_set_pinstate(struct vioapic *vioapic, uint32_t pin, bool newstate) { int oldcnt, newcnt; bool needintr; - uint32_t pincount = vioapic_pincount(vioapic->vm); - - if (pin >= pincount) { - pr_err("vioapic_set_pinstate: invalid pin number %hhu", pin); - } oldcnt = vioapic->acnt[pin]; if (newstate) { @@ -150,16 +146,15 @@ enum irqstate { IRQSTATE_PULSE }; -static int +/** + * @pre irq < vioapic_pincount(vm) + */ +static void vioapic_set_irqstate(struct vm *vm, uint32_t irq, enum irqstate irqstate) { struct vioapic *vioapic; uint32_t pin = irq; - if (pin >= vioapic_pincount(vm)) { - return -EINVAL; - } - vioapic = vm_ioapic(vm); VIOAPIC_LOCK(vioapic); @@ -178,26 +173,23 @@ vioapic_set_irqstate(struct vm *vm, uint32_t irq, enum irqstate irqstate) panic("vioapic_set_irqstate: invalid irqstate %d", irqstate); } VIOAPIC_UNLOCK(vioapic); - - return 0; } -int +void vioapic_assert_irq(struct vm *vm, uint32_t irq) { - return vioapic_set_irqstate(vm, irq, IRQSTATE_ASSERT); + vioapic_set_irqstate(vm, irq, IRQSTATE_ASSERT); } -int -vioapic_deassert_irq(struct vm *vm, uint32_t irq) +void vioapic_deassert_irq(struct vm *vm, uint32_t irq) { - return vioapic_set_irqstate(vm, irq, IRQSTATE_DEASSERT); + vioapic_set_irqstate(vm, irq, IRQSTATE_DEASSERT); } -int +void vioapic_pulse_irq(struct vm *vm, uint32_t irq) { - return vioapic_set_irqstate(vm, irq, IRQSTATE_PULSE); + vioapic_set_irqstate(vm, irq, IRQSTATE_PULSE); } /* diff --git a/hypervisor/dm/vpic.c b/hypervisor/dm/vpic.c index 266bd538c..845b67778 100644 --- a/hypervisor/dm/vpic.c +++ b/hypervisor/dm/vpic.c @@ -460,15 +460,15 @@ static int vpic_ocw3(struct acrn_vpic *vpic, struct i8259_reg_state *i8259, uint return 0; } +/** + * @pre pin < NR_VPIC_PINS_TOTAL + */ static void vpic_set_pinstate(struct acrn_vpic *vpic, uint8_t pin, bool newstate) { struct i8259_reg_state *i8259; int oldcnt, newcnt; bool level; - ASSERT(pin < NR_VPIC_PINS_TOTAL, - "vpic_set_pinstate: invalid pin number"); - i8259 = &vpic->i8259[pin >> 3U]; oldcnt = i8259->acnt[pin & 0x7U]; @@ -504,22 +504,22 @@ static void vpic_set_pinstate(struct acrn_vpic *vpic, uint8_t pin, bool newstate vpic_notify_intr(vpic); } -static int vpic_set_irqstate(struct vm *vm, uint32_t irq, enum irqstate irqstate) +/** + * @pre irq < NR_VPIC_PINS_TOTAL + */ +static void vpic_set_irqstate(struct vm *vm, uint32_t irq, + enum irqstate irqstate) { struct acrn_vpic *vpic; struct i8259_reg_state *i8259; uint8_t pin; - if (irq >= NR_VPIC_PINS_TOTAL) { - return -EINVAL; - } - vpic = vm_pic(vm); i8259 = &vpic->i8259[irq >> 3U]; pin = (uint8_t)irq; if (i8259->ready == false) { - return 0; + return; } VPIC_LOCK(vpic); @@ -538,97 +538,46 @@ static int vpic_set_irqstate(struct vm *vm, uint32_t irq, enum irqstate irqstate ASSERT(false, "vpic_set_irqstate: invalid irqstate"); } VPIC_UNLOCK(vpic); - - return 0; } /* hypervisor interface: assert/deassert/pulse irq */ -int vpic_assert_irq(struct vm *vm, uint32_t irq) +void vpic_assert_irq(struct vm *vm, uint32_t irq) { - return vpic_set_irqstate(vm, irq, IRQSTATE_ASSERT); + vpic_set_irqstate(vm, irq, IRQSTATE_ASSERT); } -int vpic_deassert_irq(struct vm *vm, uint32_t irq) +void vpic_deassert_irq(struct vm *vm, uint32_t irq) { - return vpic_set_irqstate(vm, irq, IRQSTATE_DEASSERT); + vpic_set_irqstate(vm, irq, IRQSTATE_DEASSERT); } -int vpic_pulse_irq(struct vm *vm, uint32_t irq) +void vpic_pulse_irq(struct vm *vm, uint32_t irq) { - return vpic_set_irqstate(vm, irq, IRQSTATE_PULSE); + vpic_set_irqstate(vm, irq, IRQSTATE_PULSE); } -int vpic_set_irq_trigger(struct vm *vm, uint32_t irq, enum vpic_trigger trigger) +uint32_t +vpic_pincount(void) { - struct acrn_vpic *vpic; - uint8_t pin_mask; - - if (irq >= NR_VPIC_PINS_TOTAL) { - return -EINVAL; - } - - /* - * See comment in vpic_elc_handler. These IRQs must be - * edge triggered. - */ - if (trigger == LEVEL_TRIGGER) { - switch (irq) { - case 0U: - case 1U: - case 2U: - case 8U: - case 13U: - return -EINVAL; - default: - /* - * The IRQs handled earlier are the ones that could only - * support edge trigger, while the input parameter - * 'trigger' is set as LEVEL_TRIGGER. So, an error code - * (-EINVAL) shall be returned due to the invalid - * operation. - * - * All the other IRQs will be handled properly after - * this switch statement. - */ - break; - } - } - - vpic = vm_pic(vm); - pin_mask = (uint8_t)(1U << (irq & 0x7U)); - - VPIC_LOCK(vpic); - - if (trigger == LEVEL_TRIGGER) { - vpic->i8259[irq >> 3U].elc |= pin_mask; - } else { - vpic->i8259[irq >> 3U].elc &= ~pin_mask; - } - - VPIC_UNLOCK(vpic); - - return 0; + return NR_VPIC_PINS_TOTAL; } -int vpic_get_irq_trigger(struct vm *vm, uint32_t irq, enum vpic_trigger *trigger) +/** + * @pre vm->vpic != NULL + * @pre irq < NR_VPIC_PINS_TOTAL + */ +void vpic_get_irq_trigger(struct vm *vm, uint32_t irq, + enum vpic_trigger *trigger) { struct acrn_vpic *vpic; - if (irq >= NR_VPIC_PINS_TOTAL) { - return -EINVAL; - } - vpic = vm_pic(vm); - if (vpic == NULL) { - return -EINVAL; - } if ((vpic->i8259[irq >> 3U].elc & (1U << (irq & 0x7U))) != 0U) { *trigger = LEVEL_TRIGGER; } else { *trigger = EDGE_TRIGGER; } - return 0; } void vpic_pending_intr(struct vm *vm, uint32_t *vecptr) diff --git a/hypervisor/include/arch/x86/guest/vioapic.h b/hypervisor/include/arch/x86/guest/vioapic.h index 98ffc9e5d..29bb54014 100644 --- a/hypervisor/include/arch/x86/guest/vioapic.h +++ b/hypervisor/include/arch/x86/guest/vioapic.h @@ -41,9 +41,9 @@ struct vioapic *vioapic_init(struct vm *vm); void vioapic_cleanup(struct vioapic *vioapic); void vioapic_reset(struct vioapic *vioapic); -int vioapic_assert_irq(struct vm *vm, uint32_t irq); -int vioapic_deassert_irq(struct vm *vm, uint32_t irq); -int vioapic_pulse_irq(struct vm *vm, uint32_t irq); +void vioapic_assert_irq(struct vm *vm, uint32_t irq); +void vioapic_deassert_irq(struct vm *vm, uint32_t irq); +void vioapic_pulse_irq(struct vm *vm, uint32_t irq); void vioapic_update_tmr(struct vcpu *vcpu); void vioapic_mmio_write(struct vm *vm, uint64_t gpa, uint32_t wval); diff --git a/hypervisor/include/arch/x86/guest/vpic.h b/hypervisor/include/arch/x86/guest/vpic.h index b4565b8c6..a44bf8dd6 100644 --- a/hypervisor/include/arch/x86/guest/vpic.h +++ b/hypervisor/include/arch/x86/guest/vpic.h @@ -93,17 +93,15 @@ enum vpic_trigger { void *vpic_init(struct vm *vm); void vpic_cleanup(struct vm *vm); -int vpic_assert_irq(struct vm *vm, uint32_t irq); -int vpic_deassert_irq(struct vm *vm, uint32_t irq); -int vpic_pulse_irq(struct vm *vm, uint32_t irq); +void vpic_assert_irq(struct vm *vm, uint32_t irq); +void vpic_deassert_irq(struct vm *vm, uint32_t irq); +void vpic_pulse_irq(struct vm *vm, uint32_t irq); void vpic_pending_intr(struct vm *vm, uint32_t *vecptr); void vpic_intr_accepted(struct vm *vm, uint32_t vector); -int vpic_set_irq_trigger(struct vm *vm, uint32_t irq, - enum vpic_trigger trigger); -int vpic_get_irq_trigger(struct vm *vm, uint32_t irq, +void vpic_get_irq_trigger(struct vm *vm, uint32_t irq, enum vpic_trigger *trigger); - +uint32_t vpic_pincount(void); bool vpic_is_pin_mask(struct acrn_vpic *vpic, uint8_t virt_pin_arg); #endif /* _VPIC_H_ */