On 2012-10-25 18:21, Avi Kivity wrote:
> On 10/25/2012 11:31 AM, Jan Kiszka wrote:
>> On 2012-10-25 11:01, Avi Kivity wrote:
>>> On 10/24/2012 09:17 AM, Jan Kiszka wrote:
>>>>>>
>>>>>> This is ugly for many reasons. First of all, it is racy as the register
>>>>>> content may change while dropping the device lock, no? Then you would
>>>>>> raise or clear an IRQ spuriously.
>>>>>>
>>>>> Device state's intact is protected by busy flag, and will not broken
>>>>
>>>> Except that the busy flag concept is broken in itself.
>>>
>>> How do we fix an mmio that ends up mmio'ing back to itself, perhaps
>>> indirectly? Note this is broken in mainline too, but in a different way.
>>>
>>> Do we introduce clever locks that can detect deadlocks?
>>
>> That problem is already addressed (to my understanding) by blocking
>> nested MMIO in general.
>
> That doesn't work cross-thread.
>
> vcpu A: write to device X, dma-ing to device Y
> vcpu B: write to device Y, dma-ing to device X
We will deny DMA-ing from device X on behalf of a VCPU, ie. in dispatch
context, to Y.
What we do not deny, though, is DMA-ing from an I/O thread that
processes an event for device X. If the invoked callback of device X
holds the device lock across some DMA request to Y, then we risk to run
into the same ABBA issue. Hmm...
>
> My suggestion was to drop the locks around DMA, then re-acquire the lock
> and re-validate data.
Maybe possible, but hairy depending on the device model.
>
>> The brokenness of the busy flag is that it
>> prevents concurrent MMIO by dropping requests.
>
> Right.
>
>>
>>>
>>>> I see that we have a all-or-nothing problem here: to address this
>>>> properly, we need to convert the IRQ path to lock-less (or at least
>>>> compatible with holding per-device locks) as well.
>>>
>>> There is a transitional path where writing to a register that can cause
>>> IRQ changes takes both the big lock and the local lock.
>>>
>>> Eventually, though, of course all inner subsystems must be threaded for
>>> this work to have value.
>>>
>>
>> But that transitional path must not introduce regressions. Opening a
>> race window between IRQ cause update and event injection is such a
>> thing, just like dropping concurrent requests on the floor.
>
> Can you explain the race?
Context A Context B
device.lock
...
device.set interrupt_cause = 0
lower_irq = true
...
device.unlock
device.lock
...
device.interrupt_cause = 42
raise_irq = true
...
device.unlock
if (raise_irq)
bql.lock
set_irq(device.irqno)
bql.unlock
if (lower_irq)
bql.lock
clear_irq(device.irqno)
bql.unlock
And there it goes, our interrupt event.
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
