From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:36297) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RsFFw-0008Ul-NH for qemu-devel@nongnu.org; Tue, 31 Jan 2012 10:04:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RsFFq-0000AO-Rk for qemu-devel@nongnu.org; Tue, 31 Jan 2012 10:04:24 -0500 Received: from mail-pz0-f45.google.com ([209.85.210.45]:44010) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RsFFq-0000AE-IR for qemu-devel@nongnu.org; Tue, 31 Jan 2012 10:04:18 -0500 Received: by dadp14 with SMTP id p14so30324dad.4 for ; Tue, 31 Jan 2012 07:04:17 -0800 (PST) Message-ID: <4F2802EC.7040903@codemonkey.ws> Date: Tue, 31 Jan 2012 09:04:12 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <1327604460-31142-1-git-send-email-aliguori@us.ibm.com> <1327604460-31142-6-git-send-email-aliguori@us.ibm.com> <4F27FA04.5050106@siemens.com> <4F27FE0A.9070105@codemonkey.ws> <4F27FF95.7000803@siemens.com> <4F2800B2.7030603@codemonkey.ws> <4F280124.2030804@siemens.com> In-Reply-To: <4F280124.2030804@siemens.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 05/15] piix: create the HPET and RTC through composition List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Peter Maydell , Anthony Liguori , "qemu-devel@nongnu.org" , Markus Armbruster , Blue Swirl , Avi Kivity , Paolo Bonzini 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 >>>>>> --- >>>>>> 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 in the i440fx-pmc for expansion. Regards, Anthony Liguori > > Jan >