Re: [Qemu-devel] [PATCH v6 06/13] s390x/css: device support for s390-ccw passthrough

[Auger Eric]

Hi Dong,

On 28/04/2017 15:09, Dong Jia Shi wrote:
> In order to support subchannels pass-through, we introduce a s390
> subchannel device called "s390-ccw" to hold the real subchannel info.
> The s390-ccw devices inherit from the abstract CcwDevice which connect
> to the existing virtual-css-bus.
> 
> Signed-off-by: Dong Jia Shi <address@hidden>
> ---
>  hw/s390x/Makefile.objs |   1 +
>  hw/s390x/s390-ccw.c    | 135 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/s390x/s390-ccw.h    |  38 ++++++++++++++
>  3 files changed, 174 insertions(+)
>  create mode 100644 hw/s390x/s390-ccw.c
>  create mode 100644 hw/s390x/s390-ccw.h
> 
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index 41ac4ec..72a3d37 100644
> --- a/hw/s390x/Makefile.objs
> +++ b/hw/s390x/Makefile.objs
> @@ -13,3 +13,4 @@ obj-y += ccw-device.o
>  obj-y += s390-pci-bus.o s390-pci-inst.o
>  obj-y += s390-skeys.o
>  obj-$(CONFIG_KVM) += s390-skeys-kvm.o
> +obj-y += s390-ccw.o
> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
> new file mode 100644
> index 0000000..8b4b9cf
> --- /dev/null
> +++ b/hw/s390x/s390-ccw.c
> @@ -0,0 +1,135 @@
> +/*
> + * s390 CCW Assignment Support
> + *
> + * Copyright 2017 IBM Corp
> + * Author(s): Dong Jia Shi <address@hidden>
> + *            Xiao Feng Ren <address@hidden>
> + *            Pierre Morel <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2
> + * or (at your option) any later version. See the COPYING file in the
> + * top-level directory.
> + */
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "hw/sysbus.h"
> +#include "libgen.h"
> +#include "hw/s390x/css.h"
> +#include "hw/s390x/css-bridge.h"
> +#include "s390-ccw.h"
> +
> +static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
> +                                  char *sysfsdev,
> +                                  Error **errp)
> +{
> +    unsigned int cssid, ssid, devid;
> +    char dev_path[PATH_MAX] = {0}, *tmp;
> +
> +    if (!sysfsdev) {
> +        error_setg(errp, "No host device provided");
> +        error_append_hint(errp,
> +                          "Use -device vfio-ccw,sysfsdev=PATH_TO_DEVICE\n");
> +        return;
> +    }
> +
> +    if (!realpath(sysfsdev, dev_path)) {
> +        error_setg(errp, "Host device '%s' not found", sysfsdev);
nit: error_setg_errno may be used instead.
> +        return;
> +    }
> +
> +    cdev->mdevid = g_strdup(basename(dev_path));
> +
> +    tmp = basename(dirname(dev_path));
> +    sscanf(tmp, "%2x.%1x.%4x", &cssid, &ssid, &devid);
I guess this is safe and you don't need to test the operation succeeded?
> +
> +    cdev->hostid.cssid = cssid;
> +    cdev->hostid.ssid = ssid;
> +    cdev->hostid.devid = devid;
> +    cdev->hostid.valid = true;
> +}
> +
> +static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error 
> **errp)
> +{
> +    CcwDevice *ccw_dev = CCW_DEVICE(cdev);
> +    CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
> +    DeviceState *parent = DEVICE(ccw_dev);
> +    BusState *qbus = qdev_get_parent_bus(parent);
> +    VirtualCssBus *cbus = VIRTUAL_CSS_BUS(qbus);
> +    SubchDev *sch;
> +    int ret;
> +    Error *err = NULL;
> +
> +    s390_ccw_get_dev_info(cdev, sysfsdev, errp);
> +    if (*errp) {
I was told in the past dereferencing errp is unclean in general because
callers may pass null to ignore errors, although the qdev core doesn't.
you may use local err in that case.
> +        return;
> +    }
> +
> +    sch = css_create_sch(ccw_dev->devno, false, cbus->squash_mcss, errp);
> +    if (!sch) {
> +        return;
> +    }
> +    sch->driver_data = cdev;
> +
> +    ccw_dev->sch = sch;
> +    ret = css_sch_build_schib(sch, &cdev->hostid);
> +    if (ret) {
> +        error_setg(&err, "%s: Failed to build initial schib: %d",
> +                   __func__, ret);
> +        goto out_err;
> +    }
> +
> +    ck->realize(ccw_dev, &err);
> +    if (err) {
> +        goto out_err;
> +    }
> +
> +    css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
> +                          parent->hotplugged, 1);
> +    return;
> +
> +out_err:
> +    error_propagate(errp, err);
> +    css_subch_assign(sch->cssid, sch->ssid, sch->schid, sch->devno, NULL);
> +    ccw_dev->sch = NULL;
> +    g_free(sch);
don't you need to free cdev->mdevid too?

Thanks

Eric
> +}
> +
> +static void s390_ccw_unrealize(S390CCWDevice *cdev, Error **errp)
> +{
> +    CcwDevice *ccw_dev = CCW_DEVICE(cdev);
> +    SubchDev *sch = ccw_dev->sch;
> +
> +    if (sch) {
> +        css_subch_assign(sch->cssid, sch->ssid, sch->schid, sch->devno, 
> NULL);
> +        g_free(sch);
> +        ccw_dev->sch = NULL;
> +    }
> +
> +    g_free(cdev->mdevid);
> +}
> +
> +static void s390_ccw_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    S390CCWDeviceClass *cdc = S390_CCW_DEVICE_CLASS(klass);
> +
> +    dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
> +    cdc->realize = s390_ccw_realize;
> +    cdc->unrealize = s390_ccw_unrealize;
> +}
> +
> +static const TypeInfo s390_ccw_info = {
> +    .name          = TYPE_S390_CCW,
> +    .parent        = TYPE_CCW_DEVICE,
> +    .instance_size = sizeof(S390CCWDevice),
> +    .class_size    = sizeof(S390CCWDeviceClass),
> +    .class_init    = s390_ccw_class_init,
> +    .abstract      = true,
> +};
> +
> +static void register_s390_ccw_type(void)
> +{
> +    type_register_static(&s390_ccw_info);
> +}
> +
> +type_init(register_s390_ccw_type)
> diff --git a/hw/s390x/s390-ccw.h b/hw/s390x/s390-ccw.h
> new file mode 100644
> index 0000000..b58d8e9
> --- /dev/null
> +++ b/hw/s390x/s390-ccw.h
> @@ -0,0 +1,38 @@
> +/*
> + * s390 CCW Assignment Support
> + *
> + * Copyright 2017 IBM Corp.
> + * Author(s): Dong Jia Shi <address@hidden>
> + *            Xiao Feng Ren <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#ifndef HW_S390_CCW_H
> +#define HW_S390_CCW_H
> +
> +#include "hw/s390x/ccw-device.h"
> +
> +#define TYPE_S390_CCW "s390-ccw"
> +#define S390_CCW_DEVICE(obj) \
> +    OBJECT_CHECK(S390CCWDevice, (obj), TYPE_S390_CCW)
> +#define S390_CCW_DEVICE_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(S390CCWDeviceClass, (klass), TYPE_S390_CCW)
> +#define S390_CCW_DEVICE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(S390CCWDeviceClass, (obj), TYPE_S390_CCW)
> +
> +typedef struct S390CCWDevice {
> +    CcwDevice parent_obj;
> +    CssDevId hostid;
> +    char *mdevid;
> +} S390CCWDevice;
> +
> +typedef struct S390CCWDeviceClass {
> +    CCWDeviceClass parent_class;
> +    void (*realize)(S390CCWDevice *dev, char *sysfsdev, Error **errp);
> +    void (*unrealize)(S390CCWDevice *dev, Error **errp);
> +} S390CCWDeviceClass;
> +
> +#endif
>