From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42140) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEGE7-0002HN-AY for qemu-devel@nongnu.org; Wed, 19 Sep 2012 05:05:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TEGDy-00013O-Uj for qemu-devel@nongnu.org; Wed, 19 Sep 2012 05:05:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:2039) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEGDy-00013J-Lp for qemu-devel@nongnu.org; Wed, 19 Sep 2012 05:05:38 -0400 Message-ID: <50598ADC.3030506@redhat.com> Date: Wed, 19 Sep 2012 12:05:32 +0300 From: Avi Kivity MIME-Version: 1.0 References: <1347349912-15611-1-git-send-email-qemulist@gmail.com> <1347349912-15611-11-git-send-email-qemulist@gmail.com> <504EF7CC.6040900@redhat.com> <504F0B17.5090703@siemens.com> <504F2D97.3000705@redhat.com> <504F316B.1030500@redhat.com> <50519727.8060509@redhat.com> <50597BF1.9070703@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V3 10/11] vcpu: introduce lockmap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: liu ping fan Cc: Paolo Bonzini , Marcelo Tosatti , "qemu-devel@nongnu.org" , Anthony Liguori , Jan Kiszka On 09/19/2012 11:36 AM, liu ping fan wrote: >> >> It basically means you can't hold contents of device state in local >> variables. You need to read everything again from the device. That >> includes things like DMA enable bits. >> > I think that read everything again from the device can not work. > Suppose the following scene: If the device's state contains the change > of a series of internal registers (supposing partA+partB; > partC+partD), after partA changed, the device's lock is broken. At > this point, another access to this device, it will work on partA+partB > to determine C+D, but since partB is not correctly updated yet. So C+D > may be decided by broken context and be wrong. That's the guest's problem. Note it could have happened even before the change, since the writes to A/B/C/D are unordered wrt the DMA. > >> (btw: to we respect PCI_COMMAND_MASTER? it seems that we do not. This >> is a trivial example of an iommu, we should get that going). >> >>> Or for converted device, we can just tag it a >>> busy flag, that is check&set busy flag at the entry of device's >>> mmio-dispatch. So when re-acquire device's lock, the device's state >>> is intact. >> >> The state can be changed by a parallel access to another register, which >> is valid. >> > Do you mean that the device can be accessed in parallel? But how? we > use device's lock. Some DMA is asynchronous already, like networking and IDE DMA. So we drop the big lock while doing it. With the change to drop device locks around c_p_m_rw(), we have that problem everywhere. > What my suggestion is: > lock(); > set_and_test(dev->busy); > if busy > unlock and return; > changing device registers; > do other things including calling to c_p_m_rw() //here,lock broken, > but set_and_test() works > clear(dev->busy); > unlock(); > > So changing device registers is protected, and unbreakable. But the changes may be legitimate. Maybe you're writing to a completely unrelated register, from a different vcpu, now that write is lost. -- error compiling committee.c: too many arguments to function