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

[Alexander Graf <agraf@suse.de>]

On 04/27/2012 07:51 AM, Benjamin Herrenschmidt wrote:
> The core tcg/kvm code for ppc64 now has at least the outline
> capability to support pagesizes beyond the standard 4k and 16MB.  The
> CPUState is initialized with information advertising the available
> pagesizes and their correct encodings, and under the right KVM setup
> this will be populated with page sizes beyond the standard.
>
> Obviously guests can't use the extra page sizes unless they know
> they're present.  For the pseries machine, at least, there is a
> defined method for conveying exactly this information, the
> "ibm-segment-page-sizes" property in the guest device tree.
>
> This patch generates this property using the supported page size
> information that's already in the CPUState.
>
> Signed-off-by: Nishanth Aravamudan<nacc@us.ibm.com>
> Signed-off-by: David Gibson<david@gibson.dropbear.id.au>
> Signed-off-by: Benjamin Herrenschmidt<benh@kernel.crashing.org>
> ---
>   hw/spapr.c       |   42 ++++++++++++++++++++++++++++++++++++++++++
>   target-ppc/kvm.c |   11 +++++++++--
>   2 files changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/hw/spapr.c b/hw/spapr.c
> index 94a4e1e..7c36903 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -149,6 +149,39 @@ static int spapr_set_associativity(void *fdt, sPAPREnvironment *spapr)
>       return ret;
>   }
>
> +
> +static size_t create_page_sizes_prop(CPUPPCState *env, uint32_t *prop,
> +                                     size_t maxsize)
> +{
> +    size_t maxcells = maxsize / sizeof(uint32_t);
> +    int i, j, count;
> +    uint32_t *p = prop;
> +
> +    for (i = 0; i<  PPC_PAGE_SIZES_MAX_SZ; i++) {
> +        struct ppc_one_seg_page_size *sps =&env->sps.sps[i];
> +
> +        if (!sps->page_shift) {
> +            break;
> +        }
> +        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?

> +            break;

Braces? Please run checkpatch :)

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

> +}
> +
>   static void *spapr_create_fdt_skel(const char *cpu_model,
>                                      target_phys_addr_t rma_size,
>                                      target_phys_addr_t initrd_base,
> @@ -304,6 +337,8 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>                              0xffffffff, 0xffffffff};
>           uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TIMEBASE_FREQ;
>           uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000;
> +        uint32_t page_sizes_prop[64];
> +        size_t page_sizes_prop_size;
>
>           if ((index % smt) != 0) {
>               continue;
> @@ -368,6 +403,13 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>               _FDT((fdt_property_cell(fdt, "ibm,dfp", 1)));
>           }
>
> +        page_sizes_prop_size = create_page_sizes_prop(env, page_sizes_prop,
> +                                                      sizeof(page_sizes_prop));
> +        if (page_sizes_prop_size) {
> +            _FDT((fdt_property(fdt, "ibm,segment-page-sizes",
> +                               page_sizes_prop, page_sizes_prop_size)));
> +        }
> +
>           _FDT((fdt_end_node(fdt)));
>       }
>
> 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?

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

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


Alex