| Message ID | 1517864246-11101-6-git-send-email-walling@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05.02.2018 21:57, Collin L. Walling wrote: > The s390-ccw firmware needs some information in support of the > boot process which is not available on the native machine. > Examples are the netboot firmware load address and now the > boot menu parameters. > > While storing that data in unused fields of the IPL parameter block > works, that approach could create problems if the parameter block > definition should change in the future. Because then a guest could > overwrite these fields using the set IPLB diagnose. > > In fact the data in question is of more global nature and not really > tied to an IPL device, so separating it is rather logical. > > This commit introduces a new structure to hold firmware relevant > IPL parameters set by QEMU. The data is stored at location 204 (dec) > and can contain up to 7 32-bit words. This area is available to > programming in the z/Architecture Principles of Operation and > can thus safely be used by the firmware until the IPL has completed. Sounds like a good idea. > Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> > --- > hw/s390x/ipl.c | 18 +++++++++++++++++- > hw/s390x/ipl.h | 11 +++++++++-- > pc-bios/s390-ccw/iplb.h | 12 ++++++++++-- > pc-bios/s390-ccw/main.c | 6 +++++- > 4 files changed, 41 insertions(+), 6 deletions(-) > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index 0d06fc1..3e3c3b8 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -399,6 +399,20 @@ void s390_reipl_request(void) > qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET); > } > > +static void s390_ipl_prepare_qipl(S390CPU *cpu) > +{ > + S390IPLState *ipl = get_ipl_device(); > + uint8_t *addr; > + uint64_t len = 4096; > + > + addr = cpu_physical_memory_map(cpu->env.psa, &len, 1); > + if (!addr || len < 204 + sizeof(QemuIplParameters)) { > + error_report("Cannot set QEMU IPL parameters"); I think you should return or exit() here. Otherwise the memcpy below accesses an illegal memory range. > + } > + memcpy(addr + 204, &ipl->iplb.qipl, sizeof(QemuIplParameters)); > + cpu_physical_memory_unmap(addr, len, 1, len); > +} > + > void s390_ipl_prepare_cpu(S390CPU *cpu) > { > S390IPLState *ipl = get_ipl_device(); > @@ -418,8 +432,10 @@ void s390_ipl_prepare_cpu(S390CPU *cpu) > error_report_err(err); > vm_stop(RUN_STATE_INTERNAL_ERROR); > } > - ipl->iplb.ccw.netboot_start_addr = cpu_to_be64(ipl->start_addr); > + ipl->iplb.qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr); > } > + s390_ipl_prepare_qipl(cpu); > + > } > > static void s390_ipl_reset(DeviceState *dev) > diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h > index 8a705e0..68dcaf8 100644 > --- a/hw/s390x/ipl.h > +++ b/hw/s390x/ipl.h > @@ -16,8 +16,7 @@ > #include "cpu.h" > > struct IplBlockCcw { > - uint64_t netboot_start_addr; > - uint8_t reserved0[77]; > + uint8_t reserved0[85]; > uint8_t ssid; > uint16_t devno; > uint8_t vm_flags; > @@ -59,6 +58,13 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; > > #define DIAG308_FLAGS_LP_VALID 0x80 > > +struct QemuIplParameters { > + uint8_t reserved1[4]; > + uint64_t netboot_start_addr; > + uint8_t reserved2[16]; > +} QEMU_PACKED; > +typedef struct QemuIplParameters QemuIplParameters; > + > union IplParameterBlock { > struct { > uint32_t len; > @@ -74,6 +80,7 @@ union IplParameterBlock { > IplBlockFcp fcp; > IplBlockQemuScsi scsi; > }; > + QemuIplParameters qipl; > } QEMU_PACKED; Sorry if I get that wrong, but if you store that within IplParameterBlock, the information is still passed via DIAG 0x308, isn't it? (see handle_diag_308(), case 6). Is that really what was intended here? [...] > diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c > index e857ce4..825a1a3 100644 > --- a/pc-bios/s390-ccw/main.c > +++ b/pc-bios/s390-ccw/main.c > @@ -16,6 +16,7 @@ char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE))); > static SubChannelId blk_schid = { .one = 1 }; > IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); > static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 }; > +QemuIplParameters qipl; > > /* > * Priniciples of Operations (SA22-7832-09) chapter 17 requires that > @@ -81,6 +82,7 @@ static void virtio_setup(void) > uint16_t dev_no; > char ldp[] = "LOADPARM=[________]\n"; > VDev *vdev = virtio_get_device(); > + QemuIplParameters *early_qipl = (QemuIplParameters *)204; Could you please introduce a proper #define for that magic 204 value (and use it in s390_ipl_prepare_qipl, too)? ... so that it is later easier to grep for this. > /* > * We unconditionally enable mss support. In every sane configuration, > @@ -93,6 +95,8 @@ static void virtio_setup(void) > memcpy(ldp + 10, loadparm, 8); > sclp_print(ldp); > > + memcpy(&qipl, early_qipl, sizeof(QemuIplParameters)); > + > if (store_iplb(&iplb)) { > switch (iplb.pbt) { > case S390_IPL_TYPE_CCW: > @@ -127,7 +131,7 @@ static void virtio_setup(void) > > if (virtio_get_device_type() == VIRTIO_ID_NET) { > sclp_print("Network boot device detected\n"); > - vdev->netboot_start_addr = iplb.ccw.netboot_start_addr; > + vdev->netboot_start_addr = qipl.netboot_start_addr; > } else { > virtio_blk_setup_device(blk_schid); > > Thomas
On 02/05/2018 09:57 PM, Collin L. Walling wrote: > The s390-ccw firmware needs some information in support of the > boot process which is not available on the native machine. > Examples are the netboot firmware load address and now the > boot menu parameters. > > While storing that data in unused fields of the IPL parameter block > works, that approach could create problems if the parameter block > definition should change in the future. Because then a guest could > overwrite these fields using the set IPLB diagnose. > > In fact the data in question is of more global nature and not really > tied to an IPL device, so separating it is rather logical. > > This commit introduces a new structure to hold firmware relevant > IPL parameters set by QEMU. The data is stored at location 204 (dec) > and can contain up to 7 32-bit words. This area is available to > programming in the z/Architecture Principles of Operation and > can thus safely be used by the firmware until the IPL has completed. This content is set by QEMU/the ccw bios on every IPL, so we do not have to change anything for migration. Correct? > > Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> > --- > hw/s390x/ipl.c | 18 +++++++++++++++++- > hw/s390x/ipl.h | 11 +++++++++-- > pc-bios/s390-ccw/iplb.h | 12 ++++++++++-- > pc-bios/s390-ccw/main.c | 6 +++++- > 4 files changed, 41 insertions(+), 6 deletions(-) > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index 0d06fc1..3e3c3b8 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -399,6 +399,20 @@ void s390_reipl_request(void) > qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET); > } > > +static void s390_ipl_prepare_qipl(S390CPU *cpu) > +{ > + S390IPLState *ipl = get_ipl_device(); > + uint8_t *addr; > + uint64_t len = 4096; > + > + addr = cpu_physical_memory_map(cpu->env.psa, &len, 1); > + if (!addr || len < 204 + sizeof(QemuIplParameters)) { > + error_report("Cannot set QEMU IPL parameters"); > + } > + memcpy(addr + 204, &ipl->iplb.qipl, sizeof(QemuIplParameters)); > + cpu_physical_memory_unmap(addr, len, 1, len); > +} > + > void s390_ipl_prepare_cpu(S390CPU *cpu) > { > S390IPLState *ipl = get_ipl_device(); > @@ -418,8 +432,10 @@ void s390_ipl_prepare_cpu(S390CPU *cpu) > error_report_err(err); > vm_stop(RUN_STATE_INTERNAL_ERROR); > } > - ipl->iplb.ccw.netboot_start_addr = cpu_to_be64(ipl->start_addr); > + ipl->iplb.qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr); > } > + s390_ipl_prepare_qipl(cpu); > + > } > > static void s390_ipl_reset(DeviceState *dev) > diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h > index 8a705e0..68dcaf8 100644 > --- a/hw/s390x/ipl.h > +++ b/hw/s390x/ipl.h > @@ -16,8 +16,7 @@ > #include "cpu.h" > > struct IplBlockCcw { > - uint64_t netboot_start_addr; > - uint8_t reserved0[77]; > + uint8_t reserved0[85]; > uint8_t ssid; > uint16_t devno; > uint8_t vm_flags; > @@ -59,6 +58,13 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; > > #define DIAG308_FLAGS_LP_VALID 0x80 > > +struct QemuIplParameters { > + uint8_t reserved1[4]; > + uint64_t netboot_start_addr; > + uint8_t reserved2[16]; > +} QEMU_PACKED; > +typedef struct QemuIplParameters QemuIplParameters; > + > union IplParameterBlock { > struct { > uint32_t len; > @@ -74,6 +80,7 @@ union IplParameterBlock { > IplBlockFcp fcp; > IplBlockQemuScsi scsi; > }; > + QemuIplParameters qipl; > } QEMU_PACKED; > struct { > uint8_t reserved1[110]; > diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h > index 890aed9..12f6e40 100644 > --- a/pc-bios/s390-ccw/iplb.h > +++ b/pc-bios/s390-ccw/iplb.h > @@ -13,8 +13,7 @@ > #define IPLB_H > > struct IplBlockCcw { > - uint64_t netboot_start_addr; > - uint8_t reserved0[77]; > + uint8_t reserved0[85]; > uint8_t ssid; > uint16_t devno; > uint8_t vm_flags; > @@ -73,6 +72,15 @@ typedef struct IplParameterBlock IplParameterBlock; > > extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); > > +struct QemuIplParameters { > + uint8_t reserved1[4]; > + uint64_t netboot_start_addr; > + uint8_t reserved2[16]; > +} __attribute__ ((packed)); > +typedef struct QemuIplParameters QemuIplParameters; > + > +extern QemuIplParameters qipl; > + > #define S390_IPL_TYPE_FCP 0x00 > #define S390_IPL_TYPE_CCW 0x02 > #define S390_IPL_TYPE_QEMU_SCSI 0xff > diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c > index e857ce4..825a1a3 100644 > --- a/pc-bios/s390-ccw/main.c > +++ b/pc-bios/s390-ccw/main.c > @@ -16,6 +16,7 @@ char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE))); > static SubChannelId blk_schid = { .one = 1 }; > IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); > static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 }; > +QemuIplParameters qipl; > > /* > * Priniciples of Operations (SA22-7832-09) chapter 17 requires that > @@ -81,6 +82,7 @@ static void virtio_setup(void) > uint16_t dev_no; > char ldp[] = "LOADPARM=[________]\n"; > VDev *vdev = virtio_get_device(); > + QemuIplParameters *early_qipl = (QemuIplParameters *)204; > > /* > * We unconditionally enable mss support. In every sane configuration, > @@ -93,6 +95,8 @@ static void virtio_setup(void) > memcpy(ldp + 10, loadparm, 8); > sclp_print(ldp); > > + memcpy(&qipl, early_qipl, sizeof(QemuIplParameters)); > + > if (store_iplb(&iplb)) { > switch (iplb.pbt) { > case S390_IPL_TYPE_CCW: > @@ -127,7 +131,7 @@ static void virtio_setup(void) > > if (virtio_get_device_type() == VIRTIO_ID_NET) { > sclp_print("Network boot device detected\n"); > - vdev->netboot_start_addr = iplb.ccw.netboot_start_addr; > + vdev->netboot_start_addr = qipl.netboot_start_addr; > } else { > virtio_blk_setup_device(blk_schid); >
On 06.02.2018 10:45, Christian Borntraeger wrote: > On 02/05/2018 09:57 PM, Collin L. Walling wrote: >> The s390-ccw firmware needs some information in support of the >> boot process which is not available on the native machine. >> Examples are the netboot firmware load address and now the >> boot menu parameters. >> >> While storing that data in unused fields of the IPL parameter block >> works, that approach could create problems if the parameter block >> definition should change in the future. Because then a guest could >> overwrite these fields using the set IPLB diagnose. >> >> In fact the data in question is of more global nature and not really >> tied to an IPL device, so separating it is rather logical. >> >> This commit introduces a new structure to hold firmware relevant >> IPL parameters set by QEMU. The data is stored at location 204 (dec) >> and can contain up to 7 32-bit words. This area is available to >> programming in the z/Architecture Principles of Operation and >> can thus safely be used by the firmware until the IPL has completed. > > This content is set by QEMU/the ccw bios on every IPL, so we do not > have to change anything for migration. Correct? > Hm ... this is true for the netboot start address, but I am not sure about the boot menu parameters added in a follow on patch. I'll comment there... [...]
On 06.02.2018 10:23, Thomas Huth wrote: > On 05.02.2018 21:57, Collin L. Walling wrote: >> The s390-ccw firmware needs some information in support of the >> boot process which is not available on the native machine. >> Examples are the netboot firmware load address and now the >> boot menu parameters. >> >> While storing that data in unused fields of the IPL parameter block >> works, that approach could create problems if the parameter block >> definition should change in the future. Because then a guest could >> overwrite these fields using the set IPLB diagnose. >> >> In fact the data in question is of more global nature and not really >> tied to an IPL device, so separating it is rather logical. >> >> This commit introduces a new structure to hold firmware relevant >> IPL parameters set by QEMU. The data is stored at location 204 (dec) >> and can contain up to 7 32-bit words. This area is available to >> programming in the z/Architecture Principles of Operation and >> can thus safely be used by the firmware until the IPL has completed. > > Sounds like a good idea. > >> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> >> --- >> hw/s390x/ipl.c | 18 +++++++++++++++++- >> hw/s390x/ipl.h | 11 +++++++++-- >> pc-bios/s390-ccw/iplb.h | 12 ++++++++++-- >> pc-bios/s390-ccw/main.c | 6 +++++- >> 4 files changed, 41 insertions(+), 6 deletions(-) >> >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >> index 0d06fc1..3e3c3b8 100644 >> --- a/hw/s390x/ipl.c >> +++ b/hw/s390x/ipl.c >> @@ -399,6 +399,20 @@ void s390_reipl_request(void) >> qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET); >> } >> >> +static void s390_ipl_prepare_qipl(S390CPU *cpu) >> +{ >> + S390IPLState *ipl = get_ipl_device(); >> + uint8_t *addr; >> + uint64_t len = 4096; >> + >> + addr = cpu_physical_memory_map(cpu->env.psa, &len, 1); >> + if (!addr || len < 204 + sizeof(QemuIplParameters)) { >> + error_report("Cannot set QEMU IPL parameters"); > > I think you should return or exit() here. Otherwise the memcpy below > accesses an illegal memory range.Right, I have noticed and fixed that on my private branch, but forgot to update the patch. Collin, could you squash in a return; > >> + } >> + memcpy(addr + 204, &ipl->iplb.qipl, sizeof(QemuIplParameters)); >> + cpu_physical_memory_unmap(addr, len, 1, len); >> +} >> + [...] >> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h >> index 8a705e0..68dcaf8 100644 >> --- a/hw/s390x/ipl.h >> +++ b/hw/s390x/ipl.h >> @@ -16,8 +16,7 @@ >> #include "cpu.h" >> >> struct IplBlockCcw { >> - uint64_t netboot_start_addr; >> - uint8_t reserved0[77]; >> + uint8_t reserved0[85]; >> uint8_t ssid; >> uint16_t devno; >> uint8_t vm_flags; >> @@ -59,6 +58,13 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; >> >> #define DIAG308_FLAGS_LP_VALID 0x80 >> >> +struct QemuIplParameters { >> + uint8_t reserved1[4]; >> + uint64_t netboot_start_addr; >> + uint8_t reserved2[16]; >> +} QEMU_PACKED; >> +typedef struct QemuIplParameters QemuIplParameters; >> + >> union IplParameterBlock { >> struct { >> uint32_t len; >> @@ -74,6 +80,7 @@ union IplParameterBlock { >> IplBlockFcp fcp; >> IplBlockQemuScsi scsi; >> }; >> + QemuIplParameters qipl; >> } QEMU_PACKED; > > Sorry if I get that wrong, but if you store that within > IplParameterBlock, the information is still passed via DIAG 0x308, isn't > it? (see handle_diag_308(), case 6). Is that really what was intended here? > Got me :). The intention is to remove non-standard data from the IPL parameter block. But I needed a place to store it in the s390-ipl context. The IPLB conveniently has plenty of space following the standard data. The alternative would be to have the QEMU IPL parameters explicitly called out in the object. But they're only duplication information available elsewhere (boot menu) or are computed anyway (netboot). This may change in the future... > [...] >> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c >> index e857ce4..825a1a3 100644 >> --- a/pc-bios/s390-ccw/main.c >> +++ b/pc-bios/s390-ccw/main.c >> @@ -16,6 +16,7 @@ char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE))); >> static SubChannelId blk_schid = { .one = 1 }; >> IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); >> static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 }; >> +QemuIplParameters qipl; >> >> /* >> * Priniciples of Operations (SA22-7832-09) chapter 17 requires that >> @@ -81,6 +82,7 @@ static void virtio_setup(void) >> uint16_t dev_no; >> char ldp[] = "LOADPARM=[________]\n"; >> VDev *vdev = virtio_get_device(); >> + QemuIplParameters *early_qipl = (QemuIplParameters *)204; > > Could you please introduce a proper #define for that magic 204 value > (and use it in s390_ipl_prepare_qipl, too)? ... so that it is later > easier to grep for this. > Hm ... I was following the pattern established in write_subsystem_identification(). Christian, what is your opinion? >> /* >> * We unconditionally enable mss support. In every sane configuration, >> @@ -93,6 +95,8 @@ static void virtio_setup(void) >> memcpy(ldp + 10, loadparm, 8); >> sclp_print(ldp); >> >> + memcpy(&qipl, early_qipl, sizeof(QemuIplParameters)); >> + >> if (store_iplb(&iplb)) { >> switch (iplb.pbt) { >> case S390_IPL_TYPE_CCW: >> @@ -127,7 +131,7 @@ static void virtio_setup(void) >> >> if (virtio_get_device_type() == VIRTIO_ID_NET) { >> sclp_print("Network boot device detected\n"); >> - vdev->netboot_start_addr = iplb.ccw.netboot_start_addr; >> + vdev->netboot_start_addr = qipl.netboot_start_addr; >> } else { >> virtio_blk_setup_device(blk_schid); >> >> > > Thomas >
On 02/06/2018 05:13 AM, Viktor Mihajlovski wrote: > On 06.02.2018 10:23, Thomas Huth wrote: >> On 05.02.2018 21:57, Collin L. Walling wrote: >> [...] >>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >>> index 0d06fc1..3e3c3b8 100644 >>> --- a/hw/s390x/ipl.c >>> +++ b/hw/s390x/ipl.c >>> @@ -399,6 +399,20 @@ void s390_reipl_request(void) >>> qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET); >>> } >>> >>> +static void s390_ipl_prepare_qipl(S390CPU *cpu) >>> +{ >>> + S390IPLState *ipl = get_ipl_device(); >>> + uint8_t *addr; >>> + uint64_t len = 4096; >>> + >>> + addr = cpu_physical_memory_map(cpu->env.psa, &len, 1); >>> + if (!addr || len < 204 + sizeof(QemuIplParameters)) { >>> + error_report("Cannot set QEMU IPL parameters"); >> I think you should return or exit() here. Otherwise the memcpy below >> accesses an illegal memory range.Right, I have noticed and fixed that on my private branch, but forgot to > update the patch. Collin, could you squash in a > return; Can do. [...]
On 02/06/2018 05:13 AM, Viktor Mihajlovski wrote: > On 06.02.2018 10:23, Thomas Huth wrote: >> On 05.02.2018 21:57, Collin L. Walling wrote: >> [...] >>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c >>> index e857ce4..825a1a3 100644 >>> --- a/pc-bios/s390-ccw/main.c >>> +++ b/pc-bios/s390-ccw/main.c >>> @@ -16,6 +16,7 @@ char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE))); >>> static SubChannelId blk_schid = { .one = 1 }; >>> IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); >>> static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 }; >>> +QemuIplParameters qipl; >>> >>> /* >>> * Priniciples of Operations (SA22-7832-09) chapter 17 requires that >>> @@ -81,6 +82,7 @@ static void virtio_setup(void) >>> uint16_t dev_no; >>> char ldp[] = "LOADPARM=[________]\n"; >>> VDev *vdev = virtio_get_device(); >>> + QemuIplParameters *early_qipl = (QemuIplParameters *)204; >> Could you please introduce a proper #define for that magic 204 value >> (and use it in s390_ipl_prepare_qipl, too)? ... so that it is later >> easier to grep for this. >> > Hm ... I was following the pattern established in > write_subsystem_identification(). Christian, what is your opinion? Any updates on this? Last change I need to make before the next post.
On 02/14/2018 03:51 PM, Collin L. Walling wrote: > On 02/06/2018 05:13 AM, Viktor Mihajlovski wrote: >> On 06.02.2018 10:23, Thomas Huth wrote: >>> On 05.02.2018 21:57, Collin L. Walling wrote: >>> [...] >>>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c >>>> index e857ce4..825a1a3 100644 >>>> --- a/pc-bios/s390-ccw/main.c >>>> +++ b/pc-bios/s390-ccw/main.c >>>> @@ -16,6 +16,7 @@ char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE))); >>>> static SubChannelId blk_schid = { .one = 1 }; >>>> IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); >>>> static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 }; >>>> +QemuIplParameters qipl; >>>> /* >>>> * Priniciples of Operations (SA22-7832-09) chapter 17 requires that >>>> @@ -81,6 +82,7 @@ static void virtio_setup(void) >>>> uint16_t dev_no; >>>> char ldp[] = "LOADPARM=[________]\n"; >>>> VDev *vdev = virtio_get_device(); >>>> + QemuIplParameters *early_qipl = (QemuIplParameters *)204; >>> Could you please introduce a proper #define for that magic 204 value >>> (and use it in s390_ipl_prepare_qipl, too)? ... so that it is later >>> easier to grep for this. >>> >> Hm ... I was following the pattern established in >> write_subsystem_identification(). Christian, what is your opinion? > > Any updates on this? Last change I need to make before the next post. Let's use a magic number. It is the preferred way in QEMU.
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index 0d06fc1..3e3c3b8 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -399,6 +399,20 @@ void s390_reipl_request(void) qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET); } +static void s390_ipl_prepare_qipl(S390CPU *cpu) +{ + S390IPLState *ipl = get_ipl_device(); + uint8_t *addr; + uint64_t len = 4096; + + addr = cpu_physical_memory_map(cpu->env.psa, &len, 1); + if (!addr || len < 204 + sizeof(QemuIplParameters)) { + error_report("Cannot set QEMU IPL parameters"); + } + memcpy(addr + 204, &ipl->iplb.qipl, sizeof(QemuIplParameters)); + cpu_physical_memory_unmap(addr, len, 1, len); +} + void s390_ipl_prepare_cpu(S390CPU *cpu) { S390IPLState *ipl = get_ipl_device(); @@ -418,8 +432,10 @@ void s390_ipl_prepare_cpu(S390CPU *cpu) error_report_err(err); vm_stop(RUN_STATE_INTERNAL_ERROR); } - ipl->iplb.ccw.netboot_start_addr = cpu_to_be64(ipl->start_addr); + ipl->iplb.qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr); } + s390_ipl_prepare_qipl(cpu); + } static void s390_ipl_reset(DeviceState *dev) diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h index 8a705e0..68dcaf8 100644 --- a/hw/s390x/ipl.h +++ b/hw/s390x/ipl.h @@ -16,8 +16,7 @@ #include "cpu.h" struct IplBlockCcw { - uint64_t netboot_start_addr; - uint8_t reserved0[77]; + uint8_t reserved0[85]; uint8_t ssid; uint16_t devno; uint8_t vm_flags; @@ -59,6 +58,13 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; #define DIAG308_FLAGS_LP_VALID 0x80 +struct QemuIplParameters { + uint8_t reserved1[4]; + uint64_t netboot_start_addr; + uint8_t reserved2[16]; +} QEMU_PACKED; +typedef struct QemuIplParameters QemuIplParameters; + union IplParameterBlock { struct { uint32_t len; @@ -74,6 +80,7 @@ union IplParameterBlock { IplBlockFcp fcp; IplBlockQemuScsi scsi; }; + QemuIplParameters qipl; } QEMU_PACKED; struct { uint8_t reserved1[110]; diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h index 890aed9..12f6e40 100644 --- a/pc-bios/s390-ccw/iplb.h +++ b/pc-bios/s390-ccw/iplb.h @@ -13,8 +13,7 @@ #define IPLB_H struct IplBlockCcw { - uint64_t netboot_start_addr; - uint8_t reserved0[77]; + uint8_t reserved0[85]; uint8_t ssid; uint16_t devno; uint8_t vm_flags; @@ -73,6 +72,15 @@ typedef struct IplParameterBlock IplParameterBlock; extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); +struct QemuIplParameters { + uint8_t reserved1[4]; + uint64_t netboot_start_addr; + uint8_t reserved2[16]; +} __attribute__ ((packed)); +typedef struct QemuIplParameters QemuIplParameters; + +extern QemuIplParameters qipl; + #define S390_IPL_TYPE_FCP 0x00 #define S390_IPL_TYPE_CCW 0x02 #define S390_IPL_TYPE_QEMU_SCSI 0xff diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c index e857ce4..825a1a3 100644 --- a/pc-bios/s390-ccw/main.c +++ b/pc-bios/s390-ccw/main.c @@ -16,6 +16,7 @@ char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE))); static SubChannelId blk_schid = { .one = 1 }; IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 }; +QemuIplParameters qipl; /* * Priniciples of Operations (SA22-7832-09) chapter 17 requires that @@ -81,6 +82,7 @@ static void virtio_setup(void) uint16_t dev_no; char ldp[] = "LOADPARM=[________]\n"; VDev *vdev = virtio_get_device(); + QemuIplParameters *early_qipl = (QemuIplParameters *)204; /* * We unconditionally enable mss support. In every sane configuration, @@ -93,6 +95,8 @@ static void virtio_setup(void) memcpy(ldp + 10, loadparm, 8); sclp_print(ldp); + memcpy(&qipl, early_qipl, sizeof(QemuIplParameters)); + if (store_iplb(&iplb)) { switch (iplb.pbt) { case S390_IPL_TYPE_CCW: @@ -127,7 +131,7 @@ static void virtio_setup(void) if (virtio_get_device_type() == VIRTIO_ID_NET) { sclp_print("Network boot device detected\n"); - vdev->netboot_start_addr = iplb.ccw.netboot_start_addr; + vdev->netboot_start_addr = qipl.netboot_start_addr; } else { virtio_blk_setup_device(blk_schid);
The s390-ccw firmware needs some information in support of the boot process which is not available on the native machine. Examples are the netboot firmware load address and now the boot menu parameters. While storing that data in unused fields of the IPL parameter block works, that approach could create problems if the parameter block definition should change in the future. Because then a guest could overwrite these fields using the set IPLB diagnose. In fact the data in question is of more global nature and not really tied to an IPL device, so separating it is rather logical. This commit introduces a new structure to hold firmware relevant IPL parameters set by QEMU. The data is stored at location 204 (dec) and can contain up to 7 32-bit words. This area is available to programming in the z/Architecture Principles of Operation and can thus safely be used by the firmware until the IPL has completed. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- hw/s390x/ipl.c | 18 +++++++++++++++++- hw/s390x/ipl.h | 11 +++++++++-- pc-bios/s390-ccw/iplb.h | 12 ++++++++++-- pc-bios/s390-ccw/main.c | 6 +++++- 4 files changed, 41 insertions(+), 6 deletions(-)