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<aligu...@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.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
