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

[Anthony Liguori <anthony@codemonkey.ws>]

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.

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
>