Re: [Qemu-devel] [PATCH 2/8] s390: autodetect map private

[Jan Kiszka]

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)

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux