From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49969) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dW5lK-0005ZU-55 for qemu-devel@nongnu.org; Fri, 14 Jul 2017 14:56:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dW5lH-0004Iy-0y for qemu-devel@nongnu.org; Fri, 14 Jul 2017 14:56:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46943) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dW5lG-0004HY-ON for qemu-devel@nongnu.org; Fri, 14 Jul 2017 14:56:22 -0400 Date: Fri, 14 Jul 2017 15:56:16 -0300 From: Eduardo Habkost Message-ID: <20170714185616.GQ6020@localhost.localdomain> References: <1500025208-14827-1-git-send-email-mark.cave-ayland@ilande.co.uk> <1500025208-14827-4-git-send-email-mark.cave-ayland@ilande.co.uk> <20170714180907.GM6020@localhost.localdomain> <9584b0c7-96f4-f000-e53f-93accd8a39ad@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9584b0c7-96f4-f000-e53f-93accd8a39ad@redhat.com> Subject: Re: [Qemu-devel] [PATCHv9 3/3] fw_cfg: move QOM type defines and fw_cfg types into fw_cfg.h List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: Mark Cave-Ayland , qemu-devel@nongnu.org, somlo@cmu.edu, mst@redhat.com, pbonzini@redhat.com, rjones@redhat.com, imammedo@redhat.com, peter.maydell@linaro.org On Fri, Jul 14, 2017 at 08:28:37PM +0200, Laszlo Ersek wrote: > On 07/14/17 20:09, Eduardo Habkost wrote: > > On Fri, Jul 14, 2017 at 10:40:08AM +0100, Mark Cave-Ayland wrote: > >> By exposing FWCfgIoState and FWCfgMemState internals we allow the possibility > >> for the internal MemoryRegion fields to be mapped by name for boards that wish > >> to wire up the fw_cfg device themselves. > >> > >> Signed-off-by: Mark Cave-Ayland > >> Reviewed-by: Laszlo Ersek > > [...] > >> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > >> index b980cba..b77ea48 100644 > >> --- a/include/hw/nvram/fw_cfg.h > >> +++ b/include/hw/nvram/fw_cfg.h > >> @@ -1,8 +1,19 @@ > >> #ifndef FW_CFG_H > >> #define FW_CFG_H > >> > >> +#include "qemu/typedefs.h" > >> #include "exec/hwaddr.h" > >> #include "hw/nvram/fw_cfg_keys.h" > >> +#include "hw/sysbus.h" > >> +#include "sysemu/dma.h" > >> + > >> +#define TYPE_FW_CFG "fw_cfg" > >> +#define TYPE_FW_CFG_IO "fw_cfg_io" > >> +#define TYPE_FW_CFG_MEM "fw_cfg_mem" > >> + > >> +#define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG) > >> +#define FW_CFG_IO(obj) OBJECT_CHECK(FWCfgIoState, (obj), TYPE_FW_CFG_IO) > >> +#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM) > >> > >> typedef struct FWCfgFile { > >> uint32_t size; /* file size */ > >> @@ -35,6 +46,45 @@ typedef struct FWCfgDmaAccess { > >> > >> typedef void (*FWCfgReadCallback)(void *opaque); > >> > >> +struct FWCfgState { > >> + /*< private >*/ > >> + SysBusDevice parent_obj; > >> + /*< public >*/ > >> + > >> + uint16_t file_slots; > >> + FWCfgEntry *entries[2]; > >> + int *entry_order; > >> + FWCfgFiles *files; > >> + uint16_t cur_entry; > >> + uint32_t cur_offset; > >> + Notifier machine_ready; > >> + > >> + int fw_cfg_order_override; > >> + > >> + bool dma_enabled; > >> + dma_addr_t dma_addr; > >> + AddressSpace *dma_as; > >> + MemoryRegion dma_iomem; > >> +}; > >> + > >> +struct FWCfgIoState { > >> + /*< private >*/ > >> + FWCfgState parent_obj; > >> + /*< public >*/ > >> + > >> + MemoryRegion comb_iomem; > >> +}; > >> + > >> +struct FWCfgMemState { > >> + /*< private >*/ > >> + FWCfgState parent_obj; > >> + /*< public >*/ > >> + > >> + MemoryRegion ctl_iomem, data_iomem; > >> + uint32_t data_width; > >> + MemoryRegionOps wide_data_ops; > >> +}; > > > > Why do you need the full struct declaration to be exposed in the > > header? > > Different board code wants to hook up "comb_iomem" manually to different > address spaces, so they need to access the field directly. This is the > ultimate goal of the entire exercise, IIRC. > > > The memory regions are supposed to be visible as QOM > > children to the fw_cfg device, already. > > I don't understand this. How else can board code work with "comb_iomem" > than described above? If there is a way, I agree it would be preferable. object_resolve_path_component(fw_cfg, "fwcfg[0]") and object_resolve_path_component(fw_cfg, "fwcfg.dma[0]") should return fw_cfg->comb_iomem and fw_cfg->dma_iomem, respectively. I don't know why those names were chosen, though. Probably it's a good idea to call object_property_add_child() manually with more appropriate names inside the fw_cfg code instead of letting memory_region_init() pick the child name. -- Eduardo