Am 20. Februar 2024 16:06:14 UTC schrieb Peter Maydell
<peter.mayd...@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.mayd...@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, §ors) >=
>0) {
>+ if (pcms->idebus[0] &&
>+ ide_get_geometry(pcms->idebus[0], 0,
>+ &cylinders, &heads, §ors) >= 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, §ors) >=
>0) {
>+ if (pcms->idebus[0] &&
>+ ide_get_geometry(pcms->idebus[0], 1,
>+ &cylinders, &heads, §ors) >= 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, §ors) >= 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-shen...@gmail.com/20240208220349.4948-9-shen...@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)
