On 4/5/19 9:26 AM, Thomas Huth wrote:
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...
I agree with your comment.
If you like, I can squash these changes in when I pick up the patches,
so you don't have to resend.
I'll take you up on that offer, thanks Thomas. :)
--
-- Jason J. Herne (jjhe...@linux.ibm.com)
