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

[Anthony Liguori <anthony@codemonkey.ws>]

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

So the northbridge would need to implement an LPC bus.  This can be as simple as 
having an LPC interface (which just consists of a few MemoryRegions and Pins) 
and then a few link<LPCDevice> in the i440fx-pmc for expansion.

Regards,

Anthony Liguori

>
> Jan
>