Re: [Qemu-devel] [PATCH 05/17] kvm: add kvm stub for arch specific stuff

[Jan Kiszka]

On 2011-05-04 10:40, Alexander Graf wrote:
> 
> On 04.05.2011, at 10:31, Jan Kiszka wrote:
> 
>> On 2011-05-04 07:19, Alexander Graf wrote:
>>>
>>> On 03.05.2011, at 16:57, Jan Kiszka wrote:
>>>
>>>> On 2011-05-03 16:17, Alexander Graf wrote:
>>>>>
>>>>> On 18.04.2011, at 20:34, Aurelien Jarno wrote:
>>>>>
>>>>>> On Fri, Apr 15, 2011 at 05:32:46PM +0200, Alexander Graf wrote:
>>>>>>> We have a generic stub architecture for kvm calls, but some 
>>>>>>> architectures
>>>>>>> are different from others. So we do want to be able to have stubs for
>>>>>>> architecture specific functionality as well.
>>>>>>>
>>>>>>> This patch adds kvm stubs for all architectures.
>>>>>>>
>>>>>>> Signed-off-by: Alexander Graf <address@hidden>
>>>>>>> ---
>>>>>>> Makefile.target                   |    2 +-
>>>>>>> target-alpha/kvm-arch-stub.c      |   26 +++++++++++++++++++++++++
>>>>>>> target-arm/kvm-arch-stub.c        |   26 +++++++++++++++++++++++++
>>>>>>> target-cris/kvm-arch-stub.c       |   26 +++++++++++++++++++++++++
>>>>>>> target-i386/kvm-arch-stub.c       |   26 +++++++++++++++++++++++++
>>>>>>> target-lm32/kvm-arch-stub.c       |   26 +++++++++++++++++++++++++
>>>>>>> target-m68k/kvm-arch-stub.c       |   26 +++++++++++++++++++++++++
>>>>>>> target-microblaze/kvm-arch-stub.c |   26 +++++++++++++++++++++++++
>>>>>>> target-mips/kvm-arch-stub.c       |   26 +++++++++++++++++++++++++
>>>>>>> target-ppc/kvm-arch-stub.c        |   26 +++++++++++++++++++++++++
>>>>>>> target-s390x/kvm-arch-stub.c      |   38 
>>>>>>> +++++++++++++++++++++++++++++++++++++
>>>>>>> target-sh4/kvm-arch-stub.c        |   26 +++++++++++++++++++++++++
>>>>>>> target-sparc/kvm-arch-stub.c      |   26 +++++++++++++++++++++++++
>>>>>>> target-unicore32/kvm-arch-stub.c  |   26 +++++++++++++++++++++++++
>>>>>>> 14 files changed, 351 insertions(+), 1 deletions(-)
>>>>>>> create mode 100644 target-alpha/kvm-arch-stub.c
>>>>>>> create mode 100644 target-arm/kvm-arch-stub.c
>>>>>>> create mode 100644 target-cris/kvm-arch-stub.c
>>>>>>> create mode 100644 target-i386/kvm-arch-stub.c
>>>>>>> create mode 100644 target-lm32/kvm-arch-stub.c
>>>>>>> create mode 100644 target-m68k/kvm-arch-stub.c
>>>>>>> create mode 100644 target-microblaze/kvm-arch-stub.c
>>>>>>> create mode 100644 target-mips/kvm-arch-stub.c
>>>>>>> create mode 100644 target-ppc/kvm-arch-stub.c
>>>>>>> create mode 100644 target-s390x/kvm-arch-stub.c
>>>>>>> create mode 100644 target-sh4/kvm-arch-stub.c
>>>>>>> create mode 100644 target-sparc/kvm-arch-stub.c
>>>>>>> create mode 100644 target-unicore32/kvm-arch-stub.c
>>>>>>
>>>>>> Do we really want to create so much files on architectures we will never
>>>>>> see KVM support? Actually I know very few things about KVM, so it would
>>>>>> be better to have this patch reviewed by someone else. Avi or Anthony
>>>>>> maybe?
>>>>>
>>>>> Well, the main idea is to be able to have a unified place to put stub 
>>>>> functions into. And as it stands with most generalizations, we either 
>>>>> make it generic or not :).
>>>>> Maybe there's some Makefile magic to only compile the stub if the file 
>>>>> exists? I certainly don't know of any.
>>>>
>>>> This approach looks wrong.
>>>>
>>>> The point of kvm stubs is to allow generic components to be built
>>>> independently of kvm enabled/disabled. But target-specific callbacks
>>>> can't be part of generic components anyway. So there is no need for a
>>>> stub, those bits will be built per-target anyway.
>>>>
>>>> The examples you provided with this patch underline it:
>>>> s390-virtio-bus.c should be built for s390 but nothing else.
>>>
>>> And it is, yes. The point is to not require #ifdefs in device emulation 
>>> code :).
>>
>> But that's not the purpose of the stubs. They shall avoid building
>> components target specific when just the kvm on/off dependency would
>> force them to. Moreover, I do not see any need for such in
>> infrastructure beyond s390 when considering that case valid.
>>
>> Why not simply define those few functions as static inline in the
>> already s390-specific header depending on #ifdef CONFIG_KVM?
> 
> Which already-s390-specific header depending on #ifdef CONFIG_KVM? The 
> current definitions are in cpu.h which is included in code that doesn't 
> include CONFIG*.

cpu.h is target-specific, thus must pull in all the configs (e.g. the
prototypes are already under CONFIG_USER_ONLY).

Jan

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