Re: [PATCH] memory: transaction API

Avi Kivity
2011-07-21T14:39:29Z

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