Re: [PATCH] memory: transaction API

Avi Kivity
2011-07-21T13:50:08Z

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