* [PATCH v1 1/8] xen/arm: io: remove mmio_check_t typedef
2015-09-25 14:51 [PATCH v1 0/8] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
@ 2015-09-25 14:51 ` Julien Grall
2015-09-25 16:33 ` Ian Campbell
2015-09-25 14:51 ` [PATCH v1 2/8] xen/arm: io: Extend write/read handler to pass the register in parameter Julien Grall
` (6 subsequent siblings)
7 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2015-09-25 14:51 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, vijay.kilari, manish.jaggi, Julien Grall,
stefano.stabellini, Vijaya.Kumar
From: Julien Grall <julien.grall@linaro.org>
This typedef is a left-over of the previous MMIO emulation
implementation.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
Changes in v2:
- 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 0160f09..7278bce 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);
typedef int (*mmio_write_t)(struct vcpu *v, mmio_info_t *info);
-typedef int (*mmio_check_t)(struct vcpu *v, paddr_t addr);
struct mmio_handler_ops {
mmio_read_t read_handler;
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v1 2/8] xen/arm: io: Extend write/read handler to pass the register in parameter
2015-09-25 14:51 [PATCH v1 0/8] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
2015-09-25 14:51 ` [PATCH v1 1/8] xen/arm: io: remove mmio_check_t typedef Julien Grall
@ 2015-09-25 14:51 ` Julien Grall
2015-09-25 16:36 ` Ian Campbell
2015-09-25 14:51 ` [PATCH v1 3/8] xen/arm: Support sign-extension for every read access Julien Grall
` (5 subsequent siblings)
7 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2015-09-25 14:51 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, vijay.kilari, manish.jaggi, Julien Grall,
Julien Grall, stefano.stabellini, Vijaya.Kumar
From: Julien Grall <julien.grall@linaro.org>
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,
therfore only a plain value is given.
Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
Changes in v2:
- Patch added
---
xen/arch/arm/io.c | 15 ++++---
xen/arch/arm/vgic-v2.c | 46 ++++++++++----------
xen/arch/arm/vgic-v3.c | 105 +++++++++++++++++++++------------------------
xen/arch/arm/vuart.c | 16 +++----
xen/include/asm-arm/mmio.h | 4 +-
5 files changed, 88 insertions(+), 98 deletions(-)
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 8e55d49..32b2194 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 *mmio_handler;
const struct io_handler *io_handlers = &v->domain->arch.io_handlers;
+ 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 < io_handlers->num_entries; i++ )
{
@@ -36,14 +39,16 @@ int handle_mmio(mmio_info_t *info)
if ( (info->gpa >= mmio_handler->addr) &&
(info->gpa < (mmio_handler->addr + mmio_handler->size)) )
- {
- return info->dabt.write ?
- mmio_handler->mmio_handler_ops->write_handler(v, info) :
- mmio_handler->mmio_handler_ops->read_handler(v, info);
- }
+ goto found;
}
return 0;
+
+found:
+ if ( info->dabt.write )
+ return mmio_handler->mmio_handler_ops->write_handler(v, info, *r);
+ else
+ return mmio_handler->mmio_handler_ops->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 fa71598..3d0ce1d 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -50,11 +50,10 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t vbase)
vgic_v2_hw.vbase = vbase;
}
-static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
+static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
+ 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;
int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
unsigned long flags;
@@ -247,11 +246,10 @@ static int vgic_v2_to_sgi(struct vcpu *v, register_t sgir)
return vgic_to_sgi(v, sgir, sgi_mode, virq, &target);
}
-static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
+static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
+ 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;
int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
uint32_t tr;
@@ -265,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;
@@ -289,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;
@@ -304,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;
@@ -317,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:
@@ -360,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;
@@ -408,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;
@@ -425,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;
@@ -435,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 */
@@ -475,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 f1c482d..94f1a5c 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;
@@ -634,7 +626,8 @@ static inline struct vcpu *get_vcpu_from_rdist(paddr_t gpa,
return d->vcpu[vcpu_id];
}
-static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info)
+static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info,
+ register_t *r)
{
uint32_t offset;
@@ -645,9 +638,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",
@@ -656,7 +649,8 @@ static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info)
return 0;
}
-static int vgic_v3_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info)
+static int vgic_v3_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info,
+ register_t r)
{
uint32_t offset;
@@ -667,9 +661,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",
@@ -678,11 +672,10 @@ static int vgic_v3_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info)
return 0;
}
-static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
+static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
+ 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;
int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
@@ -745,7 +738,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;
@@ -835,11 +828,10 @@ read_as_zero:
return 1;
}
-static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
+static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
+ 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;
uint64_t new_irouter, old_irouter;
@@ -855,7 +847,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;
@@ -901,7 +893,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;
@@ -910,7 +903,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,
@@ -923,7 +916,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
@@ -969,14 +962,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 d9f4249..5b8409c 100644
--- a/xen/arch/arm/vuart.c
+++ b/xen/arch/arm/vuart.c
@@ -45,8 +45,8 @@
#define domain_has_vuart(d) ((d)->arch.vuart.info != NULL)
-static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info);
-static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info);
+static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info, register_t *r);
+static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info, register_t r);
static const struct mmio_handler_ops vuart_mmio_handler = {
.read_handler = vuart_mmio_read,
@@ -105,12 +105,9 @@ 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)
+static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info, register_t *r)
{
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);
@@ -125,19 +122,16 @@ static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info)
return 1;
}
-static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info)
+static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info, register_t r)
{
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 7278bce..b511334 100644
--- a/xen/include/asm-arm/mmio.h
+++ b/xen/include/asm-arm/mmio.h
@@ -32,8 +32,8 @@ typedef struct
paddr_t gpa;
} mmio_info_t;
-typedef int (*mmio_read_t)(struct vcpu *v, mmio_info_t *info);
-typedef int (*mmio_write_t)(struct vcpu *v, mmio_info_t *info);
+typedef int (*mmio_read_t)(struct vcpu *v, mmio_info_t *info, register_t *r);
+typedef int (*mmio_write_t)(struct vcpu *v, mmio_info_t *info, register_t r);
struct mmio_handler_ops {
mmio_read_t read_handler;
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v1 2/8] xen/arm: io: Extend write/read handler to pass the register in parameter
2015-09-25 14:51 ` [PATCH v1 2/8] xen/arm: io: Extend write/read handler to pass the register in parameter Julien Grall
@ 2015-09-25 16:36 ` Ian Campbell
2015-09-28 16:35 ` Julien Grall
0 siblings, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2015-09-25 16:36 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Vijaya.Kumar, Julien Grall, stefano.stabellini, manish.jaggi,
vijay.kilari
On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
> From: Julien Grall <julien.grall@linaro.org>
>
> 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,
> therfore only a plain value is given.
"therefore"
>
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com.
Is it the case that read handlers today only update *r on success? Is that
a useful thing to mandate and/or enforce?
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 2/8] xen/arm: io: Extend write/read handler to pass the register in parameter
2015-09-25 16:36 ` Ian Campbell
@ 2015-09-28 16:35 ` Julien Grall
2015-09-29 10:51 ` Ian Campbell
0 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2015-09-28 16:35 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Vijaya.Kumar, Julien Grall, stefano.stabellini, manish.jaggi,
vijay.kilari
Hi Ian,
On 25/09/15 17:36, Ian Campbell wrote:
> On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
>> From: Julien Grall <julien.grall@linaro.org>
>>
>> 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,
>> therfore only a plain value is given.
>
> "therefore"
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com.
>
> Is it the case that read handlers today only update *r on success?
Yes, each read handler only update the register mentioned in dabt.reg.
> Is that
> a useful thing to mandate and/or enforce?
What do you mean?
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 2/8] xen/arm: io: Extend write/read handler to pass the register in parameter
2015-09-28 16:35 ` Julien Grall
@ 2015-09-29 10:51 ` Ian Campbell
2015-09-29 11:00 ` Julien Grall
0 siblings, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2015-09-29 10:51 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Vijaya.Kumar, Julien Grall, stefano.stabellini, manish.jaggi,
vijay.kilari
On Mon, 2015-09-28 at 17:35 +0100, Julien Grall wrote:
> Hi Ian,
>
> On 25/09/15 17:36, Ian Campbell wrote:
> > On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
> > > From: Julien Grall <julien.grall@linaro.org>
> > >
> > > 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,
> > > therfore only a plain value is given.
> >
> > "therefore"
> > >
> > > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> >
> > Acked-by: Ian Campbell <ian.campbell@citrix.com.
> >
> > Is it the case that read handlers today only update *r on success?
>
> Yes, each read handler only update the register mentioned in dabt.reg.
I meant that they do not touch it on failure.
> > Is that
> > a useful thing to mandate and/or enforce?
>
> What do you mean?
>
> Regards,
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 2/8] xen/arm: io: Extend write/read handler to pass the register in parameter
2015-09-29 10:51 ` Ian Campbell
@ 2015-09-29 11:00 ` Julien Grall
2015-09-29 11:09 ` Ian Campbell
0 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2015-09-29 11:00 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Vijaya.Kumar, Julien Grall, stefano.stabellini, manish.jaggi,
vijay.kilari
Hi Ian,
On 29/09/15 11:51, Ian Campbell wrote:
> On Mon, 2015-09-28 at 17:35 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 25/09/15 17:36, Ian Campbell wrote:
>>> On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
>>>> From: Julien Grall <julien.grall@linaro.org>
>>>>
>>>> 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,
>>>> therfore only a plain value is given.
>>>
>>> "therefore"
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>>>
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com.
>>>
>>> Is it the case that read handlers today only update *r on success?
>>
>> Yes, each read handler only update the register mentioned in dabt.reg.
>
> I meant that they do not touch it on failure.
Sorry, somehow I read as "Is only the register pointed by *r updated by
the read handlers"?
AFAICT, it's always the case. When the handler returns 0 (i.e failure),
a data abort will be injected to the domain unless the handler decide to
crash the domain.
I don't think it's necessary to mandate/enforce as the worst that can
happen is a guest register is corrupted. Although, the guest will
unlikely get continue after a data abort on MMIO we emulate.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 2/8] xen/arm: io: Extend write/read handler to pass the register in parameter
2015-09-29 11:00 ` Julien Grall
@ 2015-09-29 11:09 ` Ian Campbell
0 siblings, 0 replies; 43+ messages in thread
From: Ian Campbell @ 2015-09-29 11:09 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Vijaya.Kumar, Julien Grall, stefano.stabellini, manish.jaggi,
vijay.kilari
On Tue, 2015-09-29 at 12:00 +0100, Julien Grall wrote:
> Hi Ian,
>
> On 29/09/15 11:51, Ian Campbell wrote:
> > On Mon, 2015-09-28 at 17:35 +0100, Julien Grall wrote:
> > > Hi Ian,
> > >
> > > On 25/09/15 17:36, Ian Campbell wrote:
> > > > On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
> > > > > From: Julien Grall <julien.grall@linaro.org>
> > > > >
> > > > > 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,
> > > > > therfore only a plain value is given.
> > > >
> > > > "therefore"
> > > > >
> > > > > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > > >
> > > > Acked-by: Ian Campbell <ian.campbell@citrix.com.
> > > >
> > > > Is it the case that read handlers today only update *r on success?
> > >
> > > Yes, each read handler only update the register mentioned in
> > > dabt.reg.
> >
> > I meant that they do not touch it on failure.
>
> Sorry, somehow I read as "Is only the register pointed by *r updated by
> the read handlers"?
Yes, my original comment was ambiguously parsable that way, sorry.
> AFAICT, it's always the case. When the handler returns 0 (i.e failure),
> a data abort will be injected to the domain unless the handler decide to
> crash the domain.
The important thing is whether any path sets *r and then returns 0, I
think.
> I don't think it's necessary to mandate/enforce as the worst that can
> happen is a guest register is corrupted. Although, the guest will
> unlikely get continue after a data abort on MMIO we emulate.
True.
>
> Regards,
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v1 3/8] xen/arm: Support sign-extension for every read access
2015-09-25 14:51 [PATCH v1 0/8] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
2015-09-25 14:51 ` [PATCH v1 1/8] xen/arm: io: remove mmio_check_t typedef Julien Grall
2015-09-25 14:51 ` [PATCH v1 2/8] xen/arm: io: Extend write/read handler to pass the register in parameter Julien Grall
@ 2015-09-25 14:51 ` Julien Grall
2015-09-25 16:44 ` Ian Campbell
2015-09-25 14:51 ` [PATCH v1 4/8] xen/arm: vgic: ctlr stores a 32-bit hardware register so use uint32_t Julien Grall
` (4 subsequent siblings)
7 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2015-09-25 14:51 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, vijay.kilari, manish.jaggi, Julien Grall,
stefano.stabellini, Vijaya.Kumar
The guest may try to load data from the emulated MMIO region using
instruction with Sign-Extension (i.e ldrs*). This can happen for any
access smaller than the register size (byte/half-word for aarch32,
byte/half-word/word for aarch64).
The support of sign-extension was limited for byte access in vGIG
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>
---
I was thinking to completely drop the sign-extension support in Xen as
it will be very unlikely to use ldrs* instruction to access MMIO.
Although the code is fairly small, so it doesn't harm to keep it
generically.
Changes in v2:
- Patch added
---
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 32b2194..e1b03a2 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -23,6 +23,32 @@
#include <asm/current.h>
#include <asm/mmio.h>
+static int handle_read(mmio_read_t read_cb, struct vcpu *v,
+ mmio_info_t *info, register_t *r)
+{
+ uint8_t size = (1 << info->dabt.size) * 8;
+
+ if ( !read_cb(v, info, r) )
+ return 0;
+
+ /*
+ * Extend the bit sign 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 as the same size as
+ * an unsigned long or order to keep the 32bit some 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 +74,8 @@ found:
if ( info->dabt.write )
return mmio_handler->mmio_handler_ops->write_handler(v, info, *r);
else
- return mmio_handler->mmio_handler_ops->read_handler(v, info, r);
+ return handle_read(mmio_handler->mmio_handler_ops->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 3d0ce1d..47f9da9 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 94f1a5c..c013200 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:
@@ -1062,7 +1062,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] 43+ messages in thread* Re: [PATCH v1 3/8] xen/arm: Support sign-extension for every read access
2015-09-25 14:51 ` [PATCH v1 3/8] xen/arm: Support sign-extension for every read access Julien Grall
@ 2015-09-25 16:44 ` Ian Campbell
2015-09-28 16:42 ` Julien Grall
2015-09-28 18:22 ` Julien Grall
0 siblings, 2 replies; 43+ messages in thread
From: Ian Campbell @ 2015-09-25 16:44 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
> The guest may try to load data from the emulated MMIO region using
> instruction with Sign-Extension (i.e ldrs*). This can happen for any
> access smaller than the register size (byte/half-word for aarch32,
> byte/half-word/word for aarch64).
>
> The support of sign-extension was limited for byte access in vGIG
"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>
>
> ---
>
> I was thinking to completely drop the sign-extension support in Xen as
> it will be very unlikely to use ldrs* instruction to access MMIO.
> Although the code is fairly small, so it doesn't harm to keep it
> generically.
Yes, I think we should keep it, since we don't control what instructions
are used to access MMIO, however unlikely...
> Changes in v2:
> - Patch added
> ---
> 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 32b2194..e1b03a2 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -23,6 +23,32 @@
> #include <asm/current.h>
> #include <asm/mmio.h>
>
> +static int handle_read(mmio_read_t read_cb, struct vcpu *v,
> + mmio_info_t *info, register_t *r)
> +{
> + uint8_t size = (1 << info->dabt.size) * 8;
> +
> + if ( !read_cb(v, info, r) )
> + return 0;
> +
> + /*
> + * Extend the bit sign if required.
I think you meant s/bit sign/sign bit/ but more correct would be "Sign
extend if required".
> + * Note that we expect the read handler to have zeroed the bit
> + * unused in the register.
"... to have zeroed the unused bits in the register".
But I think "unused" is a bit misleading, you mean the ones outside the
requested access size, those bits are still "used" IYSWIM. I can't think of
a terse term for "outside the requested access size I'm afraid.
Did you confirm that all existing handlers meet this requirement? Perhaps
an ASSERT would be handy?
> + */
> + if ( info->dabt.sign && (*r & (1UL << (size - 1)) ))
> + {
> + /*
> + * We are relying on register_t as the same size as
> + * an unsigned long or order to keep the 32bit some smaller
"order"? I'm not sure what you meant here so I can't suggest an
alternative.
> + */
> + BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long));
> + *r |= (~0UL) << size;
I think here and in the initial if you need to be careful of the case where
size == 32 (on arm32) or == 64 (on arm64), since a shift by >= the size of
the variable is, I think, undefined behaviour.
It's also a waste of time sign extending in that case.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 3/8] xen/arm: Support sign-extension for every read access
2015-09-25 16:44 ` Ian Campbell
@ 2015-09-28 16:42 ` Julien Grall
2015-09-29 11:01 ` Ian Campbell
2015-09-28 18:22 ` Julien Grall
1 sibling, 1 reply; 43+ messages in thread
From: Julien Grall @ 2015-09-28 16:42 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On 25/09/15 17:44, Ian Campbell wrote:
>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>> index 32b2194..e1b03a2 100644
>> --- a/xen/arch/arm/io.c
>> +++ b/xen/arch/arm/io.c
>> @@ -23,6 +23,32 @@
>> #include <asm/current.h>
>> #include <asm/mmio.h>
>>
>> +static int handle_read(mmio_read_t read_cb, struct vcpu *v,
>> + mmio_info_t *info, register_t *r)
>> +{
>> + uint8_t size = (1 << info->dabt.size) * 8;
>> +
>> + if ( !read_cb(v, info, r) )
>> + return 0;
>> +
>> + /*
>> + * Extend the bit sign if required.
>
> I think you meant s/bit sign/sign bit/ but more correct would be "Sign
> extend if required".
Will fix it.
>
>> + * Note that we expect the read handler to have zeroed the bit
>> + * unused in the register.
>
> "... to have zeroed the unused bits in the register".
>
> But I think "unused" is a bit misleading, you mean the ones outside the
> requested access size, those bits are still "used" IYSWIM. I can't think of
> a terse term for "outside the requested access size I'm afraid.
>
> Did you confirm that all existing handlers meet this requirement? Perhaps
> an ASSERT would be handy?
>
>> + */
>> + if ( info->dabt.sign && (*r & (1UL << (size - 1)) ))
>> + {
>> + /*
>> + * We are relying on register_t as the same size as
>> + * an unsigned long or order to keep the 32bit some smaller
>
> "order"? I'm not sure what you meant here so I can't suggest an
> alternative.
hmmm... the end of the comment is badly written :/. I wanted to say
"We are relying on register_t using the same size as and unsigned long
in order to keep the 32-bit assembly code smaller"
>
>> + */
>> + BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long));
>> + *r |= (~0UL) << size;
>
> I think here and in the initial if you need to be careful of the case where
> size == 32 (on arm32) or == 64 (on arm64), since a shift by >= the size of
> the variable is, I think, undefined behaviour.
>
> It's also a waste of time sign extending in that case.
>From the spec, dabt.sign is only set when smaller size than the register
size. For instance for ARMv7 spec (ARM DDI 0406C.b page B3-1433):
"SSE, Syndrome sign extend. For a byte or halfword load operation,
indicates whether
the data item must be sign extended. [...] For all other operations this
bit is 0."
So we don't have to worry about waste of time and undefined behavior.
Note that mention it in the commit message. Maybe it wasn't clear enough?
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 3/8] xen/arm: Support sign-extension for every read access
2015-09-28 16:42 ` Julien Grall
@ 2015-09-29 11:01 ` Ian Campbell
2015-09-29 11:07 ` Julien Grall
0 siblings, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2015-09-29 11:01 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On Mon, 2015-09-28 at 17:42 +0100, Julien Grall wrote:
> > > + */
> > > + if ( info->dabt.sign && (*r & (1UL << (size - 1)) ))
> > > + {
> > > + /*
> > > + * We are relying on register_t as the same size as
> > > + * an unsigned long or order to keep the 32bit some smaller
> >
> > "order"? I'm not sure what you meant here so I can't suggest an
> > alternative.
>
> hmmm... the end of the comment is badly written :/. I wanted to say
> "We are relying on register_t using the same size as and unsigned long
> in order to keep the 32-bit assembly code smaller"
"as an unsigned long", then it works.
>
> >
> > > + */
> > > + BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long));
> > > + *r |= (~0UL) << size;
> >
> > I think here and in the initial if you need to be careful of the case
> > where
> > size == 32 (on arm32) or == 64 (on arm64), since a shift by >= the size
> > of
> > the variable is, I think, undefined behaviour.
> >
> > It's also a waste of time sign extending in that case.
>
> From the spec, dabt.sign is only set when smaller size than the register
> size. For instance for ARMv7 spec (ARM DDI 0406C.b page B3-1433):
>
> "SSE, Syndrome sign extend. For a byte or halfword load operation,
> indicates whether
> the data item must be sign extended. [...] For all other operations this
> bit is 0."
>
> So we don't have to worry about waste of time and undefined behavior.
> Note that mention it in the commit message. Maybe it wasn't clear enough?
It's a fairly oblique mention in the commit message, so I think it indeed
wasn't explicit enough.
I'm unsure if we need to worry about the fact that the compiler does't know
about that bit of the spec, so it might assume that size could be >=32 or
64 and do something unhelpful. Probably not.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 3/8] xen/arm: Support sign-extension for every read access
2015-09-29 11:01 ` Ian Campbell
@ 2015-09-29 11:07 ` Julien Grall
0 siblings, 0 replies; 43+ messages in thread
From: Julien Grall @ 2015-09-29 11:07 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On 29/09/15 12:01, Ian Campbell wrote:
> On Mon, 2015-09-28 at 17:42 +0100, Julien Grall wrote:
>
>>>> + */
>>>> + if ( info->dabt.sign && (*r & (1UL << (size - 1)) ))
>>>> + {
>>>> + /*
>>>> + * We are relying on register_t as the same size as
>>>> + * an unsigned long or order to keep the 32bit some smaller
>>>
>>> "order"? I'm not sure what you meant here so I can't suggest an
>>> alternative.
>>
>> hmmm... the end of the comment is badly written :/. I wanted to say
>> "We are relying on register_t using the same size as and unsigned long
>> in order to keep the 32-bit assembly code smaller"
>
> "as an unsigned long", then it works.
>
>>
>>>
>>>> + */
>>>> + BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long));
>>>> + *r |= (~0UL) << size;
>>>
>>> I think here and in the initial if you need to be careful of the case
>>> where
>>> size == 32 (on arm32) or == 64 (on arm64), since a shift by >= the size
>>> of
>>> the variable is, I think, undefined behaviour.
>>>
>>> It's also a waste of time sign extending in that case.
>>
>> From the spec, dabt.sign is only set when smaller size than the register
>> size. For instance for ARMv7 spec (ARM DDI 0406C.b page B3-1433):
>>
>> "SSE, Syndrome sign extend. For a byte or halfword load operation,
>> indicates whether
>> the data item must be sign extended. [...] For all other operations this
>> bit is 0."
>>
>> So we don't have to worry about waste of time and undefined behavior.
>> Note that mention it in the commit message. Maybe it wasn't clear enough?
>
> It's a fairly oblique mention in the commit message, so I think it indeed
> wasn't explicit enough.
I will update the commit message and mention the spec.
>
> I'm unsure if we need to worry about the fact that the compiler does't know
> about that bit of the spec, so it might assume that size could be >=32 or
> 64 and do something unhelpful. Probably not.
We have quite a few place in Xen using the idiom foo >> bar and foo <<
bar when bar is a parameter to a function or local variable.
So I don't think we need to worry about this...
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 3/8] xen/arm: Support sign-extension for every read access
2015-09-25 16:44 ` Ian Campbell
2015-09-28 16:42 ` Julien Grall
@ 2015-09-28 18:22 ` Julien Grall
2015-09-29 11:03 ` Ian Campbell
1 sibling, 1 reply; 43+ messages in thread
From: Julien Grall @ 2015-09-28 18:22 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On 25/09/15 17:44, Ian Campbell wrote:
> On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
> I think you meant s/bit sign/sign bit/ but more correct would be "Sign
> extend if required".
>
>> + * Note that we expect the read handler to have zeroed the bit
>> + * unused in the register.
>
> "... to have zeroed the unused bits in the register".
>
> But I think "unused" is a bit misleading, you mean the ones outside the
> requested access size, those bits are still "used" IYSWIM. I can't think of
> a terse term for "outside the requested access size I'm afraid.
I will switch to "Note that we expect the read handler to have zeroed
the bits outside the requested access size."
>
> Did you confirm that all existing handlers meet this requirement?
Yes, we always do *r in the existing handlers.
> Perhaps an ASSERT would be handy?
What about:
ASSERT((*r & ((~0UL) >> (BITS_PER_LONG - size))) == 0)
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 3/8] xen/arm: Support sign-extension for every read access
2015-09-28 18:22 ` Julien Grall
@ 2015-09-29 11:03 ` Ian Campbell
2015-09-29 11:13 ` Julien Grall
0 siblings, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2015-09-29 11:03 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On Mon, 2015-09-28 at 19:22 +0100, Julien Grall wrote:
> On 25/09/15 17:44, Ian Campbell wrote:
> > On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
> > I think you meant s/bit sign/sign bit/ but more correct would be "Sign
> > extend if required".
> >
> > > + * Note that we expect the read handler to have zeroed the bit
> > > + * unused in the register.
> >
> > "... to have zeroed the unused bits in the register".
> >
> > But I think "unused" is a bit misleading, you mean the ones outside the
> > requested access size, those bits are still "used" IYSWIM. I can't
> > think of
> > a terse term for "outside the requested access size I'm afraid.
>
> I will switch to "Note that we expect the read handler to have zeroed
> the bits outside the requested access size."
>
> >
> > Did you confirm that all existing handlers meet this requirement?
>
> Yes, we always do *r in the existing handlers.
>
> > Perhaps an ASSERT would be handy?
>
> What about:
>
> ASSERT((*r & ((~0UL) >> (BITS_PER_LONG - size))) == 0)
Is that not backwards, e.g for size = 8, then
~0UL >> (32-8) == 0xffffffff >> 24 == 0xff, so you end up checking that the
lowest byte is zero, but that's the one you expected to change.
Or is it, couldn't we be updating a byte in the middle of the word?
Probably figuring out the correct assertion is more hassle than it is
worth..
>
> Regards,
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 3/8] xen/arm: Support sign-extension for every read access
2015-09-29 11:03 ` Ian Campbell
@ 2015-09-29 11:13 ` Julien Grall
2015-09-29 13:13 ` Ian Campbell
0 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2015-09-29 11:13 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On 29/09/15 12:03, Ian Campbell wrote:
> On Mon, 2015-09-28 at 19:22 +0100, Julien Grall wrote:
>> On 25/09/15 17:44, Ian Campbell wrote:
>>> On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
>>> I think you meant s/bit sign/sign bit/ but more correct would be "Sign
>>> extend if required".
>>>
>>>> + * Note that we expect the read handler to have zeroed the bit
>>>> + * unused in the register.
>>>
>>> "... to have zeroed the unused bits in the register".
>>>
>>> But I think "unused" is a bit misleading, you mean the ones outside the
>>> requested access size, those bits are still "used" IYSWIM. I can't
>>> think of
>>> a terse term for "outside the requested access size I'm afraid.
>>
>> I will switch to "Note that we expect the read handler to have zeroed
>> the bits outside the requested access size."
>>
>>>
>>> Did you confirm that all existing handlers meet this requirement?
>>
>> Yes, we always do *r in the existing handlers.
>>
>>> Perhaps an ASSERT would be handy?
>>
>> What about:
>>
>> ASSERT((*r & ((~0UL) >> (BITS_PER_LONG - size))) == 0)
>
> Is that not backwards, e.g for size = 8, then
>
> ~0UL >> (32-8) == 0xffffffff >> 24 == 0xff, so you end up checking that the
> lowest byte is zero, but that's the one you expected to change.
I just mess up the computation. It should be
(*r & ((~OUL) << size)) but the behavior is undefined by the spec when
the size = 32 (on arm32) and size = 64 (on arm64) (see 6.5.7 [1])
> Or is it, couldn't we be updating a byte in the middle of the word?
*r is a register, the byte/half-word/word... are always living in the
lowest bit of the register.
> Probably figuring out the correct assertion is more hassle than it is
> worth..
I would rather skip it.
[1] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf
--
Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 3/8] xen/arm: Support sign-extension for every read access
2015-09-29 11:13 ` Julien Grall
@ 2015-09-29 13:13 ` Ian Campbell
2015-09-29 13:16 ` Julien Grall
0 siblings, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2015-09-29 13:13 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On Tue, 2015-09-29 at 12:13 +0100, Julien Grall wrote:
> > Or is it, couldn't we be updating a byte in the middle of the word?
>
> *r is a register, the byte/half-word/word... are always living in the
> lowest bit of the register.
So just to check:
ldrs* do support e.g odd offsets, but given a 32 bit MMIO region which
reads 0xAABBCCDD then when reading from offset 1 at this point *r contains
0x000000CC already (and not 0x0000C00) and sign extending the lowest byte
is therefore correct.
Right?
>
> > Probably figuring out the correct assertion is more hassle than it is
> > worth..
>
> I would rather skip it.
>
> [1] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 3/8] xen/arm: Support sign-extension for every read access
2015-09-29 13:13 ` Ian Campbell
@ 2015-09-29 13:16 ` Julien Grall
0 siblings, 0 replies; 43+ messages in thread
From: Julien Grall @ 2015-09-29 13:16 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On 29/09/15 14:13, Ian Campbell wrote:
> On Tue, 2015-09-29 at 12:13 +0100, Julien Grall wrote:
>>> Or is it, couldn't we be updating a byte in the middle of the word?
>>
>> *r is a register, the byte/half-word/word... are always living in the
>> lowest bit of the register.
>
> So just to check:
>
> ldrs* do support e.g odd offsets, but given a 32 bit MMIO region which
> reads 0xAABBCCDD then when reading from offset 1 at this point *r contains
> 0x000000CC already (and not 0x0000C00) and sign extending the lowest byte
> is therefore correct.
>
> Right?
Correct. For more details see the pseudo-code to implement the different
ldrs* instruction (for instance A8.8.86 in ARM DDI 0406C.b).
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v1 4/8] xen/arm: vgic: ctlr stores a 32-bit hardware register so use uint32_t
2015-09-25 14:51 [PATCH v1 0/8] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (2 preceding siblings ...)
2015-09-25 14:51 ` [PATCH v1 3/8] xen/arm: Support sign-extension for every read access Julien Grall
@ 2015-09-25 14:51 ` Julien Grall
2015-09-25 16:45 ` Ian Campbell
2015-09-25 14:51 ` [PATCH v1 5/8] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank Julien Grall
` (3 subsequent siblings)
7 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2015-09-25 14:51 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, vijay.kilari, manish.jaggi, Julien Grall,
stefano.stabellini, Vijaya.Kumar
Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
Changes in v2:
- 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 c3f5a95..1a5bfd7 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -90,7 +90,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] 43+ messages in thread* [PATCH v1 5/8] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank
2015-09-25 14:51 [PATCH v1 0/8] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (3 preceding siblings ...)
2015-09-25 14:51 ` [PATCH v1 4/8] xen/arm: vgic: ctlr stores a 32-bit hardware register so use uint32_t Julien Grall
@ 2015-09-25 14:51 ` Julien Grall
2015-09-28 10:50 ` Ian Campbell
2015-09-28 10:52 ` Ian Campbell
2015-09-25 14:51 ` [PATCH v1 6/8] xen/arm: vgic: Optimize the way to store the target vCPU " Julien Grall
` (2 subsequent siblings)
7 siblings, 2 replies; 43+ messages in thread
From: Julien Grall @ 2015-09-25 14:51 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, vijay.kilari, manish.jaggi, Julien Grall,
stefano.stabellini, Vijaya.Kumar
Xen is currently directly storing the value of register GICD_IPRIORITYR
in the rank. This makes emulation of the register access very simple
but makes the code to get the priority for a given IRQ more complex.
While the priority of an IRQ is retrieved everytime an IRQ is injected
to the guest, the access to 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 priority for
every interrupt in the rank. This will make the code to get the priority
very quick. The emulation code will now have to generate the GICD_PRIORITYR
register for read access and split it to store in a convenient way.
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:
- Patch added
---
xen/arch/arm/vgic-v2.c | 24 ++++++++++++++----------
xen/arch/arm/vgic-v3.c | 27 ++++++++++++++++-----------
xen/arch/arm/vgic.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/vgic.h | 18 +++++++++++++++++-
4 files changed, 93 insertions(+), 22 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 47f9da9..23d1982 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)];
+ /* Recreate the IPRIORITYR register */
+ *r = vgic_generate_ipriorityr(rank, gicd_reg - GICD_IPRIORITYR);
if ( dabt.size == DABT_BYTE )
*r = vgic_byte_read(*r, gicd_reg);
vgic_unlock_rank(v, rank, flags);
@@ -400,18 +400,25 @@ 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->ipriority[REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR,
- DABT_WORD)] = r;
+ ipriorityr = r;
else
- vgic_byte_write(&rank->ipriority[REG_RANK_INDEX(8,
- gicd_reg - GICD_IPRIORITYR, DABT_WORD)], r, gicd_reg);
+ {
+ ipriorityr = vgic_generate_ipriorityr(rank,
+ gicd_reg - GICD_IPRIORITYR);
+ vgic_byte_write(&ipriorityr, r, gicd_reg);
+ }
+ vgic_store_ipriorityr(rank, gicd_reg - GICD_IPRIORITYR, ipriorityr);
vgic_unlock_rank(v, rank, flags);
return 1;
+ }
case GICD_ICFGR: /* SGIs */
goto write_ignore_32;
@@ -516,14 +523,11 @@ static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
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;
+ return rank->priority[irq & INTERRUPT_RANK_MASK];
}
static int vgic_v2_vcpu_init(struct vcpu *v)
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index c013200..2787507 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)];
+ /* Recreate the IPRIORITYR register */
+ *r = vgic_generate_ipriorityr(rank, reg - GICD_IPRIORITYR);
if ( dabt.size == DABT_BYTE )
*r = vgic_byte_read(*r, reg);
vgic_unlock_rank(v, rank, flags);
@@ -430,18 +430,26 @@ 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;
+ 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;
+ ipriorityr = r;
else
- vgic_byte_write(&rank->ipriority[REG_RANK_INDEX(8,
- reg - GICD_IPRIORITYR, DABT_WORD)], r, reg);
+ {
+ ipriorityr = vgic_generate_ipriorityr(rank, reg - GICD_IPRIORITYR);
+ vgic_byte_write(&ipriorityr, r, reg);
+ }
+ vgic_store_ipriorityr(rank, reg - GICD_IPRIORITYR, ipriorityr);
vgic_unlock_rank(v, rank, flags);
return 1;
+ }
+
case GICD_ICFGR: /* Restricted to configure SGIs */
goto write_ignore_32;
case GICD_ICFGR + 4 ... GICD_ICFGRN: /* PPI + SPIs */
@@ -1057,14 +1065,11 @@ static const struct mmio_handler_ops vgic_distr_mmio_handler = {
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;
+ return rank->priority[irq & INTERRUPT_RANK_MASK];
}
static int vgic_v3_vcpu_init(struct vcpu *v)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index a6835a8..50ad360 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -61,6 +61,52 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
return vgic_get_rank(v, rank);
}
+#define NR_PRIORITY_PER_REG 4U
+#define NR_BIT_PER_PRIORITY 8U
+
+/*
+ * Generate the associated IPRIORITYR register based on an offset in the rank.
+ * Note the offset will be round down to match a real HW register.
+ */
+uint32_t vgic_generate_ipriorityr(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_PRIORITY_PER_REG - 1);
+
+ for ( i = 0; i < NR_PRIORITY_PER_REG; i++, offset++ )
+ reg |= rank->priority[offset] << (i * NR_BIT_PER_PRIORITY);
+
+ return reg;
+}
+
+/*
+ * Store a IPRIORITYR register in a convenient way.
+ * Note the offset will be round down to match a real HW register.
+ */
+void vgic_store_ipriorityr(struct vgic_irq_rank *rank,
+ unsigned int offset,
+ uint32_t reg)
+{
+ unsigned int i;
+
+ ASSERT(spin_is_locked(&rank->lock));
+
+ offset &= INTERRUPT_RANK_MASK;
+ offset &= ~(NR_PRIORITY_PER_REG - 1);
+
+ for ( i = 0; i < NR_PRIORITY_PER_REG; i++, offset++ )
+ {
+ rank->priority[offset] = reg & ((1 << NR_BIT_PER_PRIORITY) - 1);
+ reg >>= NR_BIT_PER_PRIORITY;
+ }
+}
+
static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq)
{
INIT_LIST_HEAD(&p->inflight);
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 354c0d4..ce9970e 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -82,12 +82,21 @@ 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];
+
+ /*
+ * It's more convenient to store one priority per interrupt than
+ * the register IPRIORITYR itself
+ */
+ uint8_t priority[32];
+
union {
struct {
uint32_t itargets[8];
@@ -244,6 +253,13 @@ void vgic_v3_setup_hw(paddr_t dbase,
uint32_t rdist_stride);
#endif
+/* Helpers to handle the GICD_IPRIORITY register */
+uint32_t vgic_generate_ipriorityr(struct vgic_irq_rank *rank,
+ unsigned int offset);
+void vgic_store_ipriorityr(struct vgic_irq_rank *rank,
+ unsigned int offset,
+ uint32_t reg);
+
#endif /* __ASM_ARM_VGIC_H__ */
/*
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v1 5/8] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank
2015-09-25 14:51 ` [PATCH v1 5/8] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank Julien Grall
@ 2015-09-28 10:50 ` Ian Campbell
2015-09-28 17:10 ` Julien Grall
2015-09-28 10:52 ` Ian Campbell
1 sibling, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2015-09-28 10:50 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
> Xen is currently directly storing the value of register GICD_IPRIORITYR
"of the GICD_... register"
> in the rank. This makes emulation of the register access very simple
> but makes the code to get the priority for a given IRQ more complex.
>
> While the priority of an IRQ is retrieved everytime an IRQ is injected
"every time".
> to the guest, the access to 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 priority for
> every interrupt in the rank. This will make the code to get the priority
> very quick. The emulation code will now have to generate the
> GICD_PRIORITYR
> register for read access and split it to store in a convenient way.
>
> 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:
> - Patch added
> ---
> xen/arch/arm/vgic-v2.c | 24 ++++++++++++++----------
> xen/arch/arm/vgic-v3.c | 27 ++++++++++++++++-----------
> xen/arch/arm/vgic.c | 46
> ++++++++++++++++++++++++++++++++++++++++++++++
> xen/include/asm-arm/vgic.h | 18 +++++++++++++++++-
> 4 files changed, 93 insertions(+), 22 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 47f9da9..23d1982 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)];
> + /* Recreate the IPRIORITYR register */
> + *r = vgic_generate_ipriorityr(rank, gicd_reg - GICD_IPRIORITYR);
> if ( dabt.size == DABT_BYTE )
> *r = vgic_byte_read(*r, gicd_reg);
> vgic_unlock_rank(v, rank, flags);
> @@ -400,18 +400,25 @@ 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->ipriority[REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR,
> - DABT_WORD)] = r;
> + ipriorityr = r;
> else
> - vgic_byte_write(&rank->ipriority[REG_RANK_INDEX(8,
> - gicd_reg - GICD_IPRIORITYR, DABT_WORD)], r, gicd_reg);
> + {
> + ipriorityr = vgic_generate_ipriorityr(rank,
> + gicd_reg - GICD_IPRIORITYR);
> + vgic_byte_write(&ipriorityr, r, gicd_reg);
> + }
> + vgic_store_ipriorityr(rank, gicd_reg - GICD_IPRIORITYR, ipriorityr);
> vgic_unlock_rank(v, rank, flags);
> return 1;
> + }
>
> case GICD_ICFGR: /* SGIs */
> goto write_ignore_32;
> @@ -516,14 +523,11 @@ static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
>
> 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;
> + return rank->priority[irq & INTERRUPT_RANK_MASK];
> }
>
> static int vgic_v2_vcpu_init(struct vcpu *v)
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index c013200..2787507 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -430,18 +430,26 @@ 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;
> + 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;
> + ipriorityr = r;
> else
> - vgic_byte_write(&rank->ipriority[REG_RANK_INDEX(8,
> - reg - GICD_IPRIORITYR, DABT_WORD)], r, reg);
> + {
> + ipriorityr = vgic_generate_ipriorityr(rank, reg - GICD_IPRIORITYR);
> + vgic_byte_write(&ipriorityr, r, reg);
> + }
> + vgic_store_ipriorityr(rank, reg - GICD_IPRIORITYR, ipriorityr);
Given the underlying change to the datastructure I think you should instead
supply a helper to set the priority of a given IRQ and use that for the
DABT_BYTE case.
For the DABT_WORD case your existing store helper would become 4 calls to
the byte helper.
That would be better than generating the full 32-bit value, masking in the
updated byte and then deconstructing the word again.
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index a6835a8..50ad360 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -61,6 +61,52 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
> return vgic_get_rank(v, rank);
> }
>
> +#define NR_PRIORITY_PER_REG 4U
> +#define NR_BIT_PER_PRIORITY 8U
> +
> +/*
> + * Generate the associated IPRIORITYR register based on an offset in the rank.
> + * Note the offset will be round down to match a real HW register.
"rounded".
> 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];
> +
> + /*
> + * It's more convenient to store one priority per interrupt than
> + * the register IPRIORITYR itself
> + */
> + uint8_t priority[32];
Does:
union {
uint32_t ipriorityr[8];
uint8_t priority[32];
}
Get us the best of both worlds, or are we stymied by endianess?
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 5/8] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank
2015-09-28 10:50 ` Ian Campbell
@ 2015-09-28 17:10 ` Julien Grall
2015-09-29 10:56 ` Ian Campbell
0 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2015-09-28 17:10 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
Hi Ian,
On 28/09/15 11:50, Ian Campbell wrote:
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index c013200..2787507 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -430,18 +430,26 @@ 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;
>> + 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;
>> + ipriorityr = r;
>> else
>> - vgic_byte_write(&rank->ipriority[REG_RANK_INDEX(8,
>> - reg - GICD_IPRIORITYR, DABT_WORD)], r, reg);
>> + {
>> + ipriorityr = vgic_generate_ipriorityr(rank, reg - GICD_IPRIORITYR);
>> + vgic_byte_write(&ipriorityr, r, reg);
>> + }
>> + vgic_store_ipriorityr(rank, reg - GICD_IPRIORITYR, ipriorityr);
>
> Given the underlying change to the datastructure I think you should instead
> supply a helper to set the priority of a given IRQ and use that for the
> DABT_BYTE case.
>
> For the DABT_WORD case your existing store helper would become 4 calls to
> the byte helper.
>
> That would be better than generating the full 32-bit value, masking in the
> updated byte and then deconstructing the word again.
We discussed it IRL, so I will summarize here for everyone IRL.
In a follow-up patch (#7), the code to emulate IPRIORITYR will call
vgic_reg32_write which is a generic helper to handle any access size.
The goal is to drop any access size handling in the code. After patch
#7, the code will look like:
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);
ipriorityr = vgic_generate_ipriorityr(rank, reg - GICD_IPRIORITYR);
vgic_reg32_write(&ipriorityr, r, info);
vgic_store_ipriorityr(rank, reg - GICD_IPRIORITYR, ipriorityr);
vgic_unlock_rank(v, rank, flags);
Introducing code path depending on access size means possibly more buggy
code. Note that we already hit a lot of wrong access size in the
emulation. So I would prefer to keep the code as generic as possible
even if it means a small impact on emulating byte access.
>
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index a6835a8..50ad360 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -61,6 +61,52 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
>> return vgic_get_rank(v, rank);
>> }
>>
>> +#define NR_PRIORITY_PER_REG 4U
>> +#define NR_BIT_PER_PRIORITY 8U
>> +
>> +/*
>> + * Generate the associated IPRIORITYR register based on an offset in the rank.
>> + * Note the offset will be round down to match a real HW register.
>
> "rounded".
>
>> 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];
>> +
>> + /*
>> + * It's more convenient to store one priority per interrupt than
>> + * the register IPRIORITYR itself
>> + */
>> + uint8_t priority[32];
>
> Does:
>
> union {
> uint32_t ipriorityr[8];
> uint8_t priority[32];
> }
>
> Get us the best of both worlds, or are we stymied by endianess?
The support of big-endian in Xen should be generic. I.e we deal with the
endianess before calling the handlers, so the handler will always have
the value in the hypervisor endianess.
Note that I've got a support for big-endian but never had the chance to
upstream it.
I think, as long as Xen stays in little endian, we would have no issue
with your solution.
Though using big endian in Xen would be much work that this small
changes. I'll give a look to implement it.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 5/8] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank
2015-09-28 17:10 ` Julien Grall
@ 2015-09-29 10:56 ` Ian Campbell
0 siblings, 0 replies; 43+ messages in thread
From: Ian Campbell @ 2015-09-29 10:56 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On Mon, 2015-09-28 at 18:10 +0100, Julien Grall wrote:
> Hi Ian,
>
> On 28/09/15 11:50, Ian Campbell wrote:
> > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> > > index c013200..2787507 100644
> > > --- a/xen/arch/arm/vgic-v3.c
> > > +++ b/xen/arch/arm/vgic-v3.c
> > > @@ -430,18 +430,26 @@ 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;
> > > + 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;
> > > + ipriorityr = r;
> > > else
> > > - vgic_byte_write(&rank->ipriority[REG_RANK_INDEX(8,
> > > - reg - GICD_IPRIORITYR, DABT_WORD)], r, reg);
> > > + {
> > > + ipriorityr = vgic_generate_ipriorityr(rank, reg -
> > > GICD_IPRIORITYR);
> > > + vgic_byte_write(&ipriorityr, r, reg);
> > > + }
> > > + vgic_store_ipriorityr(rank, reg - GICD_IPRIORITYR,
> > > ipriorityr);
> >
> > Given the underlying change to the datastructure I think you should
> > instead
> > supply a helper to set the priority of a given IRQ and use that for the
> > DABT_BYTE case.
> >
> > For the DABT_WORD case your existing store helper would become 4 calls
> > to
> > the byte helper.
> >
> > That would be better than generating the full 32-bit value, masking in
> > the
> > updated byte and then deconstructing the word again.
>
> We discussed it IRL, so I will summarize here for everyone IRL.
>
> In a follow-up patch (#7), the code to emulate IPRIORITYR will call
> vgic_reg32_write which is a generic helper to handle any access size.
> The goal is to drop any access size handling in the code. After patch
> #7, the code will look like:
>
> 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);
> ipriorityr = vgic_generate_ipriorityr(rank, reg - GICD_IPRIORITYR);
> vgic_reg32_write(&ipriorityr, r, info);
> vgic_store_ipriorityr(rank, reg - GICD_IPRIORITYR, ipriorityr);
> vgic_unlock_rank(v, rank, flags);
>
> Introducing code path depending on access size means possibly more buggy
> code. Note that we already hit a lot of wrong access size in the
> emulation. So I would prefer to keep the code as generic as possible
> even if it means a small impact on emulating byte access.
Agreed, given this context that makes sense.
[...]
> > union {
> > uint32_t ipriorityr[8];
> > uint8_t priority[32];
> > }
> >
> > Get us the best of both worlds, or are we stymied by endianess?
>
> The support of big-endian in Xen should be generic. I.e we deal with the
> endianess before calling the handlers, so the handler will always have
> the value in the hypervisor endianess.
>
> Note that I've got a support for big-endian but never had the chance to
> upstream it.
>
> I think, as long as Xen stays in little endian, we would have no issue
> with your solution.
I wasn't concerned about Xen being big-endian, but rather about whether
ipriorityr[0] is the same byte as priority[0]&0xff or if it was actually
priority[0]>>24 in little endian mode. I wasn't feeling very well on Monday
so I didn't try to think about it too hard.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 5/8] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank
2015-09-25 14:51 ` [PATCH v1 5/8] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank Julien Grall
2015-09-28 10:50 ` Ian Campbell
@ 2015-09-28 10:52 ` Ian Campbell
2015-09-28 16:43 ` Julien Grall
1 sibling, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2015-09-28 10:52 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
> rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
> - if ( rank == NULL ) goto write_ignore;
> + if ( rank == NULL) goto write_ignore;
Accidental change I think.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 5/8] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank
2015-09-28 10:52 ` Ian Campbell
@ 2015-09-28 16:43 ` Julien Grall
0 siblings, 0 replies; 43+ messages in thread
From: Julien Grall @ 2015-09-28 16:43 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On 28/09/15 11:52, Ian Campbell wrote:
> On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
>
>> rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
>> - if ( rank == NULL ) goto write_ignore;
>> + if ( rank == NULL) goto write_ignore;
>
> Accidental change I think.
Yes. I will fix it in the next version.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v1 6/8] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-09-25 14:51 [PATCH v1 0/8] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (4 preceding siblings ...)
2015-09-25 14:51 ` [PATCH v1 5/8] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank Julien Grall
@ 2015-09-25 14:51 ` Julien Grall
2015-09-29 13:07 ` Ian Campbell
2015-09-25 14:51 ` [PATCH v1 7/8] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register Julien Grall
2015-09-25 14:51 ` [PATCH v1 8/8] xen/arm: vgic-v3: Support 32-bit access for 64-bit registers Julien Grall
7 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2015-09-25 14:51 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, vijay.kilari, manish.jaggi, Julien Grall,
stefano.stabellini, Vijaya.Kumar
Xen is currently directly storing the value of register GICD_ITARGETSR
(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 IRQ more complex.
While the target vCPU of an IRQ is retrieved everytime an IRQ 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 introduce 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.
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.
Finally take the opportunity to fix coding style 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:
- Patch added
---
xen/arch/arm/vgic-v2.c | 172 ++++++++++++++++++++++++++-------------------
xen/arch/arm/vgic-v3.c | 121 +++++++++++++++++--------------
xen/arch/arm/vgic.c | 28 ++++++--
xen/include/asm-arm/vgic.h | 13 ++--
4 files changed, 197 insertions(+), 137 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 23d1982..b6db64f 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -50,6 +50,90 @@ 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
+
+/*
+ * Generate the associated ITARGETSR register based on the offset from
+ * ITARGETSR0. Only one vCPU will be listed for a given IRQ.
+ *
+ * Note the offset will be round down to match a real HW register.
+ */
+static uint32_t vgic_generate_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 IRQ
+ * if necessary.
+ * Note the offset will be round down to match a real HW register.
+ */
+static void vgic_store_itargetsr(struct domain *d, struct vgic_irq_rank *rank,
+ unsigned int offset, uint32_t itargetsr)
+{
+ unsigned int i, rank_idx;
+
+ ASSERT(spin_is_locked(&rank->lock));
+
+ rank_idx = offset / NR_INTERRUPT_PER_RANK;
+
+ 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;
+
+ /*
+ * SPI 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 IRQ 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 IRQ if the target vCPU has changed */
+ if ( new_target != old_target )
+ {
+ unsigned int irq = rank_idx * NR_INTERRUPT_PER_RANK + offset;
+
+ vgic_migrate_irq(d->vcpu[old_target],
+ d->vcpu[new_target],
+ irq);
+ }
+
+ rank->vcpu[offset] = new_target;
+ }
+
+}
+
static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
register_t *r)
{
@@ -126,8 +210,8 @@ 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)];
+ /* The recreate the ITARGETSR register */
+ *r = vgic_generate_itargetsr(rank, gicd_reg - GICD_ITARGETSR);
if ( dabt.size == DABT_BYTE )
*r = vgic_byte_read(*r, gicd_reg);
vgic_unlock_rank(v, rank, flags);
@@ -345,56 +429,23 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
case GICD_ITARGETSR + 8 ... GICD_ITARGETSRN:
{
+ uint32_t itargetsr;
+
/* unsigned long needed for find_next_bit */
- unsigned long target;
- int i;
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_generate_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;
}
@@ -504,21 +555,11 @@ static const struct mmio_handler_ops vgic_v2_distr_mmio_handler = {
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);
+ ASSERT(spin_is_locked(&rank->lock));
- /* 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;
+ return v->domain->vcpu[rank->vcpu[irq & INTERRUPT_RANK_MASK]];
}
static int vgic_v2_get_irq_priority(struct vcpu *v, unsigned int irq)
@@ -532,22 +573,14 @@ static int vgic_v2_get_irq_priority(struct vcpu *v, unsigned int irq)
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;
/*
* The hardware domain gets the hardware address.
@@ -586,11 +619,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);
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 2787507..b247043 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -89,18 +89,77 @@ static struct vcpu *vgic_v3_irouter_to_vcpu(struct domain *d, uint64_t irouter)
return d->vcpu[vcpu_id];
}
+#define NR_BYTE_PER_IROUTER 8U
+
+/*
+ * Generate the associated IROUTER register based on the offset from IROUTERO.
+ * Only one vCPU will be listed for a given IRQ.
+ *
+ * Note the offset will be round down to match a real HW register
+ */
+static uint64_t vgic_generate_irouter(struct vgic_irq_rank *rank,
+ unsigned int offset)
+{
+ ASSERT(spin_is_locked(&rank->lock));
+
+ /* There is exactly 1 IRQ per IROUTER */
+ offset /= NR_BYTE_PER_IROUTER;
+
+ /* Get the index in the rank */
+ offset &= INTERRUPT_RANK_MASK;
+
+ return vcpuid_to_vaffinity(rank->vcpu[offset]);
+}
+
+/*
+ * Store a IROUTER register in a convenient way and migrate the IRQ
+ * if necessary.
+ * Note the offset will be round down to match a real HW register.
+ */
+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 rank_idx, irq;
+
+
+ /* There is 1 IRQ per IROUTER */
+ irq = offset / NR_BYTE_PER_IROUTER;
+
+ rank_idx = irq / NR_INTERRUPT_PER_RANK;
+
+ /* Get the index in the rank */
+ offset &= irq & 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, irq);
+
+ rank->vcpu[offset] = new_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);
ASSERT(spin_is_locked(&rank->lock));
- v_target = vgic_v3_irouter_to_vcpu(v->domain, rank->v3.irouter[irq % 32]);
-
- ASSERT(v_target != NULL);
-
- return v_target;
+ return v->domain->vcpu[rank->vcpu[irq & INTERRUPT_RANK_MASK]];
}
static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
@@ -756,8 +815,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_generate_irouter(rank, gicd_reg - GICD_IROUTER);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_NSACR ... GICD_NSACRN:
@@ -842,8 +900,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);
@@ -911,32 +967,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:
@@ -1075,7 +1107,6 @@ static int vgic_v3_get_irq_priority(struct vcpu *v, unsigned int irq)
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;
@@ -1084,15 +1115,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.
@@ -1137,7 +1159,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.
@@ -1189,13 +1211,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);
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 50ad360..cce1bc2 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -65,7 +65,8 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
#define NR_BIT_PER_PRIORITY 8U
/*
- * Generate the associated IPRIORITYR register based on an offset in the rank.
+ * Generate the associated IPRIORITYR register based on the offset from
+ * IPRIORITYR0.
* Note the offset will be round down to match a real HW register.
*/
uint32_t vgic_generate_ipriorityr(struct vgic_irq_rank *rank,
@@ -114,6 +115,23 @@ 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, 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);
+
+ for ( i = 0; i < NR_INTERRUPT_PER_RANK; i++ )
+ rank->vcpu[i] = vcpu;
+}
+
int domain_vgic_init(struct domain *d, unsigned int nr_spis)
{
int i;
@@ -160,8 +178,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);
- for (i=0; i<DOMAIN_NR_RANKS(d); i++)
- spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
+ /* 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], 0);
ret = d->arch.vgic.handler->domain_init(d);
if ( ret )
@@ -215,7 +234,8 @@ 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);
+ /* SGIs/PPIs are routed to this VCPU by default */
+ vgic_rank_init(v->arch.vgic.private_irqs, v->vcpu_id);
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 ce9970e..0cb8029 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -97,14 +97,11 @@ struct vgic_irq_rank {
*/
uint8_t priority[32];
- union {
- struct {
- uint32_t itargets[8];
- }v2;
- struct {
- uint64_t irouter[32];
- }v3;
- };
+ /*
+ * It's more convenient to store a target VCPU per interrupt
+ * than the register ITARGETSR/IROUTER itself
+ */
+ uint8_t vcpu[32];
};
struct sgi_target {
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v1 6/8] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-09-25 14:51 ` [PATCH v1 6/8] xen/arm: vgic: Optimize the way to store the target vCPU " Julien Grall
@ 2015-09-29 13:07 ` Ian Campbell
2015-09-29 13:36 ` Julien Grall
0 siblings, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2015-09-29 13:07 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
> Xen is currently directly storing the value of register GICD_ITARGETSR
> (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 IRQ more complex.
>
> While the target vCPU of an IRQ is retrieved everytime an IRQ 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 introduce 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.
>
> 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 think this is probably OK, but skirting round what the spec actually says
a fair bit.
> Finally take the opportunity to fix coding style 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:
> - Patch added
> ---
> xen/arch/arm/vgic-v2.c | 172 ++++++++++++++++++++++++++-------------
> ------
> xen/arch/arm/vgic-v3.c | 121 +++++++++++++++++--------------
> xen/arch/arm/vgic.c | 28 ++++++--
> xen/include/asm-arm/vgic.h | 13 ++--
> 4 files changed, 197 insertions(+), 137 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 23d1982..b6db64f 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -50,6 +50,90 @@ 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
> +
> +/*
> + * Generate the associated ITARGETSR register based on the offset from
> + * ITARGETSR0. Only one vCPU will be listed for a given IRQ.
> + *
> + * Note the offset will be round down to match a real HW register.
"rounded down".
Although I'm not 100% sure what that comment is trying to say. From the
code I think you mean aligned to the appropriate 4 byte boundary?
> + */
> +static uint32_t vgic_generate_itargetsr(struct vgic_irq_rank *rank,
For all these why not s/generate/read/?
Or since you use "store" for the other half "fetch".
("generate" is a bit of an implementation detail, the caller doesn't care
where or how the result is achieved)
> + 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 IRQ
> + * if necessary.
> + * Note the offset will be round down to match a real HW register.
As above.
> + */
> +static void vgic_store_itargetsr(struct domain *d, struct vgic_irq_rank
> *rank,
> + unsigned int offset, uint32_t
> itargetsr)
> +{
> + unsigned int i, rank_idx;
> +
> + ASSERT(spin_is_locked(&rank->lock));
> +
> + rank_idx = offset / NR_INTERRUPT_PER_RANK;
> +
> + 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;
> +
> + /*
> + * SPI 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 IRQ to the first interrupt
> + * in the target list
> + */
I don't see anything which is handling the PPI and SGI special case (which
is that they are r/o).
Likewise on read they are supposed to always reflect the value of the CPU
doing the read.
Are both of those handled elsewhere in some way I'm missing?
[...]
> + * Note the offset will be round down to match a real HW register
As earlier.
> + * Note the offset will be round down to match a real HW register.
Again.
> + */
> +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 rank_idx, irq;
> +
> +
> + /* There is 1 IRQ per IROUTER */
> + irq = offset / NR_BYTE_PER_IROUTER;
> +
> + rank_idx = irq / NR_INTERRUPT_PER_RANK;
> +
> + /* Get the index in the rank */
> + offset &= irq & 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.
"to the interrupt being ignored"
> 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);
>
> ASSERT(spin_is_locked(&rank->lock));
>
> - v_target = vgic_v3_irouter_to_vcpu(v->domain, rank->v3.irouter[irq %
> 32]);
> -
> - ASSERT(v_target != NULL);
> -
> - return v_target;
> + return v->domain->vcpu[rank->vcpu[irq & INTERRUPT_RANK_MASK]];
Looks like there isn't much call for this to be specific to a given
revision of the gic any more?
Are there sufficient checks for the range of rank->vcpu[...] elsewhere that
we needn't worry about running of domain->vcpu[] here?
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 6/8] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-09-29 13:07 ` Ian Campbell
@ 2015-09-29 13:36 ` Julien Grall
2015-09-29 14:23 ` Ian Campbell
0 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2015-09-29 13:36 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On 29/09/15 14:07, Ian Campbell wrote:
> On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
>> Xen is currently directly storing the value of register GICD_ITARGETSR
>> (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 IRQ more complex.
>>
>> While the target vCPU of an IRQ is retrieved everytime an IRQ 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 introduce 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.
>>
>> 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 think this is probably OK, but skirting round what the spec actually says
> a fair bit.
Well, nothing in the spec clearly explain the behavior of a read access
on the register. An implementation could decide to make some bits RO or
even not store everything.
FWIW, KVM is using the same trick.
>> Finally take the opportunity to fix coding style in section we are
>> modifying.
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
[...]
>> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
>> index 23d1982..b6db64f 100644
>> --- a/xen/arch/arm/vgic-v2.c
>> +++ b/xen/arch/arm/vgic-v2.c
>> @@ -50,6 +50,90 @@ 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
>> +
>> +/*
>> + * Generate the associated ITARGETSR register based on the offset from
>> + * ITARGETSR0. Only one vCPU will be listed for a given IRQ.
>> + *
>> + * Note the offset will be round down to match a real HW register.
>
> "rounded down".
>
> Although I'm not 100% sure what that comment is trying to say. From the
> code I think you mean aligned to the appropriate 4 byte boundary?
Yes. What about: "Note the offset will be aligned to the appropriate 4
byte boundary"
>
>> + */
>> +static uint32_t vgic_generate_itargetsr(struct vgic_irq_rank *rank,
>
> For all these why not s/generate/read/?
>
> Or since you use "store" for the other half "fetch".
>
> ("generate" is a bit of an implementation detail, the caller doesn't care
> where or how the result is achieved)
I will rename to vgic_fetch_itargetsr(...).
[...]
>> + */
>> +static void vgic_store_itargetsr(struct domain *d, struct vgic_irq_rank
>> *rank,
>> + unsigned int offset, uint32_t
>> itargetsr)
>> +{
>> + unsigned int i, rank_idx;
>> +
>> + ASSERT(spin_is_locked(&rank->lock));
>> +
>> + rank_idx = offset / NR_INTERRUPT_PER_RANK;
>> +
>> + 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;
>> +
>> + /*
>> + * SPI 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 IRQ to the first interrupt
>> + * in the target list
>> + */
>
> I don't see anything which is handling the PPI and SGI special case (which
> is that they are r/o).
>
> Likewise on read they are supposed to always reflect the value of the CPU
> doing the read.
>
> Are both of those handled elsewhere in some way I'm missing?
A write to those registers is ignored and handled by a separate case
(GICD_ITARGETSR ... GICD_ITARGETSR + 7).
[...]
>> 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);
>>
>> ASSERT(spin_is_locked(&rank->lock));
>>
>> - v_target = vgic_v3_irouter_to_vcpu(v->domain, rank->v3.irouter[irq %
>> 32]);
>> -
>> - ASSERT(v_target != NULL);
>> -
>> - return v_target;
>> + return v->domain->vcpu[rank->vcpu[irq & INTERRUPT_RANK_MASK]];
>
>
> Looks like there isn't much call for this to be specific to a given
> revision of the gic any more?
Correct, although I keep them as it is because I wasn't sure how this
would fit with the ITS support.
Though we could add LPIs specific helpers if necessary. So I will add a
patch to drop the callbacks get_target_vcpu and get_irq_priority.
>
> Are there sufficient checks for the range of rank->vcpu[...] elsewhere that
> we needn't worry about running of domain->vcpu[] here?
The only places where rank->vcpu[...] is set are vgic_store_* and at the
initialization time (always routed to vCPU 0).
Before the new value is stored, we check the validity of the vCPU the
value should always be valid here.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 6/8] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-09-29 13:36 ` Julien Grall
@ 2015-09-29 14:23 ` Ian Campbell
2015-09-30 18:11 ` Julien Grall
0 siblings, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2015-09-29 14:23 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On Tue, 2015-09-29 at 14:36 +0100, Julien Grall wrote:
> On 29/09/15 14:07, Ian Campbell wrote:
> > On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
> > > Xen is currently directly storing the value of register
> > > GICD_ITARGETSR
> > > (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 IRQ more complex.
> > >
> > > While the target vCPU of an IRQ is retrieved everytime an IRQ 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 introduce 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.
> > >
> > > 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 think this is probably OK, but skirting round what the spec actually
> > says
> > a fair bit.
>
> Well, nothing in the spec clearly explain the behavior of a read access
> on the register. An implementation could decide to make some bits RO or
> even not store everything.
>
> FWIW, KVM is using the same trick.
At least we'll both get screwed by a picky OS then ;-)
> > Although I'm not 100% sure what that comment is trying to say. From the
> > code I think you mean aligned to the appropriate 4 byte boundary?
>
> Yes. What about: "Note the offset will be aligned to the appropriate 4
> byte boundary"
Yes, or just "the appropriate boundary".
> I will rename to vgic_fetch_itargetsr(...).
(and for the other similar functions I assume).
> > I don't see anything which is handling the PPI and SGI special case
> > (which
> > is that they are r/o).
> >
> > Likewise on read they are supposed to always reflect the value of the
> > CPU
> > doing the read.
> >
> > Are both of those handled elsewhere in some way I'm missing?
>
> A write to those registers is ignored and handled by a separate case
> (GICD_ITARGETSR ... GICD_ITARGETSR + 7).
Ah, good!
And a read just happens as normal due to the initialisation of the rank's
fields I saw at some point. Good.
> The only places where rank->vcpu[...] is set are vgic_store_* and at the
> initialization time (always routed to vCPU 0).
> Before the new value is stored, we check the validity of the vCPU the
> value should always be valid here.
Good.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 6/8] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-09-29 14:23 ` Ian Campbell
@ 2015-09-30 18:11 ` Julien Grall
2015-10-01 8:30 ` Ian Campbell
0 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2015-09-30 18:11 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On 29/09/15 15:23, Ian Campbell wrote:
> On Tue, 2015-09-29 at 14:36 +0100, Julien Grall wrote:
>> On 29/09/15 14:07, Ian Campbell wrote:
>>> On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
>>>> Xen is currently directly storing the value of register
>>>> GICD_ITARGETSR
>>>> (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 IRQ more complex.
>>>>
>>>> While the target vCPU of an IRQ is retrieved everytime an IRQ 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 introduce 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.
>>>>
>>>> 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 think this is probably OK, but skirting round what the spec actually
>>> says
>>> a fair bit.
>>
>> Well, nothing in the spec clearly explain the behavior of a read access
>> on the register. An implementation could decide to make some bits RO or
>> even not store everything.
>>
>> FWIW, KVM is using the same trick.
>
> At least we'll both get screwed by a picky OS then ;-)
I think our implementation could fold into 4.3.12 IHI 0048B:
"It is IMPLEMENTATION DEFINED which, if any, SPIs are statically
configured in hardware. The CPU targets field for such an SPI is
read-only, and returns a value that indicates the CPU targets for the
interrupt."
We are implementing a weird read-only but at least an OS should not
trust the written value.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 6/8] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
2015-09-30 18:11 ` Julien Grall
@ 2015-10-01 8:30 ` Ian Campbell
0 siblings, 0 replies; 43+ messages in thread
From: Ian Campbell @ 2015-10-01 8:30 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On Wed, 2015-09-30 at 19:11 +0100, Julien Grall wrote:
> On 29/09/15 15:23, Ian Campbell wrote:
> > On Tue, 2015-09-29 at 14:36 +0100, Julien Grall wrote:
> > > On 29/09/15 14:07, Ian Campbell wrote:
> > > > On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
> > > > > Xen is currently directly storing the value of register
> > > > > GICD_ITARGETSR
> > > > > (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 IRQ more complex.
> > > > >
> > > > > While the target vCPU of an IRQ is retrieved everytime an IRQ 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 introduce 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.
> > > > >
> > > > > 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 think this is probably OK, but skirting round what the spec
> > > > actually
> > > > says
> > > > a fair bit.
> > >
> > > Well, nothing in the spec clearly explain the behavior of a read
> > > access
> > > on the register. An implementation could decide to make some bits RO
> > > or
> > > even not store everything.
> > >
> > > FWIW, KVM is using the same trick.
> >
> > At least we'll both get screwed by a picky OS then ;-)
>
> I think our implementation could fold into 4.3.12 IHI 0048B:
>
> "It is IMPLEMENTATION DEFINED which, if any, SPIs are statically
> configured in hardware. The CPU targets field for such an SPI is
> read-only, and returns a value that indicates the CPU targets for the
> interrupt."
>
> We are implementing a weird read-only but at least an OS should not
> trust the written value.
It's certainly a weird read-only, in that it isn't at all read-only, but I
agree its likely to be tolerated by most OSes.
Note though that an OS written by a pedant would be quite entitled to do:
old_targets = readb(GICD_TARGETR+irq)
writeb(GICD_TARGET+irq, new_targets)
foo = readb(GICD_TARGETR+irq)
ASSERT(foo==old_targets || foo==new_targets)
That would be rather retentive though.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v1 7/8] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register ...
2015-09-25 14:51 [PATCH v1 0/8] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (5 preceding siblings ...)
2015-09-25 14:51 ` [PATCH v1 6/8] xen/arm: vgic: Optimize the way to store the target vCPU " Julien Grall
@ 2015-09-25 14:51 ` Julien Grall
2015-09-29 13:23 ` Ian Campbell
2015-09-25 14:51 ` [PATCH v1 8/8] xen/arm: vgic-v3: Support 32-bit access for 64-bit registers Julien Grall
7 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2015-09-25 14:51 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, vijay.kilari, manish.jaggi, Julien Grall,
stefano.stabellini, Vijaya.Kumar
and use them in the vGIC emulation.
The GIC registers may support different access sizes. Rather than open
coding the access for every registers, provide a set of helpers to access
them.
The caller will have to call vgic_regN_* where N is the size of the
emulated registers.
The new helpers supports any access size and expect the caller to
validate the access size supported by the emulated registers.
Finally, take the opportunity to fix the coding style in section we are
modifying.
Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
I've only done quick tests. I sent them in order to help Vijay
supported multiple access size.
Changes in v2:
- 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 | 99 ++++++++++++++++++++++++----------------
xen/arch/arm/vgic-v3.c | 111 +++++++++++++++++++++++++++++----------------
xen/include/asm-arm/vgic.h | 111 +++++++++++++++++++++++++++++++++++++++++----
3 files changed, 234 insertions(+), 87 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index b6db64f..cdc0825 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -149,24 +149,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_read(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_read(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_read(0x0000043b, info);
return 1;
/* Implementation defined -- read as zero */
@@ -182,7 +189,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_read(rank->ienable, info);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -191,7 +198,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_read(rank->ienable, info);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -206,38 +213,55 @@ 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);
/* The recreate the ITARGETSR register */
- *r = vgic_generate_itargetsr(rank, gicd_reg - GICD_ITARGETSR);
- if ( dabt.size == DABT_BYTE )
- *r = vgic_byte_read(*r, gicd_reg);
+ itargetsr = vgic_generate_itargetsr(rank,
+ gicd_reg - GICD_ITARGETSR);
vgic_unlock_rank(v, rank, flags);
+ *r = vgic_reg32_read(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;
vgic_lock_rank(v, rank, flags);
/* Recreate the IPRIORITYR register */
- *r = vgic_generate_ipriorityr(rank, gicd_reg - GICD_IPRIORITYR);
- if ( dabt.size == DABT_BYTE )
- *r = vgic_byte_read(*r, gicd_reg);
+ ipriorityr = vgic_generate_ipriorityr(rank,
+ gicd_reg - GICD_IPRIORITYR);
vgic_unlock_rank(v, rank, flags);
+ *r = vgic_reg32_read(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_read(icfgr, info);
+
return 1;
+ }
case GICD_NSACR ... GICD_NSACRN:
/* We do not implement security extensions for guests, read zero */
@@ -347,7 +371,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_write(&v->domain->arch.vgic.ctlr, r, info);
+ v->domain->arch.vgic.ctlr &= GICD_CTL_ENABLE;
vgic_unlock(v);
return 1;
@@ -371,11 +396,13 @@ 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;
- /* The virtual irq is derived from register offset.
+ vgic_reg32_setbit(&rank->ienable, r, info);
+ /*
+ * 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),
+ * to get Virtual irq number
+ */
+ vgic_enable_irqs(v, (rank->ienable) & (~tr),
(gicd_reg - GICD_ISENABLER) >> DABT_WORD);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -386,12 +413,14 @@ 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;
- /* The virtual irq is derived from register offset.
+ vgic_reg32_clearbit(&rank->ienable, r, info);
+ /*
+ * 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);
+ * to get Virtual irq number
+ */
+ vgic_disable_irqs(v, (~rank->ienable) & tr,
+ (gicd_reg - GICD_ICENABLER) >> DABT_WORD);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -436,14 +465,9 @@ 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_generate_itargetsr(rank,
- gicd_reg - GICD_ITARGETSR);
- vgic_byte_write(&itargetsr, r, gicd_reg);
- }
+ itargetsr = vgic_generate_itargetsr(rank,
+ gicd_reg - GICD_ITARGETSR);
+ vgic_reg32_write(&itargetsr, r, info);
vgic_store_itargetsr(v->domain, rank, gicd_reg - GICD_ITARGETSR,
itargetsr);
vgic_unlock_rank(v, rank, flags);
@@ -458,14 +482,9 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
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 )
- ipriorityr = r;
- else
- {
- ipriorityr = vgic_generate_ipriorityr(rank,
- gicd_reg - GICD_IPRIORITYR);
- vgic_byte_write(&ipriorityr, r, gicd_reg);
- }
+ ipriorityr = vgic_generate_ipriorityr(rank,
+ gicd_reg - GICD_IPRIORITYR);
+ vgic_reg32_write(&ipriorityr, r, info);
vgic_store_ipriorityr(rank, gicd_reg - GICD_IPRIORITYR, ipriorityr);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -481,7 +500,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_write(&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 b247043..8da695f 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -167,7 +167,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 )
{
@@ -176,21 +175,27 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
goto read_as_zero_32;
case GICR_IIDR:
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICR_IIDR_VAL;
+ *r = vgic_reg32_read(GICV3_GICR_IIDR_VAL, info);
return 1;
case GICR_TYPER:
+ {
+ uint64_t typer, aff;
+
if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
/* TBD: Update processor id in [23:8] when ITS support is added */
aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 |
MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 |
MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 |
MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32);
- *r = aff;
+ typer = aff;
if ( v->arch.vgic.flags & VGIC_V3_RDIST_LAST )
- *r |= GICR_TYPER_LAST;
+ typer |= GICR_TYPER_LAST;
+
+ *r = vgic_reg64_read(typer, info);
return 1;
+ }
case GICR_STATUSR:
/* Not implemented */
goto read_as_zero_32;
@@ -219,7 +224,7 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
case GICR_SYNCR:
if ( dabt.size != DABT_WORD ) goto bad_width;
/* RO . But when read it always returns busy bito bit[0] */
- *r = GICR_SYNCR_NOT_BUSY;
+ *r = vgic_reg32_read(GICR_SYNCR_NOT_BUSY, info);
return 1;
case GICR_MOVLPIR:
/* WO Read as zero */
@@ -229,22 +234,22 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
goto read_as_zero_64;
case GICR_PIDR0:
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICR_PIDR0;
+ *r = vgic_reg32_read(GICV3_GICR_PIDR0, info);
return 1;
case GICR_PIDR1:
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICR_PIDR1;
+ *r = vgic_reg32_read(GICV3_GICR_PIDR1, info);
return 1;
case GICR_PIDR2:
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICR_PIDR2;
+ *r = vgic_reg32_read(GICV3_GICR_PIDR2, info);
return 1;
case GICR_PIDR3:
/* Manufacture/customer defined */
goto read_as_zero_32;
case GICR_PIDR4:
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICR_PIDR4;
+ *r = vgic_reg32_read(GICV3_GICR_PIDR4, info);
return 1;
case GICR_PIDR5 ... GICR_PIDR7:
/* Reserved0 */
@@ -365,7 +370,7 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
rank = vgic_rank_offset(v, 1, reg - GICD_ISENABLER, DABT_WORD);
if ( rank == NULL ) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
- *r = rank->ienable;
+ *r = vgic_reg32_read(rank->ienable, info);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICENABLER ... GICD_ICENABLERN:
@@ -373,7 +378,7 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
rank = vgic_rank_offset(v, 1, reg - GICD_ICENABLER, DABT_WORD);
if ( rank == NULL ) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
- *r = rank->ienable;
+ *r = vgic_reg32_read(rank->ienable, info);
vgic_unlock_rank(v, rank, flags);
return 1;
/* Read the pending status of an IRQ via GICD/GICR is not supported */
@@ -387,25 +392,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);
/* Recreate the IPRIORITYR register */
- *r = vgic_generate_ipriorityr(rank, reg - GICD_IPRIORITYR);
- if ( dabt.size == DABT_BYTE )
- *r = vgic_byte_read(*r, reg);
+ ipriorityr = vgic_generate_ipriorityr(rank, reg - GICD_IPRIORITYR);
vgic_unlock_rank(v, rank, flags);
+
+ *r = vgic_reg32_read(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_read(icfgr, info);
+
return 1;
+ }
default:
printk(XENLOG_G_ERR
"%pv: %s: unhandled read r%d offset %#08x\n",
@@ -444,9 +462,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);
tr = rank->ienable;
- rank->ienable |= r;
+ vgic_reg32_setbit(&rank->ienable, r, info);
/* The irq number is extracted from offset. so shift by register size */
- vgic_enable_irqs(v, r & (~tr), (reg - GICD_ISENABLER) >> DABT_WORD);
+ vgic_enable_irqs(v, (rank->ienable) & (~tr),
+ (reg - GICD_ISENABLER) >> DABT_WORD);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICENABLER ... GICD_ICENABLERN:
@@ -455,9 +474,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);
tr = rank->ienable;
- rank->ienable &= ~r;
+ vgic_reg32_clearbit(&rank->ienable, r, info);
/* 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, (~rank->ienable) & tr,
+ (reg - GICD_ICENABLER) >> DABT_WORD);
vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ISPENDR ... GICD_ISPENDRN:
@@ -497,13 +517,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);
- if ( dabt.size == DABT_WORD )
- ipriorityr = r;
- else
- {
- ipriorityr = vgic_generate_ipriorityr(rank, reg - GICD_IPRIORITYR);
- vgic_byte_write(&ipriorityr, r, reg);
- }
+ ipriorityr = vgic_generate_ipriorityr(rank, reg - GICD_IPRIORITYR);
+ vgic_reg32_write(&ipriorityr, r, info);
vgic_store_ipriorityr(rank, reg - GICD_IPRIORITYR, ipriorityr);
vgic_unlock_rank(v, rank, flags);
return 1;
@@ -518,7 +533,9 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
if ( rank == NULL ) goto write_ignore;
vgic_lock_rank(v, rank, flags);
- rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR, DABT_WORD)] = r;
+ vgic_reg32_write(&rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR,
+ DABT_WORD)],
+ r, info);
vgic_unlock_rank(v, rank, flags);
return 1;
default:
@@ -754,7 +771,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
case GICD_CTLR:
if ( dabt.size != DABT_WORD ) goto bad_width;
vgic_lock(v);
- *r = v->domain->arch.vgic.ctlr;
+ *r = vgic_reg32_read(v->domain->arch.vgic.ctlr, info);
vgic_unlock(v);
return 1;
case GICD_TYPER:
@@ -769,13 +786,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_read(typer, info);
return 1;
}
@@ -787,7 +807,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
goto read_as_zero_32;
case GICD_IIDR:
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICD_IIDR_VAL;
+ *r = vgic_reg32_read(GICV3_GICD_IIDR_VAL, info);
return 1;
case 0x020 ... 0x03c:
case 0xc000 ... 0xffcc:
@@ -810,14 +830,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_generate_irouter(rank, gicd_reg - GICD_IROUTER);
+ irouter = vgic_generate_irouter(rank, gicd_reg - GICD_IROUTER);
vgic_unlock_rank(v, rank, flags);
+
+ *r = vgic_reg64_read(irouter, info);
+
return 1;
+ }
+
case GICD_NSACR ... GICD_NSACRN:
/* We do not implement security extensions for guests, read zero */
goto read_as_zero_32;
@@ -833,17 +861,17 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
case GICD_PIDR0:
/* GICv3 identification value */
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICD_PIDR0;
+ *r = vgic_reg32_read(GICV3_GICD_PIDR0, info);
return 1;
case GICD_PIDR1:
/* GICv3 identification value */
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICD_PIDR1;
+ *r = vgic_reg32_read(GICV3_GICD_PIDR1, info);
return 1;
case GICD_PIDR2:
/* GICv3 identification value */
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICD_PIDR2;
+ *r = vgic_reg32_read(GICV3_GICD_PIDR2, info);
return 1;
case GICD_PIDR3:
/* GICv3 identification value. Manufacturer/Customer defined */
@@ -851,7 +879,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
case GICD_PIDR4:
/* GICv3 identification value */
if ( dabt.size != DABT_WORD ) goto bad_width;
- *r = GICV3_GICD_PIDR4;
+ *r = vgic_reg32_read(GICV3_GICD_PIDR4, info);
return 1;
case GICD_PIDR5 ... GICD_PIDR7:
/* Reserved0 */
@@ -963,14 +991,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_generate_irouter(rank, gicd_reg - GICD_IROUTER);
+ vgic_reg64_write(&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 0cb8029..6d4c938 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
{
@@ -164,26 +165,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_read(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_write(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_setbit(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_clearbit(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##_read(uint##sz##_t reg, \
+ const mmio_info_t *info) \
+{ \
+ return vgic_reg_read(reg, info->gpa & offmask, \
+ info->dabt.size); \
+} \
+ \
+static inline void vgic_reg##sz##_write(uint##sz##_t *reg, \
+ register_t val, \
+ const mmio_info_t *info) \
+{ \
+ unsigned long tmp = *reg; \
+ \
+ vgic_reg_write(&tmp, val, info->gpa & offmask, \
+ info->dabt.size); \
+ \
+ *reg = tmp; \
+} \
+ \
+static inline void vgic_reg##sz##_setbit(uint##sz##_t *reg, \
+ register_t bits, \
+ const mmio_info_t *info) \
+{ \
+ unsigned long tmp = *reg; \
+ \
+ vgic_reg_setbit(&tmp, bits, info->gpa & offmask, \
+ info->dabt.size); \
+ \
+ *reg = tmp; \
+} \
+ \
+static inline void vgic_reg##sz##_clearbit(uint##sz##_t *reg, \
+ register_t bits, \
+ const mmio_info_t *info) \
+{ \
+ unsigned long tmp = *reg; \
+ \
+ vgic_reg_clearbit(&tmp, bits, info->gpa & offmask, \
+ info->dabt.size); \
+ \
+ *reg = tmp; \
}
+/*
+ * 64 bits register 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] 43+ messages in thread* Re: [PATCH v1 7/8] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register ...
2015-09-25 14:51 ` [PATCH v1 7/8] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register Julien Grall
@ 2015-09-29 13:23 ` Ian Campbell
2015-09-29 13:48 ` Julien Grall
0 siblings, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2015-09-29 13:23 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On Fri, 2015-09-25 at 15: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.
These helpers end up as e.g. vgic_regN_read/write, but they don't really
read or write anything, they just extract the required bits into a register
or update the bits into a variable.
Can we think of a better name? encode/decode?
This might be more apparent (maybe without the need to rename) if the
helpers took info->dabt.size instead of info.
> [...]
> +static inline void vgic_reg_clearbit(unsigned long *reg, register_t bits,
Please call these ones clearbits/setbits to avoid giving the impression
that they take a bit offset and just set/clear that one.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 7/8] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register ...
2015-09-29 13:23 ` Ian Campbell
@ 2015-09-29 13:48 ` Julien Grall
2015-09-29 14:24 ` Ian Campbell
0 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2015-09-29 13:48 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On 29/09/15 14:23, Ian Campbell wrote:
> On Fri, 2015-09-25 at 15: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.
>
> These helpers end up as e.g. vgic_regN_read/write, but they don't really
> read or write anything, they just extract the required bits into a register
> or update the bits into a variable.
>
> Can we think of a better name? encode/decode?
Encode/decode would be the best. I will replace read by decode and write
by encode.
>
> This might be more apparent (maybe without the need to rename) if the
> helpers took info->dabt.size instead of info.
We also need info to get the MMIO adddress. I would prefer to keep info
and avoid having another parameter here.
>> [...]
>> +static inline void vgic_reg_clearbit(unsigned long *reg, register_t bits,
>
> Please call these ones clearbits/setbits to avoid giving the impression
> that they take a bit offset and just set/clear that one.
Will do.
--
Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 7/8] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register ...
2015-09-29 13:48 ` Julien Grall
@ 2015-09-29 14:24 ` Ian Campbell
2015-10-02 9:36 ` Julien Grall
0 siblings, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2015-09-29 14:24 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On Tue, 2015-09-29 at 14:48 +0100, Julien Grall wrote:
> On 29/09/15 14:23, Ian Campbell wrote:
> > On Fri, 2015-09-25 at 15: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.
> >
> > These helpers end up as e.g. vgic_regN_read/write, but they don't
> > really
> > read or write anything, they just extract the required bits into a
> > register
> > or update the bits into a variable.
> >
> > Can we think of a better name? encode/decode?
>
> Encode/decode would be the best. I will replace read by decode and write
> by encode.
>
> >
> > This might be more apparent (maybe without the need to rename) if the
> > helpers took info->dabt.size instead of info.
>
> We also need info to get the MMIO adddress.
Ah, missed that.
> I would prefer to keep info and avoid having another parameter here.
Sure, with the improved name this is less important anyway.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 7/8] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register ...
2015-09-29 14:24 ` Ian Campbell
@ 2015-10-02 9:36 ` Julien Grall
0 siblings, 0 replies; 43+ messages in thread
From: Julien Grall @ 2015-10-02 9:36 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On 29/09/15 15:24, Ian Campbell wrote:
> On Tue, 2015-09-29 at 14:48 +0100, Julien Grall wrote:
>> On 29/09/15 14:23, Ian Campbell wrote:
>>> On Fri, 2015-09-25 at 15: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.
>>>
>>> These helpers end up as e.g. vgic_regN_read/write, but they don't
>>> really
>>> read or write anything, they just extract the required bits into a
>>> register
>>> or update the bits into a variable.
>>>
>>> Can we think of a better name? encode/decode?
>>
>> Encode/decode would be the best. I will replace read by decode and write
>> by encode.
Just to keep track, as discussed IRL, I will finally use extract/update.
The former will be used when the guest read the GIC MMIO, the latter
when it will write.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v1 8/8] xen/arm: vgic-v3: Support 32-bit access for 64-bit registers
2015-09-25 14:51 [PATCH v1 0/8] xen/arm: vgic: Support 32-bit access for 64-bit register Julien Grall
` (6 preceding siblings ...)
2015-09-25 14:51 ` [PATCH v1 7/8] xen/arm: vgic: Introduce helpers to read/write/clear/set vGIC register Julien Grall
@ 2015-09-25 14:51 ` Julien Grall
2015-09-29 13:27 ` Ian Campbell
7 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2015-09-25 14:51 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, vijay.kilari, manish.jaggi, Julien Grall,
stefano.stabellini, Vijaya.Kumar
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>
---
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 candiate for Xen 4.6. 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:
- 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 8da695f..46b5ad8 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -162,6 +162,15 @@ static struct vcpu *vgic_v3_get_target_vcpu(struct vcpu *v, unsigned int irq)
return v->domain->vcpu[rank->vcpu[irq & INTERRUPT_RANK_MASK]];
}
+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)
@@ -178,10 +187,11 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
*r = vgic_reg32_read(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 |
@@ -267,7 +277,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;
@@ -343,7 +353,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:
@@ -833,7 +843,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;
@@ -908,7 +918,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;
@@ -994,7 +1004,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;
@@ -1053,7 +1063,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] 43+ messages in thread* Re: [PATCH v1 8/8] xen/arm: vgic-v3: Support 32-bit access for 64-bit registers
2015-09-25 14:51 ` [PATCH v1 8/8] xen/arm: vgic-v3: Support 32-bit access for 64-bit registers Julien Grall
@ 2015-09-29 13:27 ` Ian Campbell
0 siblings, 0 replies; 43+ messages in thread
From: Ian Campbell @ 2015-09-29 13:27 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Vijaya.Kumar, stefano.stabellini, manish.jaggi, vijay.kilari
On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
> 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 candiate for Xen 4.6. 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.
I think if we let it bake in unstable for a bit to gain confidence we could
consider backporting the whole lot to either 4.6.1 or .2.
^ permalink raw reply [flat|nested] 43+ messages in thread