From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:32921) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QjwGv-0002pm-J7 for qemu-devel@nongnu.org; Thu, 21 Jul 2011 12:38:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QjwGu-0003rh-3A for qemu-devel@nongnu.org; Thu, 21 Jul 2011 12:38:49 -0400 Received: from tx2ehsobe004.messaging.microsoft.com ([65.55.88.14]:26167 helo=TX2EHSOBE009.bigfish.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QjwGt-0003rQ-UL for qemu-devel@nongnu.org; Thu, 21 Jul 2011 12:38:48 -0400 Date: Thu, 21 Jul 2011 11:38:40 -0500 From: Scott Wood Message-ID: <20110721113840.49798d8c@schlenkerla.am.freescale.net> In-Reply-To: <1311211654-14326-2-git-send-email-agraf@suse.de> References: <1311211654-14326-1-git-send-email-agraf@suse.de> <1311211654-14326-2-git-send-email-agraf@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 01/23] PPC: Add secondary CPU spinning code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: QEMU-devel Developers On Thu, 21 Jul 2011 03:27:12 +0200 Alexander Graf 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 > +#include > + > +/* > + * 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. > + 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