| Message ID | 1548768562-20007-2-git-send-email-jjherne@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390: vfio-ccw dasd ipl support | expand |
On Tue, 29 Jan 2019 08:29:08 -0500 "Jason J. Herne" <jjherne@linux.ibm.com> wrote: > Add bootindex property and iplb data for vfio-ccw devices. This allows us to > forward boot information into the bios for vfio-ccw devices. > > Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com> > Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > hw/s390x/ipl.c | 14 ++++++++++++++ > hw/s390x/s390-ccw.c | 9 +++++++++ > hw/vfio/ccw.c | 13 +------------ > include/hw/s390x/s390-ccw.h | 1 + > include/hw/s390x/vfio-ccw.h | 38 ++++++++++++++++++++++++++++++++++++++ > 5 files changed, 63 insertions(+), 12 deletions(-) > create mode 100644 include/hw/s390x/vfio-ccw.h > diff --git a/include/hw/s390x/vfio-ccw.h b/include/hw/s390x/vfio-ccw.h > new file mode 100644 > index 0000000..a7d699d > --- /dev/null > +++ b/include/hw/s390x/vfio-ccw.h > @@ -0,0 +1,38 @@ > +/* > + * vfio based subchannel assignment support > + * > + * Copyright 2018 IBM Corp. Why 2018? Should either be 2017 (the original date for hw/vfio/ccw.c) or 2019. > + * 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. > + */ > + > +#ifndef HW_VFIO_CCW_H > +#define HW_VFIO_CCW_H > + > +#include "hw/vfio/vfio-common.h" > +#include "hw/s390x/s390-ccw.h" > +#include "hw/s390x/ccw-device.h" > + > +#define TYPE_VFIO_CCW "vfio-ccw" > +#define VFIO_CCW(obj) \ > + OBJECT_CHECK(VFIOCCWDevice, (obj), TYPE_VFIO_CCW) > + > + > +#define TYPE_VFIO_CCW "vfio-ccw" > +typedef struct VFIOCCWDevice { > + S390CCWDevice cdev; > + VFIODevice vdev; > + uint64_t io_region_size; > + uint64_t io_region_offset; > + struct ccw_io_region *io_region; > + EventNotifier io_notifier; > + bool force_orb_pfch; > + bool warned_orb_pfch; > +} VFIOCCWDevice; > + > +#endif
On 1/30/19 11:56 AM, Cornelia Huck wrote: > On Tue, 29 Jan 2019 08:29:08 -0500 > "Jason J. Herne" <jjherne@linux.ibm.com> wrote: > >> Add bootindex property and iplb data for vfio-ccw devices. This allows us to >> forward boot information into the bios for vfio-ccw devices. >> >> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com> >> Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> hw/s390x/ipl.c | 14 ++++++++++++++ >> hw/s390x/s390-ccw.c | 9 +++++++++ >> hw/vfio/ccw.c | 13 +------------ >> include/hw/s390x/s390-ccw.h | 1 + >> include/hw/s390x/vfio-ccw.h | 38 ++++++++++++++++++++++++++++++++++++++ >> 5 files changed, 63 insertions(+), 12 deletions(-) >> create mode 100644 include/hw/s390x/vfio-ccw.h > >> diff --git a/include/hw/s390x/vfio-ccw.h b/include/hw/s390x/vfio-ccw.h >> new file mode 100644 >> index 0000000..a7d699d >> --- /dev/null >> +++ b/include/hw/s390x/vfio-ccw.h >> @@ -0,0 +1,38 @@ >> +/* >> + * vfio based subchannel assignment support >> + * >> + * Copyright 2018 IBM Corp. > > Why 2018? Should either be 2017 (the original date for hw/vfio/ccw.c) > or 2019. > Okay. I will fix it.
On 01/29/2019 08:29 AM, Jason J. Herne wrote: > Add bootindex property and iplb data for vfio-ccw devices. This allows us to > forward boot information into the bios for vfio-ccw devices. > > Signed-off-by: Jason J. Herne<jjherne@linux.ibm.com> > Acked-by: Halil Pasic<pasic@linux.vnet.ibm.com> > --- > hw/s390x/ipl.c | 14 ++++++++++++++ > hw/s390x/s390-ccw.c | 9 +++++++++ > hw/vfio/ccw.c | 13 +------------ > include/hw/s390x/s390-ccw.h | 1 + > include/hw/s390x/vfio-ccw.h | 38 ++++++++++++++++++++++++++++++++++++++ > 5 files changed, 63 insertions(+), 12 deletions(-) > create mode 100644 include/hw/s390x/vfio-ccw.h > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index 21f64ad..a993f65 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -19,6 +19,7 @@ > #include "hw/loader.h" > #include "hw/boards.h" > #include "hw/s390x/virtio-ccw.h" > +#include "hw/s390x/vfio-ccw.h" > #include "hw/s390x/css.h" > #include "hw/s390x/ebcdic.h" > #include "ipl.h" > @@ -311,8 +312,12 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st) > VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *) > object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent), > TYPE_VIRTIO_CCW_DEVICE); > + VFIOCCWDevice *vfio_ccw_dev = (VFIOCCWDevice *) > + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); > if (virtio_ccw_dev) { > ccw_dev = CCW_DEVICE(virtio_ccw_dev); > + } else if (vfio_ccw_dev) { > + ccw_dev = CCW_DEVICE(vfio_ccw_dev); > } else { > SCSIDevice *sd = (SCSIDevice *) > object_dynamic_cast(OBJECT(dev_st), > @@ -347,6 +352,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) > if (ccw_dev) { > SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st), > TYPE_SCSI_DEVICE); > + VFIOCCWDevice *vc = (VFIOCCWDevice *) > + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); > > if (sd) { > ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN); > @@ -358,6 +365,13 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) > ipl->iplb.scsi.channel = cpu_to_be16(sd->channel); > ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno); > ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3; > + } else if (vc) { > + CcwDevice *ccw_dev = CCW_DEVICE(vc); Do we need this line here? I though ccw_dev was already correctly casted in s390_get_ccw_device? > + > + ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN); > + ipl->iplb.pbt = S390_IPL_TYPE_CCW; > + ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno); > + ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3; > } else { > VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st), > TYPE_VIRTIO_NET);
On Tue, 29 Jan 2019 08:29:08 -0500 "Jason J. Herne" <jjherne@linux.ibm.com> wrote: > Add bootindex property and iplb data for vfio-ccw devices. This allows us to > forward boot information into the bios for vfio-ccw devices. > > Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com> > Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > hw/s390x/ipl.c | 14 ++++++++++++++ > hw/s390x/s390-ccw.c | 9 +++++++++ > hw/vfio/ccw.c | 13 +------------ > include/hw/s390x/s390-ccw.h | 1 + > include/hw/s390x/vfio-ccw.h | 38 ++++++++++++++++++++++++++++++++++++++ This split-out file should probably go into the vfio-ccw section in MAINTAINERS as well. > 5 files changed, 63 insertions(+), 12 deletions(-) > create mode 100644 include/hw/s390x/vfio-ccw.h
On Tue, 29 Jan 2019 08:29:08 -0500 "Jason J. Herne" <jjherne@linux.ibm.com> wrote: > Add bootindex property and iplb data for vfio-ccw devices. This allows us to > forward boot information into the bios for vfio-ccw devices. > > Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com> > Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > hw/s390x/ipl.c | 14 ++++++++++++++ > hw/s390x/s390-ccw.c | 9 +++++++++ > hw/vfio/ccw.c | 13 +------------ > include/hw/s390x/s390-ccw.h | 1 + > include/hw/s390x/vfio-ccw.h | 38 ++++++++++++++++++++++++++++++++++++++ > 5 files changed, 63 insertions(+), 12 deletions(-) > create mode 100644 include/hw/s390x/vfio-ccw.h > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index 21f64ad..a993f65 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -19,6 +19,7 @@ > #include "hw/loader.h" > #include "hw/boards.h" > #include "hw/s390x/virtio-ccw.h" > +#include "hw/s390x/vfio-ccw.h" > #include "hw/s390x/css.h" > #include "hw/s390x/ebcdic.h" > #include "ipl.h" > @@ -311,8 +312,12 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st) > VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *) > object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent), > TYPE_VIRTIO_CCW_DEVICE); > + VFIOCCWDevice *vfio_ccw_dev = (VFIOCCWDevice *) > + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); > if (virtio_ccw_dev) { > ccw_dev = CCW_DEVICE(virtio_ccw_dev); > + } else if (vfio_ccw_dev) { > + ccw_dev = CCW_DEVICE(vfio_ccw_dev); > } else { > SCSIDevice *sd = (SCSIDevice *) > object_dynamic_cast(OBJECT(dev_st), > @@ -347,6 +352,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) > if (ccw_dev) { > SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st), > TYPE_SCSI_DEVICE); > + VFIOCCWDevice *vc = (VFIOCCWDevice *) > + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); > > if (sd) { > ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN); > @@ -358,6 +365,13 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) > ipl->iplb.scsi.channel = cpu_to_be16(sd->channel); > ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno); > ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3; > + } else if (vc) { > + CcwDevice *ccw_dev = CCW_DEVICE(vc); > + > + ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN); > + ipl->iplb.pbt = S390_IPL_TYPE_CCW; > + ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno); > + ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3; > } else { > VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st), > TYPE_VIRTIO_NET); Hm, I think that this find-out-the-boot-type-and-set-up-the-right-thing mechanism is getting a bit unwieldy. Basically, we - call s390_get_ccw_device() to find out the device type via a bunch of casts and return a pointer to a CcwDevice if it's a supported type - do a bunch of casts here *again* to find out what we have and fill out the iplb, while we really only need to do grab a non-CcwDevice for the scsi case Should maybe s390_get_ccw_device() give us an ipl type in addition to the pointer to the CcwDevice, so we can use a switch/case statement to fill out the iplb here?
On 2019-01-29 14:29, Jason J. Herne wrote: > Add bootindex property and iplb data for vfio-ccw devices. This allows us to > forward boot information into the bios for vfio-ccw devices. > > Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com> > Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > hw/s390x/ipl.c | 14 ++++++++++++++ > hw/s390x/s390-ccw.c | 9 +++++++++ > hw/vfio/ccw.c | 13 +------------ > include/hw/s390x/s390-ccw.h | 1 + > include/hw/s390x/vfio-ccw.h | 38 ++++++++++++++++++++++++++++++++++++++ > 5 files changed, 63 insertions(+), 12 deletions(-) > create mode 100644 include/hw/s390x/vfio-ccw.h > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index 21f64ad..a993f65 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -19,6 +19,7 @@ > #include "hw/loader.h" > #include "hw/boards.h" > #include "hw/s390x/virtio-ccw.h" > +#include "hw/s390x/vfio-ccw.h" > #include "hw/s390x/css.h" > #include "hw/s390x/ebcdic.h" > #include "ipl.h" > @@ -311,8 +312,12 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st) > VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *) > object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent), > TYPE_VIRTIO_CCW_DEVICE); > + VFIOCCWDevice *vfio_ccw_dev = (VFIOCCWDevice *) > + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); > if (virtio_ccw_dev) { > ccw_dev = CCW_DEVICE(virtio_ccw_dev); > + } else if (vfio_ccw_dev) { > + ccw_dev = CCW_DEVICE(vfio_ccw_dev); > } else { > SCSIDevice *sd = (SCSIDevice *) > object_dynamic_cast(OBJECT(dev_st), > @@ -347,6 +352,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) > if (ccw_dev) { > SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st), > TYPE_SCSI_DEVICE); > + VFIOCCWDevice *vc = (VFIOCCWDevice *) > + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); > > if (sd) { > ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN); > @@ -358,6 +365,13 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) > ipl->iplb.scsi.channel = cpu_to_be16(sd->channel); > ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno); > ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3; > + } else if (vc) { > + CcwDevice *ccw_dev = CCW_DEVICE(vc); > + > + ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN); > + ipl->iplb.pbt = S390_IPL_TYPE_CCW; > + ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno); > + ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3; > } else { > VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st), > TYPE_VIRTIO_NET); > diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c > index cad91ee..f5f025d 100644 > --- a/hw/s390x/s390-ccw.c > +++ b/hw/s390x/s390-ccw.c > @@ -124,6 +124,14 @@ static void s390_ccw_unrealize(S390CCWDevice *cdev, Error **errp) > g_free(cdev->mdevid); > } > > +static void s390_ccw_instance_init(Object *obj) > +{ > + S390CCWDevice *dev = S390_CCW_DEVICE(obj); > + > + device_add_bootindex_property(obj, &dev->bootindex, "bootindex", > + "/disk@0,0", DEVICE(obj), NULL); > +} > + > static void s390_ccw_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -137,6 +145,7 @@ static void s390_ccw_class_init(ObjectClass *klass, void *data) > static const TypeInfo s390_ccw_info = { > .name = TYPE_S390_CCW, > .parent = TYPE_CCW_DEVICE, > + .instance_init = s390_ccw_instance_init, > .instance_size = sizeof(S390CCWDevice), > .class_size = sizeof(S390CCWDeviceClass), > .class_init = s390_ccw_class_init, > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 9246729..d815a4f 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -21,22 +21,11 @@ > #include "hw/vfio/vfio.h" > #include "hw/vfio/vfio-common.h" > #include "hw/s390x/s390-ccw.h" > +#include "hw/s390x/vfio-ccw.h" > #include "hw/s390x/ccw-device.h" > #include "exec/address-spaces.h" > #include "qemu/error-report.h" > > -#define TYPE_VFIO_CCW "vfio-ccw" > -typedef struct VFIOCCWDevice { > - S390CCWDevice cdev; > - VFIODevice vdev; > - uint64_t io_region_size; > - uint64_t io_region_offset; > - struct ccw_io_region *io_region; > - EventNotifier io_notifier; > - bool force_orb_pfch; > - bool warned_orb_pfch; > -} VFIOCCWDevice; > - > static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch, > const char *msg) > { > diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h > index 7d15a1a..901d805 100644 > --- a/include/hw/s390x/s390-ccw.h > +++ b/include/hw/s390x/s390-ccw.h > @@ -27,6 +27,7 @@ typedef struct S390CCWDevice { > CcwDevice parent_obj; > CssDevId hostid; > char *mdevid; > + int32_t bootindex; > } S390CCWDevice; > > typedef struct S390CCWDeviceClass { > diff --git a/include/hw/s390x/vfio-ccw.h b/include/hw/s390x/vfio-ccw.h > new file mode 100644 > index 0000000..a7d699d > --- /dev/null > +++ b/include/hw/s390x/vfio-ccw.h > @@ -0,0 +1,38 @@ > +/* > + * vfio based subchannel assignment support > + * > + * Copyright 2018 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. > + */ > + > +#ifndef HW_VFIO_CCW_H > +#define HW_VFIO_CCW_H > + > +#include "hw/vfio/vfio-common.h" > +#include "hw/s390x/s390-ccw.h" > +#include "hw/s390x/ccw-device.h" > + > +#define TYPE_VFIO_CCW "vfio-ccw" > +#define VFIO_CCW(obj) \ > + OBJECT_CHECK(VFIOCCWDevice, (obj), TYPE_VFIO_CCW) > + > + Remove one empty line, please. > +#define TYPE_VFIO_CCW "vfio-ccw" > +typedef struct VFIOCCWDevice { > + S390CCWDevice cdev; > + VFIODevice vdev; > + uint64_t io_region_size; > + uint64_t io_region_offset; > + struct ccw_io_region *io_region; > + EventNotifier io_notifier; > + bool force_orb_pfch; > + bool warned_orb_pfch; > +} VFIOCCWDevice; Do you really need to make the whole structure public here? If not, I think it would be sufficient to only have the "anonymous" typedef here: typedef struct VFIOCCWDevice VFIOCCWDevice; Thomas
On 2/6/19 6:30 AM, Thomas Huth wrote: > On 2019-01-29 14:29, Jason J. Herne wrote: >> Add bootindex property and iplb data for vfio-ccw devices. This allows us to >> forward boot information into the bios for vfio-ccw devices. >> >> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com> >> Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> hw/s390x/ipl.c | 14 ++++++++++++++ >> hw/s390x/s390-ccw.c | 9 +++++++++ >> hw/vfio/ccw.c | 13 +------------ >> include/hw/s390x/s390-ccw.h | 1 + >> include/hw/s390x/vfio-ccw.h | 38 ++++++++++++++++++++++++++++++++++++++ >> 5 files changed, 63 insertions(+), 12 deletions(-) >> create mode 100644 include/hw/s390x/vfio-ccw.h >> >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >> index 21f64ad..a993f65 100644 >> --- a/hw/s390x/ipl.c >> +++ b/hw/s390x/ipl.c >> @@ -19,6 +19,7 @@ >> #include "hw/loader.h" >> #include "hw/boards.h" >> #include "hw/s390x/virtio-ccw.h" >> +#include "hw/s390x/vfio-ccw.h" >> #include "hw/s390x/css.h" >> #include "hw/s390x/ebcdic.h" >> #include "ipl.h" >> @@ -311,8 +312,12 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st) >> VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *) >> object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent), >> TYPE_VIRTIO_CCW_DEVICE); >> + VFIOCCWDevice *vfio_ccw_dev = (VFIOCCWDevice *) >> + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); >> if (virtio_ccw_dev) { >> ccw_dev = CCW_DEVICE(virtio_ccw_dev); >> + } else if (vfio_ccw_dev) { >> + ccw_dev = CCW_DEVICE(vfio_ccw_dev); >> } else { >> SCSIDevice *sd = (SCSIDevice *) >> object_dynamic_cast(OBJECT(dev_st), >> @@ -347,6 +352,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) >> if (ccw_dev) { >> SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st), >> TYPE_SCSI_DEVICE); >> + VFIOCCWDevice *vc = (VFIOCCWDevice *) >> + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); >> >> if (sd) { >> ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN); >> @@ -358,6 +365,13 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) >> ipl->iplb.scsi.channel = cpu_to_be16(sd->channel); >> ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno); >> ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3; >> + } else if (vc) { >> + CcwDevice *ccw_dev = CCW_DEVICE(vc); >> + >> + ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN); >> + ipl->iplb.pbt = S390_IPL_TYPE_CCW; >> + ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno); >> + ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3; >> } else { >> VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st), >> TYPE_VIRTIO_NET); >> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c >> index cad91ee..f5f025d 100644 >> --- a/hw/s390x/s390-ccw.c >> +++ b/hw/s390x/s390-ccw.c >> @@ -124,6 +124,14 @@ static void s390_ccw_unrealize(S390CCWDevice *cdev, Error **errp) >> g_free(cdev->mdevid); >> } >> >> +static void s390_ccw_instance_init(Object *obj) >> +{ >> + S390CCWDevice *dev = S390_CCW_DEVICE(obj); >> + >> + device_add_bootindex_property(obj, &dev->bootindex, "bootindex", >> + "/disk@0,0", DEVICE(obj), NULL); >> +} >> + >> static void s390_ccw_class_init(ObjectClass *klass, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(klass); >> @@ -137,6 +145,7 @@ static void s390_ccw_class_init(ObjectClass *klass, void *data) >> static const TypeInfo s390_ccw_info = { >> .name = TYPE_S390_CCW, >> .parent = TYPE_CCW_DEVICE, >> + .instance_init = s390_ccw_instance_init, >> .instance_size = sizeof(S390CCWDevice), >> .class_size = sizeof(S390CCWDeviceClass), >> .class_init = s390_ccw_class_init, >> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c >> index 9246729..d815a4f 100644 >> --- a/hw/vfio/ccw.c >> +++ b/hw/vfio/ccw.c >> @@ -21,22 +21,11 @@ >> #include "hw/vfio/vfio.h" >> #include "hw/vfio/vfio-common.h" >> #include "hw/s390x/s390-ccw.h" >> +#include "hw/s390x/vfio-ccw.h" >> #include "hw/s390x/ccw-device.h" >> #include "exec/address-spaces.h" >> #include "qemu/error-report.h" >> >> -#define TYPE_VFIO_CCW "vfio-ccw" >> -typedef struct VFIOCCWDevice { >> - S390CCWDevice cdev; >> - VFIODevice vdev; >> - uint64_t io_region_size; >> - uint64_t io_region_offset; >> - struct ccw_io_region *io_region; >> - EventNotifier io_notifier; >> - bool force_orb_pfch; >> - bool warned_orb_pfch; >> -} VFIOCCWDevice; >> - >> static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch, >> const char *msg) >> { >> diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h >> index 7d15a1a..901d805 100644 >> --- a/include/hw/s390x/s390-ccw.h >> +++ b/include/hw/s390x/s390-ccw.h >> @@ -27,6 +27,7 @@ typedef struct S390CCWDevice { >> CcwDevice parent_obj; >> CssDevId hostid; >> char *mdevid; >> + int32_t bootindex; >> } S390CCWDevice; >> >> typedef struct S390CCWDeviceClass { >> diff --git a/include/hw/s390x/vfio-ccw.h b/include/hw/s390x/vfio-ccw.h >> new file mode 100644 >> index 0000000..a7d699d >> --- /dev/null >> +++ b/include/hw/s390x/vfio-ccw.h >> @@ -0,0 +1,38 @@ >> +/* >> + * vfio based subchannel assignment support >> + * >> + * Copyright 2018 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. >> + */ >> + >> +#ifndef HW_VFIO_CCW_H >> +#define HW_VFIO_CCW_H >> + >> +#include "hw/vfio/vfio-common.h" >> +#include "hw/s390x/s390-ccw.h" >> +#include "hw/s390x/ccw-device.h" >> + >> +#define TYPE_VFIO_CCW "vfio-ccw" >> +#define VFIO_CCW(obj) \ >> + OBJECT_CHECK(VFIOCCWDevice, (obj), TYPE_VFIO_CCW) >> + >> + > > Remove one empty line, please. > >> +#define TYPE_VFIO_CCW "vfio-ccw" >> +typedef struct VFIOCCWDevice { >> + S390CCWDevice cdev; >> + VFIODevice vdev; >> + uint64_t io_region_size; >> + uint64_t io_region_offset; >> + struct ccw_io_region *io_region; >> + EventNotifier io_notifier; >> + bool force_orb_pfch; >> + bool warned_orb_pfch; >> +} VFIOCCWDevice; > > Do you really need to make the whole structure public here? If not, I > think it would be sufficient to only have the "anonymous" typedef here: > > typedef struct VFIOCCWDevice VFIOCCWDevice; > > Thomas Perhaps the entire struct is not needed in ipl.c, and we could get by with only the typedef. But then the only thing in vfio-ccw.h will be the one line. Seems a little confusing to me. What do we gain by doing this?
On 1/30/19 5:21 PM, Farhan Ali wrote: >> ... >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >> index 21f64ad..a993f65 100644 >> --- a/hw/s390x/ipl.c >> +++ b/hw/s390x/ipl.c >> @@ -19,6 +19,7 @@ >> #include "hw/loader.h" >> #include "hw/boards.h" >> #include "hw/s390x/virtio-ccw.h" >> +#include "hw/s390x/vfio-ccw.h" >> #include "hw/s390x/css.h" >> #include "hw/s390x/ebcdic.h" >> #include "ipl.h" >> @@ -311,8 +312,12 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st) >> VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *) >> object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent), >> TYPE_VIRTIO_CCW_DEVICE); >> + VFIOCCWDevice *vfio_ccw_dev = (VFIOCCWDevice *) >> + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); >> if (virtio_ccw_dev) { >> ccw_dev = CCW_DEVICE(virtio_ccw_dev); >> + } else if (vfio_ccw_dev) { >> + ccw_dev = CCW_DEVICE(vfio_ccw_dev); >> } else { >> SCSIDevice *sd = (SCSIDevice *) >> object_dynamic_cast(OBJECT(dev_st), >> @@ -347,6 +352,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) >> if (ccw_dev) { >> SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st), >> TYPE_SCSI_DEVICE); >> + VFIOCCWDevice *vc = (VFIOCCWDevice *) >> + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); >> if (sd) { >> ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN); >> @@ -358,6 +365,13 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) >> ipl->iplb.scsi.channel = cpu_to_be16(sd->channel); >> ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno); >> ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3; >> + } else if (vc) { >> + CcwDevice *ccw_dev = CCW_DEVICE(vc); > > Do we need this line here? I though ccw_dev was already correctly casted in > s390_get_ccw_device? > >> + >> + ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN); >> + ipl->iplb.pbt = S390_IPL_TYPE_CCW; >> + ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno); >> + ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3; >> } else { >> VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st), >> TYPE_VIRTIO_NET); > Good catch, we don't need the extra cast. This is a relic of an older way of handling the data. I should have removed it when I simplified the code after the RFC version.
On Fri, 8 Feb 2019 11:04:29 -0500 "Jason J. Herne" <jjherne@linux.ibm.com> wrote: > On 2/6/19 6:30 AM, Thomas Huth wrote: > > On 2019-01-29 14:29, Jason J. Herne wrote: > >> diff --git a/include/hw/s390x/vfio-ccw.h b/include/hw/s390x/vfio-ccw.h > >> new file mode 100644 > >> index 0000000..a7d699d > >> --- /dev/null > >> +++ b/include/hw/s390x/vfio-ccw.h > >> @@ -0,0 +1,38 @@ > >> +/* > >> + * vfio based subchannel assignment support > >> + * > >> + * Copyright 2018 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. > >> + */ > >> + > >> +#ifndef HW_VFIO_CCW_H > >> +#define HW_VFIO_CCW_H > >> + > >> +#include "hw/vfio/vfio-common.h" > >> +#include "hw/s390x/s390-ccw.h" > >> +#include "hw/s390x/ccw-device.h" > >> + > >> +#define TYPE_VFIO_CCW "vfio-ccw" > >> +#define VFIO_CCW(obj) \ > >> + OBJECT_CHECK(VFIOCCWDevice, (obj), TYPE_VFIO_CCW) > >> + > >> + > > > > Remove one empty line, please. > > > >> +#define TYPE_VFIO_CCW "vfio-ccw" > >> +typedef struct VFIOCCWDevice { > >> + S390CCWDevice cdev; > >> + VFIODevice vdev; > >> + uint64_t io_region_size; > >> + uint64_t io_region_offset; > >> + struct ccw_io_region *io_region; > >> + EventNotifier io_notifier; > >> + bool force_orb_pfch; > >> + bool warned_orb_pfch; > >> +} VFIOCCWDevice; > > > > Do you really need to make the whole structure public here? If not, I > > think it would be sufficient to only have the "anonymous" typedef here: > > > > typedef struct VFIOCCWDevice VFIOCCWDevice; > > > > Thomas > Perhaps the entire struct is not needed in ipl.c, and we could get by with only the > typedef. But then the only thing in vfio-ccw.h will be the one line. Seems a little > confusing to me. What do we gain by doing this? What parts of VFIOCCWDevice are, in general, interesting to other code parts? I believe most parts are not potentially useful to anything else... The ipl code needs this mainly to find out what kind of device it deals with. Would it make sense to define a helper function instead and keep the actual definition private to the vfio-ccw code? That also would make the intention more clear.
On 2019-02-08 17:04, Jason J. Herne wrote: > On 2/6/19 6:30 AM, Thomas Huth wrote: >> On 2019-01-29 14:29, Jason J. Herne wrote: >>> Add bootindex property and iplb data for vfio-ccw devices. This >>> allows us to >>> forward boot information into the bios for vfio-ccw devices. >>> >>> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com> >>> Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>> --- >>> hw/s390x/ipl.c | 14 ++++++++++++++ >>> hw/s390x/s390-ccw.c | 9 +++++++++ >>> hw/vfio/ccw.c | 13 +------------ >>> include/hw/s390x/s390-ccw.h | 1 + >>> include/hw/s390x/vfio-ccw.h | 38 >>> ++++++++++++++++++++++++++++++++++++++ >>> 5 files changed, 63 insertions(+), 12 deletions(-) >>> create mode 100644 include/hw/s390x/vfio-ccw.h >>> >>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >>> index 21f64ad..a993f65 100644 >>> --- a/hw/s390x/ipl.c >>> +++ b/hw/s390x/ipl.c >>> @@ -19,6 +19,7 @@ >>> #include "hw/loader.h" >>> #include "hw/boards.h" >>> #include "hw/s390x/virtio-ccw.h" >>> +#include "hw/s390x/vfio-ccw.h" >>> #include "hw/s390x/css.h" >>> #include "hw/s390x/ebcdic.h" >>> #include "ipl.h" >>> @@ -311,8 +312,12 @@ static CcwDevice >>> *s390_get_ccw_device(DeviceState *dev_st) >>> VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *) >>> >>> object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent), >>> TYPE_VIRTIO_CCW_DEVICE); >>> + VFIOCCWDevice *vfio_ccw_dev = (VFIOCCWDevice *) >>> + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); >>> if (virtio_ccw_dev) { >>> ccw_dev = CCW_DEVICE(virtio_ccw_dev); >>> + } else if (vfio_ccw_dev) { >>> + ccw_dev = CCW_DEVICE(vfio_ccw_dev); >>> } else { >>> SCSIDevice *sd = (SCSIDevice *) >>> object_dynamic_cast(OBJECT(dev_st), >>> @@ -347,6 +352,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) >>> if (ccw_dev) { >>> SCSIDevice *sd = (SCSIDevice *) >>> object_dynamic_cast(OBJECT(dev_st), >>> >>> TYPE_SCSI_DEVICE); >>> + VFIOCCWDevice *vc = (VFIOCCWDevice *) >>> + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); >>> if (sd) { >>> ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN); >>> @@ -358,6 +365,13 @@ static bool s390_gen_initial_iplb(S390IPLState >>> *ipl) >>> ipl->iplb.scsi.channel = cpu_to_be16(sd->channel); >>> ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno); >>> ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3; >>> + } else if (vc) { >>> + CcwDevice *ccw_dev = CCW_DEVICE(vc); >>> + >>> + ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN); >>> + ipl->iplb.pbt = S390_IPL_TYPE_CCW; >>> + ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno); >>> + ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3; >>> } else { >>> VirtIONet *vn = (VirtIONet *) >>> object_dynamic_cast(OBJECT(dev_st), >>> >>> TYPE_VIRTIO_NET); >>> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c >>> index cad91ee..f5f025d 100644 >>> --- a/hw/s390x/s390-ccw.c >>> +++ b/hw/s390x/s390-ccw.c >>> @@ -124,6 +124,14 @@ static void s390_ccw_unrealize(S390CCWDevice >>> *cdev, Error **errp) >>> g_free(cdev->mdevid); >>> } >>> +static void s390_ccw_instance_init(Object *obj) >>> +{ >>> + S390CCWDevice *dev = S390_CCW_DEVICE(obj); >>> + >>> + device_add_bootindex_property(obj, &dev->bootindex, "bootindex", >>> + "/disk@0,0", DEVICE(obj), NULL); >>> +} >>> + >>> static void s390_ccw_class_init(ObjectClass *klass, void *data) >>> { >>> DeviceClass *dc = DEVICE_CLASS(klass); >>> @@ -137,6 +145,7 @@ static void s390_ccw_class_init(ObjectClass >>> *klass, void *data) >>> static const TypeInfo s390_ccw_info = { >>> .name = TYPE_S390_CCW, >>> .parent = TYPE_CCW_DEVICE, >>> + .instance_init = s390_ccw_instance_init, >>> .instance_size = sizeof(S390CCWDevice), >>> .class_size = sizeof(S390CCWDeviceClass), >>> .class_init = s390_ccw_class_init, >>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c >>> index 9246729..d815a4f 100644 >>> --- a/hw/vfio/ccw.c >>> +++ b/hw/vfio/ccw.c >>> @@ -21,22 +21,11 @@ >>> #include "hw/vfio/vfio.h" >>> #include "hw/vfio/vfio-common.h" >>> #include "hw/s390x/s390-ccw.h" >>> +#include "hw/s390x/vfio-ccw.h" >>> #include "hw/s390x/ccw-device.h" >>> #include "exec/address-spaces.h" >>> #include "qemu/error-report.h" >>> -#define TYPE_VFIO_CCW "vfio-ccw" >>> -typedef struct VFIOCCWDevice { >>> - S390CCWDevice cdev; >>> - VFIODevice vdev; >>> - uint64_t io_region_size; >>> - uint64_t io_region_offset; >>> - struct ccw_io_region *io_region; >>> - EventNotifier io_notifier; >>> - bool force_orb_pfch; >>> - bool warned_orb_pfch; >>> -} VFIOCCWDevice; >>> - >>> static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch, >>> const char *msg) >>> { >>> diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h >>> index 7d15a1a..901d805 100644 >>> --- a/include/hw/s390x/s390-ccw.h >>> +++ b/include/hw/s390x/s390-ccw.h >>> @@ -27,6 +27,7 @@ typedef struct S390CCWDevice { >>> CcwDevice parent_obj; >>> CssDevId hostid; >>> char *mdevid; >>> + int32_t bootindex; >>> } S390CCWDevice; >>> typedef struct S390CCWDeviceClass { >>> diff --git a/include/hw/s390x/vfio-ccw.h b/include/hw/s390x/vfio-ccw.h >>> new file mode 100644 >>> index 0000000..a7d699d >>> --- /dev/null >>> +++ b/include/hw/s390x/vfio-ccw.h >>> @@ -0,0 +1,38 @@ >>> +/* >>> + * vfio based subchannel assignment support >>> + * >>> + * Copyright 2018 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. >>> + */ >>> + >>> +#ifndef HW_VFIO_CCW_H >>> +#define HW_VFIO_CCW_H >>> + >>> +#include "hw/vfio/vfio-common.h" >>> +#include "hw/s390x/s390-ccw.h" >>> +#include "hw/s390x/ccw-device.h" >>> + >>> +#define TYPE_VFIO_CCW "vfio-ccw" >>> +#define VFIO_CCW(obj) \ >>> + OBJECT_CHECK(VFIOCCWDevice, (obj), TYPE_VFIO_CCW) >>> + >>> + >> >> Remove one empty line, please. >> >>> +#define TYPE_VFIO_CCW "vfio-ccw" >>> +typedef struct VFIOCCWDevice { >>> + S390CCWDevice cdev; >>> + VFIODevice vdev; >>> + uint64_t io_region_size; >>> + uint64_t io_region_offset; >>> + struct ccw_io_region *io_region; >>> + EventNotifier io_notifier; >>> + bool force_orb_pfch; >>> + bool warned_orb_pfch; >>> +} VFIOCCWDevice; >> >> Do you really need to make the whole structure public here? If not, I >> think it would be sufficient to only have the "anonymous" typedef here: >> >> typedef struct VFIOCCWDevice VFIOCCWDevice; >> >> Thomas > Perhaps the entire struct is not needed in ipl.c, and we could get by > with only the typedef. But then the only thing in vfio-ccw.h will be the > one line. Seems a little confusing to me. What do we gain by doing this? That's the coding principle of "information hiding". Don't expose internal data to other parts of the code if it is not really necessary. I'm fine with making the structure public here if it is really necessary, but as far as I can see until now, it's not, so there is also no need to expose those internal information to other parts of the code. Thomas
On 2/4/19 5:26 AM, Cornelia Huck wrote: > On Tue, 29 Jan 2019 08:29:08 -0500 > "Jason J. Herne" <jjherne@linux.ibm.com> wrote: > >> Add bootindex property and iplb data for vfio-ccw devices. This allows us to >> forward boot information into the bios for vfio-ccw devices. >> >> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com> >> Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> hw/s390x/ipl.c | 14 ++++++++++++++ >> hw/s390x/s390-ccw.c | 9 +++++++++ >> hw/vfio/ccw.c | 13 +------------ >> include/hw/s390x/s390-ccw.h | 1 + >> include/hw/s390x/vfio-ccw.h | 38 ++++++++++++++++++++++++++++++++++++++ >> 5 files changed, 63 insertions(+), 12 deletions(-) >> create mode 100644 include/hw/s390x/vfio-ccw.h >> >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >> index 21f64ad..a993f65 100644 >> --- a/hw/s390x/ipl.c >> +++ b/hw/s390x/ipl.c >> @@ -19,6 +19,7 @@ >> #include "hw/loader.h" >> #include "hw/boards.h" >> #include "hw/s390x/virtio-ccw.h" >> +#include "hw/s390x/vfio-ccw.h" >> #include "hw/s390x/css.h" >> #include "hw/s390x/ebcdic.h" >> #include "ipl.h" >> @@ -311,8 +312,12 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st) >> VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *) >> object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent), >> TYPE_VIRTIO_CCW_DEVICE); >> + VFIOCCWDevice *vfio_ccw_dev = (VFIOCCWDevice *) >> + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); >> if (virtio_ccw_dev) { >> ccw_dev = CCW_DEVICE(virtio_ccw_dev); >> + } else if (vfio_ccw_dev) { >> + ccw_dev = CCW_DEVICE(vfio_ccw_dev); >> } else { >> SCSIDevice *sd = (SCSIDevice *) >> object_dynamic_cast(OBJECT(dev_st), >> @@ -347,6 +352,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) >> if (ccw_dev) { >> SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st), >> TYPE_SCSI_DEVICE); >> + VFIOCCWDevice *vc = (VFIOCCWDevice *) >> + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); >> >> if (sd) { >> ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN); >> @@ -358,6 +365,13 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) >> ipl->iplb.scsi.channel = cpu_to_be16(sd->channel); >> ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno); >> ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3; >> + } else if (vc) { >> + CcwDevice *ccw_dev = CCW_DEVICE(vc); >> + >> + ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN); >> + ipl->iplb.pbt = S390_IPL_TYPE_CCW; >> + ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno); >> + ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3; >> } else { >> VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st), >> TYPE_VIRTIO_NET); > > Hm, I think that this find-out-the-boot-type-and-set-up-the-right-thing > mechanism is getting a bit unwieldy. Basically, we > > - call s390_get_ccw_device() to find out the device type via a bunch of > casts and return a pointer to a CcwDevice if it's a supported type > - do a bunch of casts here *again* to find out what we have and fill > out the iplb, while we really only need to do grab a non-CcwDevice > for the scsi case > > Should maybe s390_get_ccw_device() give us an ipl type in addition to > the pointer to the CcwDevice, so we can use a switch/case statement to > fill out the iplb here? I think this idea makes sense. s390_ipl_reset_request also calls s390_get_ccw_device but does not care bout the device type, so how about a separate function instead of integrating it with s390_get_ccw_device? Maybe s390_get_ccw_device_type? Is there any easy way to grab the type or are we just hiding the ugly casting inside our helper function?
On Wed, 13 Feb 2019 08:41:43 -0500 "Jason J. Herne" <jjherne@linux.ibm.com> wrote: > On 2/4/19 5:26 AM, Cornelia Huck wrote: > > On Tue, 29 Jan 2019 08:29:08 -0500 > > "Jason J. Herne" <jjherne@linux.ibm.com> wrote: > >> @@ -311,8 +312,12 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st) > >> VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *) > >> object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent), > >> TYPE_VIRTIO_CCW_DEVICE); > >> + VFIOCCWDevice *vfio_ccw_dev = (VFIOCCWDevice *) > >> + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); > >> if (virtio_ccw_dev) { > >> ccw_dev = CCW_DEVICE(virtio_ccw_dev); > >> + } else if (vfio_ccw_dev) { > >> + ccw_dev = CCW_DEVICE(vfio_ccw_dev); > >> } else { > >> SCSIDevice *sd = (SCSIDevice *) > >> object_dynamic_cast(OBJECT(dev_st), > >> @@ -347,6 +352,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) > >> if (ccw_dev) { > >> SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st), > >> TYPE_SCSI_DEVICE); > >> + VFIOCCWDevice *vc = (VFIOCCWDevice *) > >> + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); > >> > >> if (sd) { > >> ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN); > >> @@ -358,6 +365,13 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) > >> ipl->iplb.scsi.channel = cpu_to_be16(sd->channel); > >> ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno); > >> ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3; > >> + } else if (vc) { > >> + CcwDevice *ccw_dev = CCW_DEVICE(vc); > >> + > >> + ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN); > >> + ipl->iplb.pbt = S390_IPL_TYPE_CCW; > >> + ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno); > >> + ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3; > >> } else { > >> VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st), > >> TYPE_VIRTIO_NET); > > > > Hm, I think that this find-out-the-boot-type-and-set-up-the-right-thing > > mechanism is getting a bit unwieldy. Basically, we > > > > - call s390_get_ccw_device() to find out the device type via a bunch of > > casts and return a pointer to a CcwDevice if it's a supported type > > - do a bunch of casts here *again* to find out what we have and fill > > out the iplb, while we really only need to do grab a non-CcwDevice > > for the scsi case > > > > Should maybe s390_get_ccw_device() give us an ipl type in addition to > > the pointer to the CcwDevice, so we can use a switch/case statement to > > fill out the iplb here? > > I think this idea makes sense. s390_ipl_reset_request also calls s390_get_ccw_device but > does not care bout the device type, so how about a separate function instead of > integrating it with s390_get_ccw_device? Maybe s390_get_ccw_device_type? If I understand it correctly, we always want to be able to grab a CcwDevice if supported and for generating the iplb, we also need to know the actual type of the device. We basically need to follow the same procedure for both; so it probably makes most sense to have one function that provides both (the reset code can simply disregard the type). > Is there any easy way to grab the type or are we just hiding the ugly casting inside our > helper function? I'm not aware of an alternative to the casting, so the helper function would just hide the ugliness, I guess...
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index 21f64ad..a993f65 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -19,6 +19,7 @@ #include "hw/loader.h" #include "hw/boards.h" #include "hw/s390x/virtio-ccw.h" +#include "hw/s390x/vfio-ccw.h" #include "hw/s390x/css.h" #include "hw/s390x/ebcdic.h" #include "ipl.h" @@ -311,8 +312,12 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st) VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *) object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent), TYPE_VIRTIO_CCW_DEVICE); + VFIOCCWDevice *vfio_ccw_dev = (VFIOCCWDevice *) + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); if (virtio_ccw_dev) { ccw_dev = CCW_DEVICE(virtio_ccw_dev); + } else if (vfio_ccw_dev) { + ccw_dev = CCW_DEVICE(vfio_ccw_dev); } else { SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st), @@ -347,6 +352,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) if (ccw_dev) { SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st), TYPE_SCSI_DEVICE); + VFIOCCWDevice *vc = (VFIOCCWDevice *) + object_dynamic_cast(OBJECT(dev_st), TYPE_VFIO_CCW); if (sd) { ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN); @@ -358,6 +365,13 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) ipl->iplb.scsi.channel = cpu_to_be16(sd->channel); ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno); ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3; + } else if (vc) { + CcwDevice *ccw_dev = CCW_DEVICE(vc); + + ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN); + ipl->iplb.pbt = S390_IPL_TYPE_CCW; + ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno); + ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3; } else { VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st), TYPE_VIRTIO_NET); diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c index cad91ee..f5f025d 100644 --- a/hw/s390x/s390-ccw.c +++ b/hw/s390x/s390-ccw.c @@ -124,6 +124,14 @@ static void s390_ccw_unrealize(S390CCWDevice *cdev, Error **errp) g_free(cdev->mdevid); } +static void s390_ccw_instance_init(Object *obj) +{ + S390CCWDevice *dev = S390_CCW_DEVICE(obj); + + device_add_bootindex_property(obj, &dev->bootindex, "bootindex", + "/disk@0,0", DEVICE(obj), NULL); +} + static void s390_ccw_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -137,6 +145,7 @@ static void s390_ccw_class_init(ObjectClass *klass, void *data) static const TypeInfo s390_ccw_info = { .name = TYPE_S390_CCW, .parent = TYPE_CCW_DEVICE, + .instance_init = s390_ccw_instance_init, .instance_size = sizeof(S390CCWDevice), .class_size = sizeof(S390CCWDeviceClass), .class_init = s390_ccw_class_init, diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 9246729..d815a4f 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -21,22 +21,11 @@ #include "hw/vfio/vfio.h" #include "hw/vfio/vfio-common.h" #include "hw/s390x/s390-ccw.h" +#include "hw/s390x/vfio-ccw.h" #include "hw/s390x/ccw-device.h" #include "exec/address-spaces.h" #include "qemu/error-report.h" -#define TYPE_VFIO_CCW "vfio-ccw" -typedef struct VFIOCCWDevice { - S390CCWDevice cdev; - VFIODevice vdev; - uint64_t io_region_size; - uint64_t io_region_offset; - struct ccw_io_region *io_region; - EventNotifier io_notifier; - bool force_orb_pfch; - bool warned_orb_pfch; -} VFIOCCWDevice; - static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch, const char *msg) { diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h index 7d15a1a..901d805 100644 --- a/include/hw/s390x/s390-ccw.h +++ b/include/hw/s390x/s390-ccw.h @@ -27,6 +27,7 @@ typedef struct S390CCWDevice { CcwDevice parent_obj; CssDevId hostid; char *mdevid; + int32_t bootindex; } S390CCWDevice; typedef struct S390CCWDeviceClass { diff --git a/include/hw/s390x/vfio-ccw.h b/include/hw/s390x/vfio-ccw.h new file mode 100644 index 0000000..a7d699d --- /dev/null +++ b/include/hw/s390x/vfio-ccw.h @@ -0,0 +1,38 @@ +/* + * vfio based subchannel assignment support + * + * Copyright 2018 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. + */ + +#ifndef HW_VFIO_CCW_H +#define HW_VFIO_CCW_H + +#include "hw/vfio/vfio-common.h" +#include "hw/s390x/s390-ccw.h" +#include "hw/s390x/ccw-device.h" + +#define TYPE_VFIO_CCW "vfio-ccw" +#define VFIO_CCW(obj) \ + OBJECT_CHECK(VFIOCCWDevice, (obj), TYPE_VFIO_CCW) + + +#define TYPE_VFIO_CCW "vfio-ccw" +typedef struct VFIOCCWDevice { + S390CCWDevice cdev; + VFIODevice vdev; + uint64_t io_region_size; + uint64_t io_region_offset; + struct ccw_io_region *io_region; + EventNotifier io_notifier; + bool force_orb_pfch; + bool warned_orb_pfch; +} VFIOCCWDevice; + +#endif