On a server-class ppc host, this capability depends on the KVM type,
ie, HV or PR. If both KVM are present in the kernel, we will always
get the HV specific value, even if we explicitely requested PR on
the command line.

This can have an impact if we're using hugepages or a balloon device.

Since we've already created the VM at the time any user calls
kvm_has_sync_mmu(), switching to kvm_vm_check_extension() is
enough to fix any potential issue.

It is okay for the other archs that also implement KVM_CAP_SYNC_MMU,
ie, mips, s390, x86 and arm, because they don't depend on the VM being
created or not.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 accel/kvm/kvm-all.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index f85553a85194..323c567cfb68 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2234,7 +2234,7 @@ int kvm_device_access(int fd, int group, uint64_t attr,
 /* Return 1 on success, 0 on failure */
 int kvm_has_sync_mmu(void)
 {
-    return kvm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
+    return kvm_vm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
 }
 
 int kvm_has_vcpu_events(void)

On Thu, Sep 14, 2017 at 09:25:22PM +0200, Greg Kurz wrote:
> On a server-class ppc host, this capability depends on the KVM type,
> ie, HV or PR. If both KVM are present in the kernel, we will always
> get the HV specific value, even if we explicitely requested PR on
> the command line.
> 
> This can have an impact if we're using hugepages or a balloon device.
> 
> Since we've already created the VM at the time any user calls
> kvm_has_sync_mmu(), switching to kvm_vm_check_extension() is
> enough to fix any potential issue.
> 
> It is okay for the other archs that also implement KVM_CAP_SYNC_MMU,
> ie, mips, s390, x86 and arm, because they don't depend on the VM being
> created or not.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  accel/kvm/kvm-all.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index f85553a85194..323c567cfb68 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2234,7 +2234,7 @@ int kvm_device_access(int fd, int group, uint64_t attr,
>  /* Return 1 on success, 0 on failure */
>  int kvm_has_sync_mmu(void)
>  {
> -    return kvm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
> +    return kvm_vm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
>  }
>  
>  int kvm_has_vcpu_events(void)
> 

-- 
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

On 14.09.2017 21:25, Greg Kurz wrote:
> On a server-class ppc host, this capability depends on the KVM type,
> ie, HV or PR. If both KVM are present in the kernel, we will always
> get the HV specific value, even if we explicitely requested PR on
> the command line.
> 
> This can have an impact if we're using hugepages or a balloon device.
> 
> Since we've already created the VM at the time any user calls
> kvm_has_sync_mmu(), switching to kvm_vm_check_extension() is
> enough to fix any potential issue.
> 
> It is okay for the other archs that also implement KVM_CAP_SYNC_MMU,
> ie, mips, s390, x86 and arm, because they don't depend on the VM being
> created or not.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  accel/kvm/kvm-all.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index f85553a85194..323c567cfb68 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2234,7 +2234,7 @@ int kvm_device_access(int fd, int group, uint64_t attr,
>  /* Return 1 on success, 0 on failure */
>  int kvm_has_sync_mmu(void)
>  {
> -    return kvm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
> +    return kvm_vm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
>  }

Reviewed-by: Thomas Huth <thuth@redhat.com>

... but while you're at it, maybe it would be better to use a bool
variable for the state of this extension, too, and only check for the
extension one time at the end of kvm_init() ? kvm_has_sync_mmu() is
apparently used multiple times in other source files, so we might be
able to save some cycles by doing the syscall only once?

 Thomas

On Fri, 15 Sep 2017 07:32:10 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 14.09.2017 21:25, Greg Kurz wrote:
> > On a server-class ppc host, this capability depends on the KVM type,
> > ie, HV or PR. If both KVM are present in the kernel, we will always
> > get the HV specific value, even if we explicitely requested PR on
> > the command line.
> > 
> > This can have an impact if we're using hugepages or a balloon device.
> > 
> > Since we've already created the VM at the time any user calls
> > kvm_has_sync_mmu(), switching to kvm_vm_check_extension() is
> > enough to fix any potential issue.
> > 
> > It is okay for the other archs that also implement KVM_CAP_SYNC_MMU,
> > ie, mips, s390, x86 and arm, because they don't depend on the VM being
> > created or not.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  accel/kvm/kvm-all.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index f85553a85194..323c567cfb68 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -2234,7 +2234,7 @@ int kvm_device_access(int fd, int group, uint64_t attr,
> >  /* Return 1 on success, 0 on failure */
> >  int kvm_has_sync_mmu(void)
> >  {
> > -    return kvm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
> > +    return kvm_vm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
> >  }  
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> ... but while you're at it, maybe it would be better to use a bool
> variable for the state of this extension, too, and only check for the
> extension one time at the end of kvm_init() ? kvm_has_sync_mmu() is
> apparently used multiple times in other source files, so we might be
> able to save some cycles by doing the syscall only once?
> 
>  Thomas
> 
> 

Oops, I had overlooked your answer... yes it makes sense indeed.

I'll post a v2 of this patch (not resending the whole series).

Cheers,

--
Greg