Re: [Qemu-devel] [patch v4 13/16] e1000: add busy flag to anti broken device state

[liu ping fan]

On Fri, Oct 26, 2012 at 6:25 PM, Jan Kiszka <address@hidden> wrote:
> On 2012-10-26 05:08, liu ping fan wrote:
>> On Fri, Oct 26, 2012 at 11:05 AM, liu ping fan <address@hidden> wrote:
>>> On Thu, Oct 25, 2012 at 5:04 PM, Avi Kivity <address@hidden> wrote:
>>>> On 10/25/2012 11:00 AM, Peter Maydell wrote:
>>>>> On 23 October 2012 10:37, Avi Kivity <address@hidden> wrote:
>>>>>> On 10/23/2012 11:32 AM, liu ping fan wrote:
>>>>>>> On Tue, Oct 23, 2012 at 5:07 PM, Jan Kiszka <address@hidden> wrote:
>>>>>>>> On 2012-10-23 07:52, liu ping fan wrote:
>>>>>>>>> On Mon, Oct 22, 2012 at 6:40 PM, Avi Kivity <address@hidden> wrote:
>>>>>>>>>> On 10/22/2012 11:23 AM, Liu Ping Fan wrote:
>>>>>>>>> It will only record and fix the issue on one thread. But guest can
>>>>>>>>> touch the emulated device on muti-threads.
>>>>>>>>
>>>>>>>> Sorry, what does that mean? A second VCPU accessing the device will
>>>>>>>> simply be ignored when it races with another VCPU? Specifically
>>>>>>>>
>>>>>>> Yes, just ignored.  For device which support many logic in parallel,
>>>>>>> it should use independent busy flag for each logic
>>>>>>
>>>>>> We don't actually know that e1000 doesn't.  Why won't writing into
>>>>>> different registers in parallel work?
>>>>>
>>>>> Unless the device we're emulating supports multiple in
>>>>> parallel accesses (and I bet 99.9% of the devices we model
>>>>> don't) then the memory framework needs to serialise the
>>>>> loads/stores. Otherwise it's just going to be excessively
>>>>> hard to write a reliable device model.
>>>>
>>>> That's why we have a per-device lock.  The busy flag breaks that model
>>>> by discarding accesses that occur in parallel.
>>>>
>>> I think by adopting the model, we can avoid this.
>>>
>>> struct device_logic {
>>>   bool busy;
>>>   qemu_mutex lock;
>>>   QemuCond wait;
>>> };
>>>
>>> LOCK(logic->lock)
>>> while (logic->busy) {
>>> qemu_cond_wait(&logic->wait, &logic->lock);
>>> }
>>> ....
>>> do hardware emulation
>>> ...
>>> logic->busy = false;
>> UNLOCK(lock); <-------------------------------------- forget
>>> qemu_cond_signal(&logic->wait);
>>>
>>> This is identical to the biglock's behavior for parallel access to
>>> device for nowadays. And then, the problem left is what level for
>>> parallel we want. If we expect more parallel, we need to degrade the
>>> device into more logic unit.
>
> But where is the remaining added-value of the busy flag then? Everyone
> could just as well be serialized by the lock itself. And even when
> dropping the lock while running the hw emulation, that doesn't change

The key is that local lock is broken, so we rely on a high level lock
-- busy flag.  After threading each subsystem, we will not face broken
local lock issue, and can throw away this design.

> anything to the semantic - and our ABBA problems I sketched yesterday.
>
Oh, ABBA problem can not be solved, I think we need clever deadlock detector.

Regards,
pingfan

> Jan
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux