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

[Scott Wood]

On Thu, 21 Jul 2011 18:49:44 +0200
Alexander Graf <address@hidden> wrote:

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

The region needs to be reserved in the dt regardless.

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

I'm not so sure in this case.

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

Never mind, ePAPR does say to update the spin table with the actual PIR
prior to entry.

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

In the KVM direct-mapped memory case there might not be any memory at all
in the first 256M.  We've got a patch in our queue to choose an appropriate
IMA at runtime.

> > And don't forget about r3_hi and r6_hi on future 64-bit targets.
> 
> The code as is is 100% 32-bit.

Right, but it'd be nice to minimize the things that silently break on
64-bit.

-Scott