From a8cd6925fcf2df6a58bfa9bd226cff889374f8bd Mon Sep 17 00:00:00 2001 From: "Yan, Like" Date: Thu, 16 Aug 2018 20:21:11 +0800 Subject: [PATCH] hv: pirq: clean up irq handlers There are several similar irq handlers with confusing function names and it's not friendly to call update_irq_handler() to update a proper handler after request_irq(). With this commit, a single generic irq handler is being used, in which, no lock need to be acquired because our design could guarantee there is no concurrent irq handling and irq handler request/free. A flags field is added to irq_desc struct to select the proper processing flow for an irq. Irqflags is defined as follows: IRQF_NONE (0U) IRQF_LEVEL (1U << 1U) /* 1: level trigger; 0: edge trigger */ IRQF_PT (1U << 2U) /* 1: for passthrough dev */ Because we have only one irq handler, update_irq_handler() should be replace by set_irq_trigger_mode(), whichs set trigger mode flag of a certian irq. Accordingly, the code where called update_irq_handler() need to be updated. Signed-off-by: Yan, Like Acked-by: Anthony Xu --- hypervisor/arch/x86/assign.c | 56 ++-------------- hypervisor/arch/x86/ioapic.c | 4 +- hypervisor/arch/x86/irq.c | 112 +++++++++----------------------- hypervisor/arch/x86/notify.c | 2 +- hypervisor/arch/x86/timer.c | 23 ++----- hypervisor/arch/x86/vtd.c | 3 +- hypervisor/common/ptdev.c | 2 +- hypervisor/include/common/irq.h | 13 ++-- 8 files changed, 57 insertions(+), 158 deletions(-) diff --git a/hypervisor/arch/x86/assign.c b/hypervisor/arch/x86/assign.c index 424703475..535ca3597 100644 --- a/hypervisor/arch/x86/assign.c +++ b/hypervisor/arch/x86/assign.c @@ -139,51 +139,6 @@ lookup_entry_by_vintx(struct vm *vm, uint8_t vpin, return entry; } -static void -ptdev_update_irq_handler(struct vm *vm, struct ptdev_remapping_info *entry) -{ - uint32_t phys_irq = entry->allocated_pirq; - struct ptdev_intx_info *intx = &entry->ptdev_intr_info.intx; - - if (entry->type == PTDEV_INTR_MSI) { - /* all other MSI and normal maskable */ - update_irq_handler(phys_irq, common_handler_edge); - } - /* update irq handler for IOAPIC */ - if ((entry->type == PTDEV_INTR_INTX) - && (intx->vpin_src - == PTDEV_VPIN_IOAPIC)) { - union ioapic_rte rte; - bool trigger_lvl = false; - - /* VPIN_IOAPIC src means we have vioapic enabled */ - vioapic_get_rte(vm, intx->virt_pin, &rte); - if ((rte.full & IOAPIC_RTE_TRGRMOD) == IOAPIC_RTE_TRGRLVL) { - trigger_lvl = true; - } - - if (trigger_lvl) { - update_irq_handler(phys_irq, common_dev_handler_level); - } else { - update_irq_handler(phys_irq, common_handler_edge); - } - } - /* update irq handler for PIC */ - if ((entry->type == PTDEV_INTR_INTX) && (phys_irq < NR_LEGACY_IRQ) - && (intx->vpin_src == PTDEV_VPIN_PIC)) { - enum vpic_trigger trigger; - - /* VPIN_PIC src means we have vpic enabled */ - vpic_get_irq_trigger(vm, - intx->virt_pin, &trigger); - if (trigger == LEVEL_TRIGGER) { - update_irq_handler(phys_irq, common_dev_handler_level); - } else { - update_irq_handler(phys_irq, common_handler_edge); - } - } -} - static bool ptdev_hv_owned_intx(struct vm *vm, struct ptdev_intx_info *info) { /* vm0 pin 4 (uart) is owned by hypervisor under debug version */ @@ -677,9 +632,6 @@ int ptdev_msix_remap(struct vm *vm, uint16_t virt_bdf, entry->ptdev_intr_info.msi.phys_vector = irq_to_vector(entry->allocated_pirq); - /* update irq handler according to info in guest */ - ptdev_update_irq_handler(vm, entry); - dev_dbg(ACRN_DBG_IRQ, "PCI %x:%x.%x MSI VR[%d] 0x%x->0x%x assigned to vm%d", (entry->virt_bdf >> 8) & 0xFFU, @@ -711,6 +663,7 @@ static void activate_physical_ioapic(struct vm *vm, { union ioapic_rte rte; uint32_t phys_irq = entry->allocated_pirq; + bool is_lvl_trigger = false; /* disable interrupt */ GSI_MASK_IRQ(phys_irq); @@ -722,8 +675,11 @@ static void activate_physical_ioapic(struct vm *vm, rte.full |= IOAPIC_RTE_INTMSET; ioapic_set_rte(phys_irq, rte); - /* update irq handler according to info in guest */ - ptdev_update_irq_handler(vm, entry); + /* update irq trigger mode according to info in guest */ + if ((rte.full & IOAPIC_RTE_TRGRMOD) == IOAPIC_RTE_TRGRLVL) { + is_lvl_trigger = true; + } + set_irq_trigger_mode(phys_irq, is_lvl_trigger); /* enable interrupt */ GSI_UNMASK_IRQ(phys_irq); diff --git a/hypervisor/arch/x86/ioapic.c b/hypervisor/arch/x86/ioapic.c index 2bb586596..ddc1babce 100644 --- a/hypervisor/arch/x86/ioapic.c +++ b/hypervisor/arch/x86/ioapic.c @@ -219,9 +219,9 @@ static void ioapic_set_routing(uint32_t gsi, uint32_t vr) ioapic_set_rte_entry(addr, gsi_table[gsi].pin, rte); if ((rte.full & IOAPIC_RTE_TRGRMOD) != 0UL) { - update_irq_handler(gsi, handle_level_interrupt_common); + set_irq_trigger_mode(gsi, true); } else { - update_irq_handler(gsi, common_handler_edge); + set_irq_trigger_mode(gsi, false); } dev_dbg(ACRN_DBG_IRQ, "GSI: irq:%d pin:%hhu rte:%lx", diff --git a/hypervisor/arch/x86/irq.c b/hypervisor/arch/x86/irq.c index 97dccd46d..75ae52fd7 100644 --- a/hypervisor/arch/x86/irq.c +++ b/hypervisor/arch/x86/irq.c @@ -15,6 +15,8 @@ static uint32_t vector_to_irq[NR_MAX_VECTOR + 1]; spurious_handler_t spurious_handler; +static inline void handle_irq(struct irq_desc *desc); + #define NR_STATIC_MAPPINGS (2U) static uint32_t irq_static_mappings[NR_STATIC_MAPPINGS][2] = { {TIMER_IRQ, VECTOR_TIMER}, @@ -212,7 +214,8 @@ static void disable_pic_irq(void) */ int32_t request_irq(uint32_t req_irq, irq_action_t action_fn, - void *priv_data) + void *priv_data, + uint32_t flags) { struct irq_desc *desc; uint32_t irq, vector; @@ -234,11 +237,8 @@ int32_t request_irq(uint32_t req_irq, desc = &irq_desc_array[irq]; spinlock_irqsave_obtain(&desc->lock, &rflags); - if (desc->irq_handler == NULL) { - desc->irq_handler = common_handler_edge; - } - if (desc->action == NULL) { + desc->flags = flags; desc->priv_data = priv_data; desc->action = action_fn; spinlock_irqrestore_release(&desc->lock, rflags); @@ -327,12 +327,12 @@ void dispatch_interrupt(struct intr_excp_ctx *ctx) goto ERR; } - if ((desc->used == IRQ_NOT_ASSIGNED) || (desc->irq_handler == NULL)) { + if (desc->used == IRQ_NOT_ASSIGNED) { /* mask irq if possible */ goto ERR; } - desc->irq_handler(desc, NULL); + handle_irq(desc); return; ERR: handle_spurious_interrupt(vr); @@ -361,16 +361,26 @@ void partition_mode_dispatch_interrupt(struct intr_excp_ctx *ctx) } #endif -int handle_level_interrupt_common(struct irq_desc *desc, - __unused void *handler_data) +static inline bool irq_need_mask(struct irq_desc *desc) +{ + /* level triggered gsi should be masked */ + return (((desc->flags & IRQF_LEVEL) != 0U) + && irq_is_gsi(desc->irq)); +} + +static inline bool irq_need_unmask(struct irq_desc *desc) +{ + /* level triggered gsi for non-ptdev should be unmasked */ + return (((desc->flags & IRQF_LEVEL) != 0U) + && ((desc->flags & IRQF_PT) == 0U) + && irq_is_gsi(desc->irq)); +} + +static inline void handle_irq(struct irq_desc *desc) { - uint64_t rflags; irq_action_t action = desc->action; - spinlock_irqsave_obtain(&desc->lock, &rflags); - - /* mask iopaic pin */ - if (irq_is_gsi(desc->irq)) { + if (irq_need_mask(desc)) { GSI_MASK_IRQ(desc->irq); } @@ -381,75 +391,12 @@ int handle_level_interrupt_common(struct irq_desc *desc, action(desc->irq, desc->priv_data); } - if (irq_is_gsi(desc->irq)) { + if (irq_need_unmask(desc)) { GSI_UNMASK_IRQ(desc->irq); } - - spinlock_irqrestore_release(&desc->lock, rflags); - - return 0; } -int common_handler_edge(struct irq_desc *desc, __unused void *handler_data) -{ - uint64_t rflags; - irq_action_t action = desc->action; - - spinlock_irqsave_obtain(&desc->lock, &rflags); - - /* Send EOI to LAPIC/IOAPIC IRR */ - send_lapic_eoi(); - - if (action != NULL) { - action(desc->irq, desc->priv_data); - } - - spinlock_irqrestore_release(&desc->lock, rflags); - - return 0; -} - -int common_dev_handler_level(struct irq_desc *desc, __unused void *handler_data) -{ - uint64_t rflags; - irq_action_t action = desc->action; - - spinlock_irqsave_obtain(&desc->lock, &rflags); - - /* mask iopaic pin */ - if (irq_is_gsi(desc->irq)) { - GSI_MASK_IRQ(desc->irq); - } - - /* Send EOI to LAPIC/IOAPIC IRR */ - send_lapic_eoi(); - - if (action != NULL) { - action(desc->irq, desc->priv_data); - } - - spinlock_irqrestore_release(&desc->lock, rflags); - - /* we did not unmask irq until guest EOI the vector */ - return 0; -} - -/* no desc->lock for quick handling local interrupt like lapic timer */ -int quick_handler_nolock(struct irq_desc *desc, __unused void *handler_data) -{ - irq_action_t action = desc->action; - - /* Send EOI to LAPIC/IOAPIC IRR */ - send_lapic_eoi(); - - if (action != NULL) { - action(desc->irq, desc->priv_data); - } - - return 0; -} - -void update_irq_handler(uint32_t irq, irq_handler_t func) +void set_irq_trigger_mode(uint32_t irq, bool is_level_trigger) { uint64_t rflags; struct irq_desc *desc; @@ -460,7 +407,11 @@ void update_irq_handler(uint32_t irq, irq_handler_t func) desc = &irq_desc_array[irq]; spinlock_irqsave_obtain(&desc->lock, &rflags); - desc->irq_handler = func; + if (is_level_trigger == true) { + desc->flags |= IRQF_LEVEL; + } else { + desc->flags &= ~IRQF_LEVEL; + } spinlock_irqrestore_release(&desc->lock, rflags); } @@ -483,6 +434,7 @@ void free_irq(uint32_t irq) spinlock_irqsave_obtain(&desc->lock, &rflags); desc->action = NULL; desc->priv_data = NULL; + desc->flags = IRQF_NONE; spinlock_irqrestore_release(&desc->lock, rflags); } diff --git a/hypervisor/arch/x86/notify.c b/hypervisor/arch/x86/notify.c index 463c22d18..912018b68 100644 --- a/hypervisor/arch/x86/notify.c +++ b/hypervisor/arch/x86/notify.c @@ -66,7 +66,7 @@ static int request_notification_irq(irq_action_t func, void *data) } /* all cpu register the same notification vector */ - retval = request_irq(NOTIFY_IRQ, func, data); + retval = request_irq(NOTIFY_IRQ, func, data, IRQF_NONE); if (retval < 0) { pr_err("Failed to add notify isr"); return -ENODEV; diff --git a/hypervisor/arch/x86/timer.c b/hypervisor/arch/x86/timer.c index e959778b7..9a933b4ba 100644 --- a/hypervisor/arch/x86/timer.c +++ b/hypervisor/arch/x86/timer.c @@ -24,7 +24,7 @@ static void run_timer(struct hv_timer *timer) } /* run in interrupt context */ -static int tsc_deadline_handler(__unused int irq, __unused void *data) +static int tsc_deadline_handler(__unused uint32_t irq, __unused void *data) { fire_softirq(SOFTIRQ_TIMER); return 0; @@ -107,21 +107,6 @@ void del_timer(struct hv_timer *timer) } } -static int request_timer_irq(irq_action_t func) -{ - int32_t retval; - - retval = request_irq(TIMER_IRQ, func, NULL); - if (retval >= 0) { - update_irq_handler(TIMER_IRQ, quick_handler_nolock); - } else { - pr_err("Failed to add timer isr"); - return -ENODEV; - } - - return 0; -} - static void init_percpu_timer(uint16_t pcpu_id) { struct per_cpu_timers *cpu_timer; @@ -186,14 +171,16 @@ static void timer_softirq(uint16_t pcpu_id) void timer_init(void) { uint16_t pcpu_id = get_cpu_id(); + int32_t retval; init_percpu_timer(pcpu_id); if (pcpu_id == BOOT_CPU_ID) { register_softirq(SOFTIRQ_TIMER, timer_softirq); - if (request_timer_irq((irq_action_t)tsc_deadline_handler) - < 0) { + retval = request_irq(TIMER_IRQ, (irq_action_t)tsc_deadline_handler, + NULL, IRQF_NONE); + if (retval < 0) { pr_err("Timer setup failed"); return; } diff --git a/hypervisor/arch/x86/vtd.c b/hypervisor/arch/x86/vtd.c index 6242c8401..e1a5ed11a 100644 --- a/hypervisor/arch/x86/vtd.c +++ b/hypervisor/arch/x86/vtd.c @@ -828,7 +828,8 @@ static int dmar_setup_interrupt(struct dmar_drhd_rt *dmar_uint) retval = request_irq(IRQ_INVALID, dmar_fault_handler, - dmar_uint); + dmar_uint, + IRQF_NONE); if (retval < 0 ) { pr_err("%s: fail to setup interrupt", __func__); diff --git a/hypervisor/common/ptdev.c b/hypervisor/common/ptdev.c index ca9b02b97..9f9186733 100644 --- a/hypervisor/common/ptdev.c +++ b/hypervisor/common/ptdev.c @@ -136,7 +136,7 @@ ptdev_activate_entry(struct ptdev_remapping_info *entry, uint32_t phys_irq) /* register and allocate host vector/irq */ retval = request_irq(phys_irq, ptdev_interrupt_handler, - (void *)entry); + (void *)entry, IRQF_PT); ASSERT(retval >= 0, "dev register failed"); entry->allocated_pirq = (uint32_t)retval; diff --git a/hypervisor/include/common/irq.h b/hypervisor/include/common/irq.h index a492f6660..b4d8d83d1 100644 --- a/hypervisor/include/common/irq.h +++ b/hypervisor/include/common/irq.h @@ -7,6 +7,10 @@ #ifndef COMMON_IRQ_H #define COMMON_IRQ_H +#define IRQF_NONE (0U) +#define IRQF_LEVEL (1U << 1U) /* 1: level trigger; 0: edge trigger */ +#define IRQF_PT (1U << 2U) /* 1: for passthrough dev */ + enum irq_mode { IRQ_PULSE, IRQ_ASSERT, @@ -26,20 +30,19 @@ struct irq_desc { enum irq_use_state used; /* this irq have assigned to device */ uint32_t vector; /* assigned vector */ - int (*irq_handler)(struct irq_desc *irq_desc, void *handler_data); - /* callback for irq flow handling */ irq_action_t action; /* callback registered from component */ void *priv_data; /* irq_action private data */ + uint32_t flags; /* flags for trigger mode/ptdev */ spinlock_t lock; }; int32_t request_irq(uint32_t irq, irq_action_t action_fn, - void *priv_data); + void *priv_data, + uint32_t flags); void free_irq(uint32_t irq); -typedef int (*irq_handler_t)(struct irq_desc *desc, void *handler_data); -void update_irq_handler(uint32_t irq, irq_handler_t func); +void set_irq_trigger_mode(uint32_t irq, bool is_level_trigger); #endif /* COMMON_IRQ_H */