[Qemu-devel] [PATCH] kvm: Combine all kvm stubs in a single file and compile it only once

[Qemu-devel] [PATCH] kvm: Combine all kvm stubs in a single file and compile it only once

[Stefan Weil]

The KVM stub variables and functions don't depend on target specific data
types, so it is possible to compile kvm-stub.c only once.

Integrating the target specific KVM stubs for ARM, I386 and PPC in the
common kvm-stub.c further simplifies the build environment and allows
removing CONFIG_NO_KVM.

Instead of 53 kvm-stub.o files, there is now only one file.

abort() is replaced by g_assert_not_reached() which gives better diagnostic
messages when it is called.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

The resulting binary is slightly larger than before because it
includes more stub functions. It could be made smaller by adding
separate stubs/kvm-stub-arm.c, stubs/kvm-stub-i386.c and
stubs/kvm-stub-ppc.c files. Using alias symbols for the stub
functions would reduce the size further, but I don't think
the size is critical here.

Should we call g_assert_not_reached() in more (all?) stub functions?

If the patch is accepted, a similar modification could be done
for xen-stub.c.

Stefan

 

 Makefile.target                |    1 -
 stubs/Makefile.objs            |    1 +
 kvm-stub.c => stubs/kvm-stub.c |   70 ++++++++++++++++++++++++++++++++++++----
 target-arm/Makefile.objs       |    1 -
 target-arm/kvm-stub.c          |   23 -------------
 target-i386/Makefile.objs      |    1 -
 target-i386/kvm-stub.c         |   18 -----------
 target-ppc/Makefile.objs       |    1 -
 target-ppc/kvm-stub.c          |   18 -----------
 9 files changed, 64 insertions(+), 70 deletions(-)
 rename kvm-stub.c => stubs/kvm-stub.c (63%)
 delete mode 100644 target-arm/kvm-stub.c
 delete mode 100644 target-i386/kvm-stub.c
 delete mode 100644 target-ppc/kvm-stub.c

diff --git a/Makefile.target b/Makefile.target
index 9a49852..21a817f 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -84,7 +84,6 @@ obj-y += fpu/softfloat.o
 obj-y += target-$(TARGET_BASE_ARCH)/
 obj-y += disas.o
 obj-$(CONFIG_GDBSTUB_XML) += gdbstub-xml.o
-obj-$(CONFIG_NO_KVM) += kvm-stub.o
 
 #########################################################
 # Linux user emulator target
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 9b701b4..44bf573 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -10,6 +10,7 @@ stub-obj-y += fdset-remove-fd.o
 stub-obj-y += get-fd.o
 stub-obj-y += get-vm-name.o
 stub-obj-y += iothread-lock.o
+stub-obj-y += kvm-stub.o
 stub-obj-y += migr-blocker.o
 stub-obj-y += mon-is-qmp.o
 stub-obj-y += mon-printf.o
diff --git a/kvm-stub.c b/stubs/kvm-stub.c
similarity index 63%
rename from kvm-stub.c
rename to stubs/kvm-stub.c
index 370c837..cc0fd06 100644
--- a/kvm-stub.c
+++ b/stubs/kvm-stub.c
@@ -10,14 +10,33 @@
  *
  */
 
+/* We don't want to include exec/poison.h here. */
+#define HW_POISON_H
+
+/* Several data types are only used for unused function arguments
+   or in files which are included.
+   Define them here to avoid include dependencies. */
+
+typedef struct CPUArchState CPUArchState;
+typedef unsigned target_ulong;
+
+typedef struct ARMCPU ARMCPU;
+
+typedef struct CPUPPCState CPUPPCState;
+typedef struct PowerPCCPU PowerPCCPU;
+
+extern unsigned ram_size;
+
 #include "qemu-common.h"
 #include "hw/hw.h"
-#include "cpu.h"
-#include "sysemu/kvm.h"
 
-#ifndef CONFIG_USER_ONLY
+/* Pretend that cpu.h was included to get all prototypes. */
+#define NEED_CPU_H
+#include "sysemu/kvm.h"
+#include "hw/ppc/openpic.h"
 #include "hw/pci/msi.h"
-#endif
+#include "target-arm/kvm_arm.h"
+#include "target-ppc/kvm_ppc.h"
 
 KVMState *kvm_state;
 bool kvm_kernel_irqchip;
@@ -74,6 +93,11 @@ int kvm_has_pit_state2(void)
     return 0;
 }
 
+void *kvm_ram_alloc(ram_addr_t size)
+{
+    g_assert_not_reached();
+}
+
 void kvm_setup_guest_memory(void *start, size_t size)
 {
 }
@@ -102,7 +126,7 @@ void kvm_remove_all_breakpoints(CPUState *cpu)
 #ifndef _WIN32
 int kvm_set_signal_mask(CPUState *cpu, const sigset_t *sigset)
 {
-    abort();
+    g_assert_not_reached();
 }
 #endif
 
@@ -116,7 +140,8 @@ int kvm_on_sigbus(int code, void *addr)
     return 1;
 }
 
-#ifndef CONFIG_USER_ONLY
+/* Stubs for system emulation. */
+
 int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
 {
     return -ENOSYS;
@@ -144,4 +169,35 @@ int kvm_irqchip_remove_irqfd_notifier(KVMState *s, EventNotifier *n, int virq)
 {
     return -ENOSYS;
 }
-#endif
+
+/* Stubs for target-arm. */
+
+bool write_kvmstate_to_list(ARMCPU *cpu)
+{
+    g_assert_not_reached();
+}
+
+bool write_list_to_kvmstate(ARMCPU *cpu)
+{
+    g_assert_not_reached();
+}
+
+/* Stubs for target-i386. */
+
+uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
+                                      uint32_t index, int reg)
+{
+    g_assert_not_reached();
+}
+
+/* Stubs for target-ppc. */
+
+int kvm_openpic_connect_vcpu(DeviceState *d, CPUState *cs)
+{
+    return -EINVAL;
+}
+
+void kvmppc_init(void)
+{
+    g_assert_not_reached();
+}
diff --git a/target-arm/Makefile.objs b/target-arm/Makefile.objs
index 4a6e52e..d89b57c 100644
--- a/target-arm/Makefile.objs
+++ b/target-arm/Makefile.objs
@@ -1,6 +1,5 @@
 obj-y += arm-semi.o
 obj-$(CONFIG_SOFTMMU) += machine.o
 obj-$(CONFIG_KVM) += kvm.o
-obj-$(CONFIG_NO_KVM) += kvm-stub.o
 obj-y += translate.o op_helper.o helper.o cpu.o
 obj-y += neon_helper.o iwmmxt_helper.o
diff --git a/target-arm/kvm-stub.c b/target-arm/kvm-stub.c
deleted file mode 100644
index cd1849f..0000000
--- a/target-arm/kvm-stub.c
+++ /dev/null
@@ -1,23 +0,0 @@
-/*
- * QEMU KVM ARM specific function stubs
- *
- * Copyright Linaro Limited 2013
- *
- * Author: Peter Maydell <peter.maydell@linaro.org>
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- *
- */
-#include "qemu-common.h"
-#include "kvm_arm.h"
-
-bool write_kvmstate_to_list(ARMCPU *cpu)
-{
-    abort();
-}
-
-bool write_list_to_kvmstate(ARMCPU *cpu)
-{
-    abort();
-}
diff --git a/target-i386/Makefile.objs b/target-i386/Makefile.objs
index c1d4f05..963698a 100644
--- a/target-i386/Makefile.objs
+++ b/target-i386/Makefile.objs
@@ -3,6 +3,5 @@ obj-y += excp_helper.o fpu_helper.o cc_helper.o int_helper.o svm_helper.o
 obj-y += smm_helper.o misc_helper.o mem_helper.o seg_helper.o
 obj-$(CONFIG_SOFTMMU) += machine.o arch_memory_mapping.o arch_dump.o
 obj-$(CONFIG_KVM) += kvm.o hyperv.o
-obj-$(CONFIG_NO_KVM) += kvm-stub.o
 obj-$(CONFIG_LINUX_USER) += ioport-user.o
 obj-$(CONFIG_BSD_USER) += ioport-user.o
diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c
deleted file mode 100644
index 11429c4..0000000
--- a/target-i386/kvm-stub.c
+++ /dev/null
@@ -1,18 +0,0 @@
-/*
- * QEMU KVM x86 specific function stubs
- *
- * Copyright Linaro Limited 2012
- *
- * Author: Peter Maydell <peter.maydell@linaro.org>
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- *
- */
-#include "qemu-common.h"
-#include "kvm_i386.h"
-
-bool kvm_allows_irq0_override(void)
-{
-    return 1;
-}
diff --git a/target-ppc/Makefile.objs b/target-ppc/Makefile.objs
index 6e78cb3..2c43c34 100644
--- a/target-ppc/Makefile.objs
+++ b/target-ppc/Makefile.objs
@@ -5,7 +5,6 @@ obj-y += machine.o mmu_helper.o mmu-hash32.o
 obj-$(TARGET_PPC64) += mmu-hash64.o
 endif
 obj-$(CONFIG_KVM) += kvm.o kvm_ppc.o
-obj-$(CONFIG_NO_KVM) += kvm-stub.o
 obj-y += excp_helper.o
 obj-y += fpu_helper.o
 obj-y += int_helper.o
diff --git a/target-ppc/kvm-stub.c b/target-ppc/kvm-stub.c
deleted file mode 100644
index ee3f5d2..0000000
--- a/target-ppc/kvm-stub.c
+++ /dev/null
@@ -1,18 +0,0 @@
-/*
- * QEMU KVM PPC specific function stubs
- *
- * Copyright Freescale Inc. 2013
- *
- * Author: Alexander Graf <agraf@suse.de>
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- *
- */
-#include "qemu-common.h"
-#include "hw/ppc/openpic.h"
-
-int kvm_openpic_connect_vcpu(DeviceState *d, CPUState *cs)
-{
-    return -EINVAL;
-}
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] kvm: Combine all kvm stubs in a single file and compile it only once

[Andreas Färber]

Hi Stefan,

Am 21.07.2013 12:57, schrieb Stefan Weil:
> The KVM stub variables and functions don't depend on target specific data
> types, so it is possible to compile kvm-stub.c only once.
> 
> Integrating the target specific KVM stubs for ARM, I386 and PPC in the
> common kvm-stub.c further simplifies the build environment and allows
> removing CONFIG_NO_KVM.
> 
> Instead of 53 kvm-stub.o files, there is now only one file.
> 
> abort() is replaced by g_assert_not_reached() which gives better diagnostic
> messages when it is called.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> 
> The resulting binary is slightly larger than before because it
> includes more stub functions. It could be made smaller by adding
> separate stubs/kvm-stub-arm.c, stubs/kvm-stub-i386.c and
> stubs/kvm-stub-ppc.c files. Using alias symbols for the stub
> functions would reduce the size further, but I don't think
> the size is critical here.
> 
> Should we call g_assert_not_reached() in more (all?) stub functions?
> 
> If the patch is accepted, a similar modification could be done
> for xen-stub.c.

I had thought about turning kvm-stub.c into stubs/kvm.c but rejected
that idea at the time due to CPU dependencies beyond CPUState.
Also I was wondering whether all stubs are actually from kvm-all.c (fine
then) or whether we would be opening a door for silently forgetting to
implement some functions in a new KVM target such as MIPS.

In the current form I think it's a gross hack wrt target_ulong and
HW_POISON_H. ;)

And yes, I would prefer to keep target stubs in separate files.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] kvm: Combine all kvm stubs in a single file and compile it only once

[Stefan Weil]

Am 21.07.2013 13:20, schrieb Andreas Färber:
> Hi Stefan,
>
> Am 21.07.2013 12:57, schrieb Stefan Weil:
>> The KVM stub variables and functions don't depend on target specific data
>> types, so it is possible to compile kvm-stub.c only once.
>>
>> Integrating the target specific KVM stubs for ARM, I386 and PPC in the
>> common kvm-stub.c further simplifies the build environment and allows
>> removing CONFIG_NO_KVM.
>>
>> Instead of 53 kvm-stub.o files, there is now only one file.
>>
>> abort() is replaced by g_assert_not_reached() which gives better diagnostic
>> messages when it is called.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>
>> The resulting binary is slightly larger than before because it
>> includes more stub functions. It could be made smaller by adding
>> separate stubs/kvm-stub-arm.c, stubs/kvm-stub-i386.c and
>> stubs/kvm-stub-ppc.c files. Using alias symbols for the stub
>> functions would reduce the size further, but I don't think
>> the size is critical here.
>>
>> Should we call g_assert_not_reached() in more (all?) stub functions?
>>
>> If the patch is accepted, a similar modification could be done
>> for xen-stub.c.
> I had thought about turning kvm-stub.c into stubs/kvm.c but rejected
> that idea at the time due to CPU dependencies beyond CPUState.
> Also I was wondering whether all stubs are actually from kvm-all.c (fine
> then) or whether we would be opening a door for silently forgetting to
> implement some functions in a new KVM target such as MIPS.
>
> In the current form I think it's a gross hack wrt target_ulong and
> HW_POISON_H. ;)
>
> And yes, I would prefer to keep target stubs in separate files.
>
> Regards,
> Andreas

The hack could be removed if the include files were cleaned.
Do we still have to poison "env", for example? It is no longer
a global variable. The handling of cpu.h is also a mess.

The hack for target_ulong could also be avoided by using
modified kvm_insert_breakpoint, kvm_remove_breakpoint.

Fixing the existing include files was beyond the scope of my
patch.

Mixing kvm-all.o and kvm-stub.o would be detected by the
linker, so missing KVM interfaces for new KVM targets is
not possible.

Regards,
Stefan