1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH 0/3] spapr: fix regression with older machine types
  4. View patch

[Qemu-devel] [PATCH 2/3] spapr: compute default value of "hpt-max-page-size" later

Greg Kurz posted 3 patches 7 years, 4 months ago
There is a newer version of this series
[Qemu-devel] [PATCH 2/3] spapr: compute default value of "hpt-max-page-size" later
Posted by Greg Kurz 7 years, 4 months ago 
It is currently not possible to run a pseries-2.12 or older machine
with HV KVM. QEMU prints the following and exits right away.

qemu-system-ppc64: KVM doesn't support for base page shift 34

The "hpt-max-page-size" capability was recently added to spapr to hide
host configuration details from HPT mode guests. Its default value for
newer machine types is 64k.

For backwards compatibility, pseries-2.12 and older machine types need
a different value. This is handled as usual in a class init function.
The default value is 16G, ie, all page sizes supported by POWER7 and
newer CPUs, but HV KVM requires guest pages to be hpa contiguous as
well as gpa contiguous. The default value is the page size used to
back the guest RAM in this case.

Unfortunately kvmppc_hpt_needs_host_contiguous_pages()->kvm_enabled() is
called way before KVM init and returns false, even if the user requested
KVM. We thus end up selecting 16G, which isn't supported by HV KVM.

We fix this by moving the logic to spapr_machine_init() because this
is the earliest call where we're sure kvm_enabled() can be trusted.
Since the user cannot pass cap-hpt-max-page-size=0, we set the default
to 0 in the pseries-2.12 class init function and use that as a flag
in spapr_machine_init() to do the real work.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c |   25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 8cc996d0b822..1eb45cd8c424 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2527,6 +2527,18 @@ static void spapr_machine_init(MachineState *machine)
     QLIST_INIT(&spapr->phbs);
     QTAILQ_INIT(&spapr->pending_dimm_unplugs);
 
+    /* This is for pseries-2.12 and older machine types */
+    if (!smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE]) {
+        uint8_t mps;
+
+        if (kvmppc_hpt_needs_host_contiguous_pages()) {
+            mps = ctz64(qemu_getrampagesize());
+        } else {
+            mps = 34; /* allow everything up to 16GiB, i.e. everything */
+        }
+        smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = mps;
+    }
+
     /* Determine capabilities to run with */
     spapr_caps_init(spapr);
 
@@ -4103,17 +4115,16 @@ static void spapr_machine_2_12_instance_options(MachineState *machine)
 static void spapr_machine_2_12_class_options(MachineClass *mc)
 {
     sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
-    uint8_t mps;
 
     spapr_machine_3_0_class_options(mc);
     SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_12);
 
-    if (kvmppc_hpt_needs_host_contiguous_pages()) {
-        mps = ctz64(qemu_getrampagesize());
-    } else {
-        mps = 34; /* allow everything up to 16GiB, i.e. everything */
-    }
-    smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = mps;
+    /* We depend on kvm_enabled() to choose a default value for the
+     * hpt-max-page-size capability. Of course we can't do it here
+     * because this is too early and the HW accelerator isn't initialzed
+     * yet. Postpone this to spapr_machine_init().
+     */
+    smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = 0;
 }
 
 DEFINE_SPAPR_MACHINE(2_12, "2.12", false);
Re: [Qemu-devel] [PATCH 2/3] spapr: compute default value of "hpt-max-page-size" later
Posted by David Gibson 7 years, 4 months ago 
On Thu, Jun 28, 2018 at 12:15:14PM +0200, Greg Kurz wrote:
> It is currently not possible to run a pseries-2.12 or older machine
> with HV KVM. QEMU prints the following and exits right away.
> 
> qemu-system-ppc64: KVM doesn't support for base page shift 34
> 
> The "hpt-max-page-size" capability was recently added to spapr to hide
> host configuration details from HPT mode guests. Its default value for
> newer machine types is 64k.
> 
> For backwards compatibility, pseries-2.12 and older machine types need
> a different value. This is handled as usual in a class init function.
> The default value is 16G, ie, all page sizes supported by POWER7 and
> newer CPUs, but HV KVM requires guest pages to be hpa contiguous as
> well as gpa contiguous. The default value is the page size used to
> back the guest RAM in this case.
> 
> Unfortunately kvmppc_hpt_needs_host_contiguous_pages()->kvm_enabled() is
> called way before KVM init and returns false, even if the user requested
> KVM. We thus end up selecting 16G, which isn't supported by HV KVM.
> 
> We fix this by moving the logic to spapr_machine_init() because this
> is the earliest call where we're sure kvm_enabled() can be trusted.
> Since the user cannot pass cap-hpt-max-page-size=0, we set the default
> to 0 in the pseries-2.12 class init function and use that as a flag
> in spapr_machine_init() to do the real work.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr.c |   25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 8cc996d0b822..1eb45cd8c424 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2527,6 +2527,18 @@ static void spapr_machine_init(MachineState *machine)
>      QLIST_INIT(&spapr->phbs);
>      QTAILQ_INIT(&spapr->pending_dimm_unplugs);
>  
> +    /* This is for pseries-2.12 and older machine types */
> +    if (!smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE]) {
> +        uint8_t mps;
> +
> +        if (kvmppc_hpt_needs_host_contiguous_pages()) {
> +            mps = ctz64(qemu_getrampagesize());
> +        } else {
> +            mps = 34; /* allow everything up to 16GiB, i.e. everything */
> +        }
> +        smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = mps;
> +    }
> +

The logic above should be folded into spapr_caps_init().

>      /* Determine capabilities to run with */
>      spapr_caps_init(spapr);
>  
> @@ -4103,17 +4115,16 @@ static void spapr_machine_2_12_instance_options(MachineState *machine)
>  static void spapr_machine_2_12_class_options(MachineClass *mc)
>  {
>      sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> -    uint8_t mps;
>  
>      spapr_machine_3_0_class_options(mc);
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_12);
>  
> -    if (kvmppc_hpt_needs_host_contiguous_pages()) {
> -        mps = ctz64(qemu_getrampagesize());
> -    } else {
> -        mps = 34; /* allow everything up to 16GiB, i.e. everything */
> -    }
> -    smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = mps;
> +    /* We depend on kvm_enabled() to choose a default value for the
> +     * hpt-max-page-size capability. Of course we can't do it here
> +     * because this is too early and the HW accelerator isn't initialzed
> +     * yet. Postpone this to spapr_machine_init().
> +     */
> +    smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = 0;
>  }
>  
>  DEFINE_SPAPR_MACHINE(2_12, "2.12", false);
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [Qemu-devel] [PATCH 2/3] spapr: compute default value of "hpt-max-page-size" later
Posted by Eduardo Habkost 7 years, 4 months ago 
On Thu, Jun 28, 2018 at 12:15:14PM +0200, Greg Kurz wrote:
[...]
> @@ -2527,6 +2527,18 @@ static void spapr_machine_init(MachineState *machine)
>      QLIST_INIT(&spapr->phbs);
>      QTAILQ_INIT(&spapr->pending_dimm_unplugs);
>  
> +    /* This is for pseries-2.12 and older machine types */
> +    if (!smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE]) {
> +        uint8_t mps;
> +
> +        if (kvmppc_hpt_needs_host_contiguous_pages()) {
> +            mps = ctz64(qemu_getrampagesize());
> +        } else {
> +            mps = 34; /* allow everything up to 16GiB, i.e. everything */
> +        }
> +        smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = mps;

Initializing class fields on instance init functions is a sign
that the information doesn't belong to the class struct.  Can't
this be handled at default_caps_with_cpu()?

> +    }
> +
>      /* Determine capabilities to run with */
>      spapr_caps_init(spapr);
>  
> @@ -4103,17 +4115,16 @@ static void spapr_machine_2_12_instance_options(MachineState *machine)
>  static void spapr_machine_2_12_class_options(MachineClass *mc)
>  {
>      sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> -    uint8_t mps;
>  
>      spapr_machine_3_0_class_options(mc);
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_12);
>  
> -    if (kvmppc_hpt_needs_host_contiguous_pages()) {
> -        mps = ctz64(qemu_getrampagesize());
> -    } else {
> -        mps = 34; /* allow everything up to 16GiB, i.e. everything */
> -    }
> -    smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = mps;
> +    /* We depend on kvm_enabled() to choose a default value for the
> +     * hpt-max-page-size capability. Of course we can't do it here
> +     * because this is too early and the HW accelerator isn't initialzed
> +     * yet. Postpone this to spapr_machine_init().
> +     */
> +    smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = 0;
>  }
>  
>  DEFINE_SPAPR_MACHINE(2_12, "2.12", false);
> 

-- 
Eduardo
Re: [Qemu-devel] [PATCH 2/3] spapr: compute default value of "hpt-max-page-size" later
Posted by David Gibson 7 years, 4 months ago 
On Fri, Jun 29, 2018 at 04:08:22PM -0300, Eduardo Habkost wrote:
> On Thu, Jun 28, 2018 at 12:15:14PM +0200, Greg Kurz wrote:
> [...]
> > @@ -2527,6 +2527,18 @@ static void spapr_machine_init(MachineState *machine)
> >      QLIST_INIT(&spapr->phbs);
> >      QTAILQ_INIT(&spapr->pending_dimm_unplugs);
> >  
> > +    /* This is for pseries-2.12 and older machine types */
> > +    if (!smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE]) {
> > +        uint8_t mps;
> > +
> > +        if (kvmppc_hpt_needs_host_contiguous_pages()) {
> > +            mps = ctz64(qemu_getrampagesize());
> > +        } else {
> > +            mps = 34; /* allow everything up to 16GiB, i.e. everything */
> > +        }
> > +        smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = mps;
> 
> Initializing class fields on instance init functions is a sign
> that the information doesn't belong to the class struct.  Can't
> this be handled at default_caps_with_cpu()?

Yeah, doing it inside default_caps_with_cpu() probably makes more
sense.

> > +    }
> > +
> >      /* Determine capabilities to run with */
> >      spapr_caps_init(spapr);
> >  
> > @@ -4103,17 +4115,16 @@ static void spapr_machine_2_12_instance_options(MachineState *machine)
> >  static void spapr_machine_2_12_class_options(MachineClass *mc)
> >  {
> >      sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > -    uint8_t mps;
> >  
> >      spapr_machine_3_0_class_options(mc);
> >      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_12);
> >  
> > -    if (kvmppc_hpt_needs_host_contiguous_pages()) {
> > -        mps = ctz64(qemu_getrampagesize());
> > -    } else {
> > -        mps = 34; /* allow everything up to 16GiB, i.e. everything */
> > -    }
> > -    smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = mps;
> > +    /* We depend on kvm_enabled() to choose a default value for the
> > +     * hpt-max-page-size capability. Of course we can't do it here
> > +     * because this is too early and the HW accelerator isn't initialzed
> > +     * yet. Postpone this to spapr_machine_init().
> > +     */
> > +    smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = 0;
> >  }
> >  
> >  DEFINE_SPAPR_MACHINE(2_12, "2.12", false);
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.