Re: [Qemu-devel] [PATCH V3 10/11] vcpu: introduce lockmap

[Avi Kivity]

On 09/20/2012 10:51 AM, liu ping fan wrote:
> On Wed, Sep 19, 2012 at 5:05 PM, Avi Kivity <address@hidden> 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.

>>> 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).


-- 
error compiling committee.c: too many arguments to function