| Message ID | 20190607145353.2052-1-cohuck@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5] vfio-ccw: support async command subregion | expand |
On 06/07/2019 10:53 AM, Cornelia Huck wrote: > A vfio-ccw device may provide an async command subregion for > issuing halt/clear subchannel requests. If it is present, use > it for sending halt/clear request to the device; if not, fall > back to emulation (as done today). > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > > v4->v5: > - It seems we need to take the indirection via the class for the > callbacks after all :( > - Dropped Eric's R-b: for that reason > > --- > hw/s390x/css.c | 27 +++++++-- > hw/s390x/s390-ccw.c | 20 +++++++ > hw/vfio/ccw.c | 112 +++++++++++++++++++++++++++++++++++- > include/hw/s390x/css.h | 3 + > include/hw/s390x/s390-ccw.h | 2 + > 5 files changed, 158 insertions(+), 6 deletions(-) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index ad310b9f94bc..b92395f165e6 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -22,6 +22,7 @@ > #include "trace.h" > #include "hw/s390x/s390_flic.h" > #include "hw/s390x/s390-virtio-ccw.h" > +#include "hw/s390x/s390-ccw.h" > > typedef struct CrwContainer { > CRW crw; > @@ -1205,6 +1206,26 @@ static void sch_handle_start_func_virtual(SubchDev *sch) > > } > > +static void sch_handle_halt_func_passthrough(SubchDev *sch) > +{ > + int ret; > + > + ret = s390_ccw_halt(sch); > + if (ret == -ENOSYS) { > + sch_handle_halt_func(sch); > + } > +} > + > +static void sch_handle_clear_func_passthrough(SubchDev *sch) > +{ > + int ret; > + > + ret = s390_ccw_clear(sch); > + if (ret == -ENOSYS) { > + sch_handle_clear_func(sch); > + } > +} > + do we need an extra s390_ccw_clear/halt functions? can't we just call cdc->clear/halt in the passthrough functions? > static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch) > { > SCHIB *schib = &sch->curr_status; > @@ -1244,11 +1265,9 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev *sch) > SCHIB *schib = &sch->curr_status; > > if (schib->scsw.ctrl & SCSW_FCTL_CLEAR_FUNC) { > - /* TODO: Clear handling */ > - sch_handle_clear_func(sch); > + sch_handle_clear_func_passthrough(sch); > } else if (schib->scsw.ctrl & SCSW_FCTL_HALT_FUNC) { > - /* TODO: Halt handling */ > - sch_handle_halt_func(sch); > + sch_handle_halt_func_passthrough(sch); > } else if (schib->scsw.ctrl & SCSW_FCTL_START_FUNC) { > return sch_handle_start_func_passthrough(sch); > } > diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c > index f5f025d1b6ca..951be5ab0245 100644 > --- a/hw/s390x/s390-ccw.c > +++ b/hw/s390x/s390-ccw.c > @@ -29,6 +29,26 @@ IOInstEnding s390_ccw_cmd_request(SubchDev *sch) > return cdc->handle_request(sch); > } > > +int s390_ccw_halt(SubchDev *sch) > +{ > + S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data); > + > + if (!cdc->handle_halt) { > + return -ENOSYS; > + } > + return cdc->handle_halt(sch); > +} > + > +int s390_ccw_clear(SubchDev *sch) > +{ > + S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data); > + > + if (!cdc->handle_clear) { > + return -ENOSYS; > + } > + return cdc->handle_clear(sch); > +} > +
On Fri, 7 Jun 2019 11:02:36 -0400 Farhan Ali <alifm@linux.ibm.com> wrote: > On 06/07/2019 10:53 AM, Cornelia Huck wrote: > > A vfio-ccw device may provide an async command subregion for > > issuing halt/clear subchannel requests. If it is present, use > > it for sending halt/clear request to the device; if not, fall > > back to emulation (as done today). > > > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > --- > > > > v4->v5: > > - It seems we need to take the indirection via the class for the > > callbacks after all :( > > - Dropped Eric's R-b: for that reason > > > > --- > > hw/s390x/css.c | 27 +++++++-- > > hw/s390x/s390-ccw.c | 20 +++++++ > > hw/vfio/ccw.c | 112 +++++++++++++++++++++++++++++++++++- > > include/hw/s390x/css.h | 3 + > > include/hw/s390x/s390-ccw.h | 2 + > > 5 files changed, 158 insertions(+), 6 deletions(-) > > > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > > index ad310b9f94bc..b92395f165e6 100644 > > --- a/hw/s390x/css.c > > +++ b/hw/s390x/css.c > > @@ -22,6 +22,7 @@ > > #include "trace.h" > > #include "hw/s390x/s390_flic.h" > > #include "hw/s390x/s390-virtio-ccw.h" > > +#include "hw/s390x/s390-ccw.h" > > > > typedef struct CrwContainer { > > CRW crw; > > @@ -1205,6 +1206,26 @@ static void sch_handle_start_func_virtual(SubchDev *sch) > > > > } > > > > +static void sch_handle_halt_func_passthrough(SubchDev *sch) > > +{ > > + int ret; > > + > > + ret = s390_ccw_halt(sch); > > + if (ret == -ENOSYS) { > > + sch_handle_halt_func(sch); > > + } > > +} > > + > > +static void sch_handle_clear_func_passthrough(SubchDev *sch) > > +{ > > + int ret; > > + > > + ret = s390_ccw_clear(sch); > > + if (ret == -ENOSYS) { > > + sch_handle_clear_func(sch); > > + } > > +} > > + > > do we need an extra s390_ccw_clear/halt functions? can't we just call > cdc->clear/halt in the passthrough functions? I mostly added them for symmetry reasons... we still need to check for presence of the callback in any case, though. (vfio is not always built, e.g. on windows or os x.) > > > static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch) > > { > > SCHIB *schib = &sch->curr_status; > > @@ -1244,11 +1265,9 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev *sch) > > SCHIB *schib = &sch->curr_status; > > > > if (schib->scsw.ctrl & SCSW_FCTL_CLEAR_FUNC) { > > - /* TODO: Clear handling */ > > - sch_handle_clear_func(sch); > > + sch_handle_clear_func_passthrough(sch); > > } else if (schib->scsw.ctrl & SCSW_FCTL_HALT_FUNC) { > > - /* TODO: Halt handling */ > > - sch_handle_halt_func(sch); > > + sch_handle_halt_func_passthrough(sch); > > } else if (schib->scsw.ctrl & SCSW_FCTL_START_FUNC) { > > return sch_handle_start_func_passthrough(sch); > > } > > diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c > > index f5f025d1b6ca..951be5ab0245 100644 > > --- a/hw/s390x/s390-ccw.c > > +++ b/hw/s390x/s390-ccw.c > > @@ -29,6 +29,26 @@ IOInstEnding s390_ccw_cmd_request(SubchDev *sch) > > return cdc->handle_request(sch); > > } > > > > +int s390_ccw_halt(SubchDev *sch) > > +{ > > + S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data); > > + > > + if (!cdc->handle_halt) { > > + return -ENOSYS; > > + } > > + return cdc->handle_halt(sch); > > +} > > + > > +int s390_ccw_clear(SubchDev *sch) > > +{ > > + S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data); > > + > > + if (!cdc->handle_clear) { > > + return -ENOSYS; > > + } > > + return cdc->handle_clear(sch); > > +} > > + >
On 06/07/2019 11:09 AM, Cornelia Huck wrote: > On Fri, 7 Jun 2019 11:02:36 -0400 > Farhan Ali <alifm@linux.ibm.com> wrote: > >> On 06/07/2019 10:53 AM, Cornelia Huck wrote: >>> A vfio-ccw device may provide an async command subregion for >>> issuing halt/clear subchannel requests. If it is present, use >>> it for sending halt/clear request to the device; if not, fall >>> back to emulation (as done today). >>> >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> >>> --- >>> >>> v4->v5: >>> - It seems we need to take the indirection via the class for the >>> callbacks after all :( >>> - Dropped Eric's R-b: for that reason >>> >>> --- >>> hw/s390x/css.c | 27 +++++++-- >>> hw/s390x/s390-ccw.c | 20 +++++++ >>> hw/vfio/ccw.c | 112 +++++++++++++++++++++++++++++++++++- >>> include/hw/s390x/css.h | 3 + >>> include/hw/s390x/s390-ccw.h | 2 + >>> 5 files changed, 158 insertions(+), 6 deletions(-) >>> >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >>> index ad310b9f94bc..b92395f165e6 100644 >>> --- a/hw/s390x/css.c >>> +++ b/hw/s390x/css.c >>> @@ -22,6 +22,7 @@ >>> #include "trace.h" >>> #include "hw/s390x/s390_flic.h" >>> #include "hw/s390x/s390-virtio-ccw.h" >>> +#include "hw/s390x/s390-ccw.h" >>> >>> typedef struct CrwContainer { >>> CRW crw; >>> @@ -1205,6 +1206,26 @@ static void sch_handle_start_func_virtual(SubchDev *sch) >>> >>> } >>> >>> +static void sch_handle_halt_func_passthrough(SubchDev *sch) >>> +{ >>> + int ret; >>> + >>> + ret = s390_ccw_halt(sch); >>> + if (ret == -ENOSYS) { >>> + sch_handle_halt_func(sch); >>> + } >>> +} >>> + >>> +static void sch_handle_clear_func_passthrough(SubchDev *sch) >>> +{ >>> + int ret; >>> + >>> + ret = s390_ccw_clear(sch); >>> + if (ret == -ENOSYS) { >>> + sch_handle_clear_func(sch); >>> + } >>> +} >>> + >> >> do we need an extra s390_ccw_clear/halt functions? can't we just call >> cdc->clear/halt in the passthrough functions? > > I mostly added them for symmetry reasons... we still need to check for > presence of the callback in any case, though. > > (vfio is not always built, e.g. on windows or os x.) right, but if we are calling do_subchannel_work_passthrough, then we know for sure we are building the S390CCWDevice which is the vfio device, no? So we could just add checks for callbacks in sch_handle_clear/halt_func_passthrough, no? I would even like to get rid of the s390_ccw_cmd_request if we can, but that is out of scope for this patch. :) > >> >>> static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch) >>> { >>> SCHIB *schib = &sch->curr_status; >>> @@ -1244,11 +1265,9 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev *sch) >>> SCHIB *schib = &sch->curr_status; >>> >>> if (schib->scsw.ctrl & SCSW_FCTL_CLEAR_FUNC) { >>> - /* TODO: Clear handling */ >>> - sch_handle_clear_func(sch); >>> + sch_handle_clear_func_passthrough(sch); >>> } else if (schib->scsw.ctrl & SCSW_FCTL_HALT_FUNC) { >>> - /* TODO: Halt handling */ >>> - sch_handle_halt_func(sch); >>> + sch_handle_halt_func_passthrough(sch); >>> } else if (schib->scsw.ctrl & SCSW_FCTL_START_FUNC) { >>> return sch_handle_start_func_passthrough(sch); >>> } >>> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c >>> index f5f025d1b6ca..951be5ab0245 100644 >>> --- a/hw/s390x/s390-ccw.c >>> +++ b/hw/s390x/s390-ccw.c >>> @@ -29,6 +29,26 @@ IOInstEnding s390_ccw_cmd_request(SubchDev *sch) >>> return cdc->handle_request(sch); >>> } >>> >>> +int s390_ccw_halt(SubchDev *sch) >>> +{ >>> + S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data); >>> + >>> + if (!cdc->handle_halt) { >>> + return -ENOSYS; >>> + } >>> + return cdc->handle_halt(sch); >>> +} >>> + >>> +int s390_ccw_clear(SubchDev *sch) >>> +{ >>> + S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data); >>> + >>> + if (!cdc->handle_clear) { >>> + return -ENOSYS; >>> + } >>> + return cdc->handle_clear(sch); >>> +} >>> + >> > >
On Fri, 7 Jun 2019 11:19:09 -0400 Farhan Ali <alifm@linux.ibm.com> wrote: > On 06/07/2019 11:09 AM, Cornelia Huck wrote: > > On Fri, 7 Jun 2019 11:02:36 -0400 > > Farhan Ali <alifm@linux.ibm.com> wrote: > > > >> On 06/07/2019 10:53 AM, Cornelia Huck wrote: > >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c > >>> index ad310b9f94bc..b92395f165e6 100644 > >>> --- a/hw/s390x/css.c > >>> +++ b/hw/s390x/css.c > >>> @@ -22,6 +22,7 @@ > >>> #include "trace.h" > >>> #include "hw/s390x/s390_flic.h" > >>> #include "hw/s390x/s390-virtio-ccw.h" > >>> +#include "hw/s390x/s390-ccw.h" > >>> > >>> typedef struct CrwContainer { > >>> CRW crw; > >>> @@ -1205,6 +1206,26 @@ static void sch_handle_start_func_virtual(SubchDev *sch) > >>> > >>> } > >>> > >>> +static void sch_handle_halt_func_passthrough(SubchDev *sch) > >>> +{ > >>> + int ret; > >>> + > >>> + ret = s390_ccw_halt(sch); > >>> + if (ret == -ENOSYS) { > >>> + sch_handle_halt_func(sch); > >>> + } > >>> +} > >>> + > >>> +static void sch_handle_clear_func_passthrough(SubchDev *sch) > >>> +{ > >>> + int ret; > >>> + > >>> + ret = s390_ccw_clear(sch); > >>> + if (ret == -ENOSYS) { > >>> + sch_handle_clear_func(sch); > >>> + } > >>> +} > >>> + > >> > >> do we need an extra s390_ccw_clear/halt functions? can't we just call > >> cdc->clear/halt in the passthrough functions? > > > > I mostly added them for symmetry reasons... we still need to check for > > presence of the callback in any case, though. > > > > (vfio is not always built, e.g. on windows or os x.) > > > right, but if we are calling do_subchannel_work_passthrough, then we > know for sure we are building the S390CCWDevice which is the vfio > device, no? > > So we could just add checks for callbacks in > sch_handle_clear/halt_func_passthrough, no? > > I would even like to get rid of the s390_ccw_cmd_request if we can, but > that is out of scope for this patch. :) Ok, I just walked through various source files (some of which are a bit confusingly named) again and now I understand again why it was done that way in the first place. - hw/s390x/s390-ccw.c provides some interfaces for pass-through ccw devices. It is built unconditionally, and its interfaces are called unconditionally from the css code. It also provides a device class where code can hook up callbacks. - hw/vfio/ccw.c (which is not built for !KVM) actually hooks up callbacks in that device class. So, s390-ccw.c (not to be confused with ccw-device.c...) provides a layer that makes it possible to call things unconditionally, regardless whether we have vfio-ccw available or not. Not that the code ends up being called without vfio-ccw support; but the class indirection enables the code to be built. There's possibly a way to make this work without the class indirection as well, but I think we'd end up doing code juggling before ending up with something that's not really nicer than the code we have now. Therefore, I'd prefer to keep the class handling and hook up the halt/clear callbacks there as well.
On 06/07/2019 10:53 AM, Cornelia Huck wrote: > A vfio-ccw device may provide an async command subregion for > issuing halt/clear subchannel requests. If it is present, use > it for sending halt/clear request to the device; if not, fall > back to emulation (as done today). > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > > v4->v5: > - It seems we need to take the indirection via the class for the > callbacks after all :( > - Dropped Eric's R-b: for that reason > > --- > hw/s390x/css.c | 27 +++++++-- > hw/s390x/s390-ccw.c | 20 +++++++ > hw/vfio/ccw.c | 112 +++++++++++++++++++++++++++++++++++- > include/hw/s390x/css.h | 3 + > include/hw/s390x/s390-ccw.h | 2 + > 5 files changed, 158 insertions(+), 6 deletions(-) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index ad310b9f94bc..b92395f165e6 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -22,6 +22,7 @@ > #include "trace.h" > #include "hw/s390x/s390_flic.h" > #include "hw/s390x/s390-virtio-ccw.h" > +#include "hw/s390x/s390-ccw.h" > > typedef struct CrwContainer { > CRW crw; > @@ -1205,6 +1206,26 @@ static void sch_handle_start_func_virtual(SubchDev *sch) > > } > > +static void sch_handle_halt_func_passthrough(SubchDev *sch) > +{ > + int ret; > + > + ret = s390_ccw_halt(sch); > + if (ret == -ENOSYS) { > + sch_handle_halt_func(sch); > + } > +} > + > +static void sch_handle_clear_func_passthrough(SubchDev *sch) > +{ > + int ret; > + > + ret = s390_ccw_clear(sch); > + if (ret == -ENOSYS) { > + sch_handle_clear_func(sch); > + } > +} > + > static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch) > { > SCHIB *schib = &sch->curr_status; > @@ -1244,11 +1265,9 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev *sch) > SCHIB *schib = &sch->curr_status; > > if (schib->scsw.ctrl & SCSW_FCTL_CLEAR_FUNC) { > - /* TODO: Clear handling */ > - sch_handle_clear_func(sch); > + sch_handle_clear_func_passthrough(sch); > } else if (schib->scsw.ctrl & SCSW_FCTL_HALT_FUNC) { > - /* TODO: Halt handling */ > - sch_handle_halt_func(sch); > + sch_handle_halt_func_passthrough(sch); > } else if (schib->scsw.ctrl & SCSW_FCTL_START_FUNC) { > return sch_handle_start_func_passthrough(sch); > } > diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c > index f5f025d1b6ca..951be5ab0245 100644 > --- a/hw/s390x/s390-ccw.c > +++ b/hw/s390x/s390-ccw.c > @@ -29,6 +29,26 @@ IOInstEnding s390_ccw_cmd_request(SubchDev *sch) > return cdc->handle_request(sch); > } > > +int s390_ccw_halt(SubchDev *sch) > +{ > + S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data); > + > + if (!cdc->handle_halt) { > + return -ENOSYS; > + } > + return cdc->handle_halt(sch); > +} > + > +int s390_ccw_clear(SubchDev *sch) > +{ > + S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data); > + > + if (!cdc->handle_clear) { > + return -ENOSYS; > + } > + return cdc->handle_clear(sch); > +} > + > static void s390_ccw_get_dev_info(S390CCWDevice *cdev, > char *sysfsdev, > Error **errp) > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index d9e39552e237..c9d1c76b4d04 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -2,9 +2,12 @@ > * vfio based subchannel assignment support > * > * Copyright 2017 IBM Corp. > + * Copyright 2019 Red Hat, Inc. > + * > * 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> > + * Cornelia Huck <cohuck@redhat.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 > @@ -32,6 +35,9 @@ struct VFIOCCWDevice { > uint64_t io_region_size; > uint64_t io_region_offset; > struct ccw_io_region *io_region; > + uint64_t async_cmd_region_size; > + uint64_t async_cmd_region_offset; > + struct ccw_cmd_region *async_cmd_region; > EventNotifier io_notifier; > bool force_orb_pfch; > bool warned_orb_pfch; > @@ -114,6 +120,87 @@ again: > } > } > > +static int vfio_ccw_handle_clear(SubchDev *sch) > +{ > + S390CCWDevice *cdev = sch->driver_data; > + VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); > + struct ccw_cmd_region *region = vcdev->async_cmd_region; > + int ret; > + > + if (!vcdev->async_cmd_region) { > + /* Async command region not available, fall back to emulation */ > + return -ENOSYS; > + } > + > + memset(region, 0, sizeof(*region)); > + region->command = VFIO_CCW_ASYNC_CMD_CSCH; > + > +again: > + ret = pwrite(vcdev->vdev.fd, region, > + vcdev->async_cmd_region_size, vcdev->async_cmd_region_offset); > + if (ret != vcdev->async_cmd_region_size) { > + if (errno == EAGAIN) { > + goto again; > + } > + error_report("vfio-ccw: write cmd region failed with errno=%d", errno); > + ret = -errno; > + } else { > + ret = region->ret_code; > + } > + switch (ret) { > + case 0: > + case -ENODEV: > + case -EACCES: > + return 0; > + case -EFAULT: > + default: > + sch_gen_unit_exception(sch); > + css_inject_io_interrupt(sch); > + return 0; > + } > +} > + > +static int vfio_ccw_handle_halt(SubchDev *sch) > +{ > + S390CCWDevice *cdev = sch->driver_data; > + VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); > + struct ccw_cmd_region *region = vcdev->async_cmd_region; > + int ret; > + > + if (!vcdev->async_cmd_region) { > + /* Async command region not available, fall back to emulation */ > + return -ENOSYS; > + } > + > + memset(region, 0, sizeof(*region)); > + region->command = VFIO_CCW_ASYNC_CMD_HSCH; > + > +again: > + ret = pwrite(vcdev->vdev.fd, region, > + vcdev->async_cmd_region_size, vcdev->async_cmd_region_offset); > + if (ret != vcdev->async_cmd_region_size) { > + if (errno == EAGAIN) { > + goto again; > + } > + error_report("vfio-ccw: write cmd region failed with errno=%d", errno); > + ret = -errno; > + } else { > + ret = region->ret_code; > + } > + switch (ret) { > + case 0: > + case -EBUSY: > + case -ENODEV: > + case -EACCES: > + return 0; > + case -EFAULT: > + default: > + sch_gen_unit_exception(sch); > + css_inject_io_interrupt(sch); > + return 0; > + } > +} > + > static void vfio_ccw_reset(DeviceState *dev) > { > CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev); > @@ -287,9 +374,13 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) > return; > } > > + /* > + * We always expect at least the I/O region to be present. We also > + * may have a variable number of regions governed by capabilities. > + */ > if (vdev->num_regions < VFIO_CCW_CONFIG_REGION_INDEX + 1) { > - error_setg(errp, "vfio: Unexpected number of the I/O region %u", > - vdev->num_regions); > + error_setg(errp, "vfio: too few regions (%u), expected at least %u", > + vdev->num_regions, VFIO_CCW_CONFIG_REGION_INDEX + 1); > return; > } > > @@ -309,11 +400,26 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) > vcdev->io_region_offset = info->offset; > vcdev->io_region = g_malloc0(info->size); > > + /* check for the optional async command region */ > + ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW, > + VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD, &info); > + if (!ret) { > + vcdev->async_cmd_region_size = info->size; > + if (sizeof(*vcdev->async_cmd_region) != vcdev->async_cmd_region_size) { > + error_setg(errp, "vfio: Unexpected size of the async cmd region"); > + g_free(info); > + return; > + } > + vcdev->async_cmd_region_offset = info->offset; > + vcdev->async_cmd_region = g_malloc0(info->size); > + } > + > g_free(info); > } > > static void vfio_ccw_put_region(VFIOCCWDevice *vcdev) > { > + g_free(vcdev->async_cmd_region); > g_free(vcdev->io_region); > } I think we can have a memory leak given how the code is currently structured and how we call vfio_ccw_get_region. vfio_ccw_get_region is called in vfio_ccw_realize. So if we return an error from vfio_ccw_get_region, we would jump to out_region_err in vfio_ccw_realize which would call vfio_ccw_put_device. Now we can also return an error from vfio_ccw_get_region for the async region, and so we might never end up freeing the io_region for which we allocated memory successfully. I think we would also need to change vfio_ccw_realize, no? Thanks Farhan > > @@ -486,6 +592,8 @@ static void vfio_ccw_class_init(ObjectClass *klass, void *data) > dc->reset = vfio_ccw_reset; > > cdc->handle_request = vfio_ccw_handle_request; > + cdc->handle_halt = vfio_ccw_handle_halt; > + cdc->handle_clear = vfio_ccw_handle_clear; > } > > static const TypeInfo vfio_ccw_info = { > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h > index 7cc183ef4366..d033387fba8a 100644 > --- a/include/hw/s390x/css.h > +++ b/include/hw/s390x/css.h > @@ -215,6 +215,9 @@ IOInstEnding s390_ccw_cmd_request(SubchDev *sch); > IOInstEnding do_subchannel_work_virtual(SubchDev *sub); > IOInstEnding do_subchannel_work_passthrough(SubchDev *sub); > > +int s390_ccw_halt(SubchDev *sch); > +int s390_ccw_clear(SubchDev *sch); > + > typedef enum { > CSS_IO_ADAPTER_VIRTIO = 0, > CSS_IO_ADAPTER_PCI = 1, > diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h > index 901d805d79a3..fffb54562f6d 100644 > --- a/include/hw/s390x/s390-ccw.h > +++ b/include/hw/s390x/s390-ccw.h > @@ -35,6 +35,8 @@ typedef struct S390CCWDeviceClass { > void (*realize)(S390CCWDevice *dev, char *sysfsdev, Error **errp); > void (*unrealize)(S390CCWDevice *dev, Error **errp); > IOInstEnding (*handle_request) (SubchDev *sch); > + int (*handle_halt) (SubchDev *sch); > + int (*handle_clear) (SubchDev *sch); > } S390CCWDeviceClass; > > #endif >
On 06/11/2019 07:37 AM, Cornelia Huck wrote: > On Fri, 7 Jun 2019 11:19:09 -0400 > Farhan Ali <alifm@linux.ibm.com> wrote: > >> On 06/07/2019 11:09 AM, Cornelia Huck wrote: >>> On Fri, 7 Jun 2019 11:02:36 -0400 >>> Farhan Ali <alifm@linux.ibm.com> wrote: >>> >>>> On 06/07/2019 10:53 AM, Cornelia Huck wrote: > >>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >>>>> index ad310b9f94bc..b92395f165e6 100644 >>>>> --- a/hw/s390x/css.c >>>>> +++ b/hw/s390x/css.c >>>>> @@ -22,6 +22,7 @@ >>>>> #include "trace.h" >>>>> #include "hw/s390x/s390_flic.h" >>>>> #include "hw/s390x/s390-virtio-ccw.h" >>>>> +#include "hw/s390x/s390-ccw.h" >>>>> >>>>> typedef struct CrwContainer { >>>>> CRW crw; >>>>> @@ -1205,6 +1206,26 @@ static void sch_handle_start_func_virtual(SubchDev *sch) >>>>> >>>>> } >>>>> >>>>> +static void sch_handle_halt_func_passthrough(SubchDev *sch) >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + ret = s390_ccw_halt(sch); >>>>> + if (ret == -ENOSYS) { >>>>> + sch_handle_halt_func(sch); >>>>> + } >>>>> +} >>>>> + >>>>> +static void sch_handle_clear_func_passthrough(SubchDev *sch) >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + ret = s390_ccw_clear(sch); >>>>> + if (ret == -ENOSYS) { >>>>> + sch_handle_clear_func(sch); >>>>> + } >>>>> +} >>>>> + >>>> >>>> do we need an extra s390_ccw_clear/halt functions? can't we just call >>>> cdc->clear/halt in the passthrough functions? >>> >>> I mostly added them for symmetry reasons... we still need to check for >>> presence of the callback in any case, though. >>> >>> (vfio is not always built, e.g. on windows or os x.) >> >> >> right, but if we are calling do_subchannel_work_passthrough, then we >> know for sure we are building the S390CCWDevice which is the vfio >> device, no? >> >> So we could just add checks for callbacks in >> sch_handle_clear/halt_func_passthrough, no? >> >> I would even like to get rid of the s390_ccw_cmd_request if we can, but >> that is out of scope for this patch. :) > > Ok, I just walked through various source files (some of which are a bit > confusingly named) again and now I understand again why it was done > that way in the first place. > > - hw/s390x/s390-ccw.c provides some interfaces for pass-through ccw > devices. It is built unconditionally, and its interfaces are called > unconditionally from the css code. > It also provides a device class where code can hook up callbacks. > - hw/vfio/ccw.c (which is not built for !KVM) actually hooks up > callbacks in that device class. > > So, s390-ccw.c (not to be confused with ccw-device.c...) provides a > layer that makes it possible to call things unconditionally, regardless > whether we have vfio-ccw available or not. Not that the code ends up > being called without vfio-ccw support; but the class indirection > enables the code to be built. > Okay, now I get it. Thanks for the explanation, I really do appreciate it! :) > There's possibly a way to make this work without the class indirection > as well, but I think we'd end up doing code juggling before ending up > with something that's not really nicer than the code we have now. > Therefore, I'd prefer to keep the class handling and hook up the > halt/clear callbacks there as well. > > Yeah I agree, given the constraints I don't think any code juggling would look any prettier. Thanks Farhan
On Tue, 11 Jun 2019 15:33:59 -0400 Farhan Ali <alifm@linux.ibm.com> wrote: > On 06/07/2019 10:53 AM, Cornelia Huck wrote: > > A vfio-ccw device may provide an async command subregion for > > issuing halt/clear subchannel requests. If it is present, use > > it for sending halt/clear request to the device; if not, fall > > back to emulation (as done today). > > > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > --- > > > > v4->v5: > > - It seems we need to take the indirection via the class for the > > callbacks after all :( > > - Dropped Eric's R-b: for that reason > > > > --- > > hw/s390x/css.c | 27 +++++++-- > > hw/s390x/s390-ccw.c | 20 +++++++ > > hw/vfio/ccw.c | 112 +++++++++++++++++++++++++++++++++++- > > include/hw/s390x/css.h | 3 + > > include/hw/s390x/s390-ccw.h | 2 + > > 5 files changed, 158 insertions(+), 6 deletions(-) > > > > @@ -309,11 +400,26 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) > > vcdev->io_region_offset = info->offset; > > vcdev->io_region = g_malloc0(info->size); > > > > + /* check for the optional async command region */ > > + ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW, > > + VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD, &info); > > + if (!ret) { > > + vcdev->async_cmd_region_size = info->size; > > + if (sizeof(*vcdev->async_cmd_region) != vcdev->async_cmd_region_size) { > > + error_setg(errp, "vfio: Unexpected size of the async cmd region"); > > + g_free(info); > > + return; > > + } > > + vcdev->async_cmd_region_offset = info->offset; > > + vcdev->async_cmd_region = g_malloc0(info->size); > > + } > > + > > g_free(info); > > } > > > > static void vfio_ccw_put_region(VFIOCCWDevice *vcdev) > > { > > + g_free(vcdev->async_cmd_region); > > g_free(vcdev->io_region); > > } > > I think we can have a memory leak given how the code is currently > structured and how we call vfio_ccw_get_region. > > vfio_ccw_get_region is called in vfio_ccw_realize. So if we return an > error from vfio_ccw_get_region, we would jump to out_region_err in > vfio_ccw_realize which would call vfio_ccw_put_device. > > Now we can also return an error from vfio_ccw_get_region for the async > region, and so we might never end up freeing the io_region for which we > allocated memory successfully. > > I think we would also need to change vfio_ccw_realize, no? Indeed, you're right. I have the following change on top: diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index c9d1c76b4d04..d21ac24f743c 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -407,6 +407,7 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) vcdev->async_cmd_region_size = info->size; if (sizeof(*vcdev->async_cmd_region) != vcdev->async_cmd_region_size) { error_setg(errp, "vfio: Unexpected size of the async cmd region"); + g_free(vcdev->io_region); g_free(info); return; } Anything else before I respin?
On 06/12/2019 05:38 AM, Cornelia Huck wrote: > On Tue, 11 Jun 2019 15:33:59 -0400 > Farhan Ali <alifm@linux.ibm.com> wrote: > >> On 06/07/2019 10:53 AM, Cornelia Huck wrote: >>> A vfio-ccw device may provide an async command subregion for >>> issuing halt/clear subchannel requests. If it is present, use >>> it for sending halt/clear request to the device; if not, fall >>> back to emulation (as done today). >>> >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> >>> --- >>> >>> v4->v5: >>> - It seems we need to take the indirection via the class for the >>> callbacks after all :( >>> - Dropped Eric's R-b: for that reason >>> >>> --- >>> hw/s390x/css.c | 27 +++++++-- >>> hw/s390x/s390-ccw.c | 20 +++++++ >>> hw/vfio/ccw.c | 112 +++++++++++++++++++++++++++++++++++- >>> include/hw/s390x/css.h | 3 + >>> include/hw/s390x/s390-ccw.h | 2 + >>> 5 files changed, 158 insertions(+), 6 deletions(-) >>> > >>> @@ -309,11 +400,26 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) >>> vcdev->io_region_offset = info->offset; >>> vcdev->io_region = g_malloc0(info->size); >>> >>> + /* check for the optional async command region */ >>> + ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW, >>> + VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD, &info); >>> + if (!ret) { >>> + vcdev->async_cmd_region_size = info->size; >>> + if (sizeof(*vcdev->async_cmd_region) != vcdev->async_cmd_region_size) { >>> + error_setg(errp, "vfio: Unexpected size of the async cmd region"); >>> + g_free(info); >>> + return; >>> + } >>> + vcdev->async_cmd_region_offset = info->offset; >>> + vcdev->async_cmd_region = g_malloc0(info->size); >>> + } >>> + >>> g_free(info); >>> } >>> >>> static void vfio_ccw_put_region(VFIOCCWDevice *vcdev) >>> { >>> + g_free(vcdev->async_cmd_region); >>> g_free(vcdev->io_region); >>> } >> >> I think we can have a memory leak given how the code is currently >> structured and how we call vfio_ccw_get_region. >> >> vfio_ccw_get_region is called in vfio_ccw_realize. So if we return an >> error from vfio_ccw_get_region, we would jump to out_region_err in >> vfio_ccw_realize which would call vfio_ccw_put_device. >> >> Now we can also return an error from vfio_ccw_get_region for the async >> region, and so we might never end up freeing the io_region for which we >> allocated memory successfully. >> >> I think we would also need to change vfio_ccw_realize, no? > > Indeed, you're right. I have the following change on top: > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index c9d1c76b4d04..d21ac24f743c 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -407,6 +407,7 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) > vcdev->async_cmd_region_size = info->size; > if (sizeof(*vcdev->async_cmd_region) != vcdev->async_cmd_region_size) { > error_setg(errp, "vfio: Unexpected size of the async cmd region"); > + g_free(vcdev->io_region); > g_free(info); > return; > } This looks good to me. > > Anything else before I respin? > > Nothing else that jumped out to me. Reviewed-by: Farhan Ali <alifm@linux.ibm.com>
diff --git a/hw/s390x/css.c b/hw/s390x/css.c index ad310b9f94bc..b92395f165e6 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -22,6 +22,7 @@ #include "trace.h" #include "hw/s390x/s390_flic.h" #include "hw/s390x/s390-virtio-ccw.h" +#include "hw/s390x/s390-ccw.h" typedef struct CrwContainer { CRW crw; @@ -1205,6 +1206,26 @@ static void sch_handle_start_func_virtual(SubchDev *sch) } +static void sch_handle_halt_func_passthrough(SubchDev *sch) +{ + int ret; + + ret = s390_ccw_halt(sch); + if (ret == -ENOSYS) { + sch_handle_halt_func(sch); + } +} + +static void sch_handle_clear_func_passthrough(SubchDev *sch) +{ + int ret; + + ret = s390_ccw_clear(sch); + if (ret == -ENOSYS) { + sch_handle_clear_func(sch); + } +} + static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch) { SCHIB *schib = &sch->curr_status; @@ -1244,11 +1265,9 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev *sch) SCHIB *schib = &sch->curr_status; if (schib->scsw.ctrl & SCSW_FCTL_CLEAR_FUNC) { - /* TODO: Clear handling */ - sch_handle_clear_func(sch); + sch_handle_clear_func_passthrough(sch); } else if (schib->scsw.ctrl & SCSW_FCTL_HALT_FUNC) { - /* TODO: Halt handling */ - sch_handle_halt_func(sch); + sch_handle_halt_func_passthrough(sch); } else if (schib->scsw.ctrl & SCSW_FCTL_START_FUNC) { return sch_handle_start_func_passthrough(sch); } diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c index f5f025d1b6ca..951be5ab0245 100644 --- a/hw/s390x/s390-ccw.c +++ b/hw/s390x/s390-ccw.c @@ -29,6 +29,26 @@ IOInstEnding s390_ccw_cmd_request(SubchDev *sch) return cdc->handle_request(sch); } +int s390_ccw_halt(SubchDev *sch) +{ + S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data); + + if (!cdc->handle_halt) { + return -ENOSYS; + } + return cdc->handle_halt(sch); +} + +int s390_ccw_clear(SubchDev *sch) +{ + S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data); + + if (!cdc->handle_clear) { + return -ENOSYS; + } + return cdc->handle_clear(sch); +} + static void s390_ccw_get_dev_info(S390CCWDevice *cdev, char *sysfsdev, Error **errp) diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index d9e39552e237..c9d1c76b4d04 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -2,9 +2,12 @@ * vfio based subchannel assignment support * * Copyright 2017 IBM Corp. + * Copyright 2019 Red Hat, Inc. + * * 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> + * Cornelia Huck <cohuck@redhat.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 @@ -32,6 +35,9 @@ struct VFIOCCWDevice { uint64_t io_region_size; uint64_t io_region_offset; struct ccw_io_region *io_region; + uint64_t async_cmd_region_size; + uint64_t async_cmd_region_offset; + struct ccw_cmd_region *async_cmd_region; EventNotifier io_notifier; bool force_orb_pfch; bool warned_orb_pfch; @@ -114,6 +120,87 @@ again: } } +static int vfio_ccw_handle_clear(SubchDev *sch) +{ + S390CCWDevice *cdev = sch->driver_data; + VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); + struct ccw_cmd_region *region = vcdev->async_cmd_region; + int ret; + + if (!vcdev->async_cmd_region) { + /* Async command region not available, fall back to emulation */ + return -ENOSYS; + } + + memset(region, 0, sizeof(*region)); + region->command = VFIO_CCW_ASYNC_CMD_CSCH; + +again: + ret = pwrite(vcdev->vdev.fd, region, + vcdev->async_cmd_region_size, vcdev->async_cmd_region_offset); + if (ret != vcdev->async_cmd_region_size) { + if (errno == EAGAIN) { + goto again; + } + error_report("vfio-ccw: write cmd region failed with errno=%d", errno); + ret = -errno; + } else { + ret = region->ret_code; + } + switch (ret) { + case 0: + case -ENODEV: + case -EACCES: + return 0; + case -EFAULT: + default: + sch_gen_unit_exception(sch); + css_inject_io_interrupt(sch); + return 0; + } +} + +static int vfio_ccw_handle_halt(SubchDev *sch) +{ + S390CCWDevice *cdev = sch->driver_data; + VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); + struct ccw_cmd_region *region = vcdev->async_cmd_region; + int ret; + + if (!vcdev->async_cmd_region) { + /* Async command region not available, fall back to emulation */ + return -ENOSYS; + } + + memset(region, 0, sizeof(*region)); + region->command = VFIO_CCW_ASYNC_CMD_HSCH; + +again: + ret = pwrite(vcdev->vdev.fd, region, + vcdev->async_cmd_region_size, vcdev->async_cmd_region_offset); + if (ret != vcdev->async_cmd_region_size) { + if (errno == EAGAIN) { + goto again; + } + error_report("vfio-ccw: write cmd region failed with errno=%d", errno); + ret = -errno; + } else { + ret = region->ret_code; + } + switch (ret) { + case 0: + case -EBUSY: + case -ENODEV: + case -EACCES: + return 0; + case -EFAULT: + default: + sch_gen_unit_exception(sch); + css_inject_io_interrupt(sch); + return 0; + } +} + static void vfio_ccw_reset(DeviceState *dev) { CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev); @@ -287,9 +374,13 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) return; } + /* + * We always expect at least the I/O region to be present. We also + * may have a variable number of regions governed by capabilities. + */ if (vdev->num_regions < VFIO_CCW_CONFIG_REGION_INDEX + 1) { - error_setg(errp, "vfio: Unexpected number of the I/O region %u", - vdev->num_regions); + error_setg(errp, "vfio: too few regions (%u), expected at least %u", + vdev->num_regions, VFIO_CCW_CONFIG_REGION_INDEX + 1); return; } @@ -309,11 +400,26 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) vcdev->io_region_offset = info->offset; vcdev->io_region = g_malloc0(info->size); + /* check for the optional async command region */ + ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW, + VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD, &info); + if (!ret) { + vcdev->async_cmd_region_size = info->size; + if (sizeof(*vcdev->async_cmd_region) != vcdev->async_cmd_region_size) { + error_setg(errp, "vfio: Unexpected size of the async cmd region"); + g_free(info); + return; + } + vcdev->async_cmd_region_offset = info->offset; + vcdev->async_cmd_region = g_malloc0(info->size); + } + g_free(info); } static void vfio_ccw_put_region(VFIOCCWDevice *vcdev) { + g_free(vcdev->async_cmd_region); g_free(vcdev->io_region); } @@ -486,6 +592,8 @@ static void vfio_ccw_class_init(ObjectClass *klass, void *data) dc->reset = vfio_ccw_reset; cdc->handle_request = vfio_ccw_handle_request; + cdc->handle_halt = vfio_ccw_handle_halt; + cdc->handle_clear = vfio_ccw_handle_clear; } static const TypeInfo vfio_ccw_info = { diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h index 7cc183ef4366..d033387fba8a 100644 --- a/include/hw/s390x/css.h +++ b/include/hw/s390x/css.h @@ -215,6 +215,9 @@ IOInstEnding s390_ccw_cmd_request(SubchDev *sch); IOInstEnding do_subchannel_work_virtual(SubchDev *sub); IOInstEnding do_subchannel_work_passthrough(SubchDev *sub); +int s390_ccw_halt(SubchDev *sch); +int s390_ccw_clear(SubchDev *sch); + typedef enum { CSS_IO_ADAPTER_VIRTIO = 0, CSS_IO_ADAPTER_PCI = 1, diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h index 901d805d79a3..fffb54562f6d 100644 --- a/include/hw/s390x/s390-ccw.h +++ b/include/hw/s390x/s390-ccw.h @@ -35,6 +35,8 @@ typedef struct S390CCWDeviceClass { void (*realize)(S390CCWDevice *dev, char *sysfsdev, Error **errp); void (*unrealize)(S390CCWDevice *dev, Error **errp); IOInstEnding (*handle_request) (SubchDev *sch); + int (*handle_halt) (SubchDev *sch); + int (*handle_clear) (SubchDev *sch); } S390CCWDeviceClass; #endif
A vfio-ccw device may provide an async command subregion for issuing halt/clear subchannel requests. If it is present, use it for sending halt/clear request to the device; if not, fall back to emulation (as done today). Signed-off-by: Cornelia Huck <cohuck@redhat.com> --- v4->v5: - It seems we need to take the indirection via the class for the callbacks after all :( - Dropped Eric's R-b: for that reason --- hw/s390x/css.c | 27 +++++++-- hw/s390x/s390-ccw.c | 20 +++++++ hw/vfio/ccw.c | 112 +++++++++++++++++++++++++++++++++++- include/hw/s390x/css.h | 3 + include/hw/s390x/s390-ccw.h | 2 + 5 files changed, 158 insertions(+), 6 deletions(-)