[Qemu-devel] [PATCH 0/6] Make hpet a compile time option

[Qemu-devel] [PATCH 0/6] Make hpet a compile time option

[Juan Quintela]

Hi

This series:
a- bring back the support for config-devices.h

   Paul was the one that removed my previous submission.
   You can see on the last patch why I want config-devices.h

b- move all hpet code to hpet.c/hpet_emul.h

   In the last patch, I add CONFIG_HPET define, and made everything
   depending on that define.  When we have !CONFIG_HPET we just create
   stub functions.

This was my idea to create config-devices.h in the first place.

Paul, if you don't like it, I am open to alternatives.  Problem here
is that there are devices that we don't want to ship in RHEL for one
reason or another.  Solutions so far:

- use Makefile/ifdefs in calling files trickery: this is ugly as hell
  and was refused (I sent patches to do that ~1 year ago).

I was told to use CONFIG_FOO to disable the full compilation and a
config file, like the kernel.

Here we go, then Paul removed the config-device.h part, because:

>   Also remove config-devices.h.  Code does not and should not care which
>   devices are being built.

This is ok, when device is only called through qdev, and then it is trivial
to compile out.

Notice that that there are another cases where we have to do Makefile tricks
just because we don't have config-devices.mak symbols in "C-land".

See for example Makefile.target

CONFIG_NO_KVM = $(if $(subst n,,$(CONFIG_KVM)),n,y)

obj-$(CONFIG_KVM) += kvm.o kvm-all.o
obj-$(CONFIG_NO_KVM) += kvm-stub.o

In this case makes sense to have an stub because there are lots of
functions, but in the hpet case there are only two functions that are
exported.  My problem with the "stub file" way is that we are going to
end with three files by device (foo.c, foo-stub.c, foo.h).  Where
foo-stub.c is basically trivial.

I also removed the "info hpet" command.  I can be convinced that it is better to change its
output to
       HPET is disabled by QEMU
or
       HPET is not present
or any other stirng.

Comments?  Any better suggestion?

Later, Juan.


Juan Quintela (6):
  Create again config-device.h and config.devices.h
  Move no_hpet declaration to hpet_emul.h
  Move no_hpet test to inside hpet_init()
  Make hpet_in_legacy_mode() return 0 for !TARGET_I386
  make hpet_in_legacy_mode() return a bool
  Create CONFIG_HPET

 Makefile                           |    7 +++++--
 Makefile.target                    |    8 ++++++--
 config.h                           |   10 ++++++++++
 default-configs/i386-softmmu.mak   |    1 +
 default-configs/x86_64-softmmu.mak |    1 +
 hw/hpet.c                          |    9 ++++++---
 hw/hpet_emul.h                     |   20 +++++++++++++++++---
 hw/mc146818rtc.c                   |    6 ------
 hw/pc.c                            |    4 +---
 hw/pc.h                            |    3 ---
 monitor.c                          |    5 ++++-
 11 files changed, 51 insertions(+), 23 deletions(-)

[Qemu-devel] [PATCH 1/6] Create again config-device.h and config.devices.h

[Juan Quintela]

This were disabled in (it is reverted by hand because lot of code has
changed since then).

commit a992fe3d0fc185112677286f7a02204d8245b61e
Author: Paul Brook <paul@codesourcery.com>
Date:   Sun Nov 22 16:25:30 2009 +0000

    Makefile dependencies for device configs

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 Makefile        |    7 +++++--
 Makefile.target |    5 ++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 7986bf6..2501154 100644
--- a/Makefile
+++ b/Makefile
@@ -1,6 +1,6 @@
 # Makefile for QEMU.

-GENERATED_HEADERS = config-host.h
+GENERATED_HEADERS = config-host.h config-all-devices.h

 ifneq ($(wildcard config-host.mak),)
 # Put the all: rule here so that config-host.mak can contain dependencies.
@@ -70,6 +70,9 @@ build-all: $(DOCS) $(TOOLS) recurse-all
 config-host.h: config-host.h-timestamp
 config-host.h-timestamp: config-host.mak

+config-all-devices.h: config-all-devices.h-timestamp
+config-all-devices.h-timestamp: config-all-devices.mak
+
 SUBDIR_RULES=$(patsubst %,subdir-%, $(TARGET_DIRS))

 subdir-%: $(GENERATED_HEADERS)
@@ -164,7 +167,7 @@ clean:

 distclean: clean
 	rm -f config-host.mak config-host.h* config-host.ld $(DOCS) qemu-options.texi qemu-img-cmds.texi qemu-monitor.texi
-	rm -f config-all-devices.mak
+	rm -f config-all-devices.mak config-all-devices.h*
 	rm -f roms/seabios/config.mak roms/vgabios/config.mak
 	rm -f qemu-{doc,tech}.{info,aux,cp,dvi,fn,info,ky,log,pdf,pg,toc,tp,vr}
 	for d in $(TARGET_DIRS) libhw32 libhw64 libuser libdis libdis-user; do \
diff --git a/Makefile.target b/Makefile.target
index fda5bf3..3859dd5 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -1,6 +1,6 @@
 # -*- Mode: makefile -*-

-GENERATED_HEADERS = config-target.h
+GENERATED_HEADERS = config-target.h config-devices.h
 CONFIG_NO_KVM = $(if $(subst n,,$(CONFIG_KVM)),n,y)

 include ../config-host.mak
@@ -38,6 +38,9 @@ kvm.o kvm-all.o vhost.o vhost_net.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
 config-target.h: config-target.h-timestamp
 config-target.h-timestamp: config-target.mak

+config-devices.h: config-devices.h-timestamp
+config-devices.h-timestamp: config-devices.mak
+
 all: $(PROGS)

 # Dummy command so that make thinks it has done something
-- 
1.6.6.1

[Qemu-devel] [PATCH 2/6] Move no_hpet declaration to hpet_emul.h

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/hpet_emul.h |    2 ++
 hw/pc.h        |    3 ---
 monitor.c      |    1 +
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
index cfd95b4..7e9b610 100644
--- a/hw/hpet_emul.h
+++ b/hw/hpet_emul.h
@@ -13,6 +13,8 @@
 #ifndef QEMU_HPET_EMUL_H
 #define QEMU_HPET_EMUL_H

+extern int no_hpet;
+
 #define HPET_BASE               0xfed00000
 #define HPET_CLK_PERIOD         10000000ULL /* 10000000 femtoseconds == 10ns*/

diff --git a/hw/pc.h b/hw/pc.h
index 73cccef..5ee0aad 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -124,9 +124,6 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
                        int kvm_enabled);
 void piix4_smbus_register_device(SMBusDevice *dev, uint8_t addr);

-/* hpet.c */
-extern int no_hpet;
-
 /* pcspk.c */
 void pcspk_init(PITState *);
 int pcspk_audio_init(qemu_irq *pic);
diff --git a/monitor.c b/monitor.c
index a1ebc5d..5975f40 100644
--- a/monitor.c
+++ b/monitor.c
@@ -30,6 +30,7 @@
 #include "hw/pci.h"
 #include "hw/watchdog.h"
 #include "hw/loader.h"
+#include "hw/hpet_emul.h"
 #include "gdbstub.h"
 #include "net.h"
 #include "net/slirp.h"
-- 
1.6.6.1

[Qemu-devel] [PATCH 3/6] Move no_hpet test to inside hpet_init()

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/hpet.c |    3 +++
 hw/pc.c   |    4 +---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index 8729fb2..0ef3335 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -570,6 +570,9 @@ void hpet_init(qemu_irq *irq) {

     DPRINTF ("hpet_init\n");

+    if (no_hpet)
+        return;
+
     s = qemu_mallocz(sizeof(HPETState));
     hpet_statep = s;
     s->irqs = irq;
diff --git a/hw/pc.c b/hw/pc.c
index e7f31d3..ccff3ec 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -944,9 +944,7 @@ void pc_basic_device_init(qemu_irq *isa_irq,

     pit = pit_init(0x40, isa_reserve_irq(0));
     pcspk_init(pit);
-    if (!no_hpet) {
-        hpet_init(isa_irq);
-    }
+    hpet_init(isa_irq);

     for(i = 0; i < MAX_SERIAL_PORTS; i++) {
         if (serial_hds[i]) {
-- 
1.6.6.1

[Qemu-devel] [PATCH 4/6] Make hpet_in_legacy_mode() return 0 for !TARGET_I386

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/hpet_emul.h   |   12 ++++++++++--
 hw/mc146818rtc.c |    6 ------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
index 7e9b610..b6fae0a 100644
--- a/hw/hpet_emul.h
+++ b/hw/hpet_emul.h
@@ -15,6 +15,15 @@

 extern int no_hpet;

+#if !defined(TARGET_I386)
+
+static inline uint32_t hpet_in_legacy_mode(void)
+{
+    return 0;
+}
+
+#else
+
 #define HPET_BASE               0xfed00000
 #define HPET_CLK_PERIOD         10000000ULL /* 10000000 femtoseconds == 10ns*/

@@ -76,9 +85,8 @@ typedef struct HPETState {
     uint64_t hpet_counter;      /* main counter */
 } HPETState;

-#if defined TARGET_I386
 extern uint32_t hpet_in_legacy_mode(void);
 extern void hpet_init(qemu_irq *irq);
-#endif
+#endif /* TARGET_I386 */

 #endif
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 571c593..b2af5de 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -101,9 +101,7 @@ static void rtc_irq_raise(qemu_irq irq)
      * mode is established while interrupt is raised. We want it to
      * be lowered in any case
      */
-#if defined TARGET_I386
     if (!hpet_in_legacy_mode())
-#endif
         qemu_irq_raise(irq);
 }

@@ -148,14 +146,10 @@ static void rtc_timer_update(RTCState *s, int64_t current_time)
     int enable_pie;

     period_code = s->cmos_data[RTC_REG_A] & 0x0f;
-#if defined TARGET_I386
     /* disable periodic timer if hpet is in legacy mode, since interrupts are
      * disabled anyway.
      */
     enable_pie = !hpet_in_legacy_mode();
-#else
-    enable_pie = 1;
-#endif
     if (period_code != 0
         && (((s->cmos_data[RTC_REG_B] & REG_B_PIE) && enable_pie)
             || ((s->cmos_data[RTC_REG_B] & REG_B_SQWE) && s->sqw_irq))) {
-- 
1.6.6.1

[Qemu-devel] [PATCH 5/6] make hpet_in_legacy_mode() return a bool

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/hpet.c      |    6 +++---
 hw/hpet_emul.h |    6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index 0ef3335..33d7f5e 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -39,12 +39,12 @@

 static HPETState *hpet_statep;

-uint32_t hpet_in_legacy_mode(void)
+bool hpet_in_legacy_mode(void)
 {
     if (hpet_statep)
-        return hpet_statep->config & HPET_CFG_LEGACY;
+        return (hpet_statep->config & HPET_CFG_LEGACY) != 0;
     else
-        return 0;
+        return false;
 }

 static uint32_t timer_int_route(struct HPETTimer *timer)
diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
index b6fae0a..4f90faa 100644
--- a/hw/hpet_emul.h
+++ b/hw/hpet_emul.h
@@ -17,9 +17,9 @@ extern int no_hpet;

 #if !defined(TARGET_I386)

-static inline uint32_t hpet_in_legacy_mode(void)
+static inline bool hpet_in_legacy_mode(void)
 {
-    return 0;
+    return false;
 }

 #else
@@ -85,7 +85,7 @@ typedef struct HPETState {
     uint64_t hpet_counter;      /* main counter */
 } HPETState;

-extern uint32_t hpet_in_legacy_mode(void);
+extern bool hpet_in_legacy_mode(void);
 extern void hpet_init(qemu_irq *irq);
 #endif /* TARGET_I386 */

-- 
1.6.6.1

[Qemu-devel] [PATCH 6/6] Create CONFIG_HPET

[Juan Quintela]

Compile hpet.o depending of CONFIG_HPET.  Simplify testing for using
this functions once there.  Create inline stabs for the exported functions.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 Makefile.target                    |    3 ++-
 config.h                           |   10 ++++++++++
 default-configs/i386-softmmu.mak   |    1 +
 default-configs/x86_64-softmmu.mak |    1 +
 hw/hpet_emul.h                     |    8 ++++++--
 monitor.c                          |    4 +++-
 6 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/Makefile.target b/Makefile.target
index 3859dd5..533327e 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -194,7 +194,8 @@ obj-y += e1000.o
 obj-i386-y += vga.o
 obj-i386-y += mc146818rtc.o i8259.o pc.o
 obj-i386-y += cirrus_vga.o apic.o ioapic.o piix_pci.o
-obj-i386-y += vmmouse.o vmport.o hpet.o
+obj-i386-y += vmmouse.o vmport.o
+obj-i386-$(CONFIG_HPET) += hpet.o
 obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
 obj-i386-y += debugcon.o multiboot.o
 obj-i386-y += pc_piix.o
diff --git a/config.h b/config.h
index e20f786..7d92cfc 100644
--- a/config.h
+++ b/config.h
@@ -1,2 +1,12 @@
 #include "config-host.h"
 #include "config-target.h"
+
+/* We want to include different config files for specific targets
+   And for the common library.  They need a different name because
+   we don't want to rely in paths */
+
+#if defined(NEED_CPU_H)
+#include "config-devices.h"
+#else
+#include "config-all-devices.h"
+#endif
diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index ed00471..6341ede 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -23,3 +23,4 @@ CONFIG_NE2000_ISA=y
 CONFIG_PIIX_PCI=y
 CONFIG_SOUND=y
 CONFIG_VIRTIO_PCI=y
+CONFIG_HPET=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index 5183203..e1ca587 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -23,3 +23,4 @@ CONFIG_NE2000_ISA=y
 CONFIG_PIIX_PCI=y
 CONFIG_SOUND=y
 CONFIG_VIRTIO_PCI=y
+CONFIG_HPET=y
diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
index 4f90faa..898364b 100644
--- a/hw/hpet_emul.h
+++ b/hw/hpet_emul.h
@@ -15,13 +15,17 @@

 extern int no_hpet;

-#if !defined(TARGET_I386)
+#if !defined(CONFIG_HPET)

 static inline bool hpet_in_legacy_mode(void)
 {
     return false;
 }

+static inline void hpet_init(qemu_irq *irq)
+{
+}
+
 #else

 #define HPET_BASE               0xfed00000
@@ -87,6 +91,6 @@ typedef struct HPETState {

 extern bool hpet_in_legacy_mode(void);
 extern void hpet_init(qemu_irq *irq);
-#endif /* TARGET_I386 */
+#endif /* CONFIG_HPET */

 #endif
diff --git a/monitor.c b/monitor.c
index 5975f40..a81de0c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -778,7 +778,7 @@ static void do_info_commands(Monitor *mon, QObject **ret_data)
     *ret_data = QOBJECT(cmd_list);
 }

-#if defined(TARGET_I386)
+#if defined(CONFIG_HPET)
 static void do_info_hpet_print(Monitor *mon, const QObject *data)
 {
     monitor_printf(mon, "HPET is %s by QEMU\n",
@@ -2610,6 +2610,8 @@ static const mon_cmd_t info_cmds[] = {
         .help       = "show the active virtual memory mappings",
         .mhandler.info = mem_info,
     },
+#endif
+#if defined(CONFIG_HPET)
     {
         .name       = "hpet",
         .args_type  = "",
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Juan Quintela]

Juan Quintela <quintela@redhat.com> wrote:
> Hi

Paul, I intended to CC'd you on this series, just forgot at the last moment.

Sorry, Juan.

> This series:
> a- bring back the support for config-devices.h
>
>    Paul was the one that removed my previous submission.
>    You can see on the last patch why I want config-devices.h
>
> b- move all hpet code to hpet.c/hpet_emul.h
>
>    In the last patch, I add CONFIG_HPET define, and made everything
>    depending on that define.  When we have !CONFIG_HPET we just create
>    stub functions.
>
> This was my idea to create config-devices.h in the first place.
>
> Paul, if you don't like it, I am open to alternatives.  Problem here
> is that there are devices that we don't want to ship in RHEL for one
> reason or another.  Solutions so far:
>
> - use Makefile/ifdefs in calling files trickery: this is ugly as hell
>   and was refused (I sent patches to do that ~1 year ago).
>
> I was told to use CONFIG_FOO to disable the full compilation and a
> config file, like the kernel.
>
> Here we go, then Paul removed the config-device.h part, because:
>
>>   Also remove config-devices.h.  Code does not and should not care which
>>   devices are being built.
>
> This is ok, when device is only called through qdev, and then it is trivial
> to compile out.
>
> Notice that that there are another cases where we have to do Makefile tricks
> just because we don't have config-devices.mak symbols in "C-land".
>
> See for example Makefile.target
>
> CONFIG_NO_KVM = $(if $(subst n,,$(CONFIG_KVM)),n,y)
>
> obj-$(CONFIG_KVM) += kvm.o kvm-all.o
> obj-$(CONFIG_NO_KVM) += kvm-stub.o
>
> In this case makes sense to have an stub because there are lots of
> functions, but in the hpet case there are only two functions that are
> exported.  My problem with the "stub file" way is that we are going to
> end with three files by device (foo.c, foo-stub.c, foo.h).  Where
> foo-stub.c is basically trivial.
>
> I also removed the "info hpet" command.  I can be convinced that it is better to change its
> output to
>        HPET is disabled by QEMU
> or
>        HPET is not present
> or any other stirng.
>
> Comments?  Any better suggestion?
>
> Later, Juan.
>
>
> Juan Quintela (6):
>   Create again config-device.h and config.devices.h
>   Move no_hpet declaration to hpet_emul.h
>   Move no_hpet test to inside hpet_init()
>   Make hpet_in_legacy_mode() return 0 for !TARGET_I386
>   make hpet_in_legacy_mode() return a bool
>   Create CONFIG_HPET
>
>  Makefile                           |    7 +++++--
>  Makefile.target                    |    8 ++++++--
>  config.h                           |   10 ++++++++++
>  default-configs/i386-softmmu.mak   |    1 +
>  default-configs/x86_64-softmmu.mak |    1 +
>  hw/hpet.c                          |    9 ++++++---
>  hw/hpet_emul.h                     |   20 +++++++++++++++++---
>  hw/mc146818rtc.c                   |    6 ------
>  hw/pc.c                            |    4 +---
>  hw/pc.h                            |    3 ---
>  monitor.c                          |    5 ++++-
>  11 files changed, 51 insertions(+), 23 deletions(-)

[Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Jan Kiszka]

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

Juan Quintela wrote:
> Hi
> 
> This series:
> a- bring back the support for config-devices.h
> 
>    Paul was the one that removed my previous submission.
>    You can see on the last patch why I want config-devices.h
> 
> b- move all hpet code to hpet.c/hpet_emul.h
> 
>    In the last patch, I add CONFIG_HPET define, and made everything
>    depending on that define.  When we have !CONFIG_HPET we just create
>    stub functions.
> 
> This was my idea to create config-devices.h in the first place.
> 
> Paul, if you don't like it, I am open to alternatives.  Problem here
> is that there are devices that we don't want to ship in RHEL for one
> reason or another.  Solutions so far:
> 
> - use Makefile/ifdefs in calling files trickery: this is ugly as hell
>   and was refused (I sent patches to do that ~1 year ago).
> 
> I was told to use CONFIG_FOO to disable the full compilation and a
> config file, like the kernel.
> 
> Here we go, then Paul removed the config-device.h part, because:
> 
>>   Also remove config-devices.h.  Code does not and should not care which
>>   devices are being built.
> 
> This is ok, when device is only called through qdev, and then it is trivial
> to compile out.
> 
> Notice that that there are another cases where we have to do Makefile tricks
> just because we don't have config-devices.mak symbols in "C-land".
> 
> See for example Makefile.target
> 
> CONFIG_NO_KVM = $(if $(subst n,,$(CONFIG_KVM)),n,y)
> 
> obj-$(CONFIG_KVM) += kvm.o kvm-all.o
> obj-$(CONFIG_NO_KVM) += kvm-stub.o
> 
> In this case makes sense to have an stub because there are lots of
> functions, but in the hpet case there are only two functions that are
> exported.  My problem with the "stub file" way is that we are going to
> end with three files by device (foo.c, foo-stub.c, foo.h).  Where
> foo-stub.c is basically trivial.
> 
> I also removed the "info hpet" command.  I can be convinced that it is better to change its
> output to
>        HPET is disabled by QEMU
> or
>        HPET is not present
> or any other stirng.
> 
> Comments?  Any better suggestion?

Unless this is deadly urgent, please hold it back until we sorted out
some more fundamental issues with the HPET, specifically ported it to qdev.

But I'm also not convinced about the general approach. Except for RHEL
packagers, no one seems to gain any benefit from having CONFIG_HPET. The
HPET model is still incomplete in has some remaining quicks (hold on for
improvements), but that doesn't qualify it for !CONFIG_HPET,
specifically as it is deeply hooked into every modern PC. If I was
asked, I guess I would nack this switch.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Juan Quintela]

Jan Kiszka <jan.kiszka@web.de> wrote:
> Juan Quintela wrote:

> Unless this is deadly urgent, please hold it back until we sorted out
> some more fundamental issues with the HPET, specifically ported it to qdev.

This series are independent of the qdev change (it almost don't change
hpet code at all).  It is basically independent of almost everything else.

> But I'm also not convinced about the general approach. Except for RHEL
> packagers, no one seems to gain any benefit from having CONFIG_HPET.

This happens to us all the time for lots of devices.  And the big
problem is that there is no sane way to disable them :(

If we can agree in a mechanism to disable them (like this one) or
something similar, we could remove it.

Our biggest problem with shipping a device is that we are going to
support it for 7 years, you can guess why we want to be conservative.

>  The
> HPET model is still incomplete in has some remaining quicks (hold on for
> improvements), but that doesn't qualify it for !CONFIG_HPET,
> specifically as it is deeply hooked into every modern PC. If I was
> asked, I guess I would nack this switch.

Then, what should we do?
We already have to disable hpet for 5.4 (1 year ago).  It was done with
a local hack because it was supposed that for next big release it would
have been fixed.

Here we are, and device is still not fixed, what to do?  Another local
patch?  Just get upstream to integrate a sane way to disable it and let
in enable by default?

Notice that this patch was sent against hpet as one example, if we agree
that this "way" of disabling devices is ok, we could disable more
devices/have more flexibility.  Notice that in general, we (RHEL/KVM)
are interested in a small subset of qemu devices.

Later, Juan.

[Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Jan Kiszka]

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

Juan Quintela wrote:
> Jan Kiszka <jan.kiszka@web.de> wrote:
>> Juan Quintela wrote:
> 
>> Unless this is deadly urgent, please hold it back until we sorted out
>> some more fundamental issues with the HPET, specifically ported it to qdev.
> 
> This series are independent of the qdev change (it almost don't change
> hpet code at all).  It is basically independent of almost everything else.

It causes mechanical breakage to the qdev change (and the one I'm
hacking on ATM).

> 
>> But I'm also not convinced about the general approach. Except for RHEL
>> packagers, no one seems to gain any benefit from having CONFIG_HPET.
> 
> This happens to us all the time for lots of devices.  And the big
> problem is that there is no sane way to disable them :(
> 
> If we can agree in a mechanism to disable them (like this one) or
> something similar, we could remove it.
> 
> Our biggest problem with shipping a device is that we are going to
> support it for 7 years, you can guess why we want to be conservative.

In this particular case, it is a one line patch: "no_hpet = 1;", hardwired.

> 
>>  The
>> HPET model is still incomplete in has some remaining quicks (hold on for
>> improvements), but that doesn't qualify it for !CONFIG_HPET,
>> specifically as it is deeply hooked into every modern PC. If I was
>> asked, I guess I would nack this switch.
> 
> Then, what should we do?

Help fixing it (e.g. testers will soon be welcome).

> We already have to disable hpet for 5.4 (1 year ago).  It was done with
> a local hack because it was supposed that for next big release it would
> have been fixed.

But this remains a RHEL issue. Redhat decided to compile out features
that are unsupported, others seem to handle this differently.

> 
> Here we are, and device is still not fixed, what to do?  Another local
> patch?  Just get upstream to integrate a sane way to disable it and let
> in enable by default?

Let's start with listing your requirements to no longer disable HPET.
That would already help us to asses how long !CONFIG_HPET would actually
be of any use at all. I'm yet optimistic that we can resolve most if not
all remaining concerns for 0.13 - and CONFIG_HPET would at best be 0.13
material anyway.

> 
> Notice that this patch was sent against hpet as one example, if we agree
> that this "way" of disabling devices is ok, we could disable more
> devices/have more flexibility.  Notice that in general, we (RHEL/KVM)
> are interested in a small subset of qemu devices.

At least HPET is IMHO a bad example as it is, just like e.g. the IOAPIC,
an essential part of today's x86 systems.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Paul Brook]

> Notice that this patch was sent against hpet as one example, if we agree
> that this "way" of disabling devices is ok, we could disable more
> devices/have more flexibility.  Notice that in general, we (RHEL/KVM)
> are interested in a small subset of qemu devices.

IMO this patch is a backwards step.  The device models should be cleaned up so 
that you don't need to make a compile time decision.  You'll notice that a 
fair amount of effort has been put into making the device/system code less 
tightly coupled and less machine specific.  All inter-device interaction and 
links should be explicit. Changing from "PC with HPET" and "PC without HPET" 
should not require recompiling anything, and devices shouldn't need to know or 
care which they're part of.

Paul

Re: [Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Anthony Liguori]

On 05/24/2010 11:32 AM, Paul Brook wrote:
>> Notice that this patch was sent against hpet as one example, if we agree
>> that this "way" of disabling devices is ok, we could disable more
>> devices/have more flexibility.  Notice that in general, we (RHEL/KVM)
>> are interested in a small subset of qemu devices.
>>      
> IMO this patch is a backwards step.  The device models should be cleaned up so
> that you don't need to make a compile time decision.

I disagree.  I think the device model should be cleaned up so that no 
CONFIG_HPET is required in code but I think it's still useful to be able 
to exclude device models from the build.  That should just be a matter 
of not building the object though (that's the point of device_init()).

Regards,

Anthony LIguori

>    You'll notice that a
> fair amount of effort has been put into making the device/system code less
> tightly coupled and less machine specific.  All inter-device interaction and
> links should be explicit. Changing from "PC with HPET" and "PC without HPET"
> should not require recompiling anything, and devices shouldn't need to know or
> care which they're part of.
>
> Paul
>
>    

Re: [Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Paul Brook]

> On 05/24/2010 11:32 AM, Paul Brook wrote:
> >> Notice that this patch was sent against hpet as one example, if we agree
> >> that this "way" of disabling devices is ok, we could disable more
> >> devices/have more flexibility.  Notice that in general, we (RHEL/KVM)
> >> are interested in a small subset of qemu devices.
> > 
> > IMO this patch is a backwards step.  The device models should be cleaned
> > up so that you don't need to make a compile time decision.
> 
> I disagree.  I think the device model should be cleaned up so that no
> CONFIG_HPET is required in code but I think it's still useful to be able
> to exclude device models from the build.  That should just be a matter
> of not building the object though (that's the point of device_init()).

I think we're saying the same thing.

We already have a mechanism for avoiding things at build time - specifically 
config-devices.mak. We don't have a nice UI for it, but it's there.
At worst your distro specific patch is a 1-line change to default-
configs/i386-softmmu.mak.

I have no objection to moving hpet.c into Makefile.objs, conditional on 
CONFIG_HPET (like e.g. CONFIG_SERIAL/serial.o).  However a necessary 
prerequisite is that you fix the device model and machine initialisation so 
that it's possible to omit hpet.o without rebuilding anything else.

Paul

Re: [Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Anthony Liguori]

On 05/24/2010 12:11 PM, Paul Brook wrote:
> I think we're saying the same thing.
>
> We already have a mechanism for avoiding things at build time - specifically
> config-devices.mak. We don't have a nice UI for it, but it's there.
> At worst your distro specific patch is a 1-line change to default-
> configs/i386-softmmu.mak.
>
> I have no objection to moving hpet.c into Makefile.objs, conditional on
> CONFIG_HPET (like e.g. CONFIG_SERIAL/serial.o).  However a necessary
> prerequisite is that you fix the device model and machine initialisation so
> that it's possible to omit hpet.o without rebuilding anything else.
>    

Yes, I fully agree.

Regards,

Anthony Liguori

> Paul
>    

[Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Juan Quintela]

Paul Brook <paul@codesourcery.com> wrote:
>> On 05/24/2010 11:32 AM, Paul Brook wrote:
>> >> Notice that this patch was sent against hpet as one example, if we agree
>> >> that this "way" of disabling devices is ok, we could disable more
>> >> devices/have more flexibility.  Notice that in general, we (RHEL/KVM)
>> >> are interested in a small subset of qemu devices.
>> > 
>> > IMO this patch is a backwards step.  The device models should be cleaned
>> > up so that you don't need to make a compile time decision.
>> 
>> I disagree.  I think the device model should be cleaned up so that no
>> CONFIG_HPET is required in code but I think it's still useful to be able
>> to exclude device models from the build.  That should just be a matter
>> of not building the object though (that's the point of device_init()).
>
> I think we're saying the same thing.
>
> We already have a mechanism for avoiding things at build time - specifically 
> config-devices.mak. We don't have a nice UI for it, but it's there.
> At worst your distro specific patch is a 1-line change to default-
> configs/i386-softmmu.mak.
>
> I have no objection to moving hpet.c into Makefile.objs, conditional on 
> CONFIG_HPET (like e.g. CONFIG_SERIAL/serial.o).  However a necessary 
> prerequisite is that you fix the device model and machine initialisation so 
> that it's possible to omit hpet.o without rebuilding anything else.

We have two exported functions:

void hpet_init(qemu_irq *irq);
uint32_t hpet_in_legacy_mode(void);

This is how one is used in mc14818rtc:

#if defined TARGET_I386
    if (!hpet_in_legacy_mode())
#endif

how the other one is used on pc.c

    if (!no_hpet) {
        hpet_init(isa_irq);
    }

I agree that I could probably came with some trick with qdev_create() to
substitute the hpet_init() (my understanding is that jan already have it
or something like that).

But for the other call, what do you propose?

My best try was to hide the availability of hpet inside hpet_emul.h
with:

#ifdef CONFIG_HPET
uint32_t hpet_in_legacy_mode(void);
else
uint32_t hpet_in_legacy_mode(void) { return 0;}
#endif

I can't see any obvious way to change the hpet_in_legacy_mode() that is
cleaner than this.

Thanks, Juan.

> Paul

[Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Anthony Liguori]

On 05/24/2010 12:54 PM, Juan Quintela wrote:
> Paul Brook<paul@codesourcery.com>  wrote:
>    
>>> On 05/24/2010 11:32 AM, Paul Brook wrote:
>>>        
>>>>> Notice that this patch was sent against hpet as one example, if we agree
>>>>> that this "way" of disabling devices is ok, we could disable more
>>>>> devices/have more flexibility.  Notice that in general, we (RHEL/KVM)
>>>>> are interested in a small subset of qemu devices.
>>>>>            
>>>> IMO this patch is a backwards step.  The device models should be cleaned
>>>> up so that you don't need to make a compile time decision.
>>>>          
>>> I disagree.  I think the device model should be cleaned up so that no
>>> CONFIG_HPET is required in code but I think it's still useful to be able
>>> to exclude device models from the build.  That should just be a matter
>>> of not building the object though (that's the point of device_init()).
>>>        
>> I think we're saying the same thing.
>>
>> We already have a mechanism for avoiding things at build time - specifically
>> config-devices.mak. We don't have a nice UI for it, but it's there.
>> At worst your distro specific patch is a 1-line change to default-
>> configs/i386-softmmu.mak.
>>
>> I have no objection to moving hpet.c into Makefile.objs, conditional on
>> CONFIG_HPET (like e.g. CONFIG_SERIAL/serial.o).  However a necessary
>> prerequisite is that you fix the device model and machine initialisation so
>> that it's possible to omit hpet.o without rebuilding anything else.
>>      
> We have two exported functions:
>
> void hpet_init(qemu_irq *irq);
> uint32_t hpet_in_legacy_mode(void);
>
> This is how one is used in mc14818rtc:
>
> #if defined TARGET_I386
>      if (!hpet_in_legacy_mode())
> #endif
>    

In real hardware, and HPET would normally emulate an RTC.  The 
interaction problem here is that we aren't modelling that correctly in 
qemu as we're treating the rtc as a separate device.

What could probably work at a hand wave level, is to make the rtc init 
function take a qemu_irq instead of directly grabbing the isa irq.  When 
an HPET is in use, the rtc no longer is directly initiated but instead 
is indirectly initiated by the HPET passing a special qemu_irq to the 
device that masks the actual interrupt line when legacy mode isn't 
enabled.  When the HPET isn't in use, the rtc would be created with an 
isa allocated qemu_irq.

Regards,

Anthony Liguori

> how the other one is used on pc.c
>
>      if (!no_hpet) {
>          hpet_init(isa_irq);
>      }
>
> I agree that I could probably came with some trick with qdev_create() to
> substitute the hpet_init() (my understanding is that jan already have it
> or something like that).
>
> But for the other call, what do you propose?
>
> My best try was to hide the availability of hpet inside hpet_emul.h
> with:
>
> #ifdef CONFIG_HPET
> uint32_t hpet_in_legacy_mode(void);
> else
> uint32_t hpet_in_legacy_mode(void) { return 0;}
> #endif
>
> I can't see any obvious way to change the hpet_in_legacy_mode() that is
> cleaner than this.
>
> Thanks, Juan.
>
>    
>> Paul
>>      

[Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Juan Quintela]

Jan Kiszka <jan.kiszka@web.de> wrote:

>> This happens to us all the time for lots of devices.  And the big
>> problem is that there is no sane way to disable them :(
>> 
>> If we can agree in a mechanism to disable them (like this one) or
>> something similar, we could remove it.
>> 
>> Our biggest problem with shipping a device is that we are going to
>> support it for 7 years, you can guess why we want to be conservative.
>
> In this particular case, it is a one line patch: "no_hpet = 1;", hardwired.

Yeap, but then I end having lots of things patches in RHEL that are not
upstream, and each new version/rebase I have to redo all of them.

>>>  The
>>> HPET model is still incomplete in has some remaining quicks (hold on for
>>> improvements), but that doesn't qualify it for !CONFIG_HPET,
>>> specifically as it is deeply hooked into every modern PC. If I was
>>> asked, I guess I would nack this switch.
>> 
>> Then, what should we do?
>
> Help fixing it (e.g. testers will soon be welcome).

Sorry, no donut :-)

We try to help/fix everything that we can (we == Red Hat in this case),
but we are not going to ship will all drivers any time soon, so it needs
to be a way to disable them IMHO.  If we need to wait for _all_ devices
to be stable and bug free we could ship for next millenium (take or put
a couple century's).

>> We already have to disable hpet for 5.4 (1 year ago).  It was done with
>> a local hack because it was supposed that for next big release it would
>> have been fixed.
>
> But this remains a RHEL issue. Redhat decided to compile out features
> that are unsupported, others seem to handle this differently.

And then, everybody has a different hack to disable the features that
they don't need.  Instead of doing a local hack, we do a patch that
allows anyone to disable HPET if it sees fit.

>> Here we are, and device is still not fixed, what to do?  Another local
>> patch?  Just get upstream to integrate a sane way to disable it and let
>> in enable by default?
>
> Let's start with listing your requirements to no longer disable HPET.

It is not stable at this point in time :-(  Running with --no-hpet is
better than without it in all our testing.  If we have to ask/modify
everything to use --no-hpet, we can also compile-out it.

> That would already help us to asses how long !CONFIG_HPET would actually
> be of any use at all. I'm yet optimistic that we can resolve most if not
> all remaining concerns for 0.13 - and CONFIG_HPET would at best be 0.13
> material anyway.

At this very point in time:
- it is not stable
- lack irq-reinjection when missing ticks

(I was not the one debugging/testing this so I don't have more details,
but can ask for them).  So, it is not stable enough yet.

>> 
>> Notice that this patch was sent against hpet as one example, if we agree
>> that this "way" of disabling devices is ok, we could disable more
>> devices/have more flexibility.  Notice that in general, we (RHEL/KVM)
>> are interested in a small subset of qemu devices.
>
> At least HPET is IMHO a bad example as it is, just like e.g. the IOAPIC,
> an essential part of today's x86 systems.

Humm, we run normally without hpet enabled and all normal guests work.
And yes, I would also preffer it to work.

Later, Juan.

[Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Jan Kiszka]

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

Juan Quintela wrote:
> Paul Brook <paul@codesourcery.com> wrote:
>>> On 05/24/2010 11:32 AM, Paul Brook wrote:
>>>>> Notice that this patch was sent against hpet as one example, if we agree
>>>>> that this "way" of disabling devices is ok, we could disable more
>>>>> devices/have more flexibility.  Notice that in general, we (RHEL/KVM)
>>>>> are interested in a small subset of qemu devices.
>>>> IMO this patch is a backwards step.  The device models should be cleaned
>>>> up so that you don't need to make a compile time decision.
>>> I disagree.  I think the device model should be cleaned up so that no
>>> CONFIG_HPET is required in code but I think it's still useful to be able
>>> to exclude device models from the build.  That should just be a matter
>>> of not building the object though (that's the point of device_init()).
>> I think we're saying the same thing.
>>
>> We already have a mechanism for avoiding things at build time - specifically 
>> config-devices.mak. We don't have a nice UI for it, but it's there.
>> At worst your distro specific patch is a 1-line change to default-
>> configs/i386-softmmu.mak.
>>
>> I have no objection to moving hpet.c into Makefile.objs, conditional on 
>> CONFIG_HPET (like e.g. CONFIG_SERIAL/serial.o).  However a necessary 
>> prerequisite is that you fix the device model and machine initialisation so 
>> that it's possible to omit hpet.o without rebuilding anything else.
> 
> We have two exported functions:
> 
> void hpet_init(qemu_irq *irq);
> uint32_t hpet_in_legacy_mode(void);

One, hpet_in_legacy_mode will become hpet-private.

Once done, I will have a look if we can cleanly push more x86 platform
logic related to the HPET IRQ routing into hpet_init.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Jan Kiszka]

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

Anthony Liguori wrote:
> On 05/24/2010 12:54 PM, Juan Quintela wrote:
>> Paul Brook<paul@codesourcery.com>  wrote:
>>   
>>>> On 05/24/2010 11:32 AM, Paul Brook wrote:
>>>>       
>>>>>> Notice that this patch was sent against hpet as one example, if we
>>>>>> agree
>>>>>> that this "way" of disabling devices is ok, we could disable more
>>>>>> devices/have more flexibility.  Notice that in general, we (RHEL/KVM)
>>>>>> are interested in a small subset of qemu devices.
>>>>>>            
>>>>> IMO this patch is a backwards step.  The device models should be
>>>>> cleaned
>>>>> up so that you don't need to make a compile time decision.
>>>>>          
>>>> I disagree.  I think the device model should be cleaned up so that no
>>>> CONFIG_HPET is required in code but I think it's still useful to be
>>>> able
>>>> to exclude device models from the build.  That should just be a matter
>>>> of not building the object though (that's the point of device_init()).
>>>>        
>>> I think we're saying the same thing.
>>>
>>> We already have a mechanism for avoiding things at build time -
>>> specifically
>>> config-devices.mak. We don't have a nice UI for it, but it's there.
>>> At worst your distro specific patch is a 1-line change to default-
>>> configs/i386-softmmu.mak.
>>>
>>> I have no objection to moving hpet.c into Makefile.objs, conditional on
>>> CONFIG_HPET (like e.g. CONFIG_SERIAL/serial.o).  However a necessary
>>> prerequisite is that you fix the device model and machine
>>> initialisation so
>>> that it's possible to omit hpet.o without rebuilding anything else.
>>>      
>> We have two exported functions:
>>
>> void hpet_init(qemu_irq *irq);
>> uint32_t hpet_in_legacy_mode(void);
>>
>> This is how one is used in mc14818rtc:
>>
>> #if defined TARGET_I386
>>      if (!hpet_in_legacy_mode())
>> #endif
>>    
> 
> In real hardware, and HPET would normally emulate an RTC.  The
> interaction problem here is that we aren't modelling that correctly in
> qemu as we're treating the rtc as a separate device.

Not exactly: The HPET can only take over the periodic timer service of
the RTC. But the RTC can still deliver that one as well as all its other
IRQs via the SCI (part of ACPI). We don't implement the latter yet, though.

> 
> What could probably work at a hand wave level, is to make the rtc init
> function take a qemu_irq instead of directly grabbing the isa irq.  When
> an HPET is in use, the rtc no longer is directly initiated but instead
> is indirectly initiated by the HPET passing a special qemu_irq to the
> device that masks the actual interrupt line when legacy mode isn't
> enabled.  When the HPET isn't in use, the rtc would be created with an
> isa allocated qemu_irq.

I'm on this. But, as already indicated, the current "beautifulness" of
the RTC IRQ coalescing workaround kept me more busy than I expected.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Jan Kiszka]

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

Juan Quintela wrote:
>>> We already have to disable hpet for 5.4 (1 year ago).  It was done with
>>> a local hack because it was supposed that for next big release it would
>>> have been fixed.
>> But this remains a RHEL issue. Redhat decided to compile out features
>> that are unsupported, others seem to handle this differently.
> 
> And then, everybody has a different hack to disable the features that
> they don't need.  Instead of doing a local hack, we do a patch that
> allows anyone to disable HPET if it sees fit.

So far I only know of precisely one user that wants to disable x86
platform devices at build-time.

> 
>>> Here we are, and device is still not fixed, what to do?  Another local
>>> patch?  Just get upstream to integrate a sane way to disable it and let
>>> in enable by default?
>> Let's start with listing your requirements to no longer disable HPET.
> 
> It is not stable at this point in time :-(  Running with --no-hpet is
> better than without it in all our testing.  If we have to ask/modify
> everything to use --no-hpet, we can also compile-out it.
> 
>> That would already help us to asses how long !CONFIG_HPET would actually
>> be of any use at all. I'm yet optimistic that we can resolve most if not
>> all remaining concerns for 0.13 - and CONFIG_HPET would at best be 0.13
>> material anyway.
> 
> At this very point in time:
> - it is not stable

Well, that is helpful.

> - lack irq-reinjection when missing ticks

That is more helpful.

I just reworked the RTC regarding this, I guess it will be
straightforward to address it in the HPET too.

> 
> (I was not the one debugging/testing this so I don't have more details,
> but can ask for them).  So, it is not stable enough yet.

HPET is a fairly small device, a few hundred lines of code, only a few
ugly platform dependencies, but that's it already. I bet it could have
been fixed by now if someone just started by the time the tests reported
"it is not stable enough".

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Blue Swirl]

On Mon, May 24, 2010 at 6:03 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 05/24/2010 12:54 PM, Juan Quintela wrote:
>>
>> Paul Brook<paul@codesourcery.com>  wrote:
>>
>>>>
>>>> On 05/24/2010 11:32 AM, Paul Brook wrote:
>>>>
>>>>>>
>>>>>> Notice that this patch was sent against hpet as one example, if we
>>>>>> agree
>>>>>> that this "way" of disabling devices is ok, we could disable more
>>>>>> devices/have more flexibility.  Notice that in general, we (RHEL/KVM)
>>>>>> are interested in a small subset of qemu devices.
>>>>>>
>>>>>
>>>>> IMO this patch is a backwards step.  The device models should be
>>>>> cleaned
>>>>> up so that you don't need to make a compile time decision.
>>>>>
>>>>
>>>> I disagree.  I think the device model should be cleaned up so that no
>>>> CONFIG_HPET is required in code but I think it's still useful to be able
>>>> to exclude device models from the build.  That should just be a matter
>>>> of not building the object though (that's the point of device_init()).
>>>>
>>>
>>> I think we're saying the same thing.
>>>
>>> We already have a mechanism for avoiding things at build time -
>>> specifically
>>> config-devices.mak. We don't have a nice UI for it, but it's there.
>>> At worst your distro specific patch is a 1-line change to default-
>>> configs/i386-softmmu.mak.
>>>
>>> I have no objection to moving hpet.c into Makefile.objs, conditional on
>>> CONFIG_HPET (like e.g. CONFIG_SERIAL/serial.o).  However a necessary
>>> prerequisite is that you fix the device model and machine initialisation
>>> so
>>> that it's possible to omit hpet.o without rebuilding anything else.
>>>
>>
>> We have two exported functions:
>>
>> void hpet_init(qemu_irq *irq);
>> uint32_t hpet_in_legacy_mode(void);
>>
>> This is how one is used in mc14818rtc:
>>
>> #if defined TARGET_I386
>>     if (!hpet_in_legacy_mode())
>> #endif
>>
>
> In real hardware, and HPET would normally emulate an RTC.  The interaction
> problem here is that we aren't modelling that correctly in qemu as we're
> treating the rtc as a separate device.
>
> What could probably work at a hand wave level, is to make the rtc init
> function take a qemu_irq instead of directly grabbing the isa irq.  When an
> HPET is in use, the rtc no longer is directly initiated but instead is
> indirectly initiated by the HPET passing a special qemu_irq to the device
> that masks the actual interrupt line when legacy mode isn't enabled.  When
> the HPET isn't in use, the rtc would be created with an isa allocated
> qemu_irq.

Fully agree, HPET logic should be moved into HPET. I think my
yesterday's patches only went half way.

[Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Paolo Bonzini]

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

On 05/24/2010 07:54 PM, Juan Quintela wrote:
> But for the other call, what do you propose?
>
> My best try was to hide the availability of hpet inside hpet_emul.h
> with:
>
> #ifdef CONFIG_HPET
> uint32_t hpet_in_legacy_mode(void);
> else
> uint32_t hpet_in_legacy_mode(void) { return 0;}
> #endif

Change this to a global variable rtc_disable_interrupts in 
hw/mc146818rtc.c?  (You didn't say it would need to be particularly 
pretty...).

Not tested beyond compilation.

Paolo

[-- Attachment #2: hpet-hack.patch --]
[-- Type: text/plain, Size: 3157 bytes --]

diff --git a/hw/hpet.c b/hw/hpet.c
index 8729fb2..c2615c1 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -29,6 +29,7 @@
 #include "console.h"
 #include "qemu-timer.h"
 #include "hpet_emul.h"
+#include "mc146818rtc.h"
 
 //#define HPET_DEBUG
 #ifdef HPET_DEBUG
@@ -39,14 +40,6 @@
 
 static HPETState *hpet_statep;
 
-uint32_t hpet_in_legacy_mode(void)
-{
-    if (hpet_statep)
-        return hpet_statep->config & HPET_CFG_LEGACY;
-    else
-        return 0;
-}
-
 static uint32_t timer_int_route(struct HPETTimer *timer)
 {
     uint32_t route;
@@ -139,7 +132,7 @@ static void update_irq(struct HPETTimer *timer)
     qemu_irq irq;
     int route;
 
-    if (timer->tn <= 1 && hpet_in_legacy_mode()) {
+    if (timer->tn <= 1 && (timer->state->config & HPET_CFG_LEGACY)) {
         /* if LegacyReplacementRoute bit is set, HPET specification requires
          * timer0 be routed to IRQ0 in NON-APIC or IRQ2 in the I/O APIC,
          * timer1 be routed to IRQ8 in NON-APIC or IRQ8 in the I/O APIC.
@@ -474,8 +467,10 @@ static void hpet_ram_writel(void *opaque, target_phys_addr_t addr,
                 /* i8254 and RTC are disabled when HPET is in legacy mode */
                 if (activating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
                     hpet_pit_disable();
+		    rtc_disable_interrupts = 1;
                 } else if (deactivating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
                     hpet_pit_enable();
+		    rtc_disable_interrupts = 0;
                 }
                 break;
             case HPET_CFG + 4:
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 571c593..61d5980 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -94,6 +94,9 @@ typedef struct RTCState {
     QEMUTimer *second_timer2;
 } RTCState;
 
+
+int rtc_disable_interrupts = 0;
+
 static void rtc_irq_raise(qemu_irq irq)
 {
     /* When HPET is operating in legacy mode, RTC interrupts are disabled
@@ -101,9 +104,7 @@ static void rtc_irq_raise(qemu_irq irq)
      * mode is established while interrupt is raised. We want it to
      * be lowered in any case
      */
-#if defined TARGET_I386
-    if (!hpet_in_legacy_mode())
-#endif
+    if (!rtc_disable_interrupts)
         qemu_irq_raise(irq);
 }
 
@@ -148,14 +149,10 @@ static void rtc_timer_update(RTCState *s, int64_t current_time)
     int enable_pie;
 
     period_code = s->cmos_data[RTC_REG_A] & 0x0f;
-#if defined TARGET_I386
     /* disable periodic timer if hpet is in legacy mode, since interrupts are
      * disabled anyway.
      */
-    enable_pie = !hpet_in_legacy_mode();
-#else
-    enable_pie = 1;
-#endif
+    enable_pie = !rtc_disable_interrupts;
     if (period_code != 0
         && (((s->cmos_data[RTC_REG_B] & REG_B_PIE) && enable_pie)
             || ((s->cmos_data[RTC_REG_B] & REG_B_SQWE) && s->sqw_irq))) {
diff --git a/hw/mc146818rtc.h b/hw/mc146818rtc.h
index 6f46a68..ff4bcda 100644
--- a/hw/mc146818rtc.h
+++ b/hw/mc146818rtc.h
@@ -3,6 +3,7 @@
 
 #include "isa.h"
 
+extern int rtc_disable_interrupts;
 ISADevice *rtc_init(int base_year);
 void rtc_set_memory(ISADevice *dev, int addr, int val);
 void rtc_set_date(ISADevice *dev, const struct tm *tm);

[Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Jan Kiszka]

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

Paolo Bonzini wrote:
> On 05/24/2010 07:54 PM, Juan Quintela wrote:
>> But for the other call, what do you propose?
>>
>> My best try was to hide the availability of hpet inside hpet_emul.h
>> with:
>>
>> #ifdef CONFIG_HPET
>> uint32_t hpet_in_legacy_mode(void);
>> else
>> uint32_t hpet_in_legacy_mode(void) { return 0;}
>> #endif
> 
> Change this to a global variable rtc_disable_interrupts in
> hw/mc146818rtc.c?  (You didn't say it would need to be particularly
> pretty...).
> 
> Not tested beyond compilation.
> 
> Paolo
> 

Please don't waste your time:

http://permalink.gmane.org/gmane.comp.emulators.qemu/71377

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 0/6] Make hpet a compile time option

[Paolo Bonzini]

On 05/25/2010 11:05 AM, Jan Kiszka wrote:
> Please don't waste your time:
>
> http://permalink.gmane.org/gmane.comp.emulators.qemu/71377

I wasn't going to. :-)  I had seen the series---very nice work!

Paolo