From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46594) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TLdWa-0003rn-Pq for qemu-devel@nongnu.org; Tue, 09 Oct 2012 13:23:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TLdWY-0007GH-Ow for qemu-devel@nongnu.org; Tue, 09 Oct 2012 13:23:20 -0400 Message-ID: <50745D80.2030004@suse.de> Date: Tue, 09 Oct 2012 19:23:12 +0200 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1349714816-12618-1-git-send-email-Bharat.Bhushan@freescale.com> <1349714816-12618-3-git-send-email-Bharat.Bhushan@freescale.com> <50730BA3.5000005@suse.de> <597774F2-1995-4CCF-8518-4F9509CD4DEF@suse.de> In-Reply-To: <597774F2-1995-4CCF-8518-4F9509CD4DEF@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: qemu-devel@nongnu.org, Bharat Bhushan , qemu-ppc@nongnu.org, Avi Kivity , Bharat Bhushan Am 08.10.2012 20:58, schrieb Alexander Graf: >=20 > On 08.10.2012, at 19:21, Andreas F=E4rber wrote: >=20 >> Am 08.10.2012 18:46, schrieb Bharat Bhushan: >>> + s =3D sysbus_from_qdev(dev); >> >> s =3D SYS_BUS_DEVICE(dev); >=20 > Mind to explain why? The rest of the code uses the above helper :). SYS_BUS_DEVICE() is the QOM cast macro, relying on binary struct layout. sysbus_from_qdev() is an old qdev helper, relying on container_of() and field naming. I'm lobbying for using the new constructs in new code. You can argue of course that SysBus will go away some day so it doesn't matter much here. But we had issues with PCIHostState not adhering to QOM layout requirements (e.g., pSeries), so using the cast consistently helps to avoid/detect such issues. >>> + memory_region_add_subregion(ccsr, MPC8544_PCI_REGS_OFFSET, s->mm= io[0].memory); >> >> ... I wonder if fiddling with SysBus MMIO is a good idea. >> s->mmio[0].addr is not getting assigned this way, which is checked as >> condition for deleting the subregion. But sysbus_mmio_map() only adds = to >> / deletes from get_system_memory(). >> The alternative would be using a custom field rather than the >> SysBus-internal one. Avi/Alex? >=20 > I honestly don't mind either way. We could >=20 > a) add a new sysbus helper for maps in memory region containers > b) not use sysbus at all > c) do it this way >=20 > Anything else wouldn't improve the situation. I really don't care which= way we go. To me, c) is fine. b) is not that easily possible for the PHB, since we still haven't seen a new round of i440fx patches (PCIHostState is based on SysBusDevice, so all PHBs would need to get converted at once). I'd also be in favor of c) with Avi's suggestion of using the accessor function to obtain the MemoryRegion* (data caging). Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg