* [PATCH 0/2] Make mc146818rtc more self-contained @ 2022-02-05 17:59 Bernhard Beschow 2022-02-05 17:59 ` [PATCH 1/2] isa/piix4: Resolve RTCState attribute Bernhard Beschow 2022-02-05 17:59 ` [PATCH 2/2] mc146818rtc: Unexport RTCState Bernhard Beschow 0 siblings, 2 replies; 5+ messages in thread From: Bernhard Beschow @ 2022-02-05 17:59 UTC (permalink / raw) To: qemu-devel; +Cc: Bernhard Beschow The internal state of mc146818rtc is currently exported as RTCState. This violates encapsulation. Resolve it by first converting the last external user, then by unexporting RTCState. Bernhard Beschow (2): isa/piix4: Resolve RTCState attribute mc146818rtc: Unexport RTCState hw/isa/piix4.c | 15 +-------------- hw/rtc/mc146818rtc.c | 34 ++++++++++++++++++++++++++++++++++ include/hw/rtc/mc146818rtc.h | 35 +---------------------------------- 3 files changed, 36 insertions(+), 48 deletions(-) -- 2.35.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] isa/piix4: Resolve RTCState attribute 2022-02-05 17:59 [PATCH 0/2] Make mc146818rtc more self-contained Bernhard Beschow @ 2022-02-05 17:59 ` Bernhard Beschow 2022-02-05 18:53 ` Peter Maydell 2022-02-05 17:59 ` [PATCH 2/2] mc146818rtc: Unexport RTCState Bernhard Beschow 1 sibling, 1 reply; 5+ messages in thread From: Bernhard Beschow @ 2022-02-05 17:59 UTC (permalink / raw) To: qemu-devel Cc: Hervé Poussineau, Bernhard Beschow, Philippe Mathieu-Daudé, Aurelien Jarno Assuming that mc146818_rtc_init() is "syntactic sugar" for code that could be converted into configuration in the future, this patch is a step towards this future by freeing piix4 to care about the inner details of mc146818. Furthermore, by reusing mc146818_rtc_init(), piix4's code becomes more homogenious to other code using the mc146818. So piix4 can be refactored in the same way as code already using mc146818_rtc_init(). Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/isa/piix4.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c index 0fe7b69bc4..08b4262467 100644 --- a/hw/isa/piix4.c +++ b/hw/isa/piix4.c @@ -46,7 +46,6 @@ struct PIIX4State { qemu_irq cpu_intr; qemu_irq *isa; - RTCState rtc; /* Reset Control Register */ MemoryRegion rcr_mem; uint8_t rcr; @@ -193,22 +192,11 @@ static void piix4_realize(PCIDevice *dev, Error **errp) i8257_dma_init(isa_bus, 0); /* RTC */ - qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000); - if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) { - return; - } - isa_init_irq(ISA_DEVICE(&s->rtc), &s->rtc.irq, RTC_ISA_IRQ); + mc146818_rtc_init(isa_bus, 2000, NULL); piix4_dev = dev; } -static void piix4_init(Object *obj) -{ - PIIX4State *s = PIIX4_PCI_DEVICE(obj); - - object_initialize(&s->rtc, sizeof(s->rtc), TYPE_MC146818_RTC); -} - static void piix4_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -233,7 +221,6 @@ static const TypeInfo piix4_info = { .name = TYPE_PIIX4_PCI_DEVICE, .parent = TYPE_PCI_DEVICE, .instance_size = sizeof(PIIX4State), - .instance_init = piix4_init, .class_init = piix4_class_init, .interfaces = (InterfaceInfo[]) { { INTERFACE_CONVENTIONAL_PCI_DEVICE }, -- 2.35.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] isa/piix4: Resolve RTCState attribute 2022-02-05 17:59 ` [PATCH 1/2] isa/piix4: Resolve RTCState attribute Bernhard Beschow @ 2022-02-05 18:53 ` Peter Maydell 2022-02-05 22:59 ` Philippe Mathieu-Daudé via 0 siblings, 1 reply; 5+ messages in thread From: Peter Maydell @ 2022-02-05 18:53 UTC (permalink / raw) To: Bernhard Beschow Cc: Hervé Poussineau, qemu-devel, Aurelien Jarno, Philippe Mathieu-Daudé On Sat, 5 Feb 2022 at 18:05, Bernhard Beschow <shentey@gmail.com> wrote: > > Assuming that mc146818_rtc_init() is "syntactic sugar" for code that could > be converted into configuration in the future, this patch is a step towards > this future by freeing piix4 to care about the inner details of mc146818. > > Furthermore, by reusing mc146818_rtc_init(), piix4's code becomes more > homogenious to other code using the mc146818. So piix4 can be refactored in > the same way as code already using mc146818_rtc_init(). > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/isa/piix4.c | 15 +-------------- > 1 file changed, 1 insertion(+), 14 deletions(-) > > diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c > index 0fe7b69bc4..08b4262467 100644 > --- a/hw/isa/piix4.c > +++ b/hw/isa/piix4.c > @@ -46,7 +46,6 @@ struct PIIX4State { > qemu_irq cpu_intr; > qemu_irq *isa; > > - RTCState rtc; > /* Reset Control Register */ > MemoryRegion rcr_mem; > uint8_t rcr; > @@ -193,22 +192,11 @@ static void piix4_realize(PCIDevice *dev, Error **errp) > i8257_dma_init(isa_bus, 0); > > /* RTC */ > - qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000); > - if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) { > - return; > - } > - isa_init_irq(ISA_DEVICE(&s->rtc), &s->rtc.irq, RTC_ISA_IRQ); > + mc146818_rtc_init(isa_bus, 2000, NULL); > > piix4_dev = dev; > } > > -static void piix4_init(Object *obj) > -{ > - PIIX4State *s = PIIX4_PCI_DEVICE(obj); > - > - object_initialize(&s->rtc, sizeof(s->rtc), TYPE_MC146818_RTC); > -} > - > static void piix4_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -233,7 +221,6 @@ static const TypeInfo piix4_info = { > .name = TYPE_PIIX4_PCI_DEVICE, > .parent = TYPE_PCI_DEVICE, > .instance_size = sizeof(PIIX4State), > - .instance_init = piix4_init, > .class_init = piix4_class_init, > .interfaces = (InterfaceInfo[]) { > { INTERFACE_CONVENTIONAL_PCI_DEVICE }, This looks like it's going backwards from the way we'd usually write code for devices that contain other devices these days. The "we have an init function that does stuff" is the older style. The newer style has the inner-device as an embedded struct in the container-device struct, which is initialized, configured and realized using standard functions like object_initialize and qdev_realize. (I do wonder whether that ought to be object_initialize_child() here, incidentally, but haven't checked the details to be certain.) The existing uses of mc146818_rtc_init() are mostly in older code, and also in board-initialization code, which traditionally didn't have a convenient struct to embed the device-struct into. hw/isa/vt82c686.c is the only use in another device model (which could in theory be refactored to the embed-the-device-struct style, though the benefit of making the change isn't large, which is one reason we still have the mix of both in the tree). thanks -- PMM ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] isa/piix4: Resolve RTCState attribute 2022-02-05 18:53 ` Peter Maydell @ 2022-02-05 22:59 ` Philippe Mathieu-Daudé via 0 siblings, 0 replies; 5+ messages in thread From: Philippe Mathieu-Daudé via @ 2022-02-05 22:59 UTC (permalink / raw) To: Peter Maydell, Bernhard Beschow Cc: qemu-devel, Hervé Poussineau, Aurelien Jarno, Thomas Huth On 5/2/22 19:53, Peter Maydell wrote: > On Sat, 5 Feb 2022 at 18:05, Bernhard Beschow <shentey@gmail.com> wrote: >> >> Assuming that mc146818_rtc_init() is "syntactic sugar" for code that could >> be converted into configuration in the future, this patch is a step towards >> this future by freeing piix4 to care about the inner details of mc146818. >> >> Furthermore, by reusing mc146818_rtc_init(), piix4's code becomes more >> homogenious to other code using the mc146818. So piix4 can be refactored in >> the same way as code already using mc146818_rtc_init(). >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/isa/piix4.c | 15 +-------------- >> 1 file changed, 1 insertion(+), 14 deletions(-) >> >> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c >> index 0fe7b69bc4..08b4262467 100644 >> --- a/hw/isa/piix4.c >> +++ b/hw/isa/piix4.c >> @@ -46,7 +46,6 @@ struct PIIX4State { >> qemu_irq cpu_intr; >> qemu_irq *isa; >> >> - RTCState rtc; >> /* Reset Control Register */ >> MemoryRegion rcr_mem; >> uint8_t rcr; >> @@ -193,22 +192,11 @@ static void piix4_realize(PCIDevice *dev, Error **errp) >> i8257_dma_init(isa_bus, 0); >> >> /* RTC */ >> - qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000); >> - if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) { >> - return; >> - } >> - isa_init_irq(ISA_DEVICE(&s->rtc), &s->rtc.irq, RTC_ISA_IRQ); >> + mc146818_rtc_init(isa_bus, 2000, NULL); >> >> piix4_dev = dev; >> } >> >> -static void piix4_init(Object *obj) >> -{ >> - PIIX4State *s = PIIX4_PCI_DEVICE(obj); >> - >> - object_initialize(&s->rtc, sizeof(s->rtc), TYPE_MC146818_RTC); >> -} >> - >> static void piix4_class_init(ObjectClass *klass, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(klass); >> @@ -233,7 +221,6 @@ static const TypeInfo piix4_info = { >> .name = TYPE_PIIX4_PCI_DEVICE, >> .parent = TYPE_PCI_DEVICE, >> .instance_size = sizeof(PIIX4State), >> - .instance_init = piix4_init, >> .class_init = piix4_class_init, >> .interfaces = (InterfaceInfo[]) { >> { INTERFACE_CONVENTIONAL_PCI_DEVICE }, > > This looks like it's going backwards from the way we'd usually > write code for devices that contain other devices these days. > The "we have an init function that does stuff" is the older > style. The newer style has the inner-device as an embedded > struct in the container-device struct, which is initialized, > configured and realized using standard functions like object_initialize > and qdev_realize. (I do wonder whether that ought to be > object_initialize_child() here, incidentally, but haven't checked the > details to be certain.) > > The existing uses of mc146818_rtc_init() are mostly in older > code, and also in board-initialization code, which traditionally > didn't have a convenient struct to embed the device-struct into. > hw/isa/vt82c686.c is the only use in another device model > (which could in theory be refactored to the embed-the-device-struct > style, though the benefit of making the change isn't large, which > is one reason we still have the mix of both in the tree). I agree with Peter. The "future" is to remove the ${device}_init() helpers. Some contributors are helping toward that goal, but 1/ it involve work and 2/ there is no coordination; this is a "best effort" project maintenance. Maybe we should at least list these 'to be removed' functions as a bit-sized task in GitLab issues. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] mc146818rtc: Unexport RTCState 2022-02-05 17:59 [PATCH 0/2] Make mc146818rtc more self-contained Bernhard Beschow 2022-02-05 17:59 ` [PATCH 1/2] isa/piix4: Resolve RTCState attribute Bernhard Beschow @ 2022-02-05 17:59 ` Bernhard Beschow 1 sibling, 0 replies; 5+ messages in thread From: Bernhard Beschow @ 2022-02-05 17:59 UTC (permalink / raw) To: qemu-devel; +Cc: Paolo Bonzini, Bernhard Beschow, Michael S. Tsirkin Now that RTCState isn't used outside mc146818rtc.c any more, it can be unexported to prevent outside code to depend on its details. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/rtc/mc146818rtc.c | 34 ++++++++++++++++++++++++++++++++++ include/hw/rtc/mc146818rtc.h | 35 +---------------------------------- 2 files changed, 35 insertions(+), 34 deletions(-) diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c index e61a0cced4..e3980ac663 100644 --- a/hw/rtc/mc146818rtc.c +++ b/hw/rtc/mc146818rtc.c @@ -26,6 +26,7 @@ #include "qemu/cutils.h" #include "qemu/module.h" #include "qemu/bcd.h" +#include "qemu/queue.h" #include "hw/acpi/aml-build.h" #include "hw/irq.h" #include "hw/qdev-properties.h" @@ -41,7 +42,9 @@ #include "migration/vmstate.h" #include "qapi/error.h" #include "qapi/qapi-events-misc-target.h" +#include "qapi/qapi-types-machine.h" #include "qapi/visitor.h" +#include "qom/object.h" #include "hw/rtc/mc146818rtc_regs.h" #ifdef TARGET_I386 @@ -74,6 +77,37 @@ #define RTC_CLOCK_RATE 32768 #define UIP_HOLD_LENGTH (8 * NANOSECONDS_PER_SECOND / 32768) +OBJECT_DECLARE_SIMPLE_TYPE(RTCState, MC146818_RTC) + +struct RTCState { + ISADevice parent_obj; + + MemoryRegion io; + MemoryRegion coalesced_io; + uint8_t cmos_data[128]; + uint8_t cmos_index; + int32_t base_year; + uint64_t base_rtc; + uint64_t last_update; + int64_t offset; + qemu_irq irq; + int it_shift; + /* periodic timer */ + QEMUTimer *periodic_timer; + int64_t next_periodic_time; + /* update-ended timer */ + QEMUTimer *update_timer; + uint64_t next_alarm_time; + uint16_t irq_reinject_on_ack_count; + uint32_t irq_coalesced; + uint32_t period; + QEMUTimer *coalesced_timer; + Notifier clock_reset_notifier; + LostTickPolicy lost_tick_policy; + Notifier suspend_notifier; + QLIST_ENTRY(RTCState) link; +}; + static void rtc_set_time(RTCState *s); static void rtc_update_time(RTCState *s); static void rtc_set_cmos(RTCState *s, const struct tm *tm); diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h index 5b45b22924..0dc2cb2605 100644 --- a/include/hw/rtc/mc146818rtc.h +++ b/include/hw/rtc/mc146818rtc.h @@ -9,43 +9,10 @@ #ifndef HW_RTC_MC146818RTC_H #define HW_RTC_MC146818RTC_H -#include "qapi/qapi-types-machine.h" -#include "qemu/queue.h" -#include "qemu/timer.h" #include "hw/isa/isa.h" -#include "qom/object.h" +#include "qemu/typedefs.h" #define TYPE_MC146818_RTC "mc146818rtc" -OBJECT_DECLARE_SIMPLE_TYPE(RTCState, MC146818_RTC) - -struct RTCState { - ISADevice parent_obj; - - MemoryRegion io; - MemoryRegion coalesced_io; - uint8_t cmos_data[128]; - uint8_t cmos_index; - int32_t base_year; - uint64_t base_rtc; - uint64_t last_update; - int64_t offset; - qemu_irq irq; - int it_shift; - /* periodic timer */ - QEMUTimer *periodic_timer; - int64_t next_periodic_time; - /* update-ended timer */ - QEMUTimer *update_timer; - uint64_t next_alarm_time; - uint16_t irq_reinject_on_ack_count; - uint32_t irq_coalesced; - uint32_t period; - QEMUTimer *coalesced_timer; - Notifier clock_reset_notifier; - LostTickPolicy lost_tick_policy; - Notifier suspend_notifier; - QLIST_ENTRY(RTCState) link; -}; #define RTC_ISA_IRQ 8 #define RTC_ISA_BASE 0x70 -- 2.35.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-02-05 23:02 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-02-05 17:59 [PATCH 0/2] Make mc146818rtc more self-contained Bernhard Beschow 2022-02-05 17:59 ` [PATCH 1/2] isa/piix4: Resolve RTCState attribute Bernhard Beschow 2022-02-05 18:53 ` Peter Maydell 2022-02-05 22:59 ` Philippe Mathieu-Daudé via 2022-02-05 17:59 ` [PATCH 2/2] mc146818rtc: Unexport RTCState Bernhard Beschow
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).