[PATCH v3 0/4] Simplify initialization of PC machines

[PATCH v3 0/4] Simplify initialization of PC machines

[Bernhard Beschow]

The series aims to simplify the initialization process of all PC-based machines
by streamlining redundant code.

Since I haven't seen patches on the list so far for folding CMOS data
generation into pc.c, which frees all PC machines from performing this duty
explicitly, I've appended this cleanup as the last two patches.

Testing done:
* `make check` -> all tests pass
* `make check-avocado`
* I'm sending this series from within a VM containing these changes.

v3:
* Rebase onto master
* Fix boot-order-test
* Remove the "rtc_state" link again which got accidently reintroduced
* Add patch avoiding one use of the current_machine global

v2:
* Rebase onto master, leaving only patches 1, 3, and 5
* Patch 2: Rename "bus" attribute to "pcibus" (Phil)
* Patch 4: Spotted while rebasing
* Patch 6: New patch possible after [1]

Best regards,
Bernhard

[1]
https://patchew.org/QEMU/20240221211626.48190-1-philmd@linaro.org/20240221211626
.48190-10-philmd@linaro.org/

Bernhard Beschow (4):
  hw/i386/pc: Remove "rtc_state" link again
  hw/i386/pc: Avoid one use of the current_machine global
  hw/i386/pc: Set "normal" boot device order in pc_basic_device_init()
  hw/i386/pc: Inline pc_cmos_init() into pc_cmos_init_late() and remove
    it

 include/hw/i386/pc.h |  2 --
 hw/i386/pc.c         | 29 ++++++-----------------------
 hw/i386/pc_piix.c    |  2 --
 hw/i386/pc_q35.c     |  2 --
 4 files changed, 6 insertions(+), 29 deletions(-)

-- 
2.44.0

[PATCH v3 1/4] hw/i386/pc: Remove "rtc_state" link again

[Bernhard Beschow]

Commit 99e1c1137b6f "hw/i386/pc: Populate RTC attribute directly" made linking
the "rtc_state" property unnecessary and removed it. Commit 84e945aad2d0 "vl,
pc: turn -no-fd-bootchk into a machine property" accidently reintroduced the
link. Remove it again since it is not needed.

Fixes: 84e945aad2d0 "vl, pc: turn -no-fd-bootchk into a machine property"
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f5ff970acf..63b7583af0 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -613,14 +613,6 @@ void pc_cmos_init(PCMachineState *pcms,
     mc146818rtc_set_cmos_data(s, 0x5c, val >> 8);
     mc146818rtc_set_cmos_data(s, 0x5d, val >> 16);
 
-    object_property_add_link(OBJECT(pcms), "rtc_state",
-                             TYPE_ISA_DEVICE,
-                             (Object **)&x86ms->rtc,
-                             object_property_allow_set_link,
-                             OBJ_PROP_LINK_STRONG);
-    object_property_set_link(OBJECT(pcms), "rtc_state", OBJECT(s),
-                             &error_abort);
-
     set_boot_dev(pcms, s, MACHINE(pcms)->boot_config.order, &error_fatal);
 
     val = 0;
-- 
2.44.0

Re: [PATCH v3 1/4] hw/i386/pc: Remove "rtc_state" link again

[Philippe Mathieu-Daudé]

On 3/3/24 19:53, Bernhard Beschow wrote:
> Commit 99e1c1137b6f "hw/i386/pc: Populate RTC attribute directly" made linking
> the "rtc_state" property unnecessary and removed it. Commit 84e945aad2d0 "vl,
> pc: turn -no-fd-bootchk into a machine property" accidently reintroduced the
> link. Remove it again since it is not needed.

Argh 🤦

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

> Fixes: 84e945aad2d0 "vl, pc: turn -no-fd-bootchk into a machine property"
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/i386/pc.c | 8 --------
>   1 file changed, 8 deletions(-)

Re: [PATCH v3 1/4] hw/i386/pc: Remove "rtc_state" link again

[Peter Maydell]

On Sun, 3 Mar 2024 at 18:55, Bernhard Beschow <shentey@gmail.com> wrote:
>
> Commit 99e1c1137b6f "hw/i386/pc: Populate RTC attribute directly" made linking
> the "rtc_state" property unnecessary and removed it. Commit 84e945aad2d0 "vl,
> pc: turn -no-fd-bootchk into a machine property" accidently reintroduced the
> link. Remove it again since it is not needed.
>
> Fixes: 84e945aad2d0 "vl, pc: turn -no-fd-bootchk into a machine property"
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Ah, I did wonder when I was working with this code whether that
rtc_state link was really necessary.

We could now remove the rtc argument from the pc_cmos_init()
function, because we can guarantee that it's in x86ms->rtc,
which the function already has access to from its pcms argument.

thanks
-- PMM

Re: [PATCH v3 1/4] hw/i386/pc: Remove "rtc_state" link again

[Bernhard Beschow]

Am 5. März 2024 15:57:08 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>On Sun, 3 Mar 2024 at 18:55, Bernhard Beschow <shentey@gmail.com> wrote:
>>
>> Commit 99e1c1137b6f "hw/i386/pc: Populate RTC attribute directly" made linking
>> the "rtc_state" property unnecessary and removed it. Commit 84e945aad2d0 "vl,
>> pc: turn -no-fd-bootchk into a machine property" accidently reintroduced the
>> link. Remove it again since it is not needed.
>>
>> Fixes: 84e945aad2d0 "vl, pc: turn -no-fd-bootchk into a machine property"
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>
>Ah, I did wonder when I was working with this code whether that
>rtc_state link was really necessary.
>
>We could now remove the rtc argument from the pc_cmos_init()
>function, because we can guarantee that it's in x86ms->rtc,
>which the function already has access to from its pcms argument.

This series goes one step further and inlines this function. It would be a nice to get some feedback before the freeze.

Best regards,
Bernhard

>
>thanks
>-- PMM

Re: [PATCH v3 1/4] hw/i386/pc: Remove "rtc_state" link again

[Michael S. Tsirkin]

On Tue, Mar 05, 2024 at 07:54:40PM +0000, Bernhard Beschow wrote:
> 
> 
> Am 5. März 2024 15:57:08 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
> >On Sun, 3 Mar 2024 at 18:55, Bernhard Beschow <shentey@gmail.com> wrote:
> >>
> >> Commit 99e1c1137b6f "hw/i386/pc: Populate RTC attribute directly" made linking
> >> the "rtc_state" property unnecessary and removed it. Commit 84e945aad2d0 "vl,
> >> pc: turn -no-fd-bootchk into a machine property" accidently reintroduced the
> >> link. Remove it again since it is not needed.
> >>
> >> Fixes: 84e945aad2d0 "vl, pc: turn -no-fd-bootchk into a machine property"
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> >
> >Ah, I did wonder when I was working with this code whether that
> >rtc_state link was really necessary.
> >
> >We could now remove the rtc argument from the pc_cmos_init()
> >function, because we can guarantee that it's in x86ms->rtc,
> >which the function already has access to from its pcms argument.
> 
> This series goes one step further and inlines this function. It would be a nice to get some feedback before the freeze.
> 
> Best regards,
> Bernhard

I like it, I tagged it for merge.

> >
> >thanks
> >-- PMM

[PATCH v3 2/4] hw/i386/pc: Avoid one use of the current_machine global

[Bernhard Beschow]

The RTC can be accessed through the X86 machine instance, so rather than passing
the RTC it's possible to pass the machine state instead. This avoids
pc_boot_set() from having to access the current_machine global.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 63b7583af0..06ef278b34 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -425,9 +425,10 @@ static void set_boot_dev(PCMachineState *pcms, MC146818RtcState *s,
 
 static void pc_boot_set(void *opaque, const char *boot_device, Error **errp)
 {
-    PCMachineState *pcms = PC_MACHINE(current_machine);
+    PCMachineState *pcms = opaque;
+    X86MachineState *x86ms = X86_MACHINE(pcms);
 
-    set_boot_dev(pcms, opaque, boot_device, errp);
+    set_boot_dev(pcms, MC146818_RTC(x86ms->rtc), boot_device, errp);
 }
 
 static void pc_cmos_init_floppy(MC146818RtcState *rtc_state, ISADevice *floppy)
@@ -1252,7 +1253,7 @@ void pc_basic_device_init(struct PCMachineState *pcms,
     }
 #endif
 
-    qemu_register_boot_set(pc_boot_set, rtc_state);
+    qemu_register_boot_set(pc_boot_set, pcms);
 
     if (!xen_enabled() &&
         (x86ms->pit == ON_OFF_AUTO_AUTO || x86ms->pit == ON_OFF_AUTO_ON)) {
-- 
2.44.0

Re: [PATCH v3 2/4] hw/i386/pc: Avoid one use of the current_machine global

[Philippe Mathieu-Daudé]

On 3/3/24 19:53, Bernhard Beschow wrote:
> The RTC can be accessed through the X86 machine instance, so rather than passing
> the RTC it's possible to pass the machine state instead. This avoids
> pc_boot_set() from having to access the current_machine global.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/i386/pc.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)

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

[PATCH v3 3/4] hw/i386/pc: Set "normal" boot device order in pc_basic_device_init()

[Bernhard Beschow]

The boot device order may change during the lifetime of a VM. Usually, the
"normal" order is set once during machine init(). However, if a user specifies
`-boot once=...`, the "normal" order is overwritten by the "once" order just
before machine_done, and a reset handler is registered which restores the
"normal" order during the next reset.

In the next patch, pc_cmos_init() will be inlined into pc_cmos_init_late() which
runs during machine_done. This means that the "once" boot order would be
overwritten again with the "normal" boot order -- which renders the user's
choice ineffective. Fix this by setting the "normal" boot order in
pc_basic_device_init() which already registers the boot_set() handler.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 06ef278b34..7780d8d6dd 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -614,8 +614,6 @@ void pc_cmos_init(PCMachineState *pcms,
     mc146818rtc_set_cmos_data(s, 0x5c, val >> 8);
     mc146818rtc_set_cmos_data(s, 0x5d, val >> 16);
 
-    set_boot_dev(pcms, s, MACHINE(pcms)->boot_config.order, &error_fatal);
-
     val = 0;
     val |= 0x02; /* FPU is there */
     val |= 0x04; /* PS/2 mouse installed */
@@ -1254,6 +1252,8 @@ void pc_basic_device_init(struct PCMachineState *pcms,
 #endif
 
     qemu_register_boot_set(pc_boot_set, pcms);
+    set_boot_dev(pcms, MC146818_RTC(rtc_state),
+                 MACHINE(pcms)->boot_config.order, &error_fatal);
 
     if (!xen_enabled() &&
         (x86ms->pit == ON_OFF_AUTO_AUTO || x86ms->pit == ON_OFF_AUTO_ON)) {
-- 
2.44.0

[PATCH v3 4/4] hw/i386/pc: Inline pc_cmos_init() into pc_cmos_init_late() and remove it

[Bernhard Beschow]

Now that pc_cmos_init() doesn't populate the X86MachineState::rtc attribute any
longer, its duties can be merged into pc_cmos_init_late() which is called within
machine_done notifier. This frees pc_piix and pc_q35 from explicit CMOS
initialization.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/pc.h |  2 --
 hw/i386/pc.c         | 10 ----------
 hw/i386/pc_piix.c    |  2 --
 hw/i386/pc_q35.c     |  2 --
 4 files changed, 16 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 5065590281..bfdcd64514 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -178,8 +178,6 @@ void pc_basic_device_init(struct PCMachineState *pcms,
                           ISADevice *rtc_state,
                           bool create_fdctrl,
                           uint32_t hpet_irqs);
-void pc_cmos_init(PCMachineState *pcms,
-                  ISADevice *s);
 void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus);
 
 void pc_i8259_create(ISABus *isa_bus, qemu_irq *i8259_irqs);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7780d8d6dd..69e134e141 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -570,14 +570,6 @@ static void pc_cmos_init_late(PCMachineState *pcms)
     mc146818rtc_set_cmos_data(s, 0x39, val);
 
     pc_cmos_init_floppy(s, pc_find_fdc0());
-}
-
-void pc_cmos_init(PCMachineState *pcms,
-                  ISADevice *rtc)
-{
-    int val;
-    X86MachineState *x86ms = X86_MACHINE(pcms);
-    MC146818RtcState *s = MC146818_RTC(rtc);
 
     /* various important CMOS locations needed by PC/Bochs bios */
 
@@ -618,8 +610,6 @@ void pc_cmos_init(PCMachineState *pcms,
     val |= 0x02; /* FPU is there */
     val |= 0x04; /* PS/2 mouse installed */
     mc146818rtc_set_cmos_data(s, REG_EQUIPMENT_BYTE, val);
-
-    /* hard drives and FDC are handled by pc_cmos_init_late() */
 }
 
 static void handle_a20_line_change(void *opaque, int irq, int level)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index ce6aad758d..637f4d38be 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -342,8 +342,6 @@ static void pc_init1(MachineState *machine,
     }
 #endif
 
-    pc_cmos_init(pcms, x86ms->rtc);
-
     if (piix4_pm) {
         smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 45a4102e75..bccd13d162 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -311,8 +311,6 @@ static void pc_q35_init(MachineState *machine)
         smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
     }
 
-    pc_cmos_init(pcms, x86ms->rtc);
-
     /* the rest devices to which pci devfn is automatically assigned */
     pc_vga_init(isa_bus, pcms->pcibus);
     pc_nic_init(pcmc, isa_bus, pcms->pcibus);
-- 
2.44.0