Re: [PATCH v5 00/19] s390x: Add Full Boot Order Support

[Thomas Huth <thuth@redhat.com>]

On 07/11/2024 21.42, Jared Rossi wrote:
> 
> 
> On 11/6/24 6:10 AM, Thomas Huth wrote:
>> On 05/11/2024 17.42, Jared Rossi wrote:
>>> Hi Thomas, Sebastian,
>>>
>>> It looks like this is simply caused by the "is_cdrom" value only ever 
>>> being set
>>> to true.  I think it is a one-line fix that just makes sure to initialize 
>>> the
>>> value to false each time we try a new device:
>>>
>>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>>> index a4d1c05aac..3fdba0bedc 100644
>>> --- a/pc-bios/s390-ccw/main.c
>>> +++ b/pc-bios/s390-ccw/main.c
>>> @@ -214,6 +214,7 @@ static void boot_setup(void)
>>>   static bool find_boot_device(void)
>>>   {
>>>       VDev *vdev = virtio_get_device();
>>> +    vdev->is_cdrom = false;
>>>       bool found = false;
>>>
>>>       switch (iplb.pbt) {
>>>
>>> I tested it with the two scenarios you mention and with the existing qtests,
>>> and it seems to work correctly now.
>>
>> Agreed, this seems to fix the issue when all devices are properly marked 
>> with bootindex properties. But it still persists in case the BIOS has to 
>> scan for the boot device, e.g.:
>>
>> qemu-system-s390x -accel kvm -m 2G -nographic \
>>  -drive if=none,id=d1,file=/tmp/bad.dat,format=raw,media=cdrom \
>>  -device virtio-scsi -device scsi-cd,drive=d1 \
>>  -drive if=none,id=d2,file=good.qcow2 -device virtio-blk,drive=d2
>>
>> Isn't there a better place to set the is_cdrom variable?
>>
>>  Thomas
>>
> 
> Hi Thomas,
> 
> What I found is that the original issue with clearing the "is_cdrom" value is
> just as easily fixed for both indexed devices and probed devices by moving
> where "is_cdrom" is set to false:
> 
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index a4d1c05aac..7509755e36 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -242,6 +242,7 @@ static bool find_boot_device(void)
>   static int virtio_setup(void)
>   {
>       VDev *vdev = virtio_get_device();
> +    vdev->is_cdrom = false;
>       int ret;

That looks like a good spot, indeed! Could you please send it as a proper 
patch (i.e. with a Signed-off-by line)? I think that would still be a good 
fix for QEMU 9.2.

> Unfortunately when verifying the fix I found another unrelated issue with
> probing, which is that only the first device attached to the scsi controller
> will be found.  This is because virtio_scsi_setup() itself contains a probing
> loop to find a LUN when none is specified, and, unsurprisingly, it returns
> the first thing it finds attached to the controller.  As a result, the main
> device probing loop will move on after trying only one LUN per controller.
> 
> Fixing this won't be a simple one-liner because it will require implementation
> of nested probing for scsi devices, such that all LUNS on the controller are
> probed before moving to the next device.

Ok, maybe that change will be too big for QEMU 9.2 anyway (we're in freeze 
now), so it's ok to include this later, I think.

And in case you're interested (it's maybe not so important since it's rather 
unlikely that the users will do this), there is another issue when trying to 
boot from multiple network devices where the first one has a DHCP server but 
no bootfile:

qemu-system-s390x -nographic -accel kvm -m 2G -netdev user,id=n1 \
  -device virtio-net-ccw,netdev=n1,bootindex=1 \
  -netdev user,id=n2,tftp=/boot,bootfile=vmlinuz \
  -device virtio-net-ccw,netdev=n2,bootindex=2 -d guest_errors

The firmware seems to panic while trying to request DHCP information from 
the second boot device.

  Thomas