From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43995) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SelHT-0000tb-II for qemu-devel@nongnu.org; Wed, 13 Jun 2012 06:58:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SelHO-0002Os-NB for qemu-devel@nongnu.org; Wed, 13 Jun 2012 06:58:31 -0400 Received: from goliath.siemens.de ([192.35.17.28]:27994) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SelHO-0002OI-Ds for qemu-devel@nongnu.org; Wed, 13 Jun 2012 06:58:26 -0400 Message-ID: <4FD8724A.6020704@siemens.com> Date: Wed, 13 Jun 2012 12:58:18 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1338984323-21914-1-git-send-email-jfrei@de.ibm.com> <1338984323-21914-3-git-send-email-jfrei@de.ibm.com> <4FD70CC0.7000901@suse.de> <4FD725EB.7050501@de.ibm.com> <4FD72EC2.2010105@suse.de> <4FD72FD4.6020008@de.ibm.com> <4FD73220.5000408@suse.de> <4FD86BB1.9060900@siemens.com> <4FD87156.10705@suse.de> In-Reply-To: <4FD87156.10705@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/8] s390: autodetect map private List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Jens Freimann , Heinz Graalfs , qemu-devel , Christian Borntraeger , Jens Freimann , Cornelia Huck On 2012-06-13 12:54, Alexander Graf wrote: > On 06/13/2012 12:30 PM, Jan Kiszka wrote: >> On 2012-06-12 14:12, Alexander Graf wrote: >>> On 06/12/2012 02:02 PM, Christian Borntraeger wrote: >>>> On 12/06/12 13:57, Alexander Graf wrote: >>>>> Since it lives in an s390 specific branch, the function name should probably be called s390 specific. If we ever need another architecture to have a kvm specific ram allocator, we can make it generic when that time comes. Until then, let's treat s390 as the oddball it is :). >>>>> >>>>> Apart from that, this approach looks a lot nicer, yes. >>>> But then I have to have a *s390* function declared in kvm.h and your other comment >>>> hits me. You got me in a trap here, heh? ;-) >>> Ah, I see what you mean. I was thinking of having a >>> target-s390x/kvm_s390x.h or so. Then we could add the function >>> definition there and have everything nicely contained within >>> target-s390x only. >>> >>> Jan, which approach would you think is cleaner? Make this a generic >>> kvm_arch callback or introduce a special kvm_s390x.h header which would >>> then have to be explicitly included in exec.c? >> Maybe somethings like >> >> #ifdef __s390__ >> else if (kvm_enabled()) >> new_block->host = kvm_arch_vmalloc(size) >> #endif >> >> ? But I have no definitive opinion yet. I think that >> >> - the changes to generic code should make clear that it's an s390+kvm >> specialty >> - actual work should be done in target-s390/kvm.c (e.g. avoid >> legacy_s390_alloc) > > Thinking about this a bit more, how about > > } else if (!kvm_arch_vmalloc(size, &new_block->host)) { > > } > > Then the arch specific code could do the check and the implementation of > vmalloc, but only has to return -1 if we don't need it and things still > fall back to the generic code. But you would have to walk a while to find out that only s390x on (old) KVM actually returns success here and does some allocation. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux