Re: [PATCH] vl, pc: turn -no-fd-bootchk into a machine property

[Bernhard Beschow]

Am 21. Februar 2024 09:04:21 UTC schrieb Paolo Bonzini <pbonzini@redhat.com>:
>On Tue, Feb 20, 2024 at 11:43 PM Bernhard Beschow <shentey@gmail.com> wrote:
>>
>>
>>
>> Am 20. Februar 2024 15:53:52 UTC schrieb Paolo Bonzini <pbonzini@redhat.com>:
>> >Add a fd-bootchk property to PC machine types, so that -no-fd-bootchk
>> >returns an error if the machine does not support booting from floppies
>> >and checking for boot signatures therein.
>> >
>> >Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> >Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> >---
>> > include/hw/i386/pc.h |  2 +-
>> > hw/i386/pc.c         | 30 +++++++++++++++++++++++++-----
>> > system/globals.c     |  1 -
>> > system/vl.c          |  2 +-
>> > qemu-options.hx      |  2 +-
>> > 5 files changed, 28 insertions(+), 9 deletions(-)
>> >
>> >diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> >index 02a0deedd3c..e5382a02e7a 100644
>> >--- a/include/hw/i386/pc.h
>> >+++ b/include/hw/i386/pc.h
>> >@@ -50,6 +50,7 @@ typedef struct PCMachineState {
>> >     bool hpet_enabled;
>> >     bool i8042_enabled;
>> >     bool default_bus_bypass_iommu;
>> >+    bool fd_bootchk;
>> >     uint64_t max_fw_size;
>> >
>> >     /* ACPI Memory hotplug IO base address */
>> >@@ -147,7 +148,6 @@ OBJECT_DECLARE_TYPE(PCMachineState, PCMachineClass, 
>> >PC_MACHINE)
>> > GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
>> >
>> > /* pc.c */
>> >-extern int fd_bootchk;
>> >
>> > void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
>> >
>> >diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> >index 28194014f82..31f4bb25a3e 100644
>> >--- a/hw/i386/pc.c
>> >+++ b/hw/i386/pc.c
>> >@@ -399,8 +399,8 @@ static int boot_device2nibble(char boot_device)
>> >     return 0;
>> > }
>> >
>> >-static void set_boot_dev(MC146818RtcState *s, const char *boot_device,
>> >-                         Error **errp)
>> >+static void set_boot_dev(PCMachineState *pcms, MC146818RtcState *s,
>> >+                         const char *boot_device, Error **errp)
>> > {
>> > #define PC_MAX_BOOT_DEVICES 3
>> >     int nbds, bds[3] = { 0, };
>> >@@ -420,12 +420,14 @@ static void set_boot_dev(MC146818RtcState *s, const 
>> >char *boot_device,
>> >         }
>> >     }
>> >     mc146818rtc_set_cmos_data(s, 0x3d, (bds[1] << 4) | bds[0]);
>> >-    mc146818rtc_set_cmos_data(s, 0x38, (bds[2] << 4) | (fd_bootchk ? 0x0 : 
>> >0x1));
>> >+    mc146818rtc_set_cmos_data(s, 0x38, (bds[2] << 4) | !pcms->fd_bootchk);
>> > }
>> >
>> > static void pc_boot_set(void *opaque, const char *boot_device, Error 
>> > **errp)
>> > {
>> >-    set_boot_dev(opaque, boot_device, errp);
>> >+    PCMachineState *pcms = PC_MACHINE(current_machine);
>> >+
>> >+    set_boot_dev(pcms, opaque, boot_device, errp);
>> > }
>> >
>> > static void pc_cmos_init_floppy(MC146818RtcState *rtc_state, ISADevice 
>> > *floppy)
>> >@@ -617,6 +619,9 @@ 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_bool(obj, "fd-bootchk", pc_machine_get_fd_bootchk,
>> >+                             pc_machine_set_fd_bootchk);
>>
>> Isn't it possible to turn this into a class property or add the property in 
>> pc_machine_initfn()? Aggregating properties in one place seems more 
>> comprehensible to me.
>
>Sure, I placed it in pc_cmos_init because rtc_state is already created here.

Great! I'll rebase my PC initialization series on top of Peter's reset cleanup 
series which probably results in folding cmos initialization into pc.c.

Best regards,
Bernhard

>
>Paolo
>