Re: [PATCH] memory: transaction API

[Avi Kivity <avi@redhat.com>]

On 07/21/2011 05:32 PM, Jan Kiszka wrote:
> On 2011-07-21 15:50, Avi Kivity wrote:
> >  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).
>
> Continuous banks or mode changes are uncommon compared to offset changes
> of the mapped window. Cirrus does not need to bother about continuity of
> its banks (the memory core will), and mode changes could be implemented
> by allowing updates of the priority, thus reordering the regions instead
> of continuously deleting and recreating them.

The point is _update() can only make changes for one region atomic, 
while _commit() is more general.  You can sometimes batch all changes 
into a single container region, but sometimes it is clumsy, and 
sometimes impossible.

Deletion and creation are needed because we can't update an alias' 
offset.  I guess I can add that functionality.  But it still isn't as 
general as _commit().

> >
> >>  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.
>
> Right. The question is still if there are use cases where this matters
> (ie. update frequencies comparable to graphic scenarios).

Does even cirrus update this often?  I would guess cirrus usually uses 
the linear framebuffer, no?

I added support for aliases and the vga banks just to get Windows XP to 
clear the screen quickly on bootup (used to take ~10 seconds).rary 
changes in the cirrus remapping logs.

> >   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.
>
> I'm not sure if the cirrus or any similar hardware supports consistent
> memory accesses during ongoing bank remappings (ie. while the CPU
> issuing the remapping IO commands is blocked on QEMU executing them).

Point is, unaffected regions (and so unaffected devices) are also 
modified.  Consider a PAM modified from PCI to RAM.  The adjacent RAM 
regions are removed and re-added.  If some code on another cpu is 
running on this RAM, it would be a little confused.

The CPU that is issuing the command is unaffected.

> But such an IOCTL would resolve our problem with dropping a logged
> region as well, right?

Yes, if done right.  Still we need to support older kernels.

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