* [PATCH v2 1/2] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register ...
@ 2015-08-28 13:40 vijay.kilari
2015-08-28 13:40 ` [PATCH v2 2/2] xen/arm: vgic-v3: Support 32-bit access for 64-bit registers vijay.kilari
2015-08-28 13:58 ` [PATCH v2 1/2] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register Julien Grall
0 siblings, 2 replies; 3+ messages in thread
From: vijay.kilari @ 2015-08-28 13:40 UTC (permalink / raw)
To: Ian.Campbell, julien.grall, stefano.stabellini,
stefano.stabellini, tim, xen-devel
Cc: Prasun.Kapoor, vijaya.kumar, vijay.kilari
From: Julien Grall <julien.grall@citrix.com>
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>
---
v2: - VGIC_REG_MASK macro does not compute mask correctly.
So replace it with inline function and compute mask
correctly for all sizes
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..af1fa17 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,115 @@ static inline void vgic_byte_write(uint32_t *reg, uint32_t var, int offset)
*reg |= var;
}
+static inline uint64_t vgic_reg_mask(enum dabt_size size)
+{
+ if ( size == DABT_DOUBLE_WORD )
+ return ~0ULL;
+ else
+ return ((1ULL << ((1 << size) * 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;
/*
--
1.7.9.5
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH v2 2/2] xen/arm: vgic-v3: Support 32-bit access for 64-bit registers
2015-08-28 13:40 [PATCH v2 1/2] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register vijay.kilari
@ 2015-08-28 13:40 ` vijay.kilari
2015-08-28 13:58 ` [PATCH v2 1/2] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register Julien Grall
1 sibling, 0 replies; 3+ messages in thread
From: vijay.kilari @ 2015-08-28 13:40 UTC (permalink / raw)
To: Ian.Campbell, julien.grall, stefano.stabellini,
stefano.stabellini, tim, xen-devel
Cc: Prasun.Kapoor, vijaya.kumar, vijay.kilari
From: Julien Grall <julien.grall@citrix.com>
Based on 8.1.3 (IHI 0069A), unless stated otherwise, the 64-bit registers
supports both 32-bit and 64-bits access.
All the registers we properly emulate (i.e not RAZ/WI) supports 32-bit access.
For RAZ/WI, it's also seems to be the case but I'm not 100% sure. Anyway,
emulating 32-bit access for them doesn't hurt. Note that we would need
some extra care when they will be implemented (for instance GICR_PROPBASER).
Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
v2: - export vgic_reg64_check_access() to be used later by vITS
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 7ef7b16..daa510a 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -123,7 +123,7 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
{
uint64_t typer, aff;
- if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
+ if ( !vgic_reg64_check_access(dabt) ) 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 |
@@ -209,7 +209,7 @@ bad_width:
return 0;
read_as_zero_64:
- if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
+ if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
*r = 0;
return 1;
@@ -286,7 +286,7 @@ bad_width:
return 0;
write_ignore_64:
- if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
+ if ( vgic_reg64_check_access(dabt) ) goto bad_width;
return 1;
write_ignore_32:
@@ -774,7 +774,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
{
uint64_t irouter;
- if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
+ if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
DABT_DOUBLE_WORD);
if ( rank == NULL ) goto read_as_zero;
@@ -850,7 +850,7 @@ bad_width:
return 0;
read_as_zero_64:
- if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
+ if ( vgic_reg64_check_access(dabt) ) goto bad_width;
*r = 0;
return 1;
@@ -935,7 +935,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
/* SGI/PPI is RES0 */
goto write_ignore_64;
case GICD_IROUTER32 ... GICD_IROUTERN:
- if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
+ if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
DABT_DOUBLE_WORD);
if ( rank == NULL ) goto write_ignore;
@@ -1016,7 +1016,7 @@ write_ignore_32:
return 1;
write_ignore_64:
- if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
+ if ( vgic_reg64_check_access(dabt) ) goto bad_width;
return 1;
write_ignore:
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index af1fa17..62d24b6 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -199,6 +199,15 @@ static inline uint64_t vgic_reg_mask(enum dabt_size size)
* Note that the alignment fault will always be taken in the guest
* (see B3.12.7 DDI0406.b).
*/
+static inline bool vgic_reg64_check_access(struct hsr_dabt dabt)
+{
+ /*
+ * 64 bits registers can be accessible using 32-bit and 64-bit unless
+ * stated otherwise (See 8.1.3 ARM IHI 0069A).
+ */
+ return ( dabt.size == DABT_DOUBLE_WORD || dabt.size == DABT_WORD );
+}
+
static inline register_t vgic_reg_read(uint64_t reg,
unsigned int offset,
enum dabt_size size)
--
1.7.9.5
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2 1/2] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register ...
2015-08-28 13:40 [PATCH v2 1/2] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register vijay.kilari
2015-08-28 13:40 ` [PATCH v2 2/2] xen/arm: vgic-v3: Support 32-bit access for 64-bit registers vijay.kilari
@ 2015-08-28 13:58 ` Julien Grall
1 sibling, 0 replies; 3+ messages in thread
From: Julien Grall @ 2015-08-28 13:58 UTC (permalink / raw)
To: vijay.kilari, Ian.Campbell, stefano.stabellini,
stefano.stabellini, tim, xen-devel
Cc: Prasun.Kapoor, vijaya.kumar
Hi Vijay,
You took over this series without asking me if I was still working on
it... You don't even address all the TODOs I mentioned in the cover
letter of v1 [1] which you didn't bother to retain.
If I didn't send a new version of the patch series it's because I was
waiting reviews from the ARM maintainers and hadn't had time to complete
my TODO list.
So please don't take over any of my series unless you explicitly asked
me if it's fine. Just report the bugs on the mailing list.
Regards,
[1] http://lists.xen.org/archives/html/xen-devel/2015-08/msg00168.html
On 28/08/15 14:40, vijay.kilari@gmail.com wrote:
> From: Julien Grall <julien.grall@citrix.com>
>
> 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>
> ---
> v2: - VGIC_REG_MASK macro does not compute mask correctly.
> So replace it with inline function and compute mask
> correctly for all sizes
>
> 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..af1fa17 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,115 @@ static inline void vgic_byte_write(uint32_t *reg, uint32_t var, int offset)
> *reg |= var;
> }
>
> +static inline uint64_t vgic_reg_mask(enum dabt_size size)
> +{
> + if ( size == DABT_DOUBLE_WORD )
> + return ~0ULL;
> + else
> + return ((1ULL << ((1 << size) * 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;
>
> /*
>
--
Julien Grall
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-08-28 13:58 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-28 13:40 [PATCH v2 1/2] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register vijay.kilari
2015-08-28 13:40 ` [PATCH v2 2/2] xen/arm: vgic-v3: Support 32-bit access for 64-bit registers vijay.kilari
2015-08-28 13:58 ` [PATCH v2 1/2] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register Julien Grall
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.