* [PATCH] xen/arm: implement GICD_ICACTIVER read/write
@ 2015-11-25 16:40 Stefano Stabellini
2015-11-26 1:40 ` Shannon Zhao
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Stefano Stabellini @ 2015-11-25 16:40 UTC (permalink / raw)
To: xen-devel; +Cc: ian.campbell, stefano.stabellini
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.
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.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
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);
+ }
+
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 */
+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) )
+ {
+ 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);
+ 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;
+ }
+ 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++;
+ }
+ 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;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 0116481..0061368 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -287,6 +287,7 @@ extern unsigned int gic_number_lines(void);
int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
unsigned int *out_hwirq, unsigned int *out_type);
void gic_clear_lrs(struct vcpu *v);
+void gic_deactivate_irq(struct vcpu *v, unsigned int irq);
struct gic_info {
/* GIC version */
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index cb51a9e..9845c13 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -59,12 +59,16 @@ struct pending_irq
* vcpu while it is still inflight and on an GICH_LR register on the
* old vcpu.
*
+ * GIC_IRQ_GUEST_DEACTIVATE: an explicit deactivation request has
+ * been made by the guest (for example writing to GICD_ICACTIVER).
+ *
*/
#define GIC_IRQ_GUEST_QUEUED 0
#define GIC_IRQ_GUEST_ACTIVE 1
#define GIC_IRQ_GUEST_VISIBLE 2
#define GIC_IRQ_GUEST_ENABLED 3
#define GIC_IRQ_GUEST_MIGRATING 4
+#define GIC_IRQ_GUEST_DEACTIVATE 5
unsigned long status;
struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
unsigned int irq;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] xen/arm: implement GICD_ICACTIVER read/write
2015-11-25 16:40 [PATCH] xen/arm: implement GICD_ICACTIVER read/write Stefano Stabellini
@ 2015-11-26 1:40 ` Shannon Zhao
2015-11-30 12:35 ` Julien Grall
2015-11-30 13:59 ` Julien Grall
2 siblings, 0 replies; 5+ messages in thread
From: Shannon Zhao @ 2015-11-26 1:40 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: ian.campbell
On 2015/11/26 0: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.
>
> 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.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Tested-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> 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);
> + }
> +
> 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 */
> +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) )
> + {
> + 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);
> + 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;
> + }
> + 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++;
> + }
> + 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;
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 0116481..0061368 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -287,6 +287,7 @@ extern unsigned int gic_number_lines(void);
> int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
> unsigned int *out_hwirq, unsigned int *out_type);
> void gic_clear_lrs(struct vcpu *v);
> +void gic_deactivate_irq(struct vcpu *v, unsigned int irq);
>
> struct gic_info {
> /* GIC version */
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index cb51a9e..9845c13 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -59,12 +59,16 @@ struct pending_irq
> * vcpu while it is still inflight and on an GICH_LR register on the
> * old vcpu.
> *
> + * GIC_IRQ_GUEST_DEACTIVATE: an explicit deactivation request has
> + * been made by the guest (for example writing to GICD_ICACTIVER).
> + *
> */
> #define GIC_IRQ_GUEST_QUEUED 0
> #define GIC_IRQ_GUEST_ACTIVE 1
> #define GIC_IRQ_GUEST_VISIBLE 2
> #define GIC_IRQ_GUEST_ENABLED 3
> #define GIC_IRQ_GUEST_MIGRATING 4
> +#define GIC_IRQ_GUEST_DEACTIVATE 5
> unsigned long status;
> struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
> unsigned int irq;
>
--
Shannon
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] xen/arm: implement GICD_ICACTIVER read/write
2015-11-25 16:40 [PATCH] xen/arm: implement GICD_ICACTIVER read/write Stefano Stabellini
2015-11-26 1:40 ` Shannon Zhao
@ 2015-11-30 12:35 ` Julien Grall
2015-11-30 13:59 ` Julien Grall
2 siblings, 0 replies; 5+ messages in thread
From: Julien Grall @ 2015-11-30 12:35 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: ian.campbell
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@eu.citrix.com>
> ---
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] xen/arm: implement GICD_ICACTIVER read/write
2015-11-25 16:40 [PATCH] xen/arm: implement GICD_ICACTIVER read/write Stefano Stabellini
2015-11-26 1:40 ` Shannon Zhao
2015-11-30 12:35 ` Julien Grall
@ 2015-11-30 13:59 ` Julien Grall
2015-12-03 10:48 ` Stefano Stabellini
2 siblings, 1 reply; 5+ messages in thread
From: Julien Grall @ 2015-11-30 13:59 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: ian.campbell
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.
Looking to the Linux tree again, I found this commit:
commit 1c7d4dd46ee048afe19e1294634df6fa66862519
Author: Marc Zyngier <marc.zyngier@arm.com>
Date: Mon Nov 16 19:13:28 2015 +0000
irqchip/gic: Add save/restore of the active state
When using EOImode==1, we may mark interrupts as being forwarded
to a virtual machine. In that case, the interrupt is left active
while being passed to the VM.
If we suspend the system before the VM has deactivated the interrupt,
the active state will be lost (which may be very annoying, as this
may result in spurious interrupts and a confused guest).
To avoid this, save and restore the active state together with the
rest of the GIC registers.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: <linux-arm-kernel@lists.infradead.org>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Russell King <linux@arm.linux.org.uk>
Link:
http://lkml.kernel.org/r/1447701208-18150-5-git-send-email-marc.zyngier@arm.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
So, we have to handle properly active/deactivate in order to get
suspend/resume working correctly.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] xen/arm: implement GICD_ICACTIVER read/write
2015-11-30 13:59 ` Julien Grall
@ 2015-12-03 10:48 ` Stefano Stabellini
0 siblings, 0 replies; 5+ messages in thread
From: Stefano Stabellini @ 2015-12-03 10:48 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, ian.campbell, Stefano Stabellini
On Mon, 30 Nov 2015, Julien Grall wrote:
> 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.
>
> Looking to the Linux tree again, I found this commit:
>
> commit 1c7d4dd46ee048afe19e1294634df6fa66862519
> Author: Marc Zyngier <marc.zyngier@arm.com>
> Date: Mon Nov 16 19:13:28 2015 +0000
>
> irqchip/gic: Add save/restore of the active state
>
> When using EOImode==1, we may mark interrupts as being forwarded
> to a virtual machine. In that case, the interrupt is left active
> while being passed to the VM.
>
> If we suspend the system before the VM has deactivated the interrupt,
> the active state will be lost (which may be very annoying, as this
> may result in spurious interrupts and a confused guest).
>
> To avoid this, save and restore the active state together with the
> rest of the GIC registers.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Cc: <linux-arm-kernel@lists.infradead.org>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Russell King <linux@arm.linux.org.uk>
> Link:
> http://lkml.kernel.org/r/1447701208-18150-5-git-send-email-marc.zyngier@arm.com
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>
> So, we have to handle properly active/deactivate in order to get
> suspend/resume working correctly.
We won't have any active interrupts: suspending the guest is done in
cooperation with the guest kernel (see drivers/xen/manage.c), which will
deactive the interrupts. In any case we only have evtchn_irq and the
timers.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-12-03 10:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-25 16:40 [PATCH] xen/arm: implement GICD_ICACTIVER read/write Stefano Stabellini
2015-11-26 1:40 ` Shannon Zhao
2015-11-30 12:35 ` Julien Grall
2015-11-30 13:59 ` Julien Grall
2015-12-03 10:48 ` Stefano Stabellini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.