Xen project Mailing List

Re: [Xen-devel] [PATCH V2 23/46] xen: arm: add register_t type, native register size for the hypervisor

To: Ian Campbell <Ian.Campbell@xxxxxxxxxx>

From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

Date: Thu, 21 Feb 2013 16:49:56 +0000

Cc: "xen-devel@xxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxx>, "Tim \(Xen.org\)" <tim@xxxxxxx>, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>

Delivery-date: Thu, 21 Feb 2013 16:50:27 +0000

List-id: Xen developer discussion <xen-devel.lists.xen.org>

On Thu, 21 Feb 2013, Ian Campbell wrote: > On Thu, 2013-02-21 at 16:23 +0000, Stefano Stabellini wrote: > > On Thu, 21 Feb 2013, Ian Campbell wrote: > > > On Thu, 2013-02-14 at 16:47 +0000, Ian Campbell wrote: > > > > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx> > > > > Acked-by: Tim Deegan <tim@xxxxxxx> > > > > but: > > > > This is mostly a matter of coding taste, so I'd like Stefano's > > > > ack/nack here as well. > > > > > > Stefano, any strong opinion? > > > > Are there any concrete benefits in introducing register_t compared to > > using unsigned long? > > It decouples us from assuming a compiler where unsigned long is the size > of a register ;-) > > In the ARM port we have mostly been trying to define and use fixed size > and/or semantic types (uintXX_t, paddr_t etc) rather than compiler > variant things like int and long. OK. Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx> > > > > > > > > xen/arch/arm/domain_build.c | 2 +- > > > > xen/arch/arm/smpboot.c | 2 +- > > > > xen/arch/arm/traps.c | 44 > > > > ++++++++++++++++++++++-------------------- > > > > xen/arch/arm/vgic.c | 18 ++++++++-------- > > > > xen/arch/arm/vpl011.c | 6 ++-- > > > > xen/arch/arm/vtimer.c | 6 ++-- > > > > xen/include/asm-arm/regs.h | 2 +- > > > > xen/include/asm-arm/types.h | 4 +++ > > > > 8 files changed, 45 insertions(+), 39 deletions(-) > > > > > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > > > index 7403f1a..30d014a 100644 > > > > --- a/xen/arch/arm/domain_build.c > > > > +++ b/xen/arch/arm/domain_build.c > > > > @@ -268,7 +268,7 @@ static int prepare_dtb(struct domain *d, struct > > > > kernel_info *kinfo) > > > > > > > > static void dtb_load(struct kernel_info *kinfo) > > > > { > > > > - void * __user dtb_virt = (void *)(u32)kinfo->dtb_paddr; > > > > + void * __user dtb_virt = (void *)(register_t)kinfo->dtb_paddr; > > > > > > > > raw_copy_to_guest(dtb_virt, kinfo->fdt, fdt_totalsize(kinfo->fdt)); > > > > xfree(kinfo->fdt); > > > > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > > > > index 86379b7..d8eb5d3 100644 > > > > --- a/xen/arch/arm/smpboot.c > > > > +++ b/xen/arch/arm/smpboot.c > > > > @@ -142,7 +142,7 @@ void __cpuinit start_secondary(unsigned long > > > > boot_phys_offset, > > > > set_processor_id(cpuid); > > > > > > > > /* Setup Hyp vector base */ > > > > - WRITE_CP32((uint32_t) hyp_traps_vector, HVBAR); > > > > + WRITE_CP32((register_t) hyp_traps_vector, HVBAR); > > > > > > > > mmu_init_secondary_cpu(); > > > > enable_vfp(); > > > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > > > > index eaf1f52..0299b33 100644 > > > > --- a/xen/arch/arm/traps.c > > > > +++ b/xen/arch/arm/traps.c > > > > @@ -68,7 +68,7 @@ static void print_xen_info(void) > > > > debug_build() ? 'y' : 'n', print_tainted(taint_str)); > > > > } > > > > > > > > -uint32_t *select_user_reg(struct cpu_user_regs *regs, int reg) > > > > +register_t *select_user_reg(struct cpu_user_regs *regs, int reg) > > > > { > > > > BUG_ON( !guest_mode(regs) ); > > > > > > > > @@ -81,20 +81,20 @@ uint32_t *select_user_reg(struct cpu_user_regs > > > > *regs, int reg) > > > > > > > > switch ( reg ) { > > > > case 0 ... 7: /* Unbanked registers */ > > > > - BUILD_BUG_ON(REGOFFS(r0) + 7*sizeof(uint32_t) != REGOFFS(r7)); > > > > + BUILD_BUG_ON(REGOFFS(r0) + 7*sizeof(register_t) != > > > > REGOFFS(r7)); > > > > return &regs->r0 + reg; > > > > case 8 ... 12: /* Register banked in FIQ mode */ > > > > - BUILD_BUG_ON(REGOFFS(r8_fiq) + 4*sizeof(uint32_t) != > > > > REGOFFS(r12_fiq)); > > > > + BUILD_BUG_ON(REGOFFS(r8_fiq) + 4*sizeof(register_t) != > > > > REGOFFS(r12_fiq)); > > > > if ( fiq_mode(regs) ) > > > > return &regs->r8_fiq + reg - 8; > > > > else > > > > return &regs->r8 + reg - 8; > > > > case 13 ... 14: /* Banked SP + LR registers */ > > > > - BUILD_BUG_ON(REGOFFS(sp_fiq) + 1*sizeof(uint32_t) != > > > > REGOFFS(lr_fiq)); > > > > - BUILD_BUG_ON(REGOFFS(sp_irq) + 1*sizeof(uint32_t) != > > > > REGOFFS(lr_irq)); > > > > - BUILD_BUG_ON(REGOFFS(sp_svc) + 1*sizeof(uint32_t) != > > > > REGOFFS(lr_svc)); > > > > - BUILD_BUG_ON(REGOFFS(sp_abt) + 1*sizeof(uint32_t) != > > > > REGOFFS(lr_abt)); > > > > - BUILD_BUG_ON(REGOFFS(sp_und) + 1*sizeof(uint32_t) != > > > > REGOFFS(lr_und)); > > > > + BUILD_BUG_ON(REGOFFS(sp_fiq) + 1*sizeof(register_t) != > > > > REGOFFS(lr_fiq)); > > > > + BUILD_BUG_ON(REGOFFS(sp_irq) + 1*sizeof(register_t) != > > > > REGOFFS(lr_irq)); > > > > + BUILD_BUG_ON(REGOFFS(sp_svc) + 1*sizeof(register_t) != > > > > REGOFFS(lr_svc)); > > > > + BUILD_BUG_ON(REGOFFS(sp_abt) + 1*sizeof(register_t) != > > > > REGOFFS(lr_abt)); > > > > + BUILD_BUG_ON(REGOFFS(sp_und) + 1*sizeof(register_t) != > > > > REGOFFS(lr_und)); > > > > switch ( regs->cpsr & PSR_MODE_MASK ) > > > > { > > > > case PSR_MODE_USR: > > > > @@ -315,11 +315,11 @@ static void show_guest_stack(struct cpu_user_regs > > > > *regs) > > > > printk("GUEST STACK GOES HERE\n"); > > > > } > > > > > > > > -#define STACK_BEFORE_EXCEPTION(regs) ((uint32_t*)(regs)->sp) > > > > +#define STACK_BEFORE_EXCEPTION(regs) ((register_t*)(regs)->sp) > > > > > > > > static void show_trace(struct cpu_user_regs *regs) > > > > { > > > > - uint32_t *frame, next, addr, low, high; > > > > + register_t *frame, next, addr, low, high; > > > > > > > > printk("Xen call trace:\n "); > > > > > > > > @@ -327,7 +327,7 @@ static void show_trace(struct cpu_user_regs *regs) > > > > print_symbol(" %s\n ", regs->pc); > > > > > > > > /* Bounds for range of valid frame pointer. */ > > > > - low = (uint32_t)(STACK_BEFORE_EXCEPTION(regs)/* - 2*/); > > > > + low = (register_t)(STACK_BEFORE_EXCEPTION(regs)/* - 2*/); > > > > high = (low & ~(STACK_SIZE - 1)) + > > > > (STACK_SIZE - sizeof(struct cpu_info)); > > > > > > > > @@ -356,7 +356,7 @@ static void show_trace(struct cpu_user_regs *regs) > > > > break; > > > > { > > > > /* Ordinary stack frame. */ > > > > - frame = (uint32_t *)next; > > > > + frame = (register_t *)next; > > > > next = frame[-1]; > > > > addr = frame[0]; > > > > } > > > > @@ -364,7 +364,7 @@ static void show_trace(struct cpu_user_regs *regs) > > > > printk("[<%p>]", _p(addr)); > > > > print_symbol(" %s\n ", addr); > > > > > > > > - low = (uint32_t)&frame[1]; > > > > + low = (register_t)&frame[1]; > > > > } > > > > > > > > printk("\n"); > > > > @@ -372,7 +372,7 @@ static void show_trace(struct cpu_user_regs *regs) > > > > > > > > void show_stack(struct cpu_user_regs *regs) > > > > { > > > > - uint32_t *stack = STACK_BEFORE_EXCEPTION(regs), addr; > > > > + register_t *stack = STACK_BEFORE_EXCEPTION(regs), addr; > > > > int i; > > > > > > > > if ( guest_mode(regs) ) > > > > @@ -486,20 +486,22 @@ static arm_hypercall_t arm_hypercall_table[] = { > > > > > > > > static void do_debug_trap(struct cpu_user_regs *regs, unsigned int > > > > code) > > > > { > > > > - uint32_t reg, *r; > > > > + register_t *r; > > > > + uint32_t reg; > > > > uint32_t domid = current->domain->domain_id; > > > > switch ( code ) { > > > > case 0xe0 ... 0xef: > > > > reg = code - 0xe0; > > > > r = select_user_reg(regs, reg); > > > > - printk("DOM%d: R%d = %#010"PRIx32" at %#010"PRIx32"\n", > > > > + printk("DOM%d: R%d = 0x%"PRIregister" at 0x%"PRIvaddr"\n", > > > > domid, reg, *r, regs->pc); > > > > break; > > > > case 0xfd: > > > > - printk("DOM%d: Reached %#010"PRIx32"\n", domid, regs->pc); > > > > + printk("DOM%d: Reached %"PRIvaddr"\n", domid, regs->pc); > > > > break; > > > > case 0xfe: > > > > - printk("%c", (char)(regs->r0 & 0xff)); > > > > + r = select_user_reg(regs, 0); > > > > + printk("%c", (char)(*r & 0xff)); > > > > break; > > > > case 0xff: > > > > printk("DOM%d: DEBUG\n", domid); > > > > @@ -561,7 +563,7 @@ static void do_cp15_32(struct cpu_user_regs *regs, > > > > union hsr hsr) > > > > { > > > > struct hsr_cp32 cp32 = hsr.cp32; > > > > - uint32_t *r = select_user_reg(regs, cp32.reg); > > > > + uint32_t *r = (uint32_t*)select_user_reg(regs, cp32.reg); > > > > > > > > if ( !cp32.ccvalid ) { > > > > dprintk(XENLOG_ERR, "cp_15(32): need to handle invalid > > > > condition codes\n"); > > > > @@ -607,7 +609,7 @@ static void do_cp15_32(struct cpu_user_regs *regs, > > > > BUG_ON(!vtimer_emulate(regs, hsr)); > > > > break; > > > > default: > > > > - printk("%s p15, %d, r%d, cr%d, cr%d, %d @ %#08x\n", > > > > + printk("%s p15, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n", > > > > cp32.read ? "mrc" : "mcr", > > > > cp32.op1, cp32.reg, cp32.crn, cp32.crm, cp32.op2, > > > > regs->pc); > > > > panic("unhandled 32-bit CP15 access %#x\n", hsr.bits & > > > > HSR_CP32_REGS_MASK); > > > > @@ -637,7 +639,7 @@ static void do_cp15_64(struct cpu_user_regs *regs, > > > > BUG_ON(!vtimer_emulate(regs, hsr)); > > > > break; > > > > default: > > > > - printk("%s p15, %d, r%d, r%d, cr%d @ %#08x\n", > > > > + printk("%s p15, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n", > > > > cp64.read ? "mrrc" : "mcrr", > > > > cp64.op1, cp64.reg1, cp64.reg2, cp64.crm, regs->pc); > > > > panic("unhandled 64-bit CP15 access %#x\n", hsr.bits & > > > > HSR_CP64_REGS_MASK); > > > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > > > > index 39b9775..57147d5 100644 > > > > --- a/xen/arch/arm/vgic.c > > > > +++ b/xen/arch/arm/vgic.c > > > > @@ -160,7 +160,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, > > > > mmio_info_t *info) > > > > { > > > > struct hsr_dabt dabt = info->dabt; > > > > struct cpu_user_regs *regs = guest_cpu_user_regs(); > > > > - uint32_t *r = select_user_reg(regs, dabt.reg); > > > > + register_t *r = select_user_reg(regs, dabt.reg); > > > > struct vgic_irq_rank *rank; > > > > int offset = (int)(info->gpa - VGIC_DISTR_BASE_ADDRESS); > > > > int gicd_reg = REG(offset); > > > > @@ -372,7 +372,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, > > > > mmio_info_t *info) > > > > { > > > > struct hsr_dabt dabt = info->dabt; > > > > struct cpu_user_regs *regs = guest_cpu_user_regs(); > > > > - uint32_t *r = select_user_reg(regs, dabt.reg); > > > > + register_t *r = select_user_reg(regs, dabt.reg); > > > > struct vgic_irq_rank *rank; > > > > int offset = (int)(info->gpa - VGIC_DISTR_BASE_ADDRESS); > > > > int gicd_reg = REG(offset); > > > > @@ -421,13 +421,13 @@ static int vgic_distr_mmio_write(struct vcpu *v, > > > > mmio_info_t *info) > > > > > > > > case GICD_ISPENDR ... GICD_ISPENDRN: > > > > if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width; > > > > - printk("vGICD: unhandled %s write %#"PRIx32" to ISPENDR%d\n", > > > > + printk("vGICD: unhandled %s write %#"PRIregister" to > > > > ISPENDR%d\n", > > > > dabt.size ? "word" : "byte", *r, gicd_reg - > > > > GICD_ISPENDR); > > > > return 0; > > > > > > > > case GICD_ICPENDR ... GICD_ICPENDRN: > > > > if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width; > > > > - printk("vGICD: unhandled %s write %#"PRIx32" to ICPENDR%d\n", > > > > + printk("vGICD: unhandled %s write %#"PRIregister" to > > > > ICPENDR%d\n", > > > > dabt.size ? "word" : "byte", *r, gicd_reg - > > > > GICD_ICPENDR); > > > > return 0; > > > > > > > > @@ -499,19 +499,19 @@ static int vgic_distr_mmio_write(struct vcpu *v, > > > > mmio_info_t *info) > > > > > > > > case GICD_SGIR: > > > > if ( dabt.size != 2 ) goto bad_width; > > > > - printk("vGICD: unhandled write %#"PRIx32" to ICFGR%d\n", > > > > + printk("vGICD: unhandled write %#"PRIregister" to ICFGR%d\n", > > > > *r, gicd_reg - GICD_ICFGR); > > > > return 0; > > > > > > > > case GICD_CPENDSGIR ... GICD_CPENDSGIRN: > > > > if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width; > > > > - printk("vGICD: unhandled %s write %#"PRIx32" to > > > > ICPENDSGIR%d\n", > > > > + printk("vGICD: unhandled %s write %#"PRIregister" to > > > > ICPENDSGIR%d\n", > > > > dabt.size ? "word" : "byte", *r, gicd_reg - > > > > GICD_CPENDSGIR); > > > > return 0; > > > > > > > > case GICD_SPENDSGIR ... GICD_SPENDSGIRN: > > > > if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width; > > > > - printk("vGICD: unhandled %s write %#"PRIx32" to > > > > ISPENDSGIR%d\n", > > > > + printk("vGICD: unhandled %s write %#"PRIregister" to > > > > ISPENDSGIR%d\n", > > > > dabt.size ? "word" : "byte", *r, gicd_reg - > > > > GICD_SPENDSGIR); > > > > return 0; > > > > > > > > @@ -537,13 +537,13 @@ static int vgic_distr_mmio_write(struct vcpu *v, > > > > mmio_info_t *info) > > > > goto write_ignore; > > > > > > > > default: > > > > - printk("vGICD: unhandled write r%d=%"PRIx32" offset %#08x\n", > > > > + printk("vGICD: unhandled write r%d=%"PRIregister" offset > > > > %#08x\n", > > > > dabt.reg, *r, offset); > > > > return 0; > > > > } > > > > > > > > bad_width: > > > > - printk("vGICD: bad write width %d r%d=%"PRIx32" offset %#08x\n", > > > > + printk("vGICD: bad write width %d r%d=%"PRIregister" offset > > > > %#08x\n", > > > > dabt.size, dabt.reg, *r, offset); > > > > domain_crash_synchronous(); > > > > return 0; > > > > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c > > > > index 7dcee90..db5094e 100644 > > > > --- a/xen/arch/arm/vpl011.c > > > > +++ b/xen/arch/arm/vpl011.c > > > > @@ -92,7 +92,7 @@ static int uart0_mmio_read(struct vcpu *v, > > > > mmio_info_t *info) > > > > { > > > > struct hsr_dabt dabt = info->dabt; > > > > struct cpu_user_regs *regs = guest_cpu_user_regs(); > > > > - uint32_t *r = select_user_reg(regs, dabt.reg); > > > > + register_t *r = select_user_reg(regs, dabt.reg); > > > > int offset = (int)(info->gpa - UART0_START); > > > > > > > > switch ( offset ) > > > > @@ -114,7 +114,7 @@ static int uart0_mmio_write(struct vcpu *v, > > > > mmio_info_t *info) > > > > { > > > > struct hsr_dabt dabt = info->dabt; > > > > struct cpu_user_regs *regs = guest_cpu_user_regs(); > > > > - uint32_t *r = select_user_reg(regs, dabt.reg); > > > > + register_t *r = select_user_reg(regs, dabt.reg); > > > > int offset = (int)(info->gpa - UART0_START); > > > > > > > > switch ( offset ) > > > > @@ -127,7 +127,7 @@ static int uart0_mmio_write(struct vcpu *v, > > > > mmio_info_t *info) > > > > /* Silently ignore */ > > > > return 1; > > > > default: > > > > - printk("VPL011: unhandled write r%d=%"PRIx32" offset %#08x\n", > > > > + printk("VPL011: unhandled write r%d=%"PRIregister" offset > > > > %#08x\n", > > > > dabt.reg, *r, offset); > > > > domain_crash_synchronous(); > > > > } > > > > diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c > > > > index 85201b5..291b87e 100644 > > > > --- a/xen/arch/arm/vtimer.c > > > > +++ b/xen/arch/arm/vtimer.c > > > > @@ -99,7 +99,7 @@ static int vtimer_emulate_32(struct cpu_user_regs > > > > *regs, union hsr hsr) > > > > { > > > > struct vcpu *v = current; > > > > struct hsr_cp32 cp32 = hsr.cp32; > > > > - uint32_t *r = select_user_reg(regs, cp32.reg); > > > > + uint32_t *r = (uint32_t *)select_user_reg(regs, cp32.reg); > > > > s_time_t now; > > > > > > > > switch ( hsr.bits & HSR_CP32_REGS_MASK ) > > > > @@ -151,8 +151,8 @@ static int vtimer_emulate_64(struct cpu_user_regs > > > > *regs, union hsr hsr) > > > > { > > > > struct vcpu *v = current; > > > > struct hsr_cp64 cp64 = hsr.cp64; > > > > - uint32_t *r1 = select_user_reg(regs, cp64.reg1); > > > > - uint32_t *r2 = select_user_reg(regs, cp64.reg2); > > > > + uint32_t *r1 = (uint32_t *)select_user_reg(regs, cp64.reg1); > > > > + uint32_t *r2 = (uint32_t *)select_user_reg(regs, cp64.reg2); > > > > uint64_t ticks; > > > > s_time_t now; > > > > > > > > diff --git a/xen/include/asm-arm/regs.h b/xen/include/asm-arm/regs.h > > > > index 7486944..a723f92 100644 > > > > --- a/xen/include/asm-arm/regs.h > > > > +++ b/xen/include/asm-arm/regs.h > > > > @@ -34,7 +34,7 @@ > > > > * Returns a pointer to the given register value in regs, taking the > > > > * processor mode (CPSR) into account. > > > > */ > > > > -extern uint32_t *select_user_reg(struct cpu_user_regs *regs, int reg); > > > > +extern register_t *select_user_reg(struct cpu_user_regs *regs, int > > > > reg); > > > > > > > > #endif /* __ARM_REGS_H__ */ > > > > /* > > > > diff --git a/xen/include/asm-arm/types.h b/xen/include/asm-arm/types.h > > > > index d3e16d8..9ca32f1 100644 > > > > --- a/xen/include/asm-arm/types.h > > > > +++ b/xen/include/asm-arm/types.h > > > > @@ -41,6 +41,8 @@ typedef u32 vaddr_t; > > > > typedef u64 paddr_t; > > > > #define INVALID_PADDR (~0ULL) > > > > #define PRIpaddr "016llx" > > > > +typedef u32 register_t; > > > > +#define PRIregister "x" > > > > #elif defined (CONFIG_ARM_64) > > > > typedef signed long s64; > > > > typedef unsigned long u64; > > > > @@ -49,6 +51,8 @@ typedef u64 vaddr_t; > > > > typedef u64 paddr_t; > > > > #define INVALID_PADDR (~0UL) > > > > #define PRIpaddr "016lx" > > > > +typedef u64 register_t; > > > > +#define PRIregister "lx" > > > > #endif > > > > > > > > typedef unsigned long size_t; > > > > -- > > > > 1.7.2.5 > > > > > > > > > > > > > > > > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.