From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:39720)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <borntraeger@de.ibm.com>) id 1SpiyV-0005Zu-GR
	for qemu-devel@nongnu.org; Fri, 13 Jul 2012 12:44:16 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <borntraeger@de.ibm.com>) id 1SpiyU-0003lx-Cn
	for qemu-devel@nongnu.org; Fri, 13 Jul 2012 12:44:15 -0400
Received: from e06smtp12.uk.ibm.com ([195.75.94.108]:49766)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <borntraeger@de.ibm.com>) id 1SpiyU-0003lt-4e
	for qemu-devel@nongnu.org; Fri, 13 Jul 2012 12:44:14 -0400
Received: from /spool/local
	by e06smtp12.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use
	Only! Violators will be prosecuted
	for <qemu-devel@nongnu.org> from <borntraeger@de.ibm.com>;
	Fri, 13 Jul 2012 17:44:12 +0100
Received: from d06av12.portsmouth.uk.ibm.com (d06av12.portsmouth.uk.ibm.com
	[9.149.37.247])
	by d06nrmr1806.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with
	ESMTP id q6DGiAds2756806
	for <qemu-devel@nongnu.org>; Fri, 13 Jul 2012 17:44:10 +0100
Received: from d06av12.portsmouth.uk.ibm.com (loopback [127.0.0.1])
	by d06av12.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with
	ESMTP id q6DGi9Dm032245
	for <qemu-devel@nongnu.org>; Fri, 13 Jul 2012 10:44:09 -0600
Message-ID: <50005058.6000707@de.ibm.com>
Date: Fri, 13 Jul 2012 18:44:08 +0200
From: Christian Borntraeger <borntraeger@de.ibm.com>
MIME-Version: 1.0
References: <1342176724-43776-1-git-send-email-borntraeger@de.ibm.com>
	<1342176724-43776-3-git-send-email-borntraeger@de.ibm.com>
	<CAAu8pHuEAt09ZLhzY-Jux4gxYC2NkG=u+2jcHESUCDnGGjcJEg@mail.gmail.com>
In-Reply-To: <CAAu8pHuEAt09ZLhzY-Jux4gxYC2NkG=u+2jcHESUCDnGGjcJEg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 2/6] s390: sclp base support
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Blue Swirl <blauwirbel@gmail.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>, Jens Freimann <jfrei@linux.vnet.ibm.com>, Heinz Graalfs <graalfs@linux.vnet.ibm.com>, Alexander Graf <agraf@suse.de>, qemu-devel <qemu-devel@nongnu.org>

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.