Re: [qemu-s390x] [Qemu-devel] [PATCH v1 4/5] s390-ccw: interactive boot menu for eckd dasd

[Collin L. Walling]

On 11/28/2017 07:36 AM, Thomas Huth wrote:
> On 27.11.2017 21:55, Collin L. Walling wrote:
> [...] 
> > +static void read_stage2(block_number_t s1b_block_nr, void **stage2_data)
> > +{
> > +    block_number_t s2_block_nr;
> > +    BootEckdStage1b *s1b = (void *)sec;
> > +    int i;
> > +
> > +    /* Get Stage1b data */
> > +    memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> > +    read_block(s1b_block_nr, s1b, "Cannot read stage1b boot loader.");
> > +
> > +    /* Get Stage2 data */
> > +    *stage2_data = (void *)s2_area;
> I think you could also simply "return s2_area" at the end of the
> function instead of using a pointer-to-a-pointer as parameter of the
> function.

Ideally I'd like to figure out some way to get rid of the s2_area and
just read stage2 block-by-block (that way we're only using sec).

I'll play around with things here after I've taken care of the other
suggestions.

[...]

> > +#define KEYCODE_BACKSP '\177'
> > +#define KEYCODE_ENTER  '\r'
> > +
> >   long write(int fd, const void *str, size_t len);
> >   
> >   static char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
> > @@ -101,3 +106,130 @@ void sclp_get_loadparm_ascii(char *loadparm)
> >           ebcdic_to_ascii((char *) sccb->loadparm, loadparm, 8);
> >       }
> >   }
> > +
> > +static void read(char **str)
> Please avoid using names from the standard C-library, unless the
> function is supposed to do the same (as the write() function here).

Thinking about it, I'm not sure if it would make sense for us to have a
C-like read() function... perhaps it'd be best to rename it for now.

[...]
> > +    );
> > +}
> > +
> > +static inline bool check_clock_int(void)
> > +{
> > +    uint16_t code;
> > +
> > +    consume_sclp_int();
> > +
> > +    asm volatile(
> > +        "lh         1, 0x86(0,0)\n"
> > +        "sth        1, %0"
> > +        : "=r" (code)
> > +    );
> That way, you need r1 in the clobber list. But why don't you simply use
> only "lh %0,0x86(0,0)\n" instead? Or simply do it without assembly:
>
>   code = *(volatile uint16_t *)0x86;

This is new to me... thanks for showing me this :)

> ?
[...]
>   Thomas
Thanks for the review, Thomas.I've noted all other suggestions.

--
- Collin L Walling