Re: [PATCH 2/2] pseries: Correctly create ibm,segment-page-sizes property

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

> > +        for (count = 0; count<  PPC_PAGE_SIZES_MAX_SZ; count++) {
> > +            if (sps->enc[count].page_shift = 0) {
> > +                break;
> > +            }
> > +        }
> > +        if ((p - prop)>= (maxcells - 3 - count * 2))
> 
> Is this valid C? Can you substract one pointer from another and compare 
> the result with an int?

Ugh ... yeah it's basic stuff ;-) the difference between two pointers is
an integer (there's even a ptrdiff_t nowadays no ?)

> > +            break;
> 
> Braces? Please run checkpatch :)

Ah missed that one.

> > +        *(p++) = cpu_to_be32(sps->page_shift);
> > +        *(p++) = cpu_to_be32(sps->slb_enc);
> > +        *(p++) = cpu_to_be32(count);
> > +        for (j = 0; j<  count; j++) {
> > +            *(p++) = cpu_to_be32(sps->enc[j].page_shift);
> > +            *(p++) = cpu_to_be32(sps->enc[j].pte_enc);
> > +        }
> > +    }
> > +
> > +    return (p - prop) * sizeof(uint32_t);
> 
> I'd prefer a second integer counter "len" I think :). Pointer 
> arithmentics always make me wary...
> 
And a separate variable that might accidentally get out of sync makes
_me_ wary :-)

> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index 77aa186..860711c 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -201,6 +201,7 @@ static void kvm_get_fallback_smmu_info(CPUPPCState *env,
> >       if (kvm_check_extension(env->kvm_state, KVM_CAP_PPC_GET_PVINFO)) {
> >           /* No flags */
> >           info->flags = 0;
> > +        info->slb_size = 64;
> 
> Eh - this one belongs in the first patch, no?

Quite possibly, the split in 2 patches was done by David (I originally
did a single patch), so I'm not 100% sure why he put that there, I'll
have a closer look today.

> >           /* Standard 4k base page size segment */
> >           info->sps[0].page_shift = 12;
> > @@ -218,9 +219,15 @@ static void kvm_get_fallback_smmu_info(CPUPPCState *env,
> >
> >   	/* HV KVM has backing store size restrictions */
> >           info->flags = KVM_PPC_PAGE_SIZES_REAL;
> > +        if (env->mmu_model = POWERPC_MMU_2_06) {
> > +            info->slb_size = 32;
> > +        } else {
> > +            info->slb_size = 64;
> > +        }
> 
> This assumes that we're always using -cpu host. Is there any more 
> reliable way of calculating the slb size? Otherwise maybe we should just 
> error out in non-cpu-host cases for HV mode.

This is a fallback, it's good enough. I don't think there's such a thing
as non-cpu-host on HV anyway, at least for now (we should probably error
out elsewhere). In any case, even if the CPU is configured for backward
compat (which we don't support yet, though might using -cpu in the long
run), the SLB size so far has to be exactly the one implemented by the
host when using HV KVM.

So for anything we can work on today, the above will work.

> > -        if (env->mmu_model&  POWERPC_MMU_1TSEG)
> > -            info->flags = KVM_PPC_1T_SEGMENTS;
> > +        if (env->mmu_model&  POWERPC_MMU_1TSEG) {
> > +            info->flags |= KVM_PPC_1T_SEGMENTS;
> > +        }
> 
> Ahem :)

What's this ? Second patch adding the braces missing in the first one ?
Heh, ok, I'll fix that.

Cheers,
Ben.