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

[Avi Kivity]

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.

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


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