From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:36334) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RsF6k-0002lW-JS for qemu-devel@nongnu.org; Tue, 31 Jan 2012 09:54:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RsF6d-0006cD-T7 for qemu-devel@nongnu.org; Tue, 31 Jan 2012 09:54:54 -0500 Received: from mail-pz0-f45.google.com ([209.85.210.45]:43838) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RsF6d-0006c5-H9 for qemu-devel@nongnu.org; Tue, 31 Jan 2012 09:54:47 -0500 Received: by dadp14 with SMTP id p14so21066dad.4 for ; Tue, 31 Jan 2012 06:54:46 -0800 (PST) Message-ID: <4F2800B2.7030603@codemonkey.ws> Date: Tue, 31 Jan 2012 08:54:42 -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> In-Reply-To: <4F27FF95.7000803@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: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. I don't think it's worth modeling an LPC bus. LPC is just a spec for allowing third party chips, it's not mandated that all HPET chips have a pin-out that's LPC compatible. I don't think there's any guest-visible state that comes from a device being on the LPC verses being hard wired in the north bridge. Regards, Anthony Liguori > >> >>> The RTC is again. >> >> -ENOPARSE > > I meant that the RTC was correctly moved into the PIIX. > > Jan >