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

[Jan Kiszka]

On 2012-01-31 15:54, Anthony Liguori wrote:
> 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.

Nope, it was a separate chip in such systems. It sits on the board
(today our sysbus), nakedly and alone.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux