Re: [PATCH] memory: transaction API

[Avi Kivity <avi@redhat.com>]

On 07/21/2011 04:17 PM, Jan Kiszka wrote:
> On 2011-07-21 14:58, Avi Kivity wrote:
> >  On 07/21/2011 03:52 PM, Jan Kiszka wrote:
> >>>
> >>>   The problem is that "update" can change lots of things.  offset, size,
> >>>   whether it's mmio or RAM, read-onlyness, even the wierd things like
> >>>   coalesced mmio.  So it's either a function with 324.2 parameters (or a
> >>>   large struct), or it's a series of functions with demarcation as to
> >>>   where the update begins and ends.
> >>
> >>  We do not need to provide update support for each and every bit, but for
> >>  the common cases. memory_region_update_alias(region, offset, size) would
> >>  be an excellent first candidate IMO.
> >
> >  It's not enough, look at cirrus and PAM.
>
> It's a perfect fit for cirrus, but PAM indeed requires set_readonly in
> addition.
>

It isn't a pefect fit for cirrus.  If the mode changes in a way that 
makes mapping the map as RAM possible, or vice versa, and if the banks 
are contiguous, then _update() results in two mappings or unmappings, 
while _commit() results in just one (since m_r_update_topology() merges 
the two adjacent regions).

> I also think now that describing a memory region offline via a struct
> and then passing that to an atomic add/del/update would be a more handy
> and future-proof API than an increasing number set functions.

Maybe.  But it's not sufficient for atomic changes involving multiple 
regions.

btw, there is another implementation issue involving SMP - if a region 
that obscures the middle of another region is removed, we'll have two 
regions removed and replaced with a larger one.  That causes some memory 
to be temporarily inaccessible.  I don't think it's a problem in 
practice, but if it is, we can fix it by stopping all vcpus if we detect 
this condition, and by adding an atomic 
change-memory-map-and-get-dirty-log ioctl to kvm.

-- 
error compiling committee.c: too many arguments to function