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

[Blue Swirl]

On Fri, Jul 13, 2012 at 4:44 PM, Christian Borntraeger
<address@hidden> wrote:
> 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.

Great!

>
>
> [...]
>
>>> +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.

No, it's a recent addition. Devices (at least those which may be used
in IOMMU environment) should use DMA helpers for their memory accesses
in general but virtio may be an exception. I'm not sure which category
this falls in.

>
>
> [...]
>>> +#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.
>