Re: [Qemu-devel] [RFC PATCH v4 1/3] Mediated device Core driver

[Dong Jia]

On Sun, 5 Jun 2016 23:27:42 -0700
Neo Jia <address@hidden> wrote:

> On Mon, Jun 06, 2016 at 02:01:48PM +0800, Dong Jia wrote:
> > On Mon, 6 Jun 2016 10:57:49 +0530
> > Kirti Wankhede <address@hidden> wrote:
> > 
> > > 
> > > 
> > > On 6/3/2016 2:27 PM, Dong Jia wrote:
> > > > On Wed, 25 May 2016 01:28:15 +0530
> > > > Kirti Wankhede <address@hidden> wrote:
> > > > 
> > > > 
> > > > ...snip...
> > > > 
> > > >> +struct phy_device_ops {
> > > >> +      struct module   *owner;
> > > >> +      const struct attribute_group **dev_attr_groups;
> > > >> +      const struct attribute_group **mdev_attr_groups;
> > > >> +
> > > >> +      int     (*supported_config)(struct device *dev, char *config);
> > > >> +      int     (*create)(struct device *dev, uuid_le uuid,
> > > >> +                        uint32_t instance, char *mdev_params);
> > > >> +      int     (*destroy)(struct device *dev, uuid_le uuid,
> > > >> +                         uint32_t instance);
> > > >> +      int     (*start)(uuid_le uuid);
> > > >> +      int     (*shutdown)(uuid_le uuid);
> > > >> +      ssize_t (*read)(struct mdev_device *vdev, char *buf, size_t 
> > > >> count,
> > > >> +                      enum mdev_emul_space address_space, loff_t pos);
> > > >> +      ssize_t (*write)(struct mdev_device *vdev, char *buf, size_t 
> > > >> count,
> > > >> +                       enum mdev_emul_space address_space, loff_t 
> > > >> pos);
> > > >> +      int     (*set_irqs)(struct mdev_device *vdev, uint32_t flags,
> > > >> +                          unsigned int index, unsigned int start,
> > > >> +                          unsigned int count, void *data);
> > > >> +      int     (*get_region_info)(struct mdev_device *vdev, int 
> > > >> region_index,
> > > >> +                               struct pci_region_info *region_info);
> > > >> +      int     (*validate_map_request)(struct mdev_device *vdev,
> > > >> +                                      unsigned long virtaddr,
> > > >> +                                      unsigned long *pfn, unsigned 
> > > >> long *size,
> > > >> +                                      pgprot_t *prot);
> > > >> +};
> > > > 
> > > > Dear Kirti:
> > > > 
> > > > When I rebased my vfio-ccw patches on this series, I found I need an
> > > > extra 'ioctl' callback in phy_device_ops.
> > > > 
> > > 
> > > Thanks for taking closer look. As per my knowledge ccw is not PCI
> > > device, right? Correct me if I'm wrong.
> > Dear Kirti:
> > 
> > You are right. CCW is different to PCI. The official term is 'Channel
> > I/O device'. They use 'Channels' (co-processors) and CCWs (channel
> > command words) to handle I/O operations.
> > 
> > > I'm curious to know. Are you planning to write a driver (vfio-mccw) for
> > > mediated ccw device?
> > I wrote two drivers:
> > 1. A vfio-pccw driver for the physical ccw device, which will reigister
> > the device and callbacks to mdev framework. With this, I could create
> > a mediated ccw device for the physical one then.
> > 2. A vfio-mccw driver for the mediated ccw device, which will add
> > itself to a vfio_group, mimiced what vfio-mpci did.
> > 
> > The problem is, vfio-mccw need to implement new ioctls besides the
> > existing ones (VFIO_DEVICE_GET_INFO, etc). And these ioctls really need
> > the physical device help to handle.
> 
> Hi Dong,
> 
> Could you please help us understand a bit more about the new VFIO ioctl?
Dear Neo,

Sure, with pleasure.
Since I tried not to bring too much ccw specific technical details
here, I wrote quite briefly. Please feel free to ask me for
supplements. :>

> Since it is a new ioctl it is send down by QEMU in this case right?
Right.

> More details?
As mentioned in the former emails, I currently added two new ioctl
commands.

1. VFIO_DEVICE_CCW_HOT_RESET
Both the name and the purpose of this command looks pretty the same
with VFIO_DEVICE_PCI_HOT_RESET.
Since Kevin proposed an individual callback for hot-reset handling, I
believe this will not be a problem (only if you accept the proposal).

2. VFIO_DEVICE_CCW_CMD_REQUEST
This intends to handle an intercepted channel I/O instruction. It
basically need to do the following thing:
  a. Copy the raw data of the CCW program (a group of chained CCWs) from
     user into kernel space buffers.
  b. Do CCW program translation based on the raw data to get a
     real-device runnable CCW program. We'd pin pages for those CCWs
     which have memory space pointers for their offload, and update the
     CCW program with the pinned results (phys).
  c. Issue the translated CCW program to a real-device to perform the
     I/O operation, and wait for the I/O result interrupt.
  d. Once we got the I/O result, copy the result back to user, and
     unpin the pages.

Step c could only be done by the physical device driver, since it's it
that the int_handler belongs to.
Step b and d should be done by the physical device driver. Or we'd
pin/unpin pages in the mediated device driver?

That's why I asked for the new callback.

> 
> Thanks,
> Neo
> 
> > 
> > > 
> > > Thanks,
> > > Kirti
> > > 
> > > > The ccw physical device only supports one ccw mediated device. And I
> > > > have two new ioctl commands for the ccw mediated device. One is 
> > > > to hot-reset the resource in the physical device that allocated for
> > > > the mediated device, the other is to do an I/O instruction translation
> > > > and perform an I/O operation on the physical device. I found the
> > > > existing callbacks could not meet my requirements.
> > > > 
> > > > Something like the following would be fine for my case:
> > > >         int (*ioctl)(struct mdev_device *vdev,
> > > >                      unsigned int cmd,
> > > >                      unsigned long arg);
> > > > 
> > > > What do you think about this?
> > > > 
> > > > --------
> > > > Dong Jia
> > > > 
> > > 
> > 
> > --------
> > Dong Jia
> > 
> 



--------
Dong Jia