Re: [Qemu-ppc] [PATCH 3/3] ppc/kvm: check some capabilities with kvm_vm_check_extension()

[Greg Kurz]

On Fri, 15 Sep 2017 07:15:41 +0200
Thomas Huth <address@hidden> 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 <address@hidden>
> > ---
> >  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 <address@hidden>
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 <address@hidden>
    Signed-off-by: Paul Mackerras <address@hidden>

> Anyway, that's another topic, your patch is fine!
> 
> Reviewed-by: Thomas Huth <address@hidden>

pgpCOPL3687xW.pgp
Description: OpenPGP digital signature