[06/15] fpga: dfl: fme: add DFL_FPGA_FME_PORT_RELEASE/ASSIGN ioctl support.
diff mbox series

Message ID 20190628004951.6202-7-mdf@kernel.org
State Awaiting Upstream
Headers show
Series
  • FPGA DFL updates
Related show

Commit Message

Moritz Fischer June 28, 2019, 12:49 a.m. UTC
From: Wu Hao <hao.wu@intel.com>

In order to support virtualization usage via PCIe SRIOV, this patch
adds two ioctls under FPGA Management Engine (FME) to release and
assign back the port device. In order to safely turn Port from PF
into VF and enable PCIe SRIOV, it requires user to invoke this
PORT_RELEASE ioctl to release port firstly to remove userspace
interfaces, and then configure the PF/VF access register in FME.
After disable SRIOV, it requires user to invoke this PORT_ASSIGN
ioctl to attach the port back to PF.

 Ioctl interfaces:
 * DFL_FPGA_FME_PORT_RELEASE
   Release platform device of given port, it deletes port platform
   device to remove related userspace interfaces on PF, then
   configures PF/VF access mode to VF.

 * DFL_FPGA_FME_PORT_ASSIGN
   Assign platform device of given port back to PF, it configures
   PF/VF access mode to PF, then adds port platform device back to
   re-enable related userspace interfaces on PF.

Signed-off-by: Zhang Yi Z <yi.z.zhang@intel.com>
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
Acked-by: Moritz Fischer <mdf@kernel.org>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 drivers/fpga/dfl-fme-main.c   |  54 +++++++++++++++++
 drivers/fpga/dfl.c            | 107 ++++++++++++++++++++++++++++++----
 drivers/fpga/dfl.h            |  10 ++++
 include/uapi/linux/fpga-dfl.h |  32 ++++++++++
 4 files changed, 191 insertions(+), 12 deletions(-)

Comments

Greg Kroah-Hartman July 3, 2019, 6:07 p.m. UTC | #1
On Thu, Jun 27, 2019 at 05:49:42PM -0700, Moritz Fischer wrote:
> From: Wu Hao <hao.wu@intel.com>
> 
> In order to support virtualization usage via PCIe SRIOV, this patch
> adds two ioctls under FPGA Management Engine (FME) to release and
> assign back the port device. In order to safely turn Port from PF
> into VF and enable PCIe SRIOV, it requires user to invoke this
> PORT_RELEASE ioctl to release port firstly to remove userspace
> interfaces, and then configure the PF/VF access register in FME.
> After disable SRIOV, it requires user to invoke this PORT_ASSIGN
> ioctl to attach the port back to PF.
> 
>  Ioctl interfaces:
>  * DFL_FPGA_FME_PORT_RELEASE
>    Release platform device of given port, it deletes port platform
>    device to remove related userspace interfaces on PF, then
>    configures PF/VF access mode to VF.
> 
>  * DFL_FPGA_FME_PORT_ASSIGN
>    Assign platform device of given port back to PF, it configures
>    PF/VF access mode to PF, then adds port platform device back to
>    re-enable related userspace interfaces on PF.

Why are you not using the "generic" bind/unbind facility that userspace
already has for this with binding drivers to devices?  Why a special
ioctl?

> --- a/include/uapi/linux/fpga-dfl.h
> +++ b/include/uapi/linux/fpga-dfl.h
> @@ -176,4 +176,36 @@ struct dfl_fpga_fme_port_pr {
>  
>  #define DFL_FPGA_FME_PORT_PR	_IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 0)
>  
> +/**
> + * DFL_FPGA_FME_PORT_RELEASE - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 1,
> + *					struct dfl_fpga_fme_port_release)
> + *
> + * Driver releases the port per Port ID provided by caller.
> + * Return: 0 on success, -errno on failure.
> + */
> +struct dfl_fpga_fme_port_release {
> +	/* Input */
> +	__u32 argsz;		/* Structure length */
> +	__u32 flags;		/* Zero for now */
> +	__u32 port_id;
> +};

meta-comment, why do all of your structures for ioctls have argsz?  You
"know" the size of the structure already, it's part of the ioctl
definition.  You shouldn't need to also set it again, right?  Otherwise
ALL Linux ioctls would need something crazy like this.

thanks,

greg k-h
Wu, Hao July 3, 2019, 11:30 p.m. UTC | #2
On Wed, Jul 03, 2019 at 08:07:53PM +0200, Greg KH wrote:
> On Thu, Jun 27, 2019 at 05:49:42PM -0700, Moritz Fischer wrote:
> > From: Wu Hao <hao.wu@intel.com>
> > 
> > In order to support virtualization usage via PCIe SRIOV, this patch
> > adds two ioctls under FPGA Management Engine (FME) to release and
> > assign back the port device. In order to safely turn Port from PF
> > into VF and enable PCIe SRIOV, it requires user to invoke this
> > PORT_RELEASE ioctl to release port firstly to remove userspace
> > interfaces, and then configure the PF/VF access register in FME.
> > After disable SRIOV, it requires user to invoke this PORT_ASSIGN
> > ioctl to attach the port back to PF.
> > 
> >  Ioctl interfaces:
> >  * DFL_FPGA_FME_PORT_RELEASE
> >    Release platform device of given port, it deletes port platform
> >    device to remove related userspace interfaces on PF, then
> >    configures PF/VF access mode to VF.
> > 
> >  * DFL_FPGA_FME_PORT_ASSIGN
> >    Assign platform device of given port back to PF, it configures
> >    PF/VF access mode to PF, then adds port platform device back to
> >    re-enable related userspace interfaces on PF.
> 
> Why are you not using the "generic" bind/unbind facility that userspace
> already has for this with binding drivers to devices?  Why a special
> ioctl?

Hi Greg,

Actually we think it should be safer that making the device invisble than
just unbinding its driver. Looks like user can try to rebind it at any
time and we don't have any method to stop them.

> 
> > --- a/include/uapi/linux/fpga-dfl.h
> > +++ b/include/uapi/linux/fpga-dfl.h
> > @@ -176,4 +176,36 @@ struct dfl_fpga_fme_port_pr {
> >  
> >  #define DFL_FPGA_FME_PORT_PR	_IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 0)
> >  
> > +/**
> > + * DFL_FPGA_FME_PORT_RELEASE - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 1,
> > + *					struct dfl_fpga_fme_port_release)
> > + *
> > + * Driver releases the port per Port ID provided by caller.
> > + * Return: 0 on success, -errno on failure.
> > + */
> > +struct dfl_fpga_fme_port_release {
> > +	/* Input */
> > +	__u32 argsz;		/* Structure length */
> > +	__u32 flags;		/* Zero for now */
> > +	__u32 port_id;
> > +};
> 
> meta-comment, why do all of your structures for ioctls have argsz?  You
> "know" the size of the structure already, it's part of the ioctl
> definition.  You shouldn't need to also set it again, right?  Otherwise
> ALL Linux ioctls would need something crazy like this.

Actually we followed the same method as vfio. The major purpose should be
extendibility, as we really need this to be sth long term maintainable.
It really helps, if we add some new members for extentions/enhancement
under the same ioctl. I don't think everybody needs this, but my
consideration here is if newer generations of hardware/specs come with some
extentions, I still hope we can resue these IOCTLs as much as we could,
instead of creating more new ones.

Thanks
Hao

> 
> thanks,
> 
> greg k-h
Greg Kroah-Hartman July 4, 2019, 5:39 a.m. UTC | #3
On Thu, Jul 04, 2019 at 07:30:58AM +0800, Wu Hao wrote:
> On Wed, Jul 03, 2019 at 08:07:53PM +0200, Greg KH wrote:
> > On Thu, Jun 27, 2019 at 05:49:42PM -0700, Moritz Fischer wrote:
> > > From: Wu Hao <hao.wu@intel.com>
> > > 
> > > In order to support virtualization usage via PCIe SRIOV, this patch
> > > adds two ioctls under FPGA Management Engine (FME) to release and
> > > assign back the port device. In order to safely turn Port from PF
> > > into VF and enable PCIe SRIOV, it requires user to invoke this
> > > PORT_RELEASE ioctl to release port firstly to remove userspace
> > > interfaces, and then configure the PF/VF access register in FME.
> > > After disable SRIOV, it requires user to invoke this PORT_ASSIGN
> > > ioctl to attach the port back to PF.
> > > 
> > >  Ioctl interfaces:
> > >  * DFL_FPGA_FME_PORT_RELEASE
> > >    Release platform device of given port, it deletes port platform
> > >    device to remove related userspace interfaces on PF, then
> > >    configures PF/VF access mode to VF.
> > > 
> > >  * DFL_FPGA_FME_PORT_ASSIGN
> > >    Assign platform device of given port back to PF, it configures
> > >    PF/VF access mode to PF, then adds port platform device back to
> > >    re-enable related userspace interfaces on PF.
> > 
> > Why are you not using the "generic" bind/unbind facility that userspace
> > already has for this with binding drivers to devices?  Why a special
> > ioctl?
> 
> Hi Greg,
> 
> Actually we think it should be safer that making the device invisble than
> just unbinding its driver. Looks like user can try to rebind it at any
> time and we don't have any method to stop them.

Why do you want to "stop" the user from doing something?  They asked to
do it, why prevent it?  If they ask to do something foolish, well, they
get to keep the pieces :)

> > > --- a/include/uapi/linux/fpga-dfl.h
> > > +++ b/include/uapi/linux/fpga-dfl.h
> > > @@ -176,4 +176,36 @@ struct dfl_fpga_fme_port_pr {
> > >  
> > >  #define DFL_FPGA_FME_PORT_PR	_IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 0)
> > >  
> > > +/**
> > > + * DFL_FPGA_FME_PORT_RELEASE - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 1,
> > > + *					struct dfl_fpga_fme_port_release)
> > > + *
> > > + * Driver releases the port per Port ID provided by caller.
> > > + * Return: 0 on success, -errno on failure.
> > > + */
> > > +struct dfl_fpga_fme_port_release {
> > > +	/* Input */
> > > +	__u32 argsz;		/* Structure length */
> > > +	__u32 flags;		/* Zero for now */
> > > +	__u32 port_id;
> > > +};
> > 
> > meta-comment, why do all of your structures for ioctls have argsz?  You
> > "know" the size of the structure already, it's part of the ioctl
> > definition.  You shouldn't need to also set it again, right?  Otherwise
> > ALL Linux ioctls would need something crazy like this.
> 
> Actually we followed the same method as vfio.

vfio is a protocol on "the wire", right?  Not an ioctl.

> The major purpose should be extendibility, as we really need this to
> be sth long term maintainable.

You can't change ioctl structure sizes at any time.

> It really helps, if we add some new members for extentions/enhancement
> under the same ioctl.

You don't do that.

> I don't think everybody needs this, but my consideration here is if
> newer generations of hardware/specs come with some extentions, I still
> hope we can resue these IOCTLs as much as we could, instead of
> creating more new ones.

You create new ones, like everyone else does, as you can not change old
code.  By trying to "version" structures like this, it's just going to
be a nightmare.

thanks,

greg k-h
Wu, Hao July 4, 2019, 6:31 a.m. UTC | #4
On Thu, Jul 04, 2019 at 07:39:27AM +0200, Greg KH wrote:
> On Thu, Jul 04, 2019 at 07:30:58AM +0800, Wu Hao wrote:
> > On Wed, Jul 03, 2019 at 08:07:53PM +0200, Greg KH wrote:
> > > On Thu, Jun 27, 2019 at 05:49:42PM -0700, Moritz Fischer wrote:
> > > > From: Wu Hao <hao.wu@intel.com>
> > > > 
> > > > In order to support virtualization usage via PCIe SRIOV, this patch
> > > > adds two ioctls under FPGA Management Engine (FME) to release and
> > > > assign back the port device. In order to safely turn Port from PF
> > > > into VF and enable PCIe SRIOV, it requires user to invoke this
> > > > PORT_RELEASE ioctl to release port firstly to remove userspace
> > > > interfaces, and then configure the PF/VF access register in FME.
> > > > After disable SRIOV, it requires user to invoke this PORT_ASSIGN
> > > > ioctl to attach the port back to PF.
> > > > 
> > > >  Ioctl interfaces:
> > > >  * DFL_FPGA_FME_PORT_RELEASE
> > > >    Release platform device of given port, it deletes port platform
> > > >    device to remove related userspace interfaces on PF, then
> > > >    configures PF/VF access mode to VF.
> > > > 
> > > >  * DFL_FPGA_FME_PORT_ASSIGN
> > > >    Assign platform device of given port back to PF, it configures
> > > >    PF/VF access mode to PF, then adds port platform device back to
> > > >    re-enable related userspace interfaces on PF.
> > > 
> > > Why are you not using the "generic" bind/unbind facility that userspace
> > > already has for this with binding drivers to devices?  Why a special
> > > ioctl?
> > 
> > Hi Greg,
> > 
> > Actually we think it should be safer that making the device invisble than
> > just unbinding its driver. Looks like user can try to rebind it at any
> > time and we don't have any method to stop them.
> 
> Why do you want to "stop" the user from doing something?  They asked to
> do it, why prevent it?  If they ask to do something foolish, well, they
> get to keep the pieces :)

Actually this is for SRIOV support, as we are moving FPGA accelerator from
PF to VF, so we don't want users to see the FPGA accelerator from PF any
more. We can't allow user to touch same FPGA accelerator from both PF and
VF side (it leads to hardware erros).

> 
> > > > --- a/include/uapi/linux/fpga-dfl.h
> > > > +++ b/include/uapi/linux/fpga-dfl.h
> > > > @@ -176,4 +176,36 @@ struct dfl_fpga_fme_port_pr {
> > > >  
> > > >  #define DFL_FPGA_FME_PORT_PR	_IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 0)
> > > >  
> > > > +/**
> > > > + * DFL_FPGA_FME_PORT_RELEASE - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 1,
> > > > + *					struct dfl_fpga_fme_port_release)
> > > > + *
> > > > + * Driver releases the port per Port ID provided by caller.
> > > > + * Return: 0 on success, -errno on failure.
> > > > + */
> > > > +struct dfl_fpga_fme_port_release {
> > > > +	/* Input */
> > > > +	__u32 argsz;		/* Structure length */
> > > > +	__u32 flags;		/* Zero for now */
> > > > +	__u32 port_id;
> > > > +};
> > > 
> > > meta-comment, why do all of your structures for ioctls have argsz?  You
> > > "know" the size of the structure already, it's part of the ioctl
> > > definition.  You shouldn't need to also set it again, right?  Otherwise
> > > ALL Linux ioctls would need something crazy like this.
> > 
> > Actually we followed the same method as vfio.
> 
> vfio is a protocol on "the wire", right?  Not an ioctl.
> 
> > The major purpose should be extendibility, as we really need this to
> > be sth long term maintainable.
> 
> You can't change ioctl structure sizes at any time.
> 
> > It really helps, if we add some new members for extentions/enhancement
> > under the same ioctl.
> 
> You don't do that.
> 
> > I don't think everybody needs this, but my consideration here is if
> > newer generations of hardware/specs come with some extentions, I still
> > hope we can resue these IOCTLs as much as we could, instead of
> > creating more new ones.
> 
> You create new ones, like everyone else does, as you can not change old
> code.  By trying to "version" structures like this, it's just going to
> be a nightmare.

Actually i learned this from vfio code here, it's not trying to "version"
structures, let me copy the comments from vfio header file. It should be
more clear than above short description from me.

 "include/uapi/linux/vfio.h"

 /*
  * The IOCTL interface is designed for extensibility by embedding the
  * structure length (argsz) and flags into structures passed between
  * kernel and userspace.  We therefore use the _IO() macro for these
  * defines to avoid implicitly embedding a size into the ioctl request.
  * As structure fields are added, argsz will increase to match and flag
  * bits will be defined to indicate additional fields with valid data.
  * It's *always* the caller's responsibility to indicate the size of
  * the structure passed by setting argsz appropriately.
  */

 For example.

 struct vfio_device_info {
        __u32   argsz;
        __u32   flags;
 #define VFIO_DEVICE_FLAGS_RESET (1 << 0)        /* Device supports reset */
 #define VFIO_DEVICE_FLAGS_PCI   (1 << 1)        /* vfio-pci device */
 #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)     /* vfio-platform device */
 #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)        /* vfio-amba device */
 #define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */
 #define VFIO_DEVICE_FLAGS_AP    (1 << 5)        /* vfio-ap device */
        __u32   num_regions;    /* Max region index + 1 */
        __u32   num_irqs;       /* Max IRQ index + 1 */

Hope things could be more clear now. :)

Thanks
Hao

};

> 
> thanks,
> 
> greg k-h
Greg Kroah-Hartman July 4, 2019, 8:20 a.m. UTC | #5
On Thu, Jul 04, 2019 at 02:31:06PM +0800, Wu Hao wrote:
> On Thu, Jul 04, 2019 at 07:39:27AM +0200, Greg KH wrote:
> > On Thu, Jul 04, 2019 at 07:30:58AM +0800, Wu Hao wrote:
> > > On Wed, Jul 03, 2019 at 08:07:53PM +0200, Greg KH wrote:
> > > > On Thu, Jun 27, 2019 at 05:49:42PM -0700, Moritz Fischer wrote:
> > > > > From: Wu Hao <hao.wu@intel.com>
> > > > > 
> > > > > In order to support virtualization usage via PCIe SRIOV, this patch
> > > > > adds two ioctls under FPGA Management Engine (FME) to release and
> > > > > assign back the port device. In order to safely turn Port from PF
> > > > > into VF and enable PCIe SRIOV, it requires user to invoke this
> > > > > PORT_RELEASE ioctl to release port firstly to remove userspace
> > > > > interfaces, and then configure the PF/VF access register in FME.
> > > > > After disable SRIOV, it requires user to invoke this PORT_ASSIGN
> > > > > ioctl to attach the port back to PF.
> > > > > 
> > > > >  Ioctl interfaces:
> > > > >  * DFL_FPGA_FME_PORT_RELEASE
> > > > >    Release platform device of given port, it deletes port platform
> > > > >    device to remove related userspace interfaces on PF, then
> > > > >    configures PF/VF access mode to VF.
> > > > > 
> > > > >  * DFL_FPGA_FME_PORT_ASSIGN
> > > > >    Assign platform device of given port back to PF, it configures
> > > > >    PF/VF access mode to PF, then adds port platform device back to
> > > > >    re-enable related userspace interfaces on PF.
> > > > 
> > > > Why are you not using the "generic" bind/unbind facility that userspace
> > > > already has for this with binding drivers to devices?  Why a special
> > > > ioctl?
> > > 
> > > Hi Greg,
> > > 
> > > Actually we think it should be safer that making the device invisble than
> > > just unbinding its driver. Looks like user can try to rebind it at any
> > > time and we don't have any method to stop them.
> > 
> > Why do you want to "stop" the user from doing something?  They asked to
> > do it, why prevent it?  If they ask to do something foolish, well, they
> > get to keep the pieces :)
> 
> Actually this is for SRIOV support, as we are moving FPGA accelerator from
> PF to VF, so we don't want users to see the FPGA accelerator from PF any
> more. We can't allow user to touch same FPGA accelerator from both PF and
> VF side (it leads to hardware erros).

Ick, ok, this needs to be documented really well then.

> > > > > --- a/include/uapi/linux/fpga-dfl.h
> > > > > +++ b/include/uapi/linux/fpga-dfl.h
> > > > > @@ -176,4 +176,36 @@ struct dfl_fpga_fme_port_pr {
> > > > >  
> > > > >  #define DFL_FPGA_FME_PORT_PR	_IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 0)
> > > > >  
> > > > > +/**
> > > > > + * DFL_FPGA_FME_PORT_RELEASE - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 1,
> > > > > + *					struct dfl_fpga_fme_port_release)
> > > > > + *
> > > > > + * Driver releases the port per Port ID provided by caller.
> > > > > + * Return: 0 on success, -errno on failure.
> > > > > + */
> > > > > +struct dfl_fpga_fme_port_release {
> > > > > +	/* Input */
> > > > > +	__u32 argsz;		/* Structure length */
> > > > > +	__u32 flags;		/* Zero for now */
> > > > > +	__u32 port_id;
> > > > > +};
> > > > 
> > > > meta-comment, why do all of your structures for ioctls have argsz?  You
> > > > "know" the size of the structure already, it's part of the ioctl
> > > > definition.  You shouldn't need to also set it again, right?  Otherwise
> > > > ALL Linux ioctls would need something crazy like this.
> > > 
> > > Actually we followed the same method as vfio.
> > 
> > vfio is a protocol on "the wire", right?  Not an ioctl.
> > 
> > > The major purpose should be extendibility, as we really need this to
> > > be sth long term maintainable.
> > 
> > You can't change ioctl structure sizes at any time.
> > 
> > > It really helps, if we add some new members for extentions/enhancement
> > > under the same ioctl.
> > 
> > You don't do that.
> > 
> > > I don't think everybody needs this, but my consideration here is if
> > > newer generations of hardware/specs come with some extentions, I still
> > > hope we can resue these IOCTLs as much as we could, instead of
> > > creating more new ones.
> > 
> > You create new ones, like everyone else does, as you can not change old
> > code.  By trying to "version" structures like this, it's just going to
> > be a nightmare.
> 
> Actually i learned this from vfio code here, it's not trying to "version"
> structures, let me copy the comments from vfio header file. It should be
> more clear than above short description from me.
> 
>  "include/uapi/linux/vfio.h"
> 
>  /*
>   * The IOCTL interface is designed for extensibility by embedding the
>   * structure length (argsz) and flags into structures passed between
>   * kernel and userspace.  We therefore use the _IO() macro for these
>   * defines to avoid implicitly embedding a size into the ioctl request.
>   * As structure fields are added, argsz will increase to match and flag
>   * bits will be defined to indicate additional fields with valid data.
>   * It's *always* the caller's responsibility to indicate the size of
>   * the structure passed by setting argsz appropriately.
>   */
> 
>  For example.
> 
>  struct vfio_device_info {
>         __u32   argsz;
>         __u32   flags;
>  #define VFIO_DEVICE_FLAGS_RESET (1 << 0)        /* Device supports reset */
>  #define VFIO_DEVICE_FLAGS_PCI   (1 << 1)        /* vfio-pci device */
>  #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)     /* vfio-platform device */
>  #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)        /* vfio-amba device */
>  #define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */
>  #define VFIO_DEVICE_FLAGS_AP    (1 << 5)        /* vfio-ap device */
>         __u32   num_regions;    /* Max region index + 1 */
>         __u32   num_irqs;       /* Max IRQ index + 1 */
> 
> Hope things could be more clear now. :)

That's nice for the vfio stuff, but you are just a "normal" driver here.
You want an ioctl that just does one thing, no arguments, no flags, no
anything.  No need for a size argument then at all.  These ioctls don't
even need a structure for them!

Don't try to be fancy, it's not needed, it's not like you are running
out of ioctl space...

thanks,

greg k-h
Wu, Hao July 4, 2019, 8:58 a.m. UTC | #6
On Thu, Jul 04, 2019 at 10:20:13AM +0200, Greg KH wrote:
> On Thu, Jul 04, 2019 at 02:31:06PM +0800, Wu Hao wrote:
> > On Thu, Jul 04, 2019 at 07:39:27AM +0200, Greg KH wrote:
> > > On Thu, Jul 04, 2019 at 07:30:58AM +0800, Wu Hao wrote:
> > > > On Wed, Jul 03, 2019 at 08:07:53PM +0200, Greg KH wrote:
> > > > > On Thu, Jun 27, 2019 at 05:49:42PM -0700, Moritz Fischer wrote:
> > > > > > From: Wu Hao <hao.wu@intel.com>
> > > > > > 
> > > > > > In order to support virtualization usage via PCIe SRIOV, this patch
> > > > > > adds two ioctls under FPGA Management Engine (FME) to release and
> > > > > > assign back the port device. In order to safely turn Port from PF
> > > > > > into VF and enable PCIe SRIOV, it requires user to invoke this
> > > > > > PORT_RELEASE ioctl to release port firstly to remove userspace
> > > > > > interfaces, and then configure the PF/VF access register in FME.
> > > > > > After disable SRIOV, it requires user to invoke this PORT_ASSIGN
> > > > > > ioctl to attach the port back to PF.
> > > > > > 
> > > > > >  Ioctl interfaces:
> > > > > >  * DFL_FPGA_FME_PORT_RELEASE
> > > > > >    Release platform device of given port, it deletes port platform
> > > > > >    device to remove related userspace interfaces on PF, then
> > > > > >    configures PF/VF access mode to VF.
> > > > > > 
> > > > > >  * DFL_FPGA_FME_PORT_ASSIGN
> > > > > >    Assign platform device of given port back to PF, it configures
> > > > > >    PF/VF access mode to PF, then adds port platform device back to
> > > > > >    re-enable related userspace interfaces on PF.
> > > > > 
> > > > > Why are you not using the "generic" bind/unbind facility that userspace
> > > > > already has for this with binding drivers to devices?  Why a special
> > > > > ioctl?
> > > > 
> > > > Hi Greg,
> > > > 
> > > > Actually we think it should be safer that making the device invisble than
> > > > just unbinding its driver. Looks like user can try to rebind it at any
> > > > time and we don't have any method to stop them.
> > > 
> > > Why do you want to "stop" the user from doing something?  They asked to
> > > do it, why prevent it?  If they ask to do something foolish, well, they
> > > get to keep the pieces :)
> > 
> > Actually this is for SRIOV support, as we are moving FPGA accelerator from
> > PF to VF, so we don't want users to see the FPGA accelerator from PF any
> > more. We can't allow user to touch same FPGA accelerator from both PF and
> > VF side (it leads to hardware erros).
> 
> Ick, ok, this needs to be documented really well then.

Yes, we have add relateded descriptions for virtualization support in that
documentation patch.

[PATCH 05/15] Documentation: fpga: dfl: add descriptions for virtualization
and new interfaces.

> 
> > > > > > --- a/include/uapi/linux/fpga-dfl.h
> > > > > > +++ b/include/uapi/linux/fpga-dfl.h
> > > > > > @@ -176,4 +176,36 @@ struct dfl_fpga_fme_port_pr {
> > > > > >  
> > > > > >  #define DFL_FPGA_FME_PORT_PR	_IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 0)
> > > > > >  
> > > > > > +/**
> > > > > > + * DFL_FPGA_FME_PORT_RELEASE - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 1,
> > > > > > + *					struct dfl_fpga_fme_port_release)
> > > > > > + *
> > > > > > + * Driver releases the port per Port ID provided by caller.
> > > > > > + * Return: 0 on success, -errno on failure.
> > > > > > + */
> > > > > > +struct dfl_fpga_fme_port_release {
> > > > > > +	/* Input */
> > > > > > +	__u32 argsz;		/* Structure length */
> > > > > > +	__u32 flags;		/* Zero for now */
> > > > > > +	__u32 port_id;
> > > > > > +};
> > > > > 
> > > > > meta-comment, why do all of your structures for ioctls have argsz?  You
> > > > > "know" the size of the structure already, it's part of the ioctl
> > > > > definition.  You shouldn't need to also set it again, right?  Otherwise
> > > > > ALL Linux ioctls would need something crazy like this.
> > > > 
> > > > Actually we followed the same method as vfio.
> > > 
> > > vfio is a protocol on "the wire", right?  Not an ioctl.
> > > 
> > > > The major purpose should be extendibility, as we really need this to
> > > > be sth long term maintainable.
> > > 
> > > You can't change ioctl structure sizes at any time.
> > > 
> > > > It really helps, if we add some new members for extentions/enhancement
> > > > under the same ioctl.
> > > 
> > > You don't do that.
> > > 
> > > > I don't think everybody needs this, but my consideration here is if
> > > > newer generations of hardware/specs come with some extentions, I still
> > > > hope we can resue these IOCTLs as much as we could, instead of
> > > > creating more new ones.
> > > 
> > > You create new ones, like everyone else does, as you can not change old
> > > code.  By trying to "version" structures like this, it's just going to
> > > be a nightmare.
> > 
> > Actually i learned this from vfio code here, it's not trying to "version"
> > structures, let me copy the comments from vfio header file. It should be
> > more clear than above short description from me.
> > 
> >  "include/uapi/linux/vfio.h"
> > 
> >  /*
> >   * The IOCTL interface is designed for extensibility by embedding the
> >   * structure length (argsz) and flags into structures passed between
> >   * kernel and userspace.  We therefore use the _IO() macro for these
> >   * defines to avoid implicitly embedding a size into the ioctl request.
> >   * As structure fields are added, argsz will increase to match and flag
> >   * bits will be defined to indicate additional fields with valid data.
> >   * It's *always* the caller's responsibility to indicate the size of
> >   * the structure passed by setting argsz appropriately.
> >   */
> > 
> >  For example.
> > 
> >  struct vfio_device_info {
> >         __u32   argsz;
> >         __u32   flags;
> >  #define VFIO_DEVICE_FLAGS_RESET (1 << 0)        /* Device supports reset */
> >  #define VFIO_DEVICE_FLAGS_PCI   (1 << 1)        /* vfio-pci device */
> >  #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)     /* vfio-platform device */
> >  #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)        /* vfio-amba device */
> >  #define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */
> >  #define VFIO_DEVICE_FLAGS_AP    (1 << 5)        /* vfio-ap device */
> >         __u32   num_regions;    /* Max region index + 1 */
> >         __u32   num_irqs;       /* Max IRQ index + 1 */
> > 
> > Hope things could be more clear now. :)
> 
> That's nice for the vfio stuff, but you are just a "normal" driver here.
> You want an ioctl that just does one thing, no arguments, no flags, no
> anything.  No need for a size argument then at all.  These ioctls don't
> even need a structure for them!
> 
> Don't try to be fancy, it's not needed, it's not like you are running
> out of ioctl space...

Thanks a lot for the comments and suggestions.

That's true, it's a "normal" driver, maybe I overly considered the
extensibility of it. OK, Let me rework this patch to remove argsz from
these two ioctls.

What about the existing ioctls for this driver, they have argsz too.
shall I prepare another patch to remove them as well?

Hao

> 
> thanks,
> 
> greg k-h
Greg Kroah-Hartman July 4, 2019, 11:04 a.m. UTC | #7
On Thu, Jul 04, 2019 at 04:58:55PM +0800, Wu Hao wrote:
> > > Hope things could be more clear now. :)
> > 
> > That's nice for the vfio stuff, but you are just a "normal" driver here.
> > You want an ioctl that just does one thing, no arguments, no flags, no
> > anything.  No need for a size argument then at all.  These ioctls don't
> > even need a structure for them!
> > 
> > Don't try to be fancy, it's not needed, it's not like you are running
> > out of ioctl space...
> 
> Thanks a lot for the comments and suggestions.
> 
> That's true, it's a "normal" driver, maybe I overly considered the
> extensibility of it. OK, Let me rework this patch to remove argsz from
> these two ioctls.
> 
> What about the existing ioctls for this driver, they have argsz too.
> shall I prepare another patch to remove them as well?

I am hoping you actually have users for those ioctls in userspace today?
If not, and no one is using them, then yes, please fix those too.

thanks,

greg k-h
Wu, Hao July 4, 2019, 11:29 a.m. UTC | #8
On Thu, Jul 04, 2019 at 01:04:49PM +0200, Greg KH wrote:
> On Thu, Jul 04, 2019 at 04:58:55PM +0800, Wu Hao wrote:
> > > > Hope things could be more clear now. :)
> > > 
> > > That's nice for the vfio stuff, but you are just a "normal" driver here.
> > > You want an ioctl that just does one thing, no arguments, no flags, no
> > > anything.  No need for a size argument then at all.  These ioctls don't
> > > even need a structure for them!
> > > 
> > > Don't try to be fancy, it's not needed, it's not like you are running
> > > out of ioctl space...
> > 
> > Thanks a lot for the comments and suggestions.
> > 
> > That's true, it's a "normal" driver, maybe I overly considered the
> > extensibility of it. OK, Let me rework this patch to remove argsz from
> > these two ioctls.
> > 
> > What about the existing ioctls for this driver, they have argsz too.
> > shall I prepare another patch to remove them as well?
> 
> I am hoping you actually have users for those ioctls in userspace today?
> If not, and no one is using them, then yes, please fix those too.

Yes, we have a few users, not many, e.g. https://github.com/OPAE/opae-sdk

I believe we may have more users as we are submitting code to make this
driver more usable.

Let me think about this, if we want to do this clean up, we have to 
increase the API version to tell everybody, things are changed. If
finally we decide to do this clean up, that will be a new patch after
this patchset.

Many Thanks for your patient guide and suggestions. :)

Hao

> 
> thanks,
> 
> greg k-h

Patch
diff mbox series

diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
index 076d74f6416d..8b2a33760483 100644
--- a/drivers/fpga/dfl-fme-main.c
+++ b/drivers/fpga/dfl-fme-main.c
@@ -16,6 +16,7 @@ 
 
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/uaccess.h>
 #include <linux/fpga-dfl.h>
 
 #include "dfl.h"
@@ -105,9 +106,62 @@  static void fme_hdr_uinit(struct platform_device *pdev,
 	sysfs_remove_files(&pdev->dev.kobj, fme_hdr_attrs);
 }
 
+static long fme_hdr_ioctl_release_port(struct dfl_feature_platform_data *pdata,
+				       void __user *arg)
+{
+	struct dfl_fpga_cdev *cdev = pdata->dfl_cdev;
+	struct dfl_fpga_fme_port_release release;
+	unsigned long minsz;
+
+	minsz = offsetofend(struct dfl_fpga_fme_port_release, port_id);
+
+	if (copy_from_user(&release, arg, minsz))
+		return -EFAULT;
+
+	if (release.argsz < minsz || release.flags)
+		return -EINVAL;
+
+	return dfl_fpga_cdev_config_port(cdev, release.port_id, true);
+}
+
+static long fme_hdr_ioctl_assign_port(struct dfl_feature_platform_data *pdata,
+				      void __user *arg)
+{
+	struct dfl_fpga_cdev *cdev = pdata->dfl_cdev;
+	struct dfl_fpga_fme_port_assign assign;
+	unsigned long minsz;
+
+	minsz = offsetofend(struct dfl_fpga_fme_port_assign, port_id);
+
+	if (copy_from_user(&assign, arg, minsz))
+		return -EFAULT;
+
+	if (assign.argsz < minsz || assign.flags)
+		return -EINVAL;
+
+	return dfl_fpga_cdev_config_port(cdev, assign.port_id, false);
+}
+
+static long fme_hdr_ioctl(struct platform_device *pdev,
+			  struct dfl_feature *feature,
+			  unsigned int cmd, unsigned long arg)
+{
+	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
+
+	switch (cmd) {
+	case DFL_FPGA_FME_PORT_RELEASE:
+		return fme_hdr_ioctl_release_port(pdata, (void __user *)arg);
+	case DFL_FPGA_FME_PORT_ASSIGN:
+		return fme_hdr_ioctl_assign_port(pdata, (void __user *)arg);
+	}
+
+	return -ENODEV;
+}
+
 static const struct dfl_feature_ops fme_hdr_ops = {
 	.init = fme_hdr_init,
 	.uinit = fme_hdr_uinit,
+	.ioctl = fme_hdr_ioctl,
 };
 
 static struct dfl_feature_driver fme_feature_drvs[] = {
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 4b66aaa32b5a..308c80868af4 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -231,16 +231,20 @@  EXPORT_SYMBOL_GPL(dfl_fpga_port_ops_del);
  */
 int dfl_fpga_check_port_id(struct platform_device *pdev, void *pport_id)
 {
-	struct dfl_fpga_port_ops *port_ops = dfl_fpga_port_ops_get(pdev);
-	int port_id;
+	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct dfl_fpga_port_ops *port_ops;
+
+	if (pdata->id != FEATURE_DEV_ID_UNUSED)
+		return pdata->id == *(int *)pport_id;
 
+	port_ops = dfl_fpga_port_ops_get(pdev);
 	if (!port_ops || !port_ops->get_id)
 		return 0;
 
-	port_id = port_ops->get_id(pdev);
+	pdata->id = port_ops->get_id(pdev);
 	dfl_fpga_port_ops_put(port_ops);
 
-	return port_id == *(int *)pport_id;
+	return pdata->id == *(int *)pport_id;
 }
 EXPORT_SYMBOL_GPL(dfl_fpga_check_port_id);
 
@@ -474,6 +478,7 @@  static int build_info_commit_dev(struct build_feature_devs_info *binfo)
 	pdata->dev = fdev;
 	pdata->num = binfo->feature_num;
 	pdata->dfl_cdev = binfo->cdev;
+	pdata->id = FEATURE_DEV_ID_UNUSED;
 	mutex_init(&pdata->lock);
 	lockdep_set_class_and_name(&pdata->lock, &dfl_pdata_keys[type],
 				   dfl_pdata_key_strings[type]);
@@ -973,25 +978,27 @@  void dfl_fpga_feature_devs_remove(struct dfl_fpga_cdev *cdev)
 {
 	struct dfl_feature_platform_data *pdata, *ptmp;
 
-	remove_feature_devs(cdev);
-
 	mutex_lock(&cdev->lock);
-	if (cdev->fme_dev) {
-		/* the fme should be unregistered. */
-		WARN_ON(device_is_registered(cdev->fme_dev));
+	if (cdev->fme_dev)
 		put_device(cdev->fme_dev);
-	}
 
 	list_for_each_entry_safe(pdata, ptmp, &cdev->port_dev_list, node) {
 		struct platform_device *port_dev = pdata->dev;
 
-		/* the port should be unregistered. */
-		WARN_ON(device_is_registered(&port_dev->dev));
+		/* remove released ports */
+		if (!device_is_registered(&port_dev->dev)) {
+			dfl_id_free(feature_dev_id_type(port_dev),
+				    port_dev->id);
+			platform_device_put(port_dev);
+		}
+
 		list_del(&pdata->node);
 		put_device(&port_dev->dev);
 	}
 	mutex_unlock(&cdev->lock);
 
+	remove_feature_devs(cdev);
+
 	fpga_region_unregister(cdev->region);
 	devm_kfree(cdev->parent, cdev);
 }
@@ -1029,6 +1036,82 @@  __dfl_fpga_cdev_find_port(struct dfl_fpga_cdev *cdev, void *data,
 }
 EXPORT_SYMBOL_GPL(__dfl_fpga_cdev_find_port);
 
+static int attach_port_dev(struct dfl_fpga_cdev *cdev, u32 port_id)
+{
+	struct platform_device *port_pdev;
+	int ret = -ENODEV;
+
+	mutex_lock(&cdev->lock);
+	port_pdev = __dfl_fpga_cdev_find_port(cdev, &port_id,
+					      dfl_fpga_check_port_id);
+	if (!port_pdev)
+		goto unlock_exit;
+
+	if (device_is_registered(&port_pdev->dev)) {
+		ret = -EBUSY;
+		goto put_dev_exit;
+	}
+
+	ret = platform_device_add(port_pdev);
+	if (ret)
+		goto put_dev_exit;
+
+	dfl_feature_dev_use_end(dev_get_platdata(&port_pdev->dev));
+	cdev->released_port_num--;
+put_dev_exit:
+	put_device(&port_pdev->dev);
+unlock_exit:
+	mutex_unlock(&cdev->lock);
+	return ret;
+}
+
+static int detach_port_dev(struct dfl_fpga_cdev *cdev, u32 port_id)
+{
+	struct platform_device *port_pdev;
+	int ret = -ENODEV;
+
+	mutex_lock(&cdev->lock);
+	port_pdev = __dfl_fpga_cdev_find_port(cdev, &port_id,
+					      dfl_fpga_check_port_id);
+	if (!port_pdev)
+		goto unlock_exit;
+
+	if (!device_is_registered(&port_pdev->dev)) {
+		ret = -EBUSY;
+		goto put_dev_exit;
+	}
+
+	ret = dfl_feature_dev_use_begin(dev_get_platdata(&port_pdev->dev));
+	if (ret)
+		goto put_dev_exit;
+
+	platform_device_del(port_pdev);
+	cdev->released_port_num++;
+put_dev_exit:
+	put_device(&port_pdev->dev);
+unlock_exit:
+	mutex_unlock(&cdev->lock);
+	return ret;
+}
+
+/**
+ * dfl_fpga_cdev_config_port - configure a port feature dev
+ * @cdev: parent container device.
+ * @port_id: id of the port feature device.
+ * @release: release port or assign port back.
+ *
+ * This function allows user to release port platform device or assign it back.
+ * e.g. to safely turn one port from PF into VF for PCI device SRIOV support,
+ * release port platform device is one necessary step.
+ */
+int dfl_fpga_cdev_config_port(struct dfl_fpga_cdev *cdev,
+			      u32 port_id, bool release)
+{
+	return release ? detach_port_dev(cdev, port_id) :
+			 attach_port_dev(cdev, port_id);
+}
+EXPORT_SYMBOL_GPL(dfl_fpga_cdev_config_port);
+
 static int __init dfl_fpga_init(void)
 {
 	int ret;
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 8851c6c893fc..63f39ab08905 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -183,6 +183,8 @@  struct dfl_feature {
 
 #define DEV_STATUS_IN_USE	0
 
+#define FEATURE_DEV_ID_UNUSED	(-1)
+
 /**
  * struct dfl_feature_platform_data - platform data for feature devices
  *
@@ -191,6 +193,7 @@  struct dfl_feature {
  * @cdev: cdev of feature dev.
  * @dev: ptr to platform device linked with this platform data.
  * @dfl_cdev: ptr to container device.
+ * @id: id used for this feature device.
  * @disable_count: count for port disable.
  * @num: number for sub features.
  * @dev_status: dev status (e.g. DEV_STATUS_IN_USE).
@@ -203,6 +206,7 @@  struct dfl_feature_platform_data {
 	struct cdev cdev;
 	struct platform_device *dev;
 	struct dfl_fpga_cdev *dfl_cdev;
+	int id;
 	unsigned int disable_count;
 	unsigned long dev_status;
 	void *private;
@@ -378,6 +382,7 @@  void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info);
  * @fme_dev: FME feature device under this container device.
  * @lock: mutex lock to protect the port device list.
  * @port_dev_list: list of all port feature devices under this container device.
+ * @released_port_num: released port number under this container device.
  */
 struct dfl_fpga_cdev {
 	struct device *parent;
@@ -385,6 +390,7 @@  struct dfl_fpga_cdev {
 	struct device *fme_dev;
 	struct mutex lock;
 	struct list_head port_dev_list;
+	int released_port_num;
 };
 
 struct dfl_fpga_cdev *
@@ -412,4 +418,8 @@  dfl_fpga_cdev_find_port(struct dfl_fpga_cdev *cdev, void *data,
 
 	return pdev;
 }
+
+int dfl_fpga_cdev_config_port(struct dfl_fpga_cdev *cdev,
+			      u32 port_id, bool release);
+
 #endif /* __FPGA_DFL_H */
diff --git a/include/uapi/linux/fpga-dfl.h b/include/uapi/linux/fpga-dfl.h
index 2e324e515c41..e9a00e014114 100644
--- a/include/uapi/linux/fpga-dfl.h
+++ b/include/uapi/linux/fpga-dfl.h
@@ -176,4 +176,36 @@  struct dfl_fpga_fme_port_pr {
 
 #define DFL_FPGA_FME_PORT_PR	_IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 0)
 
+/**
+ * DFL_FPGA_FME_PORT_RELEASE - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 1,
+ *					struct dfl_fpga_fme_port_release)
+ *
+ * Driver releases the port per Port ID provided by caller.
+ * Return: 0 on success, -errno on failure.
+ */
+struct dfl_fpga_fme_port_release {
+	/* Input */
+	__u32 argsz;		/* Structure length */
+	__u32 flags;		/* Zero for now */
+	__u32 port_id;
+};
+
+#define DFL_FPGA_FME_PORT_RELEASE	_IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 1)
+
+/**
+ * DFL_FPGA_FME_PORT_ASSIGN - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 2,
+ *					struct dfl_fpga_fme_port_assign)
+ *
+ * Driver assigns the port back per Port ID provided by caller.
+ * Return: 0 on success, -errno on failure.
+ */
+struct dfl_fpga_fme_port_assign {
+	/* Input */
+	__u32 argsz;		/* Structure length */
+	__u32 flags;		/* Zero for now */
+	__u32 port_id;
+};
+
+#define DFL_FPGA_FME_PORT_ASSIGN	_IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 2)
+
 #endif /* _UAPI_LINUX_FPGA_DFL_H */