Re: [Qemu-devel] [PATCH 0/3] Memory API mutators

[Avi Kivity <avi@redhat.com>]

On 09/14/2011 01:21 PM, Peter Maydell wrote:
> On 14 September 2011 11:02, Avi Kivity<avi@redhat.com>  wrote:
> >  On 09/14/2011 12:56 PM, Peter Maydell wrote:
> >>
> >>  On 14 September 2011 10:23, Avi Kivity<avi@redhat.com>    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