* [PATCH v3 1/9] xen/arm: io: remove mmio_check_t typedef
2015-10-07 14:41 [PATCH v3 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
@ 2015-10-07 14:41 ` Julien Grall
2015-10-07 14:41 ` [PATCH v3 2/9] xen/arm: io: Extend write/read handler to pass the register in parameter Julien Grall
` (8 subsequent siblings)
9 siblings, 0 replies; 34+ messages in thread
From: Julien Grall @ 2015-10-07 14:41 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, ian.campbell, stefano.stabellini
This typedef is a left-over of the previous MMIO emulation
implementation.
Signed-off-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v2:
- Add Ian's Acked-by
Changes in v1:
- Patch added
---
xen/include/asm-arm/mmio.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/xen/include/asm-arm/mmio.h b/xen/include/asm-arm/mmio.h
index 1cd7a7a..d9b5da4 100644
--- a/xen/include/asm-arm/mmio.h
+++ b/xen/include/asm-arm/mmio.h
@@ -34,7 +34,6 @@ typedef struct
typedef int (*mmio_read_t)(struct vcpu *v, mmio_info_t *info, void *priv);
typedef int (*mmio_write_t)(struct vcpu *v, mmio_info_t *info, void *priv);
-typedef int (*mmio_check_t)(struct vcpu *v, paddr_t addr);
struct mmio_handler_ops {
mmio_read_t read;
--
2.1.4
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 2/9] xen/arm: io: Extend write/read handler to pass the register in parameter
2015-10-07 14:41 [PATCH v3 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
2015-10-07 14:41 ` [PATCH v3 1/9] xen/arm: io: remove mmio_check_t typedef Julien Grall
@ 2015-10-07 14:41 ` Julien Grall
2015-10-07 14:41 ` [PATCH v3 3/9] xen/arm: io: Support sign-extension for every read access Julien Grall
` (7 subsequent siblings)
9 siblings, 0 replies; 34+ messages in thread
From: Julien Grall @ 2015-10-07 14:41 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, ian.campbell, stefano.stabellini
Rather than letting each handler to retrieve the register used by the
I/O access, add a new parameter to pass the register in parameter.
This will help to implement generic register manipulation on I/O access
such as sign-extension and endianess.
Read handlers need to modify the value of the register, so a pointer to
it is given in argument. Write handlers shouldn't modify the register,
therefore only a plain value is given.
Signed-off-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v2:
- Rebase on top of "xen/arm: io: Extend write/read handler to
pass private data"
- Add Ian's acked-by
Changes in v1:
- Patch added
[1] http://lists.xen.org/archives/html/xen-devel/2015-09/msg03753.html
---
xen/arch/arm/io.c | 7 +++-
xen/arch/arm/vgic-v2.c | 44 +++++++++-----------
xen/arch/arm/vgic-v3.c | 101 ++++++++++++++++++++-------------------------
xen/arch/arm/vuart.c | 20 ++++-----
xen/include/asm-arm/mmio.h | 6 ++-
5 files changed, 83 insertions(+), 95 deletions(-)
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index b418173..ffe7c21 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -29,6 +29,9 @@ int handle_mmio(mmio_info_t *info)
int i;
const struct mmio_handler *handler = NULL;
const struct vmmio *vmmio = &v->domain->arch.vmmio;
+ struct hsr_dabt dabt = info->dabt;
+ struct cpu_user_regs *regs = guest_cpu_user_regs();
+ register_t *r = select_user_reg(regs, dabt.reg);
for ( i = 0; i < vmmio->num_entries; i++ )
{
@@ -43,9 +46,9 @@ int handle_mmio(mmio_info_t *info)
return 0;
if ( info->dabt.write )
- return handler->ops->write(v, info, handler->priv);
+ return handler->ops->write(v, info, *r, handler->priv);
else
- return handler->ops->read(v, info, handler->priv);
+ return handler->ops->read(v, info, r, handler->priv);
}
void register_mmio_handler(struct domain *d,
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 309d579..dc2c2d2 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -51,11 +51,9 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t vbase)
}
static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
- void *priv)
+ register_t *r, void *priv)
{
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;
int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
unsigned long flags;
@@ -249,11 +247,9 @@ static int vgic_v2_to_sgi(struct vcpu *v, register_t sgir)
}
static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
- void *priv)
+ register_t r, void *priv)
{
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;
int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
uint32_t tr;
@@ -267,7 +263,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
if ( dabt.size != DABT_WORD ) goto bad_width;
/* Ignore all but the enable bit */
vgic_lock(v);
- v->domain->arch.vgic.ctlr = (*r) & GICD_CTL_ENABLE;
+ v->domain->arch.vgic.ctlr = r & GICD_CTL_ENABLE;
vgic_unlock(v);
return 1;
@@ -291,11 +287,11 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
if ( rank == NULL) goto write_ignore;
vgic_lock_rank(v, rank, flags);
tr = rank->ienable;
- rank->ienable |= *r;
+ rank->ienable |= r;
/* The virtual irq is derived from register offset.
* The register difference is word difference. So divide by 2(DABT_WORD)
* to get Virtual irq number */
- vgic_enable_irqs(v, (*r) & (~tr),
+ vgic_enable_irqs(v, r & (~tr),
(gicd_reg - GICD_ISENABLER) >> DABT_WORD);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -306,11 +302,11 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
if ( rank == NULL) goto write_ignore;
vgic_lock_rank(v, rank, flags);
tr = rank->ienable;
- rank->ienable &= ~*r;
+ rank->ienable &= ~r;
/* The virtual irq is derived from register offset.
* The register difference is word difference. So divide by 2(DABT_WORD)
* to get Virtual irq number */
- vgic_disable_irqs(v, (*r) & tr,
+ vgic_disable_irqs(v, r & tr,
(gicd_reg - GICD_ICENABLER) >> DABT_WORD);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -319,28 +315,28 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
if ( dabt.size != DABT_WORD ) goto bad_width;
printk(XENLOG_G_ERR
"%pv: vGICD: unhandled word write %#"PRIregister" to ISPENDR%d\n",
- v, *r, gicd_reg - GICD_ISPENDR);
+ v, r, gicd_reg - GICD_ISPENDR);
return 0;
case GICD_ICPENDR ... GICD_ICPENDRN:
if ( dabt.size != DABT_WORD ) goto bad_width;
printk(XENLOG_G_ERR
"%pv: vGICD: unhandled word write %#"PRIregister" to ICPENDR%d\n",
- v, *r, gicd_reg - GICD_ICPENDR);
+ v, r, gicd_reg - GICD_ICPENDR);
return 0;
case GICD_ISACTIVER ... GICD_ISACTIVERN:
if ( dabt.size != DABT_WORD ) goto bad_width;
printk(XENLOG_G_ERR
"%pv: vGICD: unhandled word write %#"PRIregister" to ISACTIVER%d\n",
- v, *r, gicd_reg - GICD_ISACTIVER);
+ v, r, gicd_reg - GICD_ISACTIVER);
return 0;
case GICD_ICACTIVER ... GICD_ICACTIVERN:
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);
+ v, r, gicd_reg - GICD_ICACTIVER);
return 0;
case GICD_ITARGETSR ... GICD_ITARGETSR + 7:
@@ -362,7 +358,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
target = target | (target << 8) | (target << 16) | (target << 24);
else
target = (target << (8 * (gicd_reg & 0x3)));
- target &= *r;
+ target &= r;
/* ignore zero writes */
if ( !target )
goto write_ignore;
@@ -410,10 +406,10 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
vgic_lock_rank(v, rank, flags);
if ( dabt.size == DABT_WORD )
rank->ipriority[REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR,
- DABT_WORD)] = *r;
+ DABT_WORD)] = r;
else
vgic_byte_write(&rank->ipriority[REG_RANK_INDEX(8,
- gicd_reg - GICD_IPRIORITYR, DABT_WORD)], *r, gicd_reg);
+ gicd_reg - GICD_IPRIORITYR, DABT_WORD)], r, gicd_reg);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -427,7 +423,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
rank = vgic_rank_offset(v, 2, gicd_reg - GICD_ICFGR, DABT_WORD);
if ( rank == NULL) goto write_ignore;
vgic_lock_rank(v, rank, flags);
- rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR, DABT_WORD)] = *r;
+ rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR, DABT_WORD)] = r;
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -437,20 +433,20 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
case GICD_SGIR:
if ( dabt.size != DABT_WORD ) goto bad_width;
- return vgic_v2_to_sgi(v, *r);
+ return vgic_v2_to_sgi(v, r);
case GICD_CPENDSGIR ... GICD_CPENDSGIRN:
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
printk(XENLOG_G_ERR
"%pv: vGICD: unhandled %s write %#"PRIregister" to ICPENDSGIR%d\n",
- v, dabt.size ? "word" : "byte", *r, gicd_reg - GICD_CPENDSGIR);
+ v, dabt.size ? "word" : "byte", r, gicd_reg - GICD_CPENDSGIR);
return 0;
case GICD_SPENDSGIR ... GICD_SPENDSGIRN:
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
printk(XENLOG_G_ERR
"%pv: vGICD: unhandled %s write %#"PRIregister" to ISPENDSGIR%d\n",
- v, dabt.size ? "word" : "byte", *r, gicd_reg - GICD_SPENDSGIR);
+ v, dabt.size ? "word" : "byte", r, gicd_reg - GICD_SPENDSGIR);
return 0;
/* Implementation defined -- write ignored */
@@ -477,14 +473,14 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
default:
printk(XENLOG_G_ERR
"%pv: vGICD: unhandled write r%d=%"PRIregister" offset %#08x\n",
- v, dabt.reg, *r, gicd_reg);
+ v, dabt.reg, r, gicd_reg);
return 0;
}
bad_width:
printk(XENLOG_G_ERR
"%pv: vGICD: bad write width %d r%d=%"PRIregister" offset %#08x\n",
- v, dabt.size, dabt.reg, *r, gicd_reg);
+ v, dabt.size, dabt.reg, r, gicd_reg);
domain_crash_synchronous();
return 0;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index beb3621..2a09f86 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -104,11 +104,10 @@ static struct vcpu *vgic_v3_get_target_vcpu(struct vcpu *v, unsigned int irq)
}
static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
- uint32_t gicr_reg)
+ uint32_t gicr_reg,
+ register_t *r)
{
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 )
@@ -215,11 +214,10 @@ read_as_zero_32:
}
static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
- uint32_t gicr_reg)
+ uint32_t gicr_reg,
+ register_t r)
{
struct hsr_dabt dabt = info->dabt;
- struct cpu_user_regs *regs = guest_cpu_user_regs();
- register_t *r = select_user_reg(regs, dabt.reg);
switch ( gicr_reg )
{
@@ -276,7 +274,7 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
bad_width:
printk(XENLOG_G_ERR
"%pv: vGICR: bad write width %d r%d=%"PRIregister" offset %#08x\n",
- v, dabt.size, dabt.reg, *r, gicr_reg);
+ v, dabt.size, dabt.reg, r, gicr_reg);
domain_crash_synchronous();
return 0;
@@ -290,11 +288,10 @@ write_ignore_32:
}
static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
- mmio_info_t *info, uint32_t reg)
+ mmio_info_t *info, uint32_t reg,
+ register_t *r)
{
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;
@@ -369,11 +366,10 @@ read_as_zero:
}
static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
- mmio_info_t *info, uint32_t reg)
+ mmio_info_t *info, uint32_t reg,
+ register_t r)
{
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;
uint32_t tr;
unsigned long flags;
@@ -389,9 +385,9 @@ 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;
+ rank->ienable |= r;
/* The irq number is extracted from offset. so shift by register size */
- vgic_enable_irqs(v, (*r) & (~tr), (reg - GICD_ISENABLER) >> DABT_WORD);
+ vgic_enable_irqs(v, r & (~tr), (reg - GICD_ISENABLER) >> DABT_WORD);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICENABLER ... GICD_ICENABLERN:
@@ -400,37 +396,37 @@ 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;
+ 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);
+ vgic_disable_irqs(v, r & tr, (reg - GICD_ICENABLER) >> DABT_WORD);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ISPENDR ... GICD_ISPENDRN:
if ( dabt.size != DABT_WORD ) goto bad_width;
printk(XENLOG_G_ERR
"%pv: %s: unhandled word write %#"PRIregister" to ISPENDR%d\n",
- v, name, *r, reg - GICD_ISPENDR);
+ v, name, r, reg - GICD_ISPENDR);
return 0;
case GICD_ICPENDR ... GICD_ICPENDRN:
if ( dabt.size != DABT_WORD ) goto bad_width;
printk(XENLOG_G_ERR
"%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d\n",
- v, name, *r, reg - GICD_ICPENDR);
+ v, name, r, reg - GICD_ICPENDR);
return 0;
case GICD_ISACTIVER ... GICD_ISACTIVERN:
if ( dabt.size != DABT_WORD ) goto bad_width;
printk(XENLOG_G_ERR
"%pv: %s: unhandled word write %#"PRIregister" to ISACTIVER%d\n",
- v, name, *r, reg - GICD_ISACTIVER);
+ v, name, r, reg - GICD_ISACTIVER);
return 0;
case GICD_ICACTIVER ... GICD_ICACTIVERN:
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);
+ v, name, r, reg - GICD_ICACTIVER);
return 0;
case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
@@ -440,10 +436,10 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
vgic_lock_rank(v, rank, flags);
if ( dabt.size == DABT_WORD )
rank->ipriority[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
- DABT_WORD)] = *r;
+ DABT_WORD)] = r;
else
vgic_byte_write(&rank->ipriority[REG_RANK_INDEX(8,
- reg - GICD_IPRIORITYR, DABT_WORD)], *r, reg);
+ reg - GICD_IPRIORITYR, DABT_WORD)], r, reg);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICFGR: /* Restricted to configure SGIs */
@@ -455,20 +451,20 @@ 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;
+ rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR, DABT_WORD)] = r;
vgic_unlock_rank(v, rank, flags);
return 1;
default:
printk(XENLOG_G_ERR
"%pv: %s: unhandled write r%d=%"PRIregister" offset %#08x\n",
- v, name, dabt.reg, *r, reg);
+ v, name, dabt.reg, r, reg);
return 0;
}
bad_width:
printk(XENLOG_G_ERR
"%pv: %s: bad write width %d r%d=%"PRIregister" offset %#08x\n",
- v, name, dabt.size, dabt.reg, *r, reg);
+ v, name, dabt.size, dabt.reg, r, reg);
domain_crash_synchronous();
return 0;
@@ -479,11 +475,9 @@ write_ignore:
}
static int vgic_v3_rdistr_sgi_mmio_read(struct vcpu *v, mmio_info_t *info,
- uint32_t gicr_reg)
+ uint32_t gicr_reg, register_t *r)
{
struct hsr_dabt dabt = info->dabt;
- struct cpu_user_regs *regs = guest_cpu_user_regs();
- register_t *r = select_user_reg(regs, dabt.reg);
switch ( gicr_reg )
{
@@ -502,7 +496,7 @@ static int vgic_v3_rdistr_sgi_mmio_read(struct vcpu *v, mmio_info_t *info,
* So handle in common with GICD handling
*/
return __vgic_v3_distr_common_mmio_read("vGICR: SGI", v, info,
- gicr_reg);
+ gicr_reg, r);
/* Read the pending status of an SGI is via GICR is not supported */
case GICR_ISPENDR0:
@@ -533,11 +527,9 @@ read_as_zero:
}
static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info,
- uint32_t gicr_reg)
+ uint32_t gicr_reg, register_t r)
{
struct hsr_dabt dabt = info->dabt;
- struct cpu_user_regs *regs = guest_cpu_user_regs();
- register_t *r = select_user_reg(regs, dabt.reg);
switch ( gicr_reg )
{
@@ -556,19 +548,19 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info,
* So handle common with GICD handling
*/
return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
- info, gicr_reg);
+ info, gicr_reg, r);
case GICR_ISPENDR0:
if ( dabt.size != DABT_WORD ) goto bad_width;
printk(XENLOG_G_ERR
"%pv: vGICR: SGI: unhandled word write %#"PRIregister" to ISPENDR0\n",
- v, *r);
+ v, r);
return 0;
case GICR_ICPENDR0:
if ( dabt.size != DABT_WORD ) goto bad_width;
printk(XENLOG_G_ERR
"%pv: vGICR: SGI: unhandled word write %#"PRIregister" to ICPENDR0\n",
- v, *r);
+ v, r);
return 0;
case GICR_NSACR:
@@ -584,7 +576,7 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info,
bad_width:
printk(XENLOG_G_ERR
"%pv: vGICR: SGI: bad write width %d r%d=%"PRIregister" offset %#08x\n",
- v, dabt.size, dabt.reg, *r, gicr_reg);
+ v, dabt.size, dabt.reg, r, gicr_reg);
domain_crash_synchronous();
return 0;
@@ -613,7 +605,7 @@ static struct vcpu *get_vcpu_from_rdist(struct domain *d,
}
static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info,
- void *priv)
+ register_t *r, void *priv)
{
uint32_t offset;
const struct vgic_rdist_region *region = priv;
@@ -625,9 +617,9 @@ static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info,
return 0;
if ( offset < SZ_64K )
- return __vgic_v3_rdistr_rd_mmio_read(v, info, offset);
+ return __vgic_v3_rdistr_rd_mmio_read(v, info, offset, r);
else if ( (offset >= SZ_64K) && (offset < 2 * SZ_64K) )
- return vgic_v3_rdistr_sgi_mmio_read(v, info, (offset - SZ_64K));
+ return vgic_v3_rdistr_sgi_mmio_read(v, info, (offset - SZ_64K), r);
else
printk(XENLOG_G_WARNING
"%pv: vGICR: unknown gpa read address %"PRIpaddr"\n",
@@ -637,7 +629,7 @@ static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info,
}
static int vgic_v3_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info,
- void *priv)
+ register_t r, void *priv)
{
uint32_t offset;
const struct vgic_rdist_region *region = priv;
@@ -649,9 +641,9 @@ static int vgic_v3_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info,
return 0;
if ( offset < SZ_64K )
- return __vgic_v3_rdistr_rd_mmio_write(v, info, offset);
+ return __vgic_v3_rdistr_rd_mmio_write(v, info, offset, r);
else if ( (offset >= SZ_64K) && (offset < 2 * SZ_64K) )
- return vgic_v3_rdistr_sgi_mmio_write(v, info, (offset - SZ_64K));
+ return vgic_v3_rdistr_sgi_mmio_write(v, info, (offset - SZ_64K), r);
else
printk(XENLOG_G_WARNING
"%pv: vGICR: unknown gpa write address %"PRIpaddr"\n",
@@ -661,11 +653,9 @@ static int vgic_v3_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info,
}
static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
- void *priv)
+ register_t *r, void *priv)
{
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;
int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
@@ -728,7 +718,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
* Above all register are common with GICR and GICD
* Manage in common
*/
- return __vgic_v3_distr_common_mmio_read("vGICD", v, info, gicd_reg);
+ return __vgic_v3_distr_common_mmio_read("vGICD", v, info, gicd_reg, r);
case GICD_IROUTER ... GICD_IROUTER31:
/* SGI/PPI is RES0 */
goto read_as_zero_64;
@@ -819,11 +809,9 @@ read_as_zero:
}
static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
- void *priv)
+ register_t r, void *priv)
{
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;
uint64_t new_irouter, old_irouter;
@@ -839,7 +827,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
vgic_lock(v);
/* Only EnableGrp1A can be changed */
- if ( *r & GICD_CTLR_ENABLE_G1A )
+ if ( r & GICD_CTLR_ENABLE_G1A )
v->domain->arch.vgic.ctlr |= GICD_CTLR_ENABLE_G1A;
else
v->domain->arch.vgic.ctlr &= ~GICD_CTLR_ENABLE_G1A;
@@ -885,7 +873,8 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
case GICD_ICFGR ... GICD_ICFGRN:
/* Above registers are common with GICR and GICD
* Manage in common */
- return __vgic_v3_distr_common_mmio_write("vGICD", v, info, gicd_reg);
+ return __vgic_v3_distr_common_mmio_write("vGICD", v, info,
+ gicd_reg, r);
case GICD_IROUTER ... GICD_IROUTER31:
/* SGI/PPI is RES0 */
goto write_ignore_64;
@@ -894,7 +883,7 @@ 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;
+ new_irouter = r;
vgic_lock_rank(v, rank, flags);
old_irouter = rank->v3.irouter[REG_RANK_INDEX(64,
@@ -907,7 +896,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
{
printk(XENLOG_G_DEBUG
"%pv: vGICD: wrong irouter at offset %#08x val %#"PRIregister,
- v, gicd_reg, *r);
+ v, gicd_reg, r);
vgic_unlock_rank(v, rank, flags);
/*
* TODO: Don't inject a fault to the guest when the MPIDR is
@@ -953,14 +942,14 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
default:
printk(XENLOG_G_ERR
"%pv: vGICD: unhandled write r%d=%"PRIregister" offset %#08x\n",
- v, dabt.reg, *r, gicd_reg);
+ v, dabt.reg, r, gicd_reg);
return 0;
}
bad_width:
printk(XENLOG_G_ERR
"%pv: vGICD: bad write width %d r%d=%"PRIregister" offset %#08x\n",
- v, dabt.size, dabt.reg, *r, gicd_reg);
+ v, dabt.size, dabt.reg, r, gicd_reg);
domain_crash_synchronous();
return 0;
diff --git a/xen/arch/arm/vuart.c b/xen/arch/arm/vuart.c
index 2495e87..b5c9288 100644
--- a/xen/arch/arm/vuart.c
+++ b/xen/arch/arm/vuart.c
@@ -45,8 +45,10 @@
#define domain_has_vuart(d) ((d)->arch.vuart.info != NULL)
-static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info, void *priv);
-static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info, void *priv);
+static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info,
+ register_t *r, void *priv);
+static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info,
+ register_t r, void *priv);
static const struct mmio_handler_ops vuart_mmio_handler = {
.read = vuart_mmio_read,
@@ -106,12 +108,10 @@ static void vuart_print_char(struct vcpu *v, char c)
spin_unlock(&uart->lock);
}
-static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info, void *priv)
+static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info,
+ register_t *r, void *priv)
{
struct domain *d = v->domain;
- struct hsr_dabt dabt = info->dabt;
- struct cpu_user_regs *regs = guest_cpu_user_regs();
- register_t *r = select_user_reg(regs, dabt.reg);
paddr_t offset = info->gpa - d->arch.vuart.info->base_addr;
perfc_incr(vuart_reads);
@@ -126,19 +126,17 @@ static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info, void *priv)
return 1;
}
-static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info, void *priv)
+static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info,
+ register_t r, void *priv)
{
struct domain *d = v->domain;
- struct hsr_dabt dabt = info->dabt;
- struct cpu_user_regs *regs = guest_cpu_user_regs();
- register_t *r = select_user_reg(regs, dabt.reg);
paddr_t offset = info->gpa - d->arch.vuart.info->base_addr;
perfc_incr(vuart_writes);
if ( offset == d->arch.vuart.info->data_off )
/* ignore any status bits */
- vuart_print_char(v, *r & 0xFF);
+ vuart_print_char(v, r & 0xFF);
return 1;
}
diff --git a/xen/include/asm-arm/mmio.h b/xen/include/asm-arm/mmio.h
index d9b5da4..da1cc2e 100644
--- a/xen/include/asm-arm/mmio.h
+++ b/xen/include/asm-arm/mmio.h
@@ -32,8 +32,10 @@ typedef struct
paddr_t gpa;
} mmio_info_t;
-typedef int (*mmio_read_t)(struct vcpu *v, mmio_info_t *info, void *priv);
-typedef int (*mmio_write_t)(struct vcpu *v, mmio_info_t *info, void *priv);
+typedef int (*mmio_read_t)(struct vcpu *v, mmio_info_t *info,
+ register_t *r, void *priv);
+typedef int (*mmio_write_t)(struct vcpu *v, mmio_info_t *info,
+ register_t r, void *priv);
struct mmio_handler_ops {
mmio_read_t read;
--
2.1.4
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 3/9] xen/arm: io: Support sign-extension for every read access
2015-10-07 14:41 [PATCH v3 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
2015-10-07 14:41 ` [PATCH v3 1/9] xen/arm: io: remove mmio_check_t typedef Julien Grall
2015-10-07 14:41 ` [PATCH v3 2/9] xen/arm: io: Extend write/read handler to pass the register in parameter Julien Grall
@ 2015-10-07 14:41 ` Julien Grall
2015-10-07 14:41 ` [PATCH v3 4/9] xen/arm: vgic: ctlr stores a 32-bit hardware register so use uint32_t Julien Grall
` (6 subsequent siblings)
9 siblings, 0 replies; 34+ messages in thread
From: Julien Grall @ 2015-10-07 14:41 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, ian.campbell, stefano.stabellini
The guest may try to load data from the emulated MMIO region using
instructions with Sign-Extension (i.e ldrs*). Any use of one those,
will set the SSE bit (Syndrome Sign Extend) in the ISS (see B3-1433
in DDI 0406C.b).
Note that the bit can only be set for access size smaller than the
register size (i.e byte/half-word for aarch32, byte/half-word/word for
aarch32). So we don't have to worry about undefined C behavior.
Until now, the support of sign-extension was limited for byte access in
vGIC emulation. Although there is no reason to not have it generically.
So move the support just after we get the data from the MMIO emulation.
Signed-off-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v3:
- Update a comment I forgot to address it in v2
- Add Ian's acked-by
Changes in v2:
- Rebase on top of "xen/arm: io: Extend write/read handler to
pass private data" [1]
- Fix typeos
- Expand the commit message to explain the access size supported
- Add "io: " in the commit title
Changes in v1:
- Patch added
[1] http://lists.xen.org/archives/html/xen-devel/2015-09/msg03753.html
---
xen/arch/arm/io.c | 29 ++++++++++++++++++++++++++++-
xen/arch/arm/vgic-v2.c | 10 +++++-----
xen/arch/arm/vgic-v3.c | 4 ++--
xen/include/asm-arm/vgic.h | 8 +++-----
4 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index ffe7c21..de5765a 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -23,6 +23,33 @@
#include <asm/current.h>
#include <asm/mmio.h>
+static int handle_read(const struct mmio_handler *handler, struct vcpu *v,
+ mmio_info_t *info, register_t *r)
+{
+ uint8_t size = (1 << info->dabt.size) * 8;
+
+ if ( !handler->ops->read(v, info, r, handler->priv) )
+ return 0;
+
+ /*
+ * Sign extend if required.
+ * Note that we expect the read handler to have zeroed the bits
+ * outside the requested access size.
+ */
+ if ( info->dabt.sign && (*r & (1UL << (size - 1)) ))
+ {
+ /*
+ * We are relying on register_t using the same as
+ * an unsigned long in order to keep the 32-bit assembly
+ * code smaller.
+ */
+ BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long));
+ *r |= (~0UL) << size;
+ }
+
+ return 1;
+}
+
int handle_mmio(mmio_info_t *info)
{
struct vcpu *v = current;
@@ -48,7 +75,7 @@ int handle_mmio(mmio_info_t *info)
if ( info->dabt.write )
return handler->ops->write(v, info, *r, handler->priv);
else
- return handler->ops->read(v, info, r, handler->priv);
+ return handle_read(handler, v, info, r);
}
void register_mmio_handler(struct domain *d,
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index dc2c2d2..dcbba0f 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -129,7 +129,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
*r = rank->v2.itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR,
DABT_WORD)];
if ( dabt.size == DABT_BYTE )
- *r = vgic_byte_read(*r, dabt.sign, gicd_reg);
+ *r = vgic_byte_read(*r, gicd_reg);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -142,7 +142,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
*r = rank->ipriority[REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR,
DABT_WORD)];
if ( dabt.size == DABT_BYTE )
- *r = vgic_byte_read(*r, dabt.sign, gicd_reg);
+ *r = vgic_byte_read(*r, gicd_reg);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -377,7 +377,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
new_target = i % 8;
old_target_mask = vgic_byte_read(rank->v2.itargets[REG_RANK_INDEX(8,
- gicd_reg - GICD_ITARGETSR, DABT_WORD)], 0, i/8);
+ gicd_reg - GICD_ITARGETSR, DABT_WORD)], i/8);
old_target = find_first_bit(&old_target_mask, 8);
if ( new_target != old_target )
@@ -503,7 +503,7 @@ static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
ASSERT(spin_is_locked(&rank->lock));
target = vgic_byte_read(rank->v2.itargets[REG_RANK_INDEX(8,
- irq, DABT_WORD)], 0, irq & 0x3);
+ irq, DABT_WORD)], irq & 0x3);
/* 1-N SPI should be delivered as pending to all the vcpus in the
* mask, but here we just return the first vcpu for simplicity and
@@ -521,7 +521,7 @@ static int vgic_v2_get_irq_priority(struct vcpu *v, unsigned int irq)
ASSERT(spin_is_locked(&rank->lock));
priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8,
- irq, DABT_WORD)], 0, irq & 0x3);
+ irq, DABT_WORD)], irq & 0x3);
return priority;
}
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 2a09f86..6de7f00 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -336,7 +336,7 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
*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);
+ *r = vgic_byte_read(*r, reg);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICFGR ... GICD_ICFGRN:
@@ -1042,7 +1042,7 @@ static int vgic_v3_get_irq_priority(struct vcpu *v, unsigned int irq)
ASSERT(spin_is_locked(&rank->lock));
priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8,
- irq, DABT_WORD)], 0, irq & 0x3);
+ irq, DABT_WORD)], irq & 0x3);
return priority;
}
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 96839f0..354c0d4 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -158,15 +158,13 @@ static inline int REG_RANK_NR(int b, uint32_t n)
}
}
-static inline uint32_t vgic_byte_read(uint32_t val, int sign, int offset)
+static inline uint32_t vgic_byte_read(uint32_t val, int offset)
{
int byte = offset & 0x3;
val = val >> (8*byte);
- if ( sign && (val & 0x80) )
- val |= 0xffffff00;
- else
- val &= 0x000000ff;
+ val &= 0x000000ff;
+
return val;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 4/9] xen/arm: vgic: ctlr stores a 32-bit hardware register so use uint32_t
2015-10-07 14:41 [PATCH v3 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (2 preceding siblings ...)
2015-10-07 14:41 ` [PATCH v3 3/9] xen/arm: io: Support sign-extension for every read access Julien Grall
@ 2015-10-07 14:41 ` Julien Grall
2015-10-07 14:41 ` [PATCH v3 5/9] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank Julien Grall
` (5 subsequent siblings)
9 siblings, 0 replies; 34+ messages in thread
From: Julien Grall @ 2015-10-07 14:41 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, ian.campbell, stefano.stabellini
Signed-off-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v2:
- Add Ian's acked-by
Changes in v1:
- Patch added
---
xen/include/asm-arm/domain.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index b89727e..e7e40da 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -91,7 +91,7 @@ struct arch_domain
* rank order.
*/
spinlock_t lock;
- int ctlr;
+ uint32_t ctlr;
int nr_spis; /* Number of SPIs */
unsigned long *allocated_irqs; /* bitmap of IRQs allocated */
struct vgic_irq_rank *shared_irqs;
--
2.1.4
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 5/9] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank
2015-10-07 14:41 [PATCH v3 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (3 preceding siblings ...)
2015-10-07 14:41 ` [PATCH v3 4/9] xen/arm: vgic: ctlr stores a 32-bit hardware register so use uint32_t Julien Grall
@ 2015-10-07 14:41 ` Julien Grall
2015-10-07 14:41 ` [PATCH v3 6/9] xen/arm: vgic: Introduce a new field to store the rank index and use it Julien Grall
` (4 subsequent siblings)
9 siblings, 0 replies; 34+ messages in thread
From: Julien Grall @ 2015-10-07 14:41 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, ian.campbell, stefano.stabellini
Xen is currently directly storing the value of GICD_IPRIORITYR register
in the rank. This makes emulation of the register access very simple
but makes the code to get the priority for a given vIRQ more complex.
While the priority of an vIRQ is retrieved every time an vIRQ is injected
to the guest, the access to register occurs less often.
Each GICD_IPRIORITYR register stores 4 priorities associated for 4 vIRQs
(see 4.3.11 in IHI 0048B). As Xen is using little endian, we can use
an union to access directly a register or a priority for a given IRQ.
Note that the field "ipriority" has been renamed to "ipriorityr" to
match the name of the register in the GIC spec.
Finally, the implementation of the callback get_irq_priority is exactly
the same for both vGIC drivers. Consolidate the implementation in the
common vGIC code and drop the callback.
Signed-off-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
The real reason is I'd like to drop vgic_byte_* helpers in favors of more
generic access helper. Although it would make the code to retrieve the
priority more complex. So reworking the way to get the priority was the
best solution.
Changes in v3:
- Add Ian's acked-by
Changes in v2:
- Use an union to get the best of the emulation and getting the
priority quickly
- Remove the callback get_irq_priority
- Update commit message
Changes in v1:
- Patch added
---
xen/arch/arm/vgic-v2.c | 23 +++++------------------
xen/arch/arm/vgic-v3.c | 23 +++++------------------
xen/arch/arm/vgic.c | 18 ++++++++++++++----
xen/include/asm-arm/vgic.h | 17 ++++++++++++++---
4 files changed, 38 insertions(+), 43 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index dcbba0f..ad1bb15 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -139,8 +139,8 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
if ( rank == NULL) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
- *r = rank->ipriority[REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR,
- DABT_WORD)];
+ *r = rank->ipriorityr[REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR,
+ DABT_WORD)];
if ( dabt.size == DABT_BYTE )
*r = vgic_byte_read(*r, gicd_reg);
vgic_unlock_rank(v, rank, flags);
@@ -405,10 +405,10 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
if ( rank == NULL) goto write_ignore;
vgic_lock_rank(v, rank, flags);
if ( dabt.size == DABT_WORD )
- rank->ipriority[REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR,
- DABT_WORD)] = r;
+ rank->ipriorityr[REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR,
+ DABT_WORD)] = r;
else
- vgic_byte_write(&rank->ipriority[REG_RANK_INDEX(8,
+ vgic_byte_write(&rank->ipriorityr[REG_RANK_INDEX(8,
gicd_reg - GICD_IPRIORITYR, DABT_WORD)], r, gicd_reg);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -514,18 +514,6 @@ static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
return v_target;
}
-static int vgic_v2_get_irq_priority(struct vcpu *v, unsigned int irq)
-{
- int priority;
- struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
-
- ASSERT(spin_is_locked(&rank->lock));
- priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8,
- irq, DABT_WORD)], irq & 0x3);
-
- return priority;
-}
-
static int vgic_v2_vcpu_init(struct vcpu *v)
{
int i;
@@ -597,7 +585,6 @@ static int vgic_v2_domain_init(struct domain *d)
static const struct vgic_ops vgic_v2_ops = {
.vcpu_init = vgic_v2_vcpu_init,
.domain_init = vgic_v2_domain_init,
- .get_irq_priority = vgic_v2_get_irq_priority,
.get_target_vcpu = vgic_v2_get_target_vcpu,
.max_vcpus = 8,
};
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 6de7f00..92a3ccf 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -333,8 +333,8 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
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)];
+ *r = rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
+ DABT_WORD)];
if ( dabt.size == DABT_BYTE )
*r = vgic_byte_read(*r, reg);
vgic_unlock_rank(v, rank, flags);
@@ -435,10 +435,10 @@ 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);
if ( dabt.size == DABT_WORD )
- rank->ipriority[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
- DABT_WORD)] = r;
+ rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
+ DABT_WORD)] = r;
else
- vgic_byte_write(&rank->ipriority[REG_RANK_INDEX(8,
+ vgic_byte_write(&rank->ipriorityr[REG_RANK_INDEX(8,
reg - GICD_IPRIORITYR, DABT_WORD)], r, reg);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -1035,18 +1035,6 @@ static const struct mmio_handler_ops vgic_distr_mmio_handler = {
.write = vgic_v3_distr_mmio_write,
};
-static int vgic_v3_get_irq_priority(struct vcpu *v, unsigned int irq)
-{
- int priority;
- struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
-
- ASSERT(spin_is_locked(&rank->lock));
- priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8,
- irq, DABT_WORD)], irq & 0x3);
-
- return priority;
-}
-
static int vgic_v3_vcpu_init(struct vcpu *v)
{
int i;
@@ -1196,7 +1184,6 @@ static int vgic_v3_domain_init(struct domain *d)
static const struct vgic_ops v3_ops = {
.vcpu_init = vgic_v3_vcpu_init,
.domain_init = vgic_v3_domain_init,
- .get_irq_priority = vgic_v3_get_irq_priority,
.get_target_vcpu = vgic_v3_get_target_vcpu,
.emulate_sysreg = vgic_v3_emulate_sysreg,
/*
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index a6835a8..2128d29 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -204,6 +204,19 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
return v_target;
}
+static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq)
+{
+ struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
+ unsigned long flags;
+ int priority;
+
+ vgic_lock_rank(v, rank, flags);
+ priority = rank->priority[virq & INTERRUPT_RANK_MASK];
+ vgic_unlock_rank(v, rank, flags);
+
+ return priority;
+}
+
void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
{
unsigned long flags;
@@ -407,14 +420,11 @@ void vgic_clear_pending_irqs(struct vcpu *v)
void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
{
uint8_t priority;
- struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
struct pending_irq *iter, *n = irq_to_pending(v, virq);
unsigned long flags;
bool_t running;
- vgic_lock_rank(v, rank, flags);
- priority = v->domain->arch.vgic.handler->get_irq_priority(v, virq);
- vgic_unlock_rank(v, rank, flags);
+ priority = vgic_get_virq_priority(v, virq);
spin_lock_irqsave(&v->arch.vgic.lock, flags);
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 354c0d4..ff98913 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -82,12 +82,25 @@ struct pending_irq
struct list_head lr_queue;
};
+#define NR_INTERRUPT_PER_RANK 32
+#define INTERRUPT_RANK_MASK (NR_INTERRUPT_PER_RANK - 1)
+
/* 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;
uint32_t icfg[2];
- uint32_t ipriority[8];
+
+ /*
+ * Provide efficient access to the priority of an vIRQ while keeping
+ * the emulation simple.
+ * Note, this is working fine as long as Xen is using little endian.
+ */
+ union {
+ uint8_t priority[32];
+ uint32_t ipriorityr[8];
+ };
+
union {
struct {
uint32_t itargets[8];
@@ -114,8 +127,6 @@ struct vgic_ops {
int (*vcpu_init)(struct vcpu *v);
/* Domain specific initialization of vGIC */
int (*domain_init)(struct domain *d);
- /* Get priority for a given irq stored in vgic structure */
- int (*get_irq_priority)(struct vcpu *v, unsigned int irq);
/* Get the target vcpu for a given virq. The rank lock is already taken
* when calling this. */
struct vcpu *(*get_target_vcpu)(struct vcpu *v, unsigned int irq);
--
2.1.4
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 6/9] xen/arm: vgic: Introduce a new field to store the rank index and use it
2015-10-07 14:41 [PATCH v3 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (4 preceding siblings ...)
2015-10-07 14:41 ` [PATCH v3 5/9] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank Julien Grall
@ 2015-10-07 14:41 ` Julien Grall
2015-10-07 14:41 ` [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank Julien Grall
` (3 subsequent siblings)
9 siblings, 0 replies; 34+ messages in thread
From: Julien Grall @ 2015-10-07 14:41 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, ian.campbell, stefano.stabellini
Having in hand the index for the rank is very handy to avoid computing
it every time.
For now, use it when enabling/disabling the vIRQs rather than a formula
which is not obvious to understand.
Also drop the comments which were wrong because a shift by DABT_WORD
will not give the IRQ number but the index of the register.
Signed-off-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v3:
- Add missing signed-off-by
- Typoes in the commit message
- Add Ian's acked-by
Changes in v2:
- Patch added
---
xen/arch/arm/vgic-v2.c | 12 ++----------
xen/arch/arm/vgic-v3.c | 6 ++----
xen/arch/arm/vgic.c | 13 ++++++++++---
xen/include/asm-arm/vgic.h | 3 +++
4 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index ad1bb15..2d63e12 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -288,11 +288,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
vgic_lock_rank(v, rank, flags);
tr = rank->ienable;
rank->ienable |= r;
- /* The virtual irq is derived from register offset.
- * The register difference is word difference. So divide by 2(DABT_WORD)
- * to get Virtual irq number */
- vgic_enable_irqs(v, r & (~tr),
- (gicd_reg - GICD_ISENABLER) >> DABT_WORD);
+ vgic_enable_irqs(v, r & (~tr), rank->index);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -303,11 +299,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
vgic_lock_rank(v, rank, flags);
tr = rank->ienable;
rank->ienable &= ~r;
- /* The virtual irq is derived from register offset.
- * The register difference is word difference. So divide by 2(DABT_WORD)
- * to get Virtual irq number */
- vgic_disable_irqs(v, r & tr,
- (gicd_reg - GICD_ICENABLER) >> DABT_WORD);
+ vgic_disable_irqs(v, r & tr, rank->index);
vgic_unlock_rank(v, rank, flags);
return 1;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 92a3ccf..b5249ff 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -386,8 +386,7 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
vgic_lock_rank(v, rank, flags);
tr = rank->ienable;
rank->ienable |= r;
- /* The irq number is extracted from offset. so shift by register size */
- vgic_enable_irqs(v, r & (~tr), (reg - GICD_ISENABLER) >> DABT_WORD);
+ vgic_enable_irqs(v, r & (~tr), rank->index);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICENABLER ... GICD_ICENABLERN:
@@ -397,8 +396,7 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
vgic_lock_rank(v, rank, flags);
tr = rank->ienable;
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);
+ vgic_disable_irqs(v, r & tr, rank->index);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ISPENDR ... GICD_ISPENDRN:
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 2128d29..7bb4570 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -68,6 +68,13 @@ static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq)
p->irq = virq;
}
+static void vgic_rank_init(struct vgic_irq_rank *rank, uint8_t index)
+{
+ spin_lock_init(&rank->lock);
+
+ rank->index = index;
+}
+
int domain_vgic_init(struct domain *d, unsigned int nr_spis)
{
int i;
@@ -114,8 +121,8 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
for (i=0; i<d->arch.vgic.nr_spis; i++)
vgic_init_pending_irq(&d->arch.vgic.pending_irqs[i], i + 32);
- for (i=0; i<DOMAIN_NR_RANKS(d); i++)
- spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
+ for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
+ vgic_rank_init(&d->arch.vgic.shared_irqs[i], i + 1);
ret = d->arch.vgic.handler->domain_init(d);
if ( ret )
@@ -169,7 +176,7 @@ int vcpu_vgic_init(struct vcpu *v)
if ( v->arch.vgic.private_irqs == NULL )
return -ENOMEM;
- spin_lock_init(&v->arch.vgic.private_irqs->lock);
+ vgic_rank_init(v->arch.vgic.private_irqs, 0);
v->domain->arch.vgic.handler->vcpu_init(v);
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index ff98913..ba74d0f 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -88,6 +88,9 @@ 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 */
+
+ uint8_t index;
+
uint32_t ienable;
uint32_t icfg[2];
--
2.1.4
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-07 14:41 [PATCH v3 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (5 preceding siblings ...)
2015-10-07 14:41 ` [PATCH v3 6/9] xen/arm: vgic: Introduce a new field to store the rank index and use it Julien Grall
@ 2015-10-07 14:41 ` Julien Grall
2015-10-07 15:38 ` Ian Campbell
2015-10-07 17:26 ` Stefano Stabellini
2015-10-07 14:41 ` [PATCH v3 8/9] xen/arm: vgic: Introduce helpers to extract/update/clear/set vGIC register Julien Grall
` (2 subsequent siblings)
9 siblings, 2 replies; 34+ messages in thread
From: Julien Grall @ 2015-10-07 14:41 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, ian.campbell, stefano.stabellini
Xen is currently directly storing the value of GICD_ITARGETSR register
(for GICv2) and GICD_IROUTER (for GICv3) in the rank. This makes the
emulation of the registers access very simple but makes the code to get
the target vCPU for a given vIRQ more complex.
While the target vCPU of an vIRQ is retrieved every time an vIRQ is
injected to the guest, the access to the register occurs less often.
So the data structure should be optimized for the most common case
rather than the inverse.
This patch introduces the usage of an array to store the target vCPU for
every interrupt in the rank. This will make the code to get the target
very quick. The emulation code will now have to generate the GICD_ITARGETSR
and GICD_IROUTER register for read access and split it to store in a
convenient way.
With the new way to store the target vCPU, the structure vgic_irq_rank
is shrunk down from 320 bytes to 92 bytes. This is saving about 228
bytes of memory allocated separately per vCPU.
Note that with these changes, any read to those registers will list only
the target vCPU used by Xen. We think this is likely to be OK because the
GIC spec doesn't require to return exactly the value written and it can be
seen as if we decide to implement the register read-only.
Furthermore, the implementation of the callback get_target_vcpu is now
exactly the same. Consolidate the implementation in the common vGIC code
and drop the callback.
Finally take the opportunity to fix coding style and replace "irq" by
"virq" to make clear that we are dealing with virtual IRQ in section we
are modifying.
Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
The real reason is I'd like to drop vgic_byte_* helpers in favor of more
generic access helpers. Although it would make the code to retrieve the
priority more complex. So reworking the code to get the target vCPU
was the best solution.
Changes in v3:
- Make clear in the commit message that we rely on a corner case
of the spec.
- Typoes
Changes in v2:
- Remove get_target_vcpu callback
- s/irq/virq/ to make clear we are using virtual IRQ
- Update the commit message
- Rename vgic_generate_* to vgic_fetch
- Improve code comment
- Move the introduction of vgic_rank_init in a separate patch
- Make use of rank->idx
Changes in v1:
- Patch added
---
xen/arch/arm/vgic-v2.c | 184 +++++++++++++++++++++++++--------------------
xen/arch/arm/vgic-v3.c | 119 +++++++++++++++--------------
xen/arch/arm/vgic.c | 45 ++++++++---
xen/include/asm-arm/vgic.h | 18 ++---
4 files changed, 207 insertions(+), 159 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 2d63e12..bc03785 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -50,6 +50,97 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t vbase)
vgic_v2_hw.vbase = vbase;
}
+#define NR_TARGET_PER_REG 4U
+#define NR_BIT_PER_TARGET 8U
+
+/*
+ * Fetch a ITARGETSR register based on the offset from ITARGETSR0. Only
+ * one vCPU will be listed for a given vIRQ.
+ *
+ * Note the offset will be aligned to the appropriate boundary.
+ */
+static uint32_t vgic_fetch_itargetsr(struct vgic_irq_rank *rank,
+ unsigned int offset)
+{
+ uint32_t reg = 0;
+ unsigned int i;
+
+ ASSERT(spin_is_locked(&rank->lock));
+
+ offset &= INTERRUPT_RANK_MASK;
+ offset &= ~(NR_TARGET_PER_REG - 1);
+
+ for ( i = 0; i < NR_TARGET_PER_REG; i++, offset++ )
+ reg |= (1 << rank->vcpu[offset]) << (i * NR_BIT_PER_TARGET);
+
+ return reg;
+}
+
+/*
+ * Store a ITARGETSR register in a convenient way and migrate the vIRQ
+ * if necessary. This function only deals with ITARGETSR8 and onwards.
+ *
+ * Note the offset will be aligned to the appropriate boundary.
+ */
+static void vgic_store_itargetsr(struct domain *d, struct vgic_irq_rank *rank,
+ unsigned int offset, uint32_t itargetsr)
+{
+ unsigned int i;
+
+ ASSERT(spin_is_locked(&rank->lock));
+
+ /*
+ * The ITARGETSR0-7, used for SGIs/PPIs, are implemented RO in the
+ * emulation and should never call this function.
+ *
+ * They all live in the first rank.
+ */
+ BUILD_BUG_ON(NR_INTERRUPT_PER_RANK != 32);
+ ASSERT(rank->index >= 1);
+
+ offset &= INTERRUPT_RANK_MASK;
+ offset &= ~(NR_TARGET_PER_REG - 1);
+
+ for ( i = 0; i < NR_TARGET_PER_REG; i++, offset++ )
+ {
+ unsigned int new_target, old_target;
+
+ /*
+ * SPIs are using the 1-N model (see 1.4.3 in ARM IHI 0048B).
+ * While the interrupt could be set pending to all the vCPUs in
+ * target list, it's not guarantee by the spec.
+ * For simplicity, always route the vIRQ to the first interrupt
+ * in the target list
+ */
+ new_target = ffs(itargetsr & ((1 << NR_BIT_PER_TARGET) - 1));
+ old_target = rank->vcpu[offset];
+
+ itargetsr >>= NR_BIT_PER_TARGET;
+
+ /*
+ * Ignore the write request for this interrupt if the new target
+ * is invalid.
+ */
+ if ( !new_target || (new_target > d->max_vcpus) )
+ continue;
+
+ /* The vCPU ID always start from 0 */
+ new_target--;
+
+ /* Only migrate the vIRQ if the target vCPU has changed */
+ if ( new_target != old_target )
+ {
+ unsigned int virq = rank->index * NR_INTERRUPT_PER_RANK + offset;
+
+ vgic_migrate_irq(d->vcpu[old_target],
+ d->vcpu[new_target],
+ virq);
+ }
+
+ rank->vcpu[offset] = new_target;
+ }
+}
+
static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
register_t *r, void *priv)
{
@@ -126,8 +217,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD);
if ( rank == NULL) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
- *r = rank->v2.itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR,
- DABT_WORD)];
+ *r = vgic_fetch_itargetsr(rank, gicd_reg - GICD_ITARGETSR);
if ( dabt.size == DABT_BYTE )
*r = vgic_byte_read(*r, gicd_reg);
vgic_unlock_rank(v, rank, flags);
@@ -337,56 +427,21 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
case GICD_ITARGETSR + 8 ... GICD_ITARGETSRN:
{
- /* unsigned long needed for find_next_bit */
- unsigned long target;
- int i;
+ uint32_t itargetsr;
+
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD);
if ( rank == NULL) goto write_ignore;
- /* 8-bit vcpu mask for this domain */
- BUG_ON(v->domain->max_vcpus > 8);
- target = (1 << v->domain->max_vcpus) - 1;
- if ( dabt.size == 2 )
- target = target | (target << 8) | (target << 16) | (target << 24);
- else
- target = (target << (8 * (gicd_reg & 0x3)));
- target &= r;
- /* ignore zero writes */
- if ( !target )
- goto write_ignore;
- /* For word reads ignore writes where any single byte is zero */
- if ( dabt.size == 2 &&
- !((target & 0xff) && (target & (0xff << 8)) &&
- (target & (0xff << 16)) && (target & (0xff << 24))))
- goto write_ignore;
vgic_lock_rank(v, rank, flags);
- i = 0;
- while ( (i = find_next_bit(&target, 32, i)) < 32 )
- {
- unsigned int irq, new_target, old_target;
- unsigned long old_target_mask;
- struct vcpu *v_target, *v_old;
-
- new_target = i % 8;
- old_target_mask = vgic_byte_read(rank->v2.itargets[REG_RANK_INDEX(8,
- gicd_reg - GICD_ITARGETSR, DABT_WORD)], i/8);
- old_target = find_first_bit(&old_target_mask, 8);
-
- if ( new_target != old_target )
- {
- irq = gicd_reg - GICD_ITARGETSR + (i / 8);
- v_target = v->domain->vcpu[new_target];
- v_old = v->domain->vcpu[old_target];
- vgic_migrate_irq(v_old, v_target, irq);
- }
- i += 8 - new_target;
- }
if ( dabt.size == DABT_WORD )
- rank->v2.itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR,
- DABT_WORD)] = target;
+ itargetsr = r;
else
- vgic_byte_write(&rank->v2.itargets[REG_RANK_INDEX(8,
- gicd_reg - GICD_ITARGETSR, DABT_WORD)], target, gicd_reg);
+ {
+ itargetsr = vgic_fetch_itargetsr(rank, gicd_reg - GICD_ITARGETSR);
+ vgic_byte_write(&itargetsr, r, gicd_reg);
+ }
+ vgic_store_itargetsr(v->domain, rank, gicd_reg - GICD_ITARGETSR,
+ itargetsr);
vgic_unlock_rank(v, rank, flags);
return 1;
}
@@ -487,43 +542,16 @@ static const struct mmio_handler_ops vgic_v2_distr_mmio_handler = {
.write = vgic_v2_distr_mmio_write,
};
-static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
-{
- unsigned long target;
- struct vcpu *v_target;
- struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
- ASSERT(spin_is_locked(&rank->lock));
-
- target = vgic_byte_read(rank->v2.itargets[REG_RANK_INDEX(8,
- irq, DABT_WORD)], irq & 0x3);
-
- /* 1-N SPI should be delivered as pending to all the vcpus in the
- * mask, but here we just return the first vcpu for simplicity and
- * because it would be too slow to do otherwise. */
- target = find_first_bit(&target, 8);
- ASSERT(target >= 0 && target < v->domain->max_vcpus);
- v_target = v->domain->vcpu[target];
- return v_target;
-}
-
static int vgic_v2_vcpu_init(struct vcpu *v)
{
- int i;
-
- /* For SGI and PPI the target is always this CPU */
- for ( i = 0 ; i < 8 ; i++ )
- v->arch.vgic.private_irqs->v2.itargets[i] =
- (1<<(v->vcpu_id+0))
- | (1<<(v->vcpu_id+8))
- | (1<<(v->vcpu_id+16))
- | (1<<(v->vcpu_id+24));
+ /* Nothing specific to initialize for this driver */
return 0;
}
static int vgic_v2_domain_init(struct domain *d)
{
- int i, ret;
+ int ret;
paddr_t cbase;
/*
@@ -563,11 +591,6 @@ static int vgic_v2_domain_init(struct domain *d)
if ( ret )
return ret;
- /* By default deliver to CPU0 */
- for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
- memset(d->arch.vgic.shared_irqs[i].v2.itargets, 0x1,
- sizeof(d->arch.vgic.shared_irqs[i].v2.itargets));
-
register_mmio_handler(d, &vgic_v2_distr_mmio_handler, d->arch.vgic.dbase,
PAGE_SIZE, NULL);
@@ -577,7 +600,6 @@ static int vgic_v2_domain_init(struct domain *d)
static const struct vgic_ops vgic_v2_ops = {
.vcpu_init = vgic_v2_vcpu_init,
.domain_init = vgic_v2_domain_init,
- .get_target_vcpu = vgic_v2_get_target_vcpu,
.max_vcpus = 8,
};
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index b5249ff..2aab77b 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -89,18 +89,72 @@ static struct vcpu *vgic_v3_irouter_to_vcpu(struct domain *d, uint64_t irouter)
return d->vcpu[vcpu_id];
}
-static struct vcpu *vgic_v3_get_target_vcpu(struct vcpu *v, unsigned int irq)
-{
- struct vcpu *v_target;
- struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
+#define NR_BYTE_PER_IROUTER 8U
+/*
+ * Fetch a IROUTER register based on the offset from IROUTER0. Only one
+ * vCPU will be listed for a given vIRQ.
+ *
+ * Note the offset will be aligned to the appropritate boundary.
+ */
+static uint64_t vgic_fetch_irouter(struct vgic_irq_rank *rank,
+ unsigned int offset)
+{
ASSERT(spin_is_locked(&rank->lock));
- v_target = vgic_v3_irouter_to_vcpu(v->domain, rank->v3.irouter[irq % 32]);
+ /* There is exactly 1 vIRQ per IROUTER */
+ offset /= NR_BYTE_PER_IROUTER;
- ASSERT(v_target != NULL);
+ /* Get the index in the rank */
+ offset &= INTERRUPT_RANK_MASK;
- return v_target;
+ return vcpuid_to_vaffinity(rank->vcpu[offset]);
+}
+
+/*
+ * Store a IROUTER register in a convenient way and migrate the vIRQ
+ * if necessary. This function only deals with ITARGETSR32 and onwards.
+ *
+ * Note the offset will be aligned to the appriopriate boundary.
+ */
+static void vgic_store_irouter(struct domain *d, struct vgic_irq_rank *rank,
+ unsigned int offset, uint64_t irouter)
+{
+ struct vcpu *new_vcpu, *old_vcpu;
+ unsigned int virq;
+
+ /* There is 1 vIRQ per IROUTER */
+ virq = offset / NR_BYTE_PER_IROUTER;
+
+ /*
+ * The IROUTER0-31, used for SGIs/PPIs, are reserved and should
+ * never call this function.
+ */
+ ASSERT(virq >= 32);
+
+ /* Get the index in the rank */
+ offset &= virq & INTERRUPT_RANK_MASK;
+
+ new_vcpu = vgic_v3_irouter_to_vcpu(d, irouter);
+ old_vcpu = d->vcpu[rank->vcpu[offset]];
+
+ /*
+ * From the spec (see 8.9.13 in IHI 0069A), any write with an
+ * invalid vCPU will lead to the interrupt being ignored.
+ *
+ * But the current code to inject an IRQ is not able to cope with
+ * invalid vCPU. So for now, just ignore the write.
+ *
+ * TODO: Respect the spec
+ */
+ if ( !new_vcpu )
+ return;
+
+ /* Only migrate the IRQ if the target vCPU has changed */
+ if ( new_vcpu != old_vcpu )
+ vgic_migrate_irq(old_vcpu, new_vcpu, virq);
+
+ rank->vcpu[offset] = new_vcpu->vcpu_id;
}
static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
@@ -726,8 +780,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
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)];
+ *r = vgic_fetch_irouter(rank, gicd_reg - GICD_IROUTER);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_NSACR ... GICD_NSACRN:
@@ -812,8 +865,6 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
struct hsr_dabt dabt = info->dabt;
struct vgic_irq_rank *rank;
unsigned long flags;
- uint64_t new_irouter, old_irouter;
- struct vcpu *old_vcpu, *new_vcpu;
int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
perfc_incr(vgicd_writes);
@@ -881,32 +932,8 @@ 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)];
- old_vcpu = vgic_v3_irouter_to_vcpu(v->domain, old_irouter);
- new_vcpu = vgic_v3_irouter_to_vcpu(v->domain, new_irouter);
-
- if ( !new_vcpu )
- {
- printk(XENLOG_G_DEBUG
- "%pv: vGICD: wrong irouter at offset %#08x val %#"PRIregister,
- v, gicd_reg, r);
- vgic_unlock_rank(v, rank, flags);
- /*
- * TODO: Don't inject a fault to the guest when the MPIDR is
- * not valid. From the spec, the interrupt should be
- * ignored.
- */
- return 0;
- }
- rank->v3.irouter[REG_RANK_INDEX(64, (gicd_reg - GICD_IROUTER),
- DABT_DOUBLE_WORD)] = new_irouter;
- if ( old_vcpu != new_vcpu )
- vgic_migrate_irq(old_vcpu, new_vcpu, (gicd_reg - GICD_IROUTER)/8);
+ vgic_store_irouter(v->domain, rank, gicd_reg - GICD_IROUTER, r);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_NSACR ... GICD_NSACRN:
@@ -1036,7 +1063,6 @@ static const struct mmio_handler_ops vgic_distr_mmio_handler = {
static int vgic_v3_vcpu_init(struct vcpu *v)
{
int i;
- uint64_t affinity;
paddr_t rdist_base;
struct vgic_rdist_region *region;
unsigned int last_cpu;
@@ -1045,15 +1071,6 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
struct domain *d = v->domain;
uint32_t rdist_stride = d->arch.vgic.rdist_stride;
- /* For SGI and PPI the target is always this CPU */
- affinity = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 32 |
- MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 16 |
- MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 8 |
- MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0));
-
- for ( i = 0 ; i < 32 ; i++ )
- v->arch.vgic.private_irqs->v3.irouter[i] = affinity;
-
/*
* Find the region where the re-distributor lives. For this purpose,
* we look one region ahead as we have only the first CPU in hand.
@@ -1098,7 +1115,7 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
static int vgic_v3_domain_init(struct domain *d)
{
- int i, idx;
+ int i;
/*
* Domain 0 gets the hardware address.
@@ -1150,13 +1167,6 @@ static int vgic_v3_domain_init(struct domain *d)
d->arch.vgic.rdist_regions[0].first_cpu = 0;
}
- /* By default deliver to CPU0 */
- for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
- {
- for ( idx = 0; idx < 32; idx++ )
- d->arch.vgic.shared_irqs[i].v3.irouter[idx] = 0;
- }
-
/* Register mmio handle for the Distributor */
register_mmio_handler(d, &vgic_distr_mmio_handler, d->arch.vgic.dbase,
SZ_64K, NULL);
@@ -1182,7 +1192,6 @@ static int vgic_v3_domain_init(struct domain *d)
static const struct vgic_ops v3_ops = {
.vcpu_init = vgic_v3_vcpu_init,
.domain_init = vgic_v3_domain_init,
- .get_target_vcpu = vgic_v3_get_target_vcpu,
.emulate_sysreg = vgic_v3_emulate_sysreg,
/*
* We use both AFF1 and AFF0 in (v)MPIDR. Thus, the max number of CPU
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 7bb4570..531ce5d 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -68,11 +68,24 @@ static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq)
p->irq = virq;
}
-static void vgic_rank_init(struct vgic_irq_rank *rank, uint8_t index)
+static void vgic_rank_init(struct vgic_irq_rank *rank, uint8_t index,
+ unsigned int vcpu)
{
+ unsigned int i;
+
+ /*
+ * Make sure that the type chosen to store the target is able to
+ * store an VCPU ID between 0 and the maximum of virtual CPUs
+ * supported.
+ */
+ BUILD_BUG_ON((1 << (sizeof(rank->vcpu[0]) * 8)) < MAX_VIRT_CPUS);
+
spin_lock_init(&rank->lock);
rank->index = index;
+
+ for ( i = 0; i < NR_INTERRUPT_PER_RANK; i++ )
+ rank->vcpu[i] = vcpu;
}
int domain_vgic_init(struct domain *d, unsigned int nr_spis)
@@ -121,8 +134,9 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
for (i=0; i<d->arch.vgic.nr_spis; i++)
vgic_init_pending_irq(&d->arch.vgic.pending_irqs[i], i + 32);
+ /* SPIs are routed to VCPU0 by default */
for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
- vgic_rank_init(&d->arch.vgic.shared_irqs[i], i + 1);
+ vgic_rank_init(&d->arch.vgic.shared_irqs[i], i + 1, 0);
ret = d->arch.vgic.handler->domain_init(d);
if ( ret )
@@ -176,7 +190,8 @@ int vcpu_vgic_init(struct vcpu *v)
if ( v->arch.vgic.private_irqs == NULL )
return -ENOMEM;
- vgic_rank_init(v->arch.vgic.private_irqs, 0);
+ /* SGIs/PPIs are always routed to this VCPU */
+ vgic_rank_init(v->arch.vgic.private_irqs, 0, v->vcpu_id);
v->domain->arch.vgic.handler->vcpu_init(v);
@@ -197,17 +212,27 @@ int vcpu_vgic_free(struct vcpu *v)
return 0;
}
+/* The function should be called by rank lock taken. */
+static struct vcpu *__vgic_get_target_vcpu(struct vcpu *v, unsigned int virq)
+{
+ struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
+
+ ASSERT(spin_is_locked(&rank->lock));
+
+ return v->domain->vcpu[rank->vcpu[virq & INTERRUPT_RANK_MASK]];
+}
+
/* takes the rank lock */
-struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
+struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq)
{
- struct domain *d = v->domain;
struct vcpu *v_target;
- struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
+ struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
unsigned long flags;
vgic_lock_rank(v, rank, flags);
- v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
+ v_target = __vgic_get_target_vcpu(v, virq);
vgic_unlock_rank(v, rank, flags);
+
return v_target;
}
@@ -286,7 +311,6 @@ void arch_move_irqs(struct vcpu *v)
void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
{
- struct domain *d = v->domain;
const unsigned long mask = r;
struct pending_irq *p;
unsigned int irq;
@@ -296,7 +320,7 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
irq = i + (32 * n);
- v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
+ v_target = __vgic_get_target_vcpu(v, irq);
p = irq_to_pending(v_target, irq);
clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
gic_remove_from_queues(v_target, irq);
@@ -312,7 +336,6 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
{
- struct domain *d = v->domain;
const unsigned long mask = r;
struct pending_irq *p;
unsigned int irq;
@@ -322,7 +345,7 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
irq = i + (32 * n);
- v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
+ v_target = __vgic_get_target_vcpu(v, irq);
p = irq_to_pending(v_target, irq);
set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index ba74d0f..4df7755 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -104,14 +104,11 @@ struct vgic_irq_rank {
uint32_t ipriorityr[8];
};
- union {
- struct {
- uint32_t itargets[8];
- }v2;
- struct {
- uint64_t irouter[32];
- }v3;
- };
+ /*
+ * It's more convenient to store a target VCPU per vIRQ
+ * than the register ITARGETSR/IROUTER itself
+ */
+ uint8_t vcpu[32];
};
struct sgi_target {
@@ -130,9 +127,6 @@ struct vgic_ops {
int (*vcpu_init)(struct vcpu *v);
/* Domain specific initialization of vGIC */
int (*domain_init)(struct domain *d);
- /* Get the target vcpu for a given virq. The rank lock is already taken
- * when calling this. */
- struct vcpu *(*get_target_vcpu)(struct vcpu *v, unsigned int irq);
/* vGIC sysreg emulation */
int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr);
/* Maximum number of vCPU supported */
@@ -205,7 +199,7 @@ enum gic_sgi_mode;
extern int domain_vgic_init(struct domain *d, unsigned int nr_spis);
extern void domain_vgic_free(struct domain *d);
extern int vcpu_vgic_init(struct vcpu *v);
-extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);
+extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq);
extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq);
extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq);
extern void vgic_clear_pending_irqs(struct vcpu *v);
--
2.1.4
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-07 14:41 ` [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank Julien Grall
@ 2015-10-07 15:38 ` Ian Campbell
2015-10-07 15:48 ` Julien Grall
2015-10-07 17:26 ` Stefano Stabellini
1 sibling, 1 reply; 34+ messages in thread
From: Ian Campbell @ 2015-10-07 15:38 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: stefano.stabellini
On Wed, 2015-10-07 at 15:41 +0100, Julien Grall wrote:
> Note that with these changes, any read to those registers will list only
> the target vCPU used by Xen. We think this is likely to be OK because the
> GIC spec doesn't require to return exactly the value written and it can
> be seen as if we decide to implement the register read-only.
This still isn't true.
The GIC spec requires one of two modes: Either the register is read-only
_or_ it is writable and one can expect to read back what was written
(because the spec doesn't say otherwise and doing otherwise would
definitely be exceptional behaviour).
This comment needs to explain why believe we are able to get away with
implementing a third option despite that, i.e. why we think it is ok to
push the boundaries of the spec here.
At no point should the behaviour implemented be compared to implementing
the register as read-only, because that is a bogus comparison. A register
cannot be "sort of read-only" and justifying the behaviour here on that
basis is incorrect.
> Changes in v3:
> - Make clear in the commit message that we rely on a corner case
> of the spec.
No we don't. We do something out of spec which we think we can get away
with implementing as we have done.
Ian.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-07 15:38 ` Ian Campbell
@ 2015-10-07 15:48 ` Julien Grall
2015-10-07 16:00 ` Ian Campbell
0 siblings, 1 reply; 34+ messages in thread
From: Julien Grall @ 2015-10-07 15:48 UTC (permalink / raw)
To: Ian Campbell, xen-devel; +Cc: stefano.stabellini
On 07/10/15 16:38, Ian Campbell wrote:
> On Wed, 2015-10-07 at 15:41 +0100, Julien Grall wrote:
>> Note that with these changes, any read to those registers will list only
>> the target vCPU used by Xen. We think this is likely to be OK because the
>> GIC spec doesn't require to return exactly the value written and it can
>> be seen as if we decide to implement the register read-only.
>
> This still isn't true.
I understood you previous answer on v2 as please replace "this is
fine..." by "We think this is likely to be OK because..."....
> The GIC spec requires one of two modes: Either the register is read-only
> _or_ it is writable and one can expect to read back what was written
> (because the spec doesn't say otherwise and doing otherwise would
> definitely be exceptional behaviour).
How can you say that the spec requires the second mode? Nothing in the
spec say that you can expect to read back what was written...
The spec only says what you the format of the register and what you may
expect to read if the register is read-only. Other than that it's
completely blur and you can implement whatever you want.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-07 15:48 ` Julien Grall
@ 2015-10-07 16:00 ` Ian Campbell
2015-10-07 16:29 ` Julien Grall
0 siblings, 1 reply; 34+ messages in thread
From: Ian Campbell @ 2015-10-07 16:00 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: stefano.stabellini
On Wed, 2015-10-07 at 16:48 +0100, Julien Grall wrote:
> On 07/10/15 16:38, Ian Campbell wrote:
> > On Wed, 2015-10-07 at 15:41 +0100, Julien Grall wrote:
> > > Note that with these changes, any read to those registers will list
> > > only
> > > the target vCPU used by Xen. We think this is likely to be OK because
> > > the
> > > GIC spec doesn't require to return exactly the value written and it
> > > can
> > > be seen as if we decide to implement the register read-only.
> >
> > This still isn't true.
>
> I understood you previous answer on v2 as please replace "this is
> fine..." by "We think this is likely to be OK because..."....
Sorry, I meant ... as "and go on to explain it as we've been discussing".
> > The GIC spec requires one of two modes: Either the register is read
> > -only
> > _or_ it is writable and one can expect to read back what was written
> > (because the spec doesn't say otherwise and doing otherwise would
> > definitely be exceptional behaviour).
>
> How can you say that the spec requires the second mode? Nothing in the
> spec say that you can expect to read back what was written...
Because that is so clearly and obviously the only sane default for a read
write register that nobody even bothers to say it. They specify when it is
not the case that you get back what you wrote by defining what you get on
read.
> The spec only says what you the format of the register and what you may
> expect to read if the register is read-only. Other than that it's
> completely blur and you can implement whatever you want.
Regardless, I am not acking in a patch which implies we think this
behaviour is justified by the spec as this one currently does.
Ian.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-07 16:00 ` Ian Campbell
@ 2015-10-07 16:29 ` Julien Grall
2015-10-07 19:13 ` Julien Grall
0 siblings, 1 reply; 34+ messages in thread
From: Julien Grall @ 2015-10-07 16:29 UTC (permalink / raw)
To: Ian Campbell, xen-devel; +Cc: stefano.stabellini
On 07/10/15 17:00, Ian Campbell wrote:
> On Wed, 2015-10-07 at 16:48 +0100, Julien Grall wrote:
>> On 07/10/15 16:38, Ian Campbell wrote:
>>> On Wed, 2015-10-07 at 15:41 +0100, Julien Grall wrote:
>>>> Note that with these changes, any read to those registers will list
>>>> only
>>>> the target vCPU used by Xen. We think this is likely to be OK because
>>>> the
>>>> GIC spec doesn't require to return exactly the value written and it
>>>> can
>>>> be seen as if we decide to implement the register read-only.
>>>
>>> This still isn't true.
>>
>> I understood you previous answer on v2 as please replace "this is
>> fine..." by "We think this is likely to be OK because..."....
>
> Sorry, I meant ... as "and go on to explain it as we've been discussing".
>
>>> The GIC spec requires one of two modes: Either the register is read
>>> -only
>>> _or_ it is writable and one can expect to read back what was written
>>> (because the spec doesn't say otherwise and doing otherwise would
>>> definitely be exceptional behaviour).
>>
>> How can you say that the spec requires the second mode? Nothing in the
>> spec say that you can expect to read back what was written...
>
> Because that is so clearly and obviously the only sane default for a read
> write register that nobody even bothers to say it. They specify when it is
> not the case that you get back what you wrote by defining what you get on
> read.
Well that's not obvious for me...
>> The spec only says what you the format of the register and what you may
>> expect to read if the register is read-only. Other than that it's
>> completely blur and you can implement whatever you want.
>
> Regardless, I am not acking in a patch which implies we think this
> behaviour is justified by the spec as this one currently does.
TBH, I don't see how I can justify that what we are doing is fine except
by saying "for simplicity".
If you think this is not justify by the spec, then maybe we should just
drop this patch. It's nice to have it but not strictly mandatory.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-07 16:29 ` Julien Grall
@ 2015-10-07 19:13 ` Julien Grall
2015-10-08 9:39 ` Ian Campbell
0 siblings, 1 reply; 34+ messages in thread
From: Julien Grall @ 2015-10-07 19:13 UTC (permalink / raw)
To: Ian Campbell, xen-devel; +Cc: stefano.stabellini
On 07/10/15 17:29, Julien Grall wrote:
> On 07/10/15 17:00, Ian Campbell wrote:
>> On Wed, 2015-10-07 at 16:48 +0100, Julien Grall wrote:
>>> On 07/10/15 16:38, Ian Campbell wrote:
>>>> On Wed, 2015-10-07 at 15:41 +0100, Julien Grall wrote:
>>>>> Note that with these changes, any read to those registers will list
>>>>> only
>>>>> the target vCPU used by Xen. We think this is likely to be OK because
>>>>> the
>>>>> GIC spec doesn't require to return exactly the value written and it
>>>>> can
>>>>> be seen as if we decide to implement the register read-only.
>>>>
>>>> This still isn't true.
>>>
>>> I understood you previous answer on v2 as please replace "this is
>>> fine..." by "We think this is likely to be OK because..."....
>>
>> Sorry, I meant ... as "and go on to explain it as we've been discussing".
>>
>>>> The GIC spec requires one of two modes: Either the register is read
>>>> -only
>>>> _or_ it is writable and one can expect to read back what was written
>>>> (because the spec doesn't say otherwise and doing otherwise would
>>>> definitely be exceptional behaviour).
>>>
>>> How can you say that the spec requires the second mode? Nothing in the
>>> spec say that you can expect to read back what was written...
>>
>> Because that is so clearly and obviously the only sane default for a read
>> write register that nobody even bothers to say it. They specify when it is
>> not the case that you get back what you wrote by defining what you get on
>> read.
>
> Well that's not obvious for me...
>
>>> The spec only says what you the format of the register and what you may
>>> expect to read if the register is read-only. Other than that it's
>>> completely blur and you can implement whatever you want.
>>
>> Regardless, I am not acking in a patch which implies we think this
>> behaviour is justified by the spec as this one currently does.
>
> TBH, I don't see how I can justify that what we are doing is fine except
> by saying "for simplicity".
I though a bit more about it. We are both interpreting the spec
differently and I think both are valid. So hardware people and OS
developer may also lean toward one of them.
Mine is more restrictive than yours, as you said on v1, it would hit
picky OS that follow your interpretation and read back the register to
check either the value is exactly the one written and/or the previous
value. Although it make the implementation simpler and save memory.
How about:
"Note that with these changes, any read to those registers will list
only the target vCPU used by Xen. As the spec is not clear whether this
is a valid choice or not, picky OSes (i.e which read back register to
check the value written) which have a different interpretation of the
spec may not boot anymore on Xen. Although, I think this is a fair trade
between memory usage in Xen (1KB on a domain using 4 vCPUs with no SPIs)
and a strict interpretation of the spec (though all the cases are not
clearly defined)".
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-07 19:13 ` Julien Grall
@ 2015-10-08 9:39 ` Ian Campbell
2015-10-08 10:43 ` Julien Grall
0 siblings, 1 reply; 34+ messages in thread
From: Ian Campbell @ 2015-10-08 9:39 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: stefano.stabellini
On Wed, 2015-10-07 at 20:13 +0100, Julien Grall wrote:
> On 07/10/15 17:29, Julien Grall wrote:
> > TBH, I don't see how I can justify that what we are doing is fine except
> > by saying "for simplicity".
I think it is also fine to say that we currently expect that in reality
OSes won't rely on being able to read-back from this register.
> I though a bit more about it. We are both interpreting the spec
> differently and I think both are valid. So hardware people and OS
> developer may also lean toward one of them.
TBH I don't agree, your definition essentially turns any RW register which
does not specify precisely otherwise into a WO register.
> Mine is more restrictive than yours, as you said on v1, it would hit
> picky OS that follow your interpretation and read back the register to
> check either the value is exactly the one written and/or the previous
> value. Although it make the implementation simpler and save memory.
>
> How about:
>
> "Note that with these changes, any read to those registers will list
> only the target vCPU used by Xen. As the spec is not clear whether this
> is a valid choice or not, picky OSes (i.e which read back register to
> check the value written) which have a different interpretation of the
> spec may not boot anymore on Xen. Although, I think this is a fair trade
> between memory usage in Xen (1KB on a domain using 4 vCPUs with no SPIs)
> and a strict interpretation of the spec (though all the cases are not
> clearly defined)".
That's OK. I'd prefer to drop the "picky" and to make the "(i.e. ....)" in
the middle to say something like "(i.e. OSes which perform read-modify
-write operations on these registers"), which is not a picky thing to want
to do even if we don't expect any OS to actually to it.
BTW, IHI 0069A says 8.9.17:
When affinity routing is not enabled, holds the list of target PEs for
the interrupt. That is, it holds the list of CPU interfaces to which the
Distributor forwards the interrupt if it is asserted and has sufficient
priority.
Which isn't actually inconsistent with the register reading back the set of
CPUs to which the interrupt is actually going to end up being routed (i.e.
the behaviour of this patch).
However I think this has gone on long enough and some text which says it
isn't clear is still a reasonable thing to write.
Ian.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-08 9:39 ` Ian Campbell
@ 2015-10-08 10:43 ` Julien Grall
0 siblings, 0 replies; 34+ messages in thread
From: Julien Grall @ 2015-10-08 10:43 UTC (permalink / raw)
To: Ian Campbell, xen-devel; +Cc: stefano.stabellini
On 08/10/15 10:39, Ian Campbell wrote:
> On Wed, 2015-10-07 at 20:13 +0100, Julien Grall wrote:
>> On 07/10/15 17:29, Julien Grall wrote:
>>> TBH, I don't see how I can justify that what we are doing is fine except
>>> by saying "for simplicity".
>
> I think it is also fine to say that we currently expect that in reality
> OSes won't rely on being able to read-back from this register.
>
>> I though a bit more about it. We are both interpreting the spec
>> differently and I think both are valid. So hardware people and OS
>> developer may also lean toward one of them.
>
> TBH I don't agree, your definition essentially turns any RW register which
> does not specify precisely otherwise into a WO register.
>
>> Mine is more restrictive than yours, as you said on v1, it would hit
>> picky OS that follow your interpretation and read back the register to
>> check either the value is exactly the one written and/or the previous
>> value. Although it make the implementation simpler and save memory.
>>
>> How about:
>>
>> "Note that with these changes, any read to those registers will list
>> only the target vCPU used by Xen. As the spec is not clear whether this
>> is a valid choice or not, picky OSes (i.e which read back register to
>> check the value written) which have a different interpretation of the
>> spec may not boot anymore on Xen. Although, I think this is a fair trade
>> between memory usage in Xen (1KB on a domain using 4 vCPUs with no SPIs)
>> and a strict interpretation of the spec (though all the cases are not
>> clearly defined)".
>
> That's OK. I'd prefer to drop the "picky" and to make the "(i.e. ....)" in
> the middle to say something like "(i.e. OSes which perform read-modify
> -write operations on these registers"), which is not a picky thing to want
> to do even if we don't expect any OS to actually to it.
I will update the commit message.
> BTW, IHI 0069A says 8.9.17:
>
> When affinity routing is not enabled, holds the list of target PEs for
> the interrupt. That is, it holds the list of CPU interfaces to which the
> Distributor forwards the interrupt if it is asserted and has sufficient
> priority.
I read multiple time the ITARGETSR section and somehow miss this paragraph.
>
> Which isn't actually inconsistent with the register reading back the set of
> CPUs to which the interrupt is actually going to end up being routed (i.e.
> the behaviour of this patch).
>
> However I think this has gone on long enough and some text which says it
> isn't clear is still a reasonable thing to write.
I will resend a new version after I got an answer from Stefano on [1].
Regards,
[1] http://lists.xen.org/archives/html/xen-devel/2015-10/msg00893.html
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-07 14:41 ` [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank Julien Grall
2015-10-07 15:38 ` Ian Campbell
@ 2015-10-07 17:26 ` Stefano Stabellini
2015-10-07 18:16 ` Julien Grall
1 sibling, 1 reply; 34+ messages in thread
From: Stefano Stabellini @ 2015-10-07 17:26 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, ian.campbell, stefano.stabellini
On Wed, 7 Oct 2015, Julien Grall wrote:
> +/*
> + * Store a ITARGETSR register in a convenient way and migrate the vIRQ
> + * if necessary. This function only deals with ITARGETSR8 and onwards.
> + *
> + * Note the offset will be aligned to the appropriate boundary.
> + */
> +static void vgic_store_itargetsr(struct domain *d, struct vgic_irq_rank *rank,
> + unsigned int offset, uint32_t itargetsr)
> +{
> + unsigned int i;
> +
> + ASSERT(spin_is_locked(&rank->lock));
> +
> + /*
> + * The ITARGETSR0-7, used for SGIs/PPIs, are implemented RO in the
> + * emulation and should never call this function.
> + *
> + * They all live in the first rank.
> + */
> + BUILD_BUG_ON(NR_INTERRUPT_PER_RANK != 32);
> + ASSERT(rank->index >= 1);
> +
> + offset &= INTERRUPT_RANK_MASK;
> + offset &= ~(NR_TARGET_PER_REG - 1);
> +
> + for ( i = 0; i < NR_TARGET_PER_REG; i++, offset++ )
> + {
> + unsigned int new_target, old_target;
> +
> + /*
> + * SPIs are using the 1-N model (see 1.4.3 in ARM IHI 0048B).
> + * While the interrupt could be set pending to all the vCPUs in
> + * target list, it's not guarantee by the spec.
> + * For simplicity, always route the vIRQ to the first interrupt
> + * in the target list
> + */
> + new_target = ffs(itargetsr & ((1 << NR_BIT_PER_TARGET) - 1));
> + old_target = rank->vcpu[offset];
> +
> + itargetsr >>= NR_BIT_PER_TARGET;
> +
> + /*
> + * Ignore the write request for this interrupt if the new target
> + * is invalid.
> + */
> + if ( !new_target || (new_target > d->max_vcpus) )
> + continue;
> +
> + /* The vCPU ID always start from 0 */
> + new_target--;
> +
> + /* Only migrate the vIRQ if the target vCPU has changed */
> + if ( new_target != old_target )
> + {
> + unsigned int virq = rank->index * NR_INTERRUPT_PER_RANK + offset;
> +
> + vgic_migrate_irq(d->vcpu[old_target],
> + d->vcpu[new_target],
> + virq);
> + }
> +
> + rank->vcpu[offset] = new_target;
> + }
> +}
This introduces a behavioural change: previously 32-bit writes which
contained one 0 byte would be ignored in their entirety. See:
http://marc.info/?l=xen-devel&m=140431564123221
Even though I am not entirely sure about which one is the correct
behaviour according to the spec, I prefer the old one because I think it
is less surprising to users.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-07 17:26 ` Stefano Stabellini
@ 2015-10-07 18:16 ` Julien Grall
2015-10-08 10:56 ` Ian Campbell
0 siblings, 1 reply; 34+ messages in thread
From: Julien Grall @ 2015-10-07 18:16 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, ian.campbell
On 07/10/15 18:26, Stefano Stabellini wrote:
> On Wed, 7 Oct 2015, Julien Grall wrote:
>> +/*
>> + * Store a ITARGETSR register in a convenient way and migrate the vIRQ
>> + * if necessary. This function only deals with ITARGETSR8 and onwards.
>> + *
>> + * Note the offset will be aligned to the appropriate boundary.
>> + */
>> +static void vgic_store_itargetsr(struct domain *d, struct vgic_irq_rank *rank,
>> + unsigned int offset, uint32_t itargetsr)
>> +{
>> + unsigned int i;
>> +
>> + ASSERT(spin_is_locked(&rank->lock));
>> +
>> + /*
>> + * The ITARGETSR0-7, used for SGIs/PPIs, are implemented RO in the
>> + * emulation and should never call this function.
>> + *
>> + * They all live in the first rank.
>> + */
>> + BUILD_BUG_ON(NR_INTERRUPT_PER_RANK != 32);
>> + ASSERT(rank->index >= 1);
>> +
>> + offset &= INTERRUPT_RANK_MASK;
>> + offset &= ~(NR_TARGET_PER_REG - 1);
>> +
>> + for ( i = 0; i < NR_TARGET_PER_REG; i++, offset++ )
>> + {
>> + unsigned int new_target, old_target;
>> +
>> + /*
>> + * SPIs are using the 1-N model (see 1.4.3 in ARM IHI 0048B).
>> + * While the interrupt could be set pending to all the vCPUs in
>> + * target list, it's not guarantee by the spec.
>> + * For simplicity, always route the vIRQ to the first interrupt
>> + * in the target list
>> + */
>> + new_target = ffs(itargetsr & ((1 << NR_BIT_PER_TARGET) - 1));
>> + old_target = rank->vcpu[offset];
>> +
>> + itargetsr >>= NR_BIT_PER_TARGET;
>> +
>> + /*
>> + * Ignore the write request for this interrupt if the new target
>> + * is invalid.
>> + */
>> + if ( !new_target || (new_target > d->max_vcpus) )
>> + continue;
>> +
>> + /* The vCPU ID always start from 0 */
>> + new_target--;
>> +
>> + /* Only migrate the vIRQ if the target vCPU has changed */
>> + if ( new_target != old_target )
>> + {
>> + unsigned int virq = rank->index * NR_INTERRUPT_PER_RANK + offset;
>> +
>> + vgic_migrate_irq(d->vcpu[old_target],
>> + d->vcpu[new_target],
>> + virq);
>> + }
>> +
>> + rank->vcpu[offset] = new_target;
>> + }
>> +}
>
> This introduces a behavioural change: previously 32-bit writes which
> contained one 0 byte would be ignored in their entirety. See:
>
> http://marc.info/?l=xen-devel&m=140431564123221
It's true that this may change the behavior of a guest writing the
ITARGETSR with a byte to 0. But what's the point to have a guest relying
on its write to be ignored?
Aside that, I don't think we are bound to emulate exactly the same
behavior in each version of Xen. We may decide to slightly change it
because we have to rework the code to get a common emulation or fix a bug.
>
> Even though I am not entirely sure about which one is the correct
> behaviour according to the spec, I prefer the old one because I think it
> is less surprising to users.
How ignoring the whole write would be less surprising? ;) There is no
warning at all, so it's very difficult for the user to know why the
write has been ignored.
Furthermore, based on the spec (4.3.12 in IHI 0048B.b): "A register
field corresponding to an unimplemented interrupt is RAZ/WI."
If the user knows that an interrupt is not implemented, he may decide to
write 0 in the corresponding byte. With the current solution, the whole
write access is ignored.
The solution suggested in this patch is less restrictive and will just
ignore the corresponding byte if it's 0.
On another side, nothing in the spec specified what happen if the target
field is 0 for a valid interrupt. But I think this is OK to just ignore
it and carry on. It's simpler to implement.
I didn't mention this such things in the commit message. I will update
it if we decide to carry on with this patch.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-07 18:16 ` Julien Grall
@ 2015-10-08 10:56 ` Ian Campbell
2015-10-08 11:36 ` Stefano Stabellini
0 siblings, 1 reply; 34+ messages in thread
From: Ian Campbell @ 2015-10-08 10:56 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
On Wed, 2015-10-07 at 19:16 +0100, Julien Grall wrote:
> Furthermore, based on the spec (4.3.12 in IHI 0048B.b): "A register
> field corresponding to an unimplemented interrupt is RAZ/WI."
>
> If the user knows that an interrupt is not implemented, he may decide to
> write 0 in the corresponding byte. With the current solution, the whole
> write access is ignored.
>
> The solution suggested in this patch is less restrictive and will just
> ignore the corresponding byte if it's 0.
I think this (a 32-bit register covering both implemented and non
-implemented interrupts) is a compelling reason to only ignore the specific
zero bytes and not the whole word.
> On another side, nothing in the spec specified what happen if the target
> field is 0 for a valid interrupt. But I think this is OK to just ignore
> it and carry on. It's simpler to implement.
On a (not very) related notes, these registers are res0 when affinity
routing is enabled on gic-v3 (which I think is our hardcoded configuration)
and I noticed that vgic_v3_distr_mmio_read doesn't look to implement that
explicitly, hence it uses the default case which logs. Probably we should
add a proper handler which silently does RAZ/WI?
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-08 10:56 ` Ian Campbell
@ 2015-10-08 11:36 ` Stefano Stabellini
2015-10-08 12:23 ` Ian Campbell
0 siblings, 1 reply; 34+ messages in thread
From: Stefano Stabellini @ 2015-10-08 11:36 UTC (permalink / raw)
To: Ian Campbell; +Cc: Julien Grall, xen-devel, Stefano Stabellini
On Thu, 8 Oct 2015, Ian Campbell wrote:
> On Wed, 2015-10-07 at 19:16 +0100, Julien Grall wrote:
>
> > Furthermore, based on the spec (4.3.12 in IHI 0048B.b): "A register
> > field corresponding to an unimplemented interrupt is RAZ/WI."
> >
> > If the user knows that an interrupt is not implemented, he may decide to
> > write 0 in the corresponding byte. With the current solution, the whole
> > write access is ignored.
> >
> > The solution suggested in this patch is less restrictive and will just
> > ignore the corresponding byte if it's 0.
>
> I think this (a 32-bit register covering both implemented and non
> -implemented interrupts) is a compelling reason to only ignore the specific
> zero bytes and not the whole word.
I agree that zero writes to unimplemented interrupts should be allowed.
However allowing them for everything encourages 32-bit writes with just
one byte set, the one that the OS actually wants to write. It doesn't
seem correct to me. Something like:
uint32_t val = 0x2 << 8;
write32(ITARGETSR + something, val);
which I don't think is supposed to work. That said, I recognize that
this is a minor issue, so I won't insist.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-08 11:36 ` Stefano Stabellini
@ 2015-10-08 12:23 ` Ian Campbell
2015-10-08 12:34 ` Stefano Stabellini
2015-10-08 13:46 ` Julien Grall
0 siblings, 2 replies; 34+ messages in thread
From: Ian Campbell @ 2015-10-08 12:23 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Julien Grall, xen-devel
On Thu, 2015-10-08 at 12:36 +0100, Stefano Stabellini wrote:
> On Thu, 8 Oct 2015, Ian Campbell wrote:
> > On Wed, 2015-10-07 at 19:16 +0100, Julien Grall wrote:
> >
> > > Furthermore, based on the spec (4.3.12 in IHI 0048B.b): "A register
> > > field corresponding to an unimplemented interrupt is RAZ/WI."
> > >
> > > If the user knows that an interrupt is not implemented, he may decide
> > > to
> > > write 0 in the corresponding byte. With the current solution, the
> > > whole
> > > write access is ignored.
> > >
> > > The solution suggested in this patch is less restrictive and will
> > > just
> > > ignore the corresponding byte if it's 0.
> >
> > I think this (a 32-bit register covering both implemented and non
> > -implemented interrupts) is a compelling reason to only ignore the
> > specific
> > zero bytes and not the whole word.
>
> I agree that zero writes to unimplemented interrupts should be allowed.
> However allowing them for everything encourages 32-bit writes with just
> one byte set, the one that the OS actually wants to write. It doesn't
> seem correct to me. Something like:
>
> uint32_t val = 0x2 << 8;
> write32(ITARGETSR + something, val);
>
> which I don't think is supposed to work. That said, I recognize that
> this is a minor issue, so I won't insist.
Right.
The underlying issue here is that we can't cope with interrupts which are
not routed to any CPU at all, which is the expected semantics for a write
of 0 to the TARGET, right? (such interrupts essentially remain pending in
the distributor).
How hard would it be to actually implement that and therefore avoid this
whole issue?
Another approach btw would be to insist that nr_spis was a multiple of at
least 4, then you don't have registers which are a mixture of implemented
and unimplemented, which might simplify the logic.
Ian.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-08 12:23 ` Ian Campbell
@ 2015-10-08 12:34 ` Stefano Stabellini
2015-10-08 13:46 ` Julien Grall
1 sibling, 0 replies; 34+ messages in thread
From: Stefano Stabellini @ 2015-10-08 12:34 UTC (permalink / raw)
To: Ian Campbell; +Cc: Julien Grall, xen-devel, Stefano Stabellini
On Thu, 8 Oct 2015, Ian Campbell wrote:
> On Thu, 2015-10-08 at 12:36 +0100, Stefano Stabellini wrote:
> > On Thu, 8 Oct 2015, Ian Campbell wrote:
> > > On Wed, 2015-10-07 at 19:16 +0100, Julien Grall wrote:
> > >
> > > > Furthermore, based on the spec (4.3.12 in IHI 0048B.b): "A register
> > > > field corresponding to an unimplemented interrupt is RAZ/WI."
> > > >
> > > > If the user knows that an interrupt is not implemented, he may decide
> > > > to
> > > > write 0 in the corresponding byte. With the current solution, the
> > > > whole
> > > > write access is ignored.
> > > >
> > > > The solution suggested in this patch is less restrictive and will
> > > > just
> > > > ignore the corresponding byte if it's 0.
> > >
> > > I think this (a 32-bit register covering both implemented and non
> > > -implemented interrupts) is a compelling reason to only ignore the
> > > specific
> > > zero bytes and not the whole word.
> >
> > I agree that zero writes to unimplemented interrupts should be allowed.
> > However allowing them for everything encourages 32-bit writes with just
> > one byte set, the one that the OS actually wants to write. It doesn't
> > seem correct to me. Something like:
> >
> > uint32_t val = 0x2 << 8;
> > write32(ITARGETSR + something, val);
> >
> > which I don't think is supposed to work. That said, I recognize that
> > this is a minor issue, so I won't insist.
>
> Right.
>
> The underlying issue here is that we can't cope with interrupts which are
> not routed to any CPU at all, which is the expected semantics for a write
> of 0 to the TARGET, right? (such interrupts essentially remain pending in
> the distributor).
Unfortunately the spec is not clear about what is the expected behaviour
in this case. I read it as "zero is not an allowed value".
> How hard would it be to actually implement that and therefore avoid this
> whole issue?
>
> Another approach btw would be to insist that nr_spis was a multiple of at
> least 4, then you don't have registers which are a mixture of implemented
> and unimplemented, which might simplify the logic.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-08 12:23 ` Ian Campbell
2015-10-08 12:34 ` Stefano Stabellini
@ 2015-10-08 13:46 ` Julien Grall
2015-10-08 14:25 ` Ian Campbell
1 sibling, 1 reply; 34+ messages in thread
From: Julien Grall @ 2015-10-08 13:46 UTC (permalink / raw)
To: Ian Campbell, Stefano Stabellini; +Cc: xen-devel
On 08/10/15 13:23, Ian Campbell wrote:
> On Thu, 2015-10-08 at 12:36 +0100, Stefano Stabellini wrote:
>> On Thu, 8 Oct 2015, Ian Campbell wrote:
>>> On Wed, 2015-10-07 at 19:16 +0100, Julien Grall wrote:
>>>
>>>> Furthermore, based on the spec (4.3.12 in IHI 0048B.b): "A register
>>>> field corresponding to an unimplemented interrupt is RAZ/WI."
>>>>
>>>> If the user knows that an interrupt is not implemented, he may decide
>>>> to
>>>> write 0 in the corresponding byte. With the current solution, the
>>>> whole
>>>> write access is ignored.
>>>>
>>>> The solution suggested in this patch is less restrictive and will
>>>> just
>>>> ignore the corresponding byte if it's 0.
>>>
>>> I think this (a 32-bit register covering both implemented and non
>>> -implemented interrupts) is a compelling reason to only ignore the
>>> specific
>>> zero bytes and not the whole word.
>>
>> I agree that zero writes to unimplemented interrupts should be allowed.
>> However allowing them for everything encourages 32-bit writes with just
>> one byte set, the one that the OS actually wants to write. It doesn't
>> seem correct to me. Something like:
>>
>> uint32_t val = 0x2 << 8;
>> write32(ITARGETSR + something, val);
>>
>> which I don't think is supposed to work. That said, I recognize that
>> this is a minor issue, so I won't insist.
>
> Right.
>
> The underlying issue here is that we can't cope with interrupts which are
> not routed to any CPU at all, which is the expected semantics for a write
> of 0 to the TARGET, right? (such interrupts essentially remain pending in
> the distributor).
>
> How hard would it be to actually implement that and therefore avoid this
> whole issue?
It will take sometime to figure all the place which take a vcpu and
expect it valid and fix it.
While this should be done at some point to respect the GICv3 and GICv2
spec, I think the correct support of 0 byte in GICD_ITARGETSR is not
that important because it likely would hit people trying to disable an
interrupt via GICD_ITARGETSR rather than properly using GICD_ICENABLER
register to disable it.
I agree that this patch is by side effect moving from a wrong behavior
to another wrong behavior (though slightly less). Although, I don't want
to fix it properly in this series because I can't fix everything in a
single series. The main purpose here is to fix access size to emulated
register. This has to be fixed now as it prevents real guest to properly
boot on Xen.
If the concern is the behavior is changed, I'm happy to rework this code
to keep exactly the same behavior. I.e any 32-bit write containing
a 0 byte will be ignored. This is not optimal but at least I'm not
opening the pandora box of fixing every single error in the code touch
by this series.
> Another approach btw would be to insist that nr_spis was a multiple of at
> least 4, then you don't have registers which are a mixture of implemented
> and unimplemented, which might simplify the logic.
GICD_TYPER.ITLines is always a multiple of 32 (and therefore 4).
Although, this is only representing the number of SPIs supported by the
Distributor.
Some of them may not be wired and any write to the corresponding byte
should be ignored.
Those interrupt can be detected using the d->arch.vgic.allocated_irqs
bitmap.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-08 13:46 ` Julien Grall
@ 2015-10-08 14:25 ` Ian Campbell
2015-10-08 18:36 ` Julien Grall
2015-10-12 10:41 ` Stefano Stabellini
0 siblings, 2 replies; 34+ messages in thread
From: Ian Campbell @ 2015-10-08 14:25 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
On Thu, 2015-10-08 at 14:46 +0100, Julien Grall wrote:
> On 08/10/15 13:23, Ian Campbell wrote:
> > On Thu, 2015-10-08 at 12:36 +0100, Stefano Stabellini wrote:
> > > On Thu, 8 Oct 2015, Ian Campbell wrote:
> > > > On Wed, 2015-10-07 at 19:16 +0100, Julien Grall wrote:
> > > >
> > > > > Furthermore, based on the spec (4.3.12 in IHI 0048B.b): "A
> > > > > register
> > > > > field corresponding to an unimplemented interrupt is RAZ/WI."
> > > > >
> > > > > If the user knows that an interrupt is not implemented, he may
> > > > > decide
> > > > > to
> > > > > write 0 in the corresponding byte. With the current solution, the
> > > > > whole
> > > > > write access is ignored.
> > > > >
> > > > > The solution suggested in this patch is less restrictive and will
> > > > > just
> > > > > ignore the corresponding byte if it's 0.
> > > >
> > > > I think this (a 32-bit register covering both implemented and non
> > > > -implemented interrupts) is a compelling reason to only ignore the
> > > > specific
> > > > zero bytes and not the whole word.
> > >
> > > I agree that zero writes to unimplemented interrupts should be
> > > allowed.
> > > However allowing them for everything encourages 32-bit writes with
> > > just
> > > one byte set, the one that the OS actually wants to write. It doesn't
> > > seem correct to me. Something like:
> > >
> > > uint32_t val = 0x2 << 8;
> > > write32(ITARGETSR + something, val);
> > >
> > > which I don't think is supposed to work. That said, I recognize that
> > > this is a minor issue, so I won't insist.
> >
> > Right.
> >
> > The underlying issue here is that we can't cope with interrupts which
> > are
> > not routed to any CPU at all, which is the expected semantics for a
> > write
> > of 0 to the TARGET, right? (such interrupts essentially remain pending
> > in
> > the distributor).
> >
> > How hard would it be to actually implement that and therefore avoid
> > this
> > whole issue?
>
> It will take sometime to figure all the place which take a vcpu and
> expect it valid and fix it.
>
> While this should be done at some point to respect the GICv3 and GICv2
> spec, I think the correct support of 0 byte in GICD_ITARGETSR is not
> that important because it likely would hit people trying to disable an
> interrupt via GICD_ITARGETSR rather than properly using GICD_ICENABLER
> register to disable it.
>
> I agree that this patch is by side effect moving from a wrong behavior
> to another wrong behavior (though slightly less).
Right.
> Although, I don't want
> to fix it properly in this series because I can't fix everything in a
> single series. The main purpose here is to fix access size to emulated
> register. This has to be fixed now as it prevents real guest to properly
> boot on Xen.
This is fair enough.
> If the concern is the behavior is changed, I'm happy to rework this code
> to keep exactly the same behavior. I.e any 32-bit write containing
> a 0 byte will be ignored. This is not optimal but at least I'm not
> opening the pandora box of fixing every single error in the code touch
> by this series.
I'm okay with the new behaviour, I think Stefano was willing to tolerate it
(based on <alpine.DEB.2.02.1510081220190.1179@kaball.uk.xensource.com>).
So if we aren't going to fix it to DTRT WRT writing zero to a target then I
think we can go with the current variant and not change to ignoring any
word with a zero byte in it.
Ian.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-08 14:25 ` Ian Campbell
@ 2015-10-08 18:36 ` Julien Grall
2015-10-09 11:24 ` Julien Grall
2015-10-12 10:41 ` Stefano Stabellini
1 sibling, 1 reply; 34+ messages in thread
From: Julien Grall @ 2015-10-08 18:36 UTC (permalink / raw)
To: Ian Campbell, Stefano Stabellini; +Cc: xen-devel
On 08/10/15 15:25, Ian Campbell wrote:
>> If the concern is the behavior is changed, I'm happy to rework this code
>> to keep exactly the same behavior. I.e any 32-bit write containing
>> a 0 byte will be ignored. This is not optimal but at least I'm not
>> opening the pandora box of fixing every single error in the code touch
>> by this series.
>
> I'm okay with the new behaviour, I think Stefano was willing to tolerate it
> (based on <alpine.DEB.2.02.1510081220190.1179@kaball.uk.xensource.com>).
It's what I understand too. I will a resend a new version tomorrow.
> So if we aren't going to fix it to DTRT WRT writing zero to a target then I
> think we can go with the current variant and not change to ignoring any
> word with a zero byte in it.
I'm thinking to split this patch in two:
- One which will turn the current ITARGETSR emulation in a function
with the change of behavior when writing zero to a target.
- The other to optimize the way we store the target.
This will keep the second patch nearly mechanical and avoid to change
multiple behavior within the same patch.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-08 18:36 ` Julien Grall
@ 2015-10-09 11:24 ` Julien Grall
2015-10-09 11:38 ` Ian Campbell
0 siblings, 1 reply; 34+ messages in thread
From: Julien Grall @ 2015-10-09 11:24 UTC (permalink / raw)
To: Ian Campbell, Stefano Stabellini; +Cc: xen-devel
On 08/10/15 19:36, Julien Grall wrote:
> On 08/10/15 15:25, Ian Campbell wrote:
>>> If the concern is the behavior is changed, I'm happy to rework this code
>>> to keep exactly the same behavior. I.e any 32-bit write containing
>>> a 0 byte will be ignored. This is not optimal but at least I'm not
>>> opening the pandora box of fixing every single error in the code touch
>>> by this series.
>>
>> I'm okay with the new behaviour, I think Stefano was willing to tolerate it
>> (based on <alpine.DEB.2.02.1510081220190.1179@kaball.uk.xensource.com>).
>
> It's what I understand too. I will a resend a new version tomorrow.
>
>> So if we aren't going to fix it to DTRT WRT writing zero to a target then I
>> think we can go with the current variant and not change to ignoring any
>> word with a zero byte in it.
>
> I'm thinking to split this patch in two:
> - One which will turn the current ITARGETSR emulation in a function
> with the change of behavior when writing zero to a target.
> - The other to optimize the way we store the target.
>
> This will keep the second patch nearly mechanical and avoid to change
> multiple behavior within the same patch.
While looking to split the patch in two part I've noticed another error
in the current emulation of ITARGETSR.
For a byte access, the byte will always be in r[0:7] of the register.
Although we are assuming that if the guest is writing to byte N (0 <= N
< 3), the byte will be in r[N * 8: ((N + 1)* 8) - 1]. Therefore any
guest using byte access won't be able to change the target:
target = (target << (8 * (gicd_reg & 0x3)));
target &= r;
I plan to fix it in patch #1 and request a backport for Xen 4.6 and Xen
4.5. I can do a separate patch if we don't want the cleanup. Note that
the second patch will be a requirement to fix 32-bit access to 64-bit
register.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-09 11:24 ` Julien Grall
@ 2015-10-09 11:38 ` Ian Campbell
0 siblings, 0 replies; 34+ messages in thread
From: Ian Campbell @ 2015-10-09 11:38 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
On Fri, 2015-10-09 at 12:24 +0100, Julien Grall wrote:
> I plan to fix it in patch #1 and request a backport for Xen 4.6 and Xen
> 4.5. I can do a separate patch if we don't want the cleanup.
I'm not quite sure what you are proposing but please put logical changes
and cleanups into separate patches and we will evaluate them for backport
individually (taking dependencies into account too).
Ian.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-08 14:25 ` Ian Campbell
2015-10-08 18:36 ` Julien Grall
@ 2015-10-12 10:41 ` Stefano Stabellini
2015-10-12 11:00 ` Julien Grall
1 sibling, 1 reply; 34+ messages in thread
From: Stefano Stabellini @ 2015-10-12 10:41 UTC (permalink / raw)
To: Ian Campbell; +Cc: Julien Grall, xen-devel, Stefano Stabellini
On Thu, 8 Oct 2015, Ian Campbell wrote:
> > If the concern is the behavior is changed, I'm happy to rework this code
> > to keep exactly the same behavior. I.e any 32-bit write containing
> > a 0 byte will be ignored. This is not optimal but at least I'm not
> > opening the pandora box of fixing every single error in the code touch
> > by this series.
>
> I'm okay with the new behaviour, I think Stefano was willing to tolerate it
> (based on <alpine.DEB.2.02.1510081220190.1179@kaball.uk.xensource.com>).
>
> So if we aren't going to fix it to DTRT WRT writing zero to a target then I
> think we can go with the current variant and not change to ignoring any
> word with a zero byte in it.
OK.
BTW it would be interesting to see how real hardware behaves in this
regard. I seem to recall that X-Gene was ignoring 0 writes too.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-12 10:41 ` Stefano Stabellini
@ 2015-10-12 11:00 ` Julien Grall
2015-10-12 11:07 ` Stefano Stabellini
0 siblings, 1 reply; 34+ messages in thread
From: Julien Grall @ 2015-10-12 11:00 UTC (permalink / raw)
To: Stefano Stabellini, Ian Campbell; +Cc: xen-devel
On 12/10/15 11:41, Stefano Stabellini wrote:
> On Thu, 8 Oct 2015, Ian Campbell wrote:
>>> If the concern is the behavior is changed, I'm happy to rework this code
>>> to keep exactly the same behavior. I.e any 32-bit write containing
>>> a 0 byte will be ignored. This is not optimal but at least I'm not
>>> opening the pandora box of fixing every single error in the code touch
>>> by this series.
>>
>> I'm okay with the new behaviour, I think Stefano was willing to tolerate it
>> (based on <alpine.DEB.2.02.1510081220190.1179@kaball.uk.xensource.com>).
>>
>> So if we aren't going to fix it to DTRT WRT writing zero to a target then I
>> think we can go with the current variant and not change to ignoring any
>> word with a zero byte in it.
>
> OK.
>
> BTW it would be interesting to see how real hardware behaves in this
> regard. I seem to recall that X-Gene was ignoring 0 writes too.
What do you mean by 0 writes? Is it the write ignored if one byte is 0?
Although, it's really depend if you try on a target register where all
the byte correspond to an implemented field or not.
Based on the spec (4.3.12 ARM IHI 0048B.b), a field corresponding to an
interrupt not implemented (i.e not wired) is RAZ/WI.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-12 11:00 ` Julien Grall
@ 2015-10-12 11:07 ` Stefano Stabellini
2015-10-12 11:28 ` Julien Grall
0 siblings, 1 reply; 34+ messages in thread
From: Stefano Stabellini @ 2015-10-12 11:07 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, Ian Campbell, Stefano Stabellini
On Mon, 12 Oct 2015, Julien Grall wrote:
> On 12/10/15 11:41, Stefano Stabellini wrote:
> > On Thu, 8 Oct 2015, Ian Campbell wrote:
> >>> If the concern is the behavior is changed, I'm happy to rework this code
> >>> to keep exactly the same behavior. I.e any 32-bit write containing
> >>> a 0 byte will be ignored. This is not optimal but at least I'm not
> >>> opening the pandora box of fixing every single error in the code touch
> >>> by this series.
> >>
> >> I'm okay with the new behaviour, I think Stefano was willing to tolerate it
> >> (based on <alpine.DEB.2.02.1510081220190.1179@kaball.uk.xensource.com>).
> >>
> >> So if we aren't going to fix it to DTRT WRT writing zero to a target then I
> >> think we can go with the current variant and not change to ignoring any
> >> word with a zero byte in it.
> >
> > OK.
> >
> > BTW it would be interesting to see how real hardware behaves in this
> > regard. I seem to recall that X-Gene was ignoring 0 writes too.
>
> What do you mean by 0 writes? Is it the write ignored if one byte is 0?
Yes, for either 32bit or 8bit writes.
> Although, it's really depend if you try on a target register where all
> the byte correspond to an implemented field or not.
I am talking about implemented fields
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-12 11:07 ` Stefano Stabellini
@ 2015-10-12 11:28 ` Julien Grall
0 siblings, 0 replies; 34+ messages in thread
From: Julien Grall @ 2015-10-12 11:28 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Campbell
On 12/10/15 12:07, Stefano Stabellini wrote:
> On Mon, 12 Oct 2015, Julien Grall wrote:
>> On 12/10/15 11:41, Stefano Stabellini wrote:
>>> On Thu, 8 Oct 2015, Ian Campbell wrote:
>>>>> If the concern is the behavior is changed, I'm happy to rework this code
>>>>> to keep exactly the same behavior. I.e any 32-bit write containing
>>>>> a 0 byte will be ignored. This is not optimal but at least I'm not
>>>>> opening the pandora box of fixing every single error in the code touch
>>>>> by this series.
>>>>
>>>> I'm okay with the new behaviour, I think Stefano was willing to tolerate it
>>>> (based on <alpine.DEB.2.02.1510081220190.1179@kaball.uk.xensource.com>).
>>>>
>>>> So if we aren't going to fix it to DTRT WRT writing zero to a target then I
>>>> think we can go with the current variant and not change to ignoring any
>>>> word with a zero byte in it.
>>>
>>> OK.
>>>
>>> BTW it would be interesting to see how real hardware behaves in this
>>> regard. I seem to recall that X-Gene was ignoring 0 writes too.
>>
>> What do you mean by 0 writes? Is it the write ignored if one byte is 0?
>
> Yes, for either 32bit or 8bit writes.
I just gave a try on X-gene and any write is store even if it contains
a 0 byte:
(XEN) ITARGET56 = 0x1010101
(XEN) w 0x2020200 r 0x2020200
(XEN) w 0x4040004 r 0x4040004
(XEN) w 0x8000808 r 0x8000808
(XEN) w 0x101010 r 0x101010
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 5841e59..743c9eb 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -279,6 +279,32 @@ static void __init gicv2_dist_init(void)
for ( i = 32; i < nr_lines; i += 4 )
writel_gicd(cpumask, GICD_ITARGETSR + (i / 4) * 4);
+ for ( i = 32; i < nr_lines; i += 4 )
+ {
+ int n = i / 4;
+ int j = 0;
+ unsigned int reg = GICD_ITARGETSR + (i / 4) * 4;
+
+ printk("ITARGET%u = 0x%x\n", n, readl_gicd(reg));
+
+ for ( j = 0; j < 4; j++ )
+ {
+ uint32_t tmp = 1 << (j + 1);
+
+ tmp |= tmp << 8;
+ tmp |= tmp << 16;
+
+ /* Mask the field j */
+ tmp &= ~((0xff) << (j * 8));
+
+ writel_gicd(tmp, reg);
+
+ printk("\t w 0x%x r 0x%x\n", tmp, readl_gicd(reg));
+ }
+ }
+
+ while (1);
+
/* Default priority for global interrupts */
for ( i = 32; i < nr_lines; i += 4 )
writel_gicd(GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 |
Regards,
--
Julien Grall
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 8/9] xen/arm: vgic: Introduce helpers to extract/update/clear/set vGIC register ...
2015-10-07 14:41 [PATCH v3 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (6 preceding siblings ...)
2015-10-07 14:41 ` [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank Julien Grall
@ 2015-10-07 14:41 ` Julien Grall
2015-10-07 14:41 ` [PATCH v3 9/9] xen/arm: vgic-v3: Support 32-bit access for 64-bit registers Julien Grall
2015-10-08 10:44 ` [PATCH v3 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
9 siblings, 0 replies; 34+ messages in thread
From: Julien Grall @ 2015-10-07 14:41 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, ian.campbell, stefano.stabellini
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.
Finally, take the opportunity to fix the coding style in section we are
modifying.
Signed-off-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v3:
- Add Ian's acked-by
Changes in v2:
- Rename read/write helpers to extract/update
- Add a 's' to clearbit/setbit because we update multiple bits
Changes in v1:
- Make use of the new helpers in the vGICv2 code
- Drop vgic_byte_*
- Use unsigned long rather than uint64_t to optimize the code
on ARM. (~about 40% less instruction per helpers).
---
xen/arch/arm/vgic-v2.c | 89 +++++++++++++++++++++-------------
xen/arch/arm/vgic-v3.c | 116 ++++++++++++++++++++++++++++++---------------
xen/include/asm-arm/vgic.h | 111 +++++++++++++++++++++++++++++++++++++++----
3 files changed, 235 insertions(+), 81 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index bc03785..690cf2e 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -156,24 +156,31 @@ static int vgic_v2_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_extract(v->domain->arch.vgic.ctlr, info);
vgic_unlock(v);
return 1;
case GICD_TYPER:
+ {
+ uint32_t typer;
+
if ( dabt.size != DABT_WORD ) goto bad_width;
/* No secure world support for guests. */
vgic_lock(v);
- *r = ( ((v->domain->max_vcpus - 1) << GICD_TYPE_CPUS_SHIFT) )
+ typer = ((v->domain->max_vcpus - 1) << GICD_TYPE_CPUS_SHIFT)
| DIV_ROUND_UP(v->domain->arch.vgic.nr_spis, 32);
vgic_unlock(v);
+
+ *r = vgic_reg32_extract(typer, info);
+
return 1;
+ }
case GICD_IIDR:
if ( dabt.size != DABT_WORD ) goto bad_width;
/*
* XXX Do we need a JEP106 manufacturer ID?
* Just use the physical h/w value for now
*/
- *r = 0x0000043b;
+ *r = vgic_reg32_extract(0x0000043b, info);
return 1;
/* Implementation defined -- read as zero */
@@ -189,7 +196,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISENABLER, DABT_WORD);
if ( rank == NULL) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
- *r = rank->ienable;
+ *r = vgic_reg32_extract(rank->ienable, info);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -198,7 +205,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICENABLER, DABT_WORD);
if ( rank == NULL) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
- *r = rank->ienable;
+ *r = vgic_reg32_extract(rank->ienable, info);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -213,37 +220,53 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
goto read_as_zero;
case GICD_ITARGETSR ... GICD_ITARGETSRN:
+ {
+ uint32_t itargetsr;
+
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD);
if ( rank == NULL) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
- *r = vgic_fetch_itargetsr(rank, gicd_reg - GICD_ITARGETSR);
- if ( dabt.size == DABT_BYTE )
- *r = vgic_byte_read(*r, gicd_reg);
+ itargetsr = vgic_fetch_itargetsr(rank, gicd_reg - GICD_ITARGETSR);
vgic_unlock_rank(v, rank, flags);
+ *r = vgic_reg32_extract(itargetsr, info);
+
return 1;
+ }
case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
+ {
+ uint32_t ipriorityr;
+
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR, DABT_WORD);
- if ( rank == NULL) goto read_as_zero;
+ if ( rank == NULL ) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
- *r = rank->ipriorityr[REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR,
- DABT_WORD)];
- if ( dabt.size == DABT_BYTE )
- *r = vgic_byte_read(*r, gicd_reg);
+ ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8,
+ gicd_reg - GICD_IPRIORITYR,
+ DABT_WORD)];
vgic_unlock_rank(v, rank, flags);
+ *r = vgic_reg32_extract(ipriorityr, 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, gicd_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, gicd_reg - GICD_ICFGR, DABT_WORD)];
+ icfgr = rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR, DABT_WORD)];
vgic_unlock_rank(v, rank, flags);
+
+ *r = vgic_reg32_extract(icfgr, info);
+
return 1;
+ }
case GICD_NSACR ... GICD_NSACRN:
/* We do not implement security extensions for guests, read zero */
@@ -353,7 +376,8 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
if ( dabt.size != DABT_WORD ) goto bad_width;
/* Ignore all but the enable bit */
vgic_lock(v);
- v->domain->arch.vgic.ctlr = r & GICD_CTL_ENABLE;
+ vgic_reg32_update(&v->domain->arch.vgic.ctlr, r, info);
+ v->domain->arch.vgic.ctlr &= GICD_CTL_ENABLE;
vgic_unlock(v);
return 1;
@@ -377,8 +401,8 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
if ( rank == NULL) goto write_ignore;
vgic_lock_rank(v, rank, flags);
tr = rank->ienable;
- rank->ienable |= r;
- vgic_enable_irqs(v, r & (~tr), rank->index);
+ vgic_reg32_setbits(&rank->ienable, r, info);
+ vgic_enable_irqs(v, (rank->ienable) & (~tr), rank->index);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -388,8 +412,8 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
if ( rank == NULL) goto write_ignore;
vgic_lock_rank(v, rank, flags);
tr = rank->ienable;
- rank->ienable &= ~r;
- vgic_disable_irqs(v, r & tr, rank->index);
+ vgic_reg32_clearbits(&rank->ienable, r, info);
+ vgic_disable_irqs(v, (~rank->ienable) & tr, rank->index);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -433,13 +457,8 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD);
if ( rank == NULL) goto write_ignore;
vgic_lock_rank(v, rank, flags);
- if ( dabt.size == DABT_WORD )
- itargetsr = r;
- else
- {
- itargetsr = vgic_fetch_itargetsr(rank, gicd_reg - GICD_ITARGETSR);
- vgic_byte_write(&itargetsr, r, gicd_reg);
- }
+ itargetsr = vgic_fetch_itargetsr(rank, gicd_reg - GICD_ITARGETSR);
+ vgic_reg32_update(&itargetsr, r, info);
vgic_store_itargetsr(v->domain, rank, gicd_reg - GICD_ITARGETSR,
itargetsr);
vgic_unlock_rank(v, rank, flags);
@@ -447,18 +466,20 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
}
case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
+ {
+ uint32_t *ipriorityr;
+
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR, DABT_WORD);
if ( rank == NULL) goto write_ignore;
vgic_lock_rank(v, rank, flags);
- if ( dabt.size == DABT_WORD )
- rank->ipriorityr[REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR,
- DABT_WORD)] = r;
- else
- vgic_byte_write(&rank->ipriorityr[REG_RANK_INDEX(8,
- gicd_reg - GICD_IPRIORITYR, DABT_WORD)], r, gicd_reg);
+ ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8,
+ gicd_reg - GICD_IPRIORITYR,
+ DABT_WORD)];
+ vgic_reg32_update(ipriorityr, r, info);
vgic_unlock_rank(v, rank, flags);
return 1;
+ }
case GICD_ICFGR: /* SGIs */
goto write_ignore_32;
@@ -470,7 +491,9 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
rank = vgic_rank_offset(v, 2, gicd_reg - GICD_ICFGR, DABT_WORD);
if ( rank == NULL) goto write_ignore;
vgic_lock_rank(v, rank, flags);
- rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR, DABT_WORD)] = r;
+ vgic_reg32_update(&rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR,
+ DABT_WORD)],
+ r, info);
vgic_unlock_rank(v, rank, flags);
return 1;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 2aab77b..a4b8412 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -162,7 +162,6 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
register_t *r)
{
struct hsr_dabt dabt = info->dabt;
- uint64_t aff;
switch ( gicr_reg )
{
@@ -171,21 +170,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_extract(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_extract(typer, info);
return 1;
+ }
case GICR_STATUSR:
/* Not implemented */
goto read_as_zero_32;
@@ -214,7 +219,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_extract(GICR_SYNCR_NOT_BUSY, info);
return 1;
case GICR_MOVLPIR:
/* WO Read as zero */
@@ -224,22 +229,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_extract(GICV3_GICR_PIDR0, info);
return 1;
case GICR_PIDR1:
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICR_PIDR1;
+ *r = vgic_reg32_extract(GICV3_GICR_PIDR1, info);
return 1;
case GICR_PIDR2:
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICR_PIDR2;
+ *r = vgic_reg32_extract(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_extract(GICV3_GICR_PIDR4, info);
return 1;
case GICR_PIDR5 ... GICR_PIDR7:
/* Reserved0 */
@@ -360,7 +365,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_extract(rank->ienable, info);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICENABLER ... GICD_ICENABLERN:
@@ -368,7 +373,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_extract(rank->ienable, info);
vgic_unlock_rank(v, rank, flags);
return 1;
/* Read the pending status of an IRQ via GICD/GICR is not supported */
@@ -382,25 +387,38 @@ 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 ipriorityr;
+
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->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
- DABT_WORD)];
- if ( dabt.size == DABT_BYTE )
- *r = vgic_byte_read(*r, reg);
+ ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
+ DABT_WORD)];
vgic_unlock_rank(v, rank, flags);
+
+ *r = vgic_reg32_extract(ipriorityr, 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_extract(icfgr, info);
+
return 1;
+ }
default:
printk(XENLOG_G_ERR
"%pv: %s: unhandled read r%d offset %#08x\n",
@@ -439,8 +457,8 @@ 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_enable_irqs(v, r & (~tr), rank->index);
+ vgic_reg32_setbits(&rank->ienable, r, info);
+ vgic_enable_irqs(v, (rank->ienable) & (~tr), rank->index);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICENABLER ... GICD_ICENABLERN:
@@ -449,8 +467,8 @@ 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_disable_irqs(v, r & tr, rank->index);
+ vgic_reg32_clearbits(&rank->ienable, r, info);
+ vgic_disable_irqs(v, (~rank->ienable) & tr, rank->index);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ISPENDR ... GICD_ISPENDRN:
@@ -482,16 +500,16 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
return 0;
case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
+ {
+ uint32_t *ipriorityr;
+
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 write_ignore;
vgic_lock_rank(v, rank, flags);
- if ( dabt.size == DABT_WORD )
- rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
- DABT_WORD)] = r;
- else
- vgic_byte_write(&rank->ipriorityr[REG_RANK_INDEX(8,
- reg - GICD_IPRIORITYR, DABT_WORD)], r, reg);
+ ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
+ DABT_WORD)];
+ vgic_reg32_update(ipriorityr, r, info);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICFGR: /* Restricted to configure SGIs */
@@ -503,9 +521,13 @@ 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_update(&rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR,
+ DABT_WORD)],
+ r, info);
vgic_unlock_rank(v, rank, flags);
return 1;
+ }
+
default:
printk(XENLOG_G_ERR
"%pv: %s: unhandled write r%d=%"PRIregister" offset %#08x\n",
@@ -719,7 +741,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_extract(v->domain->arch.vgic.ctlr, info);
vgic_unlock(v);
return 1;
case GICD_TYPER:
@@ -734,13 +756,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));
- *r |= (irq_bits - 1) << GICD_TYPE_ID_BITS_SHIFT;
+ typer |= (irq_bits - 1) << GICD_TYPE_ID_BITS_SHIFT;
+
+ *r = vgic_reg32_extract(typer, info);
return 1;
}
@@ -752,7 +777,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_extract(GICV3_GICD_IIDR_VAL, info);
return 1;
case 0x020 ... 0x03c:
case 0xc000 ... 0xffcc:
@@ -775,14 +800,22 @@ 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 = vgic_fetch_irouter(rank, gicd_reg - GICD_IROUTER);
+ irouter = vgic_fetch_irouter(rank, gicd_reg - GICD_IROUTER);
vgic_unlock_rank(v, rank, flags);
+
+ *r = vgic_reg64_extract(irouter, info);
+
return 1;
+ }
+
case GICD_NSACR ... GICD_NSACRN:
/* We do not implement security extensions for guests, read zero */
goto read_as_zero_32;
@@ -798,17 +831,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_extract(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_extract(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_extract(GICV3_GICD_PIDR2, info);
return 1;
case GICD_PIDR3:
/* GICv3 identification value. Manufacturer/Customer defined */
@@ -816,7 +849,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_extract(GICV3_GICD_PIDR4, info);
return 1;
case GICD_PIDR5 ... GICD_PIDR7:
/* Reserved0 */
@@ -928,14 +961,21 @@ 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:
+ {
+ 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 write_ignore;
vgic_lock_rank(v, rank, flags);
- vgic_store_irouter(v->domain, rank, gicd_reg - GICD_IROUTER, r);
+ irouter = vgic_fetch_irouter(rank, gicd_reg - GICD_IROUTER);
+ vgic_reg64_update(&irouter, r, info);
+ vgic_store_irouter(v->domain, rank, gicd_reg - GICD_IROUTER, irouter);
vgic_unlock_rank(v, rank, flags);
return 1;
+ }
+
case GICD_NSACR ... GICD_NSACRN:
/* We do not implement security extensions for guests, write ignore */
goto write_ignore_32;
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 4df7755..c5fad43 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
{
@@ -166,26 +167,116 @@ static inline int REG_RANK_NR(int b, uint32_t n)
}
}
-static inline uint32_t vgic_byte_read(uint32_t val, int offset)
+#define VGIC_REG_MASK(size) ((~0UL) >> (BITS_PER_LONG - ((1 << (size)) * 8)))
+
+/*
+ * 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_extract(unsigned long reg,
+ unsigned int offset,
+ enum dabt_size size)
+{
+ reg >>= 8 * offset;
+ reg &= VGIC_REG_MASK(size);
+
+ return reg;
+}
+
+static inline void vgic_reg_update(unsigned long *reg, register_t val,
+ unsigned int offset,
+ enum dabt_size size)
{
- int byte = offset & 0x3;
+ unsigned long mask = VGIC_REG_MASK(size);
+ int shift = offset * 8;
- val = val >> (8*byte);
- val &= 0x000000ff;
+ *reg &= ~(mask << shift);
+ *reg |= ((unsigned long)val & mask) << shift;
+}
+
+static inline void vgic_reg_setbits(unsigned long *reg, register_t bits,
+ unsigned int offset,
+ enum dabt_size size)
+{
+ unsigned long mask = VGIC_REG_MASK(size);
+ int shift = offset * 8;
- return val;
+ *reg |= ((unsigned long)bits & mask) << shift;
}
-static inline void vgic_byte_write(uint32_t *reg, uint32_t var, int offset)
+static inline void vgic_reg_clearbits(unsigned long *reg, register_t bits,
+ unsigned int offset,
+ enum dabt_size size)
{
- int byte = offset & 0x3;
+ unsigned long mask = VGIC_REG_MASK(size);
+ int shift = offset * 8;
- var &= 0xff;
+ *reg &= ~(((unsigned long)bits & mask) << shift);
+}
- *reg &= ~(0xff << (8*byte));
- *reg |= (var << (8*byte));
+/* N-bit register helpers */
+#define VGIC_REG_HELPERS(sz, offmask) \
+static inline register_t vgic_reg##sz##_extract(uint##sz##_t reg, \
+ const mmio_info_t *info)\
+{ \
+ return vgic_reg_extract(reg, info->gpa & offmask, \
+ info->dabt.size); \
+} \
+ \
+static inline void vgic_reg##sz##_update(uint##sz##_t *reg, \
+ register_t val, \
+ const mmio_info_t *info) \
+{ \
+ unsigned long tmp = *reg; \
+ \
+ vgic_reg_update(&tmp, val, info->gpa & offmask, \
+ info->dabt.size); \
+ \
+ *reg = tmp; \
+} \
+ \
+static inline void vgic_reg##sz##_setbits(uint##sz##_t *reg, \
+ register_t bits, \
+ const mmio_info_t *info) \
+{ \
+ unsigned long tmp = *reg; \
+ \
+ vgic_reg_setbits(&tmp, bits, info->gpa & offmask, \
+ info->dabt.size); \
+ \
+ *reg = tmp; \
+} \
+ \
+static inline void vgic_reg##sz##_clearbits(uint##sz##_t *reg, \
+ register_t bits, \
+ const mmio_info_t *info) \
+{ \
+ unsigned long tmp = *reg; \
+ \
+ vgic_reg_clearbits(&tmp, bits, info->gpa & offmask, \
+ info->dabt.size); \
+ \
+ *reg = tmp; \
}
+/*
+ * 64 bits registers are only supported on platform with 64-bit long.
+ * This is also allow us to optimize the 32 bit case by using
+ * unsigned long rather than uint64_t
+ */
+#if BITS_PER_LONG == 64
+VGIC_REG_HELPERS(64, 0x7);
+#endif
+VGIC_REG_HELPERS(32, 0x3);
+
+#undef VGIC_REG_HELPERS
+
enum gic_sgi_mode;
/*
--
2.1.4
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 9/9] xen/arm: vgic-v3: Support 32-bit access for 64-bit registers
2015-10-07 14:41 [PATCH v3 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (7 preceding siblings ...)
2015-10-07 14:41 ` [PATCH v3 8/9] xen/arm: vgic: Introduce helpers to extract/update/clear/set vGIC register Julien Grall
@ 2015-10-07 14:41 ` Julien Grall
2015-10-08 10:44 ` [PATCH v3 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
9 siblings, 0 replies; 34+ messages in thread
From: Julien Grall @ 2015-10-07 14:41 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, ian.campbell, stefano.stabellini
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>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
This is technically fixing boot of FreeBSD ARM64 guest with GICv3.
AFAICT, Linux is not using 32-bit access in the GICv3 code expect
for the ITS (which we don't support yet).
So this patch is a good candidate for Xen 4.6.1. Although this patch is
heavily depend on previous patches. It may be possible to shuffle
and move the "opmitization" patches towards the end. I haven't yet
done that because I feel this series makes more sense in the current
order.
Also, I haven't move vgic_reg64_check_access in vgic.h because there
is no usage in this series outside of vgic-v3.c and the helpers is
GICv3 oriented.
Changes in v2:
- Add Ian's acked-by
Changes in v1:
- Support 32bit access on the most significant word of
GICR_TYPER
---
xen/arch/arm/vgic-v3.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index a4b8412..e540085 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -157,6 +157,15 @@ static void vgic_store_irouter(struct domain *d, struct vgic_irq_rank *rank,
rank->vcpu[offset] = new_vcpu->vcpu_id;
}
+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 int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
uint32_t gicr_reg,
register_t *r)
@@ -173,10 +182,11 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
*r = vgic_reg32_extract(GICV3_GICR_IIDR_VAL, info);
return 1;
case GICR_TYPER:
+ case GICR_TYPER + 4:
{
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 |
@@ -262,7 +272,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;
@@ -338,7 +348,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:
@@ -803,7 +813,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;
@@ -878,7 +888,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;
@@ -964,7 +974,7 @@ static int vgic_v3_distr_mmio_write(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 write_ignore;
@@ -1023,7 +1033,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:
--
2.1.4
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register
2015-10-07 14:41 [PATCH v3 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (8 preceding siblings ...)
2015-10-07 14:41 ` [PATCH v3 9/9] xen/arm: vgic-v3: Support 32-bit access for 64-bit registers Julien Grall
@ 2015-10-08 10:44 ` Julien Grall
2015-10-08 11:46 ` Ian Campbell
9 siblings, 1 reply; 34+ messages in thread
From: Julien Grall @ 2015-10-08 10:44 UTC (permalink / raw)
To: ian.campbell; +Cc: xen-devel, stefano.stabellini
Hi Ian,
It looks like there is only comment on patch #7. Would it be possible to
apply #1-#6? I will resend the last 3 patches after I agree with Stefano
about the chosen behavior in #7.
Regards,
On 07/10/15 15:41, Julien Grall wrote:
> Hi all,
>
> This series aims to fixc the 32-bit access on 64-bit register. Some guest
> OS such as FreeBSD and Linux (only in ITS) use 32-bit access and will crash
> at boot time.
>
> I took the opportunity to go further and optimize the way Xen is storing
> registers such as GICD_IPRIORITYR, GICD_ITARGETSR and GICD_IROUTER.
>
> For all changes see in each patch.
>
> A branch has been pushed based on the latest staging:
>
> git://xenbits.xen.org/people/julieng/xen-unstable.git branch gicv3-32bit-v2
>
>
> Julien Grall (9):
> xen/arm: io: remove mmio_check_t typedef
> xen/arm: io: Extend write/read handler to pass the register in
> parameter
> xen/arm: io: Support sign-extension for every read access
> xen/arm: vgic: ctlr stores a 32-bit hardware register so use uint32_t
> xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank
> xen/arm: vgic: Introduce a new field to store the rank index and use
> it
> xen/arm: vgic: Optimize the way to store the target vCPU in the rank
> xen/arm: vgic: Introduce helpers to extract/update/clear/set vGIC
> register ...
> xen/arm: vgic-v3: Support 32-bit access for 64-bit registers
>
> xen/arch/arm/io.c | 34 ++++-
> xen/arch/arm/vgic-v2.c | 308 +++++++++++++++++++------------------
> xen/arch/arm/vgic-v3.c | 353 +++++++++++++++++++++++--------------------
> xen/arch/arm/vgic.c | 70 +++++++--
> xen/arch/arm/vuart.c | 20 ++-
> xen/include/asm-arm/domain.h | 2 +-
> xen/include/asm-arm/mmio.h | 7 +-
> xen/include/asm-arm/vgic.h | 151 ++++++++++++++----
> 8 files changed, 582 insertions(+), 363 deletions(-)
>
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread