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

[Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>]

* Auger Eric <eric.auger@redhat.com> [2017-05-01 13:08:01 +0200]:

Hi Eric,

[...]
> > 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 <bjsdjshi@linux.vnet.ibm.com>
> > + *            Xiao Feng Ren <renxiaof@linux.vnet.ibm.com>
> > + *            Pierre Morel <pmorel@linux.vnet.ibm.com>
> > + *
> > + * 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.
Ok.

> > +        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?
Yes, it's safe, since the format of the input is an interface that the
kernel provides for the userland program.

> > +
> > +    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.
Thanks for the reminding. I'm aware of that. ;>

To make the usage of the Error here more consistent, I will use the
local @err, and propagate to @errp once error happens.

> > +        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?
Yes, I should do that. Will fix!

> 
> Thanks
> 
> Eric
> > +}
> > +
[...]

-- 
Dong Jia Shi