Re: [Qemu-devel] [PATCH v2 3/5] s390x/ipl: Load network boot image

[Thomas Huth <thuth@redhat.com>]

On 27.02.2017 12:51, Cornelia Huck wrote:
> On Sat, 25 Feb 2017 07:18:29 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 23.02.2017 13:20, Cornelia Huck wrote:
>>> From: Farhan Ali <alifm@linux.vnet.ibm.com>
>>>
>>> Load the network boot image into guest RAM when the boot
>>> device selected is a network device. Use some of the reserved
>>> space in IplBlockCcw to store the start address of the netboot
>>> image.
>>>
>>> A user could also use 'chreipl'(diag 308/5) to change the boot device.
>>> So every time we update the IPLB, we need to verify if the selected
>>> boot device is a network device so we can appropriately load the
>>> network boot image.
>>>
>>> Signed-off-by: Farhan Ali <alifm@linux.vnet.ibm.com>
>>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>>> ---
>>>  hw/s390x/ipl.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  hw/s390x/ipl.h |  4 ++-
>>>  2 files changed, 92 insertions(+), 1 deletion(-)
[...]
>>> +    if (ipl->netboot) {
>>> +        if (load_netboot_image(&err) < 0) {
>>> +            error_report_err(err);
>>> +            vm_stop(RUN_STATE_INTERNAL_ERROR);
>>> +        }
>>> +        ipl->iplb.ccw.netboot_start_addr = ipl->start_addr;
>>
>> Not sure whether it matters, but in case of early errors during
>> load_netboot_image(), ipl->start_addr could be used uninitialized here.
>> Maybe you should move the "ipl->start_addr = KERN_IMAGE_START;" there at
>> the beginning of the function, to make it also the default value for the
>> other error cases?
> 
> ipl->start_addr has already been set to some value in the realize
> function (either the kernel entry address, or the bios address).
> 
> But that should not matter with the vm_stop() on error anyway, no?

Likely not. It's just a little bit strange to see that the program flow
continues after the error in this function here ... maybe it would have
been clearer to put a "return" right after the "vm_stop()"? Anyway, it
likely does not really matter here, so never mind.

 Thomas