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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

Ian.
> 
> 
> > >  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