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

[Jan Kiszka]

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
anything to the semantic - and our ABBA problems I sketched yesterday.

Jan

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