Re: [Qemu-devel] [PATCH RFC v3 04/15] vfio: ccw: basic implementation for vfio_ccw driver

[Cornelia Huck]

On Fri, 17 Feb 2017 09:29:28 +0100
Dong Jia Shi <address@hidden> wrote:

> To make vfio support subchannel devices, we need a css driver for
> the vfio subchannels. This patch adds a basic vfio-ccw subchannel
> driver for this purpose.
> 
> To enable VFIO for vfio-ccw, enable S390_CCW_IOMMU config option
> and configure VFIO as required.
> 
> Signed-off-by: Dong Jia Shi <address@hidden>
> Acked-by: Pierre Morel <address@hidden>
> ---
>  arch/s390/Kconfig                   |  10 ++
>  arch/s390/include/asm/isc.h         |   1 +
>  drivers/iommu/Kconfig               |   8 ++
>  drivers/s390/cio/Makefile           |   3 +
>  drivers/s390/cio/vfio_ccw_drv.c     | 262 
> ++++++++++++++++++++++++++++++++++++
>  drivers/s390/cio/vfio_ccw_private.h |  25 ++++
>  6 files changed, 309 insertions(+)
>  create mode 100644 drivers/s390/cio/vfio_ccw_drv.c
>  create mode 100644 drivers/s390/cio/vfio_ccw_private.h
> 
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index c6722112..b920df8 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -670,6 +670,16 @@ config EADM_SCH
>         To compile this driver as a module, choose M here: the
>         module will be called eadm_sch.
> 
> +config VFIO_CCW
> +     def_tristate n
> +     prompt "Support for VFIO-CCW subchannels"
> +     depends on S390_CCW_IOMMU && VFIO
> +     help
> +       This driver allows usage of VFIO-CCW subchannels.

Hm...

"This driver allows usage of I/O subchannels via VFIO-CCW."

?

> +
> +       To compile this driver as a module, choose M here: the
> +       module will be called vfio_ccw.
> +
>  endmenu
> 
>  menu "Dump support"
> diff --git a/arch/s390/include/asm/isc.h b/arch/s390/include/asm/isc.h
> index 68d7d68..8a0b721 100644
> --- a/arch/s390/include/asm/isc.h
> +++ b/arch/s390/include/asm/isc.h
> @@ -16,6 +16,7 @@
>  #define CONSOLE_ISC 1                        /* console I/O subchannel */
>  #define EADM_SCH_ISC 4                       /* EADM subchannels */
>  #define CHSC_SCH_ISC 7                       /* CHSC subchannels */
> +#define VFIO_CCW_ISC IO_SCH_ISC              /* VFIO-CCW I/O subchannels */

This is OK for now, I guess; but do we want to have the isc
configurable in the long run? I.e., if a host wants to run its own I/O
devices at a different priority than the devices it passes to a guest?

>  /* Adapter interrupts. */
>  #define QDIO_AIRQ_ISC IO_SCH_ISC     /* I/O subchannel in qdio mode */
>  #define PCI_ISC 2                    /* PCI I/O subchannels */

(...)

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> new file mode 100644
> index 0000000..b068207
> --- /dev/null
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -0,0 +1,262 @@
> +/*
> + * VFIO based Physical Subchannel device driver
> + *
> + * Copyright IBM Corp. 2017
> + *
> + * Author(s): Dong Jia Shi <address@hidden>
> + *            Xiao Feng Ren <address@hidden>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +
> +#include <asm/isc.h>
> +
> +#include "vfio_ccw_private.h"
> +
> +/*
> + * Helpers
> + */
> +static int vfio_ccw_sch_quiesce(struct subchannel *sch)
> +{
> +     struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
> +     DECLARE_COMPLETION_ONSTACK(completion);
> +     int iretry, ret = 0;
> +
> +     spin_lock_irq(sch->lock);
> +     if (!sch->schib.pmcw.ena)
> +             goto out_unlock;
> +     ret = cio_disable_subchannel(sch);
> +     if (ret != -EBUSY)
> +             goto out_unlock;
> +
> +     do {
> +             iretry = 255;
> +
> +             ret = cio_cancel_halt_clear(sch, &iretry);
> +             while (ret == -EBUSY) {
> +                     /*
> +                      * Flushing all I/O and wait the

"Flush all I/O and wait for..."

> +                      * cancel/halt/clear completion.
> +                      */
> +                     private->completion = &completion;
> +                     spin_unlock_irq(sch->lock);
> +
> +                     wait_for_completion(&completion);

What happens for cancel? It won't generate an interrupt.

> +
> +                     spin_lock_irq(sch->lock);
> +                     private->completion = NULL;
> +                     ret = cio_cancel_halt_clear(sch, &iretry);
> +             };
> +
> +             ret = cio_disable_subchannel(sch);
> +     } while (ret == -EBUSY);
> +
> +out_unlock:
> +     spin_unlock_irq(sch->lock);
> +     return ret;
> +}
> +
> +/*
> + * Sysfs interfaces
> + */
> +static ssize_t chpids_show(struct device *dev,
> +                        struct device_attribute *attr,
> +                        char *buf)
> +{
> +     struct subchannel *sch = to_subchannel(dev);
> +     struct chsc_ssd_info *ssd = &sch->ssd_info;
> +     ssize_t ret = 0;
> +     int chp;
> +     int mask;
> +
> +     for (chp = 0; chp < 8; chp++) {
> +             mask = 0x80 >> chp;
> +             if (ssd->path_mask & mask)
> +                     ret += sprintf(buf + ret, "%02x ", ssd->chpid[chp].id);
> +             else
> +                     ret += sprintf(buf + ret, "00 ");
> +     }
> +     ret += sprintf(buf+ret, "\n");
> +     return ret;
> +}
> +
> +static ssize_t pimpampom_show(struct device *dev,
> +                           struct device_attribute *attr,
> +                           char *buf)
> +{
> +     struct subchannel *sch = to_subchannel(dev);
> +     struct pmcw *pmcw = &sch->schib.pmcw;
> +
> +     return sprintf(buf, "%02x %02x %02x\n",
> +                    pmcw->pim, pmcw->pam, pmcw->pom);
> +}
> +
> +static DEVICE_ATTR(chpids, 0444, chpids_show, NULL);
> +static DEVICE_ATTR(pimpampom, 0444, pimpampom_show, NULL);

Quick question: You need to duplicate these so that lscss shows a sane
output for vfio-ccw subchannels?

> +
> +static struct attribute *vfio_subchannel_attrs[] = {
> +     &dev_attr_chpids.attr,
> +     &dev_attr_pimpampom.attr,
> +     NULL,
> +};
> +
> +static struct attribute_group vfio_subchannel_attr_group = {
> +     .attrs = vfio_subchannel_attrs,
> +};
> +
> +/*
> + * Css driver callbacks
> + */
> +static void vfio_ccw_sch_irq(struct subchannel *sch)
> +{
> +     struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
> +
> +     inc_irq_stat(IRQIO_CIO);
> +
> +     if (!private)
> +             return;
> +
> +     if (private->completion)
> +             complete(private->completion);
> +}
> +
> +static int vfio_ccw_sch_probe(struct subchannel *sch)
> +{
> +     struct pmcw *pmcw = &sch->schib.pmcw;
> +     struct vfio_ccw_private *private;
> +     int ret;
> +
> +     if (pmcw->qf) {
> +             dev_warn(&sch->dev, "vfio: ccw: do not support QDIO: %s\n",

s/do/does/

> +                      dev_name(&sch->dev));
> +             return -ENOTTY;

Is -ENOTTY the right return code here? -EINVAL?

> +     }
> +
> +     private = kzalloc(sizeof(*private), GFP_KERNEL | GFP_DMA);
> +     if (!private)
> +             return -ENOMEM;
> +     private->sch = sch;
> +     dev_set_drvdata(&sch->dev, private);
> +
> +     spin_lock_irq(sch->lock);
> +     sch->isc = VFIO_CCW_ISC;
> +     ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
> +     spin_unlock_irq(sch->lock);
> +     if (ret)
> +             goto out_free;
> +
> +     ret = sysfs_create_group(&sch->dev.kobj, &vfio_subchannel_attr_group);
> +     if (ret)
> +             goto out_disable;
> +
> +     return 0;
> +
> +out_disable:
> +     cio_disable_subchannel(sch);
> +out_free:
> +     dev_set_drvdata(&sch->dev, NULL);
> +     kfree(private);
> +     return ret;
> +}
> +

(...)

> +/**
> + * vfio_ccw_sch_event - process subchannel event
> + * @sch: subchannel
> + * @process: non-zero if function is called in process context
> + *
> + * An unspecified event occurred for this subchannel. Adjust data according
> + * to the current operational state of the subchannel. Return zero when the
> + * event has been handled sufficiently or -EAGAIN when this function should
> + * be called again in process context.
> + */
> +static int vfio_ccw_sch_event(struct subchannel *sch, int process)
> +{
> +     unsigned long flags;
> +
> +     spin_lock_irqsave(sch->lock, flags);
> +     if (!device_is_registered(&sch->dev))
> +             goto out_unlock;
> +
> +     if (work_pending(&sch->todo_work))
> +             goto out_unlock;
> +
> +     if (cio_update_schib(sch)) {
> +             /* Not operational. */
> +             css_sched_sch_todo(sch, SCH_TODO_UNREG);
> +
> +             /*
> +              * TODO:
> +              * Probably we should send the machine check to the guest.

Yes, we should do that later on. Will user space notice that the device
is gone? (I think crw injection should be done by user space.)

> +              */
> +             goto out_unlock;
> +     }
> +
> +out_unlock:
> +     spin_unlock_irqrestore(sch->lock, flags);
> +
> +     return 0;
> +}

(...)

Looks sane in general from my POV.