Re: [PATCH] hw: Add "loadparm" property to scsi disk devices for booting on s390x

[Thomas Huth]

On 15/11/2024 12.49, Paolo Bonzini wrote:
> On Fri, Nov 15, 2024 at 7:30 AM Thomas Huth <thuth@redhat.com> wrote:
> > Ok, great! Sounds like we have a passable solution for QEMU 9.2. We still
> > can refine the loadparm handling of the scsi devices in future QEMU
> > releases, but this will now at least solve the present problem that Boris
> > reported from the libvirt side.
>
> Please put in the release notes that in the future loadparm might be
> limited to s390 emulators... Not sure how, but we'll come up with
> something.

Sure, will do!

Alternatively, would you rather prefer something like this for the common
scsi code:

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index c1fa02883d..5c9362fa83 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -32,6 +32,7 @@
 #include "migration/vmstate.h"
 #include "hw/scsi/emulation.h"
 #include "scsi/constants.h"
+#include "sysemu/arch_init.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "hw/block/block.h"
@@ -3136,6 +3137,31 @@ BlockAIOCB *scsi_dma_writev(int64_t offset, QEMUIOVector 
*iov,
     return blk_aio_pwritev(s->qdev.conf.blk, offset, iov, 0, cb, cb_opaque);
 }
 
+static char *scsi_property_get_loadparm(Object *obj, Error **errp)
+{
+    return g_strdup(SCSI_DISK_BASE(obj)->loadparm);
+}
+
+static void scsi_property_set_loadparm(Object *obj, const char *value,
+                                       Error **errp)
+{
+    SCSI_DISK_BASE(obj)->loadparm = g_strdup(value);
+}
+
+static void scsi_property_add_specifics(DeviceClass *dc)
+{
+    ObjectClass *oc = OBJECT_CLASS(dc);
+
+    /* The loadparm property is only supported on s390x */
+    if (arch_type & QEMU_ARCH_S390X) {
+        object_class_property_add_str(oc, "loadparm",
+                                      scsi_property_get_loadparm,
+                                      scsi_property_set_loadparm);
+        object_class_property_set_description(oc, "loadparm",
+                                              "load parameter (s390x only)");
+    }
+}
+
 static void scsi_disk_base_class_initfn(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -3166,7 +3192,6 @@ static const TypeInfo scsi_disk_base_info = {
     DEFINE_PROP_STRING("vendor", SCSIDiskState, vendor),                \
     DEFINE_PROP_STRING("product", SCSIDiskState, product),              \
     DEFINE_PROP_STRING("device_id", SCSIDiskState, device_id),          \
-    DEFINE_PROP_STRING("loadparm", SCSIDiskState, loadparm),            \
     DEFINE_PROP_BOOL("migrate-emulated-scsi-request", SCSIDiskState, 
migrate_emulated_scsi_request, true)
 
 
@@ -3220,6 +3245,8 @@ static void scsi_hd_class_initfn(ObjectClass *klass, void *data)
     dc->desc = "virtual SCSI disk";
     device_class_set_props(dc, scsi_hd_properties);
     dc->vmsd  = &vmstate_scsi_disk_state;
+
+    scsi_property_add_specifics(dc);
 }
 
 static const TypeInfo scsi_hd_info = {
@@ -3260,6 +3287,8 @@ static void scsi_cd_class_initfn(ObjectClass *klass, void 
*data)
     dc->desc = "virtual SCSI CD-ROM";
     device_class_set_props(dc, scsi_cd_properties);
     dc->vmsd  = &vmstate_scsi_disk_state;
+
+    scsi_property_add_specifics(dc);
 }
 
 static const TypeInfo scsi_cd_info = {

?

Using the global arch_type variable is likely also not the
nicest solution, but it at least limits the property to the
s390x target for now...

 Thomas