Re: [PULL 14/25] hw/i386/pc: Confine system flash handling to pc_sysfw

[Volker Rümelin]

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],