qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [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

* [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

* 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

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).