From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37863) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0qtZ-0008DX-6L for qemu-devel@nongnu.org; Tue, 16 Dec 2014 07:06:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0qtR-0002PI-NF for qemu-devel@nongnu.org; Tue, 16 Dec 2014 07:06:29 -0500 Received: from cantor2.suse.de ([195.135.220.15]:44932 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0qtR-0002Ow-ET for qemu-devel@nongnu.org; Tue, 16 Dec 2014 07:06:21 -0500 Message-ID: <5490203B.2030001@suse.de> Date: Tue, 16 Dec 2014 13:06:19 +0100 From: Alexander Graf MIME-Version: 1.0 References: <1418399932-7658-1-git-send-email-lersek@redhat.com> <1418399932-7658-4-git-send-email-lersek@redhat.com> In-Reply-To: <1418399932-7658-4-git-send-email-lersek@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 3/8] fw_cfg: introduce the "data_memwidth" property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek , peter.maydell@linaro.org, qemu-devel@nongnu.org, rjones@redhat.com, drjones@redhat.com On 12.12.14 16:58, Laszlo Ersek wrote: > The "data_memwidth" property is capable of changing the maximum valid > access size to the MMIO data register, and (corresponding to the previous > patch) resizes the memory region similarly, at device realization time. > > (Because "data_iomem" is configured and installed dynamically now, we must > delay those steps to the realize callback.) > > The default value of "data_memwidth" is set so that we don't yet diverge > from "fw_cfg_data_mem_ops". > > Most of the fw_cfg users will stick with the default, and for them we > should continue using the statically allocated "fw_cfg_data_mem_ops". This > is beneficial for debugging because gdb can resolve pointers referencing > static objects to the names of those objects. > > Signed-off-by: Laszlo Ersek > --- > > Notes: > v4: > - reject I/O port combining if data register is wider than 1 byte > [Peter] > > v3: > - new in v3 [Drew Jones] > > hw/nvram/fw_cfg.c | 24 ++++++++++++++++++------ > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index eb0ad83..0947136 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -50,8 +50,9 @@ struct FWCfgState { > /*< public >*/ > > MemoryRegion ctl_iomem, data_iomem, comb_iomem; > uint32_t ctl_iobase, data_iobase; > + uint32_t data_memwidth; > FWCfgEntry entries[2][FW_CFG_MAX_ENTRY]; > FWCfgFiles *files; > uint16_t cur_entry; > uint32_t cur_offset; > @@ -569,8 +570,10 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port, > > dev = qdev_create(NULL, TYPE_FW_CFG); > qdev_prop_set_uint32(dev, "ctl_iobase", ctl_port); > qdev_prop_set_uint32(dev, "data_iobase", data_port); > + qdev_prop_set_uint32(dev, "data_memwidth", > + fw_cfg_data_mem_ops.valid.max_access_size); > d = SYS_BUS_DEVICE(dev); > > s = FW_CFG(dev); > > @@ -607,12 +610,8 @@ static void fw_cfg_initfn(Object *obj) > > memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops, s, > "fwcfg.ctl", FW_CFG_SIZE); > sysbus_init_mmio(sbd, &s->ctl_iomem); > - memory_region_init_io(&s->data_iomem, OBJECT(s), &fw_cfg_data_mem_ops, s, > - "fwcfg.data", > - fw_cfg_data_mem_ops.valid.max_access_size); > - sysbus_init_mmio(sbd, &s->data_iomem); > /* In case ctl and data overlap: */ > memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops, s, > "fwcfg", FW_CFG_SIZE); > } > @@ -620,19 +619,31 @@ static void fw_cfg_initfn(Object *obj) > static void fw_cfg_realize(DeviceState *dev, Error **errp) > { > FWCfgState *s = FW_CFG(dev); > SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > + const MemoryRegionOps *data_mem_ops = &fw_cfg_data_mem_ops; > uint32_t ctl_io_last; > uint32_t data_io_end; > > + if (s->data_memwidth > data_mem_ops->valid.max_access_size) { > + MemoryRegionOps *ops; > + > + ops = g_memdup(data_mem_ops, sizeof(*data_mem_ops)); Hrm, this memory will leak if the device gets destroyed after realize, right? I see 2 options around this: 1) Free it on destruction 2) Add the RegionOps as field into FWCfgState. Then it gets allocated and free'd automatically Option 2 is easier (and more failure proof) but will waste a few bytes of ram for data_memwidth=1 users. I don't think we need to bother about the few bytes and rather go with safety :). Alex