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

[Alexander Graf]

On 21.07.2011, at 18:38, Scott Wood wrote:

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

Hmm - sounds like a nice idea. We'd have to reserve the region in the dt, but 
overall I agree that it might end up being simpler.

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

Yeah, but at the end of the day C is a lot more readable than asm IMHO :).

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

I merely do what the u-boot code is doing.

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

Hm - we have a linear map of 256MB. But yes, maybe creating a mapping instead 
of just assuming it'll be within there is a good idea :).

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

The code as is is 100% 32-bit.

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

Yes, it does. We can't assume everyone to be running on ppc hosts or have ppc 
cross-compilers available. Firmware blobs have to be kept in the tree 
precompiled.


Alex