|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] xen/arm: implement GICD_ICACTIVER read/write
Hi Stefano,
On 25/11/15 16:40, Stefano Stabellini wrote:
> Implement GICD_ICACTIVER and GICD_ISACTIVER reads by looking for the
> GIC_IRQ_GUEST_ACTIVE bit in the relevant struct pending_irq. However
> given that the pending to active transaction for irqs in LRs in done in
> hardware, the GIC_IRQ_GUEST_ACTIVE bit might be out of date. We'll have
> to live with that.
Is it acceptable? The guest may rely on it to save the active state of
an IRQ.
> Implement GICD_ICACTIVER writes by checking the state of the irq in our
> queues: if the irq is present in an LR, remove the hardware ACTIVE bit.
> If the irq is present in an LR of another vcpu, send an IPI. Set the
> GIC_IRQ_GUEST_DEACTIVATE bit to tell the receiving vcpu that the active
> bit needs to be deactivated.
I would explain that the write bits of GICD_IACTIVER is left unimplemented.
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> ---
> xen/arch/arm/gic.c | 40 +++++++++++++++++++++++++++++++++++++++
> xen/arch/arm/vgic-v2.c | 45
> ++++++++++++++++++++++++++++++++++++++------
> xen/arch/arm/vgic-v3.c | 44 ++++++++++++++++++++++++++++++++++++++-----
> xen/include/asm-arm/gic.h | 1 +
> xen/include/asm-arm/vgic.h | 4 ++++
> 5 files changed, 123 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 1e1e5ba..75c1f52 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -414,6 +414,15 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> gic_hw_ops->read_lr(i, &lr_val);
> irq = lr_val.virq;
> p = irq_to_pending(v, irq);
> +
> + if ( test_and_clear_bit(GIC_IRQ_GUEST_DEACTIVATE, &p->status) &&
> + (lr_val.state & GICH_LR_ACTIVE) )
> + {
> + clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> + lr_val.state &= ~GICH_LR_ACTIVE;
> + gic_hw_ops->write_lr(i, &lr_val);
You also have to handle the deactivation of an hardware IRQ route to a
guest.
> + }
> +
> if ( lr_val.state & GICH_LR_ACTIVE )
> {
> set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> @@ -489,6 +498,37 @@ void gic_clear_lrs(struct vcpu *v)
> spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> }
>
> +/* called with rank lock held */
Please add an ASSERT in the code to check this assumption.
> +void gic_deactivate_irq(struct vcpu *v, unsigned int irq)
> +{
> + unsigned long flags;
> + struct pending_irq *p;
> + struct vcpu *v_target = v->domain->arch.vgic.handler->get_target_vcpu(v,
> irq);
> +
> + spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
> +
> + p = irq_to_pending(v_target, irq);
> + /* the interrupt is not even in an LR */
> + if ( list_empty(&p->inflight) || !list_empty(&p->lr_queue) )
I think this can be simplified by testing p->lr == GIC_INVALID_LR
> + {
> + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> + return;
> + }
> +
> + /* it is in an LR, let's check */
> + set_bit(GIC_IRQ_GUEST_DEACTIVATE, &p->status);
> + if ( v_target == current )
> + {
> + gic_update_one_lr(v_target, p->lr);
> + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> + } else {
> + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> + vcpu_unblock(v_target);
Why do you need to unblock the vCPU?
> + if (v_target->is_running )
> + smp_send_event_check_mask(cpumask_of(v_target->processor));
> + }
> +}
> +
> static void gic_restore_pending_irqs(struct vcpu *v)
> {
> int lr = 0;
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index f7d784b..9042062 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -126,8 +126,31 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v,
> mmio_info_t *info,
> /* Read the active status of an IRQ via GICD is not supported */
> case GICD_ISACTIVER ... GICD_ISACTIVERN:
> case GICD_ICACTIVER ... GICD_ICACTIVERN:
> - goto read_as_zero;
> -
> + {
> + unsigned int i = 0, irq = 0;
> + struct pending_irq *p;
> + if ( dabt.size != DABT_WORD ) goto bad_width;
> + rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICACTIVER, DABT_WORD);
> + if ( rank == NULL) goto read_as_zero;
> + vgic_lock_rank(v, rank, flags);
> + *r = 0;
> + irq = (gicd_reg - GICD_ICACTIVER) << 3;
> + for (i = 0; i < 32; i++)
> + {
> + p = irq_to_pending(v, i + irq);
> + /*
> + * This information is likely out of date because we don't
> + * actually know which interrupts have become ACTIVE from
> + * PENDING in the LRs of other processors at it happens
> + * transparently in hardware. We would have to interrupt
> + * all other running vcpus to get an accurate snapshot.
> + * Let's not do that.
> + */
> + *r |= test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) ? (1 << i) : 0;
> + }
Rather than duplicating the code twice (vgic-v2 and vgic-v3), could you
create a stub function to fetch the value. See vgic_fetch_itargetsr for
instance.
> + vgic_unlock_rank(v, rank, flags);
> + return 1;
> + }
> case GICD_ITARGETSR ... GICD_ITARGETSRN:
> if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto
> bad_width;
> rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD);
> @@ -332,11 +355,21 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
> mmio_info_t *info,
> return 0;
>
> case GICD_ICACTIVER ... GICD_ICACTIVERN:
> + {
> + unsigned int i = 0, irq;
> if ( dabt.size != DABT_WORD ) goto bad_width;
> - printk(XENLOG_G_ERR
> - "%pv: vGICD: unhandled word write %#"PRIregister" to
> ICACTIVER%d\n",
> - v, r, gicd_reg - GICD_ICACTIVER);
> - return 0;
> + rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICACTIVER, DABT_WORD);
> + if ( rank == NULL) goto write_ignore;
> + vgic_lock_rank(v, rank, flags);
> + irq = (gicd_reg - GICD_ICACTIVER) << 3;
> + while ( (i = find_next_bit(&r, 32, i)) < 32 )
> + {
> + gic_deactivate_irq(v, i + irq);
> + i++;
> + }
Ditto.
> + vgic_unlock_rank(v, rank, flags);
> + return 1;
> + }
>
> case GICD_ITARGETSR ... GICD_ITARGETSR + 7:
> /* SGI/PPI target is read only */
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index b5249ff..c779f75 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -325,7 +325,31 @@ static int __vgic_v3_distr_common_mmio_read(const char
> *name, struct vcpu *v,
> /* Read the active status of an IRQ via GICD/GICR is not supported */
> case GICD_ISACTIVER ... GICD_ISACTIVERN:
> case GICD_ICACTIVER ... GICD_ICACTIVERN:
> - goto read_as_zero;
> + {
> + unsigned int i = 0, irq = 0;
> + struct pending_irq *p;
> + if ( dabt.size != DABT_WORD ) goto bad_width;
> + rank = vgic_rank_offset(v, 1, reg - GICD_ICACTIVER, DABT_WORD);
> + if ( rank == NULL) goto read_as_zero;
> + vgic_lock_rank(v, rank, flags);
> + *r = 0;
> + irq = (reg - GICD_ICACTIVER) << 3;
> + for (i = 0; i < 32; i++)
> + {
> + p = irq_to_pending(v, i + irq);
> + /*
> + * This information is likely out of date because we don't
> + * actually know which interrupts have become ACTIVE from
> + * PENDING in the LRs of other processors at it happens
> + * transparently in hardware. We would have to interrupt
> + * all other running vcpus to get an accurate snapshot.
> + * Let's not do that.
> + */
> + *r |= test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) ? (1 << i) : 0;
> + }
> + vgic_unlock_rank(v, rank, flags);
> + return 1;
> + }
>
> case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
> if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto
> bad_width;
> @@ -421,11 +445,21 @@ static int __vgic_v3_distr_common_mmio_write(const char
> *name, struct vcpu *v,
> return 0;
>
> case GICD_ICACTIVER ... GICD_ICACTIVERN:
> + {
> + unsigned int i = 0, irq;
> if ( dabt.size != DABT_WORD ) goto bad_width;
> - printk(XENLOG_G_ERR
> - "%pv: %s: unhandled word write %#"PRIregister" to
> ICACTIVER%d\n",
> - v, name, r, reg - GICD_ICACTIVER);
> - return 0;
> + rank = vgic_rank_offset(v, 1, reg - GICD_ICACTIVER, DABT_WORD);
> + if ( rank == NULL) goto write_ignore;
> + vgic_lock_rank(v, rank, flags);
> + irq = (reg - GICD_ICACTIVER) << 3;
> + while ( (i = find_next_bit(&r, 32, i)) < 32 )
> + {
> + gic_deactivate_irq(v, i + irq);
> + i++;
> + }
> + vgic_unlock_rank(v, rank, flags);
> + return 1;
> + }
>
> case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
> if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto
> bad_width;
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |