[Qemu-devel] [PATCH v1 for-2.13 0/4] pc-bios/s390-ccw: Network boot improvements

Thomas Huth posted 4 patches 5 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1524054707-20663-1-git-send-email-thuth@redhat.com
Test checkpatch passed
Test docker-build@min-glib passed
Test docker-mingw@fedora passed
Test s390x passed
There is a newer version of this series
pc-bios/s390-ccw/netboot.mak  |   5 +-
pc-bios/s390-ccw/netmain.c    | 312 ++++++++++++++++++++++++++++++++++++++----
pc-bios/s390-ccw/virtio-net.c |   8 ++
pc-bios/s390-ccw/virtio.c     |  19 ++-
pc-bios/s390-ccw/virtio.h     |   3 +
5 files changed, 312 insertions(+), 35 deletions(-)
[Qemu-devel] [PATCH v1 for-2.13 0/4] pc-bios/s390-ccw: Network boot improvements
Posted by Thomas Huth 5 years, 11 months ago
Some patches to improve the network boot experience on s390x:

First, make sure that we shut down the virtio-net device before jumping
into the kernel. Otherwise some incoming packets might destroy some of
the kernel's data if it has not taken over the device yet.

Then the last two patches add support for loading kernels via
configuration files - pxelinux-style and .INS-file style. This way
you don't have to manually glue your ramdisk to your kernel anymore,
so this should be quite a relieve for all users who want to boot
Linux via the network.

The config file parsers have been completely written by myself from
scratch and only tested with some config files that I came up with
on my own. So if anybody has some pre-existing pxelinux config files
already for booting a s390x, I'd appreciate some testing to see whether
this works as expected for you, too!

Thomas Huth (4):
  pc-bios/s390-ccw/net: Split up net_load() into init, load and uninit
    parts
  pc-bios/s390-ccw/net: Stop virtio-net device before jumping into the
    OS
  pc-bios/s390-ccw/net: Add support for pxelinux-style config files
  pc-bios/s390-ccw/net: Add support for .INS config files

 pc-bios/s390-ccw/netboot.mak  |   5 +-
 pc-bios/s390-ccw/netmain.c    | 312 ++++++++++++++++++++++++++++++++++++++----
 pc-bios/s390-ccw/virtio-net.c |   8 ++
 pc-bios/s390-ccw/virtio.c     |  19 ++-
 pc-bios/s390-ccw/virtio.h     |   3 +
 5 files changed, 312 insertions(+), 35 deletions(-)

-- 
1.8.3.1


Re: [Qemu-devel] [PATCH v1 for-2.13 0/4] pc-bios/s390-ccw: Network boot improvements
Posted by Farhan Ali 5 years, 11 months ago

On 04/18/2018 08:31 AM, Thomas Huth wrote:
> Some patches to improve the network boot experience on s390x:
> 
> First, make sure that we shut down the virtio-net device before jumping
> into the kernel. Otherwise some incoming packets might destroy some of
> the kernel's data if it has not taken over the device yet.
> 
> Then the last two patches add support for loading kernels via
> configuration files - pxelinux-style and .INS-file style. This way
> you don't have to manually glue your ramdisk to your kernel anymore,
> so this should be quite a relieve for all users who want to boot
> Linux via the network.
> 
> The config file parsers have been completely written by myself from
> scratch and only tested with some config files that I came up with
> on my own. So if anybody has some pre-existing pxelinux config files
> already for booting a s390x, I'd appreciate some testing to see whether
> this works as expected for you, too!
> 
> Thomas Huth (4):
>    pc-bios/s390-ccw/net: Split up net_load() into init, load and uninit
>      parts
>    pc-bios/s390-ccw/net: Stop virtio-net device before jumping into the
>      OS
>    pc-bios/s390-ccw/net: Add support for pxelinux-style config files
>    pc-bios/s390-ccw/net: Add support for .INS config files
> 
>   pc-bios/s390-ccw/netboot.mak  |   5 +-
>   pc-bios/s390-ccw/netmain.c    | 312 ++++++++++++++++++++++++++++++++++++++----
>   pc-bios/s390-ccw/virtio-net.c |   8 ++
>   pc-bios/s390-ccw/virtio.c     |  19 ++-
>   pc-bios/s390-ccw/virtio.h     |   3 +
>   5 files changed, 312 insertions(+), 35 deletions(-)
> 


I have tried with pxelinux default config file I had and it worked fine. 
I will try a couple more tests.

Also while going through the code again, I noticed a memory leak in the 
function virtio_net_init, and fixed it. I could send a formal patch for 
it, if you want to queue it as part of this series as I think we might 
have missed 2.12 window?

diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
index ff7f4da..e83ba08 100644
--- a/pc-bios/s390-ccw/virtio-net.c
+++ b/pc-bios/s390-ccw/virtio-net.c
@@ -61,6 +61,7 @@ int virtio_net_init(void *mac_addr)
          IPL_assert(buf != NULL, "Can not allocate memory for receive 
buffers");
          vring_send_buf(rxvq, buf, ETH_MTU_SIZE + sizeof(VirtioNetHdr),
                         VRING_DESC_F_WRITE);
+        free(buf);
      }
      vring_notify(rxvq);


Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.13 0/4] pc-bios/s390-ccw: Network boot improvements
Posted by Thomas Huth 5 years, 11 months ago
On 18.04.2018 20:21, Farhan Ali wrote:
> On 04/18/2018 08:31 AM, Thomas Huth wrote:
>> Some patches to improve the network boot experience on s390x:
>>
>> First, make sure that we shut down the virtio-net device before jumping
>> into the kernel. Otherwise some incoming packets might destroy some of
>> the kernel's data if it has not taken over the device yet.
>>
>> Then the last two patches add support for loading kernels via
>> configuration files - pxelinux-style and .INS-file style. This way
>> you don't have to manually glue your ramdisk to your kernel anymore,
>> so this should be quite a relieve for all users who want to boot
>> Linux via the network.
>>
>> The config file parsers have been completely written by myself from
>> scratch and only tested with some config files that I came up with
>> on my own. So if anybody has some pre-existing pxelinux config files
>> already for booting a s390x, I'd appreciate some testing to see whether
>> this works as expected for you, too!
[...]
>
> I have tried with pxelinux default config file I had and it worked fine.
> I will try a couple more tests.

Thanks!

> Also while going through the code again, I noticed a memory leak in the
> function virtio_net_init, and fixed it. I could send a formal patch for
> it, if you want to queue it as part of this series as I think we might
> have missed 2.12 window?
> 
> diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
> index ff7f4da..e83ba08 100644
> --- a/pc-bios/s390-ccw/virtio-net.c
> +++ b/pc-bios/s390-ccw/virtio-net.c
> @@ -61,6 +61,7 @@ int virtio_net_init(void *mac_addr)
>          IPL_assert(buf != NULL, "Can not allocate memory for receive
> buffers");
>          vring_send_buf(rxvq, buf, ETH_MTU_SIZE + sizeof(VirtioNetHdr),
>                         VRING_DESC_F_WRITE);
> +        free(buf);
>      }
>      vring_notify(rxvq);

Ah, that's not a memory leak, though it might look like one if you don't
know what the vring_send_buf function is doing: vring_send_buf adds a
buffer to a virtio ring for later use. So the buffer is not unused after
this function, but gets filled in later by the virtio-net device. Thus
we must not free the buffer here.

We could free it in the "uninit" function after shutting down the
device, but OTOH we are then leaving the firmware code anyway by jumping
into the OS kernel, so it does not really matter whether we free()ed the
buffers or not.

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.13 0/4] pc-bios/s390-ccw: Network boot improvements
Posted by Farhan Ali 5 years, 11 months ago

On 04/19/2018 01:27 AM, Thomas Huth wrote:
> Ah, that's not a memory leak, though it might look like one if you don't
> know what the vring_send_buf function is doing: vring_send_buf adds a
> buffer to a virtio ring for later use. So the buffer is not unused after
> this function, but gets filled in later by the virtio-net device. Thus
> we must not free the buffer here.
> 
> We could free it in the "uninit" function after shutting down the
> device, but OTOH we are then leaving the firmware code anyway by jumping
> into the OS kernel, so it does not really matter whether we free()ed the
> buffers or not.
> 
>   Thomas

That makes a lot of sense now :) thanks a lot for that explanation and 
sorry for my ignorance.