From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36216) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQcVt-0007QX-1l for qemu-devel@nongnu.org; Tue, 02 Feb 2016 10:05:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQcVp-0007pm-O1 for qemu-devel@nongnu.org; Tue, 02 Feb 2016 10:05:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57102) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQcVp-0007pi-Fy for qemu-devel@nongnu.org; Tue, 02 Feb 2016 10:05:01 -0500 References: <1454065944-15887-1-git-send-email-markmb@redhat.com> <1454411187.9300.54.camel@redhat.com> <1454414695.9300.57.camel@redhat.com> <20160202140733.6cef6568@crunchbang> <1454423012.9300.99.camel@redhat.com> From: Laszlo Ersek Message-ID: <56B0C59A.6070008@redhat.com> Date: Tue, 2 Feb 2016 16:04:58 +0100 MIME-Version: 1.0 In-Reply-To: <1454423012.9300.99.camel@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4] Add optionrom compatible with fw_cfg DMA version List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann , =?UTF-8?Q?Marc_Mar=c3=ad?= Cc: "Gabriel L. Somlo" , qemu-devel@nongnu.org, Kevin O'Connor , Stefan Hajnoczi , Paolo Bonzini , =?UTF-8?Q?Marc_Mar=c3=ad?= 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 > >