Re: [PATCH v5 15/28] hw/i386: Assume fw_cfg DMA is always enabled

[Igor Mammedov <imammedo@redhat.com>]

On Wed,  3 Dec 2025 00:28:22 +0800
Zhao Liu <zhao1.liu@intel.com> wrote:

> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Now all calls of x86 machines to fw_cfg_init_io_dma() pass DMA
> arguments, so the FWCfgState (FWCfgIoState) created by x86 machines
> enables DMA by default.
> 
> Although other callers of fw_cfg_init_io_dma() besides x86 also pass
> DMA arguments to create DMA-enabled FwCfgIoState, the "dma_enabled"
> property of FwCfgIoState cannot yet be removed, because Sun4u and Sun4v
> still create DMA-disabled FwCfgIoState (bypass fw_cfg_init_io_dma()) in
> sun4uv_init() (hw/sparc64/sun4u.c).
> 
> Maybe reusing fw_cfg_init_io_dma() for them would be a better choice, or
> adding fw_cfg_init_io_nodma(). However, before that, first simplify the
> handling of FwCfgState in x86.
> 
> Considering that FwCfgIoState in x86 enables DMA by default, remove the
> handling for DMA-disabled cases and replace DMA checks with assertions
> to ensure that the default DMA-enabled setting is not broken.
> 
> Then 'linuxboot.bin' isn't used anymore, and it will be removed in the
> next commit.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> Changes since v4:
>  * Keep "dma_enabled" property in fw_cfg_io_properties[].
>  * Replace DMA checks with assertions for x86 machines.
> ---
>  hw/i386/fw_cfg.c     | 16 ++++++++--------
>  hw/i386/x86-common.c |  6 ++----
>  2 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> index 5c0bcd5f8a9f..5670e8553eaa 100644
> --- a/hw/i386/fw_cfg.c
> +++ b/hw/i386/fw_cfg.c
> @@ -215,18 +215,18 @@ void fw_cfg_build_feature_control(MachineState *ms, FWCfgState *fw_cfg)
>  #ifdef CONFIG_ACPI
>  void fw_cfg_add_acpi_dsdt(Aml *scope, FWCfgState *fw_cfg)
>  {
> +    uint8_t io_size;
> +    Aml *dev = aml_device("FWCF");
> +    Aml *crs = aml_resource_template();
> +
>      /*
>       * when using port i/o, the 8-bit data register *always* overlaps
>       * with half of the 16-bit control register. Hence, the total size
> -     * of the i/o region used is FW_CFG_CTL_SIZE; when using DMA, the
> -     * DMA control register is located at FW_CFG_DMA_IO_BASE + 4
> +     * of the i/o region used is FW_CFG_CTL_SIZE; And the DMA control
> +     * register is located at FW_CFG_DMA_IO_BASE + 4
>       */
> -    Object *obj = OBJECT(fw_cfg);
> -    uint8_t io_size = object_property_get_bool(obj, "dma_enabled", NULL) ?
> -        ROUND_UP(FW_CFG_CTL_SIZE, 4) + sizeof(dma_addr_t) :
> -        FW_CFG_CTL_SIZE;
> -    Aml *dev = aml_device("FWCF");
> -    Aml *crs = aml_resource_template();
> +    assert(fw_cfg_dma_enabled(fw_cfg));

this was supposed to be machine agnostic, but given that it only used by x86
it shouldn't cause issues, so

Acked-by: Igor Mammedov <imammedo@redhat.com>


> +    io_size = ROUND_UP(FW_CFG_CTL_SIZE, 4) + sizeof(dma_addr_t);
>  
>      aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
>  
> diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
> index 1ee55382dab8..e8dc4d903bd6 100644
> --- a/hw/i386/x86-common.c
> +++ b/hw/i386/x86-common.c
> @@ -1002,10 +1002,8 @@ void x86_load_linux(X86MachineState *x86ms,
>      }
>  
>      option_rom[nb_option_roms].bootindex = 0;
> -    option_rom[nb_option_roms].name = "linuxboot.bin";
> -    if (fw_cfg_dma_enabled(fw_cfg)) {
> -        option_rom[nb_option_roms].name = "linuxboot_dma.bin";
> -    }
> +    assert(fw_cfg_dma_enabled(fw_cfg));
> +    option_rom[nb_option_roms].name = "linuxboot_dma.bin";
>      nb_option_roms++;
>  }
>