| Message ID | 20200518024944.14263-1-yan.y.zhao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce vendor ops in vfio-pci | expand |
On Sun, 17 May 2020 22:49:44 -0400 Yan Zhao <yan.y.zhao@intel.com> wrote: > This allows a simpler VFIO_DEVICE_GET_INFO ioctl in vendor driver > > Cc: Kevin Tian <kevin.tian@intel.com> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > --- > drivers/vfio/pci/vfio_pci.c | 23 +++++++++++++++++++++-- > drivers/vfio/pci/vfio_pci_private.h | 2 ++ > include/linux/vfio.h | 3 +++ > 3 files changed, 26 insertions(+), 2 deletions(-) > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > index 290b7ab55ecf..30137c1c5308 100644 > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -105,6 +105,24 @@ void *vfio_pci_vendor_data(void *device_data) > } > EXPORT_SYMBOL_GPL(vfio_pci_vendor_data); > > +int vfio_pci_set_vendor_regions(void *device_data, int num_vendor_regions) > +{ > + struct vfio_pci_device *vdev = device_data; > + > + vdev->num_vendor_regions = num_vendor_regions; Do we need any kind of sanity check here, in case this is called with a bogus value? > + return 0; > +} > +EXPORT_SYMBOL_GPL(vfio_pci_set_vendor_regions); > + > + > +int vfio_pci_set_vendor_irqs(void *device_data, int num_vendor_irqs) > +{ > + struct vfio_pci_device *vdev = device_data; > + > + vdev->num_vendor_irqs = num_vendor_irqs; Here as well. > + return 0; > +} > +EXPORT_SYMBOL_GPL(vfio_pci_set_vendor_irqs); > /* > * Our VGA arbiter participation is limited since we don't know anything > * about the device itself. However, if the device is the only VGA device (...)
On Thu, Jun 04, 2020 at 05:25:15PM +0200, Cornelia Huck wrote: > On Sun, 17 May 2020 22:49:44 -0400 > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > This allows a simpler VFIO_DEVICE_GET_INFO ioctl in vendor driver > > > > Cc: Kevin Tian <kevin.tian@intel.com> > > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > > --- > > drivers/vfio/pci/vfio_pci.c | 23 +++++++++++++++++++++-- > > drivers/vfio/pci/vfio_pci_private.h | 2 ++ > > include/linux/vfio.h | 3 +++ > > 3 files changed, 26 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > > index 290b7ab55ecf..30137c1c5308 100644 > > --- a/drivers/vfio/pci/vfio_pci.c > > +++ b/drivers/vfio/pci/vfio_pci.c > > @@ -105,6 +105,24 @@ void *vfio_pci_vendor_data(void *device_data) > > } > > EXPORT_SYMBOL_GPL(vfio_pci_vendor_data); > > > > +int vfio_pci_set_vendor_regions(void *device_data, int num_vendor_regions) > > +{ > > + struct vfio_pci_device *vdev = device_data; > > + > > + vdev->num_vendor_regions = num_vendor_regions; > > Do we need any kind of sanity check here, in case this is called with a > bogus value? > you are right. it at least needs to be >=0. maybe type of "unsigned int" is more appropriate for num_vendor_regions. we don't need to check its max value as QEMU would check it. > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(vfio_pci_set_vendor_regions); > > + > > + > > +int vfio_pci_set_vendor_irqs(void *device_data, int num_vendor_irqs) > > +{ > > + struct vfio_pci_device *vdev = device_data; > > + > > + vdev->num_vendor_irqs = num_vendor_irqs; > > Here as well. yes. will change the type to "unsigned int". Thank you for kindly reviewing:) Yan > > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(vfio_pci_set_vendor_irqs); > > /* > > * Our VGA arbiter participation is limited since we don't know anything > > * about the device itself. However, if the device is the only VGA device > > (...) >
On Thursday, 2020-06-04 at 22:15:42 -04, Yan Zhao wrote: > On Thu, Jun 04, 2020 at 05:25:15PM +0200, Cornelia Huck wrote: >> On Sun, 17 May 2020 22:49:44 -0400 >> Yan Zhao <yan.y.zhao@intel.com> wrote: >> >> > This allows a simpler VFIO_DEVICE_GET_INFO ioctl in vendor driver >> > >> > Cc: Kevin Tian <kevin.tian@intel.com> >> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> >> > --- >> > drivers/vfio/pci/vfio_pci.c | 23 +++++++++++++++++++++-- >> > drivers/vfio/pci/vfio_pci_private.h | 2 ++ >> > include/linux/vfio.h | 3 +++ >> > 3 files changed, 26 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c >> > index 290b7ab55ecf..30137c1c5308 100644 >> > --- a/drivers/vfio/pci/vfio_pci.c >> > +++ b/drivers/vfio/pci/vfio_pci.c >> > @@ -105,6 +105,24 @@ void *vfio_pci_vendor_data(void *device_data) >> > } >> > EXPORT_SYMBOL_GPL(vfio_pci_vendor_data); >> > >> > +int vfio_pci_set_vendor_regions(void *device_data, int num_vendor_regions) >> > +{ >> > + struct vfio_pci_device *vdev = device_data; >> > + >> > + vdev->num_vendor_regions = num_vendor_regions; >> >> Do we need any kind of sanity check here, in case this is called with a >> bogus value? >> > you are right. it at least needs to be >=0. > maybe type of "unsigned int" is more appropriate for num_vendor_regions. > we don't need to check its max value as QEMU would check it. That seems like a bad precedent - the caller may not be QEMU. dme.
On Thu, Jun 11, 2020 at 01:31:05PM +0100, David Edmondson wrote: > On Thursday, 2020-06-04 at 22:15:42 -04, Yan Zhao wrote: > > > On Thu, Jun 04, 2020 at 05:25:15PM +0200, Cornelia Huck wrote: > >> On Sun, 17 May 2020 22:49:44 -0400 > >> Yan Zhao <yan.y.zhao@intel.com> wrote: > >> > >> > This allows a simpler VFIO_DEVICE_GET_INFO ioctl in vendor driver > >> > > >> > Cc: Kevin Tian <kevin.tian@intel.com> > >> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > >> > --- > >> > drivers/vfio/pci/vfio_pci.c | 23 +++++++++++++++++++++-- > >> > drivers/vfio/pci/vfio_pci_private.h | 2 ++ > >> > include/linux/vfio.h | 3 +++ > >> > 3 files changed, 26 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > >> > index 290b7ab55ecf..30137c1c5308 100644 > >> > --- a/drivers/vfio/pci/vfio_pci.c > >> > +++ b/drivers/vfio/pci/vfio_pci.c > >> > @@ -105,6 +105,24 @@ void *vfio_pci_vendor_data(void *device_data) > >> > } > >> > EXPORT_SYMBOL_GPL(vfio_pci_vendor_data); > >> > > >> > +int vfio_pci_set_vendor_regions(void *device_data, int num_vendor_regions) > >> > +{ > >> > + struct vfio_pci_device *vdev = device_data; > >> > + > >> > + vdev->num_vendor_regions = num_vendor_regions; > >> > >> Do we need any kind of sanity check here, in case this is called with a > >> bogus value? > >> > > you are right. it at least needs to be >=0. > > maybe type of "unsigned int" is more appropriate for num_vendor_regions. > > we don't need to check its max value as QEMU would check it. > > That seems like a bad precedent - the caller may not be QEMU. > but the caller has to query that through vfio_pci_ioctl() and at there info.num_regions = VFIO_PCI_NUM_REGIONS + vdev->num_regions + vdev->num_vendor_regions; info.num_regions is of type __u32. Thanks Yan
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c index 290b7ab55ecf..30137c1c5308 100644 --- a/drivers/vfio/pci/vfio_pci.c +++ b/drivers/vfio/pci/vfio_pci.c @@ -105,6 +105,24 @@ void *vfio_pci_vendor_data(void *device_data) } EXPORT_SYMBOL_GPL(vfio_pci_vendor_data); +int vfio_pci_set_vendor_regions(void *device_data, int num_vendor_regions) +{ + struct vfio_pci_device *vdev = device_data; + + vdev->num_vendor_regions = num_vendor_regions; + return 0; +} +EXPORT_SYMBOL_GPL(vfio_pci_set_vendor_regions); + + +int vfio_pci_set_vendor_irqs(void *device_data, int num_vendor_irqs) +{ + struct vfio_pci_device *vdev = device_data; + + vdev->num_vendor_irqs = num_vendor_irqs; + return 0; +} +EXPORT_SYMBOL_GPL(vfio_pci_set_vendor_irqs); /* * Our VGA arbiter participation is limited since we don't know anything * about the device itself. However, if the device is the only VGA device @@ -797,8 +815,9 @@ long vfio_pci_ioctl(void *device_data, if (vdev->reset_works) info.flags |= VFIO_DEVICE_FLAGS_RESET; - info.num_regions = VFIO_PCI_NUM_REGIONS + vdev->num_regions; - info.num_irqs = VFIO_PCI_NUM_IRQS; + info.num_regions = VFIO_PCI_NUM_REGIONS + vdev->num_regions + + vdev->num_vendor_regions; + info.num_irqs = VFIO_PCI_NUM_IRQS + vdev->num_vendor_irqs; return copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0; diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h index 7758a20546fa..c6cfc4605987 100644 --- a/drivers/vfio/pci/vfio_pci_private.h +++ b/drivers/vfio/pci/vfio_pci_private.h @@ -110,6 +110,8 @@ struct vfio_pci_device { int num_ctx; int irq_type; int num_regions; + int num_vendor_regions; + int num_vendor_irqs; struct vfio_pci_region *region; u8 msi_qmax; u8 msix_bar; diff --git a/include/linux/vfio.h b/include/linux/vfio.h index 6ededceb1964..6310c53f9d36 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -231,6 +231,9 @@ extern void vfio_pci_release(void *device_data); extern int vfio_pci_match(void *device_data, char *buf); extern void *vfio_pci_vendor_data(void *device_data); +extern int vfio_pci_set_vendor_regions(void *device_data, + int num_vendor_regions); +extern int vfio_pci_set_vendor_irqs(void *device_data, int num_vendor_irqs); struct vfio_pci_vendor_driver_ops { char *name;
This allows a simpler VFIO_DEVICE_GET_INFO ioctl in vendor driver Cc: Kevin Tian <kevin.tian@intel.com> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> --- drivers/vfio/pci/vfio_pci.c | 23 +++++++++++++++++++++-- drivers/vfio/pci/vfio_pci_private.h | 2 ++ include/linux/vfio.h | 3 +++ 3 files changed, 26 insertions(+), 2 deletions(-)