From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:36881) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RsF8a-0003kG-Ix for qemu-devel@nongnu.org; Tue, 31 Jan 2012 09:56:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RsF8T-00073g-1I for qemu-devel@nongnu.org; Tue, 31 Jan 2012 09:56:48 -0500 Received: from david.siemens.de ([192.35.17.14]:29279) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RsF8S-00073Y-O3 for qemu-devel@nongnu.org; Tue, 31 Jan 2012 09:56:41 -0500 Message-ID: <4F280124.2030804@siemens.com> Date: Tue, 31 Jan 2012 15:56:36 +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> <4F27FF95.7000803@siemens.com> <4F2800B2.7030603@codemonkey.ws> In-Reply-To: <4F2800B2.7030603@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: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. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux