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

[Thomas Huth <thuth@redhat.com>]

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