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
