From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39239) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dX9kj-0004L2-02 for qemu-devel@nongnu.org; Mon, 17 Jul 2017 13:24:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dX9kf-0004Fd-NE for qemu-devel@nongnu.org; Mon, 17 Jul 2017 13:24:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52210) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dX9kf-0004FM-Gf for qemu-devel@nongnu.org; Mon, 17 Jul 2017 13:24:09 -0400 Date: Mon, 17 Jul 2017 14:24:04 -0300 From: Eduardo Habkost Message-ID: <20170717172404.GS6020@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> <20170714185616.GQ6020@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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: Mark Cave-Ayland Cc: Laszlo Ersek , peter.maydell@linaro.org, mst@redhat.com, somlo@cmu.edu, qemu-devel@nongnu.org, rjones@redhat.com, imammedo@redhat.com, pbonzini@redhat.com On Sun, Jul 16, 2017 at 08:12:13PM +0100, Mark Cave-Ayland wrote: > On 14/07/17 19:56, Eduardo Habkost wrote: > > >>> 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. > > Yes, that is correct (and I believe is mentioned in the cover letter, too). > > >>> 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. > > That's interesting. I did a grep of the codebase for > object_resolve_path_component and struggled to find an instance where it > was being used to provide access to a MemoryRegion. Even if it's an > available feature, it's certainly not one that is widely known about. > > In terms of the patch, is the v9 revision suitable for commit or does > this constitute a NACK? It's not a NACK. Using QOM properties instead of direct struct access may be nice (I'm not 100% sure, either), but not required. -- Eduardo