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