Re: [Qemu-devel] [PATCH v3 01/16] s390 vfio-ccw: Add bootindex property and IPLB data
On Wed, 6 Mar 2019 11:28:37 -0500
"Jason J. Herne" wrote:
> On 3/6/19 10:27 AM, Cornelia Huck wrote:
> > On Wed, 6 Mar 2019 09:55:40 -0500
> > "Jason J. Herne" wrote:
> >
> >> On 3/4/19 8:40 AM, Cornelia Huck wrote:
> >>> On Fri, 1 Mar 2019 13:59:21 -0500
> >>> "Jason J. Herne" wrote:
> >
> @@ -532,7 +559,7 @@ void s390_ipl_reset_request(CPUState *cs, enum
> s390_reset reset_type)
> !ipl->netboot &&
> ipl->iplb.pbt == S390_IPL_TYPE_CCW &&
> is_virtio_scsi_device(>iplb)) {
> -CcwDevice *ccw_dev = s390_get_ccw_device(get_boot_device(0));
> +CcwDevice *ccw_dev = s390_get_ccw_device(get_boot_device(0),
> );
> >>>
> >>> It feels wrong to have a variable that is not used later... what about
> >>> either
> >>> - making s390_get_ccw_device() capable of handling a NULL second
> >>> parameter, or
> >>> - (what I think would be nicer) passing in the devtype as an optional
> >>> parameter to gen_initial_iplb() so you don't need to do the casting
> >>> dance twice?
> >>>
> >>
> >> I'm with you on everything suggested for this patch except this last item.
> >> I'm not sure
> >> what you are suggesting here. I can easily visualize passing NULL for
> >> devtype when we want
> >> to ignore it but I'm not sure what you mean by 'optional parameter'
> >
> > Hm, actually you'd need the device as well :) Basically,
> >
> > static bool s390_gen_initial_iplb(S390IPLState *ipl, CcwDevice *ccw_dev,
> > int devtype)
> > {
> > (...)
> > if (!ccw_dev) {
> > //get ccw_dev, which also gives us the devtype
> > }
> >
> > if (ccw_dev) {
> > //as before; devtype is valid here
> > (...)
> > return true;
> > }
> > return false;
> > }
> >
> > So you can call it with s390_gen_initial_iplb(ipl, NULL, 0) if you
> > don't have the values already.
> >
> >
> Ahh, makes sense now. Thanks for clarifying. I can do it that way, but it
> does seem a bit
> redundant to allow s390_gen_initial_iplb to be called either with, or without
> the device
> type data. In the case where it is called without, it just has to make the
> call to
> s390_get_ccw_device anyway. So, to me, it seems like added lines of code for
> very little
> benefit. Why not either always call s390_get_ccw_device from inside
> s390_gen_initial_iplb,
> or always require the parameters to be valid?
>
My main goal was to avoid needing to do the casting twice. If that
makes the code too ugly, we can also go back to making
s390_get_ccw_device accept a NULL devtype (which I'd find less ugly
than needing to pass in an unused variable.)
Re: [Qemu-devel] [PATCH v3 01/16] s390 vfio-ccw: Add bootindex property and IPLB data
On 3/6/19 10:27 AM, Cornelia Huck wrote:
On Wed, 6 Mar 2019 09:55:40 -0500
"Jason J. Herne" wrote:
On 3/4/19 8:40 AM, Cornelia Huck wrote:
On Fri, 1 Mar 2019 13:59:21 -0500
"Jason J. Herne" wrote:
@@ -532,7 +559,7 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset
reset_type)
!ipl->netboot &&
ipl->iplb.pbt == S390_IPL_TYPE_CCW &&
is_virtio_scsi_device(>iplb)) {
-CcwDevice *ccw_dev = s390_get_ccw_device(get_boot_device(0));
+CcwDevice *ccw_dev = s390_get_ccw_device(get_boot_device(0), );
It feels wrong to have a variable that is not used later... what about
either
- making s390_get_ccw_device() capable of handling a NULL second
parameter, or
- (what I think would be nicer) passing in the devtype as an optional
parameter to gen_initial_iplb() so you don't need to do the casting
dance twice?
I'm with you on everything suggested for this patch except this last item. I'm
not sure
what you are suggesting here. I can easily visualize passing NULL for devtype
when we want
to ignore it but I'm not sure what you mean by 'optional parameter'
Hm, actually you'd need the device as well :) Basically,
static bool s390_gen_initial_iplb(S390IPLState *ipl, CcwDevice *ccw_dev, int
devtype)
{
(...)
if (!ccw_dev) {
//get ccw_dev, which also gives us the devtype
}
if (ccw_dev) {
//as before; devtype is valid here
(...)
return true;
}
return false;
}
So you can call it with s390_gen_initial_iplb(ipl, NULL, 0) if you
don't have the values already.
Ahh, makes sense now. Thanks for clarifying. I can do it that way, but it does seem a bit
redundant to allow s390_gen_initial_iplb to be called either with, or without the device
type data. In the case where it is called without, it just has to make the call to
s390_get_ccw_device anyway. So, to me, it seems like added lines of code for very little
benefit. Why not either always call s390_get_ccw_device from inside s390_gen_initial_iplb,
or always require the parameters to be valid?
--
-- Jason J. Herne (jjhe...@linux.ibm.com)
Re: [Qemu-devel] [PATCH v3 01/16] s390 vfio-ccw: Add bootindex property and IPLB data
On Wed, 6 Mar 2019 09:55:40 -0500
"Jason J. Herne" wrote:
> On 3/4/19 8:40 AM, Cornelia Huck wrote:
> > On Fri, 1 Mar 2019 13:59:21 -0500
> > "Jason J. Herne" wrote:
> >> @@ -532,7 +559,7 @@ void s390_ipl_reset_request(CPUState *cs, enum
> >> s390_reset reset_type)
> >> !ipl->netboot &&
> >> ipl->iplb.pbt == S390_IPL_TYPE_CCW &&
> >> is_virtio_scsi_device(>iplb)) {
> >> -CcwDevice *ccw_dev = s390_get_ccw_device(get_boot_device(0));
> >> +CcwDevice *ccw_dev = s390_get_ccw_device(get_boot_device(0),
> >> );
> >
> > It feels wrong to have a variable that is not used later... what about
> > either
> > - making s390_get_ccw_device() capable of handling a NULL second
> >parameter, or
> > - (what I think would be nicer) passing in the devtype as an optional
> >parameter to gen_initial_iplb() so you don't need to do the casting
> >dance twice?
> >
>
> I'm with you on everything suggested for this patch except this last item.
> I'm not sure
> what you are suggesting here. I can easily visualize passing NULL for devtype
> when we want
> to ignore it but I'm not sure what you mean by 'optional parameter'
Hm, actually you'd need the device as well :) Basically,
static bool s390_gen_initial_iplb(S390IPLState *ipl, CcwDevice *ccw_dev, int
devtype)
{
(...)
if (!ccw_dev) {
//get ccw_dev, which also gives us the devtype
}
if (ccw_dev) {
//as before; devtype is valid here
(...)
return true;
}
return false;
}
So you can call it with s390_gen_initial_iplb(ipl, NULL, 0) if you
don't have the values already.
Re: [Qemu-devel] [PATCH v3 01/16] s390 vfio-ccw: Add bootindex property and IPLB data
On 3/4/19 11:09 AM, Farhan Ali wrote:
On 03/01/2019 01:59 PM, 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.
Refactor s390_get_ccw_device() to return device type. This prevents us from
having to use messy casting logic in several places.
Signed-off-by: Jason J. Herne
Acked-by: Halil Pasic
---
MAINTAINERS | 1 +
hw/s390x/ipl.c | 39 +--
hw/s390x/s390-ccw.c | 9 +
hw/vfio/ccw.c | 2 +-
include/hw/s390x/s390-ccw.h | 1 +
include/hw/s390x/vfio-ccw.h | 28
6 files changed, 73 insertions(+), 7 deletions(-)
create mode 100644 include/hw/s390x/vfio-ccw.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 9a76845..a780916 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1400,6 +1400,7 @@ S: Supported
F: hw/vfio/ccw.c
F: hw/s390x/s390-ccw.c
F: include/hw/s390x/s390-ccw.h
+F: include/hw/s390x/vfio-ccw.h
T: githttps://github.com/cohuck/qemu.git s390-next
L:qemu-s3...@nongnu.org
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 896888b..df891bb 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"
@@ -305,16 +306,29 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
*timeout = cpu_to_be32(splash_time);
}
-static CcwDevice *s390_get_ccw_device(DeviceState *dev_st)
+#define CCW_DEVTYPE_NONE 0x00
+#define CCW_DEVTYPE_VIRTIO 0x01
+#define CCW_DEVTYPE_SCSI 0x02
+#define CCW_DEVTYPE_VFIO 0x03
+
+static CcwDevice*s390_get_ccw_device(DeviceState *dev_st, int* devtype)
{
CcwDevice *ccw_dev = NULL;
+ *devtype = CCW_DEVTYPE_NONE;
+
if (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);
+ *devtype = CCW_DEVTYPE_VIRTIO;
+ } else if (vfio_ccw_dev) {
+ ccw_dev = CCW_DEVICE(vfio_ccw_dev);
+ *devtype = CCW_DEVTYPE_VFIO;
} else {
SCSIDevice *sd = (SCSIDevice *)
object_dynamic_cast(OBJECT(dev_st),
@@ -327,6 +341,7 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st)
ccw_dev = (CcwDevice *)object_dynamic_cast(OBJECT(scsi_ccw),
TYPE_CCW_DEVICE);
+ *devtype = CCW_DEVTYPE_SCSI;
}
}
}
@@ -337,10 +352,11 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
{
DeviceState *dev_st;
CcwDevice *ccw_dev = NULL;
+ int devtype;
dev_st = get_boot_device(0);
if (dev_st) {
- ccw_dev = s390_get_ccw_device(dev_st);
+ ccw_dev = s390_get_ccw_device(dev_st, );
}
/*
@@ -349,8 +365,10 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
if (ccw_dev) {
SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st),
TYPE_SCSI_DEVICE);
+ VirtIONet *vn;
- if (sd) {
+ switch (devtype) {
+ case CCW_DEVTYPE_SCSI:
ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN);
ipl->iplb.blk0_len =
cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN -
S390_IPLB_HEADER_LEN);
@@ -360,8 +378,15 @@ 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 {
- VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st),
+ break;
+ case CCW_DEVTYPE_VFIO:
+ 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;
+ break;
Also for vfio-ccw we are not setting the ipl->iplb.blk0_len like we do for virtio. Is
there a reason?
I know we don't reference the value in the bios for both virtio and vfio.
No reason other than I saw no reason to set it. If you know of any reasons please let me
know.
--
-- Jason J. Herne (jjhe...@linux.ibm.com)
Re: [Qemu-devel] [PATCH v3 01/16] s390 vfio-ccw: Add bootindex property and IPLB data
On 3/4/19 8:40 AM, Cornelia Huck wrote:
On Fri, 1 Mar 2019 13:59:21 -0500
"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.
Refactor s390_get_ccw_device() to return device type. This prevents us from
having to use messy casting logic in several places.
Signed-off-by: Jason J. Herne
Acked-by: Halil Pasic
---
MAINTAINERS | 1 +
hw/s390x/ipl.c | 39 +--
hw/s390x/s390-ccw.c | 9 +
hw/vfio/ccw.c | 2 +-
include/hw/s390x/s390-ccw.h | 1 +
include/hw/s390x/vfio-ccw.h | 28
6 files changed, 73 insertions(+), 7 deletions(-)
create mode 100644 include/hw/s390x/vfio-ccw.h
(...)
@@ -305,16 +306,29 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
*timeout = cpu_to_be32(splash_time);
}
-static CcwDevice *s390_get_ccw_device(DeviceState *dev_st)
+#define CCW_DEVTYPE_NONE0x00
+#define CCW_DEVTYPE_VIRTIO 0x01
+#define CCW_DEVTYPE_SCSI0x02
+#define CCW_DEVTYPE_VFIO0x03
Hm, how would the code look if you introduced a CCW_DEVTYPE_VIRTIO_NET
or so? You could use the simply set the netboot flag in
get_initial_iplb and fall through to the handling of
CCW_DEVTYPE_VIRTIO...
+
+static CcwDevice *s390_get_ccw_device(DeviceState *dev_st, int* devtype)
s/int* devtype/int *devtype/
{
CcwDevice *ccw_dev = NULL;
+*devtype = CCW_DEVTYPE_NONE;
+
if (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);
+*devtype = CCW_DEVTYPE_VIRTIO;
+} else if (vfio_ccw_dev) {
+ccw_dev = CCW_DEVICE(vfio_ccw_dev);
+*devtype = CCW_DEVTYPE_VFIO;
} else {
SCSIDevice *sd = (SCSIDevice *)
object_dynamic_cast(OBJECT(dev_st),
@@ -327,6 +341,7 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st)
ccw_dev = (CcwDevice *)object_dynamic_cast(OBJECT(scsi_ccw),
TYPE_CCW_DEVICE);
+*devtype = CCW_DEVTYPE_SCSI;
}
}
}
@@ -337,10 +352,11 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
{
DeviceState *dev_st;
CcwDevice *ccw_dev = NULL;
+int devtype;
dev_st = get_boot_device(0);
if (dev_st) {
-ccw_dev = s390_get_ccw_device(dev_st);
+ccw_dev = s390_get_ccw_device(dev_st, );
}
/*
@@ -349,8 +365,10 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
if (ccw_dev) {
SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st),
TYPE_SCSI_DEVICE);
+VirtIONet *vn;
I think you could get rid of this variable with my suggestion from
above.
-if (sd) {
+switch (devtype) {
+case CCW_DEVTYPE_SCSI:
Move obtaining sd into this case?
ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN);
ipl->iplb.blk0_len =
cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN -
S390_IPLB_HEADER_LEN);
@@ -360,8 +378,15 @@ 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 {
-VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st),
+break;
+case CCW_DEVTYPE_VFIO:
+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;
+break;
+case CCW_DEVTYPE_VIRTIO:
+vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st),
TYPE_VIRTIO_NET);
ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
@@ -374,6 +399,7 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
if (vn) {
ipl->netboot = true;
}
+break;
}
if (!s390_ipl_set_loadparm(ipl->iplb.loadparm)) {
@@ -518,6 +544,7 @@ IplParameterBlock *s390_ipl_get_iplb(void)
void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
{
S390IPLState *ipl = get_ipl_device();
+int devtype;
if
Re: [Qemu-devel] [PATCH v3 01/16] s390 vfio-ccw: Add bootindex property and IPLB data
On 03/01/2019 01:59 PM, 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.
Refactor s390_get_ccw_device() to return device type. This prevents us from
having to use messy casting logic in several places.
Signed-off-by: Jason J. Herne
Acked-by: Halil Pasic
---
MAINTAINERS | 1 +
hw/s390x/ipl.c | 39 +--
hw/s390x/s390-ccw.c | 9 +
hw/vfio/ccw.c | 2 +-
include/hw/s390x/s390-ccw.h | 1 +
include/hw/s390x/vfio-ccw.h | 28
6 files changed, 73 insertions(+), 7 deletions(-)
create mode 100644 include/hw/s390x/vfio-ccw.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 9a76845..a780916 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1400,6 +1400,7 @@ S: Supported
F: hw/vfio/ccw.c
F: hw/s390x/s390-ccw.c
F: include/hw/s390x/s390-ccw.h
+F: include/hw/s390x/vfio-ccw.h
T: githttps://github.com/cohuck/qemu.git s390-next
L:qemu-s3...@nongnu.org
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 896888b..df891bb 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"
@@ -305,16 +306,29 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
*timeout = cpu_to_be32(splash_time);
}
-static CcwDevice *s390_get_ccw_device(DeviceState *dev_st)
+#define CCW_DEVTYPE_NONE0x00
+#define CCW_DEVTYPE_VIRTIO 0x01
+#define CCW_DEVTYPE_SCSI0x02
+#define CCW_DEVTYPE_VFIO0x03
+
+static CcwDevice*s390_get_ccw_device(DeviceState *dev_st, int* devtype)
{
CcwDevice *ccw_dev = NULL;
+*devtype = CCW_DEVTYPE_NONE;
+
if (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);
+*devtype = CCW_DEVTYPE_VIRTIO;
+} else if (vfio_ccw_dev) {
+ccw_dev = CCW_DEVICE(vfio_ccw_dev);
+*devtype = CCW_DEVTYPE_VFIO;
} else {
SCSIDevice *sd = (SCSIDevice *)
object_dynamic_cast(OBJECT(dev_st),
@@ -327,6 +341,7 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st)
ccw_dev = (CcwDevice *)object_dynamic_cast(OBJECT(scsi_ccw),
TYPE_CCW_DEVICE);
+*devtype = CCW_DEVTYPE_SCSI;
}
}
}
@@ -337,10 +352,11 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
{
DeviceState *dev_st;
CcwDevice *ccw_dev = NULL;
+int devtype;
dev_st = get_boot_device(0);
if (dev_st) {
-ccw_dev = s390_get_ccw_device(dev_st);
+ccw_dev = s390_get_ccw_device(dev_st, );
}
/*
@@ -349,8 +365,10 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
if (ccw_dev) {
SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st),
TYPE_SCSI_DEVICE);
+VirtIONet *vn;
-if (sd) {
+switch (devtype) {
+case CCW_DEVTYPE_SCSI:
ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN);
ipl->iplb.blk0_len =
cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN -
S390_IPLB_HEADER_LEN);
@@ -360,8 +378,15 @@ 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 {
-VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st),
+break;
+case CCW_DEVTYPE_VFIO:
+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;
+break;
Also for vfio-ccw we are not setting the ipl->iplb.blk0_len like we do
for virtio. Is there a reason?
I know we don't reference the value in the bios for both virtio and vfio.
+case CCW_DEVTYPE_VIRTIO:
+vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st),
TYPE_VIRTIO_NET);
ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
@@ -374,6 +399,7 @@ static bool
Re: [Qemu-devel] [PATCH v3 01/16] s390 vfio-ccw: Add bootindex property and IPLB data
On Fri, 1 Mar 2019 13:59:21 -0500
"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.
>
> Refactor s390_get_ccw_device() to return device type. This prevents us from
> having to use messy casting logic in several places.
>
> Signed-off-by: Jason J. Herne
> Acked-by: Halil Pasic
> ---
> MAINTAINERS | 1 +
> hw/s390x/ipl.c | 39 +--
> hw/s390x/s390-ccw.c | 9 +
> hw/vfio/ccw.c | 2 +-
> include/hw/s390x/s390-ccw.h | 1 +
> include/hw/s390x/vfio-ccw.h | 28
> 6 files changed, 73 insertions(+), 7 deletions(-)
> create mode 100644 include/hw/s390x/vfio-ccw.h
>
(...)
> @@ -305,16 +306,29 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
> *timeout = cpu_to_be32(splash_time);
> }
>
> -static CcwDevice *s390_get_ccw_device(DeviceState *dev_st)
> +#define CCW_DEVTYPE_NONE0x00
> +#define CCW_DEVTYPE_VIRTIO 0x01
> +#define CCW_DEVTYPE_SCSI0x02
> +#define CCW_DEVTYPE_VFIO0x03
Hm, how would the code look if you introduced a CCW_DEVTYPE_VIRTIO_NET
or so? You could use the simply set the netboot flag in
get_initial_iplb and fall through to the handling of
CCW_DEVTYPE_VIRTIO...
> +
> +static CcwDevice *s390_get_ccw_device(DeviceState *dev_st, int* devtype)
s/int* devtype/int *devtype/
> {
> CcwDevice *ccw_dev = NULL;
>
> +*devtype = CCW_DEVTYPE_NONE;
> +
> if (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);
> +*devtype = CCW_DEVTYPE_VIRTIO;
> +} else if (vfio_ccw_dev) {
> +ccw_dev = CCW_DEVICE(vfio_ccw_dev);
> +*devtype = CCW_DEVTYPE_VFIO;
> } else {
> SCSIDevice *sd = (SCSIDevice *)
> object_dynamic_cast(OBJECT(dev_st),
> @@ -327,6 +341,7 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st)
>
> ccw_dev = (CcwDevice *)object_dynamic_cast(OBJECT(scsi_ccw),
> TYPE_CCW_DEVICE);
> +*devtype = CCW_DEVTYPE_SCSI;
> }
> }
> }
> @@ -337,10 +352,11 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
> {
> DeviceState *dev_st;
> CcwDevice *ccw_dev = NULL;
> +int devtype;
>
> dev_st = get_boot_device(0);
> if (dev_st) {
> -ccw_dev = s390_get_ccw_device(dev_st);
> +ccw_dev = s390_get_ccw_device(dev_st, );
> }
>
> /*
> @@ -349,8 +365,10 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
> if (ccw_dev) {
> SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st),
>
> TYPE_SCSI_DEVICE);
> +VirtIONet *vn;
I think you could get rid of this variable with my suggestion from
above.
>
> -if (sd) {
> +switch (devtype) {
> +case CCW_DEVTYPE_SCSI:
Move obtaining sd into this case?
> ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN);
> ipl->iplb.blk0_len =
> cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN -
> S390_IPLB_HEADER_LEN);
> @@ -360,8 +378,15 @@ 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 {
> -VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st),
> +break;
> +case CCW_DEVTYPE_VFIO:
> +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;
> +break;
> +case CCW_DEVTYPE_VIRTIO:
> +vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st),
>
> TYPE_VIRTIO_NET);
>
> ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
> @@ -374,6 +399,7 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
> if (vn) {
> ipl->netboot = true;
> }
> +break;
> }
>
> if (!s390_ipl_set_loadparm(ipl->iplb.loadparm)) {
> @@ -518,6 +544,7 @@ IplParameterBlock *s390_ipl_get_iplb(void)
> void s390_ipl_reset_request(CPUState *cs, enum
[Qemu-devel] [PATCH v3 01/16] s390 vfio-ccw: Add bootindex property and IPLB data
Add bootindex property and iplb data for vfio-ccw devices. This allows us to
forward boot information into the bios for vfio-ccw devices.
Refactor s390_get_ccw_device() to return device type. This prevents us from
having to use messy casting logic in several places.
Signed-off-by: Jason J. Herne
Acked-by: Halil Pasic
---
MAINTAINERS | 1 +
hw/s390x/ipl.c | 39 +--
hw/s390x/s390-ccw.c | 9 +
hw/vfio/ccw.c | 2 +-
include/hw/s390x/s390-ccw.h | 1 +
include/hw/s390x/vfio-ccw.h | 28
6 files changed, 73 insertions(+), 7 deletions(-)
create mode 100644 include/hw/s390x/vfio-ccw.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 9a76845..a780916 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1400,6 +1400,7 @@ S: Supported
F: hw/vfio/ccw.c
F: hw/s390x/s390-ccw.c
F: include/hw/s390x/s390-ccw.h
+F: include/hw/s390x/vfio-ccw.h
T: git https://github.com/cohuck/qemu.git s390-next
L: qemu-s3...@nongnu.org
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 896888b..df891bb 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"
@@ -305,16 +306,29 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
*timeout = cpu_to_be32(splash_time);
}
-static CcwDevice *s390_get_ccw_device(DeviceState *dev_st)
+#define CCW_DEVTYPE_NONE0x00
+#define CCW_DEVTYPE_VIRTIO 0x01
+#define CCW_DEVTYPE_SCSI0x02
+#define CCW_DEVTYPE_VFIO0x03
+
+static CcwDevice *s390_get_ccw_device(DeviceState *dev_st, int* devtype)
{
CcwDevice *ccw_dev = NULL;
+*devtype = CCW_DEVTYPE_NONE;
+
if (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);
+*devtype = CCW_DEVTYPE_VIRTIO;
+} else if (vfio_ccw_dev) {
+ccw_dev = CCW_DEVICE(vfio_ccw_dev);
+*devtype = CCW_DEVTYPE_VFIO;
} else {
SCSIDevice *sd = (SCSIDevice *)
object_dynamic_cast(OBJECT(dev_st),
@@ -327,6 +341,7 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st)
ccw_dev = (CcwDevice *)object_dynamic_cast(OBJECT(scsi_ccw),
TYPE_CCW_DEVICE);
+*devtype = CCW_DEVTYPE_SCSI;
}
}
}
@@ -337,10 +352,11 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
{
DeviceState *dev_st;
CcwDevice *ccw_dev = NULL;
+int devtype;
dev_st = get_boot_device(0);
if (dev_st) {
-ccw_dev = s390_get_ccw_device(dev_st);
+ccw_dev = s390_get_ccw_device(dev_st, );
}
/*
@@ -349,8 +365,10 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
if (ccw_dev) {
SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st),
TYPE_SCSI_DEVICE);
+VirtIONet *vn;
-if (sd) {
+switch (devtype) {
+case CCW_DEVTYPE_SCSI:
ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN);
ipl->iplb.blk0_len =
cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN -
S390_IPLB_HEADER_LEN);
@@ -360,8 +378,15 @@ 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 {
-VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st),
+break;
+case CCW_DEVTYPE_VFIO:
+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;
+break;
+case CCW_DEVTYPE_VIRTIO:
+vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st),
TYPE_VIRTIO_NET);
ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
@@ -374,6 +399,7 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
if (vn) {
ipl->netboot = true;
}
+break;
}
if (!s390_ipl_set_loadparm(ipl->iplb.loadparm)) {
@@ -518,6 +544,7 @@ IplParameterBlock *s390_ipl_get_iplb(void)
void s390_ipl_reset_request(CPUState
