Re: [PATCH v2] hv:fix return value violations for vpic/vioapic


Xu, Anthony
 

Acked-by: Anthony Xu <anthony.xu@...>

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Mingqiang Chi
Sent: Tuesday, August 14, 2018 4:26 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v2] hv:fix return value violations for vpic/vioapic

From: Mingqiang Chi <mingqiang.chi@...>

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

Signed-off-by: Mingqiang Chi <mingqiang.chi@...>
---
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 353baa6..4247034 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 fd4bc29..289921d 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 (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 119eabe..946a057 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 266bd53..845b677 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 98ffc9e..29bb540 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 b4565b8..a44bf8d 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_ */
--
2.7.4


Join acrn-dev@lists.projectacrn.org to automatically receive all group messages.