[v3,4/4] spapr: Forbid nested KVM-HV in pre-power9 compat mode

Commit Message

Greg Kurz June 11, 2020, 1:40 p.m. UTC

Nested KVM-HV only works on POWER9.

Signed-off-by: Greg Kurz <[e-mail]>
Reviewed-by: Laurent Vivier <[e-mail]>
---
 hw/ppc/spapr_caps.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

David Gibson June 13, 2020, 7:18 a.m. UTC | #1

On Thu, Jun 11, 2020 at 03:40:33PM +0200, Greg Kurz wrote:
> Nested KVM-HV only works on POWER9.
> 
> Signed-off-by: Greg Kurz <[e-mail]>
> Reviewed-by: Laurent Vivier <[e-mail]>

Hrm.  I have mixed feelings about this.  It does bring forward an
error that we'd otherwise only discover when we try to load the kvm
module in the guest.

On the other hand, it's kind of a layering violation - really it's
KVM's business to report what it can and can't do, rather than having
qemu anticipate it.

Allowing POWER8 compat for an L2 is something we hope to have in the
fairly near future.  Allowing POWER8 compat for L1, which is what this
covers, is, I'll admit, likely to never happen.


> ---
>  HW/ppc/spapr_caps.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 27cf2b38af27..dfe3b419daaa 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -391,6 +391,8 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
>                                      uint8_t val, Error **errp)
>  {
>      ERRP_AUTO_PROPAGATE();
> +    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> +
>      if (!val) {
>          /* capability disabled by default */
>          return;
> @@ -400,6 +402,14 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
>          error_setg(errp, "No Nested KVM-HV support in TCG");
>          error_append_hint(errp, "Try appending -machine cap-nested-hv=off\n");
>      } else if (kvm_enabled()) {
> +        if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00, 0,
> +                              spapr->max_compat_pvr)) {
> +            error_setg(errp, "Nested KVM-HV only supported on POWER9");
> +            error_append_hint(errp,
> +                              "Try appending -machine max-cpu-compat=power9\n");
> +            return;
> +        }
> +
>          if (!kvmppc_has_cap_nested_kvm_hv()) {
>              error_setg(errp,
>                         "KVM implementation does not support Nested KVM-HV");
> 
>

Greg Kurz June 15, 2020, 9:20 a.m. UTC | #2

On Sat, 13 Jun 2020 17:18:04 +1000
David Gibson <[e-mail]> wrote:

> On Thu, Jun 11, 2020 at 03:40:33PM +0200, Greg Kurz wrote:
> > Nested KVM-HV only works on POWER9.
> > 
> > Signed-off-by: Greg Kurz <[e-mail]>
> > Reviewed-by: Laurent Vivier <[e-mail]>
> 
> Hrm.  I have mixed feelings about this.  It does bring forward an
> error that we'd otherwise only discover when we try to load the kvm
> module in the guest.
> 
> On the other hand, it's kind of a layering violation - really it's
> KVM's business to report what it can and can't do, rather than having
> qemu anticipate it.
> 

Agreed and it seems that we can probably get KVM to report that
already. I'll have closer look.

> Allowing POWER8 compat for an L2 is something we hope to have in the
> fairly near future.

Ok but I guess we don't want to start an L2 in compat POWER8 mode
with cap-nested-hv=on, do we ?

>  Allowing POWER8 compat for L1, which is what this
> covers, is, I'll admit, likely to never happen.
> 
> 
> > ---
> >  HW/ppc/spapr_caps.c |   10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index 27cf2b38af27..dfe3b419daaa 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -391,6 +391,8 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
> >                                      uint8_t val, Error **errp)
> >  {
> >      ERRP_AUTO_PROPAGATE();
> > +    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> > +
> >      if (!val) {
> >          /* capability disabled by default */
> >          return;
> > @@ -400,6 +402,14 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
> >          error_setg(errp, "No Nested KVM-HV support in TCG");
> >          error_append_hint(errp, "Try appending -machine cap-nested-hv=off\n");
> >      } else if (kvm_enabled()) {
> > +        if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00, 0,
> > +                              spapr->max_compat_pvr)) {
> > +            error_setg(errp, "Nested KVM-HV only supported on POWER9");
> > +            error_append_hint(errp,
> > +                              "Try appending -machine max-cpu-compat=power9\n");
> > +            return;
> > +        }
> > +
> >          if (!kvmppc_has_cap_nested_kvm_hv()) {
> >              error_setg(errp,
> >                         "KVM implementation does not support Nested KVM-HV");
> > 
> > 
>

David Gibson June 18, 2020, 11:59 p.m. UTC | #3

On Mon, Jun 15, 2020 at 11:20:31AM +0200, Greg Kurz wrote:
> On Sat, 13 Jun 2020 17:18:04 +1000
> David Gibson <[e-mail]> wrote:
> 
> > On Thu, Jun 11, 2020 at 03:40:33PM +0200, Greg Kurz wrote:
> > > Nested KVM-HV only works on POWER9.
> > > 
> > > Signed-off-by: Greg Kurz <[e-mail]>
> > > Reviewed-by: Laurent Vivier <[e-mail]>
> > 
> > Hrm.  I have mixed feelings about this.  It does bring forward an
> > error that we'd otherwise only discover when we try to load the kvm
> > module in the guest.
> > 
> > On the other hand, it's kind of a layering violation - really it's
> > KVM's business to report what it can and can't do, rather than having
> > qemu anticipate it.
> > 
> 
> Agreed and it seems that we can probably get KVM to report that
> already. I'll have closer look.
> 
> > Allowing POWER8 compat for an L2 is something we hope to have in the
> > fairly near future.
> 
> Ok but I guess we don't want to start an L2 in compat POWER8 mode
> with cap-nested-hv=on, do we ?

Sorry, "L2" was misleading, I really mean L<max>.  Setting
cap-nested-hv kind of implies there's a L<at least one more> which
contradicts that.

Greg Kurz July 3, 2020, 2:19 p.m. UTC | #4

On Mon, 15 Jun 2020 11:20:31 +0200
Greg Kurz <[e-mail]> wrote:

> On Sat, 13 Jun 2020 17:18:04 +1000
> David Gibson <[e-mail]> wrote:
> 
> > On Thu, Jun 11, 2020 at 03:40:33PM +0200, Greg Kurz wrote:
> > > Nested KVM-HV only works on POWER9.
> > > 
> > > Signed-off-by: Greg Kurz <[e-mail]>
> > > Reviewed-by: Laurent Vivier <[e-mail]>
> > 
> > Hrm.  I have mixed feelings about this.  It does bring forward an
> > error that we'd otherwise only discover when we try to load the kvm
> > module in the guest.
> > 
> > On the other hand, it's kind of a layering violation - really it's
> > KVM's business to report what it can and can't do, rather than having
> > qemu anticipate it.
> > 
> 
> Agreed and it seems that we can probably get KVM to report that
> already. I'll have closer look.
> 

Checking the KVM_CAP_PPC_NESTED_HV extension only reports what the host
supports. It can't reasonably take into account that we're going to
switch vCPUs in some compat mode later on. KVM could possibly check
that it has a vCPU in pre-power9 compat mode when we try to enable
the capability and fail... but it would be a layering violation all
the same. The KVM that doesn't like pre-power9 CPUs isn't the one in
the host, it is the one in the guest, and it's not even directly
related to the CPU type but to the MMU mode currently in use:

long kvmhv_nested_init(void)
{
	long int ptb_order;
	unsigned long ptcr;
	long rc;

	if (!kvmhv_on_pseries())
		return 0;
==>	if (!radix_enabled())
		return -ENODEV;

We cannot know either for sure the MMU mode the guest will run in
when we enable the nested cap during the initial machine reset.
So it seems we cannot do anything better than denylisting well
known broken setups, in which case QEMU seems a better fit than
KVM.

Makes sense ?

> > Allowing POWER8 compat for an L2 is something we hope to have in the
> > fairly near future.
> 
> Ok but I guess we don't want to start an L2 in compat POWER8 mode
> with cap-nested-hv=on, do we ?
> 
> >  Allowing POWER8 compat for L1, which is what this
> > covers, is, I'll admit, likely to never happen.
> > 
> > 
> > > ---
> > >  HW/ppc/spapr_caps.c |   10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > > index 27cf2b38af27..dfe3b419daaa 100644
> > > --- a/hw/ppc/spapr_caps.c
> > > +++ b/hw/ppc/spapr_caps.c
> > > @@ -391,6 +391,8 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
> > >                                      uint8_t val, Error **errp)
> > >  {
> > >      ERRP_AUTO_PROPAGATE();
> > > +    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> > > +
> > >      if (!val) {
> > >          /* capability disabled by default */
> > >          return;
> > > @@ -400,6 +402,14 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
> > >          error_setg(errp, "No Nested KVM-HV support in TCG");
> > >          error_append_hint(errp, "Try appending -machine cap-nested-hv=off\n");
> > >      } else if (kvm_enabled()) {
> > > +        if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00, 0,
> > > +                              spapr->max_compat_pvr)) {
> > > +            error_setg(errp, "Nested KVM-HV only supported on POWER9");
> > > +            error_append_hint(errp,
> > > +                              "Try appending -machine max-cpu-compat=power9\n");
> > > +            return;
> > > +        }
> > > +
> > >          if (!kvmppc_has_cap_nested_kvm_hv()) {
> > >              error_setg(errp,
> > >                         "KVM implementation does not support Nested KVM-HV");
> > > 
> > > 
> > 
>

David Gibson July 13, 2020, 4:53 a.m. UTC | #5

On Fri, Jul 03, 2020 at 04:19:24PM +0200, Greg Kurz wrote:
> On Mon, 15 Jun 2020 11:20:31 +0200
> Greg Kurz <[e-mail]> wrote:
> 
> > On Sat, 13 Jun 2020 17:18:04 +1000
> > David Gibson <[e-mail]> wrote:
> > 
> > > On Thu, Jun 11, 2020 at 03:40:33PM +0200, Greg Kurz wrote:
> > > > Nested KVM-HV only works on POWER9.
> > > > 
> > > > Signed-off-by: Greg Kurz <[e-mail]>
> > > > Reviewed-by: Laurent Vivier <[e-mail]>
> > > 
> > > Hrm.  I have mixed feelings about this.  It does bring forward an
> > > error that we'd otherwise only discover when we try to load the kvm
> > > module in the guest.
> > > 
> > > On the other hand, it's kind of a layering violation - really it's
> > > KVM's business to report what it can and can't do, rather than having
> > > qemu anticipate it.
> > > 
> > 
> > Agreed and it seems that we can probably get KVM to report that
> > already. I'll have closer look.
> > 
> 
> Checking the KVM_CAP_PPC_NESTED_HV extension only reports what the host
> supports. It can't reasonably take into account that we're going to
> switch vCPUs in some compat mode later on. KVM could possibly check
> that it has a vCPU in pre-power9 compat mode when we try to enable
> the capability and fail... but it would be a layering violation all
> the same. The KVM that doesn't like pre-power9 CPUs isn't the one in
> the host, it is the one in the guest, and it's not even directly
> related to the CPU type but to the MMU mode currently in use:
> 
> long kvmhv_nested_init(void)
> {
> 	long int ptb_order;
> 	unsigned long ptcr;
> 	long rc;
> 
> 	if (!kvmhv_on_pseries())
> 		return 0;
> ==>	if (!radix_enabled())
> 		return -ENODEV;
> 
> We cannot know either for sure the MMU mode the guest will run in
> when we enable the nested cap during the initial machine reset.
> So it seems we cannot do anything better than denylisting well
> known broken setups, in which case QEMU seems a better fit than
> KVM.
> 
> Makes sense ?

Yeah, good points.

Greg Kurz July 15, 2020, 11:14 a.m. UTC | #6

On Mon, 13 Jul 2020 14:53:30 +1000
David Gibson <[e-mail]> wrote:

> On Fri, Jul 03, 2020 at 04:19:24PM +0200, Greg Kurz wrote:
> > On Mon, 15 Jun 2020 11:20:31 +0200
> > Greg Kurz <[e-mail]> wrote:
> > 
> > > On Sat, 13 Jun 2020 17:18:04 +1000
> > > David Gibson <[e-mail]> wrote:
> > > 
> > > > On Thu, Jun 11, 2020 at 03:40:33PM +0200, Greg Kurz wrote:
> > > > > Nested KVM-HV only works on POWER9.
> > > > > 
> > > > > Signed-off-by: Greg Kurz <[e-mail]>
> > > > > Reviewed-by: Laurent Vivier <[e-mail]>
> > > > 
> > > > Hrm.  I have mixed feelings about this.  It does bring forward an
> > > > error that we'd otherwise only discover when we try to load the kvm
> > > > module in the guest.
> > > > 
> > > > On the other hand, it's kind of a layering violation - really it's
> > > > KVM's business to report what it can and can't do, rather than having
> > > > qemu anticipate it.
> > > > 
> > > 
> > > Agreed and it seems that we can probably get KVM to report that
> > > already. I'll have closer look.
> > > 
> > 
> > Checking the KVM_CAP_PPC_NESTED_HV extension only reports what the host
> > supports. It can't reasonably take into account that we're going to
> > switch vCPUs in some compat mode later on. KVM could possibly check
> > that it has a vCPU in pre-power9 compat mode when we try to enable
> > the capability and fail... but it would be a layering violation all
> > the same. The KVM that doesn't like pre-power9 CPUs isn't the one in
> > the host, it is the one in the guest, and it's not even directly
> > related to the CPU type but to the MMU mode currently in use:
> > 
> > long kvmhv_nested_init(void)
> > {
> > 	long int ptb_order;
> > 	unsigned long ptcr;
> > 	long rc;
> > 
> > 	if (!kvmhv_on_pseries())
> > 		return 0;
> > ==>	if (!radix_enabled())
> > 		return -ENODEV;
> > 
> > We cannot know either for sure the MMU mode the guest will run in
> > when we enable the nested cap during the initial machine reset.
> > So it seems we cannot do anything better than denylisting well
> > known broken setups, in which case QEMU seems a better fit than
> > KVM.
> > 
> > Makes sense ?
> 
> Yeah, good points.
> 

So, should I just rebase/repost this or do you think of another
way ?

David Gibson July 15, 2020, 11:26 p.m. UTC | #7

On Wed, Jul 15, 2020 at 01:14:42PM +0200, Greg Kurz wrote:
> On Mon, 13 Jul 2020 14:53:30 +1000
> David Gibson <[e-mail]> wrote:
> 
> > On Fri, Jul 03, 2020 at 04:19:24PM +0200, Greg Kurz wrote:
> > > On Mon, 15 Jun 2020 11:20:31 +0200
> > > Greg Kurz <[e-mail]> wrote:
> > > 
> > > > On Sat, 13 Jun 2020 17:18:04 +1000
> > > > David Gibson <[e-mail]> wrote:
> > > > 
> > > > > On Thu, Jun 11, 2020 at 03:40:33PM +0200, Greg Kurz wrote:
> > > > > > Nested KVM-HV only works on POWER9.
> > > > > > 
> > > > > > Signed-off-by: Greg Kurz <[e-mail]>
> > > > > > Reviewed-by: Laurent Vivier <[e-mail]>
> > > > > 
> > > > > Hrm.  I have mixed feelings about this.  It does bring forward an
> > > > > error that we'd otherwise only discover when we try to load the kvm
> > > > > module in the guest.
> > > > > 
> > > > > On the other hand, it's kind of a layering violation - really it's
> > > > > KVM's business to report what it can and can't do, rather than having
> > > > > qemu anticipate it.
> > > > > 
> > > > 
> > > > Agreed and it seems that we can probably get KVM to report that
> > > > already. I'll have closer look.
> > > > 
> > > 
> > > Checking the KVM_CAP_PPC_NESTED_HV extension only reports what the host
> > > supports. It can't reasonably take into account that we're going to
> > > switch vCPUs in some compat mode later on. KVM could possibly check
> > > that it has a vCPU in pre-power9 compat mode when we try to enable
> > > the capability and fail... but it would be a layering violation all
> > > the same. The KVM that doesn't like pre-power9 CPUs isn't the one in
> > > the host, it is the one in the guest, and it's not even directly
> > > related to the CPU type but to the MMU mode currently in use:
> > > 
> > > long kvmhv_nested_init(void)
> > > {
> > > 	long int ptb_order;
> > > 	unsigned long ptcr;
> > > 	long rc;
> > > 
> > > 	if (!kvmhv_on_pseries())
> > > 		return 0;
> > > ==>	if (!radix_enabled())
> > > 		return -ENODEV;
> > > 
> > > We cannot know either for sure the MMU mode the guest will run in
> > > when we enable the nested cap during the initial machine reset.
> > > So it seems we cannot do anything better than denylisting well
> > > known broken setups, in which case QEMU seems a better fit than
> > > KVM.
> > > 
> > > Makes sense ?
> > 
> > Yeah, good points.
> > 
> 
> So, should I just rebase/repost this or do you think of another
> way ?

Urgh... I've kind of forgotten the context while I've been away.  So,
I guess repost and I'll take another look at them.

Patch

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 27cf2b38af27..dfe3b419daaa 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -391,6 +391,8 @@  static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
                                     uint8_t val, Error **errp)
 {
     ERRP_AUTO_PROPAGATE();
+    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
+
     if (!val) {
         /* capability disabled by default */
         return;
@@ -400,6 +402,14 @@  static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
         error_setg(errp, "No Nested KVM-HV support in TCG");
         error_append_hint(errp, "Try appending -machine cap-nested-hv=off\n");
     } else if (kvm_enabled()) {
+        if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00, 0,
+                              spapr->max_compat_pvr)) {
+            error_setg(errp, "Nested KVM-HV only supported on POWER9");
+            error_append_hint(errp,
+                              "Try appending -machine max-cpu-compat=power9\n");
+            return;
+        }
+
         if (!kvmppc_has_cap_nested_kvm_hv()) {
             error_setg(errp,
                        "KVM implementation does not support Nested KVM-HV");