[Qemu-devel] [PATCH 0/4] prevent Qemu from waking up needlessly

[Qemu-devel] [PATCH 0/4] prevent Qemu from waking up needlessly

[Stefano Stabellini]

Hi all,
this small patch series prevents Qemu from waking up needlessly on Xen
several times a second in order to check some timers.

The first two patches stop Qemu from emulating the RTC and the PIT on
Xen, that are both already emulated in the hypervisor and consume
precious cpu cycles because they need qemu-timers to work.

The third patch makes use of a new mechanism to receive buffered io
event notifications from Xen, so that Qemu doesn't need to check the
buffered io page for data 10 times a sec for the entire life of the VM.

Finally the last patch increases the default select timeout to 1h:
nothing should rely on the select timeout to be 1sec, so we might as
well increase it to 1h.


Stefano Stabellini (4):
      xen: introduce mc146818rtcxen
      xen: do not initialize the interval timer emulator
      xen: introduce an event channel for buffered io event notifications
      qemu_calculate_timeout: increase minimum timeout to 1h

 hw/mc146818rtc.c |   36 +++++++++++++++++++++++++++++++++++-
 hw/pc.c          |    7 +++++--
 qemu-timer.c     |    2 +-
 xen-all.c        |   38 ++++++++++++++++++++++++++++++++------
 4 files changed, 73 insertions(+), 10 deletions(-)


A git tree based on v1.0-rc2 is available here:

git://xenbits.xen.org/people/sstabellini/qemu-dm.git timers-1.0-rc2

Cheers,

Stefano

[Qemu-devel] [PATCH 1/4] xen: introduce mc146818rtcxen

[stefano.stabellini]

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Xen doesn't need full RTC emulation in Qemu because the RTC is already
emulated by the hypervisor. In particular we want to avoid the timers
initialization so that Qemu doesn't need to wake up needlessly.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/mc146818rtc.c |   36 +++++++++++++++++++++++++++++++++++-
 1 files changed, 35 insertions(+), 1 deletions(-)

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 2aaca2f..91242d0 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -28,6 +28,7 @@
 #include "apic.h"
 #include "isa.h"
 #include "mc146818rtc.h"
+#include "arch_init.h"
 
 //#define DEBUG_CMOS
 //#define DEBUG_COALESCED
@@ -614,6 +615,17 @@ static const MemoryRegionOps cmos_ops = {
     .old_portio = cmos_portio
 };
 
+static int rtcxen_initfn(ISADevice *dev)
+{
+    int base = 0x70;
+    RTCState *s = DO_UPCAST(RTCState, dev, dev);
+
+    memory_region_init_io(&s->io, &cmos_ops, s, "rtc", 2);
+    isa_register_ioport(dev, &s->io, base);
+
+    return 0;
+}
+
 static int rtc_initfn(ISADevice *dev)
 {
     RTCState *s = DO_UPCAST(RTCState, dev, dev);
@@ -655,7 +667,11 @@ ISADevice *rtc_init(int base_year, qemu_irq intercept_irq)
     ISADevice *dev;
     RTCState *s;
 
-    dev = isa_create("mc146818rtc");
+    if (xen_available()) {
+        dev = isa_create("mc146818rtcxen");
+    } else {
+        dev = isa_create("mc146818rtc");
+    }
     s = DO_UPCAST(RTCState, dev, dev);
     qdev_prop_set_int32(&dev->qdev, "base_year", base_year);
     qdev_init_nofail(&dev->qdev);
@@ -684,3 +700,21 @@ static void mc146818rtc_register(void)
     isa_qdev_register(&mc146818rtc_info);
 }
 device_init(mc146818rtc_register)
+
+static ISADeviceInfo mc146818rtcxen_info = {
+    .qdev.name     = "mc146818rtcxen",
+    .qdev.size     = sizeof(RTCState),
+    .qdev.no_user  = 1,
+    .qdev.vmsd     = &vmstate_rtc,
+    .init          = rtcxen_initfn,
+    .qdev.props    = (Property[]) {
+        DEFINE_PROP_INT32("base_year", RTCState, base_year, 1980),
+        DEFINE_PROP_END_OF_LIST(),
+    }
+};
+
+static void mc146818rtcxen_register(void)
+{
+    isa_qdev_register(&mc146818rtcxen_info);
+}
+device_init(mc146818rtcxen_register)
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/4] xen: do not initialize the interval timer emulator

[stefano.stabellini]

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

PIT is emulated by the hypervisor so we don't need to emulate it in Qemu:
this patch prevents Qemu from waking up needlessly at PIT_FREQ on Xen.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/pc.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 33778fe..a0ae981 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -43,6 +43,7 @@
 #include "ui/qemu-spice.h"
 #include "memory.h"
 #include "exec-memory.h"
+#include "arch_init.h"
 
 /* output Bochs bios info messages */
 //#define DEBUG_BIOS
@@ -1121,7 +1122,7 @@ void pc_basic_device_init(qemu_irq *gsi,
     DriveInfo *fd[MAX_FD];
     qemu_irq rtc_irq = NULL;
     qemu_irq *a20_line;
-    ISADevice *i8042, *port92, *vmmouse, *pit;
+    ISADevice *i8042, *port92, *vmmouse, *pit = NULL;
     qemu_irq *cpu_exit_irq;
 
     register_ioport_write(0x80, 1, 1, ioport80_write, NULL);
@@ -1142,7 +1143,9 @@ void pc_basic_device_init(qemu_irq *gsi,
 
     qemu_register_boot_set(pc_boot_set, *rtc_state);
 
-    pit = pit_init(0x40, 0);
+    if (!xen_available()) {
+        pit = pit_init(0x40, 0);
+    }
     pcspk_init(pit);
 
     for(i = 0; i < MAX_SERIAL_PORTS; i++) {
-- 
1.7.2.3

[Qemu-devel] [PATCH 3/4] xen: introduce an event channel for buffered io event notifications

[stefano.stabellini]

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Use the newly introduced HVM_PARAM_BUFIOREQ_EVTCHN to receive
notifications for buffered io events.
After the first notification is received leave the event channel masked
and setup a timer to process the rest of the batch.
Once we have completed processing the batch, unmask the event channel
and delete the timer.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen-all.c |   38 ++++++++++++++++++++++++++++++++------
 1 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/xen-all.c b/xen-all.c
index b5e28ab..b28d7e7 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -70,6 +70,8 @@ typedef struct XenIOState {
     QEMUTimer *buffered_io_timer;
     /* the evtchn port for polling the notification, */
     evtchn_port_t *ioreq_local_port;
+    /* evtchn local port for buffered io */
+    evtchn_port_t bufioreq_local_port;
     /* the evtchn fd for polling */
     XenEvtchn xce_handle;
     /* which vcpu we are serving */
@@ -516,6 +518,12 @@ static ioreq_t *cpu_get_ioreq(XenIOState *state)
     evtchn_port_t port;
 
     port = xc_evtchn_pending(state->xce_handle);
+    if (port == state->bufioreq_local_port) {
+        qemu_mod_timer(state->buffered_io_timer,
+                BUFFER_IO_MAX_DELAY + qemu_get_clock_ms(rt_clock));
+        return NULL;
+    }
+
     if (port != -1) {
         for (i = 0; i < smp_cpus; i++) {
             if (state->ioreq_local_port[i] == port) {
@@ -664,16 +672,18 @@ static void handle_ioreq(ioreq_t *req)
     }
 }
 
-static void handle_buffered_iopage(XenIOState *state)
+static int handle_buffered_iopage(XenIOState *state)
 {
     buf_ioreq_t *buf_req = NULL;
     ioreq_t req;
     int qw;
 
     if (!state->buffered_io_page) {
-        return;
+        return 0;
     }
 
+    memset(&req, 0x00, sizeof(req));
+
     while (state->buffered_io_page->read_pointer != state->buffered_io_page->write_pointer) {
         buf_req = &state->buffered_io_page->buf_ioreq[
             state->buffered_io_page->read_pointer % IOREQ_BUFFER_SLOT_NUM];
@@ -698,15 +708,21 @@ static void handle_buffered_iopage(XenIOState *state)
         xen_mb();
         state->buffered_io_page->read_pointer += qw ? 2 : 1;
     }
+
+    return req.count;
 }
 
 static void handle_buffered_io(void *opaque)
 {
     XenIOState *state = opaque;
 
-    handle_buffered_iopage(state);
-    qemu_mod_timer(state->buffered_io_timer,
-                   BUFFER_IO_MAX_DELAY + qemu_get_clock_ms(rt_clock));
+    if (handle_buffered_iopage(state)) {
+        qemu_mod_timer(state->buffered_io_timer,
+                BUFFER_IO_MAX_DELAY + qemu_get_clock_ms(rt_clock));
+    } else {
+        qemu_del_timer(state->buffered_io_timer);
+        xc_evtchn_unmask(state->xce_handle, state->bufioreq_local_port);
+    }
 }
 
 static void cpu_handle_ioreq(void *opaque)
@@ -836,7 +852,6 @@ static void xen_main_loop_prepare(XenIOState *state)
 
     state->buffered_io_timer = qemu_new_timer_ms(rt_clock, handle_buffered_io,
                                                  state);
-    qemu_mod_timer(state->buffered_io_timer, qemu_get_clock_ms(rt_clock));
 
     if (evtchn_fd != -1) {
         qemu_set_fd_handler(evtchn_fd, cpu_handle_ioreq, NULL, state);
@@ -888,6 +903,7 @@ int xen_hvm_init(void)
 {
     int i, rc;
     unsigned long ioreq_pfn;
+    unsigned long bufioreq_evtchn;
     XenIOState *state;
 
     state = g_malloc0(sizeof (XenIOState));
@@ -937,6 +953,16 @@ int xen_hvm_init(void)
         state->ioreq_local_port[i] = rc;
     }
 
+    xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_BUFIOREQ_EVTCHN,
+            &bufioreq_evtchn);
+    rc = xc_evtchn_bind_interdomain(state->xce_handle, xen_domid,
+            (uint32_t)bufioreq_evtchn);
+    if (rc == -1) {
+        fprintf(stderr, "bind interdomain ioctl error %d\n", errno);
+        return -1;
+    }
+    state->bufioreq_local_port = rc;
+
     /* Init RAM management */
     xen_map_cache_init();
     xen_ram_init(ram_size);
-- 
1.7.2.3

[Qemu-devel] [PATCH 4/4] qemu_calculate_timeout: increase minimum timeout to 1h

[stefano.stabellini]

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

There is no reason why the minimum timeout should be 1sec, it could
easily be 1h and we would safe lots of cpu cycles.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 qemu-timer.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index cd026c6..3a9987e 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -846,6 +846,6 @@ fail:
 
 int qemu_calculate_timeout(void)
 {
-    return 1000;
+    return 1000*60*60;
 }
 
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 1/4] xen: introduce mc146818rtcxen

[Anthony Liguori]

On 11/15/2011 08:51 AM, stefano.stabellini@eu.citrix.com wrote:
> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>
> Xen doesn't need full RTC emulation in Qemu because the RTC is already
> emulated by the hypervisor. In particular we want to avoid the timers
> initialization so that Qemu doesn't need to wake up needlessly.
>
> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>

Yuck.  There's got to be a better way to do this.

I think it would be better to name timers and then in Xen specific machine code, 
disable the RTC timers.

Regards,

Anthony Liguori

> ---
>   hw/mc146818rtc.c |   36 +++++++++++++++++++++++++++++++++++-
>   1 files changed, 35 insertions(+), 1 deletions(-)
>
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index 2aaca2f..91242d0 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -28,6 +28,7 @@
>   #include "apic.h"
>   #include "isa.h"
>   #include "mc146818rtc.h"
> +#include "arch_init.h"
>
>   //#define DEBUG_CMOS
>   //#define DEBUG_COALESCED
> @@ -614,6 +615,17 @@ static const MemoryRegionOps cmos_ops = {
>       .old_portio = cmos_portio
>   };
>
> +static int rtcxen_initfn(ISADevice *dev)
> +{
> +    int base = 0x70;
> +    RTCState *s = DO_UPCAST(RTCState, dev, dev);
> +
> +    memory_region_init_io(&s->io,&cmos_ops, s, "rtc", 2);
> +    isa_register_ioport(dev,&s->io, base);
> +
> +    return 0;
> +}
> +
>   static int rtc_initfn(ISADevice *dev)
>   {
>       RTCState *s = DO_UPCAST(RTCState, dev, dev);
> @@ -655,7 +667,11 @@ ISADevice *rtc_init(int base_year, qemu_irq intercept_irq)
>       ISADevice *dev;
>       RTCState *s;
>
> -    dev = isa_create("mc146818rtc");
> +    if (xen_available()) {
> +        dev = isa_create("mc146818rtcxen");
> +    } else {
> +        dev = isa_create("mc146818rtc");
> +    }
>       s = DO_UPCAST(RTCState, dev, dev);
>       qdev_prop_set_int32(&dev->qdev, "base_year", base_year);
>       qdev_init_nofail(&dev->qdev);
> @@ -684,3 +700,21 @@ static void mc146818rtc_register(void)
>       isa_qdev_register(&mc146818rtc_info);
>   }
>   device_init(mc146818rtc_register)
> +
> +static ISADeviceInfo mc146818rtcxen_info = {
> +    .qdev.name     = "mc146818rtcxen",
> +    .qdev.size     = sizeof(RTCState),
> +    .qdev.no_user  = 1,
> +    .qdev.vmsd     =&vmstate_rtc,
> +    .init          = rtcxen_initfn,
> +    .qdev.props    = (Property[]) {
> +        DEFINE_PROP_INT32("base_year", RTCState, base_year, 1980),
> +        DEFINE_PROP_END_OF_LIST(),
> +    }
> +};
> +
> +static void mc146818rtcxen_register(void)
> +{
> +    isa_qdev_register(&mc146818rtcxen_info);
> +}
> +device_init(mc146818rtcxen_register)

Re: [Qemu-devel] [PATCH 1/4] xen: introduce mc146818rtcxen

[Stefano Stabellini]

On Tue, 15 Nov 2011, Anthony Liguori wrote:
> On 11/15/2011 08:51 AM, stefano.stabellini@eu.citrix.com wrote:
> > From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> >
> > Xen doesn't need full RTC emulation in Qemu because the RTC is already
> > emulated by the hypervisor. In particular we want to avoid the timers
> > initialization so that Qemu doesn't need to wake up needlessly.
> >
> > Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> 
> Yuck.  There's got to be a better way to do this.

Yeah, it is pretty ugly, I was hoping in some good suggestions to
improve this patch :)


> I think it would be better to name timers and then in Xen specific machine code, 
> disable the RTC timers.

Good idea!
I was thinking that I could implement an rtc_stop function in
mc146818rtc.c that stops and frees the timers.

Now the problem is that from xen-all.c I cannot easily find the
ISADevice instance to pass to rtc_stop. Do you think it would be
reasonable to call rtc_stop from pc_basic_device_init, inside the same
if (!xen_available()) introduce by the next patch?

Otherwise I could implement functions to walk the isa bus, similarly to
pci_for_each_device.


This is just an example:

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 2aaca2f..568c540 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -667,6 +667,28 @@ ISADevice *rtc_init(int base_year, qemu_irq intercept_irq)
     return dev;
 }
 
+void rtc_stop(ISADevice *dev)
+{
+    RTCState *s = DO_UPCAST(RTCState, dev, dev);
+
+    qemu_del_timer(s->periodic_timer);
+    qemu_del_timer(s->second_timer);
+    qemu_del_timer(s->second_timer2);
+#ifdef TARGET_I386
+    if (rtc_td_hack) {
+        qemu_del_timer(s->coalesced_timer);
+    }
+#endif
+    qemu_free_timer(s->periodic_timer);
+    qemu_free_timer(s->second_timer);
+    qemu_free_timer(s->second_timer2);
+#ifdef TARGET_I386
+    if (rtc_td_hack) {
+        qemu_free_timer(s->coalesced_timer);
+    }
+#endif
+}
+
 static ISADeviceInfo mc146818rtc_info = {
     .qdev.name     = "mc146818rtc",
     .qdev.size     = sizeof(RTCState),
diff --git a/hw/mc146818rtc.h b/hw/mc146818rtc.h
index 575968c..aa2b8ab 100644
--- a/hw/mc146818rtc.h
+++ b/hw/mc146818rtc.h
@@ -8,5 +8,6 @@
 ISADevice *rtc_init(int base_year, qemu_irq intercept_irq);
 void rtc_set_memory(ISADevice *dev, int addr, int val);
 void rtc_set_date(ISADevice *dev, const struct tm *tm);
+void rtc_stop(ISADevice *dev);
 
 #endif /* !MC146818RTC_H */
diff --git a/hw/pc.c b/hw/pc.c
index a0ae981..d734f75 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1145,6 +1145,8 @@ void pc_basic_device_init(qemu_irq *gsi,
 
     if (!xen_available()) {
         pit = pit_init(0x40, 0);
+    } else {
+        rtc_stop(*rtc_state);
     }
     pcspk_init(pit);
 

Re: [Qemu-devel] [Xen-devel] [PATCH 3/4] xen: introduce an event channel for buffered io event notifications

[Ian Campbell]

On Tue, 2011-11-15 at 14:51 +0000, stefano.stabellini@eu.citrix.com
wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Use the newly introduced HVM_PARAM_BUFIOREQ_EVTCHN to receive
> notifications for buffered io events.
> After the first notification is received leave the event channel masked
> and setup a timer to process the rest of the batch.
> Once we have completed processing the batch, unmask the event channel
> and delete the timer.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen-all.c |   38 ++++++++++++++++++++++++++++++++------
>  1 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/xen-all.c b/xen-all.c
> index b5e28ab..b28d7e7 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -70,6 +70,8 @@ typedef struct XenIOState {
>      QEMUTimer *buffered_io_timer;
>      /* the evtchn port for polling the notification, */
>      evtchn_port_t *ioreq_local_port;
> +    /* evtchn local port for buffered io */
> +    evtchn_port_t bufioreq_local_port;
>      /* the evtchn fd for polling */
>      XenEvtchn xce_handle;
>      /* which vcpu we are serving */
> @@ -516,6 +518,12 @@ static ioreq_t *cpu_get_ioreq(XenIOState *state)
>      evtchn_port_t port;
>  
>      port = xc_evtchn_pending(state->xce_handle);
> +    if (port == state->bufioreq_local_port) {
> +        qemu_mod_timer(state->buffered_io_timer,
> +                BUFFER_IO_MAX_DELAY + qemu_get_clock_ms(rt_clock));
> +        return NULL;
> +    }
> +
>      if (port != -1) {
>          for (i = 0; i < smp_cpus; i++) {
>              if (state->ioreq_local_port[i] == port) {
> @@ -664,16 +672,18 @@ static void handle_ioreq(ioreq_t *req)
>      }
>  }
>  
> -static void handle_buffered_iopage(XenIOState *state)
> +static int handle_buffered_iopage(XenIOState *state)
>  {
>      buf_ioreq_t *buf_req = NULL;
>      ioreq_t req;
>      int qw;
>  
>      if (!state->buffered_io_page) {
> -        return;
> +        return 0;
>      }
>  
> +    memset(&req, 0x00, sizeof(req));
> +
>      while (state->buffered_io_page->read_pointer != state->buffered_io_page->write_pointer) {
>          buf_req = &state->buffered_io_page->buf_ioreq[
>              state->buffered_io_page->read_pointer % IOREQ_BUFFER_SLOT_NUM];
> @@ -698,15 +708,21 @@ static void handle_buffered_iopage(XenIOState *state)
>          xen_mb();
>          state->buffered_io_page->read_pointer += qw ? 2 : 1;
>      }
> +
> +    return req.count;
>  }
>  
>  static void handle_buffered_io(void *opaque)
>  {
>      XenIOState *state = opaque;
>  
> -    handle_buffered_iopage(state);
> -    qemu_mod_timer(state->buffered_io_timer,
> -                   BUFFER_IO_MAX_DELAY + qemu_get_clock_ms(rt_clock));
> +    if (handle_buffered_iopage(state)) {
> +        qemu_mod_timer(state->buffered_io_timer,
> +                BUFFER_IO_MAX_DELAY + qemu_get_clock_ms(rt_clock));
> +    } else {
> +        qemu_del_timer(state->buffered_io_timer);
> +        xc_evtchn_unmask(state->xce_handle, state->bufioreq_local_port);
> +    }
>  }
>  
>  static void cpu_handle_ioreq(void *opaque)
> @@ -836,7 +852,6 @@ static void xen_main_loop_prepare(XenIOState *state)
>  
>      state->buffered_io_timer = qemu_new_timer_ms(rt_clock, handle_buffered_io,
>                                                   state);
> -    qemu_mod_timer(state->buffered_io_timer, qemu_get_clock_ms(rt_clock));
>  
>      if (evtchn_fd != -1) {
>          qemu_set_fd_handler(evtchn_fd, cpu_handle_ioreq, NULL, state);
> @@ -888,6 +903,7 @@ int xen_hvm_init(void)
>  {
>      int i, rc;
>      unsigned long ioreq_pfn;
> +    unsigned long bufioreq_evtchn;
>      XenIOState *state;
>  
>      state = g_malloc0(sizeof (XenIOState));
> @@ -937,6 +953,16 @@ int xen_hvm_init(void)
>          state->ioreq_local_port[i] = rc;
>      }
>  
> +    xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_BUFIOREQ_EVTCHN,
> +            &bufioreq_evtchn);
> +    rc = xc_evtchn_bind_interdomain(state->xce_handle, xen_domid,
> +            (uint32_t)bufioreq_evtchn);
> +    if (rc == -1) {
> +        fprintf(stderr, "bind interdomain ioctl error %d\n", errno);
> +        return -1;
> +    }
> +    state->bufioreq_local_port = rc;

Does this fallback gracefully on hypervisors which don't have this new
hvm param? It doesn't look like it but perhaps I'm missing something.

> +
>      /* Init RAM management */
>      xen_map_cache_init();
>      xen_ram_init(ram_size);

Re: [Qemu-devel] [Xen-devel] [PATCH 3/4] xen: introduce an event channel for buffered io event notifications

[Stefano Stabellini]

On Tue, 15 Nov 2011, Ian Campbell wrote:
> > +    xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_BUFIOREQ_EVTCHN,
> > +            &bufioreq_evtchn);
> > +    rc = xc_evtchn_bind_interdomain(state->xce_handle, xen_domid,
> > +            (uint32_t)bufioreq_evtchn);
> > +    if (rc == -1) {
> > +        fprintf(stderr, "bind interdomain ioctl error %d\n", errno);
> > +        return -1;
> > +    }
> > +    state->bufioreq_local_port = rc;
> 
> Does this fallback gracefully on hypervisors which don't have this new
> hvm param? It doesn't look like it but perhaps I'm missing something.

No, it does not.
However upstream Qemu doesn't work very well with Xen 4.1 anyway, the
first Xen release that is going to support it will be Xen 4.2 that
should have this feature.

Re: [Qemu-devel] [Xen-devel] [PATCH 3/4] xen: introduce an event channel for buffered io event notifications

[Ian Campbell]

On Tue, 2011-11-15 at 17:20 +0000, Stefano Stabellini wrote:
> On Tue, 15 Nov 2011, Ian Campbell wrote:
> > > +    xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_BUFIOREQ_EVTCHN,
> > > +            &bufioreq_evtchn);
> > > +    rc = xc_evtchn_bind_interdomain(state->xce_handle, xen_domid,
> > > +            (uint32_t)bufioreq_evtchn);
> > > +    if (rc == -1) {
> > > +        fprintf(stderr, "bind interdomain ioctl error %d\n", errno);
> > > +        return -1;
> > > +    }
> > > +    state->bufioreq_local_port = rc;
> > 
> > Does this fallback gracefully on hypervisors which don't have this new
> > hvm param? It doesn't look like it but perhaps I'm missing something.
> 
> No, it does not.
> However upstream Qemu doesn't work very well with Xen 4.1 anyway, the
> first Xen release that is going to support it will be Xen 4.2 that
> should have this feature.

In which case I think you need to handle the resultant error from
xc_get_hvm_param() gracefully with a suitable error message which says
something along those lines.

Ian.

Re: [Qemu-devel] [PATCH 1/4] xen: introduce mc146818rtcxen

[Stefano Stabellini]

On Tue, 15 Nov 2011, Stefano Stabellini wrote:
> On Tue, 15 Nov 2011, Anthony Liguori wrote:
> > On 11/15/2011 08:51 AM, stefano.stabellini@eu.citrix.com wrote:
> > > From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> > >
> > > Xen doesn't need full RTC emulation in Qemu because the RTC is already
> > > emulated by the hypervisor. In particular we want to avoid the timers
> > > initialization so that Qemu doesn't need to wake up needlessly.
> > >
> > > Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> > 
> > Yuck.  There's got to be a better way to do this.
> 
> Yeah, it is pretty ugly, I was hoping in some good suggestions to
> improve this patch :)
> 
> 
> > I think it would be better to name timers and then in Xen specific machine code, 
> > disable the RTC timers.
> 
> Good idea!
> I was thinking that I could implement an rtc_stop function in
> mc146818rtc.c that stops and frees the timers.
> 
> Now the problem is that from xen-all.c I cannot easily find the
> ISADevice instance to pass to rtc_stop. Do you think it would be
> reasonable to call rtc_stop from pc_basic_device_init, inside the same
> if (!xen_available()) introduce by the next patch?
> 
> Otherwise I could implement functions to walk the isa bus, similarly to
> pci_for_each_device.
> 

ping?


> This is just an example:
> 
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index 2aaca2f..568c540 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -667,6 +667,28 @@ ISADevice *rtc_init(int base_year, qemu_irq intercept_irq)
>      return dev;
>  }
>  
> +void rtc_stop(ISADevice *dev)
> +{
> +    RTCState *s = DO_UPCAST(RTCState, dev, dev);
> +
> +    qemu_del_timer(s->periodic_timer);
> +    qemu_del_timer(s->second_timer);
> +    qemu_del_timer(s->second_timer2);
> +#ifdef TARGET_I386
> +    if (rtc_td_hack) {
> +        qemu_del_timer(s->coalesced_timer);
> +    }
> +#endif
> +    qemu_free_timer(s->periodic_timer);
> +    qemu_free_timer(s->second_timer);
> +    qemu_free_timer(s->second_timer2);
> +#ifdef TARGET_I386
> +    if (rtc_td_hack) {
> +        qemu_free_timer(s->coalesced_timer);
> +    }
> +#endif
> +}
> +
>  static ISADeviceInfo mc146818rtc_info = {
>      .qdev.name     = "mc146818rtc",
>      .qdev.size     = sizeof(RTCState),
> diff --git a/hw/mc146818rtc.h b/hw/mc146818rtc.h
> index 575968c..aa2b8ab 100644
> --- a/hw/mc146818rtc.h
> +++ b/hw/mc146818rtc.h
> @@ -8,5 +8,6 @@
>  ISADevice *rtc_init(int base_year, qemu_irq intercept_irq);
>  void rtc_set_memory(ISADevice *dev, int addr, int val);
>  void rtc_set_date(ISADevice *dev, const struct tm *tm);
> +void rtc_stop(ISADevice *dev);
>  
>  #endif /* !MC146818RTC_H */
> diff --git a/hw/pc.c b/hw/pc.c
> index a0ae981..d734f75 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -1145,6 +1145,8 @@ void pc_basic_device_init(qemu_irq *gsi,
>  
>      if (!xen_available()) {
>          pit = pit_init(0x40, 0);
> +    } else {
> +        rtc_stop(*rtc_state);
>      }
>      pcspk_init(pit);
>  
> 

Re: [Qemu-devel] [PATCH 1/4] xen: introduce mc146818rtcxen

[Anthony Liguori]

On 11/18/2011 05:46 AM, Stefano Stabellini wrote:
> On Tue, 15 Nov 2011, Stefano Stabellini wrote:
>> On Tue, 15 Nov 2011, Anthony Liguori wrote:
>>> On 11/15/2011 08:51 AM, stefano.stabellini@eu.citrix.com wrote:
>>>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>>>
>>>> Xen doesn't need full RTC emulation in Qemu because the RTC is already
>>>> emulated by the hypervisor. In particular we want to avoid the timers
>>>> initialization so that Qemu doesn't need to wake up needlessly.
>>>>
>>>> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>>
>>> Yuck.  There's got to be a better way to do this.
>>
>> Yeah, it is pretty ugly, I was hoping in some good suggestions to
>> improve this patch :)
>>
>>
>>> I think it would be better to name timers and then in Xen specific machine code,
>>> disable the RTC timers.
>>
>> Good idea!
>> I was thinking that I could implement an rtc_stop function in
>> mc146818rtc.c that stops and frees the timers.

You could also just stop the rtc_clock.  The rtc is the only device that makes 
use of the rtc_clock.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/4] xen: introduce mc146818rtcxen

[Anthony Liguori]

On 11/18/2011 05:46 AM, Stefano Stabellini wrote:
> On Tue, 15 Nov 2011, Stefano Stabellini wrote:
>> On Tue, 15 Nov 2011, Anthony Liguori wrote:
>>> On 11/15/2011 08:51 AM, stefano.stabellini@eu.citrix.com wrote:
>>>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>>>
>>>> Xen doesn't need full RTC emulation in Qemu because the RTC is already
>>>> emulated by the hypervisor. In particular we want to avoid the timers
>>>> initialization so that Qemu doesn't need to wake up needlessly.
>>>>
>>>> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>>
>>> Yuck.  There's got to be a better way to do this.
>>
>> Yeah, it is pretty ugly, I was hoping in some good suggestions to
>> improve this patch :)
>>
>>
>>> I think it would be better to name timers and then in Xen specific machine code,
>>> disable the RTC timers.
>>
>> Good idea!
>> I was thinking that I could implement an rtc_stop function in
>> mc146818rtc.c that stops and frees the timers.
>>
>> Now the problem is that from xen-all.c I cannot easily find the
>> ISADevice instance to pass to rtc_stop. Do you think it would be
>> reasonable to call rtc_stop from pc_basic_device_init, inside the same
>> if (!xen_available()) introduce by the next patch?
>>
>> Otherwise I could implement functions to walk the isa bus, similarly to
>> pci_for_each_device.
>>
>
> ping?

Thinking more about it, I think this entire line of thinking is wrong (including 
mine) :-)

The problem you're trying to solve is that the RTC fires two 1 second timers 
regardless of whether the guest is reading the wall clock time, right?  And 
since wall clock time is never read from the QEMU RTC in Xen, it's a huge waste?

The Right Solution would be to modify the RTC emulation such that it did a 
qemu_get_clock() during read of the CMOS registers in order to ensure the time 
was up to date (instead of using 1 second timers).

Then the timers wouldn't even exist anymore.

Regards,

Anthony Liguori

>
>
>> This is just an example:
>>
>> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
>> index 2aaca2f..568c540 100644
>> --- a/hw/mc146818rtc.c
>> +++ b/hw/mc146818rtc.c
>> @@ -667,6 +667,28 @@ ISADevice *rtc_init(int base_year, qemu_irq intercept_irq)
>>       return dev;
>>   }
>>
>> +void rtc_stop(ISADevice *dev)
>> +{
>> +    RTCState *s = DO_UPCAST(RTCState, dev, dev);
>> +
>> +    qemu_del_timer(s->periodic_timer);
>> +    qemu_del_timer(s->second_timer);
>> +    qemu_del_timer(s->second_timer2);
>> +#ifdef TARGET_I386
>> +    if (rtc_td_hack) {
>> +        qemu_del_timer(s->coalesced_timer);
>> +    }
>> +#endif
>> +    qemu_free_timer(s->periodic_timer);
>> +    qemu_free_timer(s->second_timer);
>> +    qemu_free_timer(s->second_timer2);
>> +#ifdef TARGET_I386
>> +    if (rtc_td_hack) {
>> +        qemu_free_timer(s->coalesced_timer);
>> +    }
>> +#endif
>> +}
>> +
>>   static ISADeviceInfo mc146818rtc_info = {
>>       .qdev.name     = "mc146818rtc",
>>       .qdev.size     = sizeof(RTCState),
>> diff --git a/hw/mc146818rtc.h b/hw/mc146818rtc.h
>> index 575968c..aa2b8ab 100644
>> --- a/hw/mc146818rtc.h
>> +++ b/hw/mc146818rtc.h
>> @@ -8,5 +8,6 @@
>>   ISADevice *rtc_init(int base_year, qemu_irq intercept_irq);
>>   void rtc_set_memory(ISADevice *dev, int addr, int val);
>>   void rtc_set_date(ISADevice *dev, const struct tm *tm);
>> +void rtc_stop(ISADevice *dev);
>>
>>   #endif /* !MC146818RTC_H */
>> diff --git a/hw/pc.c b/hw/pc.c
>> index a0ae981..d734f75 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -1145,6 +1145,8 @@ void pc_basic_device_init(qemu_irq *gsi,
>>
>>       if (!xen_available()) {
>>           pit = pit_init(0x40, 0);
>> +    } else {
>> +        rtc_stop(*rtc_state);
>>       }
>>       pcspk_init(pit);
>>
>>
>

Re: [Qemu-devel] [PATCH 1/4] xen: introduce mc146818rtcxen

[Avi Kivity]

On 11/18/2011 04:54 PM, Anthony Liguori wrote:
>
> Thinking more about it, I think this entire line of thinking is wrong
> (including mine) :-)
>
> The problem you're trying to solve is that the RTC fires two 1 second
> timers regardless of whether the guest is reading the wall clock time,
> right?  And since wall clock time is never read from the QEMU RTC in
> Xen, it's a huge waste?
>
> The Right Solution would be to modify the RTC emulation such that it
> did a qemu_get_clock() during read of the CMOS registers in order to
> ensure the time was up to date (instead of using 1 second timers).
>
> Then the timers wouldn't even exist anymore.

That would make host time adjustments (suspend/resume) be reflected in
the guest.

Not sure if that's good or bad, but it's different.

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

Re: [Qemu-devel] [PATCH 1/4] xen: introduce mc146818rtcxen

[Stefano Stabellini]

On Fri, 18 Nov 2011, Anthony Liguori wrote:
> On 11/18/2011 05:46 AM, Stefano Stabellini wrote:
> > On Tue, 15 Nov 2011, Stefano Stabellini wrote:
> >> On Tue, 15 Nov 2011, Anthony Liguori wrote:
> >>> On 11/15/2011 08:51 AM, stefano.stabellini@eu.citrix.com wrote:
> >>>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> >>>>
> >>>> Xen doesn't need full RTC emulation in Qemu because the RTC is already
> >>>> emulated by the hypervisor. In particular we want to avoid the timers
> >>>> initialization so that Qemu doesn't need to wake up needlessly.
> >>>>
> >>>> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> >>>
> >>> Yuck.  There's got to be a better way to do this.
> >>
> >> Yeah, it is pretty ugly, I was hoping in some good suggestions to
> >> improve this patch :)
> >>
> >>
> >>> I think it would be better to name timers and then in Xen specific machine code,
> >>> disable the RTC timers.
> >>
> >> Good idea!
> >> I was thinking that I could implement an rtc_stop function in
> >> mc146818rtc.c that stops and frees the timers.
> >>
> >> Now the problem is that from xen-all.c I cannot easily find the
> >> ISADevice instance to pass to rtc_stop. Do you think it would be
> >> reasonable to call rtc_stop from pc_basic_device_init, inside the same
> >> if (!xen_available()) introduce by the next patch?
> >>
> >> Otherwise I could implement functions to walk the isa bus, similarly to
> >> pci_for_each_device.
> >>
> >
> > ping?
> 
> Thinking more about it, I think this entire line of thinking is wrong (including 
> mine) :-)

Actually I quite liked your suggestion of stopping the rtc_clock: the
patch becomes a one-liner in xen-all!


> The problem you're trying to solve is that the RTC fires two 1 second timers 
> regardless of whether the guest is reading the wall clock time, right?  And 
> since wall clock time is never read from the QEMU RTC in Xen, it's a huge waste?

The real problem I am trying to solve is that I don't need an RTC clock
in Qemu. However it is not easy to disentangle the RTC emulation from
the rest of the system (see rtc_state in pc.c and pc_piix.c). So I would
be happy enough with just getting rid of the timers.


> The Right Solution would be to modify the RTC emulation such that it did a 
> qemu_get_clock() during read of the CMOS registers in order to ensure the time 
> was up to date (instead of using 1 second timers).
> 
> Then the timers wouldn't even exist anymore.

That would be one way of doing it, however the timers don't only update
a clock variable, so removing them is certainly non-trivial and could
have unintended consequences. I am not sure it is worth it in this
context.
BTW the RTC emulation in Xen also has two timers.

Re: [Qemu-devel] [PATCH 1/4] xen: introduce mc146818rtcxen

[Paolo Bonzini]

On 11/18/2011 03:54 PM, Anthony Liguori wrote:
>
> The Right Solution would be to modify the RTC emulation such that it did
> a qemu_get_clock() during read of the CMOS registers in order to ensure
> the time was up to date (instead of using 1 second timers).

True, but you also have to handle UIP and the UF/AF interrupts. 
Basically you have to track the various pieces of state and trigger a 
timer when the routine would do something interesting.  It's not hard, 
but it's also a decent amount of programming.

Paolo

Re: [Qemu-devel] [PATCH 1/4] xen: introduce mc146818rtcxen

[Anthony Liguori]

On 11/20/2011 08:53 AM, Avi Kivity wrote:
> On 11/18/2011 04:54 PM, Anthony Liguori wrote:
>>
>> Thinking more about it, I think this entire line of thinking is wrong
>> (including mine) :-)
>>
>> The problem you're trying to solve is that the RTC fires two 1 second
>> timers regardless of whether the guest is reading the wall clock time,
>> right?  And since wall clock time is never read from the QEMU RTC in
>> Xen, it's a huge waste?
>>
>> The Right Solution would be to modify the RTC emulation such that it
>> did a qemu_get_clock() during read of the CMOS registers in order to
>> ensure the time was up to date (instead of using 1 second timers).
>>
>> Then the timers wouldn't even exist anymore.
>
> That would make host time adjustments (suspend/resume) be reflected in
> the guest.

qemu_get_clock(rtc_clock)

It depends on what clock rtc_clock is tied too.  If it's tied to vm_clock, it 
won't be affected.

> Not sure if that's good or bad, but it's different.

Doing this wouldn't change the behavior.  I presume you were confusing 
qemu_get_clock() with qemu_get_timedate().

But our current default behavior has the characteristic that you're concerned 
about.  It's was a conscious decision.  See:

commit 6875204c782e7c9aa5c28f96b2583fd31c50468f
Author: Jan Kiszka <jan.kiszka@siemens.com>
Date:   Tue Sep 15 13:36:04 2009 +0200

     Enable host-clock-based RTC

Regards,

Anthony Liguori