Re: [Qemu-devel] [PATCH 1/3] kvm: check KVM_CAP_SYNC_MMU with kvm_vm_check_extension()

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 2017 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]