On 2011-07-21 16:39, Avi Kivity wrote: > 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().
OK. What about providing _update wrappers? They could be implemented internally by the memory API in terms of begin - remove region - change region object - re-add region - end. That would avoid boilerplate code on the user side and still keep the option to do open-coded transaction also outside the core. > >>> >>>> 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? Not sure how this mode is called, but when vram is mapped linearly into the 0xa0000 range via two 32K banks, you get quite a few updates on larger screen changes. > > 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. We will need workarounds like we have today, e.g. confining PAM memory region fragmentation to certain patterns that known OSes require. Full, correct support would remain the privilege of host kernels that allow to combine multi-region updates to an atomic operation (+ returning or preserving dirty logs). Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
