| Message ID | 20240621082422.136217-1-thuth@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | pc-bios/s390-ccw: Merge the netboot loader into s390-ccw.img | expand |
[...] > docs/system/s390x/bootdevices.rst | 20 +++---- > pc-bios/s390-ccw/netboot.mak | 62 --------------------- > hw/s390x/ipl.h | 12 ++-- > pc-bios/s390-ccw/cio.h | 2 + > pc-bios/s390-ccw/iplb.h | 4 +- > pc-bios/s390-ccw/libc.h | 89 ------------------------------ > pc-bios/s390-ccw/s390-ccw.h | 10 +++- > pc-bios/s390-ccw/virtio.h | 1 - > hw/s390x/ipl.c | 65 +++------------------- > hw/s390x/s390-virtio-ccw.c | 10 +--- > pc-bios/s390-ccw/bootmap.c | 4 +- > pc-bios/s390-ccw/cio.c | 2 +- > pc-bios/s390-ccw/dasd-ipl.c | 2 +- > pc-bios/s390-ccw/jump2ipl.c | 2 +- > pc-bios/s390-ccw/libc.c | 88 ----------------------------- > pc-bios/s390-ccw/main.c | 15 +++-- > pc-bios/s390-ccw/menu.c | 25 ++++----- > pc-bios/s390-ccw/netmain.c | 15 +---- > pc-bios/s390-ccw/sclp.c | 2 +- > pc-bios/s390-ccw/virtio-blkdev.c | 1 - > pc-bios/s390-ccw/virtio-scsi.c | 2 +- > pc-bios/s390-ccw/virtio.c | 2 +- > pc-bios/meson.build | 1 - > pc-bios/s390-ccw/Makefile | 69 +++++++++++++++++++---- > pc-bios/s390-netboot.img | Bin 67232 -> 0 bytes Shouldnt you also update the s390-ccw.img file? > 25 files changed, 122 insertions(+), 383 deletions(-) > delete mode 100644 pc-bios/s390-ccw/netboot.mak > delete mode 100644 pc-bios/s390-ccw/libc.h > delete mode 100644 pc-bios/s390-ccw/libc.c > delete mode 100644 pc-bios/s390-netboot.img >
On 21/06/2024 11.39, Christian Borntraeger wrote: > [...] >> docs/system/s390x/bootdevices.rst | 20 +++---- >> pc-bios/s390-ccw/netboot.mak | 62 --------------------- >> hw/s390x/ipl.h | 12 ++-- >> pc-bios/s390-ccw/cio.h | 2 + >> pc-bios/s390-ccw/iplb.h | 4 +- >> pc-bios/s390-ccw/libc.h | 89 ------------------------------ >> pc-bios/s390-ccw/s390-ccw.h | 10 +++- >> pc-bios/s390-ccw/virtio.h | 1 - >> hw/s390x/ipl.c | 65 +++------------------- >> hw/s390x/s390-virtio-ccw.c | 10 +--- >> pc-bios/s390-ccw/bootmap.c | 4 +- >> pc-bios/s390-ccw/cio.c | 2 +- >> pc-bios/s390-ccw/dasd-ipl.c | 2 +- >> pc-bios/s390-ccw/jump2ipl.c | 2 +- >> pc-bios/s390-ccw/libc.c | 88 ----------------------------- >> pc-bios/s390-ccw/main.c | 15 +++-- >> pc-bios/s390-ccw/menu.c | 25 ++++----- >> pc-bios/s390-ccw/netmain.c | 15 +---- >> pc-bios/s390-ccw/sclp.c | 2 +- >> pc-bios/s390-ccw/virtio-blkdev.c | 1 - >> pc-bios/s390-ccw/virtio-scsi.c | 2 +- >> pc-bios/s390-ccw/virtio.c | 2 +- >> pc-bios/meson.build | 1 - >> pc-bios/s390-ccw/Makefile | 69 +++++++++++++++++++---- >> pc-bios/s390-netboot.img | Bin 67232 -> 0 bytes > > Shouldnt you also update the s390-ccw.img file? Sure, but I didn't want to spam the mailing list with a binary blob as long as this is not final yet (not sure whether we want to commit this separately or wait 'til Jared finished his series, too). Sorry, I should have mentioned it in the cover letter. Thomas
On Fri, 2024-06-21 at 10:24 +0200, Thomas Huth wrote: > We originally built a separate binary for the netboot code since it > was considered as experimental and we could not be sure that the > necessary SLOF module had been checked out. Time passed, the netboot > code proved its usefulness, and the build system nowadays makes sure > that the SLOF module is checked out if you have a s390x compiler > available > for building the s390-ccw bios. In fact, the possibility to build the > s390-ccw.img without s390-netboot.img has been removed in commit > bf6903f6944f ("pc-bios/s390-ccw: always build network bootloader") > already. > > So it does not make too much sense anymore to keep the netboot code > in a separate binary. To make it easier to support a more flexible > boot process soon that supports more than one boot device via the > bootindex properties, let's finally merge the netboot code into the > main s390-ccw.img binary now. Hi Thomas, I find myself wondering about the side effects of the s/sclp_print/printf/ changes, but I haven't come up with anything I can put my finger on. Maybe something will come to me over the weekend, but all-in-all I like the looks of this. So, FWIW: Reviewed-by: Eric Farman <farman@linux.ibm.com> > > Thomas Huth (7): > pc-bios/s390-ccw: Remove duplicated LDFLAGS > hw/s390x/ipl: Provide more memory to the s390-ccw.img firmware > pc-bios/s390-ccw: Use the libc from SLOF for the main s390-ccw.img > binary, too > pc-bios/s390-ccw: Link the netboot code into the main s390-ccw.img > binary > hw/s390x: Remove the possibility to load the s390-netboot.img > binary > pc-bios/s390-ccw: Merge netboot.mak into the main Makefile > docs/system/s390x/bootdevices: Update the documentation about > network > booting > > docs/system/s390x/bootdevices.rst | 20 +++---- > pc-bios/s390-ccw/netboot.mak | 62 --------------------- > hw/s390x/ipl.h | 12 ++-- > pc-bios/s390-ccw/cio.h | 2 + > pc-bios/s390-ccw/iplb.h | 4 +- > pc-bios/s390-ccw/libc.h | 89 ---------------------------- > -- > pc-bios/s390-ccw/s390-ccw.h | 10 +++- > pc-bios/s390-ccw/virtio.h | 1 - > hw/s390x/ipl.c | 65 +++------------------- > hw/s390x/s390-virtio-ccw.c | 10 +--- > pc-bios/s390-ccw/bootmap.c | 4 +- > pc-bios/s390-ccw/cio.c | 2 +- > pc-bios/s390-ccw/dasd-ipl.c | 2 +- > pc-bios/s390-ccw/jump2ipl.c | 2 +- > pc-bios/s390-ccw/libc.c | 88 ---------------------------- > - > pc-bios/s390-ccw/main.c | 15 +++-- > pc-bios/s390-ccw/menu.c | 25 ++++----- > pc-bios/s390-ccw/netmain.c | 15 +---- > pc-bios/s390-ccw/sclp.c | 2 +- > pc-bios/s390-ccw/virtio-blkdev.c | 1 - > pc-bios/s390-ccw/virtio-scsi.c | 2 +- > pc-bios/s390-ccw/virtio.c | 2 +- > pc-bios/meson.build | 1 - > pc-bios/s390-ccw/Makefile | 69 +++++++++++++++++++---- > pc-bios/s390-netboot.img | Bin 67232 -> 0 bytes > 25 files changed, 122 insertions(+), 383 deletions(-) > delete mode 100644 pc-bios/s390-ccw/netboot.mak > delete mode 100644 pc-bios/s390-ccw/libc.h > delete mode 100644 pc-bios/s390-ccw/libc.c > delete mode 100644 pc-bios/s390-netboot.img >
On 6/21/24 4:24 AM, Thomas Huth wrote: > We originally built a separate binary for the netboot code since it > was considered as experimental and we could not be sure that the > necessary SLOF module had been checked out. Time passed, the netboot > code proved its usefulness, and the build system nowadays makes sure > that the SLOF module is checked out if you have a s390x compiler available > for building the s390-ccw bios. In fact, the possibility to build the > s390-ccw.img without s390-netboot.img has been removed in commit > bf6903f6944f ("pc-bios/s390-ccw: always build network bootloader") > already. > > So it does not make too much sense anymore to keep the netboot code > in a separate binary. To make it easier to support a more flexible > boot process soon that supports more than one boot device via the > bootindex properties, let's finally merge the netboot code into the > main s390-ccw.img binary now. Hi Thomas, One area that could possibly be cleaned up further are places where net devices are treated as corner cases due to the separate bootloader. Off the top of my head I know in pc-bios main.c, the is_dev_possibly_bootable() function rejects net devices for this reason. I'm not sure if that is the only place though. Otherwise it looks good to me. I can work on a v2 of the boot order support that assumes the network bootloader is integrated. Regards, Jared Rossi > > Thomas Huth (7): > pc-bios/s390-ccw: Remove duplicated LDFLAGS > hw/s390x/ipl: Provide more memory to the s390-ccw.img firmware > pc-bios/s390-ccw: Use the libc from SLOF for the main s390-ccw.img > binary, too > pc-bios/s390-ccw: Link the netboot code into the main s390-ccw.img > binary > hw/s390x: Remove the possibility to load the s390-netboot.img binary > pc-bios/s390-ccw: Merge netboot.mak into the main Makefile > docs/system/s390x/bootdevices: Update the documentation about network > booting > > docs/system/s390x/bootdevices.rst | 20 +++---- > pc-bios/s390-ccw/netboot.mak | 62 --------------------- > hw/s390x/ipl.h | 12 ++-- > pc-bios/s390-ccw/cio.h | 2 + > pc-bios/s390-ccw/iplb.h | 4 +- > pc-bios/s390-ccw/libc.h | 89 ------------------------------ > pc-bios/s390-ccw/s390-ccw.h | 10 +++- > pc-bios/s390-ccw/virtio.h | 1 - > hw/s390x/ipl.c | 65 +++------------------- > hw/s390x/s390-virtio-ccw.c | 10 +--- > pc-bios/s390-ccw/bootmap.c | 4 +- > pc-bios/s390-ccw/cio.c | 2 +- > pc-bios/s390-ccw/dasd-ipl.c | 2 +- > pc-bios/s390-ccw/jump2ipl.c | 2 +- > pc-bios/s390-ccw/libc.c | 88 ----------------------------- > pc-bios/s390-ccw/main.c | 15 +++-- > pc-bios/s390-ccw/menu.c | 25 ++++----- > pc-bios/s390-ccw/netmain.c | 15 +---- > pc-bios/s390-ccw/sclp.c | 2 +- > pc-bios/s390-ccw/virtio-blkdev.c | 1 - > pc-bios/s390-ccw/virtio-scsi.c | 2 +- > pc-bios/s390-ccw/virtio.c | 2 +- > pc-bios/meson.build | 1 - > pc-bios/s390-ccw/Makefile | 69 +++++++++++++++++++---- > pc-bios/s390-netboot.img | Bin 67232 -> 0 bytes > 25 files changed, 122 insertions(+), 383 deletions(-) > delete mode 100644 pc-bios/s390-ccw/netboot.mak > delete mode 100644 pc-bios/s390-ccw/libc.h > delete mode 100644 pc-bios/s390-ccw/libc.c > delete mode 100644 pc-bios/s390-netboot.img >
On 21/06/2024 22.51, Eric Farman wrote: > On Fri, 2024-06-21 at 10:24 +0200, Thomas Huth wrote: >> We originally built a separate binary for the netboot code since it >> was considered as experimental and we could not be sure that the >> necessary SLOF module had been checked out. Time passed, the netboot >> code proved its usefulness, and the build system nowadays makes sure >> that the SLOF module is checked out if you have a s390x compiler >> available >> for building the s390-ccw bios. In fact, the possibility to build the >> s390-ccw.img without s390-netboot.img has been removed in commit >> bf6903f6944f ("pc-bios/s390-ccw: always build network bootloader") >> already. >> >> So it does not make too much sense anymore to keep the netboot code >> in a separate binary. To make it easier to support a more flexible >> boot process soon that supports more than one boot device via the >> bootindex properties, let's finally merge the netboot code into the >> main s390-ccw.img binary now. > > Hi Thomas, > > I find myself wondering about the side effects of the > s/sclp_print/printf/ changes, but I haven't come up with anything I can > put my finger on. Maybe something will come to me over the weekend, but > all-in-all I like the looks of this. I think it should be fine, both functions are basically just a wrapper around the write() function in sclp.c, with sclp_print() being rather dumb while printf() is doing the usual string formatting before writing it out. I think in the long run, it would be nice to get rid of sclp_print() and replace it by puts() or printf() in the whole code, but doing that right now would likely cause quite some conflicts for Jared with his patch series, so I'd rather postpone that to a later point in time. > Reviewed-by: Eric Farman <farman@linux.ibm.com> Thanks! Thomas
On 6/24/24 1:55 AM, Thomas Huth wrote: > [...] > > I think it should be fine, both functions are basically just a wrapper > around the write() function in sclp.c, with sclp_print() being rather > dumb while printf() is doing the usual string formatting before > writing it out. I think in the long run, it would be nice to get rid > of sclp_print() and replace it by puts() or printf() in the whole > code, but doing that right now would likely cause quite some conflicts > for Jared with his patch series, so I'd rather postpone that to a > later point in time. Hi Thomas, Converting the panics to returns will require me to modify/move some of the sclp_print() calls. Shall I go ahead and change them to printf() and puts() while I'm at it, or would you rather preserve the sclp_print() for now and then have a dedicated patch for the all replacements later? I'm not sure if we want to try to maintain some amount of consistency until we do a total conversion, or if you are OK with a mix of sclp_print() and printf() throughout in the meantime. Regards, Jared Rossi
On 28/06/2024 20.01, Jared Rossi wrote: > > > On 6/24/24 1:55 AM, Thomas Huth wrote: >> [...] >> >> I think it should be fine, both functions are basically just a wrapper >> around the write() function in sclp.c, with sclp_print() being rather dumb >> while printf() is doing the usual string formatting before writing it out. >> I think in the long run, it would be nice to get rid of sclp_print() and >> replace it by puts() or printf() in the whole code, but doing that right >> now would likely cause quite some conflicts for Jared with his patch >> series, so I'd rather postpone that to a later point in time. > > Hi Thomas, > > Converting the panics to returns will require me to modify/move some of the > sclp_print() calls. Shall I go ahead and change them to printf() and puts() > while I'm at it, or would you rather preserve the sclp_print() for now and > then have a dedicated patch for the all replacements later? I'm not sure if > we want to try to maintain some amount of consistency until we do a total > conversion, or if you are OK with a mix of sclp_print() and printf() > throughout in the meantime. Hi, I'm ok if we mix them for a while, so I'd say go ahead and use printf or puts for the new code. Thomas
Hi Thomas, I just wanted to get your thoughts on the status of the netboot loader merge. I see that the first patch from this series was merged, but not the others. Is there any issue with the rest of the changes? I would like to put together a comprehensive rework for all device types that replaces the IPL panics with returns, but as we discussed this is best applied after the netboot loader has been unified with the main s390-ccw.img file. I can base my patches on either current master (with a special case for netboot) or the combined bootloader, depending on how you would like to proceed. Let me know if there is anything I can do to help with test/review of the changes from your side. Thanks, Jared Rossi On 6/21/24 4:24 AM, Thomas Huth wrote: > We originally built a separate binary for the netboot code since it > was considered as experimental and we could not be sure that the > necessary SLOF module had been checked out. Time passed, the netboot > code proved its usefulness, and the build system nowadays makes sure > that the SLOF module is checked out if you have a s390x compiler available > for building the s390-ccw bios. In fact, the possibility to build the > s390-ccw.img without s390-netboot.img has been removed in commit > bf6903f6944f ("pc-bios/s390-ccw: always build network bootloader") > already. > > So it does not make too much sense anymore to keep the netboot code > in a separate binary. To make it easier to support a more flexible > boot process soon that supports more than one boot device via the > bootindex properties, let's finally merge the netboot code into the > main s390-ccw.img binary now. > > Thomas Huth (7): > pc-bios/s390-ccw: Remove duplicated LDFLAGS > hw/s390x/ipl: Provide more memory to the s390-ccw.img firmware > pc-bios/s390-ccw: Use the libc from SLOF for the main s390-ccw.img > binary, too > pc-bios/s390-ccw: Link the netboot code into the main s390-ccw.img > binary > hw/s390x: Remove the possibility to load the s390-netboot.img binary > pc-bios/s390-ccw: Merge netboot.mak into the main Makefile > docs/system/s390x/bootdevices: Update the documentation about network > booting > > docs/system/s390x/bootdevices.rst | 20 +++---- > pc-bios/s390-ccw/netboot.mak | 62 --------------------- > hw/s390x/ipl.h | 12 ++-- > pc-bios/s390-ccw/cio.h | 2 + > pc-bios/s390-ccw/iplb.h | 4 +- > pc-bios/s390-ccw/libc.h | 89 ------------------------------ > pc-bios/s390-ccw/s390-ccw.h | 10 +++- > pc-bios/s390-ccw/virtio.h | 1 - > hw/s390x/ipl.c | 65 +++------------------- > hw/s390x/s390-virtio-ccw.c | 10 +--- > pc-bios/s390-ccw/bootmap.c | 4 +- > pc-bios/s390-ccw/cio.c | 2 +- > pc-bios/s390-ccw/dasd-ipl.c | 2 +- > pc-bios/s390-ccw/jump2ipl.c | 2 +- > pc-bios/s390-ccw/libc.c | 88 ----------------------------- > pc-bios/s390-ccw/main.c | 15 +++-- > pc-bios/s390-ccw/menu.c | 25 ++++----- > pc-bios/s390-ccw/netmain.c | 15 +---- > pc-bios/s390-ccw/sclp.c | 2 +- > pc-bios/s390-ccw/virtio-blkdev.c | 1 - > pc-bios/s390-ccw/virtio-scsi.c | 2 +- > pc-bios/s390-ccw/virtio.c | 2 +- > pc-bios/meson.build | 1 - > pc-bios/s390-ccw/Makefile | 69 +++++++++++++++++++---- > pc-bios/s390-netboot.img | Bin 67232 -> 0 bytes > 25 files changed, 122 insertions(+), 383 deletions(-) > delete mode 100644 pc-bios/s390-ccw/netboot.mak > delete mode 100644 pc-bios/s390-ccw/libc.h > delete mode 100644 pc-bios/s390-ccw/libc.c > delete mode 100644 pc-bios/s390-netboot.img >
On 26/08/2024 19.07, Jared Rossi wrote: > Hi Thomas, > > I just wanted to get your thoughts on the status of the netboot loader merge. > I see that the first patch from this series was merged, but not the others. Is > there any issue with the rest of the changes? Hi Jared, there's no issue with that patch series - it's just that QEMU is in freeze now and only accepts bug fixes. See https://wiki.qemu.org/Planning/9.1 > I would like to put together a comprehensive rework for all device types that > replaces the IPL panics with returns, but as we discussed this is best applied > after the netboot loader has been unified with the main s390-ccw.img file. > > I can base my patches on either current master (with a special case for > netboot) > or the combined bootloader, depending on how you would like to proceed. Let me > know if there is anything I can do to help with test/review of the changes from > your side. I think it's maybe best if you'd include my patches at the top of your patch series, so you could also rework them in case you need something to be changed there. That way, we also do not have to rebuild the binaries in the git repo multiple times and just have to update them one time once your series is ready to go. Alternatively, if you don't want to juggle with my patches, I can also try to get them merged as soon as QEMU 9.1 has been released. Just let me know if you prefer that. Thomas
On 8/27/24 8:43 AM, Thomas Huth wrote: > I think it's maybe best if you'd include my patches at the top of your > patch series, so you could also rework them in case you need something > to be changed there. That way, we also do not have to rebuild the > binaries in the git repo multiple times and just have to update them > one time once your series is ready to go. > > Alternatively, if you don't want to juggle with my patches, I can also > try to get them merged as soon as QEMU 9.1 has been released. Just let > me know if you prefer that. > > Thomas > I am fine with including your patches in my series. I will get all that put together and submit a V2 after QEMU 9.1 is squared away. Thanks again, Jared Rossi
We originally built a separate binary for the netboot code since it was considered as experimental and we could not be sure that the necessary SLOF module had been checked out. Time passed, the netboot code proved its usefulness, and the build system nowadays makes sure that the SLOF module is checked out if you have a s390x compiler available for building the s390-ccw bios. In fact, the possibility to build the s390-ccw.img without s390-netboot.img has been removed in commit bf6903f6944f ("pc-bios/s390-ccw: always build network bootloader") already. So it does not make too much sense anymore to keep the netboot code in a separate binary. To make it easier to support a more flexible boot process soon that supports more than one boot device via the bootindex properties, let's finally merge the netboot code into the main s390-ccw.img binary now. Thomas Huth (7): pc-bios/s390-ccw: Remove duplicated LDFLAGS hw/s390x/ipl: Provide more memory to the s390-ccw.img firmware pc-bios/s390-ccw: Use the libc from SLOF for the main s390-ccw.img binary, too pc-bios/s390-ccw: Link the netboot code into the main s390-ccw.img binary hw/s390x: Remove the possibility to load the s390-netboot.img binary pc-bios/s390-ccw: Merge netboot.mak into the main Makefile docs/system/s390x/bootdevices: Update the documentation about network booting docs/system/s390x/bootdevices.rst | 20 +++---- pc-bios/s390-ccw/netboot.mak | 62 --------------------- hw/s390x/ipl.h | 12 ++-- pc-bios/s390-ccw/cio.h | 2 + pc-bios/s390-ccw/iplb.h | 4 +- pc-bios/s390-ccw/libc.h | 89 ------------------------------ pc-bios/s390-ccw/s390-ccw.h | 10 +++- pc-bios/s390-ccw/virtio.h | 1 - hw/s390x/ipl.c | 65 +++------------------- hw/s390x/s390-virtio-ccw.c | 10 +--- pc-bios/s390-ccw/bootmap.c | 4 +- pc-bios/s390-ccw/cio.c | 2 +- pc-bios/s390-ccw/dasd-ipl.c | 2 +- pc-bios/s390-ccw/jump2ipl.c | 2 +- pc-bios/s390-ccw/libc.c | 88 ----------------------------- pc-bios/s390-ccw/main.c | 15 +++-- pc-bios/s390-ccw/menu.c | 25 ++++----- pc-bios/s390-ccw/netmain.c | 15 +---- pc-bios/s390-ccw/sclp.c | 2 +- pc-bios/s390-ccw/virtio-blkdev.c | 1 - pc-bios/s390-ccw/virtio-scsi.c | 2 +- pc-bios/s390-ccw/virtio.c | 2 +- pc-bios/meson.build | 1 - pc-bios/s390-ccw/Makefile | 69 +++++++++++++++++++---- pc-bios/s390-netboot.img | Bin 67232 -> 0 bytes 25 files changed, 122 insertions(+), 383 deletions(-) delete mode 100644 pc-bios/s390-ccw/netboot.mak delete mode 100644 pc-bios/s390-ccw/libc.h delete mode 100644 pc-bios/s390-ccw/libc.c delete mode 100644 pc-bios/s390-netboot.img