Re: [Qemu-devel] [PATCH] Enable support for PPC64 on qemu target

[Alexander Graf]

On 26.02.2009, at 23:35, Laurent Vivier wrote:

> Hi,
>
> see comments below.

Thanks for looking at this :-).

> Le 26 févr. 09 à 22:15, Alexander Graf a écrit :
>
> > This patch enables OpenBIOS to initialize on PPC64, enabling  
> > support for -cpu 970fx.
> > It gets up to the boot prompt and works rather good so far, though  
> > I haven't been able to run a kernel yet.
> >
> > For more recent PowerPC CPUs the PTE layout has changed, so we need  
> > to take that into account and create PTEs according to the new  
> > layout and at the new physical positions.
> >
> > Signed-off-by: Alexander Graf <address@hidden>
>
> > Index: arch/ppc/qemu/init.c
> > ===================================================================
> > --- arch/ppc/qemu/init.c        (revision 459)
> > +++ arch/ppc/qemu/init.c        (working copy)
> > @@ -230,6 +230,19 @@
> > }
> >
> > static void
> > +cpu_g5_init(const struct cpudef *cpu)
> > +{
> > +    cpu_generic_init(cpu);
> > +
> > +    PUSH(0);
> > +    fword("encode-int");
> > +    push_str("reg");
> > +    fword("property");
> > +
> > +    fword("finish-device");
> > +}
> As you use "970fx" in ppc_defs[], name should be "cpu_970_init()".

I agree.

> > +static void
> > cpu_g4_init(const struct cpudef *cpu)
> > {
> >     cpu_generic_init(cpu);
> > @@ -359,6 +372,18 @@
> >         .clock_frequency = 0x1dcd6500,
> >         .initfn = cpu_g4_init,
> >     },
> > +    { // XXX find out real values
> > +        .iu_version = 0x003C0000,
> > +        .name = "PowerPC 970fx",
>
> could you check on a real machine, according other values it seems  
> it should be "PowerPC,970FX".
> (also, there are some examples in linux/Documentation/powerpc/ 
> booting-without-of.txt).
> I'm not able to do a "dev /cpus/PowerPC 970fx"...

Sounds about right.

> > +        .icache_size = 0x8000,
> > +        .dcache_size = 0x8000,
> > +        .icache_sets = 0x80,
> > +        .dcache_sets = 0x80,
> > +        .icache_block_size = 0x20,
> > +        .dcache_block_size = 0x20,
> > +        .clock_frequency = 0x1dcd6500,
> > +        .initfn = cpu_g5_init,
> > +    },
> > };
>
> cpu_970_init() ?
>
> > static const struct cpudef *
> > Index: arch/ppc/qemu/ofmem.c
> > ===================================================================
> > --- arch/ppc/qemu/ofmem.c       (revision 459)
> > +++ arch/ppc/qemu/ofmem.c       (working copy)
> > @@ -23,6 +23,7 @@
> > #include "mmutypes.h"
> > #include "asm/processor.h"
> >
> > +#define PPC64
>
> Is it really needed ?

Nope - I had this in because I broke ppc32 booting in between. But  
it's probably best to remove the #ifdefs again.

> > #define BIT(n)          (1U<<(31-(n)))
> >
> > #define FREE_BASE               0x00004000
> > @@ -620,10 +621,80 @@
> >         return phys;
> > }
> >
> > +#ifdef PPC64
> > static void
> > -hash_page( ulong ea, ulong phys, int mode )
> > +hash_page_64( ulong ea, ulong phys, int mode )
> > {
> >         static int next_grab_slot=0;
> > +       uint64_t vsid_mask, page_mask, pgidx, hash;
> > +       uint64_t htab_mask, mask, avpn;
> > +       ulong pgaddr;
> > +       int i, found;
> > +       unsigned int vsid, vsid_sh, sdr, sdr_sh, sdr_mask;
> > +       mPTE_64_t *pp;
> > +
> > +#define TARGET_PAGE_BITS 12
>
> in include/ofmem.h, we already have PAGE_SHIFT

Ah - I forgot about this part. As you can see the #define in the  
middle of the code indicates that this was not supposed to stay this  
way :-).

> > +
> > +       vsid = (ea>>28) + SEGR_BASE;
> > +       vsid_sh = 7;
> > +       vsid_mask = 0x00003FFFFFFFFF80ULL;
> > +       asm ( "mfsdr1 %0" : "=r" (sdr) );
> > +       sdr_sh = 18;
> > +       sdr_mask = 0x3FF80;
> > +       page_mask = 0x0FFFFFFF; // XXX correct?
> > +       pgidx = (ea & page_mask) >> TARGET_PAGE_BITS;
> > +       avpn = (vsid << 12) | ((pgidx >> 4) & 0x0F80);;
> > +
> > +       hash = ((vsid ^ pgidx) << vsid_sh) & vsid_mask;
> > +       htab_mask = 0x0FFFFFFF >> (28 - (sdr & 0x1F));
> > +       mask = (htab_mask << sdr_sh) | sdr_mask;
> > +       pgaddr = sdr | (hash & mask);
> > +       pp = (mPTE_64_t *)pgaddr;
> > +
> > +       /* replace old translation */
> > +       for( found=0, i=0; !found && i<8; i++ )
> > +               if( pp[i].avpn == avpn )
> > +                       found=1;
> > +
> > +       /* otherwise use a free slot */
> > +       for( i=0; !found && i<8; i++ )
> > +               if( !pp[i].v )
> > +                       found=1;
> > +
> > +       /* out of slots, just evict one */
> > +       if( !found ) {
> > +               i = next_grab_slot + 1;
> > +               next_grab_slot = (next_grab_slot + 1) % 8;
> > +       }
> > +       i--;
> > +       {
> > +       mPTE_64_t p = {
> > +               // .avpn_low = avpn,
> > +               .avpn = avpn >> 7,
> > +               .h = 0,
> > +               .v = 1,
> > +
> > +               .rpn = (phys & ~0xfff) >> 12,
> > +               .r = mode & (1 << 8) ? 1 : 0,
> > +               .c = mode & (1 << 7) ? 1 : 0,
> > +               .w = mode & (1 << 6) ? 1 : 0,
> > +               .i = mode & (1 << 5) ? 1 : 0,
> > +               .m = mode & (1 << 4) ? 1 : 0,
> > +               .g = mode & (1 << 3) ? 1 : 0,
> > +               .n = mode & (1 << 2) ? 1 : 0,
> > +               .pp = mode & 3,
> > +       };
> > +       pp[i] = p;
> > +       }
> > +
> > +       asm volatile( "tlbie %0"  :: "r"(ea) );
> > +}
> > +#endif
> > +
> > +static void
> > +hash_page_32( ulong ea, ulong phys, int mode )
> > +{
> > +       static int next_grab_slot=0;
> >         ulong *upte, cmp, hash1;
> >         int i, vsid, found;
> >         mPTE_t *pp;
> > @@ -660,6 +731,26 @@
> >         asm volatile( "tlbie %0"  :: "r"(ea) );
> > }
> >
> > +#ifdef PPC64
> > +static int is_ppc64(void)
> > +{
> > +       unsigned int pvr;
> > +       asm volatile("mfspr %0, 0x11f" : "=r" (pvr) );
> > +
> > +       return ((pvr >= 0x330000) && (pvr < 0x70330000));
> > +}
> > +#endif
> > +
> > +static void hash_page( ulong ea, ulong phys, int mode )
> > +{
> > +#ifdef PPC64
> > +       if ( is_ppc64() )
> > +               hash_page_64(ea, phys, mode);
> > +       else
> > +#endif
> > +               hash_page_32(ea, phys, mode);
> > +}
> > +
> > void
> > dsi_exception( void )
> > {
> > @@ -706,7 +797,14 @@
> >
> >         /* SDR1: Storage Description Register 1 */
> >
> > -       hash_base = (ramsize - 0x00100000 - HASH_SIZE) & 0xffff0000;
> > +#ifdef PPC64
> > +       if(is_ppc64()) {
> > +           hash_base = (ramsize - 0x00200000 - HASH_SIZE) & 0xfff00000;
>
> Why is it 0x00200000 ?
>
> Please update comment (and code...) in arch/ppc/qemu/start.S !

It might not be needed. I'll check again. I was running into issues  
where qemu thought the pagetable was in the middle of the rom.

> > +       } else
> > +#endif
> > +       {
> > +           hash_base = (ramsize - 0x00100000 - HASH_SIZE) & 0xffff0000;
> > +       }
> >         memset((void *)hash_base, 0, HASH_SIZE);
> >         sdr1 = hash_base | ((HASH_SIZE-1) >> 16);
> >         asm volatile("mtsdr1 %0" :: "r" (sdr1) );
> > Index: arch/ppc/qemu/mmutypes.h
> > ===================================================================
> > --- arch/ppc/qemu/mmutypes.h    (revision 459)
> > +++ arch/ppc/qemu/mmutypes.h    (working copy)
> > @@ -38,7 +38,29 @@
> >         unsigned long pp:2;     /* Page protection */
> > } mPTE_t;
> >
> > +#define uint64_t unsigned long long
>
> uint64_t is defined in include/ppc/types.h

Thanks for the catch.

> > +typedef struct mPTE_64 {
> > +       uint32_t avpn_low;      /* Abbreviated Virtual Page Number (unused) */
> > +       uint32_t avpn:25;       /* Abbreviated Virtual Page Number */
> > +       uint32_t sw:4;          /* Software Use */
> > +       uint32_t  :1;           /* Reserved */
> > +       uint32_t h:1;           /* Hash algorithm indicator */
> > +       uint32_t v:1;           /* Entry is valid */
> >
> > +       uint32_t rpn_low;       /* Real (physical) page number (unused) */
> > +       uint32_t rpn:20;        /* Real (physical) page number */
> > +       uint32_t    :2;         /* Reserved */
> > +       uint32_t ac:1;          /* Address Compare*/
> > +       uint32_t r:1;           /* Referenced */
> > +       uint32_t c:1;           /* Changed */
> > +       uint32_t w:1;           /* Write-thru cache mode */
> > +       uint32_t i:1;           /* Cache inhibited */
> > +       uint32_t m:1;           /* Memory coherence */
> > +       uint32_t g:1;           /* Guarded */
> > +       uint32_t n:1;           /* No-Execute */
> > +       uint32_t pp:2;          /* Page protection */
> > +} mPTE_64_t;
>
> Could you use "unsigned long" like in other definitions to be  
> consistent ?

I rather don't. It might be worthwhile to port the code to ppc64 one  
day and then everything using long breaks. I'd much rather see the  
rest of the code converted to uint*_t than making this long.

Alex

> > typedef struct _mBATU {         /* Upper part of BAT (all except 601) */
> >         unsigned long bepi:15;	/* Effective page index (virtual  
> > address) */
> >         unsigned long :4;       /* Unused */