All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] mc146818rtc related clean-ups and improvements
@ 2023-01-03  8:47 Thomas Huth
  2023-01-03  8:47 ` [PATCH 1/6] hw/i386/pc: Create RTC controllers in south bridges Thomas Huth
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Thomas Huth @ 2023-01-03  8:47 UTC (permalink / raw)
  To: Paolo Bonzini, Michael S Tsirkin, qemu-devel, Bernhard Beschow,
	Mark Cave-Ayland
  Cc: Philippe Mathieu-Daudé,
	BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

This patch series is a follow-up of my previous patch
"[PATCH v4] hw/rtc/mc146818rtc: Make this rtc device target independent".
It has now been split into multiple patches to ease the review, and some
further patches have been added on top.

The basic idea is to change hw/rtc/mc146818rtc.c into target independent
code so that the file only has to be compiled once instead of multiple
times (and that it can be used in a qemu-system-all binary once we get
there).

First two patches are from Bernhard - clean-ups that will help to
make it easier to introduce a new "slew-tick-policy-available" property
later.

The third patch extracts some functions from the APIC code that will be
required for linking when the mc146818rtc becomes target-independent.

The fourth patch introduces the new "slew-tick-policy-available" property
that can be used to decide whether the slew-tick policy is available or
not once the "#ifdef TARGET..." stuff got removed.

The fifth patch then removes the "#ifdef TARGET" switches and turns
the mc146818rtc code into a target-independent file.

The sixth patch just fixes a small cosmetic nit that I discovered along
the way: On systems without mc146818, the "-rtc driftfix=slew" simply
got ignored silently. We should at least emit a warning in this case.

Bernhard Beschow (2):
  hw/i386/pc: Create RTC controllers in south bridges
  hw/i386/pc: No need for rtc_state to be an out-parameter

Thomas Huth (4):
  hw/intc: Extract the IRQ counting functions into a separate file
  hw/rtc/mc146818rtc: Add a property for the availability of the slew
    tick policy
  hw/rtc/mc146818rtc: Make the mc146818 RTC device target independent
  softmmu/rtc: Emit warning when using driftfix=slew on systems without
    mc146818

 include/hw/i386/apic.h          |  2 --
 include/hw/i386/apic_internal.h |  1 -
 include/hw/i386/ich9.h          |  2 ++
 include/hw/i386/pc.h            |  2 +-
 include/hw/intc/kvm_irqcount.h  | 10 +++++++
 include/hw/rtc/mc146818rtc.h    |  2 ++
 include/hw/southbridge/piix.h   |  3 ++
 hw/i386/kvm/i8259.c             |  4 +--
 hw/i386/kvm/ioapic.c            |  4 +--
 hw/i386/pc.c                    | 16 +++++++++--
 hw/i386/pc_piix.c               | 11 +++++++-
 hw/i386/pc_q35.c                |  3 +-
 hw/intc/apic.c                  |  3 +-
 hw/intc/apic_common.c           | 30 ++------------------
 hw/intc/kvm_irqcount.c          | 49 +++++++++++++++++++++++++++++++++
 hw/isa/lpc_ich9.c               |  9 ++++++
 hw/isa/piix3.c                  | 16 +++++++++++
 hw/rtc/mc146818rtc.c            | 33 ++++++++--------------
 softmmu/rtc.c                   |  6 +++-
 hw/intc/meson.build             |  6 ++++
 hw/intc/trace-events            |  9 +++---
 hw/isa/Kconfig                  |  2 ++
 hw/rtc/meson.build              |  3 +-
 23 files changed, 156 insertions(+), 70 deletions(-)
 create mode 100644 include/hw/intc/kvm_irqcount.h
 create mode 100644 hw/intc/kvm_irqcount.c

-- 
2.31.1



^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 1/6] hw/i386/pc: Create RTC controllers in south bridges
  2023-01-03  8:47 [PATCH 0/6] mc146818rtc related clean-ups and improvements Thomas Huth
@ 2023-01-03  8:47 ` Thomas Huth
  2023-01-03  8:47 ` [PATCH 2/6] hw/i386/pc: No need for rtc_state to be an out-parameter Thomas Huth
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Thomas Huth @ 2023-01-03  8:47 UTC (permalink / raw)
  To: Paolo Bonzini, Michael S Tsirkin, qemu-devel, Bernhard Beschow,
	Mark Cave-Ayland
  Cc: Philippe Mathieu-Daudé,
	BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

From: Bernhard Beschow <shentey@gmail.com>

Just like in the real hardware (and in PIIX4), create the RTC
controllers in the south bridges.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-11-shentey@gmail.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/i386/ich9.h        |  2 ++
 include/hw/southbridge/piix.h |  3 +++
 hw/i386/pc.c                  | 12 +++++++++++-
 hw/i386/pc_piix.c             |  8 ++++++++
 hw/i386/pc_q35.c              |  1 +
 hw/isa/lpc_ich9.c             |  8 ++++++++
 hw/isa/piix3.c                | 15 +++++++++++++++
 hw/isa/Kconfig                |  2 ++
 8 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
index 23ee8e371b..672efc6bce 100644
--- a/include/hw/i386/ich9.h
+++ b/include/hw/i386/ich9.h
@@ -11,6 +11,7 @@
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/ich9.h"
 #include "hw/pci/pci_bus.h"
+#include "hw/rtc/mc146818rtc.h"
 #include "qom/object.h"
 
 void ich9_lpc_set_irq(void *opaque, int irq_num, int level);
@@ -39,6 +40,7 @@ struct ICH9LPCState {
     */
     uint8_t irr[PCI_SLOT_MAX][PCI_NUM_PINS];
 
+    RTCState rtc;
     APMState apm;
     ICH9LPCPMRegs pm;
     uint32_t sci_level; /* track sci level */
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 2693778b23..b1fa08dd2b 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -14,6 +14,7 @@
 
 #include "hw/pci/pci.h"
 #include "qom/object.h"
+#include "hw/rtc/mc146818rtc.h"
 
 /* PIRQRC[A:D]: PIRQx Route Control Registers */
 #define PIIX_PIRQCA 0x60
@@ -52,6 +53,8 @@ struct PIIXState {
     /* This member isn't used. Just for save/load compatibility */
     int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
 
+    RTCState rtc;
+
     /* Reset Control Register contents */
     uint8_t rcr;
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d489ecc0d1..448557333b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1304,7 +1304,17 @@ void pc_basic_device_init(struct PCMachineState *pcms,
         pit_alt_irq = qdev_get_gpio_in(hpet, HPET_LEGACY_PIT_INT);
         rtc_irq = qdev_get_gpio_in(hpet, HPET_LEGACY_RTC_INT);
     }
-    *rtc_state = mc146818_rtc_init(isa_bus, 2000, rtc_irq);
+
+    if (rtc_irq) {
+        qdev_connect_gpio_out(DEVICE(*rtc_state), 0, rtc_irq);
+    } else {
+        uint32_t irq = object_property_get_uint(OBJECT(*rtc_state),
+                                                "irq",
+                                                &error_fatal);
+        isa_connect_gpio_out(*rtc_state, 0, irq);
+    }
+    object_property_add_alias(OBJECT(pcms), "rtc-time", OBJECT(*rtc_state),
+                              "date");
 
     qemu_register_boot_set(pc_boot_set, *rtc_state);
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b48047f50c..87aab3d853 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -32,6 +32,7 @@
 #include "hw/i386/pc.h"
 #include "hw/i386/apic.h"
 #include "hw/pci-host/i440fx.h"
+#include "hw/rtc/mc146818rtc.h"
 #include "hw/southbridge/piix.h"
 #include "hw/display/ramfb.h"
 #include "hw/firmware/smbios.h"
@@ -223,10 +224,17 @@ static void pc_init1(MachineState *machine,
         piix3->pic = x86ms->gsi;
         piix3_devfn = piix3->dev.devfn;
         isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
+        rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
+                                                             "rtc"));
     } else {
         pci_bus = NULL;
         isa_bus = isa_bus_new(NULL, get_system_memory(), system_io,
                               &error_abort);
+
+        rtc_state = isa_new(TYPE_MC146818_RTC);
+        qdev_prop_set_int32(DEVICE(rtc_state), "base_year", 2000);
+        isa_realize_and_unref(rtc_state, isa_bus, &error_fatal);
+
         i8257_dma_init(isa_bus, 0);
         pcms->hpet_enabled = false;
     }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 67ceb04bcc..6a36e6bb12 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -238,6 +238,7 @@ static void pc_q35_init(MachineState *machine)
     lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
                                           ICH9_LPC_FUNC), true,
                                           TYPE_ICH9_LPC_DEVICE);
+    rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
 
     object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
                              TYPE_HOTPLUG_HANDLER,
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 8d541e2b54..498175c1cc 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -663,6 +663,8 @@ static void ich9_lpc_initfn(Object *obj)
     static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
     static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
 
+    object_initialize_child(obj, "rtc", &lpc->rtc, TYPE_MC146818_RTC);
+
     object_property_add_uint8_ptr(obj, ACPI_PM_PROP_SCI_INT,
                                   &lpc->sci_gsi, OBJ_PROP_FLAG_READ);
     object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
@@ -728,6 +730,12 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
     isa_bus_irqs(isa_bus, lpc->gsi);
 
     i8257_dma_init(isa_bus, 0);
+
+    /* RTC */
+    qdev_prop_set_int32(DEVICE(&lpc->rtc), "base_year", 2000);
+    if (!qdev_realize(DEVICE(&lpc->rtc), BUS(isa_bus), errp)) {
+        return;
+    }
 }
 
 static bool ich9_rst_cnt_needed(void *opaque)
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index eabad7ba58..c68e51ddad 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -28,6 +28,7 @@
 #include "hw/dma/i8257.h"
 #include "hw/southbridge/piix.h"
 #include "hw/irq.h"
+#include "hw/qdev-properties.h"
 #include "hw/isa/isa.h"
 #include "hw/xen/xen.h"
 #include "sysemu/runstate.h"
@@ -312,6 +313,12 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
                                         PIIX_RCR_IOPORT, &d->rcr_mem, 1);
 
     i8257_dma_init(isa_bus, 0);
+
+    /* RTC */
+    qdev_prop_set_int32(DEVICE(&d->rtc), "base_year", 2000);
+    if (!qdev_realize(DEVICE(&d->rtc), BUS(isa_bus), errp)) {
+        return;
+    }
 }
 
 static void build_pci_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
@@ -338,6 +345,13 @@ static void build_pci_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
     }
 }
 
+static void pci_piix3_init(Object *obj)
+{
+    PIIX3State *d = PIIX3_PCI_DEVICE(obj);
+
+    object_initialize_child(obj, "rtc", &d->rtc, TYPE_MC146818_RTC);
+}
+
 static void pci_piix3_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -364,6 +378,7 @@ static const TypeInfo piix3_pci_type_info = {
     .name = TYPE_PIIX3_PCI_DEVICE,
     .parent = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PIIX3State),
+    .instance_init = pci_piix3_init,
     .abstract = true,
     .class_init = pci_piix3_class_init,
     .interfaces = (InterfaceInfo[]) {
diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index 18b5c6bf3f..af5ec9cd61 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -35,6 +35,7 @@ config PIIX3
     bool
     select I8257
     select ISA_BUS
+    select MC146818RTC
 
 config PIIX4
     bool
@@ -79,3 +80,4 @@ config LPC_ICH9
     select ISA_BUS
     select ACPI_SMBUS
     select ACPI_X86_ICH
+    select MC146818RTC
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/6] hw/i386/pc: No need for rtc_state to be an out-parameter
  2023-01-03  8:47 [PATCH 0/6] mc146818rtc related clean-ups and improvements Thomas Huth
  2023-01-03  8:47 ` [PATCH 1/6] hw/i386/pc: Create RTC controllers in south bridges Thomas Huth
@ 2023-01-03  8:47 ` Thomas Huth
  2023-01-03 13:11   ` Philippe Mathieu-Daudé
  2023-01-03  8:47 ` [PATCH 3/6] hw/intc: Extract the IRQ counting functions into a separate file Thomas Huth
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Thomas Huth @ 2023-01-03  8:47 UTC (permalink / raw)
  To: Paolo Bonzini, Michael S Tsirkin, qemu-devel, Bernhard Beschow,
	Mark Cave-Ayland
  Cc: Philippe Mathieu-Daudé,
	BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

From: Bernhard Beschow <shentey@gmail.com>

Now that the RTC is created as part of the southbridges it doesn't need
to be an out-parameter any longer.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-12-shentey@gmail.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/i386/pc.h |  2 +-
 hw/i386/pc.c         | 12 ++++++------
 hw/i386/pc_piix.c    |  2 +-
 hw/i386/pc_q35.c     |  2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 991f905f5d..dd059e8667 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -169,7 +169,7 @@ uint64_t pc_pci_hole64_start(void);
 DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
 void pc_basic_device_init(struct PCMachineState *pcms,
                           ISABus *isa_bus, qemu_irq *gsi,
-                          ISADevice **rtc_state,
+                          ISADevice *rtc_state,
                           bool create_fdctrl,
                           uint32_t hpet_irqs);
 void pc_cmos_init(PCMachineState *pcms,
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 448557333b..53a5443e09 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1251,7 +1251,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
 
 void pc_basic_device_init(struct PCMachineState *pcms,
                           ISABus *isa_bus, qemu_irq *gsi,
-                          ISADevice **rtc_state,
+                          ISADevice *rtc_state,
                           bool create_fdctrl,
                           uint32_t hpet_irqs)
 {
@@ -1306,17 +1306,17 @@ void pc_basic_device_init(struct PCMachineState *pcms,
     }
 
     if (rtc_irq) {
-        qdev_connect_gpio_out(DEVICE(*rtc_state), 0, rtc_irq);
+        qdev_connect_gpio_out(DEVICE(rtc_state), 0, rtc_irq);
     } else {
-        uint32_t irq = object_property_get_uint(OBJECT(*rtc_state),
+        uint32_t irq = object_property_get_uint(OBJECT(rtc_state),
                                                 "irq",
                                                 &error_fatal);
-        isa_connect_gpio_out(*rtc_state, 0, irq);
+        isa_connect_gpio_out(rtc_state, 0, irq);
     }
-    object_property_add_alias(OBJECT(pcms), "rtc-time", OBJECT(*rtc_state),
+    object_property_add_alias(OBJECT(pcms), "rtc-time", OBJECT(rtc_state),
                               "date");
 
-    qemu_register_boot_set(pc_boot_set, *rtc_state);
+    qemu_register_boot_set(pc_boot_set, rtc_state);
 
     if (!xen_enabled() &&
         (x86ms->pit == ON_OFF_AUTO_AUTO || x86ms->pit == ON_OFF_AUTO_ON)) {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 87aab3d853..bc9ea8cdae 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -260,7 +260,7 @@ static void pc_init1(MachineState *machine,
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, &rtc_state, true,
+    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, true,
                          0x4);
 
     pc_nic_init(pcmc, isa_bus, pci_bus);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 6a36e6bb12..a54dd7be14 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -292,7 +292,7 @@ static void pc_q35_init(MachineState *machine)
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, &rtc_state, !mc->no_floppy,
+    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, !mc->no_floppy,
                          0xff0104);
 
     /* connect pm stuff to lpc */
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 3/6] hw/intc: Extract the IRQ counting functions into a separate file
  2023-01-03  8:47 [PATCH 0/6] mc146818rtc related clean-ups and improvements Thomas Huth
  2023-01-03  8:47 ` [PATCH 1/6] hw/i386/pc: Create RTC controllers in south bridges Thomas Huth
  2023-01-03  8:47 ` [PATCH 2/6] hw/i386/pc: No need for rtc_state to be an out-parameter Thomas Huth
@ 2023-01-03  8:47 ` Thomas Huth
  2023-01-03 12:55   ` Bernhard Beschow
  2023-01-03  8:47 ` [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy Thomas Huth
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Thomas Huth @ 2023-01-03  8:47 UTC (permalink / raw)
  To: Paolo Bonzini, Michael S Tsirkin, qemu-devel, Bernhard Beschow,
	Mark Cave-Ayland
  Cc: Philippe Mathieu-Daudé,
	BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

These IRQ counting functions will soon be required in binaries that
do not include the APIC code, too, so let's extract them into a
separate file that can be linked independently of the APIC code.

While we're at it, change the apic_* prefix into kvm_* since the
functions are used from the i8259 PIC (i.e. not the APIC), too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/i386/apic.h          |  2 --
 include/hw/i386/apic_internal.h |  1 -
 include/hw/intc/kvm_irqcount.h  | 10 +++++++
 hw/i386/kvm/i8259.c             |  4 +--
 hw/i386/kvm/ioapic.c            |  4 +--
 hw/intc/apic.c                  |  3 +-
 hw/intc/apic_common.c           | 30 ++------------------
 hw/intc/kvm_irqcount.c          | 49 +++++++++++++++++++++++++++++++++
 hw/rtc/mc146818rtc.c            |  6 ++--
 hw/intc/meson.build             |  6 ++++
 hw/intc/trace-events            |  9 +++---
 11 files changed, 81 insertions(+), 43 deletions(-)
 create mode 100644 include/hw/intc/kvm_irqcount.h
 create mode 100644 hw/intc/kvm_irqcount.c

diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
index da1d2fe155..bdc15a7a73 100644
--- a/include/hw/i386/apic.h
+++ b/include/hw/i386/apic.h
@@ -9,8 +9,6 @@ int apic_accept_pic_intr(DeviceState *s);
 void apic_deliver_pic_intr(DeviceState *s, int level);
 void apic_deliver_nmi(DeviceState *d);
 int apic_get_interrupt(DeviceState *s);
-void apic_reset_irq_delivered(void);
-int apic_get_irq_delivered(void);
 void cpu_set_apic_base(DeviceState *s, uint64_t val);
 uint64_t cpu_get_apic_base(DeviceState *s);
 void cpu_set_apic_tpr(DeviceState *s, uint8_t val);
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index c175e7e718..e61ad04769 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -199,7 +199,6 @@ typedef struct VAPICState {
 
 extern bool apic_report_tpr_access;
 
-void apic_report_irq_delivered(int delivered);
 bool apic_next_timer(APICCommonState *s, int64_t current_time);
 void apic_enable_tpr_access_reporting(DeviceState *d, bool enable);
 void apic_enable_vapic(DeviceState *d, hwaddr paddr);
diff --git a/include/hw/intc/kvm_irqcount.h b/include/hw/intc/kvm_irqcount.h
new file mode 100644
index 0000000000..0ed5999e49
--- /dev/null
+++ b/include/hw/intc/kvm_irqcount.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+
+#ifndef KVM_IRQCOUNT_H
+#define KVM_IRQCOUNT_H
+
+void kvm_report_irq_delivered(int delivered);
+void kvm_reset_irq_delivered(void);
+int kvm_get_irq_delivered(void);
+
+#endif
diff --git a/hw/i386/kvm/i8259.c b/hw/i386/kvm/i8259.c
index d61bae4dc3..3ca0e1ff03 100644
--- a/hw/i386/kvm/i8259.c
+++ b/hw/i386/kvm/i8259.c
@@ -14,7 +14,7 @@
 #include "hw/isa/i8259_internal.h"
 #include "hw/intc/i8259.h"
 #include "qemu/module.h"
-#include "hw/i386/apic_internal.h"
+#include "hw/intc/kvm_irqcount.h"
 #include "hw/irq.h"
 #include "sysemu/kvm.h"
 #include "qom/object.h"
@@ -117,7 +117,7 @@ static void kvm_pic_set_irq(void *opaque, int irq, int level)
 
     pic_stat_update_irq(irq, level);
     delivered = kvm_set_irq(kvm_state, irq, level);
-    apic_report_irq_delivered(delivered);
+    kvm_report_irq_delivered(delivered);
 }
 
 static void kvm_pic_realize(DeviceState *dev, Error **errp)
diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
index ee7c8ef68b..272e26b4a2 100644
--- a/hw/i386/kvm/ioapic.c
+++ b/hw/i386/kvm/ioapic.c
@@ -15,7 +15,7 @@
 #include "hw/i386/x86.h"
 #include "hw/qdev-properties.h"
 #include "hw/i386/ioapic_internal.h"
-#include "hw/i386/apic_internal.h"
+#include "hw/intc/kvm_irqcount.h"
 #include "sysemu/kvm.h"
 
 /* PC Utility function */
@@ -116,7 +116,7 @@ static void kvm_ioapic_set_irq(void *opaque, int irq, int level)
 
     ioapic_stat_update_irq(common, irq, level);
     delivered = kvm_set_irq(kvm_state, s->kvm_gsi_base + irq, level);
-    apic_report_irq_delivered(delivered);
+    kvm_report_irq_delivered(delivered);
 }
 
 static void kvm_ioapic_realize(DeviceState *dev, Error **errp)
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 3df11c34d6..2d3e55f4e2 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -22,6 +22,7 @@
 #include "hw/i386/apic.h"
 #include "hw/i386/ioapic.h"
 #include "hw/intc/i8259.h"
+#include "hw/intc/kvm_irqcount.h"
 #include "hw/pci/msi.h"
 #include "qemu/host-utils.h"
 #include "sysemu/kvm.h"
@@ -399,7 +400,7 @@ void apic_poll_irq(DeviceState *dev)
 
 static void apic_set_irq(APICCommonState *s, int vector_num, int trigger_mode)
 {
-    apic_report_irq_delivered(!apic_get_bit(s->irr, vector_num));
+    kvm_report_irq_delivered(!apic_get_bit(s->irr, vector_num));
 
     apic_set_bit(s->irr, vector_num);
     if (trigger_mode)
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 2a20982066..4a34f03047 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -25,6 +25,7 @@
 #include "qapi/visitor.h"
 #include "hw/i386/apic.h"
 #include "hw/i386/apic_internal.h"
+#include "hw/intc/kvm_irqcount.h"
 #include "trace.h"
 #include "hw/boards.h"
 #include "sysemu/hax.h"
@@ -33,7 +34,6 @@
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
 
-static int apic_irq_delivered;
 bool apic_report_tpr_access;
 
 void cpu_set_apic_base(DeviceState *dev, uint64_t val)
@@ -122,32 +122,6 @@ void apic_handle_tpr_access_report(DeviceState *dev, target_ulong ip,
     vapic_report_tpr_access(s->vapic, CPU(s->cpu), ip, access);
 }
 
-void apic_report_irq_delivered(int delivered)
-{
-    apic_irq_delivered += delivered;
-
-    trace_apic_report_irq_delivered(apic_irq_delivered);
-}
-
-void apic_reset_irq_delivered(void)
-{
-    /* Copy this into a local variable to encourage gcc to emit a plain
-     * register for a sys/sdt.h marker.  For details on this workaround, see:
-     * https://sourceware.org/bugzilla/show_bug.cgi?id=13296
-     */
-    volatile int a_i_d = apic_irq_delivered;
-    trace_apic_reset_irq_delivered(a_i_d);
-
-    apic_irq_delivered = 0;
-}
-
-int apic_get_irq_delivered(void)
-{
-    trace_apic_get_irq_delivered(apic_irq_delivered);
-
-    return apic_irq_delivered;
-}
-
 void apic_deliver_nmi(DeviceState *dev)
 {
     APICCommonState *s = APIC_COMMON(dev);
@@ -272,7 +246,7 @@ static void apic_reset_common(DeviceState *dev)
     s->apicbase = APIC_DEFAULT_ADDRESS | bsp | MSR_IA32_APICBASE_ENABLE;
     s->id = s->initial_apic_id;
 
-    apic_reset_irq_delivered();
+    kvm_reset_irq_delivered();
 
     s->vapic_paddr = 0;
     info->vapic_base_update(s);
diff --git a/hw/intc/kvm_irqcount.c b/hw/intc/kvm_irqcount.c
new file mode 100644
index 0000000000..2ef8a83a7a
--- /dev/null
+++ b/hw/intc/kvm_irqcount.c
@@ -0,0 +1,49 @@
+/*
+ * KVM PIC functions for counting the delivered IRQs.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#include "qemu/osdep.h"
+#include "hw/intc/kvm_irqcount.h"
+#include "trace.h"
+
+static int kvm_irq_delivered;
+
+void kvm_report_irq_delivered(int delivered)
+{
+    kvm_irq_delivered += delivered;
+
+    trace_kvm_report_irq_delivered(kvm_irq_delivered);
+}
+
+void kvm_reset_irq_delivered(void)
+{
+    /*
+     * Copy this into a local variable to encourage gcc to emit a plain
+     * register for a sys/sdt.h marker.  For details on this workaround, see:
+     * https://sourceware.org/bugzilla/show_bug.cgi?id=13296
+     */
+    volatile int k_i_d = kvm_irq_delivered;
+    trace_kvm_reset_irq_delivered(k_i_d);
+
+    kvm_irq_delivered = 0;
+}
+
+int kvm_get_irq_delivered(void)
+{
+    trace_kvm_get_irq_delivered(kvm_irq_delivered);
+
+    return kvm_irq_delivered;
+}
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index 1ebb412479..947d68c257 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -27,6 +27,7 @@
 #include "qemu/module.h"
 #include "qemu/bcd.h"
 #include "hw/acpi/acpi_aml_interface.h"
+#include "hw/intc/kvm_irqcount.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "hw/qdev-properties-system.h"
@@ -46,7 +47,6 @@
 
 #ifdef TARGET_I386
 #include "qapi/qapi-commands-misc-target.h"
-#include "hw/i386/apic.h"
 #endif
 
 //#define DEBUG_CMOS
@@ -124,9 +124,9 @@ void qmp_rtc_reset_reinjection(Error **errp)
 
 static bool rtc_policy_slew_deliver_irq(RTCState *s)
 {
-    apic_reset_irq_delivered();
+    kvm_reset_irq_delivered();
     qemu_irq_raise(s->irq);
-    return apic_get_irq_delivered();
+    return kvm_get_irq_delivered();
 }
 
 static void rtc_coalesced_timer(void *opaque)
diff --git a/hw/intc/meson.build b/hw/intc/meson.build
index bcbf22ff51..cd9f1ee888 100644
--- a/hw/intc/meson.build
+++ b/hw/intc/meson.build
@@ -25,6 +25,12 @@ softmmu_ss.add(when: 'CONFIG_XILINX', if_true: files('xilinx_intc.c'))
 softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP', if_true: files('xlnx-zynqmp-ipi.c'))
 softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP_PMU', if_true: files('xlnx-pmu-iomod-intc.c'))
 
+if config_all_devices.has_key('CONFIG_APIC') or \
+   config_all_devices.has_key('CONFIG_I8259') or \
+   config_all_devices.has_key('CONFIG_MC146818RTC')
+    softmmu_ss.add(files('kvm_irqcount.c'))
+endif
+
 specific_ss.add(when: 'CONFIG_ALLWINNER_A10_PIC', if_true: files('allwinner-a10-pic.c'))
 specific_ss.add(when: 'CONFIG_APIC', if_true: files('apic.c', 'apic_common.c'))
 specific_ss.add(when: 'CONFIG_ARM_GIC', if_true: files('arm_gicv3_cpuif_common.c'))
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index 6fbc2045e6..50cadfb996 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -10,10 +10,6 @@ pic_ioport_read(bool master, uint64_t addr, int val) "master %d addr 0x%"PRIx64"
 # apic_common.c
 cpu_set_apic_base(uint64_t val) "0x%016"PRIx64
 cpu_get_apic_base(uint64_t val) "0x%016"PRIx64
-# coalescing
-apic_report_irq_delivered(int apic_irq_delivered) "coalescing %d"
-apic_reset_irq_delivered(int apic_irq_delivered) "old coalescing %d"
-apic_get_irq_delivered(int apic_irq_delivered) "returning coalescing %d"
 
 # apic.c
 apic_local_deliver(int vector, uint32_t lvt) "vector %d delivery mode %d"
@@ -30,6 +26,11 @@ ioapic_mem_read(uint8_t addr, uint8_t regsel, uint8_t size, uint32_t val) "ioapi
 ioapic_mem_write(uint8_t addr, uint8_t regsel, uint8_t size, uint32_t val) "ioapic mem write addr 0x%"PRIx8" regsel: 0x%"PRIx8" size 0x%"PRIx8" val 0x%"PRIx32
 ioapic_set_irq(int vector, int level) "vector: %d level: %d"
 
+# kvm_irqcount.c
+kvm_report_irq_delivered(int irq_delivered) "coalescing %d"
+kvm_reset_irq_delivered(int irq_delivered) "old coalescing %d"
+kvm_get_irq_delivered(int irq_delivered) "returning coalescing %d"
+
 # slavio_intctl.c
 slavio_intctl_mem_readl(uint32_t cpu, uint64_t addr, uint32_t ret) "read cpu %d reg 0x%"PRIx64" = 0x%x"
 slavio_intctl_mem_writel(uint32_t cpu, uint64_t addr, uint32_t val) "write cpu %d reg 0x%"PRIx64" = 0x%x"
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy
  2023-01-03  8:47 [PATCH 0/6] mc146818rtc related clean-ups and improvements Thomas Huth
                   ` (2 preceding siblings ...)
  2023-01-03  8:47 ` [PATCH 3/6] hw/intc: Extract the IRQ counting functions into a separate file Thomas Huth
@ 2023-01-03  8:47 ` Thomas Huth
  2023-01-03 13:10   ` Philippe Mathieu-Daudé
                     ` (3 more replies)
  2023-01-03  8:48 ` [PATCH 5/6] hw/rtc/mc146818rtc: Make the mc146818 RTC device target independent Thomas Huth
  2023-01-03  8:48 ` [PATCH 6/6] softmmu/rtc: Emit warning when using driftfix=slew on systems without mc146818 Thomas Huth
  5 siblings, 4 replies; 19+ messages in thread
From: Thomas Huth @ 2023-01-03  8:47 UTC (permalink / raw)
  To: Paolo Bonzini, Michael S Tsirkin, qemu-devel, Bernhard Beschow,
	Mark Cave-Ayland
  Cc: Philippe Mathieu-Daudé,
	BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

We want to get rid of the "#ifdef TARGET_I386" statements in the mc146818
code, so we need a different way to decide whether the slew tick policy
is available or not. Introduce a new property "slew-tick-policy-available"
which can be set by the machines that support this tick policy.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/rtc/mc146818rtc.h |  1 +
 hw/i386/pc_piix.c            |  1 +
 hw/isa/lpc_ich9.c            |  1 +
 hw/isa/piix3.c               |  1 +
 hw/rtc/mc146818rtc.c         | 16 ++++++++++------
 5 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
index 1db0fcee92..54af63d091 100644
--- a/include/hw/rtc/mc146818rtc.h
+++ b/include/hw/rtc/mc146818rtc.h
@@ -45,6 +45,7 @@ struct RTCState {
     QEMUTimer *coalesced_timer;
     Notifier clock_reset_notifier;
     LostTickPolicy lost_tick_policy;
+    bool slew_tick_policy_available;
     Notifier suspend_notifier;
     QLIST_ENTRY(RTCState) link;
 };
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index bc9ea8cdae..382c6add3b 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -233,6 +233,7 @@ static void pc_init1(MachineState *machine,
 
         rtc_state = isa_new(TYPE_MC146818_RTC);
         qdev_prop_set_int32(DEVICE(rtc_state), "base_year", 2000);
+        qdev_prop_set_bit(DEVICE(rtc_state), "slew-tick-policy-available", true);
         isa_realize_and_unref(rtc_state, isa_bus, &error_fatal);
 
         i8257_dma_init(isa_bus, 0);
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 498175c1cc..aeab4d8549 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -733,6 +733,7 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
 
     /* RTC */
     qdev_prop_set_int32(DEVICE(&lpc->rtc), "base_year", 2000);
+    qdev_prop_set_bit(DEVICE(&lpc->rtc), "slew-tick-policy-available", true);
     if (!qdev_realize(DEVICE(&lpc->rtc), BUS(isa_bus), errp)) {
         return;
     }
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index c68e51ddad..825b1cbee2 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -316,6 +316,7 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
 
     /* RTC */
     qdev_prop_set_int32(DEVICE(&d->rtc), "base_year", 2000);
+    qdev_prop_set_bit(DEVICE(&d->rtc), "slew-tick-policy-available", true);
     if (!qdev_realize(DEVICE(&d->rtc), BUS(isa_bus), errp)) {
         return;
     }
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index 947d68c257..86381a74c3 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -922,14 +922,16 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
     rtc_set_date_from_host(isadev);
 
     switch (s->lost_tick_policy) {
-#ifdef TARGET_I386
-    case LOST_TICK_POLICY_SLEW:
-        s->coalesced_timer =
-            timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
-        break;
-#endif
     case LOST_TICK_POLICY_DISCARD:
         break;
+    case LOST_TICK_POLICY_SLEW:
+#ifdef TARGET_I386
+        if (s->slew_tick_policy_available) {
+            s->coalesced_timer = timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
+            break;
+        }
+#endif
+        /* fallthrough */
     default:
         error_setg(errp, "Invalid lost tick policy.");
         return;
@@ -989,6 +991,8 @@ static Property mc146818rtc_properties[] = {
     DEFINE_PROP_UINT8("irq", RTCState, isairq, RTC_ISA_IRQ),
     DEFINE_PROP_LOSTTICKPOLICY("lost_tick_policy", RTCState,
                                lost_tick_policy, LOST_TICK_POLICY_DISCARD),
+    DEFINE_PROP_BOOL("slew-tick-policy-available", RTCState,
+                     slew_tick_policy_available, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 5/6] hw/rtc/mc146818rtc: Make the mc146818 RTC device target independent
  2023-01-03  8:47 [PATCH 0/6] mc146818rtc related clean-ups and improvements Thomas Huth
                   ` (3 preceding siblings ...)
  2023-01-03  8:47 ` [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy Thomas Huth
@ 2023-01-03  8:48 ` Thomas Huth
  2023-01-03 12:58   ` Bernhard Beschow
  2023-01-03  8:48 ` [PATCH 6/6] softmmu/rtc: Emit warning when using driftfix=slew on systems without mc146818 Thomas Huth
  5 siblings, 1 reply; 19+ messages in thread
From: Thomas Huth @ 2023-01-03  8:48 UTC (permalink / raw)
  To: Paolo Bonzini, Michael S Tsirkin, qemu-devel, Bernhard Beschow,
	Mark Cave-Ayland
  Cc: Philippe Mathieu-Daudé,
	BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

The only reason for this code being target dependent was the IRQ-counting
related code in rtc_policy_slew_deliver_irq(). Since these functions have
been moved into a new, separate file (kvm_irqcount.c) which is now always
compiled and linked if either APIC or the mc146818 device are required,
and since we've got a new mechanism for deciding whether the slew tick
policy is available now (via the "slew-tick-policy-available" property),
we can get rid of the #ifdef TARGET_I386 switches in mc146818rtc.c and
declare it in the softmmu_ss instead of specific_ss, so that the code only
gets compiled once for all targets.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/rtc/mc146818rtc.h |  1 +
 hw/rtc/mc146818rtc.c         | 15 +--------------
 hw/rtc/meson.build           |  3 +--
 3 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
index 54af63d091..9d15d70da8 100644
--- a/include/hw/rtc/mc146818rtc.h
+++ b/include/hw/rtc/mc146818rtc.h
@@ -56,5 +56,6 @@ ISADevice *mc146818_rtc_init(ISABus *bus, int base_year,
                              qemu_irq intercept_irq);
 void rtc_set_memory(ISADevice *dev, int addr, int val);
 int rtc_get_memory(ISADevice *dev, int addr);
+void qmp_rtc_reset_reinjection(Error **errp);
 
 #endif /* HW_RTC_MC146818RTC_H */
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index 86381a74c3..4497ddf024 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -45,10 +45,6 @@
 #include "qapi/visitor.h"
 #include "hw/rtc/mc146818rtc_regs.h"
 
-#ifdef TARGET_I386
-#include "qapi/qapi-commands-misc-target.h"
-#endif
-
 //#define DEBUG_CMOS
 //#define DEBUG_COALESCED
 
@@ -112,7 +108,6 @@ static void rtc_coalesced_timer_update(RTCState *s)
 static QLIST_HEAD(, RTCState) rtc_devices =
     QLIST_HEAD_INITIALIZER(rtc_devices);
 
-#ifdef TARGET_I386
 void qmp_rtc_reset_reinjection(Error **errp)
 {
     RTCState *s;
@@ -124,6 +119,7 @@ void qmp_rtc_reset_reinjection(Error **errp)
 
 static bool rtc_policy_slew_deliver_irq(RTCState *s)
 {
+    assert(s->slew_tick_policy_available);
     kvm_reset_irq_delivered();
     qemu_irq_raise(s->irq);
     return kvm_get_irq_delivered();
@@ -145,13 +141,6 @@ static void rtc_coalesced_timer(void *opaque)
 
     rtc_coalesced_timer_update(s);
 }
-#else
-static bool rtc_policy_slew_deliver_irq(RTCState *s)
-{
-    assert(0);
-    return false;
-}
-#endif
 
 static uint32_t rtc_periodic_clock_ticks(RTCState *s)
 {
@@ -925,12 +914,10 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
     case LOST_TICK_POLICY_DISCARD:
         break;
     case LOST_TICK_POLICY_SLEW:
-#ifdef TARGET_I386
         if (s->slew_tick_policy_available) {
             s->coalesced_timer = timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
             break;
         }
-#endif
         /* fallthrough */
     default:
         error_setg(errp, "Invalid lost tick policy.");
diff --git a/hw/rtc/meson.build b/hw/rtc/meson.build
index dc33973384..34a4d316fa 100644
--- a/hw/rtc/meson.build
+++ b/hw/rtc/meson.build
@@ -13,5 +13,4 @@ softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_rtc.c'))
 softmmu_ss.add(when: 'CONFIG_GOLDFISH_RTC', if_true: files('goldfish_rtc.c'))
 softmmu_ss.add(when: 'CONFIG_LS7A_RTC', if_true: files('ls7a_rtc.c'))
 softmmu_ss.add(when: 'CONFIG_ALLWINNER_H3', if_true: files('allwinner-rtc.c'))
-
-specific_ss.add(when: 'CONFIG_MC146818RTC', if_true: files('mc146818rtc.c'))
+softmmu_ss.add(when: 'CONFIG_MC146818RTC', if_true: files('mc146818rtc.c'))
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 6/6] softmmu/rtc: Emit warning when using driftfix=slew on systems without mc146818
  2023-01-03  8:47 [PATCH 0/6] mc146818rtc related clean-ups and improvements Thomas Huth
                   ` (4 preceding siblings ...)
  2023-01-03  8:48 ` [PATCH 5/6] hw/rtc/mc146818rtc: Make the mc146818 RTC device target independent Thomas Huth
@ 2023-01-03  8:48 ` Thomas Huth
  2023-01-03 13:08   ` Philippe Mathieu-Daudé
  5 siblings, 1 reply; 19+ messages in thread
From: Thomas Huth @ 2023-01-03  8:48 UTC (permalink / raw)
  To: Paolo Bonzini, Michael S Tsirkin, qemu-devel, Bernhard Beschow,
	Mark Cave-Ayland
  Cc: Philippe Mathieu-Daudé,
	BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

The 'slew' lost tick policy is only available on systems with a mc146818
RTC. On other systems, "-rtc driftfix=slew" is currently silently ignored.
Let's emit at least a warning in this case to make the users aware that
there is something wrong in their command line settings.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 softmmu/rtc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/softmmu/rtc.c b/softmmu/rtc.c
index 7e2956f81e..f7114bed7d 100644
--- a/softmmu/rtc.c
+++ b/softmmu/rtc.c
@@ -33,6 +33,7 @@
 #include "sysemu/replay.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/rtc.h"
+#include "hw/rtc/mc146818rtc.h"
 
 static enum {
     RTC_BASE_UTC,
@@ -177,10 +178,13 @@ void configure_rtc(QemuOpts *opts)
     value = qemu_opt_get(opts, "driftfix");
     if (value) {
         if (!strcmp(value, "slew")) {
-            object_register_sugar_prop("mc146818rtc",
+            object_register_sugar_prop(TYPE_MC146818_RTC,
                                        "lost_tick_policy",
                                        "slew",
                                        false);
+            if (!object_class_by_name(TYPE_MC146818_RTC)) {
+                warn_report("driftfix 'slew' is not available with this machine");
+            }
         } else if (!strcmp(value, "none")) {
             /* discard is default */
         } else {
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/6] hw/intc: Extract the IRQ counting functions into a separate file
  2023-01-03  8:47 ` [PATCH 3/6] hw/intc: Extract the IRQ counting functions into a separate file Thomas Huth
@ 2023-01-03 12:55   ` Bernhard Beschow
  0 siblings, 0 replies; 19+ messages in thread
From: Bernhard Beschow @ 2023-01-03 12:55 UTC (permalink / raw)
  To: Thomas Huth
  Cc: Paolo Bonzini, Michael S Tsirkin, qemu-devel, Mark Cave-Ayland,
	Philippe Mathieu-Daudé,
	BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

[-- Attachment #1: Type: text/plain, Size: 12296 bytes --]

On Tue, Jan 3, 2023 at 9:48 AM Thomas Huth <thuth@redhat.com> wrote:

> These IRQ counting functions will soon be required in binaries that
> do not include the APIC code, too, so let's extract them into a
> separate file that can be linked independently of the APIC code.
>
> While we're at it, change the apic_* prefix into kvm_* since the
> functions are used from the i8259 PIC (i.e. not the APIC), too.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/hw/i386/apic.h          |  2 --
>  include/hw/i386/apic_internal.h |  1 -
>  include/hw/intc/kvm_irqcount.h  | 10 +++++++
>  hw/i386/kvm/i8259.c             |  4 +--
>  hw/i386/kvm/ioapic.c            |  4 +--
>  hw/intc/apic.c                  |  3 +-
>  hw/intc/apic_common.c           | 30 ++------------------
>  hw/intc/kvm_irqcount.c          | 49 +++++++++++++++++++++++++++++++++
>  hw/rtc/mc146818rtc.c            |  6 ++--
>  hw/intc/meson.build             |  6 ++++
>  hw/intc/trace-events            |  9 +++---
>  11 files changed, 81 insertions(+), 43 deletions(-)
>  create mode 100644 include/hw/intc/kvm_irqcount.h
>  create mode 100644 hw/intc/kvm_irqcount.c
>
> diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
> index da1d2fe155..bdc15a7a73 100644
> --- a/include/hw/i386/apic.h
> +++ b/include/hw/i386/apic.h
> @@ -9,8 +9,6 @@ int apic_accept_pic_intr(DeviceState *s);
>  void apic_deliver_pic_intr(DeviceState *s, int level);
>  void apic_deliver_nmi(DeviceState *d);
>  int apic_get_interrupt(DeviceState *s);
> -void apic_reset_irq_delivered(void);
> -int apic_get_irq_delivered(void);
>  void cpu_set_apic_base(DeviceState *s, uint64_t val);
>  uint64_t cpu_get_apic_base(DeviceState *s);
>  void cpu_set_apic_tpr(DeviceState *s, uint8_t val);
> diff --git a/include/hw/i386/apic_internal.h
> b/include/hw/i386/apic_internal.h
> index c175e7e718..e61ad04769 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -199,7 +199,6 @@ typedef struct VAPICState {
>
>  extern bool apic_report_tpr_access;
>
> -void apic_report_irq_delivered(int delivered);
>  bool apic_next_timer(APICCommonState *s, int64_t current_time);
>  void apic_enable_tpr_access_reporting(DeviceState *d, bool enable);
>  void apic_enable_vapic(DeviceState *d, hwaddr paddr);
> diff --git a/include/hw/intc/kvm_irqcount.h
> b/include/hw/intc/kvm_irqcount.h
> new file mode 100644
> index 0000000000..0ed5999e49
> --- /dev/null
> +++ b/include/hw/intc/kvm_irqcount.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +
> +#ifndef KVM_IRQCOUNT_H
> +#define KVM_IRQCOUNT_H
> +
> +void kvm_report_irq_delivered(int delivered);
> +void kvm_reset_irq_delivered(void);
> +int kvm_get_irq_delivered(void);
> +
> +#endif
> diff --git a/hw/i386/kvm/i8259.c b/hw/i386/kvm/i8259.c
> index d61bae4dc3..3ca0e1ff03 100644
> --- a/hw/i386/kvm/i8259.c
> +++ b/hw/i386/kvm/i8259.c
> @@ -14,7 +14,7 @@
>  #include "hw/isa/i8259_internal.h"
>  #include "hw/intc/i8259.h"
>  #include "qemu/module.h"
> -#include "hw/i386/apic_internal.h"
> +#include "hw/intc/kvm_irqcount.h"
>  #include "hw/irq.h"
>  #include "sysemu/kvm.h"
>  #include "qom/object.h"
> @@ -117,7 +117,7 @@ static void kvm_pic_set_irq(void *opaque, int irq, int
> level)
>
>      pic_stat_update_irq(irq, level);
>      delivered = kvm_set_irq(kvm_state, irq, level);
> -    apic_report_irq_delivered(delivered);
> +    kvm_report_irq_delivered(delivered);
>  }
>
>  static void kvm_pic_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
> index ee7c8ef68b..272e26b4a2 100644
> --- a/hw/i386/kvm/ioapic.c
> +++ b/hw/i386/kvm/ioapic.c
> @@ -15,7 +15,7 @@
>  #include "hw/i386/x86.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/i386/ioapic_internal.h"
> -#include "hw/i386/apic_internal.h"
> +#include "hw/intc/kvm_irqcount.h"
>  #include "sysemu/kvm.h"
>
>  /* PC Utility function */
> @@ -116,7 +116,7 @@ static void kvm_ioapic_set_irq(void *opaque, int irq,
> int level)
>
>      ioapic_stat_update_irq(common, irq, level);
>      delivered = kvm_set_irq(kvm_state, s->kvm_gsi_base + irq, level);
> -    apic_report_irq_delivered(delivered);
> +    kvm_report_irq_delivered(delivered);
>  }
>
>  static void kvm_ioapic_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> index 3df11c34d6..2d3e55f4e2 100644
> --- a/hw/intc/apic.c
> +++ b/hw/intc/apic.c
> @@ -22,6 +22,7 @@
>  #include "hw/i386/apic.h"
>  #include "hw/i386/ioapic.h"
>  #include "hw/intc/i8259.h"
> +#include "hw/intc/kvm_irqcount.h"
>  #include "hw/pci/msi.h"
>  #include "qemu/host-utils.h"
>  #include "sysemu/kvm.h"
> @@ -399,7 +400,7 @@ void apic_poll_irq(DeviceState *dev)
>
>  static void apic_set_irq(APICCommonState *s, int vector_num, int
> trigger_mode)
>  {
> -    apic_report_irq_delivered(!apic_get_bit(s->irr, vector_num));
> +    kvm_report_irq_delivered(!apic_get_bit(s->irr, vector_num));
>
>      apic_set_bit(s->irr, vector_num);
>      if (trigger_mode)
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index 2a20982066..4a34f03047 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -25,6 +25,7 @@
>  #include "qapi/visitor.h"
>  #include "hw/i386/apic.h"
>  #include "hw/i386/apic_internal.h"
> +#include "hw/intc/kvm_irqcount.h"
>  #include "trace.h"
>  #include "hw/boards.h"
>  #include "sysemu/hax.h"
> @@ -33,7 +34,6 @@
>  #include "hw/sysbus.h"
>  #include "migration/vmstate.h"
>
> -static int apic_irq_delivered;
>  bool apic_report_tpr_access;
>
>  void cpu_set_apic_base(DeviceState *dev, uint64_t val)
> @@ -122,32 +122,6 @@ void apic_handle_tpr_access_report(DeviceState *dev,
> target_ulong ip,
>      vapic_report_tpr_access(s->vapic, CPU(s->cpu), ip, access);
>  }
>
> -void apic_report_irq_delivered(int delivered)
> -{
> -    apic_irq_delivered += delivered;
> -
> -    trace_apic_report_irq_delivered(apic_irq_delivered);
> -}
> -
> -void apic_reset_irq_delivered(void)
> -{
> -    /* Copy this into a local variable to encourage gcc to emit a plain
> -     * register for a sys/sdt.h marker.  For details on this workaround,
> see:
> -     * https://sourceware.org/bugzilla/show_bug.cgi?id=13296
> -     */
> -    volatile int a_i_d = apic_irq_delivered;
> -    trace_apic_reset_irq_delivered(a_i_d);
> -
> -    apic_irq_delivered = 0;
> -}
> -
> -int apic_get_irq_delivered(void)
> -{
> -    trace_apic_get_irq_delivered(apic_irq_delivered);
> -
> -    return apic_irq_delivered;
> -}
> -
>  void apic_deliver_nmi(DeviceState *dev)
>  {
>      APICCommonState *s = APIC_COMMON(dev);
> @@ -272,7 +246,7 @@ static void apic_reset_common(DeviceState *dev)
>      s->apicbase = APIC_DEFAULT_ADDRESS | bsp | MSR_IA32_APICBASE_ENABLE;
>      s->id = s->initial_apic_id;
>
> -    apic_reset_irq_delivered();
> +    kvm_reset_irq_delivered();
>
>      s->vapic_paddr = 0;
>      info->vapic_base_update(s);
> diff --git a/hw/intc/kvm_irqcount.c b/hw/intc/kvm_irqcount.c
> new file mode 100644
> index 0000000000..2ef8a83a7a
> --- /dev/null
> +++ b/hw/intc/kvm_irqcount.c
> @@ -0,0 +1,49 @@
> +/*
> + * KVM PIC functions for counting the delivered IRQs.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <
> http://www.gnu.org/licenses/>
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/intc/kvm_irqcount.h"
> +#include "trace.h"
> +
> +static int kvm_irq_delivered;
> +
> +void kvm_report_irq_delivered(int delivered)
> +{
> +    kvm_irq_delivered += delivered;
> +
> +    trace_kvm_report_irq_delivered(kvm_irq_delivered);
> +}
> +
> +void kvm_reset_irq_delivered(void)
> +{
> +    /*
> +     * Copy this into a local variable to encourage gcc to emit a plain
> +     * register for a sys/sdt.h marker.  For details on this workaround,
> see:
> +     * https://sourceware.org/bugzilla/show_bug.cgi?id=13296
> +     */
> +    volatile int k_i_d = kvm_irq_delivered;
> +    trace_kvm_reset_irq_delivered(k_i_d);
> +
> +    kvm_irq_delivered = 0;
> +}
> +
> +int kvm_get_irq_delivered(void)
> +{
> +    trace_kvm_get_irq_delivered(kvm_irq_delivered);
> +
> +    return kvm_irq_delivered;
> +}
> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index 1ebb412479..947d68c257 100644
> --- a/hw/rtc/mc146818rtc.c
> +++ b/hw/rtc/mc146818rtc.c
> @@ -27,6 +27,7 @@
>  #include "qemu/module.h"
>  #include "qemu/bcd.h"
>  #include "hw/acpi/acpi_aml_interface.h"
> +#include "hw/intc/kvm_irqcount.h"
>  #include "hw/irq.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/qdev-properties-system.h"
> @@ -46,7 +47,6 @@
>
>  #ifdef TARGET_I386
>  #include "qapi/qapi-commands-misc-target.h"
> -#include "hw/i386/apic.h"
>  #endif
>
>  //#define DEBUG_CMOS
> @@ -124,9 +124,9 @@ void qmp_rtc_reset_reinjection(Error **errp)
>
>  static bool rtc_policy_slew_deliver_irq(RTCState *s)
>  {
> -    apic_reset_irq_delivered();
> +    kvm_reset_irq_delivered();
>      qemu_irq_raise(s->irq);
> -    return apic_get_irq_delivered();
> +    return kvm_get_irq_delivered();
>  }
>
>  static void rtc_coalesced_timer(void *opaque)
> diff --git a/hw/intc/meson.build b/hw/intc/meson.build
> index bcbf22ff51..cd9f1ee888 100644
> --- a/hw/intc/meson.build
> +++ b/hw/intc/meson.build
> @@ -25,6 +25,12 @@ softmmu_ss.add(when: 'CONFIG_XILINX', if_true:
> files('xilinx_intc.c'))
>  softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP', if_true:
> files('xlnx-zynqmp-ipi.c'))
>  softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP_PMU', if_true:
> files('xlnx-pmu-iomod-intc.c'))
>
> +if config_all_devices.has_key('CONFIG_APIC') or \
> +   config_all_devices.has_key('CONFIG_I8259') or \
> +   config_all_devices.has_key('CONFIG_MC146818RTC')
> +    softmmu_ss.add(files('kvm_irqcount.c'))
> +endif
>

I'd slightly prefer to track these dependencies via KConfig since its
declarative nature seems to lend itself better for tooling. Anyway:

Reviewed-by: Bernhard Beschow <shentey@gmail.com>

+
>  specific_ss.add(when: 'CONFIG_ALLWINNER_A10_PIC', if_true:
> files('allwinner-a10-pic.c'))
>  specific_ss.add(when: 'CONFIG_APIC', if_true: files('apic.c',
> 'apic_common.c'))
>  specific_ss.add(when: 'CONFIG_ARM_GIC', if_true:
> files('arm_gicv3_cpuif_common.c'))
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index 6fbc2045e6..50cadfb996 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -10,10 +10,6 @@ pic_ioport_read(bool master, uint64_t addr, int val)
> "master %d addr 0x%"PRIx64"
>  # apic_common.c
>  cpu_set_apic_base(uint64_t val) "0x%016"PRIx64
>  cpu_get_apic_base(uint64_t val) "0x%016"PRIx64
> -# coalescing
> -apic_report_irq_delivered(int apic_irq_delivered) "coalescing %d"
> -apic_reset_irq_delivered(int apic_irq_delivered) "old coalescing %d"
> -apic_get_irq_delivered(int apic_irq_delivered) "returning coalescing %d"
>
>  # apic.c
>  apic_local_deliver(int vector, uint32_t lvt) "vector %d delivery mode %d"
> @@ -30,6 +26,11 @@ ioapic_mem_read(uint8_t addr, uint8_t regsel, uint8_t
> size, uint32_t val) "ioapi
>  ioapic_mem_write(uint8_t addr, uint8_t regsel, uint8_t size, uint32_t
> val) "ioapic mem write addr 0x%"PRIx8" regsel: 0x%"PRIx8" size 0x%"PRIx8"
> val 0x%"PRIx32
>  ioapic_set_irq(int vector, int level) "vector: %d level: %d"
>
> +# kvm_irqcount.c
> +kvm_report_irq_delivered(int irq_delivered) "coalescing %d"
> +kvm_reset_irq_delivered(int irq_delivered) "old coalescing %d"
> +kvm_get_irq_delivered(int irq_delivered) "returning coalescing %d"
> +
>  # slavio_intctl.c
>  slavio_intctl_mem_readl(uint32_t cpu, uint64_t addr, uint32_t ret) "read
> cpu %d reg 0x%"PRIx64" = 0x%x"
>  slavio_intctl_mem_writel(uint32_t cpu, uint64_t addr, uint32_t val)
> "write cpu %d reg 0x%"PRIx64" = 0x%x"
> --
> 2.31.1
>
>

[-- Attachment #2: Type: text/html, Size: 15232 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 5/6] hw/rtc/mc146818rtc: Make the mc146818 RTC device target independent
  2023-01-03  8:48 ` [PATCH 5/6] hw/rtc/mc146818rtc: Make the mc146818 RTC device target independent Thomas Huth
@ 2023-01-03 12:58   ` Bernhard Beschow
  0 siblings, 0 replies; 19+ messages in thread
From: Bernhard Beschow @ 2023-01-03 12:58 UTC (permalink / raw)
  To: Thomas Huth
  Cc: Paolo Bonzini, Michael S Tsirkin, qemu-devel, Mark Cave-Ayland,
	Philippe Mathieu-Daudé,
	BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

[-- Attachment #1: Type: text/plain, Size: 4060 bytes --]

On Tue, Jan 3, 2023 at 9:48 AM Thomas Huth <thuth@redhat.com> wrote:

> The only reason for this code being target dependent was the IRQ-counting
> related code in rtc_policy_slew_deliver_irq(). Since these functions have
> been moved into a new, separate file (kvm_irqcount.c) which is now always
> compiled and linked if either APIC or the mc146818 device are required,
>

I think you don't need to mention the APIC here since this patch doesn't
touch it and since kvm_irqcount.c is also compiled if the i8259 is
required. Anyway:

Reviewed-by: Bernhard Beschow <shentey@gmail.com>


> and since we've got a new mechanism for deciding whether the slew tick
> policy is available now (via the "slew-tick-policy-available" property),
> we can get rid of the #ifdef TARGET_I386 switches in mc146818rtc.c and
> declare it in the softmmu_ss instead of specific_ss, so that the code only
> gets compiled once for all targets.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/hw/rtc/mc146818rtc.h |  1 +
>  hw/rtc/mc146818rtc.c         | 15 +--------------
>  hw/rtc/meson.build           |  3 +--
>  3 files changed, 3 insertions(+), 16 deletions(-)
>
> diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
> index 54af63d091..9d15d70da8 100644
> --- a/include/hw/rtc/mc146818rtc.h
> +++ b/include/hw/rtc/mc146818rtc.h
> @@ -56,5 +56,6 @@ ISADevice *mc146818_rtc_init(ISABus *bus, int base_year,
>                               qemu_irq intercept_irq);
>  void rtc_set_memory(ISADevice *dev, int addr, int val);
>  int rtc_get_memory(ISADevice *dev, int addr);
> +void qmp_rtc_reset_reinjection(Error **errp);
>
>  #endif /* HW_RTC_MC146818RTC_H */
> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index 86381a74c3..4497ddf024 100644
> --- a/hw/rtc/mc146818rtc.c
> +++ b/hw/rtc/mc146818rtc.c
> @@ -45,10 +45,6 @@
>  #include "qapi/visitor.h"
>  #include "hw/rtc/mc146818rtc_regs.h"
>
> -#ifdef TARGET_I386
> -#include "qapi/qapi-commands-misc-target.h"
> -#endif
> -
>  //#define DEBUG_CMOS
>  //#define DEBUG_COALESCED
>
> @@ -112,7 +108,6 @@ static void rtc_coalesced_timer_update(RTCState *s)
>  static QLIST_HEAD(, RTCState) rtc_devices =
>      QLIST_HEAD_INITIALIZER(rtc_devices);
>
> -#ifdef TARGET_I386
>  void qmp_rtc_reset_reinjection(Error **errp)
>  {
>      RTCState *s;
> @@ -124,6 +119,7 @@ void qmp_rtc_reset_reinjection(Error **errp)
>
>  static bool rtc_policy_slew_deliver_irq(RTCState *s)
>  {
> +    assert(s->slew_tick_policy_available);
>      kvm_reset_irq_delivered();
>      qemu_irq_raise(s->irq);
>      return kvm_get_irq_delivered();
> @@ -145,13 +141,6 @@ static void rtc_coalesced_timer(void *opaque)
>
>      rtc_coalesced_timer_update(s);
>  }
> -#else
> -static bool rtc_policy_slew_deliver_irq(RTCState *s)
> -{
> -    assert(0);
> -    return false;
> -}
> -#endif
>
>  static uint32_t rtc_periodic_clock_ticks(RTCState *s)
>  {
> @@ -925,12 +914,10 @@ static void rtc_realizefn(DeviceState *dev, Error
> **errp)
>      case LOST_TICK_POLICY_DISCARD:
>          break;
>      case LOST_TICK_POLICY_SLEW:
> -#ifdef TARGET_I386
>          if (s->slew_tick_policy_available) {
>              s->coalesced_timer = timer_new_ns(rtc_clock,
> rtc_coalesced_timer, s);
>              break;
>          }
> -#endif
>          /* fallthrough */
>      default:
>          error_setg(errp, "Invalid lost tick policy.");
> diff --git a/hw/rtc/meson.build b/hw/rtc/meson.build
> index dc33973384..34a4d316fa 100644
> --- a/hw/rtc/meson.build
> +++ b/hw/rtc/meson.build
> @@ -13,5 +13,4 @@ softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true:
> files('aspeed_rtc.c'))
>  softmmu_ss.add(when: 'CONFIG_GOLDFISH_RTC', if_true:
> files('goldfish_rtc.c'))
>  softmmu_ss.add(when: 'CONFIG_LS7A_RTC', if_true: files('ls7a_rtc.c'))
>  softmmu_ss.add(when: 'CONFIG_ALLWINNER_H3', if_true:
> files('allwinner-rtc.c'))
> -
> -specific_ss.add(when: 'CONFIG_MC146818RTC', if_true:
> files('mc146818rtc.c'))
> +softmmu_ss.add(when: 'CONFIG_MC146818RTC', if_true:
> files('mc146818rtc.c'))
> --
> 2.31.1
>
>

[-- Attachment #2: Type: text/html, Size: 5250 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 6/6] softmmu/rtc: Emit warning when using driftfix=slew on systems without mc146818
  2023-01-03  8:48 ` [PATCH 6/6] softmmu/rtc: Emit warning when using driftfix=slew on systems without mc146818 Thomas Huth
@ 2023-01-03 13:08   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-01-03 13:08 UTC (permalink / raw)
  To: Thomas Huth, Paolo Bonzini, Michael S Tsirkin, qemu-devel,
	Bernhard Beschow, Mark Cave-Ayland
  Cc: BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

On 3/1/23 09:48, Thomas Huth wrote:
> The 'slew' lost tick policy is only available on systems with a mc146818
> RTC. On other systems, "-rtc driftfix=slew" is currently silently ignored.
> Let's emit at least a warning in this case to make the users aware that
> there is something wrong in their command line settings.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   softmmu/rtc.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy
  2023-01-03  8:47 ` [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy Thomas Huth
@ 2023-01-03 13:10   ` Philippe Mathieu-Daudé
  2023-01-03 13:32   ` Bernhard Beschow
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-01-03 13:10 UTC (permalink / raw)
  To: Thomas Huth, Paolo Bonzini, Michael S Tsirkin, qemu-devel,
	Bernhard Beschow, Mark Cave-Ayland
  Cc: BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

On 3/1/23 09:47, Thomas Huth wrote:
> We want to get rid of the "#ifdef TARGET_I386" statements in the mc146818
> code, so we need a different way to decide whether the slew tick policy
> is available or not. Introduce a new property "slew-tick-policy-available"
> which can be set by the machines that support this tick policy.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   include/hw/rtc/mc146818rtc.h |  1 +
>   hw/i386/pc_piix.c            |  1 +
>   hw/isa/lpc_ich9.c            |  1 +
>   hw/isa/piix3.c               |  1 +
>   hw/rtc/mc146818rtc.c         | 16 ++++++++++------
>   5 files changed, 14 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/6] hw/i386/pc: No need for rtc_state to be an out-parameter
  2023-01-03  8:47 ` [PATCH 2/6] hw/i386/pc: No need for rtc_state to be an out-parameter Thomas Huth
@ 2023-01-03 13:11   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-01-03 13:11 UTC (permalink / raw)
  To: Thomas Huth, Paolo Bonzini, Michael S Tsirkin, qemu-devel,
	Bernhard Beschow, Mark Cave-Ayland
  Cc: BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

On 3/1/23 09:47, Thomas Huth wrote:
> From: Bernhard Beschow <shentey@gmail.com>
> 
> Now that the RTC is created as part of the southbridges it doesn't need
> to be an out-parameter any longer.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Message-Id: <20221022150508.26830-12-shentey@gmail.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   include/hw/i386/pc.h |  2 +-
>   hw/i386/pc.c         | 12 ++++++------
>   hw/i386/pc_piix.c    |  2 +-
>   hw/i386/pc_q35.c     |  2 +-
>   4 files changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy
  2023-01-03  8:47 ` [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy Thomas Huth
  2023-01-03 13:10   ` Philippe Mathieu-Daudé
@ 2023-01-03 13:32   ` Bernhard Beschow
  2023-01-03 13:46     ` Bernhard Beschow
  2023-01-03 15:00   ` Bernhard Beschow
  2023-01-04  8:55   ` Mark Cave-Ayland
  3 siblings, 1 reply; 19+ messages in thread
From: Bernhard Beschow @ 2023-01-03 13:32 UTC (permalink / raw)
  To: Thomas Huth
  Cc: Paolo Bonzini, Michael S Tsirkin, qemu-devel, Mark Cave-Ayland,
	Philippe Mathieu-Daudé,
	BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

[-- Attachment #1: Type: text/plain, Size: 5171 bytes --]

On Tue, Jan 3, 2023 at 9:48 AM Thomas Huth <thuth@redhat.com> wrote:

> We want to get rid of the "#ifdef TARGET_I386" statements in the mc146818
> code, so we need a different way to decide whether the slew tick policy
> is available or not. Introduce a new property "slew-tick-policy-available"
> which can be set by the machines that support this tick policy.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/hw/rtc/mc146818rtc.h |  1 +
>  hw/i386/pc_piix.c            |  1 +
>  hw/isa/lpc_ich9.c            |  1 +
>  hw/isa/piix3.c               |  1 +
>  hw/rtc/mc146818rtc.c         | 16 ++++++++++------
>  5 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
> index 1db0fcee92..54af63d091 100644
> --- a/include/hw/rtc/mc146818rtc.h
> +++ b/include/hw/rtc/mc146818rtc.h
> @@ -45,6 +45,7 @@ struct RTCState {
>      QEMUTimer *coalesced_timer;
>      Notifier clock_reset_notifier;
>      LostTickPolicy lost_tick_policy;
> +    bool slew_tick_policy_available;
>      Notifier suspend_notifier;
>      QLIST_ENTRY(RTCState) link;
>  };
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index bc9ea8cdae..382c6add3b 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -233,6 +233,7 @@ static void pc_init1(MachineState *machine,
>
>          rtc_state = isa_new(TYPE_MC146818_RTC);
>          qdev_prop_set_int32(DEVICE(rtc_state), "base_year", 2000);
> +        qdev_prop_set_bit(DEVICE(rtc_state),
> "slew-tick-policy-available", true);
>          isa_realize_and_unref(rtc_state, isa_bus, &error_fatal);
>
>          i8257_dma_init(isa_bus, 0);
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 498175c1cc..aeab4d8549 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -733,6 +733,7 @@ static void ich9_lpc_realize(PCIDevice *d, Error
> **errp)
>
>      /* RTC */
>      qdev_prop_set_int32(DEVICE(&lpc->rtc), "base_year", 2000);
> +    qdev_prop_set_bit(DEVICE(&lpc->rtc), "slew-tick-policy-available",
> true);
>

In order to not bake in machine-specific assumptions in the device model
I'd move this assignment to pc_q35.c (see below).

     if (!qdev_realize(DEVICE(&lpc->rtc), BUS(isa_bus), errp)) {
>          return;
>      }
> diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
> index c68e51ddad..825b1cbee2 100644
> --- a/hw/isa/piix3.c
> +++ b/hw/isa/piix3.c
> @@ -316,6 +316,7 @@ static void pci_piix3_realize(PCIDevice *dev, Error
> **errp)
>
>      /* RTC */
>      qdev_prop_set_int32(DEVICE(&d->rtc), "base_year", 2000);
> +    qdev_prop_set_bit(DEVICE(&d->rtc), "slew-tick-policy-available",
> true);
>
     if (!qdev_realize(DEVICE(&d->rtc), BUS(isa_bus), errp)) {
>          return;
>      }
>

This section will be reused for PIIX4 in my PIIX consolidation series.
PIIX4 is used in Malta where we want the property to be false. What you
could do instead is to set the property between creation and realization of
TYPE_PIIX3_DEVICE in pc_piix.c. There is also a patch in my PIIX
consolidation series you could take advantage of:
  https://lists.nongnu.org/archive/html/qemu-devel/2022-12/msg03792.html
Having applied the patch, you can then move the assignment to rtc_state
between pci_new_multifunction() and pci_realize_and_unref() and set the
property like so:
https://github.com/shentok/qemu/commit/2277b0abab6bc514824cd7dd76a1476485d67f50
.
There, you could even just set the property to true if kvm_enabled() but we
may need a deprecation period for this. Does it make sense to add a
deprecation message now?

Moreover, setting the property in pc_piix would also just work with other
south bridges such as VT82Cxx which I've also got working with the PC
machine!
https://github.com/shentok/qemu/tree/pc-via

Best regards,
Bernhard

> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
>
> index 947d68c257..86381a74c3 100644
> --- a/hw/rtc/mc146818rtc.c
> +++ b/hw/rtc/mc146818rtc.c
> @@ -922,14 +922,16 @@ static void rtc_realizefn(DeviceState *dev, Error
> **errp)
>      rtc_set_date_from_host(isadev);
>
>      switch (s->lost_tick_policy) {
> -#ifdef TARGET_I386
> -    case LOST_TICK_POLICY_SLEW:
> -        s->coalesced_timer =
> -            timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
> -        break;
> -#endif
>      case LOST_TICK_POLICY_DISCARD:
>          break;
> +    case LOST_TICK_POLICY_SLEW:
> +#ifdef TARGET_I386
> +        if (s->slew_tick_policy_available) {
> +            s->coalesced_timer = timer_new_ns(rtc_clock,
> rtc_coalesced_timer, s);
> +            break;
> +        }
> +#endif
> +        /* fallthrough */
>      default:
>          error_setg(errp, "Invalid lost tick policy.");
>          return;
> @@ -989,6 +991,8 @@ static Property mc146818rtc_properties[] = {
>      DEFINE_PROP_UINT8("irq", RTCState, isairq, RTC_ISA_IRQ),
>      DEFINE_PROP_LOSTTICKPOLICY("lost_tick_policy", RTCState,
>                                 lost_tick_policy,
> LOST_TICK_POLICY_DISCARD),
> +    DEFINE_PROP_BOOL("slew-tick-policy-available", RTCState,
> +                     slew_tick_policy_available, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> --
> 2.31.1
>
>

[-- Attachment #2: Type: text/html, Size: 7491 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy
  2023-01-03 13:32   ` Bernhard Beschow
@ 2023-01-03 13:46     ` Bernhard Beschow
  0 siblings, 0 replies; 19+ messages in thread
From: Bernhard Beschow @ 2023-01-03 13:46 UTC (permalink / raw)
  To: Thomas Huth
  Cc: Paolo Bonzini, Michael S Tsirkin, qemu-devel, Mark Cave-Ayland,
	Philippe Mathieu-Daudé,
	BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

[-- Attachment #1: Type: text/plain, Size: 5867 bytes --]

On Tue, Jan 3, 2023 at 2:32 PM Bernhard Beschow <shentey@gmail.com> wrote:

>
>
> On Tue, Jan 3, 2023 at 9:48 AM Thomas Huth <thuth@redhat.com> wrote:
>
>> We want to get rid of the "#ifdef TARGET_I386" statements in the mc146818
>> code, so we need a different way to decide whether the slew tick policy
>> is available or not. Introduce a new property "slew-tick-policy-available"
>> which can be set by the machines that support this tick policy.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  include/hw/rtc/mc146818rtc.h |  1 +
>>  hw/i386/pc_piix.c            |  1 +
>>  hw/isa/lpc_ich9.c            |  1 +
>>  hw/isa/piix3.c               |  1 +
>>  hw/rtc/mc146818rtc.c         | 16 ++++++++++------
>>  5 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
>> index 1db0fcee92..54af63d091 100644
>> --- a/include/hw/rtc/mc146818rtc.h
>> +++ b/include/hw/rtc/mc146818rtc.h
>> @@ -45,6 +45,7 @@ struct RTCState {
>>      QEMUTimer *coalesced_timer;
>>      Notifier clock_reset_notifier;
>>      LostTickPolicy lost_tick_policy;
>> +    bool slew_tick_policy_available;
>>      Notifier suspend_notifier;
>>      QLIST_ENTRY(RTCState) link;
>>  };
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index bc9ea8cdae..382c6add3b 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -233,6 +233,7 @@ static void pc_init1(MachineState *machine,
>>
>>          rtc_state = isa_new(TYPE_MC146818_RTC);
>>          qdev_prop_set_int32(DEVICE(rtc_state), "base_year", 2000);
>> +        qdev_prop_set_bit(DEVICE(rtc_state),
>> "slew-tick-policy-available", true);
>>          isa_realize_and_unref(rtc_state, isa_bus, &error_fatal);
>>
>>          i8257_dma_init(isa_bus, 0);
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index 498175c1cc..aeab4d8549 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -733,6 +733,7 @@ static void ich9_lpc_realize(PCIDevice *d, Error
>> **errp)
>>
>>      /* RTC */
>>      qdev_prop_set_int32(DEVICE(&lpc->rtc), "base_year", 2000);
>> +    qdev_prop_set_bit(DEVICE(&lpc->rtc), "slew-tick-policy-available",
>> true);
>>
>
> In order to not bake in machine-specific assumptions in the device model
> I'd move this assignment to pc_q35.c (see below).
>
>      if (!qdev_realize(DEVICE(&lpc->rtc), BUS(isa_bus), errp)) {
>>          return;
>>      }
>> diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
>> index c68e51ddad..825b1cbee2 100644
>> --- a/hw/isa/piix3.c
>> +++ b/hw/isa/piix3.c
>> @@ -316,6 +316,7 @@ static void pci_piix3_realize(PCIDevice *dev, Error
>> **errp)
>>
>>      /* RTC */
>>      qdev_prop_set_int32(DEVICE(&d->rtc), "base_year", 2000);
>> +    qdev_prop_set_bit(DEVICE(&d->rtc), "slew-tick-policy-available",
>> true);
>>
>      if (!qdev_realize(DEVICE(&d->rtc), BUS(isa_bus), errp)) {
>>          return;
>>      }
>>
>
> This section will be reused for PIIX4 in my PIIX consolidation series.
> PIIX4 is used in Malta where we want the property to be false. What you
> could do instead is to set the property between creation and realization of
> TYPE_PIIX3_DEVICE in pc_piix.c. There is also a patch in my PIIX
> consolidation series you could take advantage of:
>   https://lists.nongnu.org/archive/html/qemu-devel/2022-12/msg03792.html
>

Perhaps it'd make sense to upstream part of my PIIX consolidation series up
to and including
https://lists.nongnu.org/archive/html/qemu-devel/2022-12/msg03804.html to
avoid merge conflicts? This part should not be blocked by MIPS and could
release Phil from some pressure. Since that series also depends on
https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03310.html it
could be upstreamed already as well.

Best regards,
Bernhard

Having applied the patch, you can then move the assignment to rtc_state
> between pci_new_multifunction() and pci_realize_and_unref() and set the
> property like so:
>
> https://github.com/shentok/qemu/commit/2277b0abab6bc514824cd7dd76a1476485d67f50
> .
> There, you could even just set the property to true if kvm_enabled() but
> we may need a deprecation period for this. Does it make sense to add a
> deprecation message now?
>
> Moreover, setting the property in pc_piix would also just work with other
> south bridges such as VT82Cxx which I've also got working with the PC
> machine!
> https://github.com/shentok/qemu/tree/pc-via
>
> Best regards,
> Bernhard
>
> > diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
>>
>> index 947d68c257..86381a74c3 100644
>> --- a/hw/rtc/mc146818rtc.c
>> +++ b/hw/rtc/mc146818rtc.c
>> @@ -922,14 +922,16 @@ static void rtc_realizefn(DeviceState *dev, Error
>> **errp)
>>      rtc_set_date_from_host(isadev);
>>
>>      switch (s->lost_tick_policy) {
>> -#ifdef TARGET_I386
>> -    case LOST_TICK_POLICY_SLEW:
>> -        s->coalesced_timer =
>> -            timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
>> -        break;
>> -#endif
>>      case LOST_TICK_POLICY_DISCARD:
>>          break;
>> +    case LOST_TICK_POLICY_SLEW:
>> +#ifdef TARGET_I386
>> +        if (s->slew_tick_policy_available) {
>> +            s->coalesced_timer = timer_new_ns(rtc_clock,
>> rtc_coalesced_timer, s);
>> +            break;
>> +        }
>> +#endif
>> +        /* fallthrough */
>>      default:
>>          error_setg(errp, "Invalid lost tick policy.");
>>          return;
>> @@ -989,6 +991,8 @@ static Property mc146818rtc_properties[] = {
>>      DEFINE_PROP_UINT8("irq", RTCState, isairq, RTC_ISA_IRQ),
>>      DEFINE_PROP_LOSTTICKPOLICY("lost_tick_policy", RTCState,
>>                                 lost_tick_policy,
>> LOST_TICK_POLICY_DISCARD),
>> +    DEFINE_PROP_BOOL("slew-tick-policy-available", RTCState,
>> +                     slew_tick_policy_available, false),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>
>> --
>> 2.31.1
>>
>>

[-- Attachment #2: Type: text/html, Size: 8816 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy
  2023-01-03  8:47 ` [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy Thomas Huth
  2023-01-03 13:10   ` Philippe Mathieu-Daudé
  2023-01-03 13:32   ` Bernhard Beschow
@ 2023-01-03 15:00   ` Bernhard Beschow
  2023-01-04  8:55   ` Mark Cave-Ayland
  3 siblings, 0 replies; 19+ messages in thread
From: Bernhard Beschow @ 2023-01-03 15:00 UTC (permalink / raw)
  To: Thomas Huth, Paolo Bonzini, Michael S Tsirkin, qemu-devel,
	Mark Cave-Ayland
  Cc: Philippe Mathieu-Daudé,
	BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno



Am 3. Januar 2023 08:47:59 UTC schrieb Thomas Huth <thuth@redhat.com>:
>We want to get rid of the "#ifdef TARGET_I386" statements in the mc146818
>code, so we need a different way to decide whether the slew tick policy
>is available or not. Introduce a new property "slew-tick-policy-available"
>which can be set by the machines that support this tick policy.
>
>Signed-off-by: Thomas Huth <thuth@redhat.com>
>---
> include/hw/rtc/mc146818rtc.h |  1 +
> hw/i386/pc_piix.c            |  1 +
> hw/isa/lpc_ich9.c            |  1 +
> hw/isa/piix3.c               |  1 +
> hw/rtc/mc146818rtc.c         | 16 ++++++++++------

IIUC you might also need to inline mc146818_rtc_init() in i386/microvm.c and activate the new property there as well.

Best regards,
Bernhard

> 5 files changed, 14 insertions(+), 6 deletions(-)
>
>diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
>index 1db0fcee92..54af63d091 100644
>--- a/include/hw/rtc/mc146818rtc.h
>+++ b/include/hw/rtc/mc146818rtc.h
>@@ -45,6 +45,7 @@ struct RTCState {
>     QEMUTimer *coalesced_timer;
>     Notifier clock_reset_notifier;
>     LostTickPolicy lost_tick_policy;
>+    bool slew_tick_policy_available;
>     Notifier suspend_notifier;
>     QLIST_ENTRY(RTCState) link;
> };
>diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>index bc9ea8cdae..382c6add3b 100644
>--- a/hw/i386/pc_piix.c
>+++ b/hw/i386/pc_piix.c
>@@ -233,6 +233,7 @@ static void pc_init1(MachineState *machine,
> 
>         rtc_state = isa_new(TYPE_MC146818_RTC);
>         qdev_prop_set_int32(DEVICE(rtc_state), "base_year", 2000);
>+        qdev_prop_set_bit(DEVICE(rtc_state), "slew-tick-policy-available", true);
>         isa_realize_and_unref(rtc_state, isa_bus, &error_fatal);
> 
>         i8257_dma_init(isa_bus, 0);
>diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>index 498175c1cc..aeab4d8549 100644
>--- a/hw/isa/lpc_ich9.c
>+++ b/hw/isa/lpc_ich9.c
>@@ -733,6 +733,7 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
> 
>     /* RTC */
>     qdev_prop_set_int32(DEVICE(&lpc->rtc), "base_year", 2000);
>+    qdev_prop_set_bit(DEVICE(&lpc->rtc), "slew-tick-policy-available", true);
>     if (!qdev_realize(DEVICE(&lpc->rtc), BUS(isa_bus), errp)) {
>         return;
>     }
>diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
>index c68e51ddad..825b1cbee2 100644
>--- a/hw/isa/piix3.c
>+++ b/hw/isa/piix3.c
>@@ -316,6 +316,7 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
> 
>     /* RTC */
>     qdev_prop_set_int32(DEVICE(&d->rtc), "base_year", 2000);
>+    qdev_prop_set_bit(DEVICE(&d->rtc), "slew-tick-policy-available", true);
>     if (!qdev_realize(DEVICE(&d->rtc), BUS(isa_bus), errp)) {
>         return;
>     }
>diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
>index 947d68c257..86381a74c3 100644
>--- a/hw/rtc/mc146818rtc.c
>+++ b/hw/rtc/mc146818rtc.c
>@@ -922,14 +922,16 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>     rtc_set_date_from_host(isadev);
> 
>     switch (s->lost_tick_policy) {
>-#ifdef TARGET_I386
>-    case LOST_TICK_POLICY_SLEW:
>-        s->coalesced_timer =
>-            timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
>-        break;
>-#endif
>     case LOST_TICK_POLICY_DISCARD:
>         break;
>+    case LOST_TICK_POLICY_SLEW:
>+#ifdef TARGET_I386
>+        if (s->slew_tick_policy_available) {
>+            s->coalesced_timer = timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
>+            break;
>+        }
>+#endif
>+        /* fallthrough */
>     default:
>         error_setg(errp, "Invalid lost tick policy.");
>         return;
>@@ -989,6 +991,8 @@ static Property mc146818rtc_properties[] = {
>     DEFINE_PROP_UINT8("irq", RTCState, isairq, RTC_ISA_IRQ),
>     DEFINE_PROP_LOSTTICKPOLICY("lost_tick_policy", RTCState,
>                                lost_tick_policy, LOST_TICK_POLICY_DISCARD),
>+    DEFINE_PROP_BOOL("slew-tick-policy-available", RTCState,
>+                     slew_tick_policy_available, false),
>     DEFINE_PROP_END_OF_LIST(),
> };
> 


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy
  2023-01-03  8:47 ` [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy Thomas Huth
                     ` (2 preceding siblings ...)
  2023-01-03 15:00   ` Bernhard Beschow
@ 2023-01-04  8:55   ` Mark Cave-Ayland
  2023-01-09 20:12     ` Thomas Huth
  3 siblings, 1 reply; 19+ messages in thread
From: Mark Cave-Ayland @ 2023-01-04  8:55 UTC (permalink / raw)
  To: Thomas Huth, Paolo Bonzini, Michael S Tsirkin, qemu-devel,
	Bernhard Beschow
  Cc: Philippe Mathieu-Daudé,
	BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

On 03/01/2023 08:47, Thomas Huth wrote:

> We want to get rid of the "#ifdef TARGET_I386" statements in the mc146818
> code, so we need a different way to decide whether the slew tick policy
> is available or not. Introduce a new property "slew-tick-policy-available"
> which can be set by the machines that support this tick policy.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   include/hw/rtc/mc146818rtc.h |  1 +
>   hw/i386/pc_piix.c            |  1 +
>   hw/isa/lpc_ich9.c            |  1 +
>   hw/isa/piix3.c               |  1 +
>   hw/rtc/mc146818rtc.c         | 16 ++++++++++------
>   5 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
> index 1db0fcee92..54af63d091 100644
> --- a/include/hw/rtc/mc146818rtc.h
> +++ b/include/hw/rtc/mc146818rtc.h
> @@ -45,6 +45,7 @@ struct RTCState {
>       QEMUTimer *coalesced_timer;
>       Notifier clock_reset_notifier;
>       LostTickPolicy lost_tick_policy;
> +    bool slew_tick_policy_available;
>       Notifier suspend_notifier;
>       QLIST_ENTRY(RTCState) link;
>   };
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index bc9ea8cdae..382c6add3b 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -233,6 +233,7 @@ static void pc_init1(MachineState *machine,
>   
>           rtc_state = isa_new(TYPE_MC146818_RTC);
>           qdev_prop_set_int32(DEVICE(rtc_state), "base_year", 2000);
> +        qdev_prop_set_bit(DEVICE(rtc_state), "slew-tick-policy-available", true);
>           isa_realize_and_unref(rtc_state, isa_bus, &error_fatal);
>   
>           i8257_dma_init(isa_bus, 0);
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 498175c1cc..aeab4d8549 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -733,6 +733,7 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
>   
>       /* RTC */
>       qdev_prop_set_int32(DEVICE(&lpc->rtc), "base_year", 2000);
> +    qdev_prop_set_bit(DEVICE(&lpc->rtc), "slew-tick-policy-available", true);
>       if (!qdev_realize(DEVICE(&lpc->rtc), BUS(isa_bus), errp)) {
>           return;
>       }
> diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
> index c68e51ddad..825b1cbee2 100644
> --- a/hw/isa/piix3.c
> +++ b/hw/isa/piix3.c
> @@ -316,6 +316,7 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
>   
>       /* RTC */
>       qdev_prop_set_int32(DEVICE(&d->rtc), "base_year", 2000);
> +    qdev_prop_set_bit(DEVICE(&d->rtc), "slew-tick-policy-available", true);
>       if (!qdev_realize(DEVICE(&d->rtc), BUS(isa_bus), errp)) {
>           return;
>       }
> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index 947d68c257..86381a74c3 100644
> --- a/hw/rtc/mc146818rtc.c
> +++ b/hw/rtc/mc146818rtc.c
> @@ -922,14 +922,16 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>       rtc_set_date_from_host(isadev);
>   
>       switch (s->lost_tick_policy) {
> -#ifdef TARGET_I386
> -    case LOST_TICK_POLICY_SLEW:
> -        s->coalesced_timer =
> -            timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
> -        break;
> -#endif
>       case LOST_TICK_POLICY_DISCARD:
>           break;
> +    case LOST_TICK_POLICY_SLEW:
> +#ifdef TARGET_I386
> +        if (s->slew_tick_policy_available) {
> +            s->coalesced_timer = timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
> +            break;
> +        }
> +#endif
> +        /* fallthrough */
>       default:
>           error_setg(errp, "Invalid lost tick policy.");
>           return;
> @@ -989,6 +991,8 @@ static Property mc146818rtc_properties[] = {
>       DEFINE_PROP_UINT8("irq", RTCState, isairq, RTC_ISA_IRQ),
>       DEFINE_PROP_LOSTTICKPOLICY("lost_tick_policy", RTCState,
>                                  lost_tick_policy, LOST_TICK_POLICY_DISCARD),
> +    DEFINE_PROP_BOOL("slew-tick-policy-available", RTCState,
> +                     slew_tick_policy_available, false),
>       DEFINE_PROP_END_OF_LIST(),
>   };

My first thought when looking at the new "slew-tick-policy-available" property 
introduced above was that it seems to overlap with the "lost_tick_policy" property 
defined just above it using DEFINE_PROP_LOSTTICKPOLICY().

This made me wonder if a better approach here would be to move the logic that 
determines if LOST_TICK_POLICY_SLEW is available into the "lost_tick_policy" property 
setter defined at 
https://gitlab.com/qemu-project/qemu/-/blob/master/hw/core/qdev-properties-system.c#L558.

If you look at the code directly below the link above you can see how set_blocksize() 
overrides the .set function for qdev_prop_blocksize to provide additional validation, 
which is similar to what we are trying to do here.

I think it may be possible to come up with a similar solution for 
qdev_prop_losttickpolicy which makes use of the logic you suggested before i.e.

     MachineState *ms = MACHINE(qdev_get_machine());

     if (!object_dynamic_cast(OBJECT(ms), TYPE_X86_MACHINE)) {
         ....
     }

which can then emit a suitable warning or return an error accordingly. A quick glance 
at hw/core/qdev-properties-system.c suggests there are a number of similar examples 
showing how this could be done.


ATB,

Mark.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy
  2023-01-04  8:55   ` Mark Cave-Ayland
@ 2023-01-09 20:12     ` Thomas Huth
  2023-01-09 20:53       ` B
  0 siblings, 1 reply; 19+ messages in thread
From: Thomas Huth @ 2023-01-09 20:12 UTC (permalink / raw)
  To: Mark Cave-Ayland, Paolo Bonzini, Michael S Tsirkin, qemu-devel,
	Bernhard Beschow
  Cc: Philippe Mathieu-Daudé,
	BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

On 04/01/2023 09.55, Mark Cave-Ayland wrote:
> On 03/01/2023 08:47, Thomas Huth wrote:
> 
>> We want to get rid of the "#ifdef TARGET_I386" statements in the mc146818
>> code, so we need a different way to decide whether the slew tick policy
>> is available or not. Introduce a new property "slew-tick-policy-available"
>> which can be set by the machines that support this tick policy.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   include/hw/rtc/mc146818rtc.h |  1 +
>>   hw/i386/pc_piix.c            |  1 +
>>   hw/isa/lpc_ich9.c            |  1 +
>>   hw/isa/piix3.c               |  1 +
>>   hw/rtc/mc146818rtc.c         | 16 ++++++++++------
>>   5 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
>> index 1db0fcee92..54af63d091 100644
>> --- a/include/hw/rtc/mc146818rtc.h
>> +++ b/include/hw/rtc/mc146818rtc.h
>> @@ -45,6 +45,7 @@ struct RTCState {
>>       QEMUTimer *coalesced_timer;
>>       Notifier clock_reset_notifier;
>>       LostTickPolicy lost_tick_policy;
>> +    bool slew_tick_policy_available;
>>       Notifier suspend_notifier;
>>       QLIST_ENTRY(RTCState) link;
>>   };
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index bc9ea8cdae..382c6add3b 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -233,6 +233,7 @@ static void pc_init1(MachineState *machine,
>>           rtc_state = isa_new(TYPE_MC146818_RTC);
>>           qdev_prop_set_int32(DEVICE(rtc_state), "base_year", 2000);
>> +        qdev_prop_set_bit(DEVICE(rtc_state), 
>> "slew-tick-policy-available", true);
>>           isa_realize_and_unref(rtc_state, isa_bus, &error_fatal);
>>           i8257_dma_init(isa_bus, 0);
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index 498175c1cc..aeab4d8549 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -733,6 +733,7 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
>>       /* RTC */
>>       qdev_prop_set_int32(DEVICE(&lpc->rtc), "base_year", 2000);
>> +    qdev_prop_set_bit(DEVICE(&lpc->rtc), "slew-tick-policy-available", 
>> true);
>>       if (!qdev_realize(DEVICE(&lpc->rtc), BUS(isa_bus), errp)) {
>>           return;
>>       }
>> diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
>> index c68e51ddad..825b1cbee2 100644
>> --- a/hw/isa/piix3.c
>> +++ b/hw/isa/piix3.c
>> @@ -316,6 +316,7 @@ static void pci_piix3_realize(PCIDevice *dev, Error 
>> **errp)
>>       /* RTC */
>>       qdev_prop_set_int32(DEVICE(&d->rtc), "base_year", 2000);
>> +    qdev_prop_set_bit(DEVICE(&d->rtc), "slew-tick-policy-available", true);
>>       if (!qdev_realize(DEVICE(&d->rtc), BUS(isa_bus), errp)) {
>>           return;
>>       }
>> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
>> index 947d68c257..86381a74c3 100644
>> --- a/hw/rtc/mc146818rtc.c
>> +++ b/hw/rtc/mc146818rtc.c
>> @@ -922,14 +922,16 @@ static void rtc_realizefn(DeviceState *dev, Error 
>> **errp)
>>       rtc_set_date_from_host(isadev);
>>       switch (s->lost_tick_policy) {
>> -#ifdef TARGET_I386
>> -    case LOST_TICK_POLICY_SLEW:
>> -        s->coalesced_timer =
>> -            timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
>> -        break;
>> -#endif
>>       case LOST_TICK_POLICY_DISCARD:
>>           break;
>> +    case LOST_TICK_POLICY_SLEW:
>> +#ifdef TARGET_I386
>> +        if (s->slew_tick_policy_available) {
>> +            s->coalesced_timer = timer_new_ns(rtc_clock, 
>> rtc_coalesced_timer, s);
>> +            break;
>> +        }
>> +#endif
>> +        /* fallthrough */
>>       default:
>>           error_setg(errp, "Invalid lost tick policy.");
>>           return;
>> @@ -989,6 +991,8 @@ static Property mc146818rtc_properties[] = {
>>       DEFINE_PROP_UINT8("irq", RTCState, isairq, RTC_ISA_IRQ),
>>       DEFINE_PROP_LOSTTICKPOLICY("lost_tick_policy", RTCState,
>>                                  lost_tick_policy, LOST_TICK_POLICY_DISCARD),
>> +    DEFINE_PROP_BOOL("slew-tick-policy-available", RTCState,
>> +                     slew_tick_policy_available, false),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
> 
> My first thought when looking at the new "slew-tick-policy-available" 
> property introduced above was that it seems to overlap with the 
> "lost_tick_policy" property defined just above it using 
> DEFINE_PROP_LOSTTICKPOLICY().

You've got a point here ... it's a little bit ugly that we have two 
user-visible properties for the lost tick policy now...

> This made me wonder if a better approach here would be to move the logic 
> that determines if LOST_TICK_POLICY_SLEW is available into the 
> "lost_tick_policy" property setter defined at 
> https://gitlab.com/qemu-project/qemu/-/blob/master/hw/core/qdev-properties-system.c#L558. 
> 
> If you look at the code directly below the link above you can see how 
> set_blocksize() overrides the .set function for qdev_prop_blocksize to 
> provide additional validation, which is similar to what we are trying to do 
> here.
> 
> I think it may be possible to come up with a similar solution for 
> qdev_prop_losttickpolicy which makes use of the logic you suggested before i.e.
> 
>      MachineState *ms = MACHINE(qdev_get_machine());
> 
>      if (!object_dynamic_cast(OBJECT(ms), TYPE_X86_MACHINE)) {
>          ....
>      }
> 
> which can then emit a suitable warning or return an error accordingly. A 
> quick glance at hw/core/qdev-properties-system.c suggests there are a number 
> of similar examples showing how this could be done.

Thanks, I like that idea! I'll give it a try!

  Thomas



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy
  2023-01-09 20:12     ` Thomas Huth
@ 2023-01-09 20:53       ` B
  2023-01-10  7:52         ` Thomas Huth
  0 siblings, 1 reply; 19+ messages in thread
From: B @ 2023-01-09 20:53 UTC (permalink / raw)
  To: Thomas Huth, Mark Cave-Ayland, Paolo Bonzini, Michael S Tsirkin,
	qemu-devel
  Cc: Philippe Mathieu-Daudé,
	BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno



Am 9. Januar 2023 20:12:29 UTC schrieb Thomas Huth <thuth@redhat.com>:
>On 04/01/2023 09.55, Mark Cave-Ayland wrote:
>> On 03/01/2023 08:47, Thomas Huth wrote:
>> 
>>> We want to get rid of the "#ifdef TARGET_I386" statements in the mc146818
>>> code, so we need a different way to decide whether the slew tick policy
>>> is available or not. Introduce a new property "slew-tick-policy-available"
>>> which can be set by the machines that support this tick policy.
>>> 
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   include/hw/rtc/mc146818rtc.h |  1 +
>>>   hw/i386/pc_piix.c            |  1 +
>>>   hw/isa/lpc_ich9.c            |  1 +
>>>   hw/isa/piix3.c               |  1 +
>>>   hw/rtc/mc146818rtc.c         | 16 ++++++++++------
>>>   5 files changed, 14 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
>>> index 1db0fcee92..54af63d091 100644
>>> --- a/include/hw/rtc/mc146818rtc.h
>>> +++ b/include/hw/rtc/mc146818rtc.h
>>> @@ -45,6 +45,7 @@ struct RTCState {
>>>       QEMUTimer *coalesced_timer;
>>>       Notifier clock_reset_notifier;
>>>       LostTickPolicy lost_tick_policy;
>>> +    bool slew_tick_policy_available;
>>>       Notifier suspend_notifier;
>>>       QLIST_ENTRY(RTCState) link;
>>>   };
>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>> index bc9ea8cdae..382c6add3b 100644
>>> --- a/hw/i386/pc_piix.c
>>> +++ b/hw/i386/pc_piix.c
>>> @@ -233,6 +233,7 @@ static void pc_init1(MachineState *machine,
>>>           rtc_state = isa_new(TYPE_MC146818_RTC);
>>>           qdev_prop_set_int32(DEVICE(rtc_state), "base_year", 2000);
>>> +        qdev_prop_set_bit(DEVICE(rtc_state), "slew-tick-policy-available", true);
>>>           isa_realize_and_unref(rtc_state, isa_bus, &error_fatal);
>>>           i8257_dma_init(isa_bus, 0);
>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>>> index 498175c1cc..aeab4d8549 100644
>>> --- a/hw/isa/lpc_ich9.c
>>> +++ b/hw/isa/lpc_ich9.c
>>> @@ -733,6 +733,7 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
>>>       /* RTC */
>>>       qdev_prop_set_int32(DEVICE(&lpc->rtc), "base_year", 2000);
>>> +    qdev_prop_set_bit(DEVICE(&lpc->rtc), "slew-tick-policy-available", true);
>>>       if (!qdev_realize(DEVICE(&lpc->rtc), BUS(isa_bus), errp)) {
>>>           return;
>>>       }
>>> diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
>>> index c68e51ddad..825b1cbee2 100644
>>> --- a/hw/isa/piix3.c
>>> +++ b/hw/isa/piix3.c
>>> @@ -316,6 +316,7 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
>>>       /* RTC */
>>>       qdev_prop_set_int32(DEVICE(&d->rtc), "base_year", 2000);
>>> +    qdev_prop_set_bit(DEVICE(&d->rtc), "slew-tick-policy-available", true);
>>>       if (!qdev_realize(DEVICE(&d->rtc), BUS(isa_bus), errp)) {
>>>           return;
>>>       }
>>> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
>>> index 947d68c257..86381a74c3 100644
>>> --- a/hw/rtc/mc146818rtc.c
>>> +++ b/hw/rtc/mc146818rtc.c
>>> @@ -922,14 +922,16 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>>>       rtc_set_date_from_host(isadev);
>>>       switch (s->lost_tick_policy) {
>>> -#ifdef TARGET_I386
>>> -    case LOST_TICK_POLICY_SLEW:
>>> -        s->coalesced_timer =
>>> -            timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
>>> -        break;
>>> -#endif
>>>       case LOST_TICK_POLICY_DISCARD:
>>>           break;
>>> +    case LOST_TICK_POLICY_SLEW:
>>> +#ifdef TARGET_I386
>>> +        if (s->slew_tick_policy_available) {
>>> +            s->coalesced_timer = timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
>>> +            break;
>>> +        }
>>> +#endif
>>> +        /* fallthrough */
>>>       default:
>>>           error_setg(errp, "Invalid lost tick policy.");
>>>           return;
>>> @@ -989,6 +991,8 @@ static Property mc146818rtc_properties[] = {
>>>       DEFINE_PROP_UINT8("irq", RTCState, isairq, RTC_ISA_IRQ),
>>>       DEFINE_PROP_LOSTTICKPOLICY("lost_tick_policy", RTCState,
>>>                                  lost_tick_policy, LOST_TICK_POLICY_DISCARD),
>>> +    DEFINE_PROP_BOOL("slew-tick-policy-available", RTCState,
>>> +                     slew_tick_policy_available, false),
>>>       DEFINE_PROP_END_OF_LIST(),
>>>   };
>> 
>> My first thought when looking at the new "slew-tick-policy-available" property introduced above was that it seems to overlap with the "lost_tick_policy" property defined just above it using DEFINE_PROP_LOSTTICKPOLICY().
>
>You've got a point here ... it's a little bit ugly that we have two user-visible properties for the lost tick policy now...

Indeed!

>> This made me wonder if a better approach here would be to move the logic that determines if LOST_TICK_POLICY_SLEW is available into the "lost_tick_policy" property setter defined at https://gitlab.com/qemu-project/qemu/-/blob/master/hw/core/qdev-properties-system.c#L558. 
>> If you look at the code directly below the link above you can see how set_blocksize() overrides the .set function for qdev_prop_blocksize to provide additional validation, which is similar to what we are trying to do here.
>> 
>> I think it may be possible to come up with a similar solution for qdev_prop_losttickpolicy which makes use of the logic you suggested before i.e.
>> 
>>      MachineState *ms = MACHINE(qdev_get_machine());
>> 
>>      if (!object_dynamic_cast(OBJECT(ms), TYPE_X86_MACHINE)) {
>>          ....
>>      }
>> 
>> which can then emit a suitable warning or return an error accordingly. A quick glance at hw/core/qdev-properties-system.c suggests there are a number of similar examples showing how this could be done.
>
>Thanks, I like that idea! I'll give it a try!

Does the microvm need consideration as well?

Best regards,
Bernhard
>
> Thomas
>


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy
  2023-01-09 20:53       ` B
@ 2023-01-10  7:52         ` Thomas Huth
  0 siblings, 0 replies; 19+ messages in thread
From: Thomas Huth @ 2023-01-10  7:52 UTC (permalink / raw)
  To: B, Mark Cave-Ayland, Paolo Bonzini, Michael S Tsirkin, qemu-devel
  Cc: Philippe Mathieu-Daudé,
	BALATON Zoltan, Marcel Apfelbaum, Richard Henderson,
	Eduardo Habkost, Hervé Poussineau, Aurelien Jarno

On 09/01/2023 21.53, B wrote:
> 
>>> This made me wonder if a better approach here would be to move the logic that determines if LOST_TICK_POLICY_SLEW is available into the "lost_tick_policy" property setter defined at https://gitlab.com/qemu-project/qemu/-/blob/master/hw/core/qdev-properties-system.c#L558.
>>> If you look at the code directly below the link above you can see how set_blocksize() overrides the .set function for qdev_prop_blocksize to provide additional validation, which is similar to what we are trying to do here.
>>>
>>> I think it may be possible to come up with a similar solution for qdev_prop_losttickpolicy which makes use of the logic you suggested before i.e.
>>>
>>>       MachineState *ms = MACHINE(qdev_get_machine());
>>>
>>>       if (!object_dynamic_cast(OBJECT(ms), TYPE_X86_MACHINE)) {
>>>           ....
>>>       }
>>>
>>> which can then emit a suitable warning or return an error accordingly. A quick glance at hw/core/qdev-properties-system.c suggests there are a number of similar examples showing how this could be done.
>>
>> Thanks, I like that idea! I'll give it a try!
> 
> Does the microvm need consideration as well?

microvm is also derived from TYPE_X86_MACHINE, so I think the check that 
Mark suggested should be fine.

  Thomas



^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2023-01-10  7:53 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-03  8:47 [PATCH 0/6] mc146818rtc related clean-ups and improvements Thomas Huth
2023-01-03  8:47 ` [PATCH 1/6] hw/i386/pc: Create RTC controllers in south bridges Thomas Huth
2023-01-03  8:47 ` [PATCH 2/6] hw/i386/pc: No need for rtc_state to be an out-parameter Thomas Huth
2023-01-03 13:11   ` Philippe Mathieu-Daudé
2023-01-03  8:47 ` [PATCH 3/6] hw/intc: Extract the IRQ counting functions into a separate file Thomas Huth
2023-01-03 12:55   ` Bernhard Beschow
2023-01-03  8:47 ` [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy Thomas Huth
2023-01-03 13:10   ` Philippe Mathieu-Daudé
2023-01-03 13:32   ` Bernhard Beschow
2023-01-03 13:46     ` Bernhard Beschow
2023-01-03 15:00   ` Bernhard Beschow
2023-01-04  8:55   ` Mark Cave-Ayland
2023-01-09 20:12     ` Thomas Huth
2023-01-09 20:53       ` B
2023-01-10  7:52         ` Thomas Huth
2023-01-03  8:48 ` [PATCH 5/6] hw/rtc/mc146818rtc: Make the mc146818 RTC device target independent Thomas Huth
2023-01-03 12:58   ` Bernhard Beschow
2023-01-03  8:48 ` [PATCH 6/6] softmmu/rtc: Emit warning when using driftfix=slew on systems without mc146818 Thomas Huth
2023-01-03 13:08   ` Philippe Mathieu-Daudé

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.