diff mbox

[1/3] hw/ppc/spapr: Move code related to "ibm, pa-features" to a separate function

Message ID 1475061390-17644-2-git-send-email-thuth@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Thomas Huth Sept. 28, 2016, 11:16 a.m. UTC
The function spapr_populate_cpu_dt() has become quite big
already, and since we likely have to extend the pa-features
property for every new processor generation, it is nicer
if we put the related code into a separate function.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c | 66 ++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 36 insertions(+), 30 deletions(-)

Comments

Cédric Le Goater Sept. 28, 2016, 1:07 p.m. UTC | #1
On 09/28/2016 01:16 PM, Thomas Huth wrote:
> The function spapr_populate_cpu_dt() has become quite big
> already, and since we likely have to extend the pa-features
> property for every new processor generation, it is nicer
> if we put the related code into a separate function.

Looks good to me,

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C. 

(It would be really nice to have a routine to build pa_features 
in a less cryptic way. )


> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/ppc/spapr.c | 66 ++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 36 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 648576e..7ac775d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -546,6 +546,41 @@ static int spapr_populate_memory(sPAPRMachineState *spapr, void *fdt)
>      return 0;
>  }
>  
> +/* Populate the "ibm,pa-features" property */
> +static void spapr_populate_pa_features(CPUPPCState *env, void *fdt, int offset)
> +{
> +    uint8_t pa_features_206[] = { 6, 0,
> +        0xf6, 0x1f, 0xc7, 0x00, 0x80, 0xc0 };
> +    uint8_t pa_features_207[] = { 24, 0,
> +        0xf6, 0x1f, 0xc7, 0xc0, 0x80, 0xf0,
> +        0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
> +        0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
> +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00 };
> +    uint8_t *pa_features;
> +    size_t pa_size;
> +
> +    if (env->mmu_model == POWERPC_MMU_2_06) {
> +        pa_features = pa_features_206;
> +        pa_size = sizeof(pa_features_206);
> +    } else { /* env->mmu_model == POWERPC_MMU_2_07 */
> +        pa_features = pa_features_207;
> +        pa_size = sizeof(pa_features_207);
> +    }
> +
> +    if (env->ci_large_pages) {
> +        /*
> +         * Note: we keep CI large pages off by default because a 64K capable
> +         * guest provisioned with large pages might otherwise try to map a qemu
> +         * framebuffer (or other kind of memory mapped PCI BAR) using 64K pages
> +         * even if that qemu runs on a 4k host.
> +         * We dd this bit back here if we are confident this is not an issue
> +         */
> +        pa_features[3] |= 0x20;
> +    }
> +
> +    _FDT((fdt_setprop(fdt, offset, "ibm,pa-features", pa_features, pa_size)));
> +}
> +
>  static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>                                    sPAPRMachineState *spapr)
>  {
> @@ -573,24 +608,6 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>          _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
>      }
>  
> -    /* Note: we keep CI large pages off for now because a 64K capable guest
> -     * provisioned with large pages might otherwise try to map a qemu
> -     * framebuffer (or other kind of memory mapped PCI BAR) using 64K pages
> -     * even if that qemu runs on a 4k host.
> -     *
> -     * We can later add this bit back when we are confident this is not
> -     * an issue (!HV KVM or 64K host)
> -     */
> -    uint8_t pa_features_206[] = { 6, 0,
> -        0xf6, 0x1f, 0xc7, 0x00, 0x80, 0xc0 };
> -    uint8_t pa_features_207[] = { 24, 0,
> -        0xf6, 0x1f, 0xc7, 0xc0, 0x80, 0xf0,
> -        0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
> -        0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
> -        0x80, 0x00, 0x80, 0x00, 0x80, 0x00 };
> -    uint8_t *pa_features;
> -    size_t pa_size;
> -
>      _FDT((fdt_setprop_cell(fdt, offset, "reg", index)));
>      _FDT((fdt_setprop_string(fdt, offset, "device_type", "cpu")));
>  
> @@ -657,18 +674,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>                            page_sizes_prop, page_sizes_prop_size)));
>      }
>  
> -    /* Do the ibm,pa-features property, adjust it for ci-large-pages */
> -    if (env->mmu_model == POWERPC_MMU_2_06) {
> -        pa_features = pa_features_206;
> -        pa_size = sizeof(pa_features_206);
> -    } else /* env->mmu_model == POWERPC_MMU_2_07 */ {
> -        pa_features = pa_features_207;
> -        pa_size = sizeof(pa_features_207);
> -    }
> -    if (env->ci_large_pages) {
> -        pa_features[3] |= 0x20;
> -    }
> -    _FDT((fdt_setprop(fdt, offset, "ibm,pa-features", pa_features, pa_size)));
> +    spapr_populate_pa_features(env, fdt, offset);
>  
>      _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
>                             cs->cpu_index / vcpus_per_socket)));
>
David Gibson Sept. 29, 2016, 1:14 a.m. UTC | #2
On Wed, Sep 28, 2016 at 03:07:31PM +0200, Cédric Le Goater wrote:
> On 09/28/2016 01:16 PM, Thomas Huth wrote:
> > The function spapr_populate_cpu_dt() has become quite big
> > already, and since we likely have to extend the pa-features
> > property for every new processor generation, it is nicer
> > if we put the related code into a separate function.
> 
> Looks good to me,
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C. 
> 
> (It would be really nice to have a routine to build pa_features 
> in a less cryptic way. )

Applied, thanks.
diff mbox

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 648576e..7ac775d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -546,6 +546,41 @@  static int spapr_populate_memory(sPAPRMachineState *spapr, void *fdt)
     return 0;
 }
 
+/* Populate the "ibm,pa-features" property */
+static void spapr_populate_pa_features(CPUPPCState *env, void *fdt, int offset)
+{
+    uint8_t pa_features_206[] = { 6, 0,
+        0xf6, 0x1f, 0xc7, 0x00, 0x80, 0xc0 };
+    uint8_t pa_features_207[] = { 24, 0,
+        0xf6, 0x1f, 0xc7, 0xc0, 0x80, 0xf0,
+        0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
+        0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
+        0x80, 0x00, 0x80, 0x00, 0x80, 0x00 };
+    uint8_t *pa_features;
+    size_t pa_size;
+
+    if (env->mmu_model == POWERPC_MMU_2_06) {
+        pa_features = pa_features_206;
+        pa_size = sizeof(pa_features_206);
+    } else { /* env->mmu_model == POWERPC_MMU_2_07 */
+        pa_features = pa_features_207;
+        pa_size = sizeof(pa_features_207);
+    }
+
+    if (env->ci_large_pages) {
+        /*
+         * Note: we keep CI large pages off by default because a 64K capable
+         * guest provisioned with large pages might otherwise try to map a qemu
+         * framebuffer (or other kind of memory mapped PCI BAR) using 64K pages
+         * even if that qemu runs on a 4k host.
+         * We dd this bit back here if we are confident this is not an issue
+         */
+        pa_features[3] |= 0x20;
+    }
+
+    _FDT((fdt_setprop(fdt, offset, "ibm,pa-features", pa_features, pa_size)));
+}
+
 static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
                                   sPAPRMachineState *spapr)
 {
@@ -573,24 +608,6 @@  static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
         _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
     }
 
-    /* Note: we keep CI large pages off for now because a 64K capable guest
-     * provisioned with large pages might otherwise try to map a qemu
-     * framebuffer (or other kind of memory mapped PCI BAR) using 64K pages
-     * even if that qemu runs on a 4k host.
-     *
-     * We can later add this bit back when we are confident this is not
-     * an issue (!HV KVM or 64K host)
-     */
-    uint8_t pa_features_206[] = { 6, 0,
-        0xf6, 0x1f, 0xc7, 0x00, 0x80, 0xc0 };
-    uint8_t pa_features_207[] = { 24, 0,
-        0xf6, 0x1f, 0xc7, 0xc0, 0x80, 0xf0,
-        0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
-        0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
-        0x80, 0x00, 0x80, 0x00, 0x80, 0x00 };
-    uint8_t *pa_features;
-    size_t pa_size;
-
     _FDT((fdt_setprop_cell(fdt, offset, "reg", index)));
     _FDT((fdt_setprop_string(fdt, offset, "device_type", "cpu")));
 
@@ -657,18 +674,7 @@  static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
                           page_sizes_prop, page_sizes_prop_size)));
     }
 
-    /* Do the ibm,pa-features property, adjust it for ci-large-pages */
-    if (env->mmu_model == POWERPC_MMU_2_06) {
-        pa_features = pa_features_206;
-        pa_size = sizeof(pa_features_206);
-    } else /* env->mmu_model == POWERPC_MMU_2_07 */ {
-        pa_features = pa_features_207;
-        pa_size = sizeof(pa_features_207);
-    }
-    if (env->ci_large_pages) {
-        pa_features[3] |= 0x20;
-    }
-    _FDT((fdt_setprop(fdt, offset, "ibm,pa-features", pa_features, pa_size)));
+    spapr_populate_pa_features(env, fdt, offset);
 
     _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
                            cs->cpu_index / vcpus_per_socket)));