| Message ID | 20240220160622.114437-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reset: Make whole system three-phase-reset aware | expand |
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
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, §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-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 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(-)
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>
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); } void pc_guest_info_init(PCMachineState *pcms)
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(-)