Re: [qemu-s390x] [PATCH v6 00/16] s390: vfio-ccw dasd ipl support

[Thomas Huth]

On 05/04/2019 15.11, Jason J. Herne wrote:
> On 4/5/19 3:52 AM, Thomas Huth wrote:
>> On 05/04/2019 08.58, Thomas Huth wrote:
[...]
>>> while running my s390-ccw bios tests, I noticed that network booting
>>> seems to be broken now. This used to work before:
>>>
>>> s390x-softmmu/qemu-system-s390x -nographic -accel kvm \
>>>   -bios pc-bios/s390-ccw/s390-ccw.img \
>>>   -global s390-ipl.netboot_fw=pc-bios/s390-ccw/s390-netboot.img \
>>>   -netdev user,id=n1,tftp=/boot,bootfile=vmlinuz-4.18.0 \
>>>   -device virtio-net-ccw,netdev=n1,bootindex=1
>>>
>>> Now it simply fails with "! No IPL device available !".
>>>
>>> Could you have a look at it, please?
>>
>> FWIW: The problem seems to be in the last patch: virtio_is_supported()
>> is now not called anymore, and so virtio_get_device_type() now returns
>> the wrong type.
>>
>>   Thomas
>>
> 
> Good catch. Testing netboot for this iteration did slip my mind. I've
> now added a basic netboot script to my test suite to avoid this type of
> regression in the future.
> 
> Your analysis of the problem matches what I'm seeing as well. Here is
> what I'm proposing to fix it. If you like it, let me know if you want me
> to re-send just the final patch, or the entire series again with a v7 tag.
> 
> 
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 03e90e3..3d1e9a4 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -67,6 +67,7 @@ static bool find_subch(int dev_no)
>  {
>      Schib schib;
>      int i, r;
> +    bool is_virtio;
> 
>      for (i = 0; i < 0x10000; i++) {
>          blk_schid.sch_no = i;
> @@ -80,12 +81,13 @@ static bool find_subch(int dev_no)
> 
>          enable_subchannel(blk_schid);
>          cutype = cu_type(blk_schid);
> +        is_virtio = virtio_is_supported(blk_schid);
> 
>          /* No specific devno given, just return 1st possibly bootable
> device */
>          if (dev_no < 0) {
>              switch (cutype) {
>              case CU_TYPE_VIRTIO:
> -                if (virtio_is_supported(blk_schid)) {
> +                if (is_virtio) {
>                      /*
>                       * Skip net devices since no IPLB is created and
> therefore
>                       * no network bootloader has been loaded
> 
> 

Looks fine to me. But maybe we should also add a comment before the new
"virtio_is_supported" line, saying something like "Note: we always have
to run virtio_is_supported() here to make sure that the vdev.senseid
data gets pre-initialized"? Otherwise, we might accidentally "revert"
this patch when somebody thinks that the code might be optimizable by
removing the is_virtio variable again...

If you like, I can squash these changes in when I pick up the patches,
so you don't have to resend.

 Thomas