Re: [PATCH] hw/s390x: Restrict "loadparm" property to devices that can be used for booting

[Jared Rossi <jrossi@linux.ibm.com>]

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/20241114122919.973930-1-thuth@redhat.com/ 
>
>
> 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