Re: [Qemu-devel] Re: [PATCH] vhost: fix features ack

[Anthony Liguori <anthony@codemonkey.ws>]

[-- Attachment #1: Type: text/plain, Size: 2886 bytes --]

On 03/31/2010 02:38 PM, Blue Swirl wrote:
> On 3/31/10, Michael S. Tsirkin<mst@redhat.com>  wrote:
>    
>> On Wed, Mar 31, 2010 at 03:38:05PM -0300, Luiz Capitulino wrote:
>>   >  On Wed, 31 Mar 2010 13:26:23 -0500
>>   >  Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>   >
>>   >  >  On 03/31/2010 01:20 PM, Michael S. Tsirkin wrote:
>>   >  >  >  From: David L Stevens<dlstevens@us.ibm.com>
>>   >  >  >
>>   >  >  >  vhost driver in qemu didn't ack features, and this happens
>>   >  >  >  to work because we don't really require any features. However,
>>   >  >  >  it's better not to rely on this. This patch passes features to
>>   >  >  >  vhost as guest acks them.
>>   >  >  >
>>   >  >  >  Signed-off-by: David L Stevens<dlstevens@us.ibm.com>
>>   >  >  >  Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>>   >  >  >  ---
>>   >  >  >
>>   >  >  >  Anthony, here's a fixup patch to address an issue in vhost
>>   >  >  >  patches. Incidentially, what's the status of the vhost patchset?
>>   >  >  >
>>   >  >
>>   >  >  http://repo.or.cz/w/qemu/aliguori-queue.git vhost
>>   >  >
>>   >  >  Is what I'm currently testing.  With vhost disabled,  the following seg
>>   >  >  faults:
>>   >  >
>>   >  >  qemu-system-x86_64 -hda ~/images/linux.img -net tap -net
>>   >  >  nic,model=virtio -enable-kvm
>>   >  >
>>   >  >  But not when using TCG.  I'm not sure that it's your patches at fault
>>   >  >  and I'm attempting to bisect now to figure that out.
>>   >
>>   >   Probably this is the same segfault I'm getting right now in master,
>>   >  bisect says it's:
>>   >
>>   >  """
>>   >  commit ad96090a01d848df67d70c5259ed8aa321fa8716
>>   >  Author: Blue Swirl<blauwirbel@gmail.com>
>>   >  Date:   Mon Mar 29 19:23:52 2010 +0000
>>   >
>>   >      Refactor target specific handling, compile vl.c only once
>>   >  """
>>
>>   Why are the compile once patches helpful? They seem to introduce
>>   churn and bugs, they actively make it harder to extend qemu as you can't use
>>   target-specific code in code that is compiled once, they might have
>>   performance penalty - and what do we gain? Any given user is unlikely to
>>   need to build on more than one target, distros have enough computing
>>   power to build in parallel.
>>      
> As has been explained many times, knowledge about CPU specific
> features has no place in devices. Actively removing CPU specifics from
> devices is good but preventing new bad code to be committed is better.
>
> I don't have distro computing powers (unless some distro's compute
> center only has two low power machines) and I guess some other
> developers don't have either. All developers and patch submitters are
> expected to compile all targets. This patch set has decreased the
> number of files compiled by about 20%.
>    

BTW, this seems to do it.  I'll commit after some testing.

Regards,

Anthony Liguori

[-- Attachment #2: 0001-Fix-enable-kvm.patch --]
[-- Type: text/plain, Size: 4387 bytes --]

From 7c1920d330fcf7ed5b477bc8e22ca4f7e5e2b343 Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
Date: Wed, 31 Mar 2010 14:20:11 -0500
Subject: [PATCH] Fix -enable-kvm

commit ad96090a01d848df67d70c5259ed8aa321fa8716
Author: Blue Swirl <blauwirbel@gmail.com>
Date:   Mon Mar 29 19:23:52 2010 +0000

    Refactor target specific handling, compile vl.c only once

Introduced a regression in -enable-kvm because it left CONFIG_KVM in kvm.h
while allowing compiled-once objects to use kvm.h.  CONFIG_KVM is set on a
per-target basis.

commit 53b67b3052f39b049bc7c79ae1ce132c90098c6c
Author: Blue Swirl <blauwirbel@gmail.com>
Date:   Mon Mar 29 19:23:52 2010 +0000

    Compile acpi only once

Also regressed -enable-kvm because of a thinko.

Once we make these changes, the if(0) magic of kvm_enabled() no longer works
so we need empty stub functions.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/acpi.c     |    3 +-
 kvm-generic.c |  133 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 kvm.h         |    9 ++--
 3 files changed, 139 insertions(+), 6 deletions(-)
 create mode 100644 kvm-generic.c

diff --git a/hw/acpi.c b/hw/acpi.c
index 33c6bc8..5c01c2e 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -529,7 +529,8 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
 
     register_ioport_write(ACPI_DBG_IO_ADDR, 4, 4, acpi_dbg_writel, s);
 
-    if (kvm_enabled) {
+    s->kvm_enabled = kvm_enabled;
+    if (s->kvm_enabled) {
         /* Mark SMM as already inited to prevent SMM from running.  KVM does not
          * support SMM mode. */
         pci_conf[0x5B] = 0x02;
diff --git a/kvm-generic.c b/kvm-generic.c
new file mode 100644
index 0000000..c67fb55
--- /dev/null
+++ b/kvm-generic.c
@@ -0,0 +1,133 @@
+/*
+ * QEMU KVM support
+ *
+ * Copyright IBM, Corp. 2010
+ *           Red Hat, Inc. 2008
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *  Glauber Costa     <gcosta@redhat.com>
+ *
+ * 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 "cpu.h"
+#include "kvm.h"
+
+int kvm_init(int smp_cpus)
+{
+    return -ENOSYS;
+}
+
+int kvm_init_vcpu(CPUState *env)
+{
+    return -ENOSYS;
+}
+
+int kvm_cpu_exec(CPUState *env)
+{
+    return -ENOSYS;
+}
+
+int kvm_log_start(target_phys_addr_t phys_addr, ram_addr_t size)
+{
+    return -ENOSYS;
+}
+
+int kvm_log_stop(target_phys_addr_t phys_addr, ram_addr_t size)
+{
+    return -ENOSYS;
+}
+
+int kvm_has_sync_mmu(void)
+{
+    return -ENOSYS;
+}
+
+int kvm_has_vcpu_events(void)
+{
+    return -ENOSYS;
+}
+
+int kvm_has_robust_singlestep(void)
+{
+    return -ENOSYS;
+}
+
+void kvm_setup_guest_memory(void *start, size_t size)
+{
+}
+
+int kvm_coalesce_mmio_region(target_phys_addr_t start, ram_addr_t size)
+{
+    return -ENOSYS;
+}
+
+int kvm_uncoalesce_mmio_region(target_phys_addr_t start, ram_addr_t size)
+{
+    return -ENOSYS;
+}
+
+void kvm_flush_coalesced_mmio_buffer(void)
+{
+}
+
+int kvm_insert_breakpoint(CPUState *current_env, target_ulong addr,
+                          target_ulong len, int type)
+{
+    return -ENOSYS;
+}
+
+int kvm_remove_breakpoint(CPUState *current_env, target_ulong addr,
+                          target_ulong len, int type)
+{
+    return -ENOSYS;
+}
+
+void kvm_remove_all_breakpoints(CPUState *current_env)
+{
+}
+
+int kvm_update_guest_debug(CPUState *env, unsigned long reinject_trap)
+{
+    return -ENOSYS;
+}
+
+#ifndef _WIN32
+int kvm_set_signal_mask(CPUState *env, const sigset_t *sigset)
+{
+    return -ENOSYS;
+}
+#endif
+
+int kvm_pit_in_kernel(void)
+{
+    return -ENOSYS;
+}
+
+int kvm_irqchip_in_kernel(void)
+{
+    return -ENOSYS;
+}
+
+void kvm_cpu_synchronize_state(CPUState *env)
+{
+}
+
+void kvm_cpu_synchronize_post_reset(CPUState *env)
+{
+}
+
+void kvm_cpu_synchronize_post_init(CPUState *env)
+{
+}
+
+/* This is evil but the KVM PPC code needs refactoring */
+void kvmppc_init(void);
+
+void kvmppc_init(void)
+{
+}
diff --git a/kvm.h b/kvm.h
index 4f77188..9f57fd5 100644
--- a/kvm.h
+++ b/kvm.h
@@ -19,11 +19,10 @@
 
 extern int kvm_allowed;
 
-#ifdef CONFIG_KVM
-#define kvm_enabled() (kvm_allowed)
-#else
-#define kvm_enabled() (0)
-#endif
+static inline int kvm_enabled(void)
+{
+    return kvm_allowed;
+}
 
 struct kvm_run;
 
-- 
1.6.5.2