|
[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 Mon, 2 Feb 2015, Ian Campbell wrote:
> 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?
I think that if we really need those fields, we can always use git log
to find the patch and reintroduce them. It doesn't make too much sense
to keep them now as we do nothing with them. We don't even implement
the corresponding register reads and writes in the vgic v2 driver.
> > 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 |