From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:56244) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R3o2n-0003wj-Ki for qemu-devel@nongnu.org; Wed, 14 Sep 2011 07:54:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R3o2h-0006kh-RW for qemu-devel@nongnu.org; Wed, 14 Sep 2011 07:54:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41190) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R3o2h-0006kR-KL for qemu-devel@nongnu.org; Wed, 14 Sep 2011 07:54:15 -0400 Message-ID: <4E7095E3.1080909@redhat.com> Date: Wed, 14 Sep 2011 14:54:11 +0300 From: Avi Kivity MIME-Version: 1.0 References: <1315992222-24069-1-git-send-email-avi@redhat.com> <4E707BCE.60100@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 0/3] Memory API mutators List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-devel@nongnu.org On 09/14/2011 01:21 PM, Peter Maydell wrote: > On 14 September 2011 11:02, Avi Kivity wrote: > > On 09/14/2011 12:56 PM, Peter Maydell wrote: > >> > >> On 14 September 2011 10:23, Avi Kivity wrote: > >> > This patchset introduces memory_region_set_enabled() and > >> > memory_region_set_address() to avoid the requirement on memory > >> > routers to track the internal state of the memory API (so they know > >> > whether they need to add or remove a region). Instead, they can > >> > simply copy the state of the region from the guest-exposed register > >> > to the memory core, via the new mutator functions. > >> > > >> > Please review. Do we need a memory_region_set_size() as well? > >> > >> Would set_size() allow things like omap_gpmc() to avoid the need > >> to create an intermediate container subregion to enforce size > >> clipping on the child region it's trying to map? > > > > I'd recommend not calling _set_size() on somebody else's region - this > > quickly leads to confusion. Only call set_size() if you also called _init() > > and will call _destroy(). > > > > Can you point me at the code in question? > > hw/omap_gpmc.c:omap_gpmc_cs_map(). For each of the 8 children you > can connect to it, the GPMC has a base and mask register. The > hardware logic is effectively > if ((address& mask) == base) { send transaction to this child } > > (complicated only slightly by the register for base only having > bits [29:24] with the others implied-zero, and the register for > mask only having bits [27:24].) The effect is that you can use > the mask value to set the size of the area the child is mapped in. > (Silly mask settings with "holes" are discouraged by the TRM, > and the current code doesn't handle them.) Thanks; and I think that in this case the omap code should avoid touching the child region (who knows, its owner may call memory_region_size() one day) and use a container (or alias - seems a better fit?) instead. btw, what does a truncating _set_size() do to a RAM region? Discard the excess state? Or remember it and maintain two size, one internal and one for show? > The repeated-in-the-space effect happens if the child is smaller > than the space it's in: the child hardware just ignores the higher > bits of the address so appears multiple times. > > >> (Strictly speaking what omap_gpmc() wants is not merely clipping > >> to a guest-specified size but also wrapping, so you can take a > >> 16MB child region and map the bottom 4MB of it repeating into > >> a 32MB chunk of address space, say. But that would require a lot > >> of playing games with aliases to implement a bizarre corner > >> case that nobody uses in practice.) > > > That's best done in the memory core, the rendering loop can be adjusted to > > do this replication. > > That would be nice, although as I say nobody is actually relying > on it so probably not worth the effort unless there's another user > for it. There's another user in the infamous pflash_cfi02 - see pflash_setup_mappings(). -- error compiling committee.c: too many arguments to function