Re: [Xen-devel] [PATCH V2 1/2] xen: arm: introduce Cortex-A7 support

[Tim Deegan <tim@xxxxxxx>]

At 06:43 -0600 on 03 Jul (1372833795), Bamvor Jian Zhang wrote:
> Hi, Ian
> 
>  >>>Wrote "Ian Campbell <Ian.Campbell@xxxxxxxxxx>"> On Tue,
>  Tue, 2013-07-02 at 21:11 +0800, Bamvor Jian Zhang wrote:
> > >  
> > > +        /* Set return address(PIC) */
> > > +        /* XXX: it only work while thumb2 is not enable in xen */
> > 
> > That's true of lots/all of our asm. Lets ignore that for now (no need to
> > comment).
> ok
> > Since we already have physoffset in r10 would it be clearer to make use
> > of that?
> sorry if i am wrong. do u mean change "adr lr, 1f" to something like 
> "add lr, pc, r10"?

No, I think this code is correct as it stands. 

But it is a bit strange that the CPUinfo struct contains _code_ to jump
to a routine, rather than just containing the address of the routine
itself.  If it just contained the address, the structures could be
defined in C (so we'd be certain that they matched the offset and size
constants we use here).  And then this asm would indeed use physoffset,
something like:

        ldr   r1, [r1, #PROCINFO_cpu_init]   /* r1 := vaddr(init func) */
        adr   lr, 1f                         /* Save return address */
        add   pc, r1, r10                    /* Call paddr(init func) */
1:

The way you've done it is certainly very clever, but we don't need to
optimize single instructions out of the boot time and I think this is
clearer.

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel