[Qemu-devel] [PATCH] ide: fix DMA register transitions

[Qemu-devel] [PATCH] ide: fix DMA register transitions

[John Snow]

ATA8-APT defines the state transitions for both a host controller and
for the hardware device during the lifecycle of a DMA transfer, in
section 9.7 "DMA command protocol."

One of the interesting tidbits here is that when a device transitions
from DDMA0 ("Prepare state") to DDMA1 ("Data_Transfer State"), it can
choose to set either BSY or DRQ to signal this transition, but not both.

as ide_sector_dma_start is the last point in our preparation process
before we begin the real data transfer process (for either AHCI or BMDMA),
this is the correct transition point for DDMA0 to DDMA1.

I have chosen !BSY && DRQ for QEMU to make the transition from DDMA0 the
most obvious.

Reported-by: Benjamin David Lunt <fys@fysnet.net>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index d117b7c..e961d42 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -907,7 +907,7 @@ eot:
 
 static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
 {
-    s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
+    s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
     s->io_buffer_size = 0;
     s->dma_cmd = dma_cmd;
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH] ide: fix DMA register transitions

[Stefan Weil]

[-- Attachment #1: Type: text/plain, Size: 1479 bytes --]

Am 03.08.2016 um 00:05 schrieb John Snow:
> ATA8-APT defines the state transitions for both a host controller and
> for the hardware device during the lifecycle of a DMA transfer, in
> section 9.7 "DMA command protocol."
> 
> One of the interesting tidbits here is that when a device transitions
> from DDMA0 ("Prepare state") to DDMA1 ("Data_Transfer State"), it can
> choose to set either BSY or DRQ to signal this transition, but not both.
> 
> as ide_sector_dma_start is the last point in our preparation process
> before we begin the real data transfer process (for either AHCI or BMDMA),
> this is the correct transition point for DDMA0 to DDMA1.
> 
> I have chosen !BSY && DRQ for QEMU to make the transition from DDMA0 the
> most obvious.
> 
> Reported-by: Benjamin David Lunt <fys@fysnet.net>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index d117b7c..e961d42 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -907,7 +907,7 @@ eot:
>  
>  static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
>  {
> -    s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
> +    s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
>      s->io_buffer_size = 0;
>      s->dma_cmd = dma_cmd;


This patch fixes the reported test case, thank you.

Tested-by: Stefan Weil <sw@weilnetz.de>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] ide: fix DMA register transitions

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1855 bytes --]

Am 03.08.2016 um 07:06 hat Stefan Weil geschrieben:
> Am 03.08.2016 um 00:05 schrieb John Snow:
> > ATA8-APT defines the state transitions for both a host controller and
> > for the hardware device during the lifecycle of a DMA transfer, in
> > section 9.7 "DMA command protocol."
> > 
> > One of the interesting tidbits here is that when a device transitions
> > from DDMA0 ("Prepare state") to DDMA1 ("Data_Transfer State"), it can
> > choose to set either BSY or DRQ to signal this transition, but not both.
> > 
> > as ide_sector_dma_start is the last point in our preparation process
> > before we begin the real data transfer process (for either AHCI or BMDMA),
> > this is the correct transition point for DDMA0 to DDMA1.
> > 
> > I have chosen !BSY && DRQ for QEMU to make the transition from DDMA0 the
> > most obvious.
> > 
> > Reported-by: Benjamin David Lunt <fys@fysnet.net>
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  hw/ide/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index d117b7c..e961d42 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -907,7 +907,7 @@ eot:
> >  
> >  static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
> >  {
> > -    s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
> > +    s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
> >      s->io_buffer_size = 0;
> >      s->dma_cmd = dma_cmd;
> 
> 
> This patch fixes the reported test case, thank you.
> 
> Tested-by: Stefan Weil <sw@weilnetz.de>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Benjamin, you'll still want to fix your driver code. If John had chosen
BSY && !DRQ, which is an equally valid option, qemu would be working
correctly, but your test case would still hang.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH] ide: fix DMA register transitions

[John Snow]

On 08/03/2016 05:24 AM, Kevin Wolf wrote:
> Am 03.08.2016 um 07:06 hat Stefan Weil geschrieben:
>> Am 03.08.2016 um 00:05 schrieb John Snow:
>>> ATA8-APT defines the state transitions for both a host controller and
>>> for the hardware device during the lifecycle of a DMA transfer, in
>>> section 9.7 "DMA command protocol."
>>>
>>> One of the interesting tidbits here is that when a device transitions
>>> from DDMA0 ("Prepare state") to DDMA1 ("Data_Transfer State"), it can
>>> choose to set either BSY or DRQ to signal this transition, but not both.
>>>
>>> as ide_sector_dma_start is the last point in our preparation process
>>> before we begin the real data transfer process (for either AHCI or BMDMA),
>>> this is the correct transition point for DDMA0 to DDMA1.
>>>
>>> I have chosen !BSY && DRQ for QEMU to make the transition from DDMA0 the
>>> most obvious.
>>>
>>> Reported-by: Benjamin David Lunt <fys@fysnet.net>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  hw/ide/core.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index d117b7c..e961d42 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -907,7 +907,7 @@ eot:
>>>
>>>  static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
>>>  {
>>> -    s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
>>> +    s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
>>>      s->io_buffer_size = 0;
>>>      s->dma_cmd = dma_cmd;
>>
>>
>> This patch fixes the reported test case, thank you.
>>
>> Tested-by: Stefan Weil <sw@weilnetz.de>
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
> Benjamin, you'll still want to fix your driver code. If John had chosen
> BSY && !DRQ, which is an equally valid option, qemu would be working
> correctly, but your test case would still hang.
>
> Kevin
>

I also have to admit that I misread the report; I was under the 
impression that this was affecting Linux/Windows guests in some 
capacity, but re-reading the report I realize that you meant that you 
have observed behavior with a test case on Windows and Linux *hosts*.

As such, I'll be staging this for 2.8 instead of 2.7 unless there's 
strong arguments for its inclusion in 2.7 during freeze.

--js

Re: [Qemu-devel] [PATCH] ide: fix DMA register transitions

[Stefan Weil]

Am 03.08.2016 um 15:19 schrieb John Snow:

> I also have to admit that I misread the report; I was under the
> impression that this was affecting Linux/Windows guests in some
> capacity, but re-reading the report I realize that you meant that you
> have observed behavior with a test case on Windows and Linux *hosts*.
> 
> As such, I'll be staging this for 2.8 instead of 2.7 unless there's
> strong arguments for its inclusion in 2.7 during freeze.
> 
> --js

It's a bug fix for something which had been broken for a long time
(or even from the beginning), so I think it can wait for 2.8.

Ben will get a fixed version of QEMU for Windows because I already
added the patch to my QEMU tree, so the next installer will include
it.

And it is still possible to add the fix to a later version in the
2.7 branch, too.

Stefan

Re: [Qemu-devel] [PATCH] ide: fix DMA register transitions

[John Snow]

On 08/02/2016 06:05 PM, John Snow wrote:
> ATA8-APT defines the state transitions for both a host controller and
> for the hardware device during the lifecycle of a DMA transfer, in
> section 9.7 "DMA command protocol."
>
> One of the interesting tidbits here is that when a device transitions
> from DDMA0 ("Prepare state") to DDMA1 ("Data_Transfer State"), it can
> choose to set either BSY or DRQ to signal this transition, but not both.
>
> as ide_sector_dma_start is the last point in our preparation process
> before we begin the real data transfer process (for either AHCI or BMDMA),
> this is the correct transition point for DDMA0 to DDMA1.
>
> I have chosen !BSY && DRQ for QEMU to make the transition from DDMA0 the
> most obvious.
>
> Reported-by: Benjamin David Lunt <fys@fysnet.net>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index d117b7c..e961d42 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -907,7 +907,7 @@ eot:
>
>  static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
>  {
> -    s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
> +    s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
>      s->io_buffer_size = 0;
>      s->dma_cmd = dma_cmd;
>
>

Thanks, applied to my IDE-Next tree:

https://github.com/jnsnow/qemu/commits/ide-next
https://github.com/jnsnow/qemu.git

--js