diff mbox series

[3/3] ppc/kvm: check some capabilities with kvm_vm_check_extension()

Message ID 150541714343.1616.11893912014879112098.stgit@bahia.lan
State New
Headers show
Series kvm: use kvm_vm_check_extension() with VM capabilities | expand

Commit Message

Greg Kurz Sept. 14, 2017, 7:25 p.m. UTC 
The following capabilities are VM specific:
- KVM_CAP_PPC_SMT_POSSIBLE
- KVM_CAP_PPC_HTAB_FD
- KVM_CAP_PPC_ALLOC_HTAB

If both KVM HV and KVM PR are present, checking them always return
the HV value, even if we explicitely requested to use PR.

This has no visible effect for KVM_CAP_PPC_ALLOC_HTAB, because we also
try the KVM_PPC_ALLOCATE_HTAB ioctl which is only suppored by HV. As
a consequence, the spapr code doesn't even check KVM_CAP_PPC_HTAB_FD.

However, this will cause kvmppc_hint_smt_possible(), introduced by
commit fa98fbfcdfcb9, to report several VSMT modes (eg, Available
VSMT modes: 8 4 2 1) whereas PR only support mode 1.

This patch fixes all three anyway to use kvm_vm_check_extension(). It
is okay since the VM is already created at the time kvm_arch_init() or
kvmppc_reset_htab() is called.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 target/ppc/kvm.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Thomas Huth Sept. 15, 2017, 5:15 a.m. UTC | #1 
On 14.09.2017 21:25, Greg Kurz wrote:
> The following capabilities are VM specific:
> - KVM_CAP_PPC_SMT_POSSIBLE
> - KVM_CAP_PPC_HTAB_FD

BTW, looks like kvmppc_has_cap_htab_fd() is dead code ... should we
either remove it or check it somewhere?

> - KVM_CAP_PPC_ALLOC_HTAB
> 
> If both KVM HV and KVM PR are present, checking them always return
> the HV value, even if we explicitely requested to use PR.
> 
> This has no visible effect for KVM_CAP_PPC_ALLOC_HTAB, because we also
> try the KVM_PPC_ALLOCATE_HTAB ioctl which is only suppored by HV. As
> a consequence, the spapr code doesn't even check KVM_CAP_PPC_HTAB_FD.
>
> However, this will cause kvmppc_hint_smt_possible(), introduced by
> commit fa98fbfcdfcb9, to report several VSMT modes (eg, Available
> VSMT modes: 8 4 2 1) whereas PR only support mode 1.
> 
> This patch fixes all three anyway to use kvm_vm_check_extension(). It
> is okay since the VM is already created at the time kvm_arch_init() or
> kvmppc_reset_htab() is called.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  target/ppc/kvm.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 1deaf106d2b9..208c70e81426 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -131,7 +131,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_interrupt_level = kvm_check_extension(s, KVM_CAP_PPC_IRQ_LEVEL);
>      cap_segstate = kvm_check_extension(s, KVM_CAP_PPC_SEGSTATE);
>      cap_booke_sregs = kvm_check_extension(s, KVM_CAP_PPC_BOOKE_SREGS);
> -    cap_ppc_smt_possible = kvm_check_extension(s, KVM_CAP_PPC_SMT_POSSIBLE);
> +    cap_ppc_smt_possible = kvm_vm_check_extension(s, KVM_CAP_PPC_SMT_POSSIBLE);
>      cap_ppc_rma = kvm_check_extension(s, KVM_CAP_PPC_RMA);
>      cap_spapr_tce = kvm_check_extension(s, KVM_CAP_SPAPR_TCE);
>      cap_spapr_tce_64 = kvm_check_extension(s, KVM_CAP_SPAPR_TCE_64);
> @@ -143,7 +143,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_ppc_watchdog = kvm_check_extension(s, KVM_CAP_PPC_BOOKE_WATCHDOG);
>      /* Note: we don't set cap_papr here, because this capability is
>       * only activated after this by kvmppc_set_papr() */
> -    cap_htab_fd = kvm_check_extension(s, KVM_CAP_PPC_HTAB_FD);
> +    cap_htab_fd = kvm_vm_check_extension(s, KVM_CAP_PPC_HTAB_FD);
>      cap_fixup_hcalls = kvm_check_extension(s, KVM_CAP_PPC_FIXUP_HCALL);
>      cap_ppc_smt = kvm_vm_check_extension(s, KVM_CAP_PPC_SMT);
>      cap_htm = kvm_vm_check_extension(s, KVM_CAP_PPC_HTM);
> @@ -2353,7 +2353,7 @@ int kvmppc_reset_htab(int shift_hint)
>          /* Full emulation, tell caller to allocate htab itself */
>          return 0;
>      }
> -    if (kvm_check_extension(kvm_state, KVM_CAP_PPC_ALLOC_HTAB)) {
> +    if (kvm_vm_check_extension(kvm_state, KVM_CAP_PPC_ALLOC_HTAB)) {
>          int ret;
>          ret = kvm_vm_ioctl(kvm_state, KVM_PPC_ALLOCATE_HTAB, &shift);
>          if (ret == -ENOTTY) {

Looking at the comment in the code after the "if (ret == -ENOTTY)" line,
it sounds like there is a bug in the kernel and the
KVM_CAP_PPC_ALLOC_HTAB should depend on the hv_enabled variable, too?
Anyway, that's another topic, your patch is fine!

Reviewed-by: Thomas Huth <thuth@redhat.com>
David Gibson Sept. 15, 2017, 6:34 a.m. UTC | #2 
On Thu, Sep 14, 2017 at 09:25:43PM +0200, Greg Kurz wrote:
> The following capabilities are VM specific:
> - KVM_CAP_PPC_SMT_POSSIBLE
> - KVM_CAP_PPC_HTAB_FD
> - KVM_CAP_PPC_ALLOC_HTAB
> 
> If both KVM HV and KVM PR are present, checking them always return
> the HV value, even if we explicitely requested to use PR.
> 
> This has no visible effect for KVM_CAP_PPC_ALLOC_HTAB, because we also
> try the KVM_PPC_ALLOCATE_HTAB ioctl which is only suppored by HV. As
> a consequence, the spapr code doesn't even check KVM_CAP_PPC_HTAB_FD.
> 
> However, this will cause kvmppc_hint_smt_possible(), introduced by
> commit fa98fbfcdfcb9, to report several VSMT modes (eg, Available
> VSMT modes: 8 4 2 1) whereas PR only support mode 1.
> 
> This patch fixes all three anyway to use kvm_vm_check_extension(). It
> is okay since the VM is already created at the time kvm_arch_init() or
> kvmppc_reset_htab() is called.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-2.11.

> ---
>  target/ppc/kvm.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 1deaf106d2b9..208c70e81426 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -131,7 +131,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_interrupt_level = kvm_check_extension(s, KVM_CAP_PPC_IRQ_LEVEL);
>      cap_segstate = kvm_check_extension(s, KVM_CAP_PPC_SEGSTATE);
>      cap_booke_sregs = kvm_check_extension(s, KVM_CAP_PPC_BOOKE_SREGS);
> -    cap_ppc_smt_possible = kvm_check_extension(s, KVM_CAP_PPC_SMT_POSSIBLE);
> +    cap_ppc_smt_possible = kvm_vm_check_extension(s, KVM_CAP_PPC_SMT_POSSIBLE);
>      cap_ppc_rma = kvm_check_extension(s, KVM_CAP_PPC_RMA);
>      cap_spapr_tce = kvm_check_extension(s, KVM_CAP_SPAPR_TCE);
>      cap_spapr_tce_64 = kvm_check_extension(s, KVM_CAP_SPAPR_TCE_64);
> @@ -143,7 +143,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_ppc_watchdog = kvm_check_extension(s, KVM_CAP_PPC_BOOKE_WATCHDOG);
>      /* Note: we don't set cap_papr here, because this capability is
>       * only activated after this by kvmppc_set_papr() */
> -    cap_htab_fd = kvm_check_extension(s, KVM_CAP_PPC_HTAB_FD);
> +    cap_htab_fd = kvm_vm_check_extension(s, KVM_CAP_PPC_HTAB_FD);
>      cap_fixup_hcalls = kvm_check_extension(s, KVM_CAP_PPC_FIXUP_HCALL);
>      cap_ppc_smt = kvm_vm_check_extension(s, KVM_CAP_PPC_SMT);
>      cap_htm = kvm_vm_check_extension(s, KVM_CAP_PPC_HTM);
> @@ -2353,7 +2353,7 @@ int kvmppc_reset_htab(int shift_hint)
>          /* Full emulation, tell caller to allocate htab itself */
>          return 0;
>      }
> -    if (kvm_check_extension(kvm_state, KVM_CAP_PPC_ALLOC_HTAB)) {
> +    if (kvm_vm_check_extension(kvm_state, KVM_CAP_PPC_ALLOC_HTAB)) {
>          int ret;
>          ret = kvm_vm_ioctl(kvm_state, KVM_PPC_ALLOCATE_HTAB, &shift);
>          if (ret == -ENOTTY) {
>
David Gibson Sept. 15, 2017, 6:35 a.m. UTC | #3 
On Fri, Sep 15, 2017 at 07:15:41AM +0200, Thomas Huth wrote:
> On 14.09.2017 21:25, Greg Kurz wrote:
> > The following capabilities are VM specific:
> > - KVM_CAP_PPC_SMT_POSSIBLE
> > - KVM_CAP_PPC_HTAB_FD
> 
> BTW, looks like kvmppc_has_cap_htab_fd() is dead code ... should we
> either remove it or check it somewhere?

It's checked in kvmppc_get_htab_fd().
Thomas Huth Sept. 15, 2017, 8:18 a.m. UTC | #4 
On 15.09.2017 08:35, David Gibson wrote:
> On Fri, Sep 15, 2017 at 07:15:41AM +0200, Thomas Huth wrote:
>> On 14.09.2017 21:25, Greg Kurz wrote:
>>> The following capabilities are VM specific:
>>> - KVM_CAP_PPC_SMT_POSSIBLE
>>> - KVM_CAP_PPC_HTAB_FD
>>
>> BTW, looks like kvmppc_has_cap_htab_fd() is dead code ... should we
>> either remove it or check it somewhere?
> 
> It's checked in kvmppc_get_htab_fd().

That function checks the "cap_htab_fd" variable, but is not using the
kvmppc_has_cap_htab_fd() function which I was referring to.

 Thomas
David Gibson Sept. 15, 2017, 8:39 a.m. UTC | #5 
On Fri, Sep 15, 2017 at 10:18:49AM +0200, Thomas Huth wrote:
> On 15.09.2017 08:35, David Gibson wrote:
> > On Fri, Sep 15, 2017 at 07:15:41AM +0200, Thomas Huth wrote:
> >> On 14.09.2017 21:25, Greg Kurz wrote:
> >>> The following capabilities are VM specific:
> >>> - KVM_CAP_PPC_SMT_POSSIBLE
> >>> - KVM_CAP_PPC_HTAB_FD
> >>
> >> BTW, looks like kvmppc_has_cap_htab_fd() is dead code ... should we
> >> either remove it or check it somewhere?
> > 
> > It's checked in kvmppc_get_htab_fd().
> 
> That function checks the "cap_htab_fd" variable, but is not using the
> kvmppc_has_cap_htab_fd() function which I was referring to.

Oh, sorry, good point.

We should remove it.
Greg Kurz Sept. 15, 2017, 8:43 a.m. UTC | #6 
On Fri, 15 Sep 2017 07:15:41 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 14.09.2017 21:25, Greg Kurz wrote:
> > The following capabilities are VM specific:
> > - KVM_CAP_PPC_SMT_POSSIBLE
> > - KVM_CAP_PPC_HTAB_FD  
> 
> BTW, looks like kvmppc_has_cap_htab_fd() is dead code ... should we
> either remove it or check it somewhere?
> 

Heh you're right :)

The only user for cap_htab_fd is kvmppc_get_htab_fd(), in order to provide a
sensible error message that KVM doesn't allow saving HPTEs (ie, migration isn't
supported with some older KVM HV). It doesn't need kvmppc_has_cap_htab_fd() for
that since all the code sits in target/ppc/kvm.c.

I can't think of any other use, so I guess we can drop it.

> > - KVM_CAP_PPC_ALLOC_HTAB
> > 
> > If both KVM HV and KVM PR are present, checking them always return
> > the HV value, even if we explicitely requested to use PR.
> > 
> > This has no visible effect for KVM_CAP_PPC_ALLOC_HTAB, because we also
> > try the KVM_PPC_ALLOCATE_HTAB ioctl which is only suppored by HV. As
> > a consequence, the spapr code doesn't even check KVM_CAP_PPC_HTAB_FD.
> >
> > However, this will cause kvmppc_hint_smt_possible(), introduced by
> > commit fa98fbfcdfcb9, to report several VSMT modes (eg, Available
> > VSMT modes: 8 4 2 1) whereas PR only support mode 1.
> > 
> > This patch fixes all three anyway to use kvm_vm_check_extension(). It
> > is okay since the VM is already created at the time kvm_arch_init() or
> > kvmppc_reset_htab() is called.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  target/ppc/kvm.c |    6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 1deaf106d2b9..208c70e81426 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -131,7 +131,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> >      cap_interrupt_level = kvm_check_extension(s, KVM_CAP_PPC_IRQ_LEVEL);
> >      cap_segstate = kvm_check_extension(s, KVM_CAP_PPC_SEGSTATE);
> >      cap_booke_sregs = kvm_check_extension(s, KVM_CAP_PPC_BOOKE_SREGS);
> > -    cap_ppc_smt_possible = kvm_check_extension(s, KVM_CAP_PPC_SMT_POSSIBLE);
> > +    cap_ppc_smt_possible = kvm_vm_check_extension(s, KVM_CAP_PPC_SMT_POSSIBLE);
> >      cap_ppc_rma = kvm_check_extension(s, KVM_CAP_PPC_RMA);
> >      cap_spapr_tce = kvm_check_extension(s, KVM_CAP_SPAPR_TCE);
> >      cap_spapr_tce_64 = kvm_check_extension(s, KVM_CAP_SPAPR_TCE_64);
> > @@ -143,7 +143,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> >      cap_ppc_watchdog = kvm_check_extension(s, KVM_CAP_PPC_BOOKE_WATCHDOG);
> >      /* Note: we don't set cap_papr here, because this capability is
> >       * only activated after this by kvmppc_set_papr() */
> > -    cap_htab_fd = kvm_check_extension(s, KVM_CAP_PPC_HTAB_FD);
> > +    cap_htab_fd = kvm_vm_check_extension(s, KVM_CAP_PPC_HTAB_FD);
> >      cap_fixup_hcalls = kvm_check_extension(s, KVM_CAP_PPC_FIXUP_HCALL);
> >      cap_ppc_smt = kvm_vm_check_extension(s, KVM_CAP_PPC_SMT);
> >      cap_htm = kvm_vm_check_extension(s, KVM_CAP_PPC_HTM);
> > @@ -2353,7 +2353,7 @@ int kvmppc_reset_htab(int shift_hint)
> >          /* Full emulation, tell caller to allocate htab itself */
> >          return 0;
> >      }
> > -    if (kvm_check_extension(kvm_state, KVM_CAP_PPC_ALLOC_HTAB)) {
> > +    if (kvm_vm_check_extension(kvm_state, KVM_CAP_PPC_ALLOC_HTAB)) {
> >          int ret;
> >          ret = kvm_vm_ioctl(kvm_state, KVM_PPC_ALLOCATE_HTAB, &shift);
> >          if (ret == -ENOTTY) {  
> 
> Looking at the comment in the code after the "if (ret == -ENOTTY)" line,
> it sounds like there is a bug in the kernel and the
> KVM_CAP_PPC_ALLOC_HTAB should depend on the hv_enabled variable, too?

It already does :)

commit a8acaece5d88db234d0b82b8692dea15d602f622
Author: David Gibson <david@gibson.dropbear.id.au>
Date:   Wed Nov 23 16:14:07 2016 +1100

    KVM: PPC: Correctly report KVM_CAP_PPC_ALLOC_HTAB
    
    At present KVM on powerpc always reports KVM_CAP_PPC_ALLOC_HTAB as enabled.
    However, the ioctl() it advertises (KVM_PPC_ALLOCATE_HTAB) only actually
    works on KVM HV.  On KVM PR it will fail with ENOTTY.
    
    QEMU already has a workaround for this, so it's not breaking things in
    practice, but it would be better to advertise this correctly.
    
    Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
    Signed-off-by: Paul Mackerras <paulus@ozlabs.org>

> Anyway, that's another topic, your patch is fine!
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
Thomas Huth Sept. 15, 2017, 8:52 a.m. UTC | #7 
On 15.09.2017 10:43, Greg Kurz wrote:
> On Fri, 15 Sep 2017 07:15:41 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 14.09.2017 21:25, Greg Kurz wrote:
>>> The following capabilities are VM specific:
>>> - KVM_CAP_PPC_SMT_POSSIBLE
>>> - KVM_CAP_PPC_HTAB_FD  
>>
>> BTW, looks like kvmppc_has_cap_htab_fd() is dead code ... should we
>> either remove it or check it somewhere?
>>
> 
> Heh you're right :)
> 
> The only user for cap_htab_fd is kvmppc_get_htab_fd(), in order to provide a
> sensible error message that KVM doesn't allow saving HPTEs (ie, migration isn't
> supported with some older KVM HV). It doesn't need kvmppc_has_cap_htab_fd() for
> that since all the code sits in target/ppc/kvm.c.
> 
> I can't think of any other use, so I guess we can drop it.

OK. If you've got some spare minutes, could you maybe send a patch?

[...]
>>> @@ -2353,7 +2353,7 @@ int kvmppc_reset_htab(int shift_hint)
>>>          /* Full emulation, tell caller to allocate htab itself */
>>>          return 0;
>>>      }
>>> -    if (kvm_check_extension(kvm_state, KVM_CAP_PPC_ALLOC_HTAB)) {
>>> +    if (kvm_vm_check_extension(kvm_state, KVM_CAP_PPC_ALLOC_HTAB)) {
>>>          int ret;
>>>          ret = kvm_vm_ioctl(kvm_state, KVM_PPC_ALLOCATE_HTAB, &shift);
>>>          if (ret == -ENOTTY) {  
>>
>> Looking at the comment in the code after the "if (ret == -ENOTTY)" line,
>> it sounds like there is a bug in the kernel and the
>> KVM_CAP_PPC_ALLOC_HTAB should depend on the hv_enabled variable, too?
> 
> It already does :)
> 
> commit a8acaece5d88db234d0b82b8692dea15d602f622

Oh, right, seems like I was looking at an older kernel branch :-/ So
never mind, we're fine here.

 Thomas
diff mbox series

Patch

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 1deaf106d2b9..208c70e81426 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -131,7 +131,7 @@  int kvm_arch_init(MachineState *ms, KVMState *s)
     cap_interrupt_level = kvm_check_extension(s, KVM_CAP_PPC_IRQ_LEVEL);
     cap_segstate = kvm_check_extension(s, KVM_CAP_PPC_SEGSTATE);
     cap_booke_sregs = kvm_check_extension(s, KVM_CAP_PPC_BOOKE_SREGS);
-    cap_ppc_smt_possible = kvm_check_extension(s, KVM_CAP_PPC_SMT_POSSIBLE);
+    cap_ppc_smt_possible = kvm_vm_check_extension(s, KVM_CAP_PPC_SMT_POSSIBLE);
     cap_ppc_rma = kvm_check_extension(s, KVM_CAP_PPC_RMA);
     cap_spapr_tce = kvm_check_extension(s, KVM_CAP_SPAPR_TCE);
     cap_spapr_tce_64 = kvm_check_extension(s, KVM_CAP_SPAPR_TCE_64);
@@ -143,7 +143,7 @@  int kvm_arch_init(MachineState *ms, KVMState *s)
     cap_ppc_watchdog = kvm_check_extension(s, KVM_CAP_PPC_BOOKE_WATCHDOG);
     /* Note: we don't set cap_papr here, because this capability is
      * only activated after this by kvmppc_set_papr() */
-    cap_htab_fd = kvm_check_extension(s, KVM_CAP_PPC_HTAB_FD);
+    cap_htab_fd = kvm_vm_check_extension(s, KVM_CAP_PPC_HTAB_FD);
     cap_fixup_hcalls = kvm_check_extension(s, KVM_CAP_PPC_FIXUP_HCALL);
     cap_ppc_smt = kvm_vm_check_extension(s, KVM_CAP_PPC_SMT);
     cap_htm = kvm_vm_check_extension(s, KVM_CAP_PPC_HTM);
@@ -2353,7 +2353,7 @@  int kvmppc_reset_htab(int shift_hint)
         /* Full emulation, tell caller to allocate htab itself */
         return 0;
     }
-    if (kvm_check_extension(kvm_state, KVM_CAP_PPC_ALLOC_HTAB)) {
+    if (kvm_vm_check_extension(kvm_state, KVM_CAP_PPC_ALLOC_HTAB)) {
         int ret;
         ret = kvm_vm_ioctl(kvm_state, KVM_PPC_ALLOCATE_HTAB, &shift);
         if (ret == -ENOTTY) {