Re: [Qemu-devel] [PATCH 1/2] KVM: page track: add a new notifier type: track_flush_slot

[Alex Williamson <alex.williamson@redhat.com>]

On Fri, 14 Oct 2016 08:41:58 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Fri, 14 Oct 2016 18:37:45 +0800
> Jike Song <jike.song@intel.com> wrote:
> 
> > On 10/11/2016 05:47 PM, Paolo Bonzini wrote:  
> > > 
> > > 
> > > On 11/10/2016 11:21, Xiao Guangrong wrote:    
> > >>
> > >>
> > >> On 10/11/2016 04:54 PM, Paolo Bonzini wrote:    
> > >>>
> > >>>
> > >>> On 11/10/2016 04:39, Xiao Guangrong wrote:    
> > >>>>
> > >>>>
> > >>>> On 10/11/2016 02:32 AM, Paolo Bonzini wrote:    
> > >>>>>
> > >>>>>
> > >>>>> On 10/10/2016 20:01, Neo Jia wrote:    
> > >>>>>>> Hi Neo,
> > >>>>>>>
> > >>>>>>> AFAIK this is needed because KVMGT doesn't paravirtualize the PPGTT,
> > >>>>>>> while nVidia does.    
> > >>>>>>
> > >>>>>> Hi Paolo and Xiaoguang,
> > >>>>>>
> > >>>>>> I am just wondering how device driver can register a notifier so he
> > >>>>>> can be
> > >>>>>> notified for write-protected pages when writes are happening.    
> > >>>>>
> > >>>>> It can't yet, but the API is ready for that.  kvm_vfio_set_group is
> > >>>>> currently where a struct kvm_device* and struct vfio_group* touch.
> > >>>>> Given
> > >>>>> a struct kvm_device*, dev->kvm provides the struct kvm to be passed to
> > >>>>> kvm_page_track_register_notifier.  So I guess you could add a callback
> > >>>>> that passes the struct kvm_device* to the mdev device.
> > >>>>>
> > >>>>> Xiaoguang and Guangrong, what were your plans?  We discussed it briefly
> > >>>>> at KVM Forum but I don't remember the details.    
> > >>>>
> > >>>> Your suggestion was that pass kvm fd to KVMGT via VFIO, so that we can
> > >>>> figure out the kvm instance based on the fd.
> > >>>>
> > >>>> We got a new idea, how about search the kvm instance by mm_struct, it
> > >>>> can work as KVMGT is running in the vcpu context and it is much more
> > >>>> straightforward.    
> > >>>
> > >>> Perhaps I didn't understand your suggestion, but the same mm_struct can
> > >>> have more than 1 struct kvm so I'm not sure that it can work.    
> > >>
> > >> vcpu->pid is valid during vcpu running so that it can be used to figure
> > >> out which kvm instance owns the vcpu whose pid is the one as current
> > >> thread, i think it can work. :)    
> > > 
> > > No, don't do that.  There's no reason for a thread to run a single VCPU,
> > > and if you can have multiple VCPUs you can also have multiple VCPUs from
> > > multiple VMs.
> > > 
> > > Passing file descriptors around are the right way to connect subsystems.    
> > 
> > [CC Alex, Kevin and Qemu-devel]
> > 
> > Hi Paolo & Alex,
> > 
> > IIUC, passing file descriptors means touching QEMU and the UAPI between
> > QEMU and VFIO. Would you guys have a look at below draft patch? If it's
> > on the correct direction, I'll send the split ones. Thanks!
> > 
> > --
> > Thanks,
> > Jike
> > 
> > 
> > diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> > index bec694c..f715d37 100644
> > --- a/hw/vfio/pci-quirks.c
> > +++ b/hw/vfio/pci-quirks.c
> > @@ -10,12 +10,14 @@
> >   * the COPYING file in the top-level directory.
> >   */
> >  
> > +#include <sys/ioctl.h>
> >  #include "qemu/osdep.h"
> >  #include "qemu/error-report.h"
> >  #include "qemu/range.h"
> >  #include "qapi/error.h"
> >  #include "hw/nvram/fw_cfg.h"
> >  #include "pci.h"
> > +#include "sysemu/kvm.h"
> >  #include "trace.h"
> >  
> >  /* Use uin32_t for vendor & device so PCI_ANY_ID expands and cannot match hw */
> > @@ -1844,3 +1846,15 @@ void vfio_setup_resetfn_quirk(VFIOPCIDevice *vdev)
> >          break;
> >      }
> >  }
> > +
> > +void vfio_quirk_kvmgt(VFIOPCIDevice *vdev)
> > +{
> > +    int vmfd;
> > +
> > +    if (!kvm_enabled() || !vdev->kvmgt)
> > +        return;
> > +
> > +    /* Tell the device what KVM it attached */
> > +    vmfd = kvm_get_vmfd(kvm_state);
> > +    ioctl(vdev->vbasedev.fd, VFIO_SET_KVMFD, vmfd);
> > +}
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index a5a620a..8732552 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2561,6 +2561,8 @@ static int vfio_initfn(PCIDevice *pdev)
> >          return ret;
> >      }
> >  
> > +    vfio_quirk_kvmgt(vdev);
> > +
> >      /* Get a copy of config space */
> >      ret = pread(vdev->vbasedev.fd, vdev->pdev.config,
> >                  MIN(pci_config_size(&vdev->pdev), vdev->config_size),
> > @@ -2832,6 +2834,7 @@ static Property vfio_pci_dev_properties[] = {
> >      DEFINE_PROP_UINT32("x-pci-sub-device-id", VFIOPCIDevice,
> >                         sub_device_id, PCI_ANY_ID),
> >      DEFINE_PROP_UINT32("x-igd-gms", VFIOPCIDevice, igd_gms, 0),
> > +    DEFINE_PROP_BOOL("kvmgt", VFIOPCIDevice, kvmgt, false),  
> 
> Just a side note, device options are a headache, users are prone to get
> them wrong and minimally it requires an entire round to get libvirt
> support.  We should be able to detect from the device or vfio API
> whether such a call is required.  Obviously if we can use the existing
> kvm-vfio device, that's the better option anyway.  Thanks,

Also, vfio devices currently have no hard dependencies on KVM, if kvmgt
does, it needs to produce a device failure when unavailable.  Thanks,

Alex

> >      /*
> >       * TODO - support passed fds... is this necessary?
> >       * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),
> > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> > index 7d482d9..813832c 100644
> > --- a/hw/vfio/pci.h
> > +++ b/hw/vfio/pci.h
> > @@ -143,6 +143,7 @@ typedef struct VFIOPCIDevice {
> >      bool no_kvm_intx;
> >      bool no_kvm_msi;
> >      bool no_kvm_msix;
> > +    bool kvmgt;
> >  } VFIOPCIDevice;
> >  
> >  uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
> > @@ -166,4 +167,6 @@ int vfio_populate_vga(VFIOPCIDevice *vdev);
> >  int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
> >                                 struct vfio_region_info *info);
> >  
> > +void vfio_quirk_kvmgt(VFIOPCIDevice *vdev);
> > +
> >  #endif /* HW_VFIO_VFIO_PCI_H */
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index df67cc0..dd8320a 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -254,6 +254,7 @@ void phys_mem_set_alloc(void *(*alloc)(size_t, uint64_t *align));
> >  int kvm_ioctl(KVMState *s, int type, ...);
> >  
> >  int kvm_vm_ioctl(KVMState *s, int type, ...);
> > +int kvm_get_vmfd(KVMState *s);
> >  
> >  int kvm_vcpu_ioctl(CPUState *cpu, int type, ...);
> >  
> > diff --git a/kvm-all.c b/kvm-all.c
> > index efb5fe3..bd72ce3 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -2065,6 +2065,11 @@ int kvm_vm_ioctl(KVMState *s, int type, ...)
> >      return ret;
> >  }
> >  
> > +int kvm_get_vmfd(KVMState *s)
> > +{
> > +	return s->vmfd;
> > +}
> > +
> >  int kvm_vcpu_ioctl(CPUState *cpu, int type, ...)
> >  {
> >      int ret;
> > diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
> > index 759b850..952303f 100644
> > --- a/linux-headers/linux/vfio.h
> > +++ b/linux-headers/linux/vfio.h
> > @@ -686,6 +686,12 @@ struct vfio_iommu_spapr_tce_remove {
> >  };
> >  #define VFIO_IOMMU_SPAPR_TCE_REMOVE	_IO(VFIO_TYPE, VFIO_BASE + 20)
> >  
> > +
> > +/**
> > + * VFIO_SET_KVMFD - _IO(VFIO_TYPE, VFIO_BASE + 21, __u32)
> > + */
> > +#define VFIO_SET_KVMFD		_IO(VFIO_TYPE, VFIO_BASE + 21)
> > +
> >  /* ***************************************************************** */
> >  
> >  #endif /* VFIO_H */
> >   
>