On Thu, Sep 20, 2012 at 5:15 PM, Avi Kivity <a...@redhat.com> wrote: > On 09/20/2012 10:51 AM, liu ping fan wrote: >> 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? > > It may be evil. > >> 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? > > The device needs to perform according to its specifications. If the > specifications allow for this kind of access, we must ensure it works. > If they don't, we must ensure something sane happens, instead of a qemu > crash or exploit. This means that anything dangerous like pointers must > be revalidated. To be on the safe side, I recommend revalidating (or > reloading) everything, but it may not be necessary in all cases. > Yeah, catch the two points exactly.
>>>> 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. > > It's possible. Let's defer the discussion until a concrete case is > before us. It may be that different devices will want different > solutions (though there is value in applying one solution everywhere). > Okay. appreciate for the total detail explanation. Regards, pingfan > > -- > error compiling committee.c: too many arguments to function
