| Message ID | 20240221211626.48190-15-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/25] hw/input/pckbd: Open-code i8042_setup_a20_line() wrapper | expand |
Am 21.02.24 um 22:16 schrieb Philippe Mathieu-Daudé: > From: Bernhard Beschow <shentey@gmail.com> > > Rather than distributing PC system flash handling across three files, let's > confine it to one. Now, pc_system_firmware_init() creates, configures and cleans > up the system flash which makes the code easier to understand. It also avoids > the extra call to pc_system_flash_cleanup_unused() in the Xen case. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Message-ID: <20240208220349.4948-7-shentey@gmail.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > include/hw/i386/pc.h | 2 -- > hw/i386/pc.c | 1 - > hw/i386/pc_piix.c | 1 - > hw/i386/pc_sysfw.c | 6 ++++-- > 4 files changed, 4 insertions(+), 6 deletions(-) Hi Bernhard, this patch breaks QEMU on my system. ./qemu-system-x86_64 -machine q35,pflash0=pflash0-storage -blockdev driver=file,node-name=pflash0-storage,filename=/usr/share/qemu/ovmf-x86_64.bin,read-only=true qemu-system-x86_64: Property 'pc-q35-9.0-machine.pflash0' not found I had to revert cb05cc1602 ("hw/i386/pc_sysfw: Inline pc_system_flash_create() and remove it") and 6f6ad2b245 ("hw/i386/pc: Confine system flash handling to pc_sysfw") to make it work again. With best regards, Volker > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 0a8a96600c..e8f4af5d5c 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -193,8 +193,6 @@ void pc_i8259_create(ISABus *isa_bus, qemu_irq *i8259_irqs); > #define TYPE_PORT92 "port92" > > /* pc_sysfw.c */ > -void pc_system_flash_create(PCMachineState *pcms); > -void pc_system_flash_cleanup_unused(PCMachineState *pcms); > void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory); > bool pc_system_ovmf_table_find(const char *entry, uint8_t **data, > int *data_len); > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index e526498164..1ee41a5e56 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1733,7 +1733,6 @@ static void pc_machine_initfn(Object *obj) > #endif > pcms->default_bus_bypass_iommu = false; > > - pc_system_flash_create(pcms); > pcms->pcspk = isa_new(TYPE_PC_SPEAKER); > object_property_add_alias(OBJECT(pcms), "pcspk-audiodev", > OBJECT(pcms->pcspk), "audiodev"); > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 34203927e1..ec7c07b362 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -231,7 +231,6 @@ static void pc_init1(MachineState *machine, > assert(machine->ram_size == x86ms->below_4g_mem_size + > x86ms->above_4g_mem_size); > > - pc_system_flash_cleanup_unused(pcms); > if (machine->kernel_filename != NULL) { > /* For xen HVM direct kernel boot, load linux here */ > xen_load_linux(pcms); > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c > index c8d9e71b88..b4c3833352 100644 > --- a/hw/i386/pc_sysfw.c > +++ b/hw/i386/pc_sysfw.c > @@ -91,7 +91,7 @@ static PFlashCFI01 *pc_pflash_create(PCMachineState *pcms, > return PFLASH_CFI01(dev); > } > > -void pc_system_flash_create(PCMachineState *pcms) > +static void pc_system_flash_create(PCMachineState *pcms) > { > PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); > > @@ -103,7 +103,7 @@ void pc_system_flash_create(PCMachineState *pcms) > } > } > > -void pc_system_flash_cleanup_unused(PCMachineState *pcms) > +static void pc_system_flash_cleanup_unused(PCMachineState *pcms) > { > char *prop_name; > int i; > @@ -212,6 +212,8 @@ void pc_system_firmware_init(PCMachineState *pcms, > return; > } > > + pc_system_flash_create(pcms); > + > /* Map legacy -drive if=pflash to machine properties */ > for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) { > pflash_cfi01_legacy_drive(pcms->flash[i],
Am 25. Februar 2024 13:03:46 UTC schrieb "Volker Rümelin" <vr_qemu@t-online.de>: >Am 21.02.24 um 22:16 schrieb Philippe Mathieu-Daudé: >> From: Bernhard Beschow <shentey@gmail.com> >> >> Rather than distributing PC system flash handling across three files, let's >> confine it to one. Now, pc_system_firmware_init() creates, configures and cleans >> up the system flash which makes the code easier to understand. It also avoids >> the extra call to pc_system_flash_cleanup_unused() in the Xen case. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Message-ID: <20240208220349.4948-7-shentey@gmail.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> include/hw/i386/pc.h | 2 -- >> hw/i386/pc.c | 1 - >> hw/i386/pc_piix.c | 1 - >> hw/i386/pc_sysfw.c | 6 ++++-- >> 4 files changed, 4 insertions(+), 6 deletions(-) > >Hi Bernhard, Hi Volker, > >this patch breaks QEMU on my system. > >./qemu-system-x86_64 -machine q35,pflash0=pflash0-storage -blockdev >driver=file,node-name=pflash0-storage,filename=/usr/share/qemu/ovmf-x86_64.bin,read-only=true >qemu-system-x86_64: Property 'pc-q35-9.0-machine.pflash0' not found > >I had to revert cb05cc1602 ("hw/i386/pc_sysfw: Inline >pc_system_flash_create() and remove it") and 6f6ad2b245 ("hw/i386/pc: >Confine system flash handling to pc_sysfw") to make it work again. In my tests I've followed https://wiki.archlinux.org/title/QEMU#Booting_in_UEFI_mode for both pc and q35 machine, and therefore missed the machine properties. If there is no way to fix it with compat properties or the like, I propose to revert the two patches until a different solution is found. Best regards, Bernhard > >With best regards, >Volker > >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h >> index 0a8a96600c..e8f4af5d5c 100644 >> --- a/include/hw/i386/pc.h >> +++ b/include/hw/i386/pc.h >> @@ -193,8 +193,6 @@ void pc_i8259_create(ISABus *isa_bus, qemu_irq *i8259_irqs); >> #define TYPE_PORT92 "port92" >> >> /* pc_sysfw.c */ >> -void pc_system_flash_create(PCMachineState *pcms); >> -void pc_system_flash_cleanup_unused(PCMachineState *pcms); >> void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory); >> bool pc_system_ovmf_table_find(const char *entry, uint8_t **data, >> int *data_len); >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index e526498164..1ee41a5e56 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -1733,7 +1733,6 @@ static void pc_machine_initfn(Object *obj) >> #endif >> pcms->default_bus_bypass_iommu = false; >> >> - pc_system_flash_create(pcms); >> pcms->pcspk = isa_new(TYPE_PC_SPEAKER); >> object_property_add_alias(OBJECT(pcms), "pcspk-audiodev", >> OBJECT(pcms->pcspk), "audiodev"); >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >> index 34203927e1..ec7c07b362 100644 >> --- a/hw/i386/pc_piix.c >> +++ b/hw/i386/pc_piix.c >> @@ -231,7 +231,6 @@ static void pc_init1(MachineState *machine, >> assert(machine->ram_size == x86ms->below_4g_mem_size + >> x86ms->above_4g_mem_size); >> >> - pc_system_flash_cleanup_unused(pcms); >> if (machine->kernel_filename != NULL) { >> /* For xen HVM direct kernel boot, load linux here */ >> xen_load_linux(pcms); >> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c >> index c8d9e71b88..b4c3833352 100644 >> --- a/hw/i386/pc_sysfw.c >> +++ b/hw/i386/pc_sysfw.c >> @@ -91,7 +91,7 @@ static PFlashCFI01 *pc_pflash_create(PCMachineState *pcms, >> return PFLASH_CFI01(dev); >> } >> >> -void pc_system_flash_create(PCMachineState *pcms) >> +static void pc_system_flash_create(PCMachineState *pcms) >> { >> PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); >> >> @@ -103,7 +103,7 @@ void pc_system_flash_create(PCMachineState *pcms) >> } >> } >> >> -void pc_system_flash_cleanup_unused(PCMachineState *pcms) >> +static void pc_system_flash_cleanup_unused(PCMachineState *pcms) >> { >> char *prop_name; >> int i; >> @@ -212,6 +212,8 @@ void pc_system_firmware_init(PCMachineState *pcms, >> return; >> } >> >> + pc_system_flash_create(pcms); >> + >> /* Map legacy -drive if=pflash to machine properties */ >> for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) { >> pflash_cfi01_legacy_drive(pcms->flash[i], >
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 0a8a96600c..e8f4af5d5c 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -193,8 +193,6 @@ void pc_i8259_create(ISABus *isa_bus, qemu_irq *i8259_irqs); #define TYPE_PORT92 "port92" /* pc_sysfw.c */ -void pc_system_flash_create(PCMachineState *pcms); -void pc_system_flash_cleanup_unused(PCMachineState *pcms); void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory); bool pc_system_ovmf_table_find(const char *entry, uint8_t **data, int *data_len); diff --git a/hw/i386/pc.c b/hw/i386/pc.c index e526498164..1ee41a5e56 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1733,7 +1733,6 @@ static void pc_machine_initfn(Object *obj) #endif pcms->default_bus_bypass_iommu = false; - pc_system_flash_create(pcms); pcms->pcspk = isa_new(TYPE_PC_SPEAKER); object_property_add_alias(OBJECT(pcms), "pcspk-audiodev", OBJECT(pcms->pcspk), "audiodev"); diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 34203927e1..ec7c07b362 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -231,7 +231,6 @@ static void pc_init1(MachineState *machine, assert(machine->ram_size == x86ms->below_4g_mem_size + x86ms->above_4g_mem_size); - pc_system_flash_cleanup_unused(pcms); if (machine->kernel_filename != NULL) { /* For xen HVM direct kernel boot, load linux here */ xen_load_linux(pcms); diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c index c8d9e71b88..b4c3833352 100644 --- a/hw/i386/pc_sysfw.c +++ b/hw/i386/pc_sysfw.c @@ -91,7 +91,7 @@ static PFlashCFI01 *pc_pflash_create(PCMachineState *pcms, return PFLASH_CFI01(dev); } -void pc_system_flash_create(PCMachineState *pcms) +static void pc_system_flash_create(PCMachineState *pcms) { PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); @@ -103,7 +103,7 @@ void pc_system_flash_create(PCMachineState *pcms) } } -void pc_system_flash_cleanup_unused(PCMachineState *pcms) +static void pc_system_flash_cleanup_unused(PCMachineState *pcms) { char *prop_name; int i; @@ -212,6 +212,8 @@ void pc_system_firmware_init(PCMachineState *pcms, return; } + pc_system_flash_create(pcms); + /* Map legacy -drive if=pflash to machine properties */ for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) { pflash_cfi01_legacy_drive(pcms->flash[i],