|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 14/15] xen/arm: vgic: Drop iactive, ipend, pendsgi field
On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> The current VGIC code doesn't support to change the pending and active status
> of an IRQ via the (re-)distributor.
> If we plan to support it in the future, it will unlikely require a specific
> bitfield as we already store the status per vIRQ.
I think we would want to track the state like we do for enabled, since
walking 32 virqs to construct the value would be a bit expensive.
Stefano, what do you think?
> Rather than wasting memory for nothing, drop thoses field. Any read to
"those"
> these registers will be RAZ and any write will print an error and inject
> a data abort to the guest.
>
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
>
> ---
> Changes in v2:
> - Patch added
> ---
> xen/arch/arm/vgic-v2.c | 71 +++++--------------------
> xen/arch/arm/vgic-v3.c | 125
> +++++++++++++++------------------------------
> xen/include/asm-arm/vgic.h | 2 +-
> 3 files changed, 55 insertions(+), 143 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 1a02541..3cf67a9 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -94,41 +94,15 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v,
> mmio_info_t *info)
> vgic_unlock_rank(v, rank, flags);
> return 1;
>
> + /* Read the pending status of an IRQ via GICD is not supported */
> case GICD_ISPENDR ... GICD_ISPENDRN:
> - if ( dabt.size != DABT_WORD ) goto bad_width;
> - rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISPENDR, DABT_WORD);
> - if ( rank == NULL) goto read_as_zero;
> - vgic_lock_rank(v, rank, flags);
> - *r = vgic_byte_read(rank->ipend, dabt.sign, gicd_reg);
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> -
> case GICD_ICPENDR ... GICD_ICPENDRN:
> - if ( dabt.size != DABT_WORD ) goto bad_width;
> - rank = vgic_rank_offset(v, 0, gicd_reg - GICD_ICPENDR, DABT_WORD);
> - if ( rank == NULL) goto read_as_zero;
> - vgic_lock_rank(v, rank, flags);
> - *r = vgic_byte_read(rank->ipend, dabt.sign, gicd_reg);
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> + goto read_as_zero;
>
> + /* Read the active status of an IRQ via GICD is not supported */
> case GICD_ISACTIVER ... GICD_ISACTIVERN:
> - if ( dabt.size != DABT_WORD ) goto bad_width;
> - rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISACTIVER, DABT_WORD);
> - if ( rank == NULL) goto read_as_zero;
> - vgic_lock_rank(v, rank, flags);
> - *r = rank->iactive;
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> -
> case GICD_ICACTIVER ... GICD_ICACTIVERN:
> - 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 = rank->iactive;
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> + goto read_as_zero;
>
> case GICD_ITARGETSR ... GICD_ITARGETSRN:
> if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto
> bad_width;
> @@ -174,23 +148,10 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v,
> mmio_info_t *info)
> *r = 0xdeadbeef;
> return 1;
>
> + /* Setting/Clearing the SGI pending bit via GICD is not supported */
> case GICD_CPENDSGIR ... GICD_CPENDSGIRN:
> - if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto
> bad_width;
> - rank = vgic_rank_offset(v, 1, gicd_reg - GICD_CPENDSGIR, DABT_WORD);
> - if ( rank == NULL) goto read_as_zero;
> - vgic_lock_rank(v, rank, flags);
> - *r = vgic_byte_read(rank->pendsgi, dabt.sign, gicd_reg);
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> -
> case GICD_SPENDSGIR ... GICD_SPENDSGIRN:
> - if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto
> bad_width;
> - rank = vgic_rank_offset(v, 1, gicd_reg - GICD_SPENDSGIR, DABT_WORD);
> - if ( rank == NULL) goto read_as_zero;
> - vgic_lock_rank(v, rank, flags);
> - *r = vgic_byte_read(rank->pendsgi, dabt.sign, gicd_reg);
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> + goto read_as_zero;
>
> /* Implementation defined -- read as zero */
> case 0xfd0 ... 0xfe4:
> @@ -346,21 +307,17 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
> mmio_info_t *info)
>
> case GICD_ISACTIVER ... GICD_ISACTIVERN:
> if ( dabt.size != DABT_WORD ) goto bad_width;
> - rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISACTIVER, DABT_WORD);
> - if ( rank == NULL) goto write_ignore;
> - vgic_lock_rank(v, rank, flags);
> - rank->iactive &= ~*r;
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> + printk(XENLOG_G_ERR
> + "%pv: vGICD: unhandled word write %#"PRIregister" to
> ISACTIVER%d\n",
> + v, *r, gicd_reg - GICD_ISACTIVER);
> + return 0;
>
> case GICD_ICACTIVER ... GICD_ICACTIVERN:
> if ( dabt.size != DABT_WORD ) goto bad_width;
> - rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICACTIVER, DABT_WORD);
> - if ( rank == NULL) goto write_ignore;
> - vgic_lock_rank(v, rank, flags);
> - rank->iactive &= ~*r;
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> + printk(XENLOG_ERR
> + "%pv: vGICD: unhandled word write %#"PRIregister" to
> ICACTIVER%d\n",
> + v, *r, gicd_reg - GICD_ICACTIVER);
> + return 0;
>
> 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 b59cc49..c68ef05 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -306,38 +306,16 @@ static int __vgic_v3_distr_common_mmio_read(const char
> *name, struct vcpu *v,
> *r = rank->ienable;
> vgic_unlock_rank(v, rank, flags);
> return 1;
> + /* Read the pending status of an IRQ via GICD/GICR is not supported */
> case GICD_ISPENDR ... GICD_ISPENDRN:
> - if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto
> bad_width;
> - rank = vgic_rank_offset(v, 1, reg - GICD_ISPENDR, DABT_WORD);
> - if ( rank == NULL ) goto read_as_zero;
> - vgic_lock_rank(v, rank, flags);
> - *r = vgic_byte_read(rank->ipend, dabt.sign, reg);
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> case GICD_ICPENDR ... GICD_ICPENDRN:
> - if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto
> bad_width;
> - rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
> - if ( rank == NULL ) goto read_as_zero;
> - vgic_lock_rank(v, rank, flags);
> - *r = vgic_byte_read(rank->ipend, dabt.sign, reg);
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> + goto read_as_zero;
> +
> + /* Read the active status of an IRQ via GICD/GICR is not supported */
> case GICD_ISACTIVER ... GICD_ISACTIVERN:
> - if ( dabt.size != DABT_WORD ) goto bad_width;
> - rank = vgic_rank_offset(v, 1, reg - GICD_ISACTIVER, DABT_WORD);
> - if ( rank == NULL ) goto read_as_zero;
> - vgic_lock_rank(v, rank, flags);
> - *r = rank->iactive;
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> case GICD_ICACTIVER ... GICD_ICACTIVERN:
> - 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 = rank->iactive;
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> + goto read_as_zero;
> +
> case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
> if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto
> bad_width;
> rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
> @@ -415,36 +393,32 @@ static int __vgic_v3_distr_common_mmio_write(const char
> *name, struct vcpu *v,
> return 1;
> case GICD_ISPENDR ... GICD_ISPENDRN:
> if ( dabt.size != DABT_WORD ) goto bad_width;
> - rank = vgic_rank_offset(v, 1, reg - GICD_ISPENDR, DABT_WORD);
> - if ( rank == NULL ) goto write_ignore;
> - vgic_lock_rank(v, rank, flags);
> - rank->ipend = *r;
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> + printk(XENLOG_G_ERR
> + "%pv: %s: unhandled word write %#"PRIregister" to
> ISPENDR%d\n",
> + v, name, *r, reg - GICD_ISPENDR);
> + return 0;
> +
> case GICD_ICPENDR ... GICD_ICPENDRN:
> if ( dabt.size != DABT_WORD ) goto bad_width;
> - rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
> - if ( rank == NULL ) goto write_ignore;
> - vgic_lock_rank(v, rank, flags);
> - rank->ipend &= ~*r;
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> + printk(XENLOG_G_ERR
> + "%pv: %s: unhandled word write %#"PRIregister" to
> ICPENDR%d\n",
> + v, name, *r, reg - GICD_ICPENDR);
> + return 0;
> +
> case GICD_ISACTIVER ... GICD_ISACTIVERN:
> if ( dabt.size != DABT_WORD ) goto bad_width;
> - rank = vgic_rank_offset(v, 1, reg - GICD_ISACTIVER, DABT_WORD);
> - if ( rank == NULL ) goto write_ignore;
> - vgic_lock_rank(v, rank, flags);
> - rank->iactive &= ~*r;
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> + printk(XENLOG_G_ERR
> + "%pv: %s: unhandled word write %#"PRIregister" to
> ISACTIVER%d\n",
> + v, name, *r, reg - GICD_ISACTIVER);
> + return 0;
> +
> case GICD_ICACTIVER ... GICD_ICACTIVERN:
> if ( dabt.size != DABT_WORD ) goto bad_width;
> - rank = vgic_rank_offset(v, 1, reg - GICD_ICACTIVER, DABT_WORD);
> - if ( rank == NULL ) goto write_ignore;
> - vgic_lock_rank(v, rank, flags);
> - rank->iactive &= ~*r;
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> + printk(XENLOG_G_ERR
> + "%pv: %s: unhandled word write %#"PRIregister" to
> ICACTIVER%d\n",
> + v, name, *r, reg - GICD_ICACTIVER);
> + return 0;
> +
> case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
> if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto
> bad_width;
> rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
> @@ -496,8 +470,6 @@ static int vgic_v3_rdistr_sgi_mmio_read(struct vcpu *v,
> mmio_info_t *info,
> struct hsr_dabt dabt = info->dabt;
> struct cpu_user_regs *regs = guest_cpu_user_regs();
> register_t *r = select_user_reg(regs, dabt.reg);
> - struct vgic_irq_rank *rank;
> - unsigned long flags;
>
> switch ( gicr_reg )
> {
> @@ -517,22 +489,12 @@ static int vgic_v3_rdistr_sgi_mmio_read(struct vcpu *v,
> mmio_info_t *info,
> */
> return __vgic_v3_distr_common_mmio_read("vGICR: SGI", v, info,
> gicr_reg);
> +
> + /* Read the pending status of an SGI is via GICR is not supported */
> case GICR_ISPENDR0:
> - if ( dabt.size != DABT_WORD ) goto bad_width;
> - rank = vgic_rank_offset(v, 1, gicr_reg - GICR_ISPENDR0, DABT_WORD);
> - if ( rank == NULL ) goto read_as_zero;
> - vgic_lock_rank(v, rank, flags);
> - *r = rank->pendsgi;
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> case GICR_ICPENDR0:
> - if ( dabt.size != DABT_WORD ) goto bad_width;
> - rank = vgic_rank_offset(v, 1, gicr_reg - GICR_ICPENDR0, DABT_WORD);
> - if ( rank == NULL ) goto read_as_zero;
> - vgic_lock_rank(v, rank, flags);
> - *r = rank->pendsgi;
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> + goto read_as_zero;
> +
> case GICR_NSACR:
> /* We do not implement security extensions for guests, read zero */
> goto read_as_zero_32;
> @@ -562,8 +524,6 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v,
> mmio_info_t *info,
> struct hsr_dabt dabt = info->dabt;
> struct cpu_user_regs *regs = guest_cpu_user_regs();
> register_t *r = select_user_reg(regs, dabt.reg);
> - struct vgic_irq_rank *rank;
> - unsigned long flags;
>
> switch ( gicr_reg )
> {
> @@ -585,22 +545,18 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu
> *v, mmio_info_t *info,
> info, gicr_reg);
> case GICR_ISPENDR0:
> if ( dabt.size != DABT_WORD ) goto bad_width;
> - rank = vgic_rank_offset(v, 1, gicr_reg - GICR_ISACTIVER0, DABT_WORD);
> - if ( rank == NULL ) goto write_ignore;
> - vgic_lock_rank(v, rank, flags);
> - /* TODO: we just store the SGI pending status. Handle it properly */
> - rank->pendsgi |= *r;
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> + printk(XENLOG_G_ERR
> + "%pv: vGICR: SGI: unhandled word write %#"PRIregister" to
> ISPENDR0\n",
> + v, *r);
> + return 0;
> +
> case GICR_ICPENDR0:
> if ( dabt.size != DABT_WORD ) goto bad_width;
> - rank = vgic_rank_offset(v, 1, gicr_reg - GICR_ISACTIVER0, DABT_WORD);
> - if ( rank == NULL ) goto write_ignore;
> - vgic_lock_rank(v, rank, flags);
> - /* TODO: we just store the SGI pending status. Handle it properly */
> - rank->pendsgi &= ~*r;
> - vgic_unlock_rank(v, rank, flags);
> - return 1;
> + printk(XENLOG_G_ERR
> + "%pv: vGICR: SGI: unhandled word write %#"PRIregister" to
> ICPENDR0\n",
> + v, *r);
> + return 0;
> +
> case GICR_NSACR:
> /* We do not implement security extensions for guests, write ignore
> */
> goto write_ignore_32;
> @@ -620,7 +576,6 @@ bad_width:
>
> write_ignore_32:
> if ( dabt.size != DABT_WORD ) goto bad_width;
> -write_ignore:
> return 1;
> }
>
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 74d5a4e..0c7da7f 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -85,7 +85,7 @@ struct pending_irq
> /* Represents state corresponding to a block of 32 interrupts */
> struct vgic_irq_rank {
> spinlock_t lock; /* Covers access to all other members of this struct */
> - uint32_t ienable, iactive, ipend, pendsgi;
> + uint32_t ienable;
> uint32_t icfg[2];
> uint32_t ipriority[8];
> union {
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |