This patch series adds pxelinux.cfg-style network booting to the s390-ccw firmware. The core pxelinux.cfg loading and parsing logic has recently been merged to SLOF, so these patches now just have to make sure to call the right functions to get the config file loaded and parsed. Once this is done, the kernel and initrd are loaded separately, and are then glued together in RAM. v2: - Update SLOF submodule now that the git mirror is in sync again - Last parameter to tftp_get_error_info() must not be NULL - Check CC when calling STSI, and use a #define for the UUID offset - Only support files with the magic "# pxelinux" string comment when trying to guess the contents of a file that has been downloaded via the "bootfile" DHCP parameter. This is just for developers' convenience, the official way to specify pxelinux.cfg files is to use the DHCP options 209 and 210 instead. Thomas Huth (4): roms: Update SLOF submodule to current status pc-bios/s390-ccw/net: Update code for the latest changes in SLOF pc-bios/s390-ccw/net: Add support for pxelinux-style config files pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the UUID pc-bios/s390-ccw/netboot.mak | 9 +- pc-bios/s390-ccw/netmain.c | 226 +++++++++++++++++++++++++++++-------------- roms/SLOF | 2 +- 3 files changed, 162 insertions(+), 75 deletions(-) -- 1.8.3.1
On 06/07/2018 02:22 PM, Thomas Huth wrote: > This patch series adds pxelinux.cfg-style network booting to the s390-ccw > firmware. The core pxelinux.cfg loading and parsing logic has recently > been merged to SLOF, so these patches now just have to make sure to call > the right functions to get the config file loaded and parsed. Once this is > done, the kernel and initrd are loaded separately, and are then glued > together in RAM. > > v2: > - Update SLOF submodule now that the git mirror is in sync again > - Last parameter to tftp_get_error_info() must not be NULL > - Check CC when calling STSI, and use a #define for the UUID offset > - Only support files with the magic "# pxelinux" string comment when > trying to guess the contents of a file that has been downloaded via > the "bootfile" DHCP parameter. This is just for developers' convenience, > the official way to specify pxelinux.cfg files is to use the DHCP > options 209 and 210 instead. > > Thomas Huth (4): > roms: Update SLOF submodule to current status > pc-bios/s390-ccw/net: Update code for the latest changes in SLOF > pc-bios/s390-ccw/net: Add support for pxelinux-style config files > pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the > UUID > > pc-bios/s390-ccw/netboot.mak | 9 +- > pc-bios/s390-ccw/netmain.c | 226 +++++++++++++++++++++++++++++-------------- > roms/SLOF | 2 +- > 3 files changed, 162 insertions(+), 75 deletions(-) > Series Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 1528374154-14680-1-git-send-email-thuth@redhat.com Subject: [Qemu-devel] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu 5d328d7d2f..8e36d27c5a master -> master * [new tag] patchew/1528374154-14680-1-git-send-email-thuth@redhat.com -> patchew/1528374154-14680-1-git-send-email-thuth@redhat.com * [new tag] patchew/1528403460-30526-1-git-send-email-gengdongjiu@huawei.com -> patchew/1528403460-30526-1-git-send-email-gengdongjiu@huawei.com t [tag update] patchew/20180530180120.13355-1-richard.henderson@linaro.org -> patchew/20180530180120.13355-1-richard.henderson@linaro.org Switched to a new branch 'test' 132ffa3587 pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the UUID b3b73613ba pc-bios/s390-ccw/net: Add support for pxelinux-style config files 699020402a pc-bios/s390-ccw/net: Update code for the latest changes in SLOF 5dd76a9c89 roms: Update SLOF submodule to current status === OUTPUT BEGIN === Checking PATCH 1/4: roms: Update SLOF submodule to current status... Checking PATCH 2/4: pc-bios/s390-ccw/net: Update code for the latest changes in SLOF... Checking PATCH 3/4: pc-bios/s390-ccw/net: Add support for pxelinux-style config files... Checking PATCH 4/4: pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the UUID... ERROR: space prohibited before open square bracket '[' #57: FILE: pc-bios/s390-ccw/netmain.c:267: + : [cc] "=d" (cc) total: 1 errors, 0 warnings, 74 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On 07.06.2018 14:31, no-reply@patchew.org wrote: > Hi, > > This series seems to have some coding style problems. See output below for > more information: > > Type: series > Message-id: 1528374154-14680-1-git-send-email-thuth@redhat.com > Subject: [Qemu-devel] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg > > === TEST SCRIPT BEGIN === > #!/bin/bash > > BASE=base > n=1 > total=$(git log --oneline $BASE.. | wc -l) > failed=0 > > git config --local diff.renamelimit 0 > git config --local diff.renames True > git config --local diff.algorithm histogram > > commits="$(git log --format=%H --reverse $BASE..)" > for c in $commits; do > echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." > if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then > failed=1 > echo > fi > n=$((n+1)) > done > > exit $failed > === TEST SCRIPT END === > > Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 > From https://github.com/patchew-project/qemu > 5d328d7d2f..8e36d27c5a master -> master > * [new tag] patchew/1528374154-14680-1-git-send-email-thuth@redhat.com -> patchew/1528374154-14680-1-git-send-email-thuth@redhat.com > * [new tag] patchew/1528403460-30526-1-git-send-email-gengdongjiu@huawei.com -> patchew/1528403460-30526-1-git-send-email-gengdongjiu@huawei.com > t [tag update] patchew/20180530180120.13355-1-richard.henderson@linaro.org -> patchew/20180530180120.13355-1-richard.henderson@linaro.org > Switched to a new branch 'test' > 132ffa3587 pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the UUID > b3b73613ba pc-bios/s390-ccw/net: Add support for pxelinux-style config files > 699020402a pc-bios/s390-ccw/net: Update code for the latest changes in SLOF > 5dd76a9c89 roms: Update SLOF submodule to current status > > === OUTPUT BEGIN === > Checking PATCH 1/4: roms: Update SLOF submodule to current status... > Checking PATCH 2/4: pc-bios/s390-ccw/net: Update code for the latest changes in SLOF... > Checking PATCH 3/4: pc-bios/s390-ccw/net: Add support for pxelinux-style config files... > Checking PATCH 4/4: pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the UUID... > ERROR: space prohibited before open square bracket '[' > #57: FILE: pc-bios/s390-ccw/netmain.c:267: > + : [cc] "=d" (cc) Patchew obviously does not know about inline assembly ... I think I prefer to keep the space here. Thomas
On 07.06.2018 14:22, Thomas Huth wrote: > This patch series adds pxelinux.cfg-style network booting to the s390-ccw > firmware. The core pxelinux.cfg loading and parsing logic has recently > been merged to SLOF, so these patches now just have to make sure to call > the right functions to get the config file loaded and parsed. Once this is > done, the kernel and initrd are loaded separately, and are then glued > together in RAM. > > v2: > - Update SLOF submodule now that the git mirror is in sync again > - Last parameter to tftp_get_error_info() must not be NULL > - Check CC when calling STSI, and use a #define for the UUID offset > - Only support files with the magic "# pxelinux" string comment when > trying to guess the contents of a file that has been downloaded via > the "bootfile" DHCP parameter. This is just for developers' convenience, > the official way to specify pxelinux.cfg files is to use the DHCP > options 209 and 210 instead. > > Thomas Huth (4): > roms: Update SLOF submodule to current status > pc-bios/s390-ccw/net: Update code for the latest changes in SLOF > pc-bios/s390-ccw/net: Add support for pxelinux-style config files > pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the > UUID > > pc-bios/s390-ccw/netboot.mak | 9 +- > pc-bios/s390-ccw/netmain.c | 226 +++++++++++++++++++++++++++++-------------- > roms/SLOF | 2 +- > 3 files changed, 162 insertions(+), 75 deletions(-) > I tested the series both with a self-created pxelinux.0 blob (to verify backward compatibility) and without an existing pxelinux.0 file (to force the standard pxelinux pattern). Both worked as expected, although the built-in search took significantly longer (timeout?). Tested-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> -- Regards, Viktor Mihajlovski
On 11.06.2018 11:08, Viktor VM Mihajlovski wrote: > On 07.06.2018 14:22, Thomas Huth wrote: >> This patch series adds pxelinux.cfg-style network booting to the s390-ccw >> firmware. The core pxelinux.cfg loading and parsing logic has recently >> been merged to SLOF, so these patches now just have to make sure to call >> the right functions to get the config file loaded and parsed. Once this is >> done, the kernel and initrd are loaded separately, and are then glued >> together in RAM. >> >> v2: >> - Update SLOF submodule now that the git mirror is in sync again >> - Last parameter to tftp_get_error_info() must not be NULL >> - Check CC when calling STSI, and use a #define for the UUID offset >> - Only support files with the magic "# pxelinux" string comment when >> trying to guess the contents of a file that has been downloaded via >> the "bootfile" DHCP parameter. This is just for developers' convenience, >> the official way to specify pxelinux.cfg files is to use the DHCP >> options 209 and 210 instead. >> >> Thomas Huth (4): >> roms: Update SLOF submodule to current status >> pc-bios/s390-ccw/net: Update code for the latest changes in SLOF >> pc-bios/s390-ccw/net: Add support for pxelinux-style config files >> pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the >> UUID >> >> pc-bios/s390-ccw/netboot.mak | 9 +- >> pc-bios/s390-ccw/netmain.c | 226 +++++++++++++++++++++++++++++-------------- >> roms/SLOF | 2 +- >> 3 files changed, 162 insertions(+), 75 deletions(-) >> > I tested the series both with a self-created pxelinux.0 blob (to verify > backward compatibility) and without an existing pxelinux.0 file (to > force the standard pxelinux pattern). Both worked as expected, although > the built-in search took significantly longer (timeout?). > > Tested-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Thanks a lot for the testing! Hmm, I've got no real clue why there should be a big difference in the amount of time here ... is there maybe a lot of unrelated broadcast network traffic from other hosts going on in that network where you've tested it? It could be that the virtio-net driver of the s390-ccw bios can not deal with that situation very well yet. You could try to increase the "64" in virtio_net_init() in pc-bios/s390-ccw/virtio-net.c to see whether it makes a difference. Or if you've got some spare time, could you maybe run Wireshark on the server side to have a look at the time-stamps of the related packets, and to see whether there are duplicated TFTP read requests? This could indicate that the firmware missed a packet and thus ran into a timeout. Thomas PS: After I posted v1 of the patch set, you reported on IRC that you saw a problem with a corrupted initrd download or something similar. Is that problem now fixed for you?
On 11.06.2018 13:12, Thomas Huth wrote: > On 11.06.2018 11:08, Viktor VM Mihajlovski wrote: >> On 07.06.2018 14:22, Thomas Huth wrote: >>> This patch series adds pxelinux.cfg-style network booting to the s390-ccw >>> firmware. The core pxelinux.cfg loading and parsing logic has recently >>> been merged to SLOF, so these patches now just have to make sure to call >>> the right functions to get the config file loaded and parsed. Once this is >>> done, the kernel and initrd are loaded separately, and are then glued >>> together in RAM. >>> >>> v2: >>> - Update SLOF submodule now that the git mirror is in sync again >>> - Last parameter to tftp_get_error_info() must not be NULL >>> - Check CC when calling STSI, and use a #define for the UUID offset >>> - Only support files with the magic "# pxelinux" string comment when >>> trying to guess the contents of a file that has been downloaded via >>> the "bootfile" DHCP parameter. This is just for developers' convenience, >>> the official way to specify pxelinux.cfg files is to use the DHCP >>> options 209 and 210 instead. >>> >>> Thomas Huth (4): >>> roms: Update SLOF submodule to current status >>> pc-bios/s390-ccw/net: Update code for the latest changes in SLOF >>> pc-bios/s390-ccw/net: Add support for pxelinux-style config files >>> pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the >>> UUID >>> >>> pc-bios/s390-ccw/netboot.mak | 9 +- >>> pc-bios/s390-ccw/netmain.c | 226 +++++++++++++++++++++++++++++-------------- >>> roms/SLOF | 2 +- >>> 3 files changed, 162 insertions(+), 75 deletions(-) >>> >> I tested the series both with a self-created pxelinux.0 blob (to verify >> backward compatibility) and without an existing pxelinux.0 file (to >> force the standard pxelinux pattern). Both worked as expected, although >> the built-in search took significantly longer (timeout?). >> >> Tested-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> > > Thanks a lot for the testing! > > Hmm, I've got no real clue why there should be a big difference in the > amount of time here ... is there maybe a lot of unrelated broadcast > network traffic from other hosts going on in that network where you've > tested it? It could be that the virtio-net driver of the s390-ccw bios > can not deal with that situation very well yet. You could try to > increase the "64" in virtio_net_init() in pc-bios/s390-ccw/virtio-net.c > to see whether it makes a difference. Or if you've got some spare time, > could you maybe run Wireshark on the server side to have a look at the > time-stamps of the related packets, and to see whether there are > duplicated TFTP read requests? This could indicate that the firmware > missed a packet and thus ran into a timeout. FWIW: I'm using the isolated libvirt network on an otherwise idle host, so it's unlikely that there's interference. If I find the time I'll have a look at the traffic. > > Thomas > > PS: After I posted v1 of the patch set, you reported on IRC that you saw > a problem with a corrupted initrd download or something similar. Is that > problem now fixed for you? > Yes, it was my bad. I used an older kernel with the following problem https://lkml.org/lkml/2017/3/13/683. -- Regards, Viktor Mihajlovski
On 11.06.2018 14:03, Viktor VM Mihajlovski wrote: > On 11.06.2018 13:12, Thomas Huth wrote: >> On 11.06.2018 11:08, Viktor VM Mihajlovski wrote: >>> On 07.06.2018 14:22, Thomas Huth wrote: >>>> This patch series adds pxelinux.cfg-style network booting to the s390-ccw >>>> firmware. The core pxelinux.cfg loading and parsing logic has recently >>>> been merged to SLOF, so these patches now just have to make sure to call >>>> the right functions to get the config file loaded and parsed. Once this is >>>> done, the kernel and initrd are loaded separately, and are then glued >>>> together in RAM. >>>> >>>> v2: >>>> - Update SLOF submodule now that the git mirror is in sync again >>>> - Last parameter to tftp_get_error_info() must not be NULL >>>> - Check CC when calling STSI, and use a #define for the UUID offset >>>> - Only support files with the magic "# pxelinux" string comment when >>>> trying to guess the contents of a file that has been downloaded via >>>> the "bootfile" DHCP parameter. This is just for developers' convenience, >>>> the official way to specify pxelinux.cfg files is to use the DHCP >>>> options 209 and 210 instead. >>>> >>>> Thomas Huth (4): >>>> roms: Update SLOF submodule to current status >>>> pc-bios/s390-ccw/net: Update code for the latest changes in SLOF >>>> pc-bios/s390-ccw/net: Add support for pxelinux-style config files >>>> pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the >>>> UUID >>>> >>>> pc-bios/s390-ccw/netboot.mak | 9 +- >>>> pc-bios/s390-ccw/netmain.c | 226 +++++++++++++++++++++++++++++-------------- >>>> roms/SLOF | 2 +- >>>> 3 files changed, 162 insertions(+), 75 deletions(-) >>>> >>> I tested the series both with a self-created pxelinux.0 blob (to verify >>> backward compatibility) and without an existing pxelinux.0 file (to >>> force the standard pxelinux pattern). Both worked as expected, although >>> the built-in search took significantly longer (timeout?). >>> >>> Tested-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> >> >> Thanks a lot for the testing! >> >> Hmm, I've got no real clue why there should be a big difference in the >> amount of time here ... is there maybe a lot of unrelated broadcast >> network traffic from other hosts going on in that network where you've >> tested it? It could be that the virtio-net driver of the s390-ccw bios >> can not deal with that situation very well yet. You could try to >> increase the "64" in virtio_net_init() in pc-bios/s390-ccw/virtio-net.c >> to see whether it makes a difference. Or if you've got some spare time, >> could you maybe run Wireshark on the server side to have a look at the >> time-stamps of the related packets, and to see whether there are >> duplicated TFTP read requests? This could indicate that the firmware >> missed a packet and thus ran into a timeout. > FWIW: I'm using the isolated libvirt network on an otherwise idle host, > so it's unlikely that there's interference. If I find the time I'll have > a look at the traffic. If you have the time to look at the traffic, could you please also check the TFTP block size option that is negotiated at the beginning of the TFTP transfer? If this other client is negotiating a transfer block size that is bigger than the one from the s390-ccw firmware, this could explain the differences in the downloading time, too. libnet from SLOF currently uses a block size of 1428. This is the size where all TFTP data should still fit nicely into one ethernet packet - and this is also the size which is still supported by all TFTP servers that support the blksize option. But theoretically it's also possible to use a bigger block sizes if both, the server and the client support fragmented UDP packets. Unfortunately, as far as I can see, SLOF's libnet does not support fragmented UDP packets, so we can't increase the block size here anymore so easily. Can you tell which tftp client you are using in your other pxelinux.0 blob? (busybox tftp? HPA-tftp ?) Thomas
On 12.06.2018 08:17, Thomas Huth wrote: > On 11.06.2018 14:03, Viktor VM Mihajlovski wrote: [...] > > If you have the time to look at the traffic, could you please also check > the TFTP block size option that is negotiated at the beginning of the > TFTP transfer? If this other client is negotiating a transfer block size > that is bigger than the one from the s390-ccw firmware, this could > explain the differences in the downloading time, too. > > libnet from SLOF currently uses a block size of 1428. This is the size > where all TFTP data should still fit nicely into one ethernet packet - > and this is also the size which is still supported by all TFTP servers > that support the blksize option. But theoretically it's also possible to > use a bigger block sizes if both, the server and the client support > fragmented UDP packets. Unfortunately, as far as I can see, SLOF's > libnet does not support fragmented UDP packets, so we can't increase the > block size here anymore so easily. You will be pleased to hear that the SLOF TFTP client outperforms the busybox version (which uses 544-byte packets) by 30%. There's some randomness introduced by the differences in DHCP response times which is clearly not the client's fault. All is good... [...] -- Regards, Viktor Mihajlovski
On 13.06.2018 12:56, Viktor VM Mihajlovski wrote: > On 12.06.2018 08:17, Thomas Huth wrote: >> On 11.06.2018 14:03, Viktor VM Mihajlovski wrote: > [...] >> >> If you have the time to look at the traffic, could you please also check >> the TFTP block size option that is negotiated at the beginning of the >> TFTP transfer? If this other client is negotiating a transfer block size >> that is bigger than the one from the s390-ccw firmware, this could >> explain the differences in the downloading time, too. >> >> libnet from SLOF currently uses a block size of 1428. This is the size >> where all TFTP data should still fit nicely into one ethernet packet - >> and this is also the size which is still supported by all TFTP servers >> that support the blksize option. But theoretically it's also possible to >> use a bigger block sizes if both, the server and the client support >> fragmented UDP packets. Unfortunately, as far as I can see, SLOF's >> libnet does not support fragmented UDP packets, so we can't increase the >> block size here anymore so easily. > > You will be pleased to hear that the SLOF TFTP client outperforms the > busybox version (which uses 544-byte packets) by 30%. There's some > randomness introduced by the differences in DHCP response times which is > clearly not the client's fault. All is good... Ah, great, thanks for checking! So I'm going to prepare a pull request for this next... Thomas
© 2016 - 2024 Red Hat, Inc.