[PATCH v2 0/3] QOM improvements for rtc/mc146818rtc

[PATCH v2 0/3] QOM improvements for rtc/mc146818rtc

[Bernhard Beschow]

v2:
* Explicitly fail with &error_abort rather than NULL (Mark)
* Explicitly fail with &error_abort rather than NULL in existing code (me)
* Unexport rather than remove RTC_ISA_BASE (Mark)
* Use object_property_get_*u*int() also for "iobase" (me)

v1:
This little series enhances QOM support for mc146818rtc:
* makes microvm-dt respect mc146818rtc's IRQ number set by QOM property and
* adds an io_base QOM property similar to other ISA devices

Bernhard Beschow (3):
  hw/i386/microvm-dt: Force explicit failure if retrieving QOM property
    fails
  hw/i386/microvm-dt: Determine mc146818rtc's IRQ number from QOM
    property
  rtc/mc146818rtc: QOM'ify io_base offset

 hw/i386/microvm-dt.c         | 9 +++++----
 hw/rtc/mc146818rtc.c         | 9 ++++++---
 include/hw/rtc/mc146818rtc.h | 2 +-
 3 files changed, 12 insertions(+), 8 deletions(-)

-- 
2.36.1

[PATCH v2 1/3] hw/i386/microvm-dt: Force explicit failure if retrieving QOM property fails

[Bernhard Beschow]

New code will be added where this is best practice. So update existing code
as well.
---
 hw/i386/microvm-dt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/i386/microvm-dt.c b/hw/i386/microvm-dt.c
index 9c3c4995b4..fde74819f2 100644
--- a/hw/i386/microvm-dt.c
+++ b/hw/i386/microvm-dt.c
@@ -32,6 +32,7 @@
  */
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
+#include "qapi/error.h"
 #include "sysemu/device_tree.h"
 #include "hw/char/serial.h"
 #include "hw/i386/fw_cfg.h"
@@ -187,8 +188,8 @@ static void dt_add_ioapic(MicrovmMachineState *mms, SysBusDevice *dev)
 static void dt_add_isa_serial(MicrovmMachineState *mms, ISADevice *dev)
 {
     const char compat[] = "ns16550";
-    uint32_t irq = object_property_get_int(OBJECT(dev), "irq", NULL);
-    hwaddr base = object_property_get_int(OBJECT(dev), "iobase", NULL);
+    uint32_t irq = object_property_get_int(OBJECT(dev), "irq", &error_fatal);
+    hwaddr base = object_property_get_int(OBJECT(dev), "iobase", &error_fatal);
     hwaddr size = 8;
     char *nodename;
 
-- 
2.36.1

[PATCH v2 2/3] hw/i386/microvm-dt: Determine mc146818rtc's IRQ number from QOM property

[Bernhard Beschow]

Since commit 3b004a16540aa41f2aa6a1ceb0bf306716766914 'hw/rtc/
mc146818rtc: QOM'ify IRQ number' mc146818rtc's IRQ number is
configurable. Fix microvm-dt to respect its value.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/i386/microvm-dt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/microvm-dt.c b/hw/i386/microvm-dt.c
index fde74819f2..287818c641 100644
--- a/hw/i386/microvm-dt.c
+++ b/hw/i386/microvm-dt.c
@@ -209,7 +209,7 @@ static void dt_add_isa_serial(MicrovmMachineState *mms, ISADevice *dev)
 static void dt_add_isa_rtc(MicrovmMachineState *mms, ISADevice *dev)
 {
     const char compat[] = "motorola,mc146818";
-    uint32_t irq = RTC_ISA_IRQ;
+    uint32_t irq = object_property_get_uint(OBJECT(dev), "irq", &error_fatal);
     hwaddr base = RTC_ISA_BASE;
     hwaddr size = 8;
     char *nodename;
-- 
2.36.1

[PATCH v2 3/3] rtc/mc146818rtc: QOM'ify io_base offset

[Bernhard Beschow]

Exposing the io_base offset as a QOM property not only allows it to be
configurable but also to be displayed in HMP:

Before:

(qemu) info qtree
       ...
          dev: mc146818rtc, id ""
            gpio-out "" 1
            base_year = 0 (0x0)
            irq = 8 (0x8)
            lost_tick_policy = "discard"

After:

          dev: mc146818rtc, id ""
            gpio-out "" 1
            base_year = 0 (0x0)
            iobase = 112 (0x70)
            irq = 8 (0x8)
            lost_tick_policy = "discard"

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/i386/microvm-dt.c         | 2 +-
 hw/rtc/mc146818rtc.c         | 9 ++++++---
 include/hw/rtc/mc146818rtc.h | 2 +-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/i386/microvm-dt.c b/hw/i386/microvm-dt.c
index 287818c641..b3049e4f9f 100644
--- a/hw/i386/microvm-dt.c
+++ b/hw/i386/microvm-dt.c
@@ -210,7 +210,7 @@ static void dt_add_isa_rtc(MicrovmMachineState *mms, ISADevice *dev)
 {
     const char compat[] = "motorola,mc146818";
     uint32_t irq = object_property_get_uint(OBJECT(dev), "irq", &error_fatal);
-    hwaddr base = RTC_ISA_BASE;
+    hwaddr base = object_property_get_uint(OBJECT(dev), "iobase", &error_fatal);
     hwaddr size = 8;
     char *nodename;
 
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index f235c2ddbe..5a5dd21a3c 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -74,6 +74,8 @@
 #define RTC_CLOCK_RATE            32768
 #define UIP_HOLD_LENGTH           (8 * NANOSECONDS_PER_SECOND / 32768)
 
+#define RTC_ISA_BASE 0x70
+
 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);
@@ -941,7 +943,7 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
     qemu_register_suspend_notifier(&s->suspend_notifier);
 
     memory_region_init_io(&s->io, OBJECT(s), &cmos_ops, s, "rtc", 2);
-    isa_register_ioport(isadev, &s->io, RTC_ISA_BASE);
+    isa_register_ioport(isadev, &s->io, s->io_base);
 
     /* register rtc 0x70 port for coalesced_pio */
     memory_region_set_flush_coalesced(&s->io);
@@ -950,7 +952,7 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
     memory_region_add_subregion(&s->io, 0, &s->coalesced_io);
     memory_region_add_coalescing(&s->coalesced_io, 0, 1);
 
-    qdev_set_legacy_instance_id(dev, RTC_ISA_BASE, 3);
+    qdev_set_legacy_instance_id(dev, s->io_base, 3);
 
     object_property_add_tm(OBJECT(s), "date", rtc_get_date);
 
@@ -983,6 +985,7 @@ ISADevice *mc146818_rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq)
 
 static Property mc146818rtc_properties[] = {
     DEFINE_PROP_INT32("base_year", RTCState, base_year, 1980),
+    DEFINE_PROP_UINT32("iobase", RTCState, io_base, RTC_ISA_BASE),
     DEFINE_PROP_UINT8("irq", RTCState, isairq, RTC_ISA_IRQ),
     DEFINE_PROP_LOSTTICKPOLICY("lost_tick_policy", RTCState,
                                lost_tick_policy, LOST_TICK_POLICY_DISCARD),
@@ -1028,7 +1031,7 @@ static void rtc_build_aml(ISADevice *isadev, Aml *scope)
      * does, even though qemu only responds to the first two ports.
      */
     crs = aml_resource_template();
-    aml_append(crs, aml_io(AML_DECODE16, RTC_ISA_BASE, RTC_ISA_BASE,
+    aml_append(crs, aml_io(AML_DECODE16, s->io_base, s->io_base,
                            0x01, 0x08));
     aml_append(crs, aml_irq_no_flags(s->isairq));
 
diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
index 33d85753c0..1f7942a9f8 100644
--- a/include/hw/rtc/mc146818rtc.h
+++ b/include/hw/rtc/mc146818rtc.h
@@ -26,6 +26,7 @@ struct RTCState {
     uint8_t cmos_data[128];
     uint8_t cmos_index;
     uint8_t isairq;
+    uint32_t io_base;
     int32_t base_year;
     uint64_t base_rtc;
     uint64_t last_update;
@@ -49,7 +50,6 @@ struct RTCState {
 };
 
 #define RTC_ISA_IRQ 8
-#define RTC_ISA_BASE 0x70
 
 ISADevice *mc146818_rtc_init(ISABus *bus, int base_year,
                              qemu_irq intercept_irq);
-- 
2.36.1

Re: [PATCH v2 0/3] QOM improvements for rtc/mc146818rtc

[Philippe Mathieu-Daudé via]

On 22/5/22 22:43, Bernhard Beschow wrote:
> v2:
> * Explicitly fail with &error_abort rather than NULL (Mark)
> * Explicitly fail with &error_abort rather than NULL in existing code (me)
> * Unexport rather than remove RTC_ISA_BASE (Mark)
> * Use object_property_get_*u*int() also for "iobase" (me)
> 
> v1:
> This little series enhances QOM support for mc146818rtc:
> * makes microvm-dt respect mc146818rtc's IRQ number set by QOM property and
> * adds an io_base QOM property similar to other ISA devices
> 
> Bernhard Beschow (3):
>    hw/i386/microvm-dt: Force explicit failure if retrieving QOM property
>      fails
>    hw/i386/microvm-dt: Determine mc146818rtc's IRQ number from QOM
>      property
>    rtc/mc146818rtc: QOM'ify io_base offset

In patch 3 "iobase" could be u16. Otherwise,

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>