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

[liu ping fan]

On Thu, Sep 20, 2012 at 5:15 PM, Avi Kivity <address@hidden> wrote:
> 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.
>
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