qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 05/15] piix: create the HPET and RTC through com


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH 05/15] piix: create the HPET and RTC through composition
Date: Tue, 31 Jan 2012 08:54:42 -0600
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.23) Gecko/20110922 Lightning/1.0b2 Thunderbird/3.1.15

On 01/31/2012 08:49 AM, Jan Kiszka wrote:
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.

We could move the HPET to a child of the 440fx-pmc.  That's probably more 
correct.

I don't think it's worth modeling an LPC bus. LPC is just a spec for allowing third party chips, it's not mandated that all HPET chips have a pin-out that's LPC compatible. I don't think there's any guest-visible state that comes from a device being on the LPC verses being hard wired in the north bridge.

Regards,

Anthony Liguori



The RTC is again.

-ENOPARSE

I meant that the RTC was correctly moved into the PIIX.

Jan





reply via email to

[Prev in Thread] Current Thread [Next in Thread]