[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [qemu-s390x] [Qemu-devel] [PULL 08/15] pc-bios/s390-ccw: fix loadpar
From: |
Cornelia Huck |
Subject: |
Re: [qemu-s390x] [Qemu-devel] [PULL 08/15] pc-bios/s390-ccw: fix loadparm initialization and int conversion |
Date: |
Tue, 22 May 2018 14:44:26 +0200 |
On Tue, 15 May 2018 10:45:02 +0200
Laszlo Ersek <address@hidden> wrote:
> I've come across this patch in downstream review (although I really have
> zero background in s390x), and Cornelia suggested I might want to repeat
> my comments on the upstream list too:
>
> On 05/04/18 09:25, Cornelia Huck wrote:
> > From: Collin Walling <address@hidden>
> >
> > Rename the loadparm char array in main.c to loadparm_str and
> > increased the size by one byte to account for a null termination
> > when converting the loadparm string to an int via atoui. We
> > also allow the boot menu to be enabled when loadparm is set to
> > an empty string or a series of spaces.
> >
> > Signed-off-by: Collin Walling <address@hidden>
> > Reported-by: Vasily Gorbik <address@hidden>
> > Reviewed-by: Thomas Huth <address@hidden>
> > Reviewed-by: Janosch Frank <address@hidden>
> > Signed-off-by: Thomas Huth <address@hidden>
> > ---
> > hw/s390x/ipl.c | 4 ++++
> > pc-bios/s390-ccw/main.c | 14 +++++++-------
> > 2 files changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> > index fb554ab156..150f6c0582 100644
> > --- a/hw/s390x/ipl.c
> > +++ b/hw/s390x/ipl.c
> > @@ -373,6 +373,10 @@ int s390_ipl_set_loadparm(uint8_t *loadparm)
> > loadparm[i] = ascii2ebcdic[(uint8_t) lp[i]];
> > }
> >
> > + if (i < 8) {
> > + memset(loadparm + i, 0x40, 8 - i); /* fill with EBCDIC spaces
> > */
> > + }
> > +
> > g_free(lp);
> > return 0;
> > }
> > diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> > index 9d9f8cf4d3..26f9adf84a 100644
> > --- a/pc-bios/s390-ccw/main.c
> > +++ b/pc-bios/s390-ccw/main.c
> > @@ -15,11 +15,11 @@
> > 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 };
> > +static char loadparm_str[9] = { 0, 0, 0, 0, 0, 0, 0, 0, 0 };
> > QemuIplParameters qipl;
> >
> > #define LOADPARM_PROMPT "PROMPT "
> > -#define LOADPARM_EMPTY "........"
> > +#define LOADPARM_EMPTY " "
> > #define BOOT_MENU_FLAG_MASK (QIPL_FLAG_BM_OPTS_CMD |
> > QIPL_FLAG_BM_OPTS_ZIPL)
> >
> > /*
> > @@ -45,7 +45,7 @@ void panic(const char *string)
> >
> > unsigned int get_loadparm_index(void)
> > {
> > - return atoui(loadparm);
> > + return atoui(loadparm_str);
> > }
> >
> > static bool find_dev(Schib *schib, int dev_no)
> > @@ -80,13 +80,13 @@ static bool find_dev(Schib *schib, int dev_no)
> >
> > static void menu_setup(void)
> > {
> > - if (memcmp(loadparm, LOADPARM_PROMPT, 8) == 0) {
> > + if (memcmp(loadparm_str, LOADPARM_PROMPT, 8) == 0) {
> > menu_set_parms(QIPL_FLAG_BM_OPTS_CMD, 0);
> > return;
> > }
> >
> > /* If loadparm was set to any other value, then do not enable menu */
> > - if (memcmp(loadparm, LOADPARM_EMPTY, 8) != 0) {
> > + if (memcmp(loadparm_str, LOADPARM_EMPTY, 8) != 0) {
> > return;
> > }
> >
> > @@ -116,8 +116,8 @@ static void virtio_setup(void)
> > */
> > enable_mss_facility();
> >
> > - sclp_get_loadparm_ascii(loadparm);
> > - memcpy(ldp + 10, loadparm, 8);
> > + sclp_get_loadparm_ascii(loadparm_str);
> > + memcpy(ldp + 10, loadparm_str, 8);
> > sclp_print(ldp);
> >
> > memcpy(&qipl, early_qipl, sizeof(QemuIplParameters));
> >
>
> So here I said, "There are a bunch of naked 8 constants; those should
> all be (sizeof loadparm_str - 1), I think. Should be a separate patch,
> of course."
Alternatively, as loadparm size is architectured IIRC and unlikely to
change, do
#define LOADPARM_LEN 8
char loadparm_str[LOADPARM_LEN + 1];
and compare against LOADPARM_LEN... opinions?
- [qemu-s390x] [PULL 00/15] first s390x update for 2.13, Cornelia Huck, 2018/05/04
- [qemu-s390x] [PULL 01/15] s390x: introduce 2.13 compat machine, Cornelia Huck, 2018/05/04
- [qemu-s390x] [PULL 03/15] vfio-ccw: introduce vfio_ccw_get_device(), Cornelia Huck, 2018/05/04
- [qemu-s390x] [PULL 02/15] s390x/sclp: extend SCLP event masks to 64 bits, Cornelia Huck, 2018/05/04
- [qemu-s390x] [PULL 04/15] s390x/kvm: cleanup calls to cpu_synchronize_state(), Cornelia Huck, 2018/05/04
- [qemu-s390x] [PULL 06/15] pc-bios/s390-ccw: size_t should be unsigned, Cornelia Huck, 2018/05/04
- [qemu-s390x] [PULL 05/15] hw/s390x: Allow to configure the consoles with the "-serial" parameter, Cornelia Huck, 2018/05/04
- [qemu-s390x] [PULL 07/15] pc-bios/s390-ccw: rename MAX_TABLE_ENTRIES to MAX_BOOT_ENTRIES, Cornelia Huck, 2018/05/04
- [qemu-s390x] [PULL 08/15] pc-bios/s390-ccw: fix loadparm initialization and int conversion, Cornelia Huck, 2018/05/04
- [qemu-s390x] [PULL 09/15] pc-bios/s390-ccw: fix non-sequential boot entries (eckd), Cornelia Huck, 2018/05/04
- [qemu-s390x] [PULL 10/15] pc-bios/s390-ccw: fix non-sequential boot entries (enum), Cornelia Huck, 2018/05/04
- [qemu-s390x] [PULL 11/15] pc-bios/s390-ccw/net: Split up net_load() into init, load and release parts, Cornelia Huck, 2018/05/04
- [qemu-s390x] [PULL 13/15] pc-bios/s390-ccw/net: Add support for .INS config files, Cornelia Huck, 2018/05/04
- [qemu-s390x] [PULL 12/15] pc-bios/s390-ccw/net: Use diag308 to reset machine before jumping to the OS, Cornelia Huck, 2018/05/04
- [qemu-s390x] [PULL 14/15] s390-ccw: force diag 308 subcode to unsigned long, Cornelia Huck, 2018/05/04
- [qemu-s390x] [PULL 15/15] pc-bios/s390: Update firmware images, Cornelia Huck, 2018/05/04