From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:60017) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RsF2A-0007vk-LW for qemu-devel@nongnu.org; Tue, 31 Jan 2012 09:50:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RsF24-0005YO-23 for qemu-devel@nongnu.org; Tue, 31 Jan 2012 09:50:10 -0500 Received: from goliath.siemens.de ([192.35.17.28]:34237) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RsF23-0005XK-Hu for qemu-devel@nongnu.org; Tue, 31 Jan 2012 09:50:04 -0500 Message-ID: <4F27FF95.7000803@siemens.com> Date: Tue, 31 Jan 2012 15:49:57 +0100 From: Jan Kiszka 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> In-Reply-To: <4F27FE0A.9070105@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-15 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: Anthony Liguori Cc: Peter Maydell , Anthony Liguori , "qemu-devel@nongnu.org" , Markus Armbruster , Blue Swirl , Avi Kivity , Paolo Bonzini 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. > >> The RTC is again. > > -ENOPARSE I meant that the RTC was correctly moved into the PIIX. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux