Re: [Qemu-devel] [PATCH v4] Add optionrom compatible with fw_cfg DMA version

[Laszlo Ersek <lersek@redhat.com>]

On 02/02/16 15:23, Gerd Hoffmann wrote:
>   Hi,
> 
>> I don't remember discussing the topic of machine types when touching
>> fw_cfg DMA. Which means, it probably slipped amongst the other details.
>> But it is now merged and in stable, so it should probably be left as it
>> is now.
> 
> We have to fix it, it breaks live migration.  With fw_cfg_dma turned on
> we send an extra vmstate section.  qemu 2.4 (+older) will not understand
> it.  So we have to turn it off for those machine types.
> 
> dma_enabled property is there already, but the logic is wrong.  It
> defaults to false, but is flipped to true when the arch supports dma
> (i.e. on x86 and arm), unconditionally.  Instead it should default to
> true.  When set to false by the user or compat properties don't enable
> fw_cfg dma (and also flip it to false when dma is not supported by the
> arch).

I think the "dma_enabled" property is not exposed to the user. It is
internal to the fw_cfg implementation. The only reason we use a property
for this purpose because we want to pass a parameter to realize(), and
that's only possible via properties.

The default value of "dma_enabled" in both fw_cfg_io_properties and
fw_cfg_mem_properties is irrelevant; the actual property value is always
overwritten in fw_cfg_init_io_dma() and fw_cfg_init_mem_wide(), which
all of the init paths go through.

I agree that DMA capability should be filtered with machine type.
However, that distinction should not be made using the current
"dma_enabled" properties (i.e., of "fw_cfg_io_properties" and
"fw_cfg_mem_properties". Instead, it should be made in the
board-specific callers of fw_cfg_init_(io_dma|mem_wide). Those functions
are:

- create_fw_cfg() [hw/arm/virt.c]
- bochs_bios_init() [hw/i386/pc.c]

It looks like aarch64 virt machine types are not versioned yet, so that
leaves us with bochs_bios_init().

bochs_bios_init() is called by pc_memory_init(), and pc_memory_init() at
last has knowledge of machine type knobs -- it takes both "pcms" and
"guest_info".

"pcms" seems to handle the -machine options, but controlling this
feature from the command line is not really a goal here. Whereas machine
type compat flags arrive through "guest_info".

So I think we need the following:
- a new boolean field in PCMachineClass (not PCMachineState) called
  "fw_cfg_dma_disabled"
- In the pc_i440fx_2_4_machine_options() and
  pc_q35_2_4_machine_options() functions, this should be set to "true"
- The same field should be added to PcGuestInfo
- The pc_init1() and pc_q35_init() functions should copy the field from
  *pcmc to *guest_info
- The pc_memory_init() function should pass
  guest_info->fw_cfg_dma_disabled to bochs_bios_init()
- bochs_bios_init() should call fw_cfg_init_io() if the flag is "true".

Thanks
Laszlo

>> Should this optionrom be enabled only with the latest machine type?
> 
> The logic to pick the correct rom is fine.
> 
> cheers,
>   Gerd
> 
>