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

[Anthony Liguori]

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.

> 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.

> The RTC is again.

-ENOPARSE

Regards,

Anthony Liguori

> Jan