MAINTAINERS | 2 + docs/devel/s390-dasd-ipl.txt | 133 ++++++++++++++ hw/s390x/ipl.c | 61 +++++-- hw/s390x/s390-ccw.c | 9 + hw/vfio/ccw.c | 2 +- include/hw/s390x/s390-ccw.h | 1 + include/hw/s390x/vfio-ccw.h | 28 +++ pc-bios/s390-ccw/Makefile | 2 +- pc-bios/s390-ccw/cio.c | 423 +++++++++++++++++++++++++++++++++++++++++++ pc-bios/s390-ccw/cio.h | 270 +++++++++++++++++++++------ pc-bios/s390-ccw/dasd-ipl.c | 235 ++++++++++++++++++++++++ pc-bios/s390-ccw/dasd-ipl.h | 16 ++ pc-bios/s390-ccw/helper.h | 31 ++++ pc-bios/s390-ccw/libc.h | 11 ++ pc-bios/s390-ccw/main.c | 185 +++++++++++++------ pc-bios/s390-ccw/netboot.mak | 2 +- pc-bios/s390-ccw/netmain.c | 2 + pc-bios/s390-ccw/s390-arch.h | 103 +++++++++++ pc-bios/s390-ccw/s390-ccw.h | 10 +- pc-bios/s390-ccw/start.S | 29 +++ pc-bios/s390-ccw/virtio.c | 81 +++------ tests/boot-serial-test.c | 2 +- 22 files changed, 1438 insertions(+), 200 deletions(-) create mode 100644 docs/devel/s390-dasd-ipl.txt create mode 100644 include/hw/s390x/vfio-ccw.h create mode 100644 pc-bios/s390-ccw/cio.c create mode 100644 pc-bios/s390-ccw/dasd-ipl.c create mode 100644 pc-bios/s390-ccw/dasd-ipl.h create mode 100644 pc-bios/s390-ccw/helper.h create mode 100644 pc-bios/s390-ccw/s390-arch.h
This is to support booting from vfio-ccw dasd devices. We basically implement the real hardware ipl procedure. This allows for booting Linux guests on vfio-ccw devices. vfio-ccw's channel program prefetch algorithm complicates ipl because most ipl channel programs dynamically modify themselves. Details on the ipl process and how we worked around this issue can be found in docs/devel/s390-dasd-ipl.txt. Changelog ========== v6 - Added patch #16 to fix regression whereby we attempted to boot from console devices when no default boot device was specified. - Bug fix: Referencing a byte past end of sense data status array - Reorganized patch order slightly - Other misc wording/spelling fixups Jason J. Herne (16): s390 vfio-ccw: Add bootindex property and IPLB data s390-bios: decouple cio setup from virtio s390-bios: decouple common boot logic from virtio s390-bios: Clean up cio.h s390-bios: Decouple channel i/o logic from virtio s390-bios: Map low core memory s390-bios: ptr2u32 and u32toptr s390-bios: Support for running format-0/1 channel programs s390-bios: cio error handling s390-bios: Extend find_dev() for non-virtio devices s390-bios: Factor finding boot device out of virtio code path s390-bios: Refactor virtio to run channel programs via cio s390-bios: Use control unit type to determine boot method s390-bios: Add channel command codes/structs needed for dasd-ipl s390-bios: Support booting from real dasd device s390-bios: Use control unit type to find bootable devices MAINTAINERS | 2 + docs/devel/s390-dasd-ipl.txt | 133 ++++++++++++++ hw/s390x/ipl.c | 61 +++++-- hw/s390x/s390-ccw.c | 9 + hw/vfio/ccw.c | 2 +- include/hw/s390x/s390-ccw.h | 1 + include/hw/s390x/vfio-ccw.h | 28 +++ pc-bios/s390-ccw/Makefile | 2 +- pc-bios/s390-ccw/cio.c | 423 +++++++++++++++++++++++++++++++++++++++++++ pc-bios/s390-ccw/cio.h | 270 +++++++++++++++++++++------ pc-bios/s390-ccw/dasd-ipl.c | 235 ++++++++++++++++++++++++ pc-bios/s390-ccw/dasd-ipl.h | 16 ++ pc-bios/s390-ccw/helper.h | 31 ++++ pc-bios/s390-ccw/libc.h | 11 ++ pc-bios/s390-ccw/main.c | 185 +++++++++++++------ pc-bios/s390-ccw/netboot.mak | 2 +- pc-bios/s390-ccw/netmain.c | 2 + pc-bios/s390-ccw/s390-arch.h | 103 +++++++++++ pc-bios/s390-ccw/s390-ccw.h | 10 +- pc-bios/s390-ccw/start.S | 29 +++ pc-bios/s390-ccw/virtio.c | 81 +++------ tests/boot-serial-test.c | 2 +- 22 files changed, 1438 insertions(+), 200 deletions(-) create mode 100644 docs/devel/s390-dasd-ipl.txt create mode 100644 include/hw/s390x/vfio-ccw.h create mode 100644 pc-bios/s390-ccw/cio.c create mode 100644 pc-bios/s390-ccw/dasd-ipl.c create mode 100644 pc-bios/s390-ccw/dasd-ipl.h create mode 100644 pc-bios/s390-ccw/helper.h create mode 100644 pc-bios/s390-ccw/s390-arch.h -- 2.7.4
Patchew URL: https://patchew.org/QEMU/1554388475-18329-1-git-send-email-jjherne@linux.ibm.com/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 1554388475-18329-1-git-send-email-jjherne@linux.ibm.com Subject: [Qemu-devel] [PATCH v6 00/16] s390: vfio-ccw dasd ipl support Type: series === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu t [tag update] patchew/1554388475-18329-1-git-send-email-jjherne@linux.ibm.com -> patchew/1554388475-18329-1-git-send-email-jjherne@linux.ibm.com Switched to a new branch 'test' 56f3c00ae7 s390-bios: Use control unit type to find bootable devices c2966e5f5d s390-bios: Support booting from real dasd device 983766a941 s390-bios: Add channel command codes/structs needed for dasd-ipl 611ee8ffd5 s390-bios: Use control unit type to determine boot method 50e3c7ec0a s390-bios: Refactor virtio to run channel programs via cio e1e4dd2d06 s390-bios: Factor finding boot device out of virtio code path 1545b18713 s390-bios: Extend find_dev() for non-virtio devices 059f0520c8 s390-bios: cio error handling 1ec28c0f96 s390-bios: Support for running format-0/1 channel programs 85d5a7e24e s390-bios: ptr2u32 and u32toptr 2cd731c880 s390-bios: Map low core memory 49c910eb88 s390-bios: Decouple channel i/o logic from virtio baf28e7a02 s390-bios: Clean up cio.h edd8b22728 s390-bios: decouple common boot logic from virtio 29a974796f s390-bios: decouple cio setup from virtio 34c790a9b0 s390 vfio-ccw: Add bootindex property and IPLB data === OUTPUT BEGIN === 1/16 Checking commit 34c790a9b010 (s390 vfio-ccw: Add bootindex property and IPLB data) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #222: new file mode 100644 total: 0 errors, 1 warnings, 199 lines checked Patch 1/16 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 2/16 Checking commit 29a974796f76 (s390-bios: decouple cio setup from virtio) 3/16 Checking commit edd8b2272899 (s390-bios: decouple common boot logic from virtio) ERROR: externs should be avoided in .c files #31: FILE: pc-bios/s390-ccw/main.c:19: +IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); total: 1 errors, 0 warnings, 65 lines checked Patch 3/16 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 4/16 Checking commit baf28e7a023e (s390-bios: Clean up cio.h) 5/16 Checking commit 49c910eb88a3 (s390-bios: Decouple channel i/o logic from virtio) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #30: new file mode 100644 total: 0 errors, 1 warnings, 123 lines checked Patch 5/16 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 6/16 Checking commit 2cd731c8806e (s390-bios: Map low core memory) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #37: new file mode 100644 total: 0 errors, 1 warnings, 104 lines checked Patch 6/16 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 7/16 Checking commit 85d5a7e24ef9 (s390-bios: ptr2u32 and u32toptr) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #17: new file mode 100644 total: 0 errors, 1 warnings, 31 lines checked Patch 7/16 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 8/16 Checking commit 1ec28c0f965b (s390-bios: Support for running format-0/1 channel programs) 9/16 Checking commit 059f0520c805 (s390-bios: cio error handling) 10/16 Checking commit 1545b18713d6 (s390-bios: Extend find_dev() for non-virtio devices) 11/16 Checking commit e1e4dd2d06c0 (s390-bios: Factor finding boot device out of virtio code path) 12/16 Checking commit 50e3c7ec0a59 (s390-bios: Refactor virtio to run channel programs via cio) WARNING: line over 80 characters #105: FILE: pc-bios/s390-ccw/virtio.c:298: + run_ccw(vdev, CCW_CMD_READ_VQ_CONF, &config, sizeof(config), false) == 0, WARNING: line over 80 characters #118: FILE: pc-bios/s390-ccw/virtio.c:308: + run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false) == 0, total: 0 errors, 2 warnings, 115 lines checked Patch 12/16 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 13/16 Checking commit 611ee8ffd5d1 (s390-bios: Use control unit type to determine boot method) 14/16 Checking commit 983766a94135 (s390-bios: Add channel command codes/structs needed for dasd-ipl) 15/16 Checking commit c2966e5f5d6b (s390-bios: Support booting from real dasd device) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #26: new file mode 100644 total: 0 errors, 1 warnings, 433 lines checked Patch 15/16 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 16/16 Checking commit 56f3c00ae7c6 (s390-bios: Use control unit type to find bootable devices) === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/1554388475-18329-1-git-send-email-jjherne@linux.ibm.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
Patchew URL: https://patchew.org/QEMU/1554388475-18329-1-git-send-email-jjherne@linux.ibm.com/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 1554388475-18329-1-git-send-email-jjherne@linux.ibm.com Subject: [Qemu-devel] [PATCH v6 00/16] s390: vfio-ccw dasd ipl support Type: series === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/1554388475-18329-1-git-send-email-jjherne@linux.ibm.com -> patchew/1554388475-18329-1-git-send-email-jjherne@linux.ibm.com t [tag update] patchew/20190402161900.7374-1-armbru@redhat.com -> patchew/20190402161900.7374-1-armbru@redhat.com t [tag update] patchew/20190404091725.20595-1-dgilbert@redhat.com -> patchew/20190404091725.20595-1-dgilbert@redhat.com * [new tag] patchew/20190404112953.4058-1-berto@igalia.com -> patchew/20190404112953.4058-1-berto@igalia.com * [new tag] patchew/20190404121015.28634-1-pl@kamp.de -> patchew/20190404121015.28634-1-pl@kamp.de Switched to a new branch 'test' e794c6367e s390-bios: Use control unit type to find bootable devices aea63e7dfb s390-bios: Support booting from real dasd device 8f7c6a5a42 s390-bios: Add channel command codes/structs needed for dasd-ipl dd95e5ec50 s390-bios: Use control unit type to determine boot method e408cfc1f1 s390-bios: Refactor virtio to run channel programs via cio 8357bd089e s390-bios: Factor finding boot device out of virtio code path 05d2aa89c9 s390-bios: Extend find_dev() for non-virtio devices 54e72ca6e1 s390-bios: cio error handling 9577eecec1 s390-bios: Support for running format-0/1 channel programs 1274b880cc s390-bios: ptr2u32 and u32toptr 63ce0ae260 s390-bios: Map low core memory 995578a87b s390-bios: Decouple channel i/o logic from virtio c0c43849b8 s390-bios: Clean up cio.h 1b82e3b63f s390-bios: decouple common boot logic from virtio 88ddf9dcb2 s390-bios: decouple cio setup from virtio 1357e39998 s390 vfio-ccw: Add bootindex property and IPLB data === OUTPUT BEGIN === 1/16 Checking commit 1357e399982b (s390 vfio-ccw: Add bootindex property and IPLB data) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #222: new file mode 100644 total: 0 errors, 1 warnings, 199 lines checked Patch 1/16 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 2/16 Checking commit 88ddf9dcb222 (s390-bios: decouple cio setup from virtio) 3/16 Checking commit 1b82e3b63f0f (s390-bios: decouple common boot logic from virtio) ERROR: externs should be avoided in .c files #31: FILE: pc-bios/s390-ccw/main.c:19: +IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); total: 1 errors, 0 warnings, 65 lines checked Patch 3/16 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 4/16 Checking commit c0c43849b8a6 (s390-bios: Clean up cio.h) 5/16 Checking commit 995578a87b7b (s390-bios: Decouple channel i/o logic from virtio) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #30: new file mode 100644 total: 0 errors, 1 warnings, 123 lines checked Patch 5/16 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 6/16 Checking commit 63ce0ae260c6 (s390-bios: Map low core memory) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #37: new file mode 100644 total: 0 errors, 1 warnings, 104 lines checked Patch 6/16 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 7/16 Checking commit 1274b880cc60 (s390-bios: ptr2u32 and u32toptr) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #17: new file mode 100644 total: 0 errors, 1 warnings, 31 lines checked Patch 7/16 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 8/16 Checking commit 9577eecec1f6 (s390-bios: Support for running format-0/1 channel programs) 9/16 Checking commit 54e72ca6e1dc (s390-bios: cio error handling) 10/16 Checking commit 05d2aa89c93e (s390-bios: Extend find_dev() for non-virtio devices) 11/16 Checking commit 8357bd089e30 (s390-bios: Factor finding boot device out of virtio code path) 12/16 Checking commit e408cfc1f1ce (s390-bios: Refactor virtio to run channel programs via cio) WARNING: line over 80 characters #105: FILE: pc-bios/s390-ccw/virtio.c:298: + run_ccw(vdev, CCW_CMD_READ_VQ_CONF, &config, sizeof(config), false) == 0, WARNING: line over 80 characters #118: FILE: pc-bios/s390-ccw/virtio.c:308: + run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false) == 0, total: 0 errors, 2 warnings, 115 lines checked Patch 12/16 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 13/16 Checking commit dd95e5ec5028 (s390-bios: Use control unit type to determine boot method) 14/16 Checking commit 8f7c6a5a42f6 (s390-bios: Add channel command codes/structs needed for dasd-ipl) 15/16 Checking commit aea63e7dfb1f (s390-bios: Support booting from real dasd device) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #26: new file mode 100644 total: 0 errors, 1 warnings, 433 lines checked Patch 15/16 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 16/16 Checking commit e794c6367eaa (s390-bios: Use control unit type to find bootable devices) === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/1554388475-18329-1-git-send-email-jjherne@linux.ibm.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On 04/04/2019 16.34, Jason J. Herne wrote: > This is to support booting from vfio-ccw dasd devices. We basically implement > the real hardware ipl procedure. This allows for booting Linux guests on > vfio-ccw devices. > > vfio-ccw's channel program prefetch algorithm complicates ipl because most ipl > channel programs dynamically modify themselves. Details on the ipl process and > how we worked around this issue can be found in docs/devel/s390-dasd-ipl.txt. Hi Jason, 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? Thanks, Thomas
On 05/04/2019 08.58, Thomas Huth wrote: > On 04/04/2019 16.34, Jason J. Herne wrote: >> This is to support booting from vfio-ccw dasd devices. We basically implement >> the real hardware ipl procedure. This allows for booting Linux guests on >> vfio-ccw devices. >> >> vfio-ccw's channel program prefetch algorithm complicates ipl because most ipl >> channel programs dynamically modify themselves. Details on the ipl process and >> how we worked around this issue can be found in docs/devel/s390-dasd-ipl.txt. > > Hi Jason, > > 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
On 4/5/19 3:52 AM, Thomas Huth wrote:
> On 05/04/2019 08.58, Thomas Huth wrote:
>> On 04/04/2019 16.34, Jason J. Herne wrote:
>>> This is to support booting from vfio-ccw dasd devices. We basically implement
>>> the real hardware ipl procedure. This allows for booting Linux guests on
>>> vfio-ccw devices.
>>>
>>> vfio-ccw's channel program prefetch algorithm complicates ipl because most ipl
>>> channel programs dynamically modify themselves. Details on the ipl process and
>>> how we worked around this issue can be found in docs/devel/s390-dasd-ipl.txt.
>>
>> Hi Jason,
>>
>> 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
--
-- Jason J. Herne (jjherne@linux.ibm.com)
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
On Fri, 5 Apr 2019 15:26:24 +0200
Thomas Huth <thuth@redhat.com> wrote:
> On 05/04/2019 15.11, Jason J. Herne wrote:
> > 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.
If you (Jason) resend just that patch, I'm happy to give it a 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...
Agreed.
>
> If you like, I can squash these changes in when I pick up the patches,
> so you don't have to resend.
>
> Thomas
>
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 (jjherne@linux.ibm.com)
© 2016 - 2024 Red Hat, Inc.