|
From: | Jared Rossi |
Subject: | Re: [PATCH] hw: Add "loadparm" property to scsi disk devices for booting on s390x |
Date: | Thu, 14 Nov 2024 18:30:51 -0500 |
User-agent: | Mozilla Thunderbird |
On 11/14/24 12:47 PM, Thomas Huth wrote:
On 14/11/2024 16.55, Jared Rossi wrote:On 11/14/24 7:29 AM, Thomas Huth wrote:While adding the new flexible boot order feature on s390x recently, we missed to add the "loadparm" property to the scsi-hd and scsi-cd devices. This property is required on s390x to pass the information to the boot loader about which kernel should be started or whether the boot menu should be shown. But even more serious: The missing property is now causing trouble with the corresponding libvirt patches that assume that the "loadparm" property is either settable for all bootable devices (when the "boot order" feature is implemented in QEMU), or none (meaning the behaviour of older QEMUs that only allowed one "loadparm" at the machine level). To fix this broken situation, let's implement the "loadparm" property for the SCSI devices, too. Signed-off-by: Thomas Huth <thuth@redhat.com> --- NB: Unlike the ccw_device_set_loadparm() logic that we use for CCW devices, I've decided to use a string property for the "loadparm"in the SCSI devices to avoid spoiling the common code with too much s390x logic. So the check for valid characters is now done after theproperty has been set, i.e. we only print out an error instead offorbidding the setting (like we do it with the CCW devices), whichis IMHO still perfectly acceptable. Or are there other opinions?I wasn't able to think of a way to abuse passing invalid characters, but I didfind two additional differences about the string approach:a) it is not possible to override the machine loadparm by assigning an emptystring (loadparm="") to the deviceAgreed, that's a (small) problem. There does not seem to be a way to distinguish between "property has not been set" and "property has been set to a string with zero length" with object_property_get_str() ...I don't think that the inability to pass the empty string is a significantproblem because passing a single space will have the same effect.That sounds like a good work-around, indeed. > b) it is possible to assign a loadparm value to a non-boot device >Assigning a loadparm to a non-boot device generally does nothing, but in the case of device probing (i.e. no boot devices assigned at all), the device withthe loadparm assigned could be selected for IPL, but it will not use theassigned loadparm (because no IPLB was generated for the device). This check is normally handled by ccw_device_set_loadparm(), but I'm not sure if there is a way to do the validation without having a setter function for the property.Hmmm, that could be confusing for the users, indeed. But maybe it would be sufficient to properly document that loadparm is only working for devices with a bootindex?What do you think?By the way, the loadparm section in docs/system/s390x/bootdevices.rst looks like it should get an update, too ... if you have some spare minutes, could you maybe look at it?
Yes, I suppose I would agree that documenting the behavior is sufficient in this case. I will update bootdevices.rst to include per-device loadparm support and also indicate that that per-device loadparm values are only used for devices that have an assigned bootindex.
Thanks, Jared Rossi
[Prev in Thread] | Current Thread | [Next in Thread] |