Re: [Qemu-devel] [PATCH 01/23] PPC: Add secondary CPU spinning code

[Scott Wood]

On Thu, 21 Jul 2011 03:27:12 +0200
Alexander Graf <address@hidden> wrote:

> When directly starting an SMP system with -kernel on PPC e500, we need to
> simulate the spin table code from u-boot. This code adds a small c file
> plus generated .elf file that enable secondary CPUs to spin just like they
> would with u-boot.

Why not just handle the spin table as an MMIO region?

Besides being simpler, it avoids any spinning overhead if the guest doesn't
spin up all the cpus.

> diff --git a/pc-bios/ppc_spin.c b/pc-bios/ppc_spin.c
> new file mode 100644
> index 0000000..e46a6a7
> --- /dev/null
> +++ b/pc-bios/ppc_spin.c
> @@ -0,0 +1,97 @@
> +#include <stdio.h>
> +#include <stdint.h>
> +
> +/*
> + * Secondary CPU spin code
> + *
> + * Compile using: gcc -m32 -nostdlib ppc_spin.c -o ppc_spin.elf -Os \
> + *                    -fno-stack-protector -Wl,-Ttext,0x7700000
> + */
> +
> +/* Initialize stack pointer */
> +asm (
> +"    .global _start             \n"
> +"    _start:                    \n"
> +"    addis   1, 3, address@hidden    \n"
> +"    b       spin               \n");
> +
> +typedef struct spin_info {
> +    uint32_t addr_hi;
> +    uint32_t addr;
> +    uint32_t r3_hi;
> +    uint32_t r3;
> +    uint32_t resv;
> +    uint32_t pir;
> +    uint32_t r6_hi;
> +    uint32_t r6;
> +} SpinInfo;
> +
> +#define __stringify_1(x...)     #x
> +#define __stringify(x...)       __stringify_1(x)
> +
> +#define mfspr(rn)       ({unsigned long rval; \
> +                        asm volatile("mfspr %0," __stringify(rn) \
> +                                : "=r" (rval)); rval;})
> +#define mtspr(rn, v)    asm volatile("mtspr " __stringify(rn) ",%0" : : "r" 
> (v)\
> +                                     : "memory")
> +static inline void mtdec(unsigned long v)
> +{
> +    asm volatile("mtdec %0" : : "r" (v));
> +}
> +
> +static inline void mtmsr(unsigned long msr)
> +{
> +    asm("mtmsr %0" : : "r"(msr));
> +}
> +
> +#define __MASK(X)        (1UL<<(X))
> +#define MSR_WE_LG        18              /* Wait State Enable */
> +#define MSR_EE_LG        15              /* External Interrupt Enable */
> +#define MSR_WE           __MASK(MSR_WE_LG)       /* Wait State Enable */
> +#define MSR_EE           __MASK(MSR_EE_LG)       /* External Interrupt 
> Enable */
> +#define SPR_PIR          0x11E
> +#define SPR_HID0         (0x3F0)
> +#define SPR_BOOKE_IVOR10 (0x19A)
> +#define SPR_BOOKE_IVPR   (0x03F)
> +
> +void loop(void);
> +
> +__attribute__((noreturn)) void spin(unsigned long ptr)
> +{
> +    volatile SpinInfo *info = (void*)ptr;
> +    uint32_t pir = mfspr(SPR_PIR);
> +    __attribute__((noreturn)) void (*entry)(
> +        unsigned long r3, unsigned long r4, unsigned long r5, unsigned long 
> r6,
> +        unsigned long r7, unsigned long r8, unsigned long r9);
> +    unsigned long dec_p = (unsigned long)loop;
> +
> +    info->pir = pir;
> +    info->r3 = pir;
> +    info->addr = 1;
> +    info->r6 = 0;
> +
> +    /* we don't want to keep the other CPUs from running, so set the IVOR for
> +       DEC to our loop and only check for info->addr every other cycle */
> +
> +    mtspr(SPR_HID0, 0x00E00000);
> +    mtspr(SPR_BOOKE_IVOR10, dec_p & 0xfff);
> +    mtspr(SPR_BOOKE_IVPR, dec_p & ~0xfff);
> +loop:
> +    asm volatile (".global loop\n"
> +        "   loop:" : : : "memory", "cc");

You're jumping through a lot of hoops to (nominally) do this in C.

> +    info->pir = pir;

While I'm fine with not allowing the guest to set PIR (the ISA says it's
read-only in virtualized implementations), I'm not sure we should be
overwriting the spintable value here.

> +    entry = (void*)(unsigned long)info->addr;

You're ignoring addr_hi, and you should create an IMA appropriate for the
guest's chosen entry point rather than assume it's in the first 64M.

And don't forget about r3_hi and r6_hi on future 64-bit targets.

The function pointer approach might also run into trouble on 64-bit, due
to ABI weirdness.

> diff --git a/pc-bios/ppc_spin.elf b/pc-bios/ppc_spin.elf
> new file mode 100755
> index 
> 0000000000000000000000000000000000000000..71c872b2d4685100b0050d549735662d7d763e08
> GIT binary patch
> literal 66553

Must this go into the tree as a binary?  Why can't it be built when qemu is
built (if it's needed at all)?

This has proven to be rather annoying with the dtb as well.

-Scott