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 <mingqiang.chi@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>

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;
 	}

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)) {
+	if ((vm == NULL) || (param == NULL) || target_vm == NULL) {
-		return -1;
+		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;
 }

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
+void vioapic_deassert_irq(struct vm *vm, uint32_t irq)
-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);
 }
 
 /*

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;
+	return NR_VPIC_PINS_TOTAL;
-	uint8_t pin_mask;
+}
 
-	if (irq >= NR_VPIC_PINS_TOTAL) {
+/**
-		return -EINVAL;
+ * @pre vm->vpic != NULL
-	}
+ * @pre irq < NR_VPIC_PINS_TOTAL
-
-	/*
-	 * See comment in vpic_elc_handler.  These IRQs must be
-	 * edge triggered.
  */
-	if (trigger == LEVEL_TRIGGER) {
+void vpic_get_irq_trigger(struct vm *vm, uint32_t irq,
-		switch (irq) {
+		enum vpic_trigger *trigger)
-		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;
-}
-
-int 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)

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);
+void	vioapic_assert_irq(struct vm *vm, uint32_t irq);
-int	vioapic_deassert_irq(struct vm *vm, uint32_t irq);
+void	vioapic_deassert_irq(struct vm *vm, uint32_t irq);
-int	vioapic_pulse_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);

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);
+void vpic_assert_irq(struct vm *vm, uint32_t irq);
-int vpic_deassert_irq(struct vm *vm, uint32_t irq);
+void vpic_deassert_irq(struct vm *vm, uint32_t irq);
-int vpic_pulse_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,
+void vpic_get_irq_trigger(struct vm *vm, uint32_t irq,
-	enum vpic_trigger trigger);
-int 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_ */