Re: [PATCH v2 03/16] hw/i386/x86: Remove X86MachineClass::fwcfg_dma_enabled field

[Thomas Huth <thuth@redhat.com>]

On 02/05/2025 12.45, Philippe Mathieu-Daudé wrote:
> On 2/5/25 11:08, Mark Cave-Ayland wrote:
>> On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:
>>
>>> The X86MachineClass::fwcfg_dma_enabled boolean was only used
>>> by the pc-q35-2.6 and pc-i440fx-2.6 machines, which got
>>> removed. Remove it and simplify.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   include/hw/i386/x86.h | 2 --
>>>   hw/i386/microvm.c     | 3 ---
>>>   hw/i386/multiboot.c   | 7 +------
>>>   hw/i386/x86-common.c  | 3 +--
>>>   hw/i386/x86.c         | 2 --
>>>   5 files changed, 2 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
>>> index fc460b82f82..29d37af11e6 100644
>>> --- a/include/hw/i386/x86.h
>>> +++ b/include/hw/i386/x86.h
>>> @@ -29,8 +29,6 @@
>>>   struct X86MachineClass {
>>>       MachineClass parent;
>>> -    /* use DMA capable linuxboot option rom */
>>> -    bool fwcfg_dma_enabled;
>>>       /* CPU and apic information: */
>>>       bool apic_xrupt_override;
>>>   };
> 
> 
>>> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
>>> index 6e6b96bc345..bfa7e8f1e83 100644
>>> --- a/hw/i386/multiboot.c
>>> +++ b/hw/i386/multiboot.c
>>> @@ -153,7 +153,6 @@ int load_multiboot(X86MachineState *x86ms,
>>>                      int kernel_file_size,
>>>                      uint8_t *header)
>>>   {
>>> -    bool multiboot_dma_enabled = X86_MACHINE_GET_CLASS(x86ms)- 
>>> >fwcfg_dma_enabled;
>>>       int i, is_multiboot = 0;
>>>       uint32_t flags = 0;
>>>       uint32_t mh_entry_addr;
>>> @@ -402,11 +401,7 @@ int load_multiboot(X86MachineState *x86ms,
>>>       fw_cfg_add_bytes(fw_cfg, FW_CFG_INITRD_DATA, mb_bootinfo_data,
>>>                        sizeof(bootinfo));
>>> -    if (multiboot_dma_enabled) {
>>> -        option_rom[nb_option_roms].name = "multiboot_dma.bin";
>>> -    } else {
>>> -        option_rom[nb_option_roms].name = "multiboot.bin";
>>> -    }
>>> +    option_rom[nb_option_roms].name = "multiboot_dma.bin";
>>
>> Question: now that all machines support DMA-capable fw_cfg, does that mean 
>> that the non-DMA options roms above can also be removed?
> 
> All x86 machines, but there are still 2 not supporting it: HPPA and
> MIPS Loongson-3:
> 
> hw/hppa/machine.c:204:    fw_cfg = fw_cfg_init_mem(addr, addr + 4);
> 
> hw/mips/loongson3_virt.c:289:    fw_cfg = fw_cfg_init_mem_wide(cfg_addr, 
> cfg_addr + 8, 8, 0, NULL);
> 

But these don't use "multiboot.bin", do they? So I think you could remove 
pc-bios/multiboot.bin now from the repo?

Same question for "linuxboot.bin" : All users in hw/i386 seem to enable DMA, 
so fw_cfg_dma_enabled() should always return true here? If so, I think the 
normal "linuxboot.bin" could go away, too?

  Thomas