[PATCH][QEMU] Use a separate device for in-kernel PIT

[PATCH][QEMU] Use a separate device for in-kernel PIT

[Anthony Liguori]

Part of the feedback we received from Fabrice about the KVM patches for QEMU
is that we should create a separate device for the in-kernel APIC to avoid
having lots of if (kvm_enabled()) within the APIC code that were difficult to
understand why there were needed.

This patch separates the in-kernel PIT into a separate device.  It also
introduces some configure logic to only compile in support for the in-kernel
PIT if it's available.

The result of this is that we now only need a single if (kvm_enabled()) to
determine which device to use.  Besides making it more upstream friendly, I
think this makes the code much easier to understand.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/qemu/Makefile.target b/qemu/Makefile.target
index 67a433e..2579688 100644
--- a/qemu/Makefile.target
+++ b/qemu/Makefile.target
@@ -585,6 +585,9 @@ OBJS+= ide.o pckbd.o ps2.o vga.o $(SOUND_HW) dma.o
 OBJS+= fdc.o mc146818rtc.o serial.o i8259.o i8254.o pcspk.o pc.o
 OBJS+= cirrus_vga.o apic.o parallel.o acpi.o piix_pci.o
 OBJS+= usb-uhci.o vmmouse.o vmport.o vmware_vga.o extboot.o
+ifeq ($(USE_KVM_PIT), 1)
+OBJS+= i8254-kvm.o
+endif
 CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE
 endif
 ifeq ($(TARGET_BASE_ARCH), ia64)
diff --git a/qemu/configure b/qemu/configure
index bbedddc..22f44c8 100755
--- a/qemu/configure
+++ b/qemu/configure
@@ -100,6 +100,7 @@ bsd="no"
 linux="no"
 kqemu="no"
 kvm="no"
+kvm_cap_pit="no"
 profiler="no"
 kernel_path=""
 cocoa="no"
@@ -612,6 +613,22 @@ int main(void) {
 EOF
 
 ##########################################
+# KVM probe
+
+if test "$kvm" = "yes" ; then
+cat > $TMPC <<EOF
+#include <libkvm.h>
+#ifndef KVM_CAP_PIT
+#error "kvm no pit capability"
+#endif
+int main(void) { return 0; }
+EOF
+    if $cc $ARCH_CFLAGS -o $TMPE ${OS_CFLAGS} $TMPC 2> /dev/null ; then
+	kvm_cap_pit="yes"
+    fi
+fi
+
+##########################################
 # SDL probe
 
 sdl_too_old=no
@@ -1136,6 +1153,9 @@ configure_kvm() {
     echo "#define USE_KVM 1" >> $config_h
     echo "USE_KVM=1" >> $config_mak
     echo "CONFIG_KVM_KERNEL_INC=$kernel_path/include" >> $config_mak
+    if test $kvm_cap_pit = "yes" ; then
+	echo "USE_KVM_PIT=1" >> $config_mak
+    fi
     disable_cpu_emulation
   fi
 }
diff --git a/qemu/hw/i8254-kvm.c b/qemu/hw/i8254-kvm.c
new file mode 100644
index 0000000..f0669cb
--- /dev/null
+++ b/qemu/hw/i8254-kvm.c
@@ -0,0 +1,178 @@
+/*
+ * QEMU 8253/8254 interval timer emulation
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include "hw.h"
+#include "pc.h"
+#include "isa.h"
+#include "i8254.h"
+
+#include "qemu-kvm.h"
+
+static PITState pit_state;
+
+static void kvm_kernel_pit_save_to_user(PITState *s)
+{
+    struct kvm_pit_state pit;
+    struct kvm_pit_channel_state *c;
+    struct PITChannelState *sc;
+    int i;
+
+    kvm_get_pit(kvm_context, &pit);
+
+    for (i = 0; i < 3; i++) {
+	c = &pit.channels[i];
+	sc = &s->channels[i];
+	sc->count = c->count;
+	sc->latched_count = c->latched_count;
+	sc->count_latched = c->count_latched;
+	sc->status_latched = c->status_latched;
+	sc->status = c->status;
+	sc->read_state = c->read_state;
+	sc->write_state = c->write_state;
+	sc->write_latch = c->write_latch;
+	sc->rw_mode = c->rw_mode;
+	sc->mode = c->mode;
+	sc->bcd = c->bcd;
+	sc->gate = c->gate;
+	sc->count_load_time = c->count_load_time;
+    }
+}
+
+static void kvm_kernel_pit_load_from_user(PITState *s)
+{
+    struct kvm_pit_state pit;
+    struct kvm_pit_channel_state *c;
+    struct PITChannelState *sc;
+    int i;
+
+    for (i = 0; i < 3; i++) {
+	c = &pit.channels[i];
+	sc = &s->channels[i];
+	c->count = sc->count;
+	c->latched_count = sc->latched_count;
+	c->count_latched = sc->count_latched;
+	c->status_latched = sc->status_latched;
+	c->status = sc->status;
+	c->read_state = sc->read_state;
+	c->write_state = sc->write_state;
+	c->write_latch = sc->write_latch;
+	c->rw_mode = sc->rw_mode;
+	c->mode = sc->mode;
+	c->bcd = sc->bcd;
+	c->gate = sc->gate;
+	c->count_load_time = sc->count_load_time;
+    }
+
+    kvm_set_pit(kvm_context, &pit);
+}
+
+static void pit_save(QEMUFile *f, void *opaque)
+{
+    PITState *pit = opaque;
+    PITChannelState *s;
+    int i;
+
+    kvm_kernel_pit_save_to_user(pit);
+
+    for(i = 0; i < 3; i++) {
+        s = &pit->channels[i];
+        qemu_put_be32(f, s->count);
+        qemu_put_be16s(f, &s->latched_count);
+        qemu_put_8s(f, &s->count_latched);
+        qemu_put_8s(f, &s->status_latched);
+        qemu_put_8s(f, &s->status);
+        qemu_put_8s(f, &s->read_state);
+        qemu_put_8s(f, &s->write_state);
+        qemu_put_8s(f, &s->write_latch);
+        qemu_put_8s(f, &s->rw_mode);
+        qemu_put_8s(f, &s->mode);
+        qemu_put_8s(f, &s->bcd);
+        qemu_put_8s(f, &s->gate);
+        qemu_put_be64(f, s->count_load_time);
+    }
+}
+
+static int pit_load(QEMUFile *f, void *opaque, int version_id)
+{
+    PITState *pit = opaque;
+    PITChannelState *s;
+    int i;
+
+    if (version_id != 1)
+        return -EINVAL;
+
+    for(i = 0; i < 3; i++) {
+        s = &pit->channels[i];
+        s->count=qemu_get_be32(f);
+        qemu_get_be16s(f, &s->latched_count);
+        qemu_get_8s(f, &s->count_latched);
+        qemu_get_8s(f, &s->status_latched);
+        qemu_get_8s(f, &s->status);
+        qemu_get_8s(f, &s->read_state);
+        qemu_get_8s(f, &s->write_state);
+        qemu_get_8s(f, &s->write_latch);
+        qemu_get_8s(f, &s->rw_mode);
+        qemu_get_8s(f, &s->mode);
+        qemu_get_8s(f, &s->bcd);
+        qemu_get_8s(f, &s->gate);
+        s->count_load_time=qemu_get_be64(f);
+    }
+
+    kvm_kernel_pit_load_from_user(pit);
+
+    return 0;
+}
+
+static inline void pit_load_count(PITChannelState *s, int val)
+{
+    if (val == 0)
+        val = 0x10000;
+    s->count_load_time = qemu_get_clock(vm_clock);
+    s->count = val;
+}
+
+static void pit_reset(void *opaque)
+{
+    PITState *pit = opaque;
+    PITChannelState *s;
+    int i;
+
+    for(i = 0;i < 3; i++) {
+        s = &pit->channels[i];
+        s->mode = 3;
+        s->gate = (i != 2);
+        pit_load_count(s, 0);
+    }
+}
+
+PITState *kvm_pit_init(int base, qemu_irq irq)
+{
+    PITState *pit = &pit_state;
+
+    register_savevm("i8254", base, 1, pit_save, pit_load, pit);
+
+    qemu_register_reset(pit_reset, pit);
+    pit_reset(pit);
+
+    return pit;
+}
diff --git a/qemu/hw/i8254.c b/qemu/hw/i8254.c
index e215f8b..18ed5b6 100644
--- a/qemu/hw/i8254.c
+++ b/qemu/hw/i8254.c
@@ -25,40 +25,10 @@
 #include "pc.h"
 #include "isa.h"
 #include "qemu-timer.h"
-
-#include "qemu-kvm.h"
+#include "i8254.h"
 
 //#define DEBUG_PIT
 
-#define RW_STATE_LSB 1
-#define RW_STATE_MSB 2
-#define RW_STATE_WORD0 3
-#define RW_STATE_WORD1 4
-
-typedef struct PITChannelState {
-    int count; /* can be 65536 */
-    uint16_t latched_count;
-    uint8_t count_latched;
-    uint8_t status_latched;
-    uint8_t status;
-    uint8_t read_state;
-    uint8_t write_state;
-    uint8_t write_latch;
-    uint8_t rw_mode;
-    uint8_t mode;
-    uint8_t bcd; /* not supported */
-    uint8_t gate; /* timer start */
-    int64_t count_load_time;
-    /* irq handling */
-    int64_t next_transition_time;
-    QEMUTimer *irq_timer;
-    qemu_irq irq;
-} PITChannelState;
-
-struct PITState {
-    PITChannelState channels[3];
-};
-
 static PITState pit_state;
 
 static void pit_irq_timer_update(PITChannelState *s, int64_t current_time);
@@ -414,78 +384,12 @@ static void pit_irq_timer(void *opaque)
     pit_irq_timer_update(s, s->next_transition_time);
 }
 
-#ifdef KVM_CAP_PIT
-
-static void kvm_kernel_pit_save_to_user(PITState *s)
-{
-    struct kvm_pit_state pit;
-    struct kvm_pit_channel_state *c;
-    struct PITChannelState *sc;
-    int i;
-
-    kvm_get_pit(kvm_context, &pit);
-
-    for (i = 0; i < 3; i++) {
-	c = &pit.channels[i];
-	sc = &s->channels[i];
-	sc->count = c->count;
-	sc->latched_count = c->latched_count;
-	sc->count_latched = c->count_latched;
-	sc->status_latched = c->status_latched;
-	sc->status = c->status;
-	sc->read_state = c->read_state;
-	sc->write_state = c->write_state;
-	sc->write_latch = c->write_latch;
-	sc->rw_mode = c->rw_mode;
-	sc->mode = c->mode;
-	sc->bcd = c->bcd;
-	sc->gate = c->gate;
-	sc->count_load_time = c->count_load_time;
-    }
-}
-
-static void kvm_kernel_pit_load_from_user(PITState *s)
-{
-    struct kvm_pit_state pit;
-    struct kvm_pit_channel_state *c;
-    struct PITChannelState *sc;
-    int i;
-
-    for (i = 0; i < 3; i++) {
-	c = &pit.channels[i];
-	sc = &s->channels[i];
-	c->count = sc->count;
-	c->latched_count = sc->latched_count;
-	c->count_latched = sc->count_latched;
-	c->status_latched = sc->status_latched;
-	c->status = sc->status;
-	c->read_state = sc->read_state;
-	c->write_state = sc->write_state;
-	c->write_latch = sc->write_latch;
-	c->rw_mode = sc->rw_mode;
-	c->mode = sc->mode;
-	c->bcd = sc->bcd;
-	c->gate = sc->gate;
-	c->count_load_time = sc->count_load_time;
-    }
-
-    kvm_set_pit(kvm_context, &pit);
-}
-
-#endif
-
 static void pit_save(QEMUFile *f, void *opaque)
 {
     PITState *pit = opaque;
     PITChannelState *s;
     int i;
 
-#ifdef KVM_CAP_PIT
-    if (kvm_enabled() && qemu_kvm_pit_in_kernel()) {
-        kvm_kernel_pit_save_to_user(pit);
-    }
-#endif
-
     for(i = 0; i < 3; i++) {
         s = &pit->channels[i];
         qemu_put_be32(f, s->count);
@@ -537,13 +441,6 @@ static int pit_load(QEMUFile *f, void *opaque, int version_id)
             qemu_get_timer(f, s->irq_timer);
         }
     }
-
-#ifdef KVM_CAP_PIT
-    if (kvm_enabled() && qemu_kvm_pit_in_kernel()) {
-        kvm_kernel_pit_load_from_user(pit);
-    }
-#endif
-
     return 0;
 }
 
@@ -566,12 +463,10 @@ PITState *pit_init(int base, qemu_irq irq)
     PITState *pit = &pit_state;
     PITChannelState *s;
 
-    if (!kvm_enabled() || !qemu_kvm_pit_in_kernel()) {
-	    s = &pit->channels[0];
-	    /* the timer 0 is connected to an IRQ */
-	    s->irq_timer = qemu_new_timer(vm_clock, pit_irq_timer, s);
-	    s->irq = irq;
-    }
+    s = &pit->channels[0];
+    /* the timer 0 is connected to an IRQ */
+    s->irq_timer = qemu_new_timer(vm_clock, pit_irq_timer, s);
+    s->irq = irq;
 
     register_savevm("i8254", base, 1, pit_save, pit_load, pit);
 
diff --git a/qemu/hw/i8254.h b/qemu/hw/i8254.h
new file mode 100644
index 0000000..6ee86db
--- /dev/null
+++ b/qemu/hw/i8254.h
@@ -0,0 +1,36 @@
+#ifndef _QEMU_I8254_H
+#define _QEMU_I8254_H
+
+#include "hw.h"
+#include "qemu-timer.h"
+
+#define RW_STATE_LSB 1
+#define RW_STATE_MSB 2
+#define RW_STATE_WORD0 3
+#define RW_STATE_WORD1 4
+
+typedef struct PITChannelState {
+    int count; /* can be 65536 */
+    uint16_t latched_count;
+    uint8_t count_latched;
+    uint8_t status_latched;
+    uint8_t status;
+    uint8_t read_state;
+    uint8_t write_state;
+    uint8_t write_latch;
+    uint8_t rw_mode;
+    uint8_t mode;
+    uint8_t bcd; /* not supported */
+    uint8_t gate; /* timer start */
+    int64_t count_load_time;
+    /* irq handling */
+    int64_t next_transition_time;
+    QEMUTimer *irq_timer;
+    qemu_irq irq;
+} PITChannelState;
+
+struct PITState {
+    PITChannelState channels[3];
+};
+
+#endif
diff --git a/qemu/hw/pc.c b/qemu/hw/pc.c
index 1122b87..b8419f0 100644
--- a/qemu/hw/pc.c
+++ b/qemu/hw/pc.c
@@ -983,7 +983,10 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
     if (pci_enabled) {
         ioapic = ioapic_init();
     }
-    pit = pit_init(0x40, i8259[0]);
+    if (kvm_enabled() && qemu_kvm_pit_in_kernel())
+	pit = kvm_pit_init(0x40, i8259[0]);
+    else
+	pit = pit_init(0x40, i8259[0]);
     pcspk_init(pit);
     if (pci_enabled) {
         pic_set_alt_irq_func(isa_pic, ioapic_set_irq, ioapic);
diff --git a/qemu/hw/pc.h b/qemu/hw/pc.h
index f879f30..e0da49d 100644
--- a/qemu/hw/pc.h
+++ b/qemu/hw/pc.h
@@ -58,6 +58,10 @@ int pit_get_initial_count(PITState *pit, int channel);
 int pit_get_mode(PITState *pit, int channel);
 int pit_get_out(PITState *pit, int channel, int64_t current_time);
 
+/* i8254-kvm.c */
+
+PITState *kvm_pit_init(int base, qemu_irq irq);
+
 /* vmport.c */
 void vmport_init(CPUState *env);
 void vmport_register(unsigned char command, IOPortReadFunc *func, void *opaque);

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH][QEMU] Use a separate device for in-kernel PIT

[Dor Laor]

On Wed, 2008-03-12 at 12:38 -0500, Anthony Liguori wrote:
> Part of the feedback we received from Fabrice about the KVM patches
> for QEMU
> is that we should create a separate device for the in-kernel APIC to
> avoid
> having lots of if (kvm_enabled()) within the APIC code that were
> difficult to
> understand why there were needed.
> 
> This patch separates the in-kernel PIT into a separate device.  It
> also
> introduces some configure logic to only compile in support for the
> in-kernel
> PIT if it's available.
> 
> The result of this is that we now only need a single if
> (kvm_enabled()) to
> determine which device to use.  Besides making it more upstream
> friendly, I
> think this makes the code much easier to understand.
> 

Seems like a good idea.

[snip]
..



> +static void pit_reset(void *opaque)
> +{
> +    PITState *pit = opaque;
> +    PITChannelState *s;
> +    int i;
> +
> +    for(i = 0;i < 3; i++) {
> +        s = &pit->channels[i];
> +        s->mode = 3;
> +        s->gate = (i != 2);
> +        pit_load_count(s, 0);
> +    }
> +}
> +

Seems like pit_reset won't change the in-kernel pit state.
Actually this should handled as a part of more general reset ioctl to
all of kvm's in-kernel devices.

Cheers,
Dor


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH][QEMU] Use a separate device for in-kernel PIT

[Anthony Liguori]

Dor Laor wrote:
> On Wed, 2008-03-12 at 12:38 -0500, Anthony Liguori wrote:
>   
>> Part of the feedback we received from Fabrice about the KVM patches
>> for QEMU
>> is that we should create a separate device for the in-kernel APIC to
>> avoid
>> having lots of if (kvm_enabled()) within the APIC code that were
>> difficult to
>> understand why there were needed.
>>
>> This patch separates the in-kernel PIT into a separate device.  It
>> also
>> introduces some configure logic to only compile in support for the
>> in-kernel
>> PIT if it's available.
>>
>> The result of this is that we now only need a single if
>> (kvm_enabled()) to
>> determine which device to use.  Besides making it more upstream
>> friendly, I
>> think this makes the code much easier to understand.
>>
>>     
>
> Seems like a good idea.
>
> [snip]
> ..
>
>
>
>   
>> +static void pit_reset(void *opaque)
>> +{
>> +    PITState *pit = opaque;
>> +    PITChannelState *s;
>> +    int i;
>> +
>> +    for(i = 0;i < 3; i++) {
>> +        s = &pit->channels[i];
>> +        s->mode = 3;
>> +        s->gate = (i != 2);
>> +        pit_load_count(s, 0);
>> +    }
>> +}
>> +
>>     
>
> Seems like pit_reset won't change the in-kernel pit state.
>   

Yeah, that seemed suspicious to me too.  I didn't want to change the 
existing behavior though for fear of introducing regressions.  Perhaps 
Sheng can comment on whether it's necessary to even have a reset handler 
in userspace?

Regards,

Anthony Liguori

> Actually this should handled as a part of more general reset ioctl to
> all of kvm's in-kernel devices.
>
> Cheers,
> Dor
>
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH][QEMU] Use a separate device for in-kernel PIT

[Yang, Sheng]

On Thursday 13 March 2008 06:13:48 Anthony Liguori wrote:
> Dor Laor wrote:
> > On Wed, 2008-03-12 at 12:38 -0500, Anthony Liguori wrote:
> >> Part of the feedback we received from Fabrice about the KVM patches
> >> for QEMU
> >> is that we should create a separate device for the in-kernel APIC to
> >> avoid
> >> having lots of if (kvm_enabled()) within the APIC code that were
> >> difficult to
> >> understand why there were needed.
> >>
> >> This patch separates the in-kernel PIT into a separate device.  It
> >> also
> >> introduces some configure logic to only compile in support for the
> >> in-kernel
> >> PIT if it's available.
> >>
> >> The result of this is that we now only need a single if
> >> (kvm_enabled()) to
> >> determine which device to use.  Besides making it more upstream
> >> friendly, I
> >> think this makes the code much easier to understand.
> >
> > Seems like a good idea.
> >
> > [snip]
> > ..
> >
> >> +static void pit_reset(void *opaque)
> >> +{
> >> +    PITState *pit = opaque;
> >> +    PITChannelState *s;
> >> +    int i;
> >> +
> >> +    for(i = 0;i < 3; i++) {
> >> +        s = &pit->channels[i];
> >> +        s->mode = 3;
> >> +        s->gate = (i != 2);
> >> +        pit_load_count(s, 0);
> >> +    }
> >> +}
> >> +
> >
> > Seems like pit_reset won't change the in-kernel pit state.
>
> Yeah, that seemed suspicious to me too.  I didn't want to change the
> existing behavior though for fear of introducing regressions.  Perhaps
> Sheng can comment on whether it's necessary to even have a reset handler
> in userspace?

IMO the reset support is needed even it would introduce regression (e would 
finally solve the regression if exists, won't we? :) ).

And we got two choices in userspace: one ioctl to reset all kvm devices, or 
one ioctl for each device. For we are separating in kernel device into 
separate devices, seems the later is more proper. But would it bring other 
troubles like inconsistent state for smp? 

Thanks
Yang, Sheng

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH][QEMU] Use a separate device for in-kernel PIT

[Avi Kivity]

Yang, Sheng wrote:
> And we got two choices in userspace: one ioctl to reset all kvm devices, or 
> one ioctl for each device. For we are separating in kernel device into 
> separate devices, seems the later is more proper. But would it bring other 
> troubles like inconsistent state for smp? 
>
>   

I agree that a separate ioctl would introduce smp problems, so I think 
one ioctl that resets all devices and all vcpus is the way to go.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH][QEMU] Use a separate device for in-kernel PIT

[Avi Kivity]

Anthony Liguori wrote:
> Part of the feedback we received from Fabrice about the KVM patches for QEMU
> is that we should create a separate device for the in-kernel APIC to avoid
> having lots of if (kvm_enabled()) within the APIC code that were difficult to
> understand why there were needed.
>
> This patch separates the in-kernel PIT into a separate device.  It also
> introduces some configure logic to only compile in support for the in-kernel
> PIT if it's available.
>
> The result of this is that we now only need a single if (kvm_enabled()) to
> determine which device to use.  Besides making it more upstream friendly, I
> think this makes the code much easier to understand.
>
>   

It introduces a new issue, the save/restore format is effectively forked 
and will have to be maintained in parallel if there are any changes.

Perhaps keep in the same file, but as two separate devices that can 
share the save/restore code?

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [kvm-ppc-devel] [kvm-devel] [PATCH][QEMU] Use a separate device

[Hollis Blanchard]

On Wed, 2008-03-12 at 12:38 -0500, Anthony Liguori wrote:
> Part of the feedback we received from Fabrice about the KVM patches
> for QEMU
> is that we should create a separate device for the in-kernel APIC to
> avoid
> having lots of if (kvm_enabled()) within the APIC code that were
> difficult to
> understand why there were needed.
> 
> This patch separates the in-kernel PIT into a separate device.  It
> also
> introduces some configure logic to only compile in support for the
> in-kernel
> PIT if it's available.
> 
> The result of this is that we now only need a single if
> (kvm_enabled()) to
> determine which device to use.  Besides making it more upstream
> friendly, I
> think this makes the code much easier to understand.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

This patch solves annoying qemu build breakage hitting PowerPC around
struct kvm_pit_state, so that's another vote in favor...

-- 
Hollis Blanchard
IBM Linux Technology Center


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

Re: [PATCH][QEMU] Use a separate device for in-kernel PIT

[Hollis Blanchard]

On Wed, 2008-03-12 at 12:38 -0500, Anthony Liguori wrote:
> Part of the feedback we received from Fabrice about the KVM patches
> for QEMU
> is that we should create a separate device for the in-kernel APIC to
> avoid
> having lots of if (kvm_enabled()) within the APIC code that were
> difficult to
> understand why there were needed.
> 
> This patch separates the in-kernel PIT into a separate device.  It
> also
> introduces some configure logic to only compile in support for the
> in-kernel
> PIT if it's available.
> 
> The result of this is that we now only need a single if
> (kvm_enabled()) to
> determine which device to use.  Besides making it more upstream
> friendly, I
> think this makes the code much easier to understand.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

This patch solves annoying qemu build breakage hitting PowerPC around
struct kvm_pit_state, so that's another vote in favor...

-- 
Hollis Blanchard
IBM Linux Technology Center


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [kvm-ppc-devel] [kvm-devel] [PATCH][QEMU] Use a separate device

[Anthony Liguori]

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

Hollis Blanchard wrote:
> This patch solves annoying qemu build breakage hitting PowerPC around
> struct kvm_pit_state, so that's another vote in favor...
>   

I have an updated version of the patch but it's breaking the build b/c 
something fouled up right now with configure.  libkvm pulls in 
linux/kvm.h which wants to pull in linux/compiler.h.  We don't ship a 
linux/compiler.h though so it's pulling from /usr/include/linux which on 
my system doesn't have a compiler.h.

The lack of this header is causing the configure test to fail.  I've 
attached the patch here for you to use and I'll send it out again once I 
figure out the fix for this linux/compiler.h.

Regards,

Anthony Liguori



[-- Attachment #2: qemu:cleanup_pit.patch --]
[-- Type: application/mbox, Size: 13983 bytes --]

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 170 bytes --]

_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

Re: [PATCH][QEMU] Use a separate device for in-kernel PIT

[Anthony Liguori]

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

Hollis Blanchard wrote:
> This patch solves annoying qemu build breakage hitting PowerPC around
> struct kvm_pit_state, so that's another vote in favor...
>   

I have an updated version of the patch but it's breaking the build b/c 
something fouled up right now with configure.  libkvm pulls in 
linux/kvm.h which wants to pull in linux/compiler.h.  We don't ship a 
linux/compiler.h though so it's pulling from /usr/include/linux which on 
my system doesn't have a compiler.h.

The lack of this header is causing the configure test to fail.  I've 
attached the patch here for you to use and I'll send it out again once I 
figure out the fix for this linux/compiler.h.

Regards,

Anthony Liguori



[-- Attachment #2: qemu:cleanup_pit.patch --]
[-- Type: application/mbox, Size: 13983 bytes --]

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 158 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [kvm-ppc-devel] [kvm-devel] [PATCH][QEMU] Use a separate device

[Avi Kivity]

Anthony Liguori wrote:
> Hollis Blanchard wrote:
>> This patch solves annoying qemu build breakage hitting PowerPC around
>> struct kvm_pit_state, so that's another vote in favor...
>>   
>
> I have an updated version of the patch but it's breaking the build b/c 
> something fouled up right now with configure.  libkvm pulls in 
> linux/kvm.h which wants to pull in linux/compiler.h.  We don't ship a 
> linux/compiler.h though so it's pulling from /usr/include/linux which 
> on my system doesn't have a compiler.h.
>
> The lack of this header is causing the configure test to fail.  I've 
> attached the patch here for you to use and I'll send it out again once 
> I figure out the fix for this linux/compiler.h.
>

The patch suffers from the same problem as the apic split; the 
save/restore code is needlessly duplicated.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

Re: [PATCH][QEMU] Use a separate device for in-kernel PIT

[Avi Kivity]

Anthony Liguori wrote:
> Hollis Blanchard wrote:
>> This patch solves annoying qemu build breakage hitting PowerPC around
>> struct kvm_pit_state, so that's another vote in favor...
>>   
>
> I have an updated version of the patch but it's breaking the build b/c 
> something fouled up right now with configure.  libkvm pulls in 
> linux/kvm.h which wants to pull in linux/compiler.h.  We don't ship a 
> linux/compiler.h though so it's pulling from /usr/include/linux which 
> on my system doesn't have a compiler.h.
>
> The lack of this header is causing the configure test to fail.  I've 
> attached the patch here for you to use and I'll send it out again once 
> I figure out the fix for this linux/compiler.h.
>

The patch suffers from the same problem as the apic split; the 
save/restore code is needlessly duplicated.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [kvm-ppc-devel] [kvm-devel] [PATCH][QEMU] Use a separate device

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>   
>> Hollis Blanchard wrote:
>>     
>>> This patch solves annoying qemu build breakage hitting PowerPC around
>>> struct kvm_pit_state, so that's another vote in favor...
>>>   
>>>       
>> I have an updated version of the patch but it's breaking the build b/c 
>> something fouled up right now with configure.  libkvm pulls in 
>> linux/kvm.h which wants to pull in linux/compiler.h.  We don't ship a 
>> linux/compiler.h though so it's pulling from /usr/include/linux which 
>> on my system doesn't have a compiler.h.
>>
>> The lack of this header is causing the configure test to fail.  I've 
>> attached the patch here for you to use and I'll send it out again once 
>> I figure out the fix for this linux/compiler.h.
>>
>>     
>
> The patch suffers from the same problem as the apic split; the 
> save/restore code is needlessly duplicated.
>   

The updated patch addresses this problem.  I have to fix the 
linux/compiler.h issue first though before it can be applied or it will 
break the build.

Regards,

Anthony Liguori



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

Re: [PATCH][QEMU] Use a separate device for in-kernel PIT

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>   
>> Hollis Blanchard wrote:
>>     
>>> This patch solves annoying qemu build breakage hitting PowerPC around
>>> struct kvm_pit_state, so that's another vote in favor...
>>>   
>>>       
>> I have an updated version of the patch but it's breaking the build b/c 
>> something fouled up right now with configure.  libkvm pulls in 
>> linux/kvm.h which wants to pull in linux/compiler.h.  We don't ship a 
>> linux/compiler.h though so it's pulling from /usr/include/linux which 
>> on my system doesn't have a compiler.h.
>>
>> The lack of this header is causing the configure test to fail.  I've 
>> attached the patch here for you to use and I'll send it out again once 
>> I figure out the fix for this linux/compiler.h.
>>
>>     
>
> The patch suffers from the same problem as the apic split; the 
> save/restore code is needlessly duplicated.
>   

The updated patch addresses this problem.  I have to fix the 
linux/compiler.h issue first though before it can be applied or it will 
break the build.

Regards,

Anthony Liguori



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/