On 2012-01-31 15:43, Anthony Liguori wrote:
On 01/31/2012 08:26 AM, Jan Kiszka wrote:
On 2012-01-26 20:00, Anthony Liguori wrote:
Signed-off-by: Anthony Liguori<address@hidden>
---
hw/hpet.c | 38 +-------------------------
hw/hpet_emul.h | 40 ++++++++++++++++++++++++++++
hw/mc146818rtc.c | 30 ++-------------------
hw/mc146818rtc.h | 27 +++++++++++++++++++
hw/pc.c | 38 +++++----------------------
hw/piix_pci.c | 76
++++++++++++++++++++++++++++++++++++++++++++++-------
6 files changed, 145 insertions(+), 104 deletions(-)
diff --git a/hw/hpet.c b/hw/hpet.c
index b6ace4e..c5b8b9e 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -41,40 +41,6 @@
#define HPET_MSI_SUPPORT 0
-struct HPETState;
-typedef struct HPETTimer { /* timers */
- uint8_t tn; /*timer number*/
- QEMUTimer *qemu_timer;
- struct HPETState *state;
- /* Memory-mapped, software visible timer registers */
- uint64_t config; /* configuration/cap */
- uint64_t cmp; /* comparator */
- uint64_t fsb; /* FSB route */
- /* Hidden register state */
- uint64_t period; /* Last value written to comparator */
- uint8_t wrap_flag; /* timer pop will indicate wrap for one-shot 32-bit
- * mode. Next pop will be actual timer expiration.
- */
-} HPETTimer;
-
-typedef struct HPETState {
- SysBusDevice busdev;
- MemoryRegion iomem;
- uint64_t hpet_offset;
- qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
- uint32_t flags;
- uint8_t rtc_irq_level;
- uint8_t num_timers;
- HPETTimer timer[HPET_MAX_TIMERS];
-
- /* Memory-mapped, software visible registers */
- uint64_t capability; /* capabilities */
- uint64_t config; /* configuration */
- uint64_t isr; /* interrupt status reg */
- uint64_t hpet_counter; /* main counter */
- uint8_t hpet_id; /* instance id */
-} HPETState;
-
Both structs are private and should remain so, same for similar patches
in this series. Does your composition concept requires publicizing them?
If yes, can't it be fixed. Would be a step backward if not.
It doesn't strictly require it, no, but I like it. It encourages using proper
interfaces like:
void rtc_set_memory(RTCState *rtc, int addr, int val);
Instead of:
void rtc_set_memory(ISADevice *dev, int addr, int val);
Yes, we can achieve the same thing with forward declarations. The second thing
I like about this style is that it makes it easier to use a code generator to
generate serialization functions. Finally, I think embedded a devices memory
within its parent device provides a certain level of elegance.
It reopens the door for poking inside the device states. That was closed
(widely) by privatizing the states (I think mostly driven by Blue). I'm
not convinced yet that being able to embed the struct into a containing
device is worth giving up on this.
Also note that the HPET is not a part of the PIIX, so composition is
wrong here.
There is no HPET in an i440fx system. The HPET usually sits on the LPC bus
(which replaces ISA in modern systems). It's sometimes a dedicated chip but can
certain co-exist in a Super IO chip. I think in terms of where it would live in
this hypothetical device model, putting it in the PIIX is rational.
Does it buy us anything? I don't see the advantage of this imprecision.
If the model works well, it should be able to cover the real
architecture elegantly, too.