Re: [Xen-devel] [PATCH RFC 5/7] xen: arm: rewrite start of day page table and cpu bring up

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2013-09-17 at 17:18 +0100, Julien Grall wrote:
> > -1:      dsb
> > -        ldr   r0, =smp_up_cpu        /* VA of gate */
> > -        add   r0, r0, r10            /* PA of gate */
> > -        ldr   r1, [r0]               /* Which CPU is being booted? */
> > -        teq   r1, r12                /* Is it us? */
> > -        wfene
> > -        bne   1b
> > +        mov   r12, #0                /* r12 := !!is_boot_cpu */
> 
> Do you mean !is_boot_cpu?

I think I do. r12 == 0 on the boot and == 1 on secondaries.

> 
> > +
> > +        b     common_start
> > +
> > +GLOBAL(init_secondary)
> > +        cpsid aif                    /* Disable all interrupts */
> > +
> > +        /* Find out where we are */
> > +        ldr   r0, =start
> > +        adr   r9, start              /* r9  := paddr (start) */
> > +        sub   r10, r9, r0            /* r10 := phys-offset */
> > +
> > +        mov   r12, #1                /* r12 := !!is_boot_cpu */
> 
> Same here.

Right, and twice again the 64-bit version ;-)


> > +        /* Write Xen's PT's paddr into the HTTBR */
> > +        ldr   r4, =boot_pgtable
> > +        add   r4, r4, r10            /* r4 := paddr (xen_pagetable) */
> 
> s/xen_pagetable/boot_pgtable/?
> 
> > +        mov   r5, #0                 /* r4:r5 is paddr (xen_pagetable) */
> 
> Same here.

Ack.

> 
> > +        mcrr  CP64(r4, r5, HTTBR)
> > +
> > +        /* Setup boot_pgtable: */
> > +        ldr   r1, =boot_second
> > +        add   r1, r1, r10            /* r1 := paddr (boot_second) */
> >          mov   r3, #0x0
> > +
> > +        /* ... map boot_second in boot_pgtable[0] */
> >          orr   r2, r1, #PT_UPPER(PT)  /* r2:r3 := table map of xen_second */
> s/xen_second/boot_second/?

Yep.

> > +        /* Now we can install the fixmap and dtb mappings, since we
> > +         * don't need the 1:1 map any more */
> > +        dsb   sy
> > +        ldr   r1, =boot_second
> > +#if defined(EARLY_PRINTK)
> > +        /* xen_fixmap pagetable */
> 
> Can you add a comment to explain why we don't need to map the fixmap
> when early printk is not enabled?

It's covered by the overall description of the boot tables layout which
is in mm.c and referenced elsewhere in this file. Is the suficient?
[..]
> > diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
> > index f460e9c..f616807 100644
> > --- a/xen/include/asm-arm/platform.h
> > +++ b/xen/include/asm-arm/platform.h
> > @@ -14,6 +14,11 @@ struct platform_desc {
> >      /* Platform initialization */
> >      int (*init)(void);
> >      int (*init_time)(void);
> > +#ifdef CONFIG_ARM_32
> > +    /* SMP */
> > +    int (*cpu_init)(int cpu);
> 
> I don't think a cpu_init callback is usefull. An smp_init callback would
> be better.
> 
> This will allow you to move the sys_flags check for the versatile
> express in smp_init.

I wondered if there might be platforms with differeing mbox addresses
for different CPU. e.g. the armv8 stuff (which doesn't use this path)
makes provisions for this.

But I'll make the suggested change -- we can also refactor or add a
second callback if such a platform shows up.

You didn't trim your quotes, I hope I didn't miss any comments (I
trimmed a bunch of the s/xen_/boot_/ ones... Will do a thorough
sweep...)

Ian.


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