Re: [Qemu-devel] [PATCH 2/6] s390: sclp base support

[Christian Borntraeger <borntraeger@de.ibm.com>]

Thanks fpr the review,


On 13/07/12 17:08, Blue Swirl wrote:

>> + * Copyright IBM, Corp. 2007, 2012
> 
> 2007, really?

Well, yes and no. The first userspace for kvm on s390 was kuli and some of that
code was used for bringup. But it looks pretty different now, so we can
change that to be 2012 as the first release.

[...]
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> 
> Why GPLv2only, can't this be licensed under later versions?

Was simply copied from other qemu files. We can certainly change that to 2 and later.


[...]

>> +int do_sclp_service_call(uint32_t sccb, uint64_t code)
> 
> sccb could be target_phys_addr_t.

the architecture explicitely requires to have the 32 msbs to be 0. target_phys_addr_t
would certainly work as well, but 

[...]
>> +    cpu_physical_memory_write(sccb, &work_sccb,
>> +                              be16_to_cpu(work_sccb.h.length));
> 
> Perhaps the DMA helpers should be used instead.
> 

Is there any rule what to use under which circumstances.


[...]
>> +#ifndef _QEMU_S390_SCLP_H
>> +#define _QEMU_S390_SCLP_H
> 
> HW_S390_SCLP_H

Ok

[...]
>> +} __attribute__((packed)) SCCBHeader;
> 
> QEMU_PACKED

Ok

[...]
>> +extern SCLPS390Bus *sclp_bus;
> 
> Global state is suspicious, usually it can be avoided with parameter passing.
[...]

Will check if we can make that go away.