* [PATCH v2 1/9] xen/arm: io: remove mmio_check_t typedef
2015-10-05 12:51 [PATCH v2 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
@ 2015-10-05 12:51 ` Julien Grall
2015-10-05 12:51 ` [PATCH v2 2/9] xen/arm: io: Extend write/read handler to pass the register in parameter Julien Grall
` (7 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Julien Grall @ 2015-10-05 12:51 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, 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@linaro.org>
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] 20+ messages in thread* [PATCH v2 2/9] xen/arm: io: Extend write/read handler to pass the register in parameter
2015-10-05 12:51 [PATCH v2 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
2015-10-05 12:51 ` [PATCH v2 1/9] xen/arm: io: remove mmio_check_t typedef Julien Grall
@ 2015-10-05 12:51 ` Julien Grall
2015-10-05 12:58 ` Julien Grall
2015-10-05 12:51 ` [PATCH v2 3/9] xen/arm: io: Support sign-extension for every read access Julien Grall
` (6 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Julien Grall @ 2015-10-05 12:51 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>
---
Ian, I haven't kept my Acked-by because of code clash while rebasing
on top of "xen/arm: io: Extend write/read handler to pass private data"
[1].
Changes in v2:
- Rebase on top of "xen/arm: io: Extend write/read handler to
pass private data"
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] 20+ messages in thread* Re: [PATCH v2 2/9] xen/arm: io: Extend write/read handler to pass the register in parameter
2015-10-05 12:51 ` [PATCH v2 2/9] xen/arm: io: Extend write/read handler to pass the register in parameter Julien Grall
@ 2015-10-05 12:58 ` Julien Grall
2015-10-06 13:44 ` Ian Campbell
0 siblings, 1 reply; 20+ messages in thread
From: Julien Grall @ 2015-10-05 12:58 UTC (permalink / raw)
To: xen-devel; +Cc: ian.campbell, stefano.stabellini
On 05/10/15 13:51, Julien Grall wrote:
> 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>
>
> ---
>
> Ian, I haven't kept my Acked-by because of code clash while rebasing
> on top of "xen/arm: io: Extend write/read handler to pass private data"
> [1].
Hmmm, this doesn't match what I did (i.e keeping the acked-by from Ian).
The conflict with "xen/arm: io: Extend write/read handler to pass
private data" should not be controversial. Let me know if you disagree.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/9] xen/arm: io: Extend write/read handler to pass the register in parameter
2015-10-05 12:58 ` Julien Grall
@ 2015-10-06 13:44 ` Ian Campbell
0 siblings, 0 replies; 20+ messages in thread
From: Ian Campbell @ 2015-10-06 13:44 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: stefano.stabellini
On Mon, 2015-10-05 at 13:58 +0100, Julien Grall wrote:
> On 05/10/15 13:51, Julien Grall wrote:
> > 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>
> >
> > ---
> >
> > Ian, I haven't kept my Acked-by because of code clash while rebasing
> > on top of "xen/arm: io: Extend write/read handler to pass private data"
> > [1].
>
> Hmmm, this doesn't match what I did (i.e keeping the acked-by from Ian).
>
> The conflict with "xen/arm: io: Extend write/read handler to pass
> private data" should not be controversial. Let me know if you disagree.
It's fine.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 3/9] xen/arm: io: Support sign-extension for every read access
2015-10-05 12:51 [PATCH v2 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
2015-10-05 12:51 ` [PATCH v2 1/9] xen/arm: io: remove mmio_check_t typedef Julien Grall
2015-10-05 12:51 ` [PATCH v2 2/9] xen/arm: io: Extend write/read handler to pass the register in parameter Julien Grall
@ 2015-10-05 12:51 ` Julien Grall
2015-10-06 13:47 ` Ian Campbell
2015-10-05 12:51 ` [PATCH v2 4/9] xen/arm: vgic: ctlr stores a 32-bit hardware register so use uint32_t Julien Grall
` (5 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Julien Grall @ 2015-10-05 12:51 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>
---
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..75f7c82 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 bit
+ * unused in the register.
+ */
+ 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] 20+ messages in thread* Re: [PATCH v2 3/9] xen/arm: io: Support sign-extension for every read access
2015-10-05 12:51 ` [PATCH v2 3/9] xen/arm: io: Support sign-extension for every read access Julien Grall
@ 2015-10-06 13:47 ` Ian Campbell
2015-10-06 13:55 ` Julien Grall
0 siblings, 1 reply; 20+ messages in thread
From: Ian Campbell @ 2015-10-06 13:47 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: stefano.stabellini
On Mon, 2015-10-05 at 13:51 +0100, Julien Grall wrote:
> [...]
> + /*
> + * Sign extend if required.
> + * Note that we expect the read handler to have zeroed the bit
> + * unused in the register.
You were (correctly) going to change this to
Note that we expect the read handler to have zeroed the bits outside the
requested access size.
Apart from that (which I can fix on commit):
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 3/9] xen/arm: io: Support sign-extension for every read access
2015-10-06 13:47 ` Ian Campbell
@ 2015-10-06 13:55 ` Julien Grall
0 siblings, 0 replies; 20+ messages in thread
From: Julien Grall @ 2015-10-06 13:55 UTC (permalink / raw)
To: Ian Campbell, xen-devel; +Cc: stefano.stabellini
On 06/10/15 14:47, Ian Campbell wrote:
> On Mon, 2015-10-05 at 13:51 +0100, Julien Grall wrote:
>> [...]
>> + /*
>> + * Sign extend if required.
>> + * Note that we expect the read handler to have zeroed the bit
>> + * unused in the register.
>
> You were (correctly) going to change this to
> Note that we expect the read handler to have zeroed the bits outside the
> requested access size.
Right and I forgot to update the patch :/.
> Apart from that (which I can fix on commit):
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Thank you!
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 4/9] xen/arm: vgic: ctlr stores a 32-bit hardware register so use uint32_t
2015-10-05 12:51 [PATCH v2 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (2 preceding siblings ...)
2015-10-05 12:51 ` [PATCH v2 3/9] xen/arm: io: Support sign-extension for every read access Julien Grall
@ 2015-10-05 12:51 ` Julien Grall
2015-10-05 12:51 ` [PATCH v2 5/9] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank Julien Grall
` (4 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Julien Grall @ 2015-10-05 12:51 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] 20+ messages in thread* [PATCH v2 5/9] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank
2015-10-05 12:51 [PATCH v2 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (3 preceding siblings ...)
2015-10-05 12:51 ` [PATCH v2 4/9] xen/arm: vgic: ctlr stores a 32-bit hardware register so use uint32_t Julien Grall
@ 2015-10-05 12:51 ` Julien Grall
2015-10-06 13:49 ` Ian Campbell
2015-10-05 12:51 ` [PATCH v2 6/9] xen/arm: vgic: Introduce a new field to store the rank index and use it Julien Grall
` (3 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Julien Grall @ 2015-10-05 12:51 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>
---
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 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] 20+ messages in thread* Re: [PATCH v2 5/9] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank
2015-10-05 12:51 ` [PATCH v2 5/9] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank Julien Grall
@ 2015-10-06 13:49 ` Ian Campbell
0 siblings, 0 replies; 20+ messages in thread
From: Ian Campbell @ 2015-10-06 13:49 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: stefano.stabellini
On Mon, 2015-10-05 at 13:51 +0100, Julien Grall wrote:
> 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>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 6/9] xen/arm: vgic: Introduce a new field to store the rank index and use it
2015-10-05 12:51 [PATCH v2 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (4 preceding siblings ...)
2015-10-05 12:51 ` [PATCH v2 5/9] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank Julien Grall
@ 2015-10-05 12:51 ` Julien Grall
2015-10-06 13:51 ` Ian Campbell
2015-10-05 12:51 ` [PATCH v2 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank Julien Grall
` (2 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Julien Grall @ 2015-10-05 12:51 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 everytime.
For now, use it when enabling/disabling the vIRQs rather than a formula
which is not obvious to understand.
Also drop the comments which where wrong because a shift by DABT_WORD
will not give the IRQ number but the index of the register.
---
Changes in v3:
- 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] 20+ messages in thread* Re: [PATCH v2 6/9] xen/arm: vgic: Introduce a new field to store the rank index and use it
2015-10-05 12:51 ` [PATCH v2 6/9] xen/arm: vgic: Introduce a new field to store the rank index and use it Julien Grall
@ 2015-10-06 13:51 ` Ian Campbell
2015-10-06 13:56 ` Julien Grall
0 siblings, 1 reply; 20+ messages in thread
From: Ian Campbell @ 2015-10-06 13:51 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: stefano.stabellini
On Mon, 2015-10-05 at 13:51 +0100, Julien Grall wrote:
> Having in hand the index for the rank is very handy to avoid computing
> it everytime.
"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 where wrong because a shift by DABT_WORD
"were" instead of "where".
> will not give the IRQ number but the index of the register.
You forgot your S-o-b.
Apart from those:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/9] xen/arm: vgic: Introduce a new field to store the rank index and use it
2015-10-06 13:51 ` Ian Campbell
@ 2015-10-06 13:56 ` Julien Grall
0 siblings, 0 replies; 20+ messages in thread
From: Julien Grall @ 2015-10-06 13:56 UTC (permalink / raw)
To: Ian Campbell, xen-devel; +Cc: stefano.stabellini
On 06/10/15 14:51, Ian Campbell wrote:
> On Mon, 2015-10-05 at 13:51 +0100, Julien Grall wrote:
>> Having in hand the index for the rank is very handy to avoid computing
>> it everytime.
>
> "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 where wrong because a shift by DABT_WORD
>
> "were" instead of "where".
>
>> will not give the IRQ number but the index of the register.
>
> You forgot your S-o-b.
oops right.
Signed-off-by: Julien Grall <julien.grall@citrix.com>
> Apart from those:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-05 12:51 [PATCH v2 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (5 preceding siblings ...)
2015-10-05 12:51 ` [PATCH v2 6/9] xen/arm: vgic: Introduce a new field to store the rank index and use it Julien Grall
@ 2015-10-05 12:51 ` Julien Grall
2015-10-06 13:59 ` Ian Campbell
2015-10-05 12:51 ` [PATCH v2 8/9] xen/arm: vgic: Introduce helpers to extract/update/clear/set vGIC register Julien Grall
2015-10-05 12:51 ` [PATCH v2 9/9] xen/arm: vgic-v3: Support 32-bit access for 64-bit registers Julien Grall
8 siblings, 1 reply; 20+ messages in thread
From: Julien Grall @ 2015-10-05 12:51 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 shrinked 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. This is fine 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 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..3a4133c 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 ignore the interrupt.
+ *
+ * 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] 20+ messages in thread* Re: [PATCH v2 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-05 12:51 ` [PATCH v2 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank Julien Grall
@ 2015-10-06 13:59 ` Ian Campbell
2015-10-06 14:18 ` Julien Grall
0 siblings, 1 reply; 20+ messages in thread
From: Ian Campbell @ 2015-10-06 13:59 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: stefano.stabellini
On Mon, 2015-10-05 at 13:51 +0100, Julien Grall wrote:
> 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 shrinked down from 320 bytes to 92 bytes. This is saving about 228
"shrunk"
> 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. This is fine 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.
I'd prefer if instead of "this is fine..." this said something like "We
think this is likely to be OK because...(stuff)...".
It is not the case that the spec actually says what is implemented here is
acceptable and I think that needs to be clear here along with why we
currently think it is something we can get away with, for future reference.
> + /*
> + * From the spec (see 8.9.13 in IHI 0069A), any write with an
> + * invalid vCPU will lead to ignore the interrupt.
"will lead to the interrupt being ignored"
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-10-06 13:59 ` Ian Campbell
@ 2015-10-06 14:18 ` Julien Grall
0 siblings, 0 replies; 20+ messages in thread
From: Julien Grall @ 2015-10-06 14:18 UTC (permalink / raw)
To: Ian Campbell, xen-devel; +Cc: stefano.stabellini
On 06/10/15 14:59, Ian Campbell wrote:
> On Mon, 2015-10-05 at 13:51 +0100, Julien Grall wrote:
>> 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 shrinked down from 320 bytes to 92 bytes. This is saving about 228
>
> "shrunk"
>
>> 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. This is fine 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.
>
> I'd prefer if instead of "this is fine..." this said something like "We
> think this is likely to be OK because...(stuff)...".
Will do. I will resend the whole series fixing the different error you
mentioned within the series and add your various acked.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 8/9] xen/arm: vgic: Introduce helpers to extract/update/clear/set vGIC register ...
2015-10-05 12:51 [PATCH v2 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (6 preceding siblings ...)
2015-10-05 12:51 ` [PATCH v2 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank Julien Grall
@ 2015-10-05 12:51 ` Julien Grall
2015-10-06 14:01 ` Ian Campbell
2015-10-05 12:51 ` [PATCH v2 9/9] xen/arm: vgic-v3: Support 32-bit access for 64-bit registers Julien Grall
8 siblings, 1 reply; 20+ messages in thread
From: Julien Grall @ 2015-10-05 12:51 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>
---
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 3a4133c..27d49a8 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] 20+ messages in thread* Re: [PATCH v2 8/9] xen/arm: vgic: Introduce helpers to extract/update/clear/set vGIC register ...
2015-10-05 12:51 ` [PATCH v2 8/9] xen/arm: vgic: Introduce helpers to extract/update/clear/set vGIC register Julien Grall
@ 2015-10-06 14:01 ` Ian Campbell
0 siblings, 0 replies; 20+ messages in thread
From: Ian Campbell @ 2015-10-06 14:01 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: stefano.stabellini
On Mon, 2015-10-05 at 13:51 +0100, Julien Grall wrote:
> 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>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 9/9] xen/arm: vgic-v3: Support 32-bit access for 64-bit registers
2015-10-05 12:51 [PATCH v2 0/9] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (7 preceding siblings ...)
2015-10-05 12:51 ` [PATCH v2 8/9] xen/arm: vgic: Introduce helpers to extract/update/clear/set vGIC register Julien Grall
@ 2015-10-05 12:51 ` Julien Grall
8 siblings, 0 replies; 20+ messages in thread
From: Julien Grall @ 2015-10-05 12:51 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 27d49a8..c3c5cdc 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] 20+ messages in thread