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

[Alexander Graf]

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)) {
<normal code>
}

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