From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49414) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SelDR-0005VW-0x for qemu-devel@nongnu.org; Wed, 13 Jun 2012 06:54:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SelDP-00015M-01 for qemu-devel@nongnu.org; Wed, 13 Jun 2012 06:54:20 -0400 Received: from cantor2.suse.de ([195.135.220.15]:38859 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SelDO-00014w-Mm for qemu-devel@nongnu.org; Wed, 13 Jun 2012 06:54:18 -0400 Message-ID: <4FD87156.10705@suse.de> Date: Wed, 13 Jun 2012 12:54:14 +0200 From: Alexander Graf 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> In-Reply-To: <4FD86BB1.9060900@siemens.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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: Jan Kiszka Cc: Jens Freimann , Heinz Graalfs , qemu-devel , Christian Borntraeger , Jens Freimann , Cornelia Huck 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. Alex