On Wed, Sep 19, 2012 at 5:05 PM, Avi Kivity <a...@redhat.com> wrote: > 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. > Yes, agree, it is the guest's problem. So it means that ready_of(A+B) is not signaled to guest, the guest should not launch operations on (C+D). Right? But here comes the question, if ready not signaled to guest, how can guest launch operation on (A+B) again? i.e. although local lock is broken, the (A+B) is still intact when re-acquire local lock. So need not to read everything again from the device. Wrong?
>> >>> (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. > But I think it will mean more-fine locks for each groups of unrelated register, and accordingly, the busy should be bitmap for each group. Thanks and regards, pingfan > > -- > error compiling committee.c: too many arguments to function