In the i386 PC machine, we want to run the pc_cmos_init_late()
function only once the IDE and floppy drive devices have been set up.
We currently do this using qemu_register_reset(), and then have the
function call qemu_unregister_reset() on itself, so it runs exactly
once.

This was an expedient way to do it back in 2010 when we first added
this (in commit c0897e0cb94e8), but now we have a more obvious point
to do "machine initialization that has to happen after generic device
init": the machine-init-done hook.

Do the pc_cmos_init_late() work from our existing PC machine init
done hook function, so we can drop the use of qemu_register_reset()
and qemu_unregister_reset().

Because the pointers to the devices we need (the IDE buses and the
RTC) are now all in the machine state, we don't need the
pc_cmos_init_late_arg struct and can just pass the PCMachineState
pointer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/i386/pc.c | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8b0f54e284c..4c3cfe9fc35 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -465,11 +465,6 @@ static void pc_cmos_init_floppy(MC146818RtcState *rtc_state, ISADevice *floppy)
     mc146818rtc_set_cmos_data(rtc_state, REG_EQUIPMENT_BYTE, val);
 }
 
-typedef struct pc_cmos_init_late_arg {
-    MC146818RtcState *rtc_state;
-    BusState *idebus[2];
-} pc_cmos_init_late_arg;
-
 typedef struct check_fdc_state {
     ISADevice *floppy;
     bool multiple;
@@ -530,23 +525,25 @@ static ISADevice *pc_find_fdc0(void)
     return state.floppy;
 }
 
-static void pc_cmos_init_late(void *opaque)
+static void pc_cmos_init_late(PCMachineState *pcms)
 {
-    pc_cmos_init_late_arg *arg = opaque;
-    MC146818RtcState *s = arg->rtc_state;
+    X86MachineState *x86ms = X86_MACHINE(pcms);
+    MC146818RtcState *s = MC146818_RTC(x86ms->rtc);
     int16_t cylinders;
     int8_t heads, sectors;
     int val;
     int i, trans;
 
     val = 0;
-    if (arg->idebus[0] && ide_get_geometry(arg->idebus[0], 0,
-                                           &cylinders, &heads, &sectors) >= 0) {
+    if (pcms->idebus[0] &&
+        ide_get_geometry(pcms->idebus[0], 0,
+                         &cylinders, &heads, &sectors) >= 0) {
         cmos_init_hd(s, 0x19, 0x1b, cylinders, heads, sectors);
         val |= 0xf0;
     }
-    if (arg->idebus[0] && ide_get_geometry(arg->idebus[0], 1,
-                                           &cylinders, &heads, &sectors) >= 0) {
+    if (pcms->idebus[0] &&
+        ide_get_geometry(pcms->idebus[0], 1,
+                         &cylinders, &heads, &sectors) >= 0) {
         cmos_init_hd(s, 0x1a, 0x24, cylinders, heads, sectors);
         val |= 0x0f;
     }
@@ -558,10 +555,11 @@ static void pc_cmos_init_late(void *opaque)
            geometry.  It is always such that: 1 <= sects <= 63, 1
            <= heads <= 16, 1 <= cylinders <= 16383. The BIOS
            geometry can be different if a translation is done. */
-        if (arg->idebus[i / 2] &&
-            ide_get_geometry(arg->idebus[i / 2], i % 2,
+        BusState *idebus = pcms->idebus[i / 2];
+        if (idebus &&
+            ide_get_geometry(idebus, i % 2,
                              &cylinders, &heads, &sectors) >= 0) {
-            trans = ide_get_bios_chs_trans(arg->idebus[i / 2], i % 2) - 1;
+            trans = ide_get_bios_chs_trans(idebus, i % 2) - 1;
             assert((trans & ~3) == 0);
             val |= trans << (i * 2);
         }
@@ -569,15 +567,12 @@ static void pc_cmos_init_late(void *opaque)
     mc146818rtc_set_cmos_data(s, 0x39, val);
 
     pc_cmos_init_floppy(s, pc_find_fdc0());
-
-    qemu_unregister_reset(pc_cmos_init_late, opaque);
 }
 
 void pc_cmos_init(PCMachineState *pcms,
                   ISADevice *rtc)
 {
     int val;
-    static pc_cmos_init_late_arg arg;
     X86MachineState *x86ms = X86_MACHINE(pcms);
     MC146818RtcState *s = MC146818_RTC(rtc);
 
@@ -631,11 +626,7 @@ void pc_cmos_init(PCMachineState *pcms,
     val |= 0x04; /* PS/2 mouse installed */
     mc146818rtc_set_cmos_data(s, REG_EQUIPMENT_BYTE, val);
 
-    /* hard drives and FDC */
-    arg.rtc_state = s;
-    arg.idebus[0] = pcms->idebus[0];
-    arg.idebus[1] = pcms->idebus[1];
-    qemu_register_reset(pc_cmos_init_late, &arg);
+    /* hard drives and FDC are handled by pc_cmos_init_late() */
 }
 
 static void handle_a20_line_change(void *opaque, int irq, int level)
@@ -703,6 +694,8 @@ void pc_machine_done(Notifier *notifier, void *data)
         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
         fw_cfg_modify_i16(x86ms->fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus);
     }
+
+    pc_cmos_init_late(pcms);
 }
 
 void pc_guest_info_init(PCMachineState *pcms)
-- 
2.34.1

On Tue, Feb 20, 2024 at 04:06:14PM +0000, Peter Maydell wrote:
> Date: Tue, 20 Feb 2024 16:06:14 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 02/10] hw/i386/pc: Do pc_cmos_init_late() from
>  pc_machine_done()
> X-Mailer: git-send-email 2.34.1
> 
> In the i386 PC machine, we want to run the pc_cmos_init_late()
> function only once the IDE and floppy drive devices have been set up.
> We currently do this using qemu_register_reset(), and then have the
> function call qemu_unregister_reset() on itself, so it runs exactly
> once.
> 
> This was an expedient way to do it back in 2010 when we first added
> this (in commit c0897e0cb94e8), but now we have a more obvious point
> to do "machine initialization that has to happen after generic device
> init": the machine-init-done hook.
> 
> Do the pc_cmos_init_late() work from our existing PC machine init
> done hook function, so we can drop the use of qemu_register_reset()
> and qemu_unregister_reset().
> 
> Because the pointers to the devices we need (the IDE buses and the
> RTC) are now all in the machine state, we don't need the
> pc_cmos_init_late_arg struct and can just pass the PCMachineState
> pointer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/i386/pc.c | 39 ++++++++++++++++-----------------------
>  1 file changed, 16 insertions(+), 23 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

On 20/2/24 17:06, Peter Maydell wrote:
> In the i386 PC machine, we want to run the pc_cmos_init_late()
> function only once the IDE and floppy drive devices have been set up.
> We currently do this using qemu_register_reset(), and then have the
> function call qemu_unregister_reset() on itself, so it runs exactly
> once.
> 
> This was an expedient way to do it back in 2010 when we first added
> this (in commit c0897e0cb94e8), but now we have a more obvious point
> to do "machine initialization that has to happen after generic device
> init": the machine-init-done hook.
> 
> Do the pc_cmos_init_late() work from our existing PC machine init
> done hook function, so we can drop the use of qemu_register_reset()
> and qemu_unregister_reset().
> 
> Because the pointers to the devices we need (the IDE buses and the
> RTC) are now all in the machine state, we don't need the
> pc_cmos_init_late_arg struct and can just pass the PCMachineState
> pointer.

Even if we remove the IDEBus/RTC pointers from PCMachineState,
we can still QOM-resolve them from it in pc_cmos_init_late().

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

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/i386/pc.c | 39 ++++++++++++++++-----------------------
>   1 file changed, 16 insertions(+), 23 deletions(-)

Am 20. Februar 2024 16:06:14 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>In the i386 PC machine, we want to run the pc_cmos_init_late()
>function only once the IDE and floppy drive devices have been set up.
>We currently do this using qemu_register_reset(), and then have the
>function call qemu_unregister_reset() on itself, so it runs exactly
>once.
>
>This was an expedient way to do it back in 2010 when we first added
>this (in commit c0897e0cb94e8), but now we have a more obvious point
>to do "machine initialization that has to happen after generic device
>init": the machine-init-done hook.
>
>Do the pc_cmos_init_late() work from our existing PC machine init
>done hook function, so we can drop the use of qemu_register_reset()
>and qemu_unregister_reset().
>
>Because the pointers to the devices we need (the IDE buses and the
>RTC) are now all in the machine state, we don't need the
>pc_cmos_init_late_arg struct and can just pass the PCMachineState
>pointer.
>
>Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>---
> hw/i386/pc.c | 39 ++++++++++++++++-----------------------
> 1 file changed, 16 insertions(+), 23 deletions(-)
>
>diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>index 8b0f54e284c..4c3cfe9fc35 100644
>--- a/hw/i386/pc.c
>+++ b/hw/i386/pc.c
>@@ -465,11 +465,6 @@ static void pc_cmos_init_floppy(MC146818RtcState *rtc_state, ISADevice *floppy)
>     mc146818rtc_set_cmos_data(rtc_state, REG_EQUIPMENT_BYTE, val);
> }
> 
>-typedef struct pc_cmos_init_late_arg {
>-    MC146818RtcState *rtc_state;
>-    BusState *idebus[2];
>-} pc_cmos_init_late_arg;
>-
> typedef struct check_fdc_state {
>     ISADevice *floppy;
>     bool multiple;
>@@ -530,23 +525,25 @@ static ISADevice *pc_find_fdc0(void)
>     return state.floppy;
> }
> 
>-static void pc_cmos_init_late(void *opaque)
>+static void pc_cmos_init_late(PCMachineState *pcms)
> {
>-    pc_cmos_init_late_arg *arg = opaque;
>-    MC146818RtcState *s = arg->rtc_state;
>+    X86MachineState *x86ms = X86_MACHINE(pcms);
>+    MC146818RtcState *s = MC146818_RTC(x86ms->rtc);
>     int16_t cylinders;
>     int8_t heads, sectors;
>     int val;
>     int i, trans;
> 
>     val = 0;
>-    if (arg->idebus[0] && ide_get_geometry(arg->idebus[0], 0,
>-                                           &cylinders, &heads, &sectors) >= 0) {
>+    if (pcms->idebus[0] &&
>+        ide_get_geometry(pcms->idebus[0], 0,
>+                         &cylinders, &heads, &sectors) >= 0) {
>         cmos_init_hd(s, 0x19, 0x1b, cylinders, heads, sectors);
>         val |= 0xf0;
>     }
>-    if (arg->idebus[0] && ide_get_geometry(arg->idebus[0], 1,
>-                                           &cylinders, &heads, &sectors) >= 0) {
>+    if (pcms->idebus[0] &&
>+        ide_get_geometry(pcms->idebus[0], 1,
>+                         &cylinders, &heads, &sectors) >= 0) {
>         cmos_init_hd(s, 0x1a, 0x24, cylinders, heads, sectors);
>         val |= 0x0f;
>     }
>@@ -558,10 +555,11 @@ static void pc_cmos_init_late(void *opaque)
>            geometry.  It is always such that: 1 <= sects <= 63, 1
>            <= heads <= 16, 1 <= cylinders <= 16383. The BIOS
>            geometry can be different if a translation is done. */
>-        if (arg->idebus[i / 2] &&
>-            ide_get_geometry(arg->idebus[i / 2], i % 2,
>+        BusState *idebus = pcms->idebus[i / 2];
>+        if (idebus &&
>+            ide_get_geometry(idebus, i % 2,
>                              &cylinders, &heads, &sectors) >= 0) {
>-            trans = ide_get_bios_chs_trans(arg->idebus[i / 2], i % 2) - 1;
>+            trans = ide_get_bios_chs_trans(idebus, i % 2) - 1;
>             assert((trans & ~3) == 0);
>             val |= trans << (i * 2);
>         }
>@@ -569,15 +567,12 @@ static void pc_cmos_init_late(void *opaque)
>     mc146818rtc_set_cmos_data(s, 0x39, val);
> 
>     pc_cmos_init_floppy(s, pc_find_fdc0());
>-
>-    qemu_unregister_reset(pc_cmos_init_late, opaque);
> }
> 
> void pc_cmos_init(PCMachineState *pcms,
>                   ISADevice *rtc)
> {
>     int val;
>-    static pc_cmos_init_late_arg arg;
>     X86MachineState *x86ms = X86_MACHINE(pcms);
>     MC146818RtcState *s = MC146818_RTC(rtc);
> 
>@@ -631,11 +626,7 @@ void pc_cmos_init(PCMachineState *pcms,
>     val |= 0x04; /* PS/2 mouse installed */
>     mc146818rtc_set_cmos_data(s, REG_EQUIPMENT_BYTE, val);
> 
>-    /* hard drives and FDC */
>-    arg.rtc_state = s;
>-    arg.idebus[0] = pcms->idebus[0];
>-    arg.idebus[1] = pcms->idebus[1];
>-    qemu_register_reset(pc_cmos_init_late, &arg);
>+    /* hard drives and FDC are handled by pc_cmos_init_late() */
> }
> 
> static void handle_a20_line_change(void *opaque, int irq, int level)
>@@ -703,6 +694,8 @@ void pc_machine_done(Notifier *notifier, void *data)
>         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
>         fw_cfg_modify_i16(x86ms->fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus);
>     }
>+
>+    pc_cmos_init_late(pcms);

Nice. With https://patchew.org/QEMU/20240208220349.4948-1-shentey@gmail.com/20240208220349.4948-9-shentey@gmail.com/ on top it might be possible to merge pc_cmos_init_late() and pc_cmos_init(), thus freeing pc_piix and pc_q35 entirely from having to deal with it.

Best regards,
Bernhard

> }
> 
> void pc_guest_info_init(PCMachineState *pcms)

On 2/20/24 06:06, Peter Maydell wrote:
> -static void pc_cmos_init_late(void *opaque)
> +static void pc_cmos_init_late(PCMachineState *pcms)
>   {
> -    pc_cmos_init_late_arg *arg = opaque;
> -    MC146818RtcState *s = arg->rtc_state;
> +    X86MachineState *x86ms = X86_MACHINE(pcms);

We've already done the X86_MACHINE resolution in pc_machine_done -- why not just pass it in?

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

On Tue, 20 Feb 2024 at 19:31, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/20/24 06:06, Peter Maydell wrote:
> > -static void pc_cmos_init_late(void *opaque)
> > +static void pc_cmos_init_late(PCMachineState *pcms)
> >   {
> > -    pc_cmos_init_late_arg *arg = opaque;
> > -    MC146818RtcState *s = arg->rtc_state;
> > +    X86MachineState *x86ms = X86_MACHINE(pcms);
>
> We've already done the X86_MACHINE resolution in pc_machine_done -- why not just pass it in?

We want both the PCMachineState and X86MachineState and I think
our usual style is not to pass in two arguments that are the
same object under different pointer types.

-- PMM