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

[Cornelia Huck <cornelia.huck@de.ibm.com>]

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(-)

> > +static bool is_virtio_net_device(IplParameterBlock *iplb)
> > +{
> > +    uint8_t cssid;
> > +    uint8_t ssid;
> > +    uint16_t devno;
> > +    uint16_t schid;
> > +    SubchDev *sch = NULL;
> > +
> > +    if (iplb->pbt != S390_IPL_TYPE_CCW) {
> > +        return false;
> > +    }
> > +
> > +    devno = be16_to_cpu(iplb->ccw.devno);
> > +    ssid = iplb->ccw.ssid & 3;
> > +
> > +    for (schid = 0; schid < MAX_SCHID; schid++) {
> > +        for (cssid = 0; cssid < MAX_CSSID; cssid++) {
> > +            sch = css_find_subch(1, cssid, ssid, schid);
> > +
> > +            if (sch && sch->devno == devno) {
> > +                return sch->id.cu_model == VIRTIO_ID_NET;
> > +            }
> > +        }
> > +    }
> > +   return false;
> 
> The above line has only 3 instead of 4 spaces. I wonder why checkpatch
> does not complain here...?

Odd. Will fixup.

> 
> > +}

> > +    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?