| Message ID | 20200529080303.15449-5-jasowang@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vDPA: doorbell mapping | expand |
Hi, On 29.5.2020 11.03, Jason Wang wrote: > Currently the doorbell is relayed via eventfd which may have > significant overhead because of the cost of vmexits or syscall. This > patch introduces mmap() based doorbell mapping which can eliminate the > overhead caused by vmexit or syscall. Just wondering. I know very little about vdpa. But how is such a "sw doorbell" monitored or observed, if no fault or wmexit etc. Is there some kind of polling used? > To ease the userspace modeling of the doorbell layout (usually > virtio-pci), this patch starts from a doorbell per page > model. Vhost-vdpa only support the hardware doorbell that sit at the > boundary of a page and does not share the page with other registers. > > Doorbell of each virtqueue must be mapped separately, pgoff is the > index of the virtqueue. This allows userspace to map a subset of the > doorbell which may be useful for the implementation of software > assisted virtqueue (control vq) in the future. > > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > drivers/vhost/vdpa.c | 59 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c > index 6ff72289f488..bbe23cea139a 100644 > --- a/drivers/vhost/vdpa.c > +++ b/drivers/vhost/vdpa.c > @@ -15,6 +15,7 @@ > #include <linux/module.h> > #include <linux/cdev.h> > #include <linux/device.h> > +#include <linux/mm.h> > #include <linux/iommu.h> > #include <linux/uuid.h> > #include <linux/vdpa.h> > @@ -741,12 +742,70 @@ static int vhost_vdpa_release(struct inode *inode, struct file *filep) > return 0; > } > > +static vm_fault_t vhost_vdpa_fault(struct vm_fault *vmf) > +{ > + struct vhost_vdpa *v = vmf->vma->vm_file->private_data; > + struct vdpa_device *vdpa = v->vdpa; > + const struct vdpa_config_ops *ops = vdpa->config; > + struct vdpa_notification_area notify; > + struct vm_area_struct *vma = vmf->vma; > + u16 index = vma->vm_pgoff; > + > + notify = ops->get_vq_notification(vdpa, index); > + > + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > + if (remap_pfn_range(vma, vmf->address & PAGE_MASK, > + notify.addr >> PAGE_SHIFT, PAGE_SIZE, > + vma->vm_page_prot)) > + return VM_FAULT_SIGBUS; > + > + return VM_FAULT_NOPAGE; > +} > + > +static const struct vm_operations_struct vhost_vdpa_vm_ops = { > + .fault = vhost_vdpa_fault, > +}; > + > +static int vhost_vdpa_mmap(struct file *file, struct vm_area_struct *vma) > +{ > + struct vhost_vdpa *v = vma->vm_file->private_data; > + struct vdpa_device *vdpa = v->vdpa; > + const struct vdpa_config_ops *ops = vdpa->config; > + struct vdpa_notification_area notify; > + int index = vma->vm_pgoff; > + > + if (vma->vm_end - vma->vm_start != PAGE_SIZE) > + return -EINVAL; > + if ((vma->vm_flags & VM_SHARED) == 0) > + return -EINVAL; > + if (vma->vm_flags & VM_READ) > + return -EINVAL; > + if (index > 65535) > + return -EINVAL; > + if (!ops->get_vq_notification) > + return -ENOTSUPP; > + > + /* To be safe and easily modelled by userspace, We only > + * support the doorbell which sits on the page boundary and > + * does not share the page with other registers. > + */ > + notify = ops->get_vq_notification(vdpa, index); > + if (notify.addr & (PAGE_SIZE - 1)) > + return -EINVAL; > + if (vma->vm_end - vma->vm_start != notify.size) > + return -ENOTSUPP; > + > + vma->vm_ops = &vhost_vdpa_vm_ops; > + return 0; > +} > + > static const struct file_operations vhost_vdpa_fops = { > .owner = THIS_MODULE, > .open = vhost_vdpa_open, > .release = vhost_vdpa_release, > .write_iter = vhost_vdpa_chr_write_iter, > .unlocked_ioctl = vhost_vdpa_unlocked_ioctl, > + .mmap = vhost_vdpa_mmap, > .compat_ioctl = compat_ptr_ioctl, > }; >
On 2020/5/29 下午5:16, Mika Penttilä wrote: > Hi, > > On 29.5.2020 11.03, Jason Wang wrote: >> Currently the doorbell is relayed via eventfd which may have >> significant overhead because of the cost of vmexits or syscall. This >> patch introduces mmap() based doorbell mapping which can eliminate the >> overhead caused by vmexit or syscall. > > Just wondering. I know very little about vdpa. But how is such a "sw > doorbell" monitored or observed, if no fault or wmexit etc. > Is there some kind of polling used? Hi Mika: It's not a software doorbell. It just allow userspace to map page of hardware doorbell directly into userspace. Without this, for KVM, it needs to trap the MMIO access of the guest and write to eventfd, for other userspace driver, it needs to write to eventfd. vhost-vDPA's eventfd wakeup function may let the driver to do touch the doorbell. With this, since the doorbell page is mapped into userspace address space, guest or other userspace driver may write directly to the hardware doorbell register. Thanks > >> To ease the userspace modeling of the doorbell layout (usually >> virtio-pci), this patch starts from a doorbell per page >> model. Vhost-vdpa only support the hardware doorbell that sit at the >> boundary of a page and does not share the page with other registers. >> >> Doorbell of each virtqueue must be mapped separately, pgoff is the >> index of the virtqueue. This allows userspace to map a subset of the >> doorbell which may be useful for the implementation of software >> assisted virtqueue (control vq) in the future. >> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> drivers/vhost/vdpa.c | 59 ++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 59 insertions(+) >> >> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c >> index 6ff72289f488..bbe23cea139a 100644 >> --- a/drivers/vhost/vdpa.c >> +++ b/drivers/vhost/vdpa.c >> @@ -15,6 +15,7 @@ >> #include <linux/module.h> >> #include <linux/cdev.h> >> #include <linux/device.h> >> +#include <linux/mm.h> >> #include <linux/iommu.h> >> #include <linux/uuid.h> >> #include <linux/vdpa.h> >> @@ -741,12 +742,70 @@ static int vhost_vdpa_release(struct inode >> *inode, struct file *filep) >> return 0; >> } >> +static vm_fault_t vhost_vdpa_fault(struct vm_fault *vmf) >> +{ >> + struct vhost_vdpa *v = vmf->vma->vm_file->private_data; >> + struct vdpa_device *vdpa = v->vdpa; >> + const struct vdpa_config_ops *ops = vdpa->config; >> + struct vdpa_notification_area notify; >> + struct vm_area_struct *vma = vmf->vma; >> + u16 index = vma->vm_pgoff; >> + >> + notify = ops->get_vq_notification(vdpa, index); >> + >> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); >> + if (remap_pfn_range(vma, vmf->address & PAGE_MASK, >> + notify.addr >> PAGE_SHIFT, PAGE_SIZE, >> + vma->vm_page_prot)) >> + return VM_FAULT_SIGBUS; >> + >> + return VM_FAULT_NOPAGE; >> +} >> + >> +static const struct vm_operations_struct vhost_vdpa_vm_ops = { >> + .fault = vhost_vdpa_fault, >> +}; >> + >> +static int vhost_vdpa_mmap(struct file *file, struct vm_area_struct >> *vma) >> +{ >> + struct vhost_vdpa *v = vma->vm_file->private_data; >> + struct vdpa_device *vdpa = v->vdpa; >> + const struct vdpa_config_ops *ops = vdpa->config; >> + struct vdpa_notification_area notify; >> + int index = vma->vm_pgoff; >> + >> + if (vma->vm_end - vma->vm_start != PAGE_SIZE) >> + return -EINVAL; >> + if ((vma->vm_flags & VM_SHARED) == 0) >> + return -EINVAL; >> + if (vma->vm_flags & VM_READ) >> + return -EINVAL; >> + if (index > 65535) >> + return -EINVAL; >> + if (!ops->get_vq_notification) >> + return -ENOTSUPP; >> + >> + /* To be safe and easily modelled by userspace, We only >> + * support the doorbell which sits on the page boundary and >> + * does not share the page with other registers. >> + */ >> + notify = ops->get_vq_notification(vdpa, index); >> + if (notify.addr & (PAGE_SIZE - 1)) >> + return -EINVAL; >> + if (vma->vm_end - vma->vm_start != notify.size) >> + return -ENOTSUPP; >> + >> + vma->vm_ops = &vhost_vdpa_vm_ops; >> + return 0; >> +} >> + >> static const struct file_operations vhost_vdpa_fops = { >> .owner = THIS_MODULE, >> .open = vhost_vdpa_open, >> .release = vhost_vdpa_release, >> .write_iter = vhost_vdpa_chr_write_iter, >> .unlocked_ioctl = vhost_vdpa_unlocked_ioctl, >> + .mmap = vhost_vdpa_mmap, >> .compat_ioctl = compat_ptr_ioctl, >> }; >
Hi Jason,
I love your patch! Yet something to improve:
[auto build test ERROR on vhost/linux-next]
[also build test ERROR on linus/master v5.7 next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Jason-Wang/vDPA-doorbell-mapping/20200531-070834
base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: m68k-randconfig-r011-20200601 (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
drivers/vhost/vdpa.c: In function 'vhost_vdpa_fault':
>> drivers/vhost/vdpa.c:754:22: error: implicit declaration of function 'pgprot_noncached' [-Werror=implicit-function-declaration]
754 | vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
| ^~~~~~~~~~~~~~~~
>> drivers/vhost/vdpa.c:754:22: error: incompatible types when assigning to type 'pgprot_t' {aka 'struct <anonymous>'} from type 'int'
cc1: some warnings being treated as errors
vim +/pgprot_noncached +754 drivers/vhost/vdpa.c
742
743 static vm_fault_t vhost_vdpa_fault(struct vm_fault *vmf)
744 {
745 struct vhost_vdpa *v = vmf->vma->vm_file->private_data;
746 struct vdpa_device *vdpa = v->vdpa;
747 const struct vdpa_config_ops *ops = vdpa->config;
748 struct vdpa_notification_area notify;
749 struct vm_area_struct *vma = vmf->vma;
750 u16 index = vma->vm_pgoff;
751
752 notify = ops->get_vq_notification(vdpa, index);
753
> 754 vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
755 if (remap_pfn_range(vma, vmf->address & PAGE_MASK,
756 notify.addr >> PAGE_SHIFT, PAGE_SIZE,
757 vma->vm_page_prot))
758 return VM_FAULT_SIGBUS;
759
760 return VM_FAULT_NOPAGE;
761 }
762
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 2020/5/30 上午2:30, Rob Miller wrote: > Given the need for 4K doorbell such that QEMU can easily map, ect, and > assuming that I have a HW device which exposes 2 VQ's, with a > notification area off of BAR3, offset=whatever, notifier_multiplier=4, > we don't need to have 2 x 4K pages mapped into the VM for both > doorbells do we? The guest driver would ring DB0 at BAR4+offset, and > DB1 at BAR4+offset+(4*1). This requires qemu to advertise notifier_multiplier = 4 to guest, it has several implications: - guest may think control virtqueue sit in the same page then we can't trap access to control virtqueue, qemu may lose the state of the device - it requires the destination to have exact the same queue_notify_off and notifier_multiplier in order to let migration to work So for doorbell mapping itself, we can support the mapping of doorbells at once which may occupy one or more pages, but it may bring troubles for other features. > > The 4K per DB is useful how? This allows for QEMU trapping of > individual DBs, that can then be used to do what, just forward the DBs > via some other scheme - this makes sense for non-HW related Virtio > devices I guess. Is this why there is a qemu option? The page per vq option provides extra flexibility. Note that it's the layout seen by guest, so actually for hardware vendor, the most easy way is to: - use a single doorbell register for all virtqueues except for the control virtqueue (driver can differ queue index by the value wrote by the driver) - do not share the page with other registers In this way, it doesn't require much BAR space and we can still: - map doorbell separately to guest: guest may see a page per vq doorbell layout but in fact all those pages are mapped into the same page - trap the control virtqueue, (don't do mmap for control vq) Thanks > > Rob Miller > rob.miller@broadcom.com <mailto:rob.miller@broadcom.com> > (919)721-3339 > > > On Fri, May 29, 2020 at 4:03 AM Jason Wang <jasowang@redhat.com > <mailto:jasowang@redhat.com>> wrote: > > Currently the doorbell is relayed via eventfd which may have > significant overhead because of the cost of vmexits or syscall. This > patch introduces mmap() based doorbell mapping which can eliminate the > overhead caused by vmexit or syscall. > > To ease the userspace modeling of the doorbell layout (usually > virtio-pci), this patch starts from a doorbell per page > model. Vhost-vdpa only support the hardware doorbell that sit at the > boundary of a page and does not share the page with other registers. > > Doorbell of each virtqueue must be mapped separately, pgoff is the > index of the virtqueue. This allows userspace to map a subset of the > doorbell which may be useful for the implementation of software > assisted virtqueue (control vq) in the future. > > Signed-off-by: Jason Wang <jasowang@redhat.com > <mailto:jasowang@redhat.com>> > --- > drivers/vhost/vdpa.c | 59 > ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c > index 6ff72289f488..bbe23cea139a 100644 > --- a/drivers/vhost/vdpa.c > +++ b/drivers/vhost/vdpa.c > @@ -15,6 +15,7 @@ > #include <linux/module.h> > #include <linux/cdev.h> > #include <linux/device.h> > +#include <linux/mm.h> > #include <linux/iommu.h> > #include <linux/uuid.h> > #include <linux/vdpa.h> > @@ -741,12 +742,70 @@ static int vhost_vdpa_release(struct inode > *inode, struct file *filep) > return 0; > } > > +static vm_fault_t vhost_vdpa_fault(struct vm_fault *vmf) > +{ > + struct vhost_vdpa *v = vmf->vma->vm_file->private_data; > + struct vdpa_device *vdpa = v->vdpa; > + const struct vdpa_config_ops *ops = vdpa->config; > + struct vdpa_notification_area notify; > + struct vm_area_struct *vma = vmf->vma; > + u16 index = vma->vm_pgoff; > + > + notify = ops->get_vq_notification(vdpa, index); > + > + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > + if (remap_pfn_range(vma, vmf->address & PAGE_MASK, > + notify.addr >> PAGE_SHIFT, PAGE_SIZE, > + vma->vm_page_prot)) > + return VM_FAULT_SIGBUS; > + > + return VM_FAULT_NOPAGE; > +} > + > +static const struct vm_operations_struct vhost_vdpa_vm_ops = { > + .fault = vhost_vdpa_fault, > +}; > + > +static int vhost_vdpa_mmap(struct file *file, struct > vm_area_struct *vma) > +{ > + struct vhost_vdpa *v = vma->vm_file->private_data; > + struct vdpa_device *vdpa = v->vdpa; > + const struct vdpa_config_ops *ops = vdpa->config; > + struct vdpa_notification_area notify; > + int index = vma->vm_pgoff; > + > + if (vma->vm_end - vma->vm_start != PAGE_SIZE) > + return -EINVAL; > + if ((vma->vm_flags & VM_SHARED) == 0) > + return -EINVAL; > + if (vma->vm_flags & VM_READ) > + return -EINVAL; > + if (index > 65535) > + return -EINVAL; > + if (!ops->get_vq_notification) > + return -ENOTSUPP; > + > + /* To be safe and easily modelled by userspace, We only > + * support the doorbell which sits on the page boundary and > + * does not share the page with other registers. > + */ > + notify = ops->get_vq_notification(vdpa, index); > + if (notify.addr & (PAGE_SIZE - 1)) > + return -EINVAL; > + if (vma->vm_end - vma->vm_start != notify.size) > + return -ENOTSUPP; > + > + vma->vm_ops = &vhost_vdpa_vm_ops; > + return 0; > +} > + > static const struct file_operations vhost_vdpa_fops = { > .owner = THIS_MODULE, > .open = vhost_vdpa_open, > .release = vhost_vdpa_release, > .write_iter = vhost_vdpa_chr_write_iter, > .unlocked_ioctl = vhost_vdpa_unlocked_ioctl, > + .mmap = vhost_vdpa_mmap, > .compat_ioctl = compat_ptr_ioctl, > }; > > -- > 2.20.1 >
On Tue, Jun 02, 2020 at 03:22:49AM +0800, kbuild test robot wrote: > Hi Jason, > > I love your patch! Yet something to improve: > > [auto build test ERROR on vhost/linux-next] > [also build test ERROR on linus/master v5.7 next-20200529] > [if your patch is applied to the wrong git tree, please drop us a note to help > improve the system. BTW, we also suggest to use '--base' option to specify the > base tree in git format-patch, please see https://stackoverflow.com/a/37406982] > > url: https://github.com/0day-ci/linux/commits/Jason-Wang/vDPA-doorbell-mapping/20200531-070834 > base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next > config: m68k-randconfig-r011-20200601 (attached as .config) > compiler: m68k-linux-gcc (GCC) 9.3.0 > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kbuild test robot <lkp@intel.com> > > All errors (new ones prefixed by >>, old ones prefixed by <<): > > drivers/vhost/vdpa.c: In function 'vhost_vdpa_fault': > >> drivers/vhost/vdpa.c:754:22: error: implicit declaration of function 'pgprot_noncached' [-Werror=implicit-function-declaration] > 754 | vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > | ^~~~~~~~~~~~~~~~ > >> drivers/vhost/vdpa.c:754:22: error: incompatible types when assigning to type 'pgprot_t' {aka 'struct <anonymous>'} from type 'int' > cc1: some warnings being treated as errors > > vim +/pgprot_noncached +754 drivers/vhost/vdpa.c > > 742 > 743 static vm_fault_t vhost_vdpa_fault(struct vm_fault *vmf) > 744 { > 745 struct vhost_vdpa *v = vmf->vma->vm_file->private_data; > 746 struct vdpa_device *vdpa = v->vdpa; > 747 const struct vdpa_config_ops *ops = vdpa->config; > 748 struct vdpa_notification_area notify; > 749 struct vm_area_struct *vma = vmf->vma; > 750 u16 index = vma->vm_pgoff; > 751 > 752 notify = ops->get_vq_notification(vdpa, index); > 753 > > 754 vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > 755 if (remap_pfn_range(vma, vmf->address & PAGE_MASK, > 756 notify.addr >> PAGE_SHIFT, PAGE_SIZE, > 757 vma->vm_page_prot)) > 758 return VM_FAULT_SIGBUS; > 759 > 760 return VM_FAULT_NOPAGE; > 761 } > 762 Yes well, all this remapping clearly has no chance to work on systems without CONFIG_MMU. > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 2020/6/2 下午12:56, Michael S. Tsirkin wrote: > On Tue, Jun 02, 2020 at 03:22:49AM +0800, kbuild test robot wrote: >> Hi Jason, >> >> I love your patch! Yet something to improve: >> >> [auto build test ERROR on vhost/linux-next] >> [also build test ERROR on linus/master v5.7 next-20200529] >> [if your patch is applied to the wrong git tree, please drop us a note to help >> improve the system. BTW, we also suggest to use '--base' option to specify the >> base tree in git format-patch, please seehttps://stackoverflow.com/a/37406982] >> >> url:https://github.com/0day-ci/linux/commits/Jason-Wang/vDPA-doorbell-mapping/20200531-070834 >> base:https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next >> config: m68k-randconfig-r011-20200601 (attached as .config) >> compiler: m68k-linux-gcc (GCC) 9.3.0 >> reproduce (this is a W=1 build): >> wgethttps://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross >> chmod +x ~/bin/make.cross >> # save the attached .config to linux build tree >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k >> >> If you fix the issue, kindly add following tag as appropriate >> Reported-by: kbuild test robot<lkp@intel.com> >> >> All errors (new ones prefixed by >>, old ones prefixed by <<): >> >> drivers/vhost/vdpa.c: In function 'vhost_vdpa_fault': >>>> drivers/vhost/vdpa.c:754:22: error: implicit declaration of function 'pgprot_noncached' [-Werror=implicit-function-declaration] >> 754 | vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); >> | ^~~~~~~~~~~~~~~~ >>>> drivers/vhost/vdpa.c:754:22: error: incompatible types when assigning to type 'pgprot_t' {aka 'struct <anonymous>'} from type 'int' >> cc1: some warnings being treated as errors >> >> vim +/pgprot_noncached +754 drivers/vhost/vdpa.c >> >> 742 >> 743 static vm_fault_t vhost_vdpa_fault(struct vm_fault *vmf) >> 744 { >> 745 struct vhost_vdpa *v = vmf->vma->vm_file->private_data; >> 746 struct vdpa_device *vdpa = v->vdpa; >> 747 const struct vdpa_config_ops *ops = vdpa->config; >> 748 struct vdpa_notification_area notify; >> 749 struct vm_area_struct *vma = vmf->vma; >> 750 u16 index = vma->vm_pgoff; >> 751 >> 752 notify = ops->get_vq_notification(vdpa, index); >> 753 >> > 754 vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); >> 755 if (remap_pfn_range(vma, vmf->address & PAGE_MASK, >> 756 notify.addr >> PAGE_SHIFT, PAGE_SIZE, >> 757 vma->vm_page_prot)) >> 758 return VM_FAULT_SIGBUS; >> 759 >> 760 return VM_FAULT_NOPAGE; >> 761 } >> 762 > Yes well, all this remapping clearly has no chance to work > on systems without CONFIG_MMU. It looks to me mmap can work according to Documentation/nommu-mmap.txt. But I'm not sure it's worth to bother. Thanks > > >
On Tue, Jun 02, 2020 at 02:49:38PM +0800, Jason Wang wrote: > > On 2020/6/2 下午12:56, Michael S. Tsirkin wrote: > > On Tue, Jun 02, 2020 at 03:22:49AM +0800, kbuild test robot wrote: > > > Hi Jason, > > > > > > I love your patch! Yet something to improve: > > > > > > [auto build test ERROR on vhost/linux-next] > > > [also build test ERROR on linus/master v5.7 next-20200529] > > > [if your patch is applied to the wrong git tree, please drop us a note to help > > > improve the system. BTW, we also suggest to use '--base' option to specify the > > > base tree in git format-patch, please seehttps://stackoverflow.com/a/37406982] > > > > > > url:https://github.com/0day-ci/linux/commits/Jason-Wang/vDPA-doorbell-mapping/20200531-070834 > > > base:https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next > > > config: m68k-randconfig-r011-20200601 (attached as .config) > > > compiler: m68k-linux-gcc (GCC) 9.3.0 > > > reproduce (this is a W=1 build): > > > wgethttps://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > > > chmod +x ~/bin/make.cross > > > # save the attached .config to linux build tree > > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k > > > > > > If you fix the issue, kindly add following tag as appropriate > > > Reported-by: kbuild test robot<lkp@intel.com> > > > > > > All errors (new ones prefixed by >>, old ones prefixed by <<): > > > > > > drivers/vhost/vdpa.c: In function 'vhost_vdpa_fault': > > > > > drivers/vhost/vdpa.c:754:22: error: implicit declaration of function 'pgprot_noncached' [-Werror=implicit-function-declaration] > > > 754 | vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > > > | ^~~~~~~~~~~~~~~~ > > > > > drivers/vhost/vdpa.c:754:22: error: incompatible types when assigning to type 'pgprot_t' {aka 'struct <anonymous>'} from type 'int' > > > cc1: some warnings being treated as errors > > > > > > vim +/pgprot_noncached +754 drivers/vhost/vdpa.c > > > > > > 742 > > > 743 static vm_fault_t vhost_vdpa_fault(struct vm_fault *vmf) > > > 744 { > > > 745 struct vhost_vdpa *v = vmf->vma->vm_file->private_data; > > > 746 struct vdpa_device *vdpa = v->vdpa; > > > 747 const struct vdpa_config_ops *ops = vdpa->config; > > > 748 struct vdpa_notification_area notify; > > > 749 struct vm_area_struct *vma = vmf->vma; > > > 750 u16 index = vma->vm_pgoff; > > > 751 > > > 752 notify = ops->get_vq_notification(vdpa, index); > > > 753 > > > > 754 vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > > > 755 if (remap_pfn_range(vma, vmf->address & PAGE_MASK, > > > 756 notify.addr >> PAGE_SHIFT, PAGE_SIZE, > > > 757 vma->vm_page_prot)) > > > 758 return VM_FAULT_SIGBUS; > > > 759 > > > 760 return VM_FAULT_NOPAGE; > > > 761 } > > > 762 > > Yes well, all this remapping clearly has no chance to work > > on systems without CONFIG_MMU. > > > It looks to me mmap can work according to Documentation/nommu-mmap.txt. But > I'm not sure it's worth to bother. > > Thanks Well int remap_pfn_range(struct vm_area_struct *vma, unsigned long addr, unsigned long pfn, unsigned long size, pgprot_t prot) { if (addr != (pfn << PAGE_SHIFT)) return -EINVAL; vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; return 0; } EXPORT_SYMBOL(remap_pfn_range); So things aren't going to work if you have a fixed PFN which is the case of the hardware device. > > > > > > >
On 2020/6/2 下午9:31, Michael S. Tsirkin wrote: > On Tue, Jun 02, 2020 at 02:49:38PM +0800, Jason Wang wrote: >> On 2020/6/2 下午12:56, Michael S. Tsirkin wrote: >>> On Tue, Jun 02, 2020 at 03:22:49AM +0800, kbuild test robot wrote: >>>> Hi Jason, >>>> >>>> I love your patch! Yet something to improve: >>>> >>>> [auto build test ERROR on vhost/linux-next] >>>> [also build test ERROR on linus/master v5.7 next-20200529] >>>> [if your patch is applied to the wrong git tree, please drop us a note to help >>>> improve the system. BTW, we also suggest to use '--base' option to specify the >>>> base tree in git format-patch, please seehttps://stackoverflow.com/a/37406982] >>>> >>>> url:https://github.com/0day-ci/linux/commits/Jason-Wang/vDPA-doorbell-mapping/20200531-070834 >>>> base:https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next >>>> config: m68k-randconfig-r011-20200601 (attached as .config) >>>> compiler: m68k-linux-gcc (GCC) 9.3.0 >>>> reproduce (this is a W=1 build): >>>> wgethttps://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross >>>> chmod +x ~/bin/make.cross >>>> # save the attached .config to linux build tree >>>> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k >>>> >>>> If you fix the issue, kindly add following tag as appropriate >>>> Reported-by: kbuild test robot<lkp@intel.com> >>>> >>>> All errors (new ones prefixed by >>, old ones prefixed by <<): >>>> >>>> drivers/vhost/vdpa.c: In function 'vhost_vdpa_fault': >>>>>> drivers/vhost/vdpa.c:754:22: error: implicit declaration of function 'pgprot_noncached' [-Werror=implicit-function-declaration] >>>> 754 | vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); >>>> | ^~~~~~~~~~~~~~~~ >>>>>> drivers/vhost/vdpa.c:754:22: error: incompatible types when assigning to type 'pgprot_t' {aka 'struct <anonymous>'} from type 'int' >>>> cc1: some warnings being treated as errors >>>> >>>> vim +/pgprot_noncached +754 drivers/vhost/vdpa.c >>>> >>>> 742 >>>> 743 static vm_fault_t vhost_vdpa_fault(struct vm_fault *vmf) >>>> 744 { >>>> 745 struct vhost_vdpa *v = vmf->vma->vm_file->private_data; >>>> 746 struct vdpa_device *vdpa = v->vdpa; >>>> 747 const struct vdpa_config_ops *ops = vdpa->config; >>>> 748 struct vdpa_notification_area notify; >>>> 749 struct vm_area_struct *vma = vmf->vma; >>>> 750 u16 index = vma->vm_pgoff; >>>> 751 >>>> 752 notify = ops->get_vq_notification(vdpa, index); >>>> 753 >>>> > 754 vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); >>>> 755 if (remap_pfn_range(vma, vmf->address & PAGE_MASK, >>>> 756 notify.addr >> PAGE_SHIFT, PAGE_SIZE, >>>> 757 vma->vm_page_prot)) >>>> 758 return VM_FAULT_SIGBUS; >>>> 759 >>>> 760 return VM_FAULT_NOPAGE; >>>> 761 } >>>> 762 >>> Yes well, all this remapping clearly has no chance to work >>> on systems without CONFIG_MMU. >> >> It looks to me mmap can work according to Documentation/nommu-mmap.txt. But >> I'm not sure it's worth to bother. >> >> Thanks > > Well > > int remap_pfn_range(struct vm_area_struct *vma, unsigned long addr, > unsigned long pfn, unsigned long size, pgprot_t prot) > { > if (addr != (pfn << PAGE_SHIFT)) > return -EINVAL; > > vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; > return 0; > } > EXPORT_SYMBOL(remap_pfn_range); > > > So things aren't going to work if you have a fixed PFN > which is the case of the hardware device. Looking at the implementation of some drivers e.g mtd_char. If I read the code correctly, we can do this by providing get_unmapped_area method and use physical address directly. But start form CONFIG_MMU should be fine. Do you prefer making vhost_vdpa depends on CONFIG_MMU or just fail mmap when CONFIG_MMU is not configured? Thanks > > >>> >>>
On Wed, Jun 03, 2020 at 12:18:44PM +0800, Jason Wang wrote: > > On 2020/6/2 下午9:31, Michael S. Tsirkin wrote: > > On Tue, Jun 02, 2020 at 02:49:38PM +0800, Jason Wang wrote: > > > On 2020/6/2 下午12:56, Michael S. Tsirkin wrote: > > > > On Tue, Jun 02, 2020 at 03:22:49AM +0800, kbuild test robot wrote: > > > > > Hi Jason, > > > > > > > > > > I love your patch! Yet something to improve: > > > > > > > > > > [auto build test ERROR on vhost/linux-next] > > > > > [also build test ERROR on linus/master v5.7 next-20200529] > > > > > [if your patch is applied to the wrong git tree, please drop us a note to help > > > > > improve the system. BTW, we also suggest to use '--base' option to specify the > > > > > base tree in git format-patch, please seehttps://stackoverflow.com/a/37406982] > > > > > > > > > > url:https://github.com/0day-ci/linux/commits/Jason-Wang/vDPA-doorbell-mapping/20200531-070834 > > > > > base:https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next > > > > > config: m68k-randconfig-r011-20200601 (attached as .config) > > > > > compiler: m68k-linux-gcc (GCC) 9.3.0 > > > > > reproduce (this is a W=1 build): > > > > > wgethttps://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > > > > > chmod +x ~/bin/make.cross > > > > > # save the attached .config to linux build tree > > > > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k > > > > > > > > > > If you fix the issue, kindly add following tag as appropriate > > > > > Reported-by: kbuild test robot<lkp@intel.com> > > > > > > > > > > All errors (new ones prefixed by >>, old ones prefixed by <<): > > > > > > > > > > drivers/vhost/vdpa.c: In function 'vhost_vdpa_fault': > > > > > > > drivers/vhost/vdpa.c:754:22: error: implicit declaration of function 'pgprot_noncached' [-Werror=implicit-function-declaration] > > > > > 754 | vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > > > > > | ^~~~~~~~~~~~~~~~ > > > > > > > drivers/vhost/vdpa.c:754:22: error: incompatible types when assigning to type 'pgprot_t' {aka 'struct <anonymous>'} from type 'int' > > > > > cc1: some warnings being treated as errors > > > > > > > > > > vim +/pgprot_noncached +754 drivers/vhost/vdpa.c > > > > > > > > > > 742 > > > > > 743 static vm_fault_t vhost_vdpa_fault(struct vm_fault *vmf) > > > > > 744 { > > > > > 745 struct vhost_vdpa *v = vmf->vma->vm_file->private_data; > > > > > 746 struct vdpa_device *vdpa = v->vdpa; > > > > > 747 const struct vdpa_config_ops *ops = vdpa->config; > > > > > 748 struct vdpa_notification_area notify; > > > > > 749 struct vm_area_struct *vma = vmf->vma; > > > > > 750 u16 index = vma->vm_pgoff; > > > > > 751 > > > > > 752 notify = ops->get_vq_notification(vdpa, index); > > > > > 753 > > > > > > 754 vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > > > > > 755 if (remap_pfn_range(vma, vmf->address & PAGE_MASK, > > > > > 756 notify.addr >> PAGE_SHIFT, PAGE_SIZE, > > > > > 757 vma->vm_page_prot)) > > > > > 758 return VM_FAULT_SIGBUS; > > > > > 759 > > > > > 760 return VM_FAULT_NOPAGE; > > > > > 761 } > > > > > 762 > > > > Yes well, all this remapping clearly has no chance to work > > > > on systems without CONFIG_MMU. > > > > > > It looks to me mmap can work according to Documentation/nommu-mmap.txt. But > > > I'm not sure it's worth to bother. > > > > > > Thanks > > > > Well > > > > int remap_pfn_range(struct vm_area_struct *vma, unsigned long addr, > > unsigned long pfn, unsigned long size, pgprot_t prot) > > { > > if (addr != (pfn << PAGE_SHIFT)) > > return -EINVAL; > > > > vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; > > return 0; > > } > > EXPORT_SYMBOL(remap_pfn_range); > > > > > > So things aren't going to work if you have a fixed PFN > > which is the case of the hardware device. > > > Looking at the implementation of some drivers e.g mtd_char. If I read the > code correctly, we can do this by providing get_unmapped_area method and use > physical address directly. > > But start form CONFIG_MMU should be fine. Do you prefer making vhost_vdpa > depends on CONFIG_MMU or just fail mmap when CONFIG_MMU is not configured? > > Thanks I'd just not specify the mmap callback at all. > > > > > > > > > > > > >
On 2020/6/3 下午2:34, Michael S. Tsirkin wrote: > On Wed, Jun 03, 2020 at 12:18:44PM +0800, Jason Wang wrote: >> On 2020/6/2 下午9:31, Michael S. Tsirkin wrote: >>> On Tue, Jun 02, 2020 at 02:49:38PM +0800, Jason Wang wrote: >>>> On 2020/6/2 下午12:56, Michael S. Tsirkin wrote: >>>>> On Tue, Jun 02, 2020 at 03:22:49AM +0800, kbuild test robot wrote: >>>>>> Hi Jason, >>>>>> >>>>>> I love your patch! Yet something to improve: >>>>>> >>>>>> [auto build test ERROR on vhost/linux-next] >>>>>> [also build test ERROR on linus/master v5.7 next-20200529] >>>>>> [if your patch is applied to the wrong git tree, please drop us a note to help >>>>>> improve the system. BTW, we also suggest to use '--base' option to specify the >>>>>> base tree in git format-patch, please seehttps://stackoverflow.com/a/37406982] >>>>>> >>>>>> url:https://github.com/0day-ci/linux/commits/Jason-Wang/vDPA-doorbell-mapping/20200531-070834 >>>>>> base:https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next >>>>>> config: m68k-randconfig-r011-20200601 (attached as .config) >>>>>> compiler: m68k-linux-gcc (GCC) 9.3.0 >>>>>> reproduce (this is a W=1 build): >>>>>> wgethttps://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross >>>>>> chmod +x ~/bin/make.cross >>>>>> # save the attached .config to linux build tree >>>>>> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k >>>>>> >>>>>> If you fix the issue, kindly add following tag as appropriate >>>>>> Reported-by: kbuild test robot<lkp@intel.com> >>>>>> >>>>>> All errors (new ones prefixed by >>, old ones prefixed by <<): >>>>>> >>>>>> drivers/vhost/vdpa.c: In function 'vhost_vdpa_fault': >>>>>>>> drivers/vhost/vdpa.c:754:22: error: implicit declaration of function 'pgprot_noncached' [-Werror=implicit-function-declaration] >>>>>> 754 | vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); >>>>>> | ^~~~~~~~~~~~~~~~ >>>>>>>> drivers/vhost/vdpa.c:754:22: error: incompatible types when assigning to type 'pgprot_t' {aka 'struct <anonymous>'} from type 'int' >>>>>> cc1: some warnings being treated as errors >>>>>> >>>>>> vim +/pgprot_noncached +754 drivers/vhost/vdpa.c >>>>>> >>>>>> 742 >>>>>> 743 static vm_fault_t vhost_vdpa_fault(struct vm_fault *vmf) >>>>>> 744 { >>>>>> 745 struct vhost_vdpa *v = vmf->vma->vm_file->private_data; >>>>>> 746 struct vdpa_device *vdpa = v->vdpa; >>>>>> 747 const struct vdpa_config_ops *ops = vdpa->config; >>>>>> 748 struct vdpa_notification_area notify; >>>>>> 749 struct vm_area_struct *vma = vmf->vma; >>>>>> 750 u16 index = vma->vm_pgoff; >>>>>> 751 >>>>>> 752 notify = ops->get_vq_notification(vdpa, index); >>>>>> 753 >>>>>> > 754 vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); >>>>>> 755 if (remap_pfn_range(vma, vmf->address & PAGE_MASK, >>>>>> 756 notify.addr >> PAGE_SHIFT, PAGE_SIZE, >>>>>> 757 vma->vm_page_prot)) >>>>>> 758 return VM_FAULT_SIGBUS; >>>>>> 759 >>>>>> 760 return VM_FAULT_NOPAGE; >>>>>> 761 } >>>>>> 762 >>>>> Yes well, all this remapping clearly has no chance to work >>>>> on systems without CONFIG_MMU. >>>> It looks to me mmap can work according to Documentation/nommu-mmap.txt. But >>>> I'm not sure it's worth to bother. >>>> >>>> Thanks >>> Well >>> >>> int remap_pfn_range(struct vm_area_struct *vma, unsigned long addr, >>> unsigned long pfn, unsigned long size, pgprot_t prot) >>> { >>> if (addr != (pfn << PAGE_SHIFT)) >>> return -EINVAL; >>> >>> vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; >>> return 0; >>> } >>> EXPORT_SYMBOL(remap_pfn_range); >>> >>> >>> So things aren't going to work if you have a fixed PFN >>> which is the case of the hardware device. >> Looking at the implementation of some drivers e.g mtd_char. If I read the >> code correctly, we can do this by providing get_unmapped_area method and use >> physical address directly. >> >> But start form CONFIG_MMU should be fine. Do you prefer making vhost_vdpa >> depends on CONFIG_MMU or just fail mmap when CONFIG_MMU is not configured? >> >> Thanks > I'd just not specify the mmap callback at all. Ok, will do. Thanks >
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 6ff72289f488..bbe23cea139a 100644 --- a/drivers/vhost/vdpa.c +++ b/drivers/vhost/vdpa.c @@ -15,6 +15,7 @@ #include <linux/module.h> #include <linux/cdev.h> #include <linux/device.h> +#include <linux/mm.h> #include <linux/iommu.h> #include <linux/uuid.h> #include <linux/vdpa.h> @@ -741,12 +742,70 @@ static int vhost_vdpa_release(struct inode *inode, struct file *filep) return 0; } +static vm_fault_t vhost_vdpa_fault(struct vm_fault *vmf) +{ + struct vhost_vdpa *v = vmf->vma->vm_file->private_data; + struct vdpa_device *vdpa = v->vdpa; + const struct vdpa_config_ops *ops = vdpa->config; + struct vdpa_notification_area notify; + struct vm_area_struct *vma = vmf->vma; + u16 index = vma->vm_pgoff; + + notify = ops->get_vq_notification(vdpa, index); + + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); + if (remap_pfn_range(vma, vmf->address & PAGE_MASK, + notify.addr >> PAGE_SHIFT, PAGE_SIZE, + vma->vm_page_prot)) + return VM_FAULT_SIGBUS; + + return VM_FAULT_NOPAGE; +} + +static const struct vm_operations_struct vhost_vdpa_vm_ops = { + .fault = vhost_vdpa_fault, +}; + +static int vhost_vdpa_mmap(struct file *file, struct vm_area_struct *vma) +{ + struct vhost_vdpa *v = vma->vm_file->private_data; + struct vdpa_device *vdpa = v->vdpa; + const struct vdpa_config_ops *ops = vdpa->config; + struct vdpa_notification_area notify; + int index = vma->vm_pgoff; + + if (vma->vm_end - vma->vm_start != PAGE_SIZE) + return -EINVAL; + if ((vma->vm_flags & VM_SHARED) == 0) + return -EINVAL; + if (vma->vm_flags & VM_READ) + return -EINVAL; + if (index > 65535) + return -EINVAL; + if (!ops->get_vq_notification) + return -ENOTSUPP; + + /* To be safe and easily modelled by userspace, We only + * support the doorbell which sits on the page boundary and + * does not share the page with other registers. + */ + notify = ops->get_vq_notification(vdpa, index); + if (notify.addr & (PAGE_SIZE - 1)) + return -EINVAL; + if (vma->vm_end - vma->vm_start != notify.size) + return -ENOTSUPP; + + vma->vm_ops = &vhost_vdpa_vm_ops; + return 0; +} + static const struct file_operations vhost_vdpa_fops = { .owner = THIS_MODULE, .open = vhost_vdpa_open, .release = vhost_vdpa_release, .write_iter = vhost_vdpa_chr_write_iter, .unlocked_ioctl = vhost_vdpa_unlocked_ioctl, + .mmap = vhost_vdpa_mmap, .compat_ioctl = compat_ptr_ioctl, };
Currently the doorbell is relayed via eventfd which may have significant overhead because of the cost of vmexits or syscall. This patch introduces mmap() based doorbell mapping which can eliminate the overhead caused by vmexit or syscall. To ease the userspace modeling of the doorbell layout (usually virtio-pci), this patch starts from a doorbell per page model. Vhost-vdpa only support the hardware doorbell that sit at the boundary of a page and does not share the page with other registers. Doorbell of each virtqueue must be mapped separately, pgoff is the index of the virtqueue. This allows userspace to map a subset of the doorbell which may be useful for the implementation of software assisted virtqueue (control vq) in the future. Signed-off-by: Jason Wang <jasowang@redhat.com> --- drivers/vhost/vdpa.c | 59 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)