| Message ID | 1518818879-18608-6-git-send-email-walling@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Interactive Boot Menu for DASD and SCSI Guests on s390x | expand |
On 16.02.2018 23:07, 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. > > Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> > Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> > --- [...] > diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h > index 8a705e0..74469b1 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,21 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; > > #define DIAG308_FLAGS_LP_VALID 0x80 > > +#define QIPL_ADDRESS 0xcc > + > +/* > + * The QEMU IPL Parameters will be stored 32-bit word aligned. > + * Placement of data fields in this area must account for > + * their alignment needs. > + * The entire structure must not be larger than 28 bytes. > + */ > +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 +88,7 @@ union IplParameterBlock { > IplBlockFcp fcp; > IplBlockQemuScsi scsi; > }; > + QemuIplParameters qipl; > } QEMU_PACKED; > struct { > uint8_t reserved1[110]; I still think that the information should *not* be stored within the IplParameterBlock to avoid that we pass it via DIAG 0x308, too. If we do it like this, I'm pretty sure that we will look at this code in a couple of years and wonder whether we can change it again or whether this is an established interface between the host and the guest. So please, let's avoid establishing such "hidden" interfaces just out of current convenience. There must be a better location for this. Christian, do you have an idea? Thomas
On 17.02.2018 09:11, Thomas Huth wrote: [...] > > I still think that the information should *not* be stored within the > IplParameterBlock to avoid that we pass it via DIAG 0x308, too. > If we do it like this, I'm pretty sure that we will look at this code in > a couple of years and wonder whether we can change it again or whether > this is an established interface between the host and the guest. So > please, let's avoid establishing such "hidden" interfaces just out of > current convenience. There must be a better location for this. > Christian, do you have an idea? > > Thomas > In principle I do agree, although I think we can manage the host/guest boundary. If we believe we can't, we have a much bigger problem (looking at the position of the iplb smack in the middle of the s390-ipl state). The main reason why I didn't bother to introduce a new private field in s390-ipl was that I expect more changes to the IPL area in the future (earlier than in a couple of years) and am not really sure whether this QEMU <-> BIOS interface will remain the same and how much effort to spend on it. The major point of this change is to move non-standard data out of the guest-visible IPLB to avoid compatibility problems in the future, while still catering for features like network boot and boot menus. I have no bias against other solutions achieving this objective. If you and Christian think we need a new field, it's all right with me.
On 19.02.2018 09:50, Viktor Mihajlovski wrote: > On 17.02.2018 09:11, Thomas Huth wrote: > [...] >> >> I still think that the information should *not* be stored within the >> IplParameterBlock to avoid that we pass it via DIAG 0x308, too. >> If we do it like this, I'm pretty sure that we will look at this code in >> a couple of years and wonder whether we can change it again or whether >> this is an established interface between the host and the guest. So >> please, let's avoid establishing such "hidden" interfaces just out of >> current convenience. There must be a better location for this. >> Christian, do you have an idea? >> >> Thomas >> > In principle I do agree, although I think we can manage the host/guest > boundary. If we believe we can't, we have a much bigger problem (looking > at the position of the iplb smack in the middle of the s390-ipl state). > > The main reason why I didn't bother to introduce a new private field in > s390-ipl was that I expect more changes to the IPL area in the future > (earlier than in a couple of years) and am not really sure whether this > QEMU <-> BIOS interface will remain the same and how much effort to > spend on it. > > The major point of this change is to move non-standard data out of the > guest-visible IPLB to avoid compatibility problems in the future, while > still catering for features like network boot and boot menus. I have no > bias against other solutions achieving this objective. If you and > Christian think we need a new field, it's all right with me. > With below squashed in (and a subsequent fixup for the next patch), we can logically separate the QEMU IPL parameter from the IPLB. Better? --- diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index 31565ce..c5923b5 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -410,7 +410,7 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu) error_report("Cannot set QEMU IPL parameters"); return; } - memcpy(addr + QIPL_ADDRESS, &ipl->iplb.qipl, sizeof(QemuIplParameters)); + memcpy(addr + QIPL_ADDRESS, &ipl->qipl, sizeof(QemuIplParameters)); cpu_physical_memory_unmap(addr, len, 1, len); } @@ -433,7 +433,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu) error_report_err(err); vm_stop(RUN_STATE_INTERNAL_ERROR); } - ipl->iplb.qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr); + ipl->qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr); } s390_ipl_prepare_qipl(cpu); diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h index 74469b1..775d363 100644 --- a/hw/s390x/ipl.h +++ b/hw/s390x/ipl.h @@ -88,7 +88,6 @@ union IplParameterBlock { IplBlockFcp fcp; IplBlockQemuScsi scsi; }; - QemuIplParameters qipl; } QEMU_PACKED; struct { uint8_t reserved1[110]; @@ -120,6 +119,7 @@ struct S390IPLState { bool iplb_valid; bool reipl_requested; bool netboot; + QemuIplParameters qipl; /*< public >*/ char *kernel;
On 19.02.2018 13:15, Viktor Mihajlovski wrote: > On 19.02.2018 09:50, Viktor Mihajlovski wrote: >> On 17.02.2018 09:11, Thomas Huth wrote: >> [...] >>> >>> I still think that the information should *not* be stored within the >>> IplParameterBlock to avoid that we pass it via DIAG 0x308, too. >>> If we do it like this, I'm pretty sure that we will look at this code in >>> a couple of years and wonder whether we can change it again or whether >>> this is an established interface between the host and the guest. So >>> please, let's avoid establishing such "hidden" interfaces just out of >>> current convenience. There must be a better location for this. >>> Christian, do you have an idea? >>> >>> Thomas >>> >> In principle I do agree, although I think we can manage the host/guest >> boundary. If we believe we can't, we have a much bigger problem (looking >> at the position of the iplb smack in the middle of the s390-ipl state). >> >> The main reason why I didn't bother to introduce a new private field in >> s390-ipl was that I expect more changes to the IPL area in the future >> (earlier than in a couple of years) and am not really sure whether this >> QEMU <-> BIOS interface will remain the same and how much effort to >> spend on it. >> >> The major point of this change is to move non-standard data out of the >> guest-visible IPLB to avoid compatibility problems in the future, while >> still catering for features like network boot and boot menus. I have no >> bias against other solutions achieving this objective. If you and >> Christian think we need a new field, it's all right with me. >> > With below squashed in (and a subsequent fixup for the next patch), we can > logically separate the QEMU IPL parameter from the IPLB. Better? Yes, sounds much better to me! Collin, could you please squash that in? Thanks, Thomas > --- > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index 31565ce..c5923b5 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -410,7 +410,7 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu) > error_report("Cannot set QEMU IPL parameters"); > return; > } > - memcpy(addr + QIPL_ADDRESS, &ipl->iplb.qipl, sizeof(QemuIplParameters)); > + memcpy(addr + QIPL_ADDRESS, &ipl->qipl, sizeof(QemuIplParameters)); > cpu_physical_memory_unmap(addr, len, 1, len); > } > > @@ -433,7 +433,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu) > error_report_err(err); > vm_stop(RUN_STATE_INTERNAL_ERROR); > } > - ipl->iplb.qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr); > + ipl->qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr); > } > s390_ipl_prepare_qipl(cpu); > > diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h > index 74469b1..775d363 100644 > --- a/hw/s390x/ipl.h > +++ b/hw/s390x/ipl.h > @@ -88,7 +88,6 @@ union IplParameterBlock { > IplBlockFcp fcp; > IplBlockQemuScsi scsi; > }; > - QemuIplParameters qipl; > } QEMU_PACKED; > struct { > uint8_t reserved1[110]; > @@ -120,6 +119,7 @@ struct S390IPLState { > bool iplb_valid; > bool reipl_requested; > bool netboot; > + QemuIplParameters qipl; > > /*< public >*/ > char *kernel; > >
On 16.02.2018 23:07, Collin L. Walling wrote: [...] > diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h > index 8a705e0..74469b1 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,21 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; > > #define DIAG308_FLAGS_LP_VALID 0x80 > > +#define QIPL_ADDRESS 0xcc > + > +/* > + * The QEMU IPL Parameters will be stored 32-bit word aligned. > + * Placement of data fields in this area must account for > + * their alignment needs. > + * The entire structure must not be larger than 28 bytes. > + */ I realize that my initial suggestion was flawed. Hopefully better: /* * The QEMU IPL Parameters will be stored at absolute address * 204 (0xcc) which means it is 32-bit word aligned but not * double-word aligned. * Placement of data fields in this area must account for * their alignment needs. E.g., netboot_start_address must * have an offset of n * 8 bytes within the struct in order * to keep it double-word aligned. * The total size of the struct must never exceed 28 bytes. * This definition must be kept in sync with the defininition * in pc-bios/s390-ccw/iplb.h. */ > +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; [...] > diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h > index 890aed9..a23237e 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,23 @@ typedef struct IplParameterBlock IplParameterBlock; > > extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); > > +#define QIPL_ADDRESS 0xcc > + > +/* > + * The QEMU IPL Parameters will be stored 32-bit word aligned. > + * Placement of data fields in this area must account for > + * their alignment needs. > + * The entire structure must not be larger than 28 bytes. > + */ The comment can probably be a bit more terse, avoiding text duplication and potential inconsistencies arising thereof. /* * This definition must be kept in sync with the defininition * in hw/s390x/ipl.h */ > +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/hw/s390x/ipl.c b/hw/s390x/ipl.c index 0d06fc1..31565ce 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -399,6 +399,21 @@ 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 < QIPL_ADDRESS + sizeof(QemuIplParameters)) { + error_report("Cannot set QEMU IPL parameters"); + return; + } + memcpy(addr + QIPL_ADDRESS, &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 +433,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..74469b1 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,21 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; #define DIAG308_FLAGS_LP_VALID 0x80 +#define QIPL_ADDRESS 0xcc + +/* + * The QEMU IPL Parameters will be stored 32-bit word aligned. + * Placement of data fields in this area must account for + * their alignment needs. + * The entire structure must not be larger than 28 bytes. + */ +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 +88,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..a23237e 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,23 @@ typedef struct IplParameterBlock IplParameterBlock; extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); +#define QIPL_ADDRESS 0xcc + +/* + * The QEMU IPL Parameters will be stored 32-bit word aligned. + * Placement of data fields in this area must account for + * their alignment needs. + * The entire structure must not be larger than 28 bytes. + */ +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..e41b264 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 *)QIPL_ADDRESS; /* * 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);