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

[Anthony Liguori <anthony@codemonkey.ws>]

On 01/31/2012 08:26 AM, Jan Kiszka wrote:
> On 2012-01-26 20:00, Anthony Liguori wrote:
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   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
>