[Qemu-devel] [PATCH] dma: Avoid reentrancy in DMA transfer handlers

[Qemu-devel] [PATCH] dma: Avoid reentrancy in DMA transfer handlers

[Kevin Wolf]

With the conversion of the block layer to coroutines, bdrv_read/write
have changed to run a nested event loop that calls qemu_bh_poll.
Consequently a scheduled BH can be called while a DMA transfer handler
runs and this means that DMA_run becomes reentrant.

Devices haven't been designed to cope with that, so instead of running a
nested transfer handler just wait for the next invocation of the BH from the
main loop.

This fixes some problems with the floppy device.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/dma.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/hw/dma.c b/hw/dma.c
index 8a7302a..e8d6341 100644
--- a/hw/dma.c
+++ b/hw/dma.c
@@ -358,6 +358,13 @@ static void DMA_run (void)
     struct dma_cont *d;
     int icont, ichan;
     int rearm = 0;
+    static int running = 0;
+
+    if (running) {
+        goto out;
+    } else {
+        running = 1;
+    }
 
     d = dma_controllers;
 
@@ -374,6 +381,8 @@ static void DMA_run (void)
         }
     }
 
+out:
+    running = 0;
     if (rearm)
         qemu_bh_schedule_idle(dma_bh);
 }
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH] dma: Avoid reentrancy in DMA transfer handlers

[Paolo Bonzini]

On 10/28/2011 04:18 PM, Kevin Wolf wrote:
> With the conversion of the block layer to coroutines, bdrv_read/write
> have changed to run a nested event loop that calls qemu_bh_poll.
> Consequently a scheduled BH can be called while a DMA transfer handler
> runs and this means that DMA_run becomes reentrant.
>
> Devices haven't been designed to cope with that, so instead of running a
> nested transfer handler just wait for the next invocation of the BH from the
> main loop.
>
> This fixes some problems with the floppy device.
>
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> ---
>   hw/dma.c |    9 +++++++++
>   1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/hw/dma.c b/hw/dma.c
> index 8a7302a..e8d6341 100644
> --- a/hw/dma.c
> +++ b/hw/dma.c
> @@ -358,6 +358,13 @@ static void DMA_run (void)
>       struct dma_cont *d;
>       int icont, ichan;
>       int rearm = 0;
> +    static int running = 0;
> +
> +    if (running) {
> +        goto out;
> +    } else {
> +        running = 1;
> +    }
>
>       d = dma_controllers;
>
> @@ -374,6 +381,8 @@ static void DMA_run (void)
>           }
>       }
>
> +out:
> +    running = 0;
>       if (rearm)
>           qemu_bh_schedule_idle(dma_bh);
>   }

Hmm, I think you should set rearm = 1 to ensure the BH is run when 
ultimately you leave the sync read.  Sorry for not spotting this before.

Paolo

Re: [Qemu-devel] [PATCH] dma: Avoid reentrancy in DMA transfer handlers

[Kevin Wolf]

Am 28.10.2011 18:35, schrieb Paolo Bonzini:
> On 10/28/2011 04:18 PM, Kevin Wolf wrote:
>> With the conversion of the block layer to coroutines, bdrv_read/write
>> have changed to run a nested event loop that calls qemu_bh_poll.
>> Consequently a scheduled BH can be called while a DMA transfer handler
>> runs and this means that DMA_run becomes reentrant.
>>
>> Devices haven't been designed to cope with that, so instead of running a
>> nested transfer handler just wait for the next invocation of the BH from the
>> main loop.
>>
>> This fixes some problems with the floppy device.
>>
>> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
>> ---
>>   hw/dma.c |    9 +++++++++
>>   1 files changed, 9 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/dma.c b/hw/dma.c
>> index 8a7302a..e8d6341 100644
>> --- a/hw/dma.c
>> +++ b/hw/dma.c
>> @@ -358,6 +358,13 @@ static void DMA_run (void)
>>       struct dma_cont *d;
>>       int icont, ichan;
>>       int rearm = 0;
>> +    static int running = 0;
>> +
>> +    if (running) {
>> +        goto out;
>> +    } else {
>> +        running = 1;
>> +    }
>>
>>       d = dma_controllers;
>>
>> @@ -374,6 +381,8 @@ static void DMA_run (void)
>>           }
>>       }
>>
>> +out:
>> +    running = 0;
>>       if (rearm)
>>           qemu_bh_schedule_idle(dma_bh);
>>   }
> 
> Hmm, I think you should set rearm = 1 to ensure the BH is run when 
> ultimately you leave the sync read.  Sorry for not spotting this before.

I was about to agree, but in fact adding a rearm = 1; line leads to
crashes, whereas in the version I posted it just works. So it looks like
something is wrong with doing it, even though it seemed to make perfect
sense at the first sight.

Kevin

Re: [Qemu-devel] [PATCH] dma: Avoid reentrancy in DMA transfer handlers

[Paolo Bonzini]

On 10/31/2011 03:46 PM, Kevin Wolf wrote:
> >  Hmm, I think you should set rearm = 1 to ensure the BH is run when
> >  ultimately you leave the sync read.  Sorry for not spotting this before.
>
> I was about to agree, but in fact adding a rearm = 1; line leads to
> crashes, whereas in the version I posted it just works. So it looks like
> something is wrong with doing it, even though it seemed to make perfect
> sense at the first sight.

But what will restart the DMA at the end of the synchronous I/O, then?

Paolo

Re: [Qemu-devel] [PATCH] dma: Avoid reentrancy in DMA transfer handlers

[Kevin Wolf]

Am 31.10.2011 16:34, schrieb Paolo Bonzini:
> On 10/31/2011 03:46 PM, Kevin Wolf wrote:
>>>  Hmm, I think you should set rearm = 1 to ensure the BH is run when
>>>  ultimately you leave the sync read.  Sorry for not spotting this before.
>>
>> I was about to agree, but in fact adding a rearm = 1; line leads to
>> crashes, whereas in the version I posted it just works. So it looks like
>> something is wrong with doing it, even though it seemed to make perfect
>> sense at the first sight.
> 
> But what will restart the DMA at the end of the synchronous I/O, then?

bdrv_read/write are called inside fdctrl_read_data(), so the outer
DMA_run() already has rearm = 1.

I think the more interesting question is why rescheduling can break
anything. Where would we schedule the BH additionally when it isn't
already scheduled anyway?

Kevin

Re: [Qemu-devel] [PATCH] dma: Avoid reentrancy in DMA transfer handlers

[Kevin Wolf]

Am 31.10.2011 17:00, schrieb Kevin Wolf:
> Am 31.10.2011 16:34, schrieb Paolo Bonzini:
>> On 10/31/2011 03:46 PM, Kevin Wolf wrote:
>>>>  Hmm, I think you should set rearm = 1 to ensure the BH is run when
>>>>  ultimately you leave the sync read.  Sorry for not spotting this before.
>>>
>>> I was about to agree, but in fact adding a rearm = 1; line leads to
>>> crashes, whereas in the version I posted it just works. So it looks like
>>> something is wrong with doing it, even though it seemed to make perfect
>>> sense at the first sight.
>>
>> But what will restart the DMA at the end of the synchronous I/O, then?
> 
> bdrv_read/write are called inside fdctrl_read_data(), so the outer
> DMA_run() already has rearm = 1.
> 
> I think the more interesting question is why rescheduling can break
> anything. Where would we schedule the BH additionally when it isn't
> already scheduled anyway?

I think I found the problem:

> @@ -374,6 +381,8 @@ static void DMA_run (void)
>          }
>      }
>  
> +out:
> +    running = 0;
>      if (rearm)
>          qemu_bh_schedule_idle(dma_bh);
>  }

We should only reset running to 0 in the outermost instance. Moving the
out: label a line down seems to fix the crashes.

Kevin