* [RFC 1/2] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register ...
2015-08-04 11:59 [RFC 0/2] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
@ 2015-08-04 11:59 ` Julien Grall
2015-08-04 11:59 ` [RFC 2/2] xen/arm: vgic-v3: Support 32-bit access for 64-bit registers Julien Grall
2015-09-07 13:30 ` [RFC 0/2] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
2 siblings, 0 replies; 5+ messages in thread
From: Julien Grall @ 2015-08-04 11:59 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, vijay.kilari, manish.jaggi, Julien Grall,
stefano.stabellini, Vijaya.Kumar
and use them in the vGIC emulation.
The GIC registers may support different access sizes. Rather than open
coding the access for every registers, provide a set of helpers to access
them.
The caller will have to call vgic_regN_* where N is the size of the
emulated registers.
The new helpers supports any access size and expect the caller to
validate the access size supported by the emulated registers.
Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
I've only done quick tests. I sent them in order to help Vijay
supported multiple access size.
TODO:
- Use them in the vGIC v2
- Drop vgic_byte_* helpers
=> Support sign extend in common I/O handlers
- The helpers make use of uint64_t. It may be possible to
optimize a bit for arm32 to avoid uint64_t. Although I'm not
sure it's worth to do it.
---
xen/arch/arm/vgic-v3.c | 103 ++++++++++++++++++++++++++++----------------
xen/include/asm-arm/vgic.h | 104 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 171 insertions(+), 36 deletions(-)
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index f1c482d..7ef7b16 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -109,7 +109,6 @@ static int __vgic_v3_rdistr_rd_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);
- uint64_t aff;
switch ( gicr_reg )
{
@@ -118,21 +117,27 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
goto read_as_zero_32;
case GICR_IIDR:
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICR_IIDR_VAL;
+ *r = vgic_reg32_read(GICV3_GICR_IIDR_VAL, info);
return 1;
case GICR_TYPER:
+ {
+ uint64_t typer, aff;
+
if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
/* TBD: Update processor id in [23:8] when ITS support is added */
aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 |
MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 |
MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 |
MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32);
- *r = aff;
+ typer = aff;
if ( v->arch.vgic.flags & VGIC_V3_RDIST_LAST )
- *r |= GICR_TYPER_LAST;
+ typer |= GICR_TYPER_LAST;
+
+ *r = vgic_reg64_read(typer, info);
return 1;
+ }
case GICR_STATUSR:
/* Not implemented */
goto read_as_zero_32;
@@ -161,7 +166,7 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
case GICR_SYNCR:
if ( dabt.size != DABT_WORD ) goto bad_width;
/* RO . But when read it always returns busy bito bit[0] */
- *r = GICR_SYNCR_NOT_BUSY;
+ *r = vgic_reg32_read(GICR_SYNCR_NOT_BUSY, info);
return 1;
case GICR_MOVLPIR:
/* WO Read as zero */
@@ -171,22 +176,22 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
goto read_as_zero_64;
case GICR_PIDR0:
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICR_PIDR0;
+ *r = vgic_reg32_read(GICV3_GICR_PIDR0, info);
return 1;
case GICR_PIDR1:
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICR_PIDR1;
+ *r = vgic_reg32_read(GICV3_GICR_PIDR1, info);
return 1;
case GICR_PIDR2:
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICR_PIDR2;
+ *r = vgic_reg32_read(GICV3_GICR_PIDR2, info);
return 1;
case GICR_PIDR3:
/* Manufacture/customer defined */
goto read_as_zero_32;
case GICR_PIDR4:
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICR_PIDR4;
+ *r = vgic_reg32_read(GICV3_GICR_PIDR4, info);
return 1;
case GICR_PIDR5 ... GICR_PIDR7:
/* Reserved0 */
@@ -309,7 +314,7 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
rank = vgic_rank_offset(v, 1, reg - GICD_ISENABLER, DABT_WORD);
if ( rank == NULL ) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
- *r = rank->ienable;
+ *r = vgic_reg32_read(rank->ienable, info);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICENABLER ... GICD_ICENABLERN:
@@ -317,7 +322,7 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
rank = vgic_rank_offset(v, 1, reg - GICD_ICENABLER, DABT_WORD);
if ( rank == NULL ) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
- *r = rank->ienable;
+ *r = vgic_reg32_read(rank->ienable, info);
vgic_unlock_rank(v, rank, flags);
return 1;
/* Read the pending status of an IRQ via GICD/GICR is not supported */
@@ -331,25 +336,37 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
goto read_as_zero;
case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
+ {
+ uint32_t ipriority;
+
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
if ( rank == NULL ) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
- *r = rank->ipriority[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
- DABT_WORD)];
- if ( dabt.size == DABT_BYTE )
- *r = vgic_byte_read(*r, dabt.sign, reg);
+ ipriority = rank->ipriority[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
+ DABT_WORD)];
vgic_unlock_rank(v, rank, flags);
+
+ *r = vgic_reg32_read(ipriority, info);
+
return 1;
+ }
case GICD_ICFGR ... GICD_ICFGRN:
+ {
+ uint32_t icfgr;
+
if ( dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
if ( rank == NULL ) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
- *r = rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR, DABT_WORD)];
+ icfgr = rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR, DABT_WORD)];
vgic_unlock_rank(v, rank, flags);
+
+ *r = vgic_reg32_read(icfgr, info);
+
return 1;
+ }
default:
printk(XENLOG_G_ERR
"%pv: %s: unhandled read r%d offset %#08x\n",
@@ -389,7 +406,7 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
if ( rank == NULL ) goto write_ignore;
vgic_lock_rank(v, rank, flags);
tr = rank->ienable;
- rank->ienable |= *r;
+ vgic_reg32_setbit(&rank->ienable, *r, info);
/* The irq number is extracted from offset. so shift by register size */
vgic_enable_irqs(v, (*r) & (~tr), (reg - GICD_ISENABLER) >> DABT_WORD);
vgic_unlock_rank(v, rank, flags);
@@ -400,6 +417,7 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
if ( rank == NULL ) goto write_ignore;
vgic_lock_rank(v, rank, flags);
tr = rank->ienable;
+ vgic_reg32_clearbit(&rank->ienable, *r, info);
rank->ienable &= ~*r;
/* The irq number is extracted from offset. so shift by register size */
vgic_disable_irqs(v, (*r) & tr, (reg - GICD_ICENABLER) >> DABT_WORD);
@@ -438,12 +456,10 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
if ( rank == NULL ) goto write_ignore;
vgic_lock_rank(v, rank, flags);
- if ( dabt.size == DABT_WORD )
- rank->ipriority[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
- DABT_WORD)] = *r;
- else
- vgic_byte_write(&rank->ipriority[REG_RANK_INDEX(8,
- reg - GICD_IPRIORITYR, DABT_WORD)], *r, reg);
+ vgic_reg32_write(&rank->ipriority[REG_RANK_INDEX(8,
+ reg - GICD_IPRIORITYR,
+ DABT_WORD)],
+ *r, info);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICFGR: /* Restricted to configure SGIs */
@@ -455,7 +471,9 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
if ( rank == NULL ) goto write_ignore;
vgic_lock_rank(v, rank, flags);
- rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR, DABT_WORD)] = *r;
+ vgic_reg32_write(&rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR,
+ DABT_WORD)],
+ *r, info);
vgic_unlock_rank(v, rank, flags);
return 1;
default:
@@ -694,7 +712,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
case GICD_CTLR:
if ( dabt.size != DABT_WORD ) goto bad_width;
vgic_lock(v);
- *r = v->domain->arch.vgic.ctlr;
+ *r = vgic_reg32_read(v->domain->arch.vgic.ctlr, info);
vgic_unlock(v);
return 1;
case GICD_TYPER:
@@ -709,13 +727,16 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
* bit is zero. The maximum is 8.
*/
unsigned int ncpus = min_t(unsigned int, v->domain->max_vcpus, 8);
+ uint32_t typer;
if ( dabt.size != DABT_WORD ) goto bad_width;
/* No secure world support for guests. */
- *r = ((ncpus - 1) << GICD_TYPE_CPUS_SHIFT |
- DIV_ROUND_UP(v->domain->arch.vgic.nr_spis, 32));
+ typer = ((ncpus - 1) << GICD_TYPE_CPUS_SHIFT |
+ DIV_ROUND_UP(v->domain->arch.vgic.nr_spis, 32));
+
+ typer |= (irq_bits - 1) << GICD_TYPE_ID_BITS_SHIFT;
- *r |= (irq_bits - 1) << GICD_TYPE_ID_BITS_SHIFT;
+ *r = vgic_reg32_read(typer, info);
return 1;
}
@@ -727,7 +748,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
goto read_as_zero_32;
case GICD_IIDR:
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICD_IIDR_VAL;
+ *r = vgic_reg32_read(GICV3_GICD_IIDR_VAL, info);
return 1;
case 0x020 ... 0x03c:
case 0xc000 ... 0xffcc:
@@ -750,15 +771,23 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
/* SGI/PPI is RES0 */
goto read_as_zero_64;
case GICD_IROUTER32 ... GICD_IROUTERN:
+ {
+ uint64_t irouter;
+
if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
DABT_DOUBLE_WORD);
if ( rank == NULL ) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
- *r = rank->v3.irouter[REG_RANK_INDEX(64,
- (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)];
+ irouter = rank->v3.irouter[REG_RANK_INDEX(64,
+ (gicd_reg - GICD_IROUTER),
+ DABT_DOUBLE_WORD)];
vgic_unlock_rank(v, rank, flags);
+
+ *r = vgic_reg64_read(irouter, info);
+
return 1;
+ }
case GICD_NSACR ... GICD_NSACRN:
/* We do not implement security extensions for guests, read zero */
goto read_as_zero_32;
@@ -774,17 +803,17 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
case GICD_PIDR0:
/* GICv3 identification value */
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICD_PIDR0;
+ *r = vgic_reg32_read(GICV3_GICD_PIDR0, info);
return 1;
case GICD_PIDR1:
/* GICv3 identification value */
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICD_PIDR1;
+ *r = vgic_reg32_read(GICV3_GICD_PIDR1, info);
return 1;
case GICD_PIDR2:
/* GICv3 identification value */
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICD_PIDR2;
+ *r = vgic_reg32_read(GICV3_GICD_PIDR2, info);
return 1;
case GICD_PIDR3:
/* GICv3 identification value. Manufacturer/Customer defined */
@@ -792,7 +821,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
case GICD_PIDR4:
/* GICv3 identification value */
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICD_PIDR4;
+ *r = vgic_reg32_read(GICV3_GICD_PIDR4, info);
return 1;
case GICD_PIDR5 ... GICD_PIDR7:
/* Reserved0 */
@@ -910,12 +939,14 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
DABT_DOUBLE_WORD);
if ( rank == NULL ) goto write_ignore;
- new_irouter = *r;
vgic_lock_rank(v, rank, flags);
old_irouter = rank->v3.irouter[REG_RANK_INDEX(64,
(gicd_reg - GICD_IROUTER),
DABT_DOUBLE_WORD)];
+ new_irouter = old_irouter;
+ vgic_reg64_write(&new_irouter, *r, info);
+
old_vcpu = vgic_v3_irouter_to_vcpu(v->domain, old_irouter);
new_vcpu = vgic_v3_irouter_to_vcpu(v->domain, new_irouter);
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 41cadb1..f995ddf 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -19,6 +19,7 @@
#define __ASM_ARM_VGIC_H__
#include <xen/bitops.h>
+#include <asm/mmio.h>
struct pending_irq
{
@@ -180,6 +181,109 @@ static inline void vgic_byte_write(uint32_t *reg, uint32_t var, int offset)
*reg |= var;
}
+#define VGIC_REG_MASK(size) ((1ULL << (((size) + 1) * 8)) - 1)
+
+/*
+ * The check on the size supported by the register has to be done by
+ * the caller of vgic_regN_*.
+ *
+ * vgic_reg_* should never be called directly. Instead use the vgic_regN_*
+ * according to size of the emulated register
+ *
+ * Note that the alignment fault will always be taken in the guest
+ * (see B3.12.7 DDI0406.b).
+ */
+static inline register_t vgic_reg_read(uint64_t reg,
+ unsigned int offset,
+ enum dabt_size size)
+{
+ reg >>= 8 * offset;
+ reg &= VGIC_REG_MASK(size);
+
+ return reg;
+}
+
+static inline void vgic_reg_write(uint64_t *reg, register_t val,
+ unsigned int offset,
+ enum dabt_size size)
+{
+ uint64_t mask = VGIC_REG_MASK(size);
+ int shift = offset * 8;
+
+ *reg &= ~(mask << shift);
+ *reg |= ((uint64_t)val & mask) << shift;
+}
+
+static inline void vgic_reg_setbit(uint64_t *reg, register_t bits,
+ unsigned int offset,
+ enum dabt_size size)
+{
+ uint64_t mask = VGIC_REG_MASK(size);
+ int shift = offset * 8;
+
+ *reg |= ((uint64_t)bits & mask) << shift;
+}
+
+static inline void vgic_reg_clearbit(uint64_t *reg, register_t bits,
+ unsigned int offset,
+ enum dabt_size size)
+{
+ uint64_t mask = VGIC_REG_MASK(size);
+ int shift = offset * 8;
+
+ *reg &= ~(((uint64_t)bits & mask) << shift);
+}
+
+/* N-bit register helpers */
+#define VGIC_REG_HELPERS(sz, offmask) \
+static inline register_t vgic_reg##sz##_read(uint##sz##_t reg, \
+ const mmio_info_t *info) \
+{ \
+ return vgic_reg_read(reg, info->gpa & offmask, \
+ info->dabt.size); \
+} \
+ \
+static inline void vgic_reg##sz##_write(uint##sz##_t *reg, \
+ register_t val, \
+ const mmio_info_t *info) \
+{ \
+ uint64_t tmp = *reg; \
+ \
+ vgic_reg_write(&tmp, val, info->gpa & offmask, \
+ info->dabt.size); \
+ \
+ *reg = tmp; \
+} \
+ \
+static inline void vgic_reg##sz##_setbit(uint##sz##_t *reg, \
+ register_t bits, \
+ const mmio_info_t *info) \
+{ \
+ uint64_t tmp = *reg; \
+ \
+ vgic_reg_setbit(&tmp, bits, info->gpa & offmask, \
+ info->dabt.size); \
+ \
+ *reg = tmp; \
+} \
+ \
+static inline void vgic_reg##sz##_clearbit(uint##sz##_t *reg, \
+ register_t bits, \
+ const mmio_info_t *info) \
+{ \
+ uint64_t tmp = *reg; \
+ \
+ vgic_reg_clearbit(&tmp, bits, info->gpa & offmask, \
+ info->dabt.size); \
+ \
+ *reg = tmp; \
+}
+
+VGIC_REG_HELPERS(64, 0x7);
+VGIC_REG_HELPERS(32, 0x3);
+
+#undef VGIC_REG_HELPERS
+
enum gic_sgi_mode;
/*
--
2.1.4
^ permalink raw reply related [flat|nested] 5+ messages in thread