On 21.07.2011, at 18:38, Scott Wood wrote: > On Thu, 21 Jul 2011 03:27:12 +0200 > Alexander Graf <ag...@suse.de> 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, 0x10000@h \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