From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] memory: transaction API Date: Thu, 21 Jul 2011 17:39:22 +0300 Message-ID: <4E283A1A.5070908@redhat.com> References: <1311243679-18403-1-git-send-email-avi@redhat.com> <4E280195.5040003@siemens.com> <4E281609.7090809@redhat.com> <4E2816DE.5010102@siemens.com> <4E2817DA.4030505@redhat.com> <4E28211C.5060705@siemens.com> <4E282286.2050508@redhat.com> <4E2826E1.6090404@siemens.com> <4E282E8A.40606@redhat.com> <4E283866.7040601@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: "qemu-devel@nongnu.org" , "kvm@vger.kernel.org" To: Jan Kiszka Return-path: Received: from mx1.redhat.com ([209.132.183.28]:14930 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751742Ab1GUOj3 (ORCPT ); Thu, 21 Jul 2011 10:39:29 -0400 In-Reply-To: <4E283866.7040601@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: 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