|
From: | Farhan Ali |
Subject: | Re: [Qemu-devel] [PATCH 3/5] s390x/ipl: Load network boot image |
Date: | Wed, 22 Feb 2017 10:01:01 -0500 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 |
Hi Thomas, Thanks for the review. On 02/20/2017 10:28 AM, Thomas Huth wrote:
On 20.02.2017 15:19, Cornelia Huck wrote:From: Farhan Ali <address@hidden> 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 <address@hidden> Signed-off-by: Cornelia Huck <address@hidden> --- hw/s390x/ipl.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/s390x/ipl.h | 4 ++- 2 files changed, 89 insertions(+), 1 deletion(-) diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index fd656718a7..80e05cc7a5 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -20,6 +20,7 @@ #include "hw/s390x/virtio-ccw.h" #include "hw/s390x/css.h" #include "ipl.h" +#include "qemu/error-report.h" #define KERN_IMAGE_START 0x010000UL #define KERN_PARM_AREA 0x010480UL @@ -227,6 +228,12 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) TYPE_VIRTIO_CCW_DEVICE); SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st), TYPE_SCSI_DEVICE); + VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st), + TYPE_VIRTIO_NET); + + if (vn) { + ipl->netboot = true; + } if (virtio_ccw_dev) { CcwDevice *ccw_dev = CCW_DEVICE(virtio_ccw_dev); @@ -259,12 +266,84 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) return false; } +static int load_netboot_image(void) +{ +Please remove that empty line above.
Will do.
+ S390IPLState *ipl = get_ipl_device(); + char *netboot_filename; + MemoryRegion *sysmem = get_system_memory(); + MemoryRegion *mr = NULL; + void *ram_ptr = NULL; + int img_size = -1; + + mr = memory_region_find(sysmem, 0, 1).mr; + if (!mr) { + error_report("Failed to find memory region at address 0"); + goto out;<bikeshedpainting>I'd prefer a "return -1 here</bikeshedpainting>
will make the change.
+ } + + ram_ptr = memory_region_get_ram_ptr(mr); + if (!ram_ptr) { + error_report("No RAM found"); + goto unref_mr; + } + + netboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, ipl->netboot_fw); + if (netboot_filename == NULL) { + error_report("Could not find network bootloader"); + goto unref_mr; + }So you're doing error_report() here already ...+ img_size = load_elf_ram(netboot_filename, NULL, NULL, &ipl->start_addr, + NULL, NULL, 1, EM_S390, 0, 0, NULL, false); + + if (img_size < 0) { + img_size = load_image_size(netboot_filename, ram_ptr, ram_size); + ipl->start_addr = KERN_IMAGE_START; + } + + g_free(netboot_filename); + +unref_mr: + memory_region_unref(mr); +out: + return img_size; +} + +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; +} + void s390_ipl_update_diag308(IplParameterBlock *iplb) { S390IPLState *ipl = get_ipl_device(); ipl->iplb = *iplb; ipl->iplb_valid = true; + ipl->netboot = is_virtio_net_device(iplb); } IplParameterBlock *s390_ipl_get_iplb(void) @@ -298,6 +377,13 @@ void s390_ipl_prepare_cpu(S390CPU *cpu) ipl->iplb_valid = s390_gen_initial_iplb(ipl); } } + if (ipl->netboot) { + if (load_netboot_image() < 0) { + error_report("Failed to load network bootloader");... and then you do another error_report here again ... so one error gets reported with two error message. Wouldn't it be nicer to rather do error_setg(...) in load_netboot_image() and then report only one error at this level here?
What would be the advantage of doing that? Thanks Farhan
+ vm_stop(RUN_STATE_INTERNAL_ERROR); + } + ipl->iplb.ccw.netboot_start_addr = ipl->start_addr; + } } static void s390_ipl_reset(DeviceState *dev) diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h index 4ad9a7c05e..46930e4c64 100644 --- a/hw/s390x/ipl.h +++ b/hw/s390x/ipl.h @@ -16,7 +16,8 @@ #include "cpu.h" struct IplBlockCcw { - uint8_t reserved0[85]; + uint64_t netboot_start_addr; + uint8_t reserved0[77]; uint8_t ssid; uint16_t devno; uint8_t vm_flags; @@ -100,6 +101,7 @@ struct S390IPLState { IplParameterBlock iplb; bool iplb_valid; bool reipl_requested; + bool netboot; /*< public >*/ char *kernel;
[Prev in Thread] | Current Thread | [Next in Thread] |