uq/master: Introduce KVM PIT support

PATCH In review Jan Kiszka 4 participants 11 messages Feb 5, 2012 – Feb 8, 2012

Subsystems (2)

KVM Kbuild

lore

Share & tools

lore

Jan Kiszka

author

2012-02-05 10:46 UTC #0 lore raw

cover uq/master: Introduce KVM PIT support

This adds another piece of qemu-kvm to upstream: The accelerated in-kernel model of the i8254. It does this in the same fashion as the interrupt controllers were already introduced. And it even has one bug less than qemu-kvm: PC speaker output still works with KVM acceleration enabled.

Changes in v2:
- Rebased over recent upstream master to reflect QOM changes

The patches depend on the follow upstream or uq/master patches:
- pit, hpet, pcspk: fixes & preparation for KVM (v4, 7 patches)
http://thread.gmane.org/gmane.comp.emulators.kvm.devel/86006
- kvm: Implement kvm_irqchip_in_kernel
http://thread.gmane.org/gmane.comp.emulators.qemu/134399
- KVM: Fix breakages of QOM conversion
http://permalink.gmane.org/gmane.comp.emulators.qemu/134844

It is based on upstream commit cb437e48ab (before the VGA breakage). The series plus dependencies is also available at

git://git.kiszka.org/qemu-kvm.git queues/kvm-pit

Please review / merge.

Jan Kiszka (4):
i8254: Factor out base class for KVM reuse
i8254: Open-code timer restore
kvm: Add kvm_has_pit_state2 helper
kvm: x86: Add user space part for in-kernel i8254

 Makefile.objs       |    2 +-
 Makefile.target     |    2 +-
 hw/i8254.c          |  281 +++++++---------------------------------------
 hw/i8254.h          |   11 ++
 hw/i8254_common.c   |  307 +++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/i8254_internal.h |   85 ++++++++++++++
 hw/kvm/i8254.c      |  241 ++++++++++++++++++++++++++++++++++++++++
 hw/pc.c             |   14 ++-
 kvm-all.c           |   10 ++
 kvm-stub.c          |    5 +
 kvm.h               |    1 +
 11 files changed, 717 insertions(+), 242 deletions(-)
 create mode 100644 hw/i8254_common.c
 create mode 100644 hw/i8254_internal.h
 create mode 100644 hw/kvm/i8254.c

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2012-02-05 10:46 UTC #1 lore raw

PATCH 1/4 i8254: Factor out base class for KVM reuse

From: Jan Kiszka <[email protected]>

Applying the concept used for the *PICs once again: establish a base class for the i8254 that can be used both by the current user space emulation and the upcoming KVM in-kernel version. We share most of the public interface of the i8254, specifically to the pcspk, vmstate, reset and certain init parts.

Signed-off-by Jan Kiszka <[email protected]>

---
 Makefile.objs       |    2 +-
 hw/i8254.c          |  269 +++++---------------------------------------
 hw/i8254_common.c   |  307 +++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/i8254_internal.h |   85 ++++++++++++++
 4 files changed, 424 insertions(+), 239 deletions(-)
 create mode 100644 hw/i8254_common.c
 create mode 100644 hw/i8254_internal.h

Makefile.objs +1 -1

index ec35320..15c3915 100644
@@ -210,7 +210,7 @@ hw-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
 
 hw-obj-$(CONFIG_SERIAL) += serial.o
 hw-obj-$(CONFIG_PARALLEL) += parallel.o
-hw-obj-$(CONFIG_I8254) += i8254.o
+hw-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
 hw-obj-$(CONFIG_PCSPK) += pcspk.o
 hw-obj-$(CONFIG_PCKBD) += pckbd.o
 hw-obj-$(CONFIG_USB_UHCI) += usb-uhci.o

hw/i8254.c +31 ~~-238~~

365 lines not on this page · raw message

hw/i8254_common.c new file +307 -0

307 lines not on this page · raw message

hw/i8254_internal.h new file +85 -0

85 lines not on this page · raw message

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2012-02-05 10:47 UTC #2 lore raw

PATCH 2/4 i8254: Open-code timer restore

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Same as for the APIC: To enable migration between accelerated and non-accelerated models, we need to arm the channel 0 timer only inside the emulated PIT model. The common code just saves/restores that timer to the the next_transition_time field.

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/i8254.c        |   12 ++++++++++++
 hw/i8254_common.c |    6 +++---
 2 files changed, 15 insertions(+), 3 deletions(-)

hw/i8254.c +12 -0

index f9f58e0..d41ee05 100644
@@ -300,6 +300,17 @@ static const MemoryRegionOps pit_ioport_ops = {
     .old_portio = pit_portio
 };
 
+static void pit_post_load(PITCommonState *s)
+{
+    PITChannelState *sc = &s->channels[0];
+
+    if (sc->next_transition_time != -1) {
+        qemu_mod_timer(sc->irq_timer, sc->next_transition_time);
+    } else {
+        qemu_del_timer(sc->irq_timer);
+    }
+}
+
 static int pit_initfn(PITCommonState *pit)
 {
     PITChannelState *s;

@@ -329,6 +340,7 @@ static void pit_class_initfn(ObjectClass *klass, void *data)
     k->init = pit_initfn;
     k->set_channel_gate = pit_set_channel_gate;
     k->get_channel_info = pit_get_channel_info_common;
+    k->post_load = pit_post_load;
     dc->reset = pit_reset;
     dc->props = pit_properties;
 }

hw/i8254_common.c +3 -3

index 0601d88..b60fbda 100644
@@ -234,9 +234,8 @@ static int pit_load_old(QEMUFile *f, void *opaque, int version_id)
         qemu_get_8s(f, &s->gate);
         s->count_load_time = qemu_get_be64(f);
         s->irq_disabled = 0;
-        if (s->irq_timer) {
+        if (i == 0) {
             s->next_transition_time = qemu_get_be64(f);
-            qemu_get_timer(f, s->irq_timer);
         }
     }
     return 0;

@@ -275,7 +274,8 @@ static const VMStateDescription vmstate_pit_common = {
         VMSTATE_UINT32_V(channels[0].irq_disabled, PITCommonState, 3),
         VMSTATE_STRUCT_ARRAY(channels, PITCommonState, 3, 2,
                              vmstate_pit_channel, PITChannelState),
-        VMSTATE_TIMER(channels[0].irq_timer, PITCommonState),
+        VMSTATE_INT64(channels[0].next_transition_time,
+                      PITCommonState), /* formerly irq_timer */
         VMSTATE_END_OF_LIST()
     }
 };

Signature · 1 line

1.7.3.4

Paolo Bonzini

maintainer

2012-02-05 11:24 UTC #3 lore raw

Re: [PATCH 2/4] i8254: Open-code timer restore

Quoted 5 lines · Jan Kiszka

hw/i8254_common.c +0 -0
index 0601d88..b60fbda 100644

hw/i8254_common.c static int pit_load_old(QEMUFile *f, void *opaque, int version_id) · line 241

Quoted 10 lines

hw/i8254_common.c +1 -2

@@ -234,9 +234,8 @@ static int pit_load_old(QEMUFile *f, void *opaque, int version_id)
          qemu_get_8s(f,&s->gate);
          s->count_load_time = qemu_get_be64(f);
          s->irq_disabled = 0;
-        if (s->irq_timer) {
+        if (i == 0) {
              s->next_transition_time = qemu_get_be64(f);
-            qemu_get_timer(f, s->irq_timer);
          }
      }
      return 0;

You need to invoke the post load callback manually in the load_old callback; see vmstate_load_state:

    if  (version_id < vmsd->minimum_version_id) {
        return vmsd->load_state_old(f, opaque, version_id);
    }

I noticed that in apic_common's apic_load_old you don't have the bug, but on the other hand you're unconditionally loading into s->timer, so "old" migration to a destination with in-kernel APIC doesn't work:

    if (version_id >= 2) {
        qemu_get_timer(f, s->timer);
    }

Paolo

Jan Kiszka

author

2012-02-05 11:33 UTC #4 lore raw

Re: [PATCH 2/4] i8254: Open-code timer restore

Quoted 25 lines · Paolo Bonzini

Quoted 15 lines · Jan Kiszka

hw/i8254_common.c +1 -2

index 0601d88..b60fbda 100644
@@ -234,9 +234,8 @@ static int pit_load_old(QEMUFile *f, void *opaque,
 int version_id)
          qemu_get_8s(f,&s->gate);
          s->count_load_time = qemu_get_be64(f);
          s->irq_disabled = 0;
-        if (s->irq_timer) {
+        if (i == 0) {
              s->next_transition_time = qemu_get_be64(f);
-            qemu_get_timer(f, s->irq_timer);
          }
      }

Quoted 1 line

      return 0;

You need to invoke the post load callback manually in the load_old callback; see vmstate_load_state:

    if  (version_id < vmsd->minimum_version_id) {
        return vmsd->load_state_old(f, opaque, version_id);
    }

Quoted 8 lines

I noticed that in apic_common's apic_load_old you don't have the bug, but on the other hand you're unconditionally loading into s->timer, so "old" migration to a destination with in-kernel APIC doesn't work:
    if (version_id >= 2) {
        qemu_get_timer(f, s->timer);
    }

Hmm, true. The whole load_old is broken, in both APIC and PIT. Need to call the post_load callbacks from there as well. Will fix, thanks.

Jan

Jan Kiszka

author

2012-02-05 12:03 UTC #5 lore raw

PATCH v3 2/4 i8254: Open-code timer restore

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Signed-off-by Jan Kiszka <[email protected]>

---

Changes in v3:
- invoke post_load callback from load_old (as noted by Paolo)

 hw/i8254.c        |   12 ++++++++++++
 hw/i8254_common.c |   10 +++++++---
 2 files changed, 19 insertions(+), 3 deletions(-)

hw/i8254.c +12 -0

index f9f58e0..d41ee05 100644
@@ -300,6 +300,17 @@ static const MemoryRegionOps pit_ioport_ops = {
     .old_portio = pit_portio
 };
 
+static void pit_post_load(PITCommonState *s)
+{
+    PITChannelState *sc = &s->channels[0];
+
+    if (sc->next_transition_time != -1) {
+        qemu_mod_timer(sc->irq_timer, sc->next_transition_time);
+    } else {
+        qemu_del_timer(sc->irq_timer);
+    }
+}
+
 static int pit_initfn(PITCommonState *pit)
 {
     PITChannelState *s;

@@ -329,6 +340,7 @@ static void pit_class_initfn(ObjectClass *klass, void *data)
     k->init = pit_initfn;
     k->set_channel_gate = pit_set_channel_gate;
     k->get_channel_info = pit_get_channel_info_common;
+    k->post_load = pit_post_load;
     dc->reset = pit_reset;
     dc->props = pit_properties;
 }

hw/i8254_common.c +7 -3

index 0601d88..2517db6 100644
@@ -211,6 +211,7 @@ static const VMStateDescription vmstate_pit_channel = {
 static int pit_load_old(QEMUFile *f, void *opaque, int version_id)
 {
     PITCommonState *pit = opaque;
+    PITCommonClass *c = PIT_COMMON_GET_CLASS(pit);
     PITChannelState *s;
     int i;

@@ -234,11 +235,13 @@ static int pit_load_old(QEMUFile *f, void *opaque, int version_id)
         qemu_get_8s(f, &s->gate);
         s->count_load_time = qemu_get_be64(f);
         s->irq_disabled = 0;
-        if (s->irq_timer) {
+        if (i == 0) {
             s->next_transition_time = qemu_get_be64(f);
-            qemu_get_timer(f, s->irq_timer);
         }
     }
+    if (c->post_load) {
+        c->post_load(pit);
+    }
     return 0;
 }

@@ -275,7 +278,8 @@ static const VMStateDescription vmstate_pit_common = {
         VMSTATE_UINT32_V(channels[0].irq_disabled, PITCommonState, 3),
         VMSTATE_STRUCT_ARRAY(channels, PITCommonState, 3, 2,
                              vmstate_pit_channel, PITChannelState),
-        VMSTATE_TIMER(channels[0].irq_timer, PITCommonState),
+        VMSTATE_INT64(channels[0].next_transition_time,
+                      PITCommonState), /* formerly irq_timer */
         VMSTATE_END_OF_LIST()
     }
 };

Jan Kiszka

author

2012-02-05 10:47 UTC #6 lore raw

PATCH 3/4 kvm: Add kvm_has_pit_state2 helper

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

To be used for in-kernel PIT emulation.

Signed-off-by Jan Kiszka <[email protected]>

---
 kvm-all.c  |   10 ++++++++++
 kvm-stub.c |    5 +++++
 kvm.h      |    1 +
 3 files changed, 16 insertions(+), 0 deletions(-)

kvm-all.c +10 -0

index c4babda..bddf922 100644
@@ -75,6 +75,7 @@ struct KVMState
     struct kvm_sw_breakpoint_head kvm_sw_breakpoints;
 #endif
     int pit_in_kernel;
+    int pit_state2;
     int xsave, xcrs;
     int many_ioeventfds;
     int irqchip_inject_ioctl;

@@ -954,6 +955,10 @@ int kvm_init(void)
     s->xcrs = kvm_check_extension(s, KVM_CAP_XCRS);
 #endif
 
+#ifdef KVM_CAP_PIT_STATE2
+    s->pit_state2 = kvm_check_extension(s, KVM_CAP_PIT_STATE2);
+#endif
+
     ret = kvm_arch_init(s);
     if (ret < 0) {
         goto err;

@@ -1291,6 +1296,11 @@ int kvm_has_xcrs(void)
     return kvm_state->xcrs;
 }
 
+int kvm_has_pit_state2(void)
+{
+    return kvm_state->pit_state2;
+}
+
 int kvm_has_many_ioeventfds(void)
 {
     if (!kvm_enabled()) {

kvm-stub.c +5 -0

index f63a0d2..1f1c686 100644
@@ -78,6 +78,11 @@ int kvm_allows_irq0_override(void)
     return 1;
 }
 
+int kvm_has_pit_state2(void)
+{
+    return 0;
+}
+
 void kvm_setup_guest_memory(void *start, size_t size)
 {
 }

kvm.h +1 -0

index f9f1dc8..8ef4476 100644
@@ -54,6 +54,7 @@ int kvm_has_robust_singlestep(void);
 int kvm_has_debugregs(void);
 int kvm_has_xsave(void);
 int kvm_has_xcrs(void);
+int kvm_has_pit_state2(void);
 int kvm_has_many_ioeventfds(void);
 int kvm_has_gsi_routing(void);

Signature · 1 line

1.7.3.4

Juan Quintela

2012-02-05 20:03 UTC #7 lore raw

Re: [PATCH 3/4] kvm: Add kvm_has_pit_state2 helper

Quoted 3 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>
To be used for in-kernel PIT emulation.

....

Quoted 1 line

+ int pit_state2;

This is used as a bool.

kvm-all.c int kvm_init(void) · line 961

Quoted 11 lines

     int xsave, xcrs;
     int many_ioeventfds;
     int irqchip_inject_ioctl;

kvm-all.c +4 -0

@@ -954,6 +955,10 @@ int kvm_init(void)
     s->xcrs = kvm_check_extension(s, KVM_CAP_XCRS);
 #endif
 
+#ifdef KVM_CAP_PIT_STATE2
+    s->pit_state2 = kvm_check_extension(s, KVM_CAP_PIT_STATE2);
+#endif
+

[ this happened to me when I was reviewing this patch, but culprit is not this patch]

really kvm_check_extension() should also return a bool, but that is a bigger change that this patch series tend to introduce.

So, I went to "man ioctl"

Quoted 4 lines

RETURN VALUE
Usually, on success zero is returned. A few ioctl() requests use the return value as an output parameter and return a nonnegative value on
success. On error, -1 is returned, and errno is set appropriately.

Usually is the important word there.

And then went to kvm-all.c

int kvm_check_extension(KVMState *s, unsigned int extension)

{
    int ret;

    ret = kvm_ioctl(s, KVM_CHECK_EXTENSION, extension);
    if (ret < 0) {
        ret = 0;
    }

    return ret;
}

What? it allways return zero? Something should be wrong here. I will expect kvm_check_extension() to work by now.

Yes, kvm_ioctl() return 1 went the extension is there, just for the people confused like me.

Later, Juan.

Jan Kiszka

author

2012-02-05 20:36 UTC #8 lore raw

Re: [PATCH 3/4] kvm: Add kvm_has_pit_state2 helper

Quoted 29 lines · Juan Quintela

Quoted 3 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
To be used for in-kernel PIT emulation.
....
Quoted 1 line
+ int pit_state2;
This is used as a bool.
kvm-all.c int kvm_init(void) · line 961
Quoted 11 lines
     int xsave, xcrs;
     int many_ioeventfds;
     int irqchip_inject_ioctl;
kvm-all.c +4 -0
@@ -954,6 +955,10 @@ int kvm_init(void)
     s->xcrs = kvm_check_extension(s, KVM_CAP_XCRS);
 #endif
 
+#ifdef KVM_CAP_PIT_STATE2
+    s->pit_state2 = kvm_check_extension(s, KVM_CAP_PIT_STATE2);
+#endif
+
[ this happened to me when I was reviewing this patch, but culprit is not this patch]
really kvm_check_extension() should also return a bool, but that is a bigger change that this patch series tend to introduce.

Quoted 8 lines

So, I went to "man ioctl"
Quoted 4 lines
RETURN VALUE
Usually, on success zero is returned. A few ioctl() requests use the return value as an output parameter and return a nonnegative value on
success. On error, -1 is returned, and errno is set appropriately.
Usually is the important word there.

Right, because the driver decides in the end what is returned.

In case of KVM_CHECK_EXTENSION, it is > 0 for supported. Sometimes it returns an integer value that encode "how much" is supported, e.g. KVM_CAP_IRQ_ROUTING provides the number of supported GSI routes.

That said, all those boolean caps could indeed be encoded as such. Some smarter way to initialize and retrieve them would also be nice. Too much boilerplate code.

Thanks for bending your brain around this :)

Jan

Jan Kiszka

author

2012-02-05 10:46 UTC #9 lore raw

PATCH 4/4 kvm: x86: Add user space part for in-kernel i8254

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

This provides the required user space stubs to enable the in-kernel i8254 emulation of KVM.

The in-kernel model supports lost tick compensation according to the "delay" policy. This is enabled by default and can be switched off via a device property.

Depending on the feature set of the host kernel (before 2.6.32), we may have to disable the HPET or lack sound output from the PC speaker.

Signed-off-by Jan Kiszka <[email protected]>

---
 Makefile.target |    2 +-
 hw/i8254.h      |   11 +++
 hw/kvm/i8254.c  |  241 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pc.c         |   14 +++-
 4 files changed, 265 insertions(+), 3 deletions(-)
 create mode 100644 hw/kvm/i8254.c

Makefile.target +1 -1

index 68481a3..2af5511 100644
@@ -235,7 +235,7 @@ obj-i386-y += vmport.o
 obj-i386-y += pci-hotplug.o smbios.o wdt_ib700.o
 obj-i386-y += debugcon.o multiboot.o
 obj-i386-y += pc_piix.o
-obj-i386-$(CONFIG_KVM) += kvm/clock.o kvm/apic.o kvm/i8259.o kvm/ioapic.o
+obj-i386-$(CONFIG_KVM) += kvm/clock.o kvm/apic.o kvm/i8259.o kvm/ioapic.o kvm/i8254.o
 obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
 
 # shared objects

hw/i8254.h +11 -0

index a1d2e98..ba6b598 100644
@@ -51,6 +51,17 @@ static inline ISADevice *pit_init(ISABus *bus, int base, int isa_irq,
     return dev;
 }
 
+static inline ISADevice *kvm_pit_init(ISABus *bus, int base)
+{
+    ISADevice *dev;
+
+    dev = isa_create(bus, "kvm-pit");
+    qdev_prop_set_uint32(&dev->qdev, "iobase", base);
+    qdev_init_nofail(&dev->qdev);
+
+    return dev;
+}
+
 void pit_set_gate(ISADevice *dev, int channel, int val);
 void pit_get_channel_info(ISADevice *dev, int channel, PITChannelInfo *info);

hw/kvm/i8254.c new file +241 -0

241 lines not on this page · raw message

hw/pc.c +12 -2

index e2b6560..94952a4 100644
@@ -1151,7 +1151,13 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
 
     register_ioport_write(0xf0, 1, 1, ioportF0_write, NULL);
 
-    if (!no_hpet) {
+    /*
+     * Check if an HPET shall be created.
+     *
+     * Without KVM_CAP_PIT_STATE2, we cannot switch off the in-kernel PIT
+     * when the HPET wants to take over. Thus we have to disable the latter.
+     */
+    if (!no_hpet && (!kvm_irqchip_in_kernel() || kvm_has_pit_state2())) {
         hpet = sysbus_try_create_simple("hpet", HPET_BASE, NULL);
 
         if (hpet) {

@@ -1167,7 +1173,11 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
 
     qemu_register_boot_set(pc_boot_set, *rtc_state);
 
-    pit = pit_init(isa_bus, 0x40, pit_isa_irq, pit_alt_irq);
+    if (kvm_irqchip_in_kernel()) {
+        pit = kvm_pit_init(isa_bus, 0x40);
+    } else {
+        pit = pit_init(isa_bus, 0x40, pit_isa_irq, pit_alt_irq);
+    }
     if (hpet) {
         /* connect PIT to output control line of the HPET */
         qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(&pit->qdev, 0));

Signature · 1 line

1.7.3.4

Marcelo Tosatti

2012-02-08 18:24 UTC #10 lore raw

In reply to Jan Kiszka

Quoted 17 lines · Jan Kiszka

This adds another piece of qemu-kvm to upstream: The accelerated in-kernel model of the i8254. It does this in the same fashion as the interrupt controllers were already introduced. And it even has one bug less than qemu-kvm: PC speaker output still works with KVM acceleration enabled.
Changes in v2:
- Rebased over recent upstream master to reflect QOM changes
The patches depend on the follow upstream or uq/master patches:
- pit, hpet, pcspk: fixes & preparation for KVM (v4, 7 patches)
http://thread.gmane.org/gmane.comp.emulators.kvm.devel/86006
- kvm: Implement kvm_irqchip_in_kernel
http://thread.gmane.org/gmane.comp.emulators.qemu/134399
- KVM: Fix breakages of QOM conversion
http://permalink.gmane.org/gmane.comp.emulators.qemu/134844

Quoted 6 lines

It is based on upstream commit cb437e48ab (before the VGA breakage). The series plus dependencies is also available at
git://git.kiszka.org/qemu-kvm.git queues/kvm-pit
Please review / merge.

Looks good.