| Message ID | 1460729047-6346-6-git-send-email-andre.przywara@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Andre, On 04/15/2016 04:04 PM, Andre Przywara wrote: > From: Christoffer Dall <christoffer.dall@linaro.org> > > The communication of a Linux IRQ number from outside the VGIC to the > vgic was a leftover from the day when the vgic code cared about how a > particular device injects virtual interrupts mapped to a physical > interrupt. > > We can safely remove this notion, leaving all physical IRQ handling to > be done in the device driver (the arch timer in this case), which makes > room for a saner API for the new VGIC. > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > include/kvm/arm_vgic.h | 3 +-- > virt/kvm/arm/arch_timer.c | 22 ++++++++++++++++++++-- > virt/kvm/arm/vgic.c | 20 ++------------------ > 3 files changed, 23 insertions(+), 22 deletions(-) > > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h > index 43eeb18..49c559e 100644 > --- a/include/kvm/arm_vgic.h > +++ b/include/kvm/arm_vgic.h > @@ -157,7 +157,6 @@ struct vgic_io_device { > struct irq_phys_map { > u32 virt_irq; > u32 phys_irq; > - u32 irq; > }; > > struct irq_phys_map_entry { > @@ -345,7 +344,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, > void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg); > int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu); > struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, > - int virt_irq, int irq); > + int virt_irq, int phys_irq); > int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq); > bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq); > > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c > index b4d96b1..cfdf88f 100644 > --- a/virt/kvm/arm/arch_timer.c > +++ b/virt/kvm/arm/arch_timer.c > @@ -274,7 +274,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu) > if (timer->active_cleared_last && !phys_active) > return; > > - ret = irq_set_irqchip_state(timer->map->irq, > + ret = irq_set_irqchip_state(host_vtimer_irq, > IRQCHIP_STATE_ACTIVE, > phys_active); > WARN_ON(ret); > @@ -307,6 +307,9 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu, > { > struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; > struct irq_phys_map *map; > + struct irq_desc *desc; > + struct irq_data *data; > + int phys_irq; > > /* > * The vcpu timer irq number cannot be determined in > @@ -326,10 +329,25 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu, > kvm_timer_update_state(vcpu); > > /* > + * Find the physical IRQ number corresponding to the host_vtimer_irq > + */ > + desc = irq_to_desc(host_vtimer_irq); > + if (!desc) { can this really happen? > + kvm_err("%s: no interrupt descriptor\n", __func__); > + return -EINVAL; > + } > + > + data = irq_desc_get_irq_data(desc); > + while (data->parent_data) > + data = data->parent_data; > + > + phys_irq = data->hwirq; > + > + /* > * Tell the VGIC that the virtual interrupt is tied to a > * physical interrupt. We do that once per VCPU. > */ > - map = kvm_vgic_map_phys_irq(vcpu, irq->irq, host_vtimer_irq); > + map = kvm_vgic_map_phys_irq(vcpu, irq->irq, phys_irq); > if (WARN_ON(IS_ERR(map))) > return PTR_ERR(map); > > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c > index 2d7ae35..ac5838b 100644 > --- a/virt/kvm/arm/vgic.c > +++ b/virt/kvm/arm/vgic.c > @@ -1723,27 +1723,13 @@ static struct list_head *vgic_get_irq_phys_map_list(struct kvm_vcpu *vcpu, > * Returns a valid pointer on success, and an error pointer otherwise > */ the doc comment must be updated * @irq: The Linux IRQ number Besides Reviewed-by: Eric Auger <eric.auger@linaro.org> Cheers Eric > struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, > - int virt_irq, int irq) > + int virt_irq, int phys_irq) > { > struct vgic_dist *dist = &vcpu->kvm->arch.vgic; > struct list_head *root = vgic_get_irq_phys_map_list(vcpu, virt_irq); > struct irq_phys_map *map; > struct irq_phys_map_entry *entry; > - struct irq_desc *desc; > - struct irq_data *data; > - int phys_irq; > > - desc = irq_to_desc(irq); > - if (!desc) { > - kvm_err("%s: no interrupt descriptor\n", __func__); > - return ERR_PTR(-EINVAL); > - } > - > - data = irq_desc_get_irq_data(desc); > - while (data->parent_data) > - data = data->parent_data; > - > - phys_irq = data->hwirq; > > /* Create a new mapping */ > entry = kzalloc(sizeof(*entry), GFP_KERNEL); > @@ -1756,8 +1742,7 @@ struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, > map = vgic_irq_map_search(vcpu, virt_irq); > if (map) { > /* Make sure this mapping matches */ > - if (map->phys_irq != phys_irq || > - map->irq != irq) > + if (map->phys_irq != phys_irq) > map = ERR_PTR(-EINVAL); > > /* Found an existing, valid mapping */ > @@ -1767,7 +1752,6 @@ struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, > map = &entry->map; > map->virt_irq = virt_irq; > map->phys_irq = phys_irq; > - map->irq = irq; > > list_add_tail_rcu(&entry->entry, root); > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Apr 21, 2016 at 07:41:01PM +0200, Eric Auger wrote: > Hi Andre, > On 04/15/2016 04:04 PM, Andre Przywara wrote: > > From: Christoffer Dall <christoffer.dall@linaro.org> > > > > The communication of a Linux IRQ number from outside the VGIC to the > > vgic was a leftover from the day when the vgic code cared about how a > > particular device injects virtual interrupts mapped to a physical > > interrupt. > > > > We can safely remove this notion, leaving all physical IRQ handling to > > be done in the device driver (the arch timer in this case), which makes > > room for a saner API for the new VGIC. > > > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > include/kvm/arm_vgic.h | 3 +-- > > virt/kvm/arm/arch_timer.c | 22 ++++++++++++++++++++-- > > virt/kvm/arm/vgic.c | 20 ++------------------ > > 3 files changed, 23 insertions(+), 22 deletions(-) > > > > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h > > index 43eeb18..49c559e 100644 > > --- a/include/kvm/arm_vgic.h > > +++ b/include/kvm/arm_vgic.h > > @@ -157,7 +157,6 @@ struct vgic_io_device { > > struct irq_phys_map { > > u32 virt_irq; > > u32 phys_irq; > > - u32 irq; > > }; > > > > struct irq_phys_map_entry { > > @@ -345,7 +344,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, > > void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg); > > int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu); > > struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, > > - int virt_irq, int irq); > > + int virt_irq, int phys_irq); > > int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq); > > bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq); > > > > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c > > index b4d96b1..cfdf88f 100644 > > --- a/virt/kvm/arm/arch_timer.c > > +++ b/virt/kvm/arm/arch_timer.c > > @@ -274,7 +274,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu) > > if (timer->active_cleared_last && !phys_active) > > return; > > > > - ret = irq_set_irqchip_state(timer->map->irq, > > + ret = irq_set_irqchip_state(host_vtimer_irq, > > IRQCHIP_STATE_ACTIVE, > > phys_active); > > WARN_ON(ret); > > @@ -307,6 +307,9 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu, > > { > > struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; > > struct irq_phys_map *map; > > + struct irq_desc *desc; > > + struct irq_data *data; > > + int phys_irq; > > > > /* > > * The vcpu timer irq number cannot be determined in > > @@ -326,10 +329,25 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu, > > kvm_timer_update_state(vcpu); > > > > /* > > + * Find the physical IRQ number corresponding to the host_vtimer_irq > > + */ > > + desc = irq_to_desc(host_vtimer_irq); > > + if (!desc) { > can this really happen? this is just moving the logic. We had this check before, so I assume so... > > + kvm_err("%s: no interrupt descriptor\n", __func__); > > + return -EINVAL; > > + } > > + > > + data = irq_desc_get_irq_data(desc); > > + while (data->parent_data) > > + data = data->parent_data; > > + > > + phys_irq = data->hwirq; > > + > > + /* > > * Tell the VGIC that the virtual interrupt is tied to a > > * physical interrupt. We do that once per VCPU. > > */ > > - map = kvm_vgic_map_phys_irq(vcpu, irq->irq, host_vtimer_irq); > > + map = kvm_vgic_map_phys_irq(vcpu, irq->irq, phys_irq); > > if (WARN_ON(IS_ERR(map))) > > return PTR_ERR(map); > > > > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c > > index 2d7ae35..ac5838b 100644 > > --- a/virt/kvm/arm/vgic.c > > +++ b/virt/kvm/arm/vgic.c > > @@ -1723,27 +1723,13 @@ static struct list_head *vgic_get_irq_phys_map_list(struct kvm_vcpu *vcpu, > > * Returns a valid pointer on success, and an error pointer otherwise > > */ > the doc comment must be updated > * @irq: The Linux IRQ number > > Besides > > Reviewed-by: Eric Auger <eric.auger@linaro.org> > Thanks! Andre, let me know if you need me to provide an updated patch or if you can just tweak that comment. -Christoffer _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index 43eeb18..49c559e 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -157,7 +157,6 @@ struct vgic_io_device { struct irq_phys_map { u32 virt_irq; u32 phys_irq; - u32 irq; }; struct irq_phys_map_entry { @@ -345,7 +344,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg); int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu); struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, - int virt_irq, int irq); + int virt_irq, int phys_irq); int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq); bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq); diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c index b4d96b1..cfdf88f 100644 --- a/virt/kvm/arm/arch_timer.c +++ b/virt/kvm/arm/arch_timer.c @@ -274,7 +274,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu) if (timer->active_cleared_last && !phys_active) return; - ret = irq_set_irqchip_state(timer->map->irq, + ret = irq_set_irqchip_state(host_vtimer_irq, IRQCHIP_STATE_ACTIVE, phys_active); WARN_ON(ret); @@ -307,6 +307,9 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu, { struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; struct irq_phys_map *map; + struct irq_desc *desc; + struct irq_data *data; + int phys_irq; /* * The vcpu timer irq number cannot be determined in @@ -326,10 +329,25 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu, kvm_timer_update_state(vcpu); /* + * Find the physical IRQ number corresponding to the host_vtimer_irq + */ + desc = irq_to_desc(host_vtimer_irq); + if (!desc) { + kvm_err("%s: no interrupt descriptor\n", __func__); + return -EINVAL; + } + + data = irq_desc_get_irq_data(desc); + while (data->parent_data) + data = data->parent_data; + + phys_irq = data->hwirq; + + /* * Tell the VGIC that the virtual interrupt is tied to a * physical interrupt. We do that once per VCPU. */ - map = kvm_vgic_map_phys_irq(vcpu, irq->irq, host_vtimer_irq); + map = kvm_vgic_map_phys_irq(vcpu, irq->irq, phys_irq); if (WARN_ON(IS_ERR(map))) return PTR_ERR(map); diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 2d7ae35..ac5838b 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1723,27 +1723,13 @@ static struct list_head *vgic_get_irq_phys_map_list(struct kvm_vcpu *vcpu, * Returns a valid pointer on success, and an error pointer otherwise */ struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, - int virt_irq, int irq) + int virt_irq, int phys_irq) { struct vgic_dist *dist = &vcpu->kvm->arch.vgic; struct list_head *root = vgic_get_irq_phys_map_list(vcpu, virt_irq); struct irq_phys_map *map; struct irq_phys_map_entry *entry; - struct irq_desc *desc; - struct irq_data *data; - int phys_irq; - desc = irq_to_desc(irq); - if (!desc) { - kvm_err("%s: no interrupt descriptor\n", __func__); - return ERR_PTR(-EINVAL); - } - - data = irq_desc_get_irq_data(desc); - while (data->parent_data) - data = data->parent_data; - - phys_irq = data->hwirq; /* Create a new mapping */ entry = kzalloc(sizeof(*entry), GFP_KERNEL); @@ -1756,8 +1742,7 @@ struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, map = vgic_irq_map_search(vcpu, virt_irq); if (map) { /* Make sure this mapping matches */ - if (map->phys_irq != phys_irq || - map->irq != irq) + if (map->phys_irq != phys_irq) map = ERR_PTR(-EINVAL); /* Found an existing, valid mapping */ @@ -1767,7 +1752,6 @@ struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, map = &entry->map; map->virt_irq = virt_irq; map->phys_irq = phys_irq; - map->irq = irq; list_add_tail_rcu(&entry->entry, root);