| Message ID | 1327604460-31142-6-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 2012-01-26 20:00, Anthony Liguori wrote: > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > --- > 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. Also note that the HPET is not a part of the PIIX, so composition is wrong here. The RTC is again. Jan
On 01/31/2012 08:26 AM, Jan Kiszka wrote: > On 2012-01-26 20:00, Anthony Liguori wrote: >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >> --- >> 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 >
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<aliguori@us.ibm.com> >>> --- >>> 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
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<aliguori@us.ibm.com> >>>> --- >>>> 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 >
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<aliguori@us.ibm.com> >>>>> --- >>>>> 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
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<aliguori@us.ibm.com> >>>>>> --- >>>>>> 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<LPCDevice> in the i440fx-pmc for expansion. Regards, Anthony Liguori > > Jan >
On 2012-01-31 15:26, Jan Kiszka wrote: > Also note that the HPET is not a part of the PIIX, so composition is > wrong here. The RTC is again. Err, forgot my nonsense. The HPET is part of the PIIX. Dunno, I was somehow thinking of the IOAPIC while reading "HPET". Too few sleep, I guess... Jan
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; - static uint32_t hpet_in_legacy_mode(HPETState *s) { return s->config & HPET_CFG_LEGACY; @@ -258,7 +224,7 @@ static const VMStateDescription vmstate_hpet_timer = { }; static const VMStateDescription vmstate_hpet = { - .name = "hpet", + .name = TYPE_HPET, .version_id = 2, .minimum_version_id = 1, .minimum_version_id_old = 1, @@ -714,7 +680,7 @@ static void hpet_device_class_init(ObjectClass *klass, void *data) } static TypeInfo hpet_device_info = { - .name = "hpet", + .name = TYPE_HPET, .parent = TYPE_SYS_BUS_DEVICE, .instance_size = sizeof(HPETState), .class_init = hpet_device_class_init, diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h index 6128702..da8ecdc 100644 --- a/hw/hpet_emul.h +++ b/hw/hpet_emul.h @@ -13,6 +13,9 @@ #ifndef QEMU_HPET_EMUL_H #define QEMU_HPET_EMUL_H +#include "hw.h" +#include "sysbus.h" + #define HPET_BASE 0xfed00000 #define HPET_CLK_PERIOD 10000000ULL /* 10000000 femtoseconds == 10ns*/ @@ -68,4 +71,41 @@ struct hpet_fw_config } QEMU_PACKED; extern struct hpet_fw_config hpet_cfg; + +#define TYPE_HPET "hpet" + +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; + #endif diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c index 346a95e..f967e05 100644 --- a/hw/mc146818rtc.c +++ b/hw/mc146818rtc.c @@ -79,30 +79,6 @@ #define REG_C_PF 0x40 #define REG_C_AF 0x20 -typedef struct RTCState { - ISADevice dev; - MemoryRegion io; - uint8_t cmos_data[128]; - uint8_t cmos_index; - struct tm current_tm; - int32_t base_year; - qemu_irq irq; - qemu_irq sqw_irq; - int it_shift; - /* periodic timer */ - QEMUTimer *periodic_timer; - int64_t next_periodic_time; - /* second update */ - int64_t next_second_time; - uint16_t irq_reinject_on_ack_count; - uint32_t irq_coalesced; - uint32_t period; - QEMUTimer *coalesced_timer; - QEMUTimer *second_timer; - QEMUTimer *second_timer2; - Notifier clock_reset_notifier; -} RTCState; - static void rtc_set_time(RTCState *s); static void rtc_copy_date(RTCState *s); @@ -553,7 +529,7 @@ static int rtc_post_load(void *opaque, int version_id) } static const VMStateDescription vmstate_rtc = { - .name = "mc146818rtc", + .name = TYPE_RTC, .version_id = 2, .minimum_version_id = 1, .minimum_version_id_old = 1, @@ -687,7 +663,7 @@ ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq) ISADevice *dev; RTCState *s; - dev = isa_create(bus, "mc146818rtc"); + dev = isa_create(bus, TYPE_RTC); s = DO_UPCAST(RTCState, dev, dev); qdev_prop_set_int32(&dev->qdev, "base_year", base_year); qdev_init_nofail(&dev->qdev); @@ -715,7 +691,7 @@ static void rtc_class_initfn(ObjectClass *klass, void *data) } static TypeInfo mc146818rtc_info = { - .name = "mc146818rtc", + .name = TYPE_RTC, .parent = TYPE_ISA_DEVICE, .instance_size = sizeof(RTCState), .class_init = rtc_class_initfn, diff --git a/hw/mc146818rtc.h b/hw/mc146818rtc.h index f119930..ce807c0 100644 --- a/hw/mc146818rtc.h +++ b/hw/mc146818rtc.h @@ -2,9 +2,36 @@ #define MC146818RTC_H #include "isa.h" +#include "notify.h" #define RTC_ISA_IRQ 8 +#define TYPE_RTC "mc146818rtc" + +typedef struct RTCState { + ISADevice dev; + MemoryRegion io; + uint8_t cmos_data[128]; + uint8_t cmos_index; + struct tm current_tm; + int32_t base_year; + qemu_irq irq; + qemu_irq sqw_irq; + int it_shift; + /* periodic timer */ + QEMUTimer *periodic_timer; + int64_t next_periodic_time; + /* second update */ + int64_t next_second_time; + uint16_t irq_reinject_on_ack_count; + uint32_t irq_coalesced; + uint32_t period; + QEMUTimer *coalesced_timer; + QEMUTimer *second_timer; + QEMUTimer *second_timer2; + Notifier clock_reset_notifier; +} RTCState; + ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq); void rtc_set_memory(ISADevice *dev, int addr, int val); void rtc_set_date(ISADevice *dev, const struct tm *tm); diff --git a/hw/pc.c b/hw/pc.c index 4b11e44..d3eba63 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -1143,13 +1143,11 @@ static void cpu_request_exit(void *opaque, int irq, int level) } static void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, - ISADevice **rtc_state, - ISADevice **floppy, - bool no_vmport) + ISADevice **floppy, + bool no_vmport) { int i; DriveInfo *fd[MAX_FD]; - qemu_irq rtc_irq = NULL; qemu_irq *a20_line; ISADevice *i8042, *port92, *vmmouse, *pit; qemu_irq *cpu_exit_irq; @@ -1158,20 +1156,6 @@ static void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, register_ioport_write(0xf0, 1, 1, ioportF0_write, NULL); - if (!no_hpet) { - DeviceState *hpet = sysbus_try_create_simple("hpet", HPET_BASE, NULL); - - if (hpet) { - for (i = 0; i < GSI_NUM_PINS; i++) { - sysbus_connect_irq(sysbus_from_qdev(hpet), i, gsi[i]); - } - rtc_irq = qdev_get_gpio_in(hpet, 0); - } - } - *rtc_state = rtc_init(isa_bus, 2000, rtc_irq); - - qemu_register_boot_set(pc_boot_set, *rtc_state); - pit = pit_init(isa_bus, 0x40, 0); pcspk_init(pit); @@ -1377,7 +1361,6 @@ static void pc_init1(MemoryRegion *system_memory, isa_bus = isa_bus_new(NULL, system_io); no_hpet = 1; } - isa_bus_irqs(isa_bus, gsi); if (kvm_enabled() && kvm_irqchip_in_kernel()) { i8259 = kvm_i8259_init(isa_bus); @@ -1407,7 +1390,7 @@ static void pc_init1(MemoryRegion *system_memory, } /* init basic PC hardware */ - pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled()); + pc_basic_device_init(isa_bus, gsi, &floppy, xen_enabled()); for(i = 0; i < nb_nics; i++) { NICInfo *nd = &nd_table[i]; @@ -1428,17 +1411,6 @@ static void pc_init1(MemoryRegion *system_memory, } idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0"); idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1"); - - /* FIXME there's some major spaghetti here. Somehow we create the - * devices on the PIIX before we actually create it. We create the - * PIIX3 deep in the recess of the i440fx creation too and then lose - * the DeviceState. - * - * For now, let's "fix" this by making judicious use of paths. This - * is not generally the right way to do this. - */ - object_property_add_child(object_resolve_path("/i440fx/piix3", NULL), - "rtc", (Object *)rtc_state, NULL); } else { for(i = 0; i < MAX_IDE_BUS; i++) { ISADevice *dev; @@ -1449,6 +1421,10 @@ static void pc_init1(MemoryRegion *system_memory, } } + /* FIXME */ + rtc_state = ISA_DEVICE(object_resolve_path("rtc", NULL)); + qemu_register_boot_set(pc_boot_set, rtc_state); + audio_init(isa_bus, pci_enabled ? pci_bus : NULL); pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device, diff --git a/hw/piix_pci.c b/hw/piix_pci.c index ec75725..5d7d175 100644 --- a/hw/piix_pci.c +++ b/hw/piix_pci.c @@ -30,6 +30,8 @@ #include "sysbus.h" #include "range.h" #include "xen.h" +#include "hpet_emul.h" +#include "mc146818rtc.h" /* * I440FX chipset data sheet. @@ -63,6 +65,13 @@ typedef struct PIIX3State { #endif uint64_t pic_levels; + bool hpet_enable; + + HPETState hpet; + RTCState rtc; + + ISABus *bus; + qemu_irq *pic; /* This member isn't used. Just for save/load compatibility */ @@ -309,20 +318,28 @@ static PCIBus *i440fx_common_init(const char *device_name, * connected to the IOAPIC directly. * These additional routes can be discovered through ACPI. */ if (xen_enabled()) { - piix3 = DO_UPCAST(PIIX3State, dev, - pci_create_simple_multifunction(b, -1, true, "PIIX3-xen")); + piix3 = PIIX3(object_new("PIIX3-xen")); + } else { + piix3 = PIIX3(object_new("PIIX3")); + } + + /* FIXME make this a property */ + piix3->pic = pic; + qdev_prop_set_uint32(DEVICE(piix3), "addr", PCI_DEVFN(1, 0)); + qdev_prop_set_bit(DEVICE(piix3), "multifunction", true); + qdev_set_parent_bus(DEVICE(piix3), BUS(s->bus)); + qdev_init_nofail(DEVICE(piix3)); + + if (xen_enabled()) { pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq, - piix3, XEN_PIIX_NUM_PIRQS); + piix3, XEN_PIIX_NUM_PIRQS); } else { - piix3 = DO_UPCAST(PIIX3State, dev, - pci_create_simple_multifunction(b, -1, true, "PIIX3")); pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3, PIIX_NUM_PIRQS); } + object_property_add_child(OBJECT(dev), "piix3", OBJECT(piix3), NULL); - piix3->pic = pic; - *isa_bus = DO_UPCAST(ISABus, qbus, - qdev_get_child_bus(&piix3->dev.qdev, "isa.0")); + *isa_bus = piix3->bus; *piix3_devfn = piix3->dev.devfn; @@ -498,14 +515,53 @@ static const VMStateDescription vmstate_piix3 = { static int piix3_realize(PCIDevice *dev) { - PIIX3State *d = DO_UPCAST(PIIX3State, dev, dev); + PIIX3State *s = PIIX3(dev); + qemu_irq rtc_irq; + + /* Initialize ISA Bus */ + s->bus = isa_bus_new(DEVICE(dev), pci_address_space_io(dev)); + isa_bus_irqs(s->bus, s->pic); + + /* Realize the RTC */ + qdev_set_parent_bus(DEVICE(&s->rtc), BUS(s->bus)); + qdev_init_nofail(DEVICE(&s->rtc)); + + /* Realize HPET */ + if (s->hpet_enable) { + int i; + + /* We need to introduce a proper IRQ and Memory QOM infrastructure + * so that the HPET isn't a sysbus device */ + qdev_set_parent_bus(DEVICE(&s->hpet), sysbus_get_default()); + qdev_init_nofail(DEVICE(&s->hpet)); + + sysbus_mmio_map(SYS_BUS_DEVICE(&s->hpet), 0, HPET_BASE); + for (i = 0; i < GSI_NUM_PINS; i++) { + sysbus_connect_irq(SYS_BUS_DEVICE(&s->hpet), i, s->pic[i]); + } + + rtc_irq = qdev_get_gpio_in(DEVICE(&s->hpet), 0); + } else { + isa_init_irq(ISA_DEVICE(&s->rtc), &rtc_irq, RTC_ISA_IRQ); + } + + /* Setup the RTC IRQ */ + s->rtc.irq = rtc_irq; - isa_bus_new(&d->dev.qdev, pci_address_space_io(dev)); return 0; } static void piix3_initfn(Object *obj) { + PIIX3State *s = PIIX3(obj); + + object_initialize(&s->hpet, TYPE_HPET); + object_property_add_child(obj, "hpet", OBJECT(&s->hpet), NULL); + + object_initialize(&s->rtc, TYPE_RTC); + object_property_add_child(obj, "rtc", OBJECT(&s->rtc), NULL); + + qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000); } static void piix3_class_init(ObjectClass *klass, void *data)
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- 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(-)