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

["Collin L. Walling" <walling@linux.vnet.ibm.com>]

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