On 11/14/24 7:39 AM, Thomas Huth wrote:
On 13/11/2024 15.49, Jared Rossi wrote:
...
Hi Thomas,
Thanks for putting this fix together. As we previously discussed, I
do agree
that my naive implementation of the “loadparm” property at the top-level
CcwDevice was not satisfactory, and certainly virtio-gpu and
virtio-tablet
should not have a “loadparm.”
The reason I had not yet submitted a fix is that I’ve gotten some
feedback from
the Libvirt side that suggests the CcwDevice implementation is not
sufficient
in general. Libvirt will require that non-ccw devices (e.g. scsi-hd)
also
support per-device loadparm. I do not yet know how to add that type
of support
and given that we are in hard freeze I’m not sure it is possible now.
Obviously this is not ideal, and I truly do apologize for the confusion.
Hi Jared,
yes, that scsi-hd problem is a little bit tricky, since it's common
code that we should not "pollute" too much with s390x specific stuff,
especially since we're in hard freeze now.
After staring at the code for quite a while, I think one option might
be to just add a string "loadparm" property to the SCSI devices,
that's just a simple two-line change to the common code. Patch
suggestion can be found here:
https://lore.kernel.org/qemu-devel/[email protected]/
The only disadvantage is that this is now checking for valid
characters in the string after the property has already been set, so
it cannot prevent the setting of bad characters. But it still prints
out an error message later, so I hope that is also still acceptable.
Let me know what you think about it!
Thanks,
Thomas
Thanks Thomas,
I replied to the patch as well, but the setter does other validation
beyond just the characters. In particular it ensures a loadparm value
is assigned only to boot devices. This has significance when probing
for devices, since the per-device loadparm value is stored in the IPLB,
and an IPLB is only generated for devices with an assigned boot index.
I would say the scsi loadparm probing issue is actually a legitimate
concern since it can result in unexpected behavior with which kernel is
selected, but probably we want to avoid adding too much s390x specific
validation to the common code, so I'm not sure what the best solution
is. Maybe there is a better way to restrict the loadparm to boot
devices that doesn't need the property setter?
On a side note, while testing your patch I also realized that the
machine loadparm was being ignored during probing, so I've sent out a
patch to fix that...
Thanks again for all the help with this,
Jared Rossi
