Re: [Qemu-devel] [PATCH 21/28] PPC: E500: Add PV spinning code

[Scott Wood]

On Wed, 27 Jul 2011 15:34:31 +0200
Alexander Graf <address@hidden> wrote:

> On 07/25/2011 10:40 PM, Scott Wood wrote:
> > On Sat, 23 Jul 2011 12:50:05 +0200
> > Alexander Graf<address@hidden>  wrote:
> >
> >> +typedef struct spin_info {
> >> +    uint64_t addr;
> >> +    uint64_t r3;
> >> +    uint32_t resv;
> >> +    uint32_t pir;
> >> +    uint64_t r6;
> >> +} __attribute__ ((packed)) SpinInfo;
> > Note that r6 isn't part of the ePAPR spin table -- I think it may have been
> > in an early draft and gotten into U-Boot that way.  A future ePAPR could
> > assign another use to that position in the spin table.
> 
> How is the size defined then?

The size of what?

The size of the IMA is up to the boot program (ePAPR 1.1 will impose a
minimum of 1 MiB), and is conveyed to the OS in r7.

The size of the spin table entry is determined by the release method name.
If fields are added in the future, we'd call it something like
"spin-table-v2".  Any OS that writes to the r6 field with a "spin-table"
release-method is invoking undefined behavior.

> > Do we support any host ABIs strange enough that __attribute__((packed))
> > would be needed here?
> 
> I don't think we do, but in general I prefer to be safe rather than 
> sorry. It doesn't hurt, right?

GCC takes it as meaning the start of the structure could be misaligned, and
thus generates poor code for some architectures.

I think specifying both packed and an explicit struct alignment will avoid
that problem and still be sufficiently paranoid, if desired.  Not that this
is particularly performance critical, of course. :-)

> >> +static void mmubooke_create_initial_mapping(CPUState *env,
> >> +                                     target_ulong va,
> >> +                                     target_phys_addr_t pa,
> >> +                                     target_phys_addr_t len)
> >> +{
> >> +    ppcmas_tlb_t *tlb = booke206_get_tlbm(env, 1, 0, 0);
> >> +    target_phys_addr_t size;
> >> +
> >> +    size = (booke206_page_size_to_tlb(len)<<  MAS1_TSIZE_SHIFT);
> >> +    tlb->mas1 = MAS1_VALID | size;
> >> +    tlb->mas2 = va&  TARGET_PAGE_MASK;
> >> +    tlb->mas7_3 = pa&  TARGET_PAGE_MASK;
> >> +    tlb->mas7_3 |= MAS3_UR | MAS3_UW | MAS3_UX | MAS3_SR | MAS3_SW | 
> >> MAS3_SX;
> >> +}
> > [snip]
> >> +    mmubooke_create_initial_mapping(env, env->nip, env->nip, map_size);
> > ePAPR says:
> >
> >    The Secondary IMA (SIMA) mapping in the MMU shall map effective address 0
> >    to the entry_addr field in the spin table, aligned down to the SIMA size.
> 
> So it jumps to nip & ~64MB?

Yes.

-Scott