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

[liu ping fan]

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?
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