From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:33856) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RsEvl-0004AN-TZ for qemu-devel@nongnu.org; Tue, 31 Jan 2012 09:43:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RsEvf-0003LP-P2 for qemu-devel@nongnu.org; Tue, 31 Jan 2012 09:43:33 -0500 Received: from mail-pz0-f45.google.com ([209.85.210.45]:40779) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RsEvf-0003LF-K7 for qemu-devel@nongnu.org; Tue, 31 Jan 2012 09:43:27 -0500 Received: by dadp14 with SMTP id p14so10675dad.4 for ; Tue, 31 Jan 2012 06:43:26 -0800 (PST) Message-ID: <4F27FE0A.9070105@codemonkey.ws> Date: Tue, 31 Jan 2012 08:43:22 -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> In-Reply-To: <4F27FA04.5050106@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 , Avi Kivity , Paolo Bonzini 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. > 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. > The RTC is again. -ENOPARSE Regards, Anthony Liguori > > Jan >