[Qemu-devel] [PATCH repost] ahci: fix win7 hang on boot

[Qemu-devel] [PATCH repost] ahci: fix win7 hang on boot

[Michael S. Tsirkin]

From: Alexander Graf <agraf@suse.de>

When AHCI executes an asynchronous IDE command, it checked DRDY without
checking either DRQ or BSY.  This sometimes caused interrupt to be sent
before command is actually completed.

This resulted in a race condition: if guest then managed to access the
device before command has completed, it would hang waiting for an
interrupt.
This was observed with windows 7 guests.

To fix, check for DRQ or BSY in additiona to DRDY, if set,
the command is asynchronous so delay the interrupt until
asynchronous done callback is invoked.

Reported-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---
 hw/ide/ahci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index a8be62c..fbea9e8 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -961,7 +961,8 @@ static int handle_cmd(AHCIState *s, int port, int slot)
         /* We're ready to process the command in FIS byte 2. */
         ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
 
-        if (s->dev[port].port.ifs[0].status & READY_STAT) {
+        if ((s->dev[port].port.ifs[0].status & (READY_STAT|DRQ_STAT|BUSY_STAT)) ==
+            READY_STAT) {
             ahci_write_fis_d2h(&s->dev[port], cmd_fis);
         }
     }
-- 
MST

Re: [Qemu-devel] [PATCH repost] ahci: fix win7 hang on boot

[Kevin Wolf]

Am 28.10.2013 um 20:01 hat Michael S. Tsirkin geschrieben:
> From: Alexander Graf <agraf@suse.de>
> 
> When AHCI executes an asynchronous IDE command, it checked DRDY without
> checking either DRQ or BSY.  This sometimes caused interrupt to be sent
> before command is actually completed.
> 
> This resulted in a race condition: if guest then managed to access the
> device before command has completed, it would hang waiting for an
> interrupt.
> This was observed with windows 7 guests.
> 
> To fix, check for DRQ or BSY in additiona to DRDY, if set,
> the command is asynchronous so delay the interrupt until
> asynchronous done callback is invoked.
> 
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Tested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH repost] ahci: fix win7 hang on boot

[Paolo Bonzini]

Il 28/10/2013 20:01, Michael S. Tsirkin ha scritto:
> From: Alexander Graf <agraf@suse.de>
> 
> When AHCI executes an asynchronous IDE command, it checked DRDY without
> checking either DRQ or BSY.  This sometimes caused interrupt to be sent
> before command is actually completed.
> 
> This resulted in a race condition: if guest then managed to access the
> device before command has completed, it would hang waiting for an
> interrupt.
> This was observed with windows 7 guests.
> 
> To fix, check for DRQ or BSY in additiona to DRDY, if set,
> the command is asynchronous so delay the interrupt until
> asynchronous done callback is invoked.
> 
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Tested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ---
>  hw/ide/ahci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index a8be62c..fbea9e8 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -961,7 +961,8 @@ static int handle_cmd(AHCIState *s, int port, int slot)
>          /* We're ready to process the command in FIS byte 2. */
>          ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
>  
> -        if (s->dev[port].port.ifs[0].status & READY_STAT) {
> +        if ((s->dev[port].port.ifs[0].status & (READY_STAT|DRQ_STAT|BUSY_STAT)) ==
> +            READY_STAT) {
>              ahci_write_fis_d2h(&s->dev[port], cmd_fis);
>          }
>      }
> 

While the patch fixes the symptom, I think it is only a bandaid.

There is no reason why the async_cmd_done should be restricted to
asynchronous commands.  If synchronous commands are made to go through
the async_cmd_done callback, you'll automatically get the D2H FIS
written for all commands.

It's good for 1.7, but let's revisit it for 1.8.

Paolo

Re: [Qemu-devel] [PATCH repost] ahci: fix win7 hang on boot

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 12:32:12PM +0100, Paolo Bonzini wrote:
> Il 28/10/2013 20:01, Michael S. Tsirkin ha scritto:
> > From: Alexander Graf <agraf@suse.de>
> > 
> > When AHCI executes an asynchronous IDE command, it checked DRDY without
> > checking either DRQ or BSY.  This sometimes caused interrupt to be sent
> > before command is actually completed.
> > 
> > This resulted in a race condition: if guest then managed to access the
> > device before command has completed, it would hang waiting for an
> > interrupt.
> > This was observed with windows 7 guests.
> > 
> > To fix, check for DRQ or BSY in additiona to DRDY, if set,
> > the command is asynchronous so delay the interrupt until
> > asynchronous done callback is invoked.
> > 
> > Reported-by: Michael S. Tsirkin <mst@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Tested-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > ---
> >  hw/ide/ahci.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> > index a8be62c..fbea9e8 100644
> > --- a/hw/ide/ahci.c
> > +++ b/hw/ide/ahci.c
> > @@ -961,7 +961,8 @@ static int handle_cmd(AHCIState *s, int port, int slot)
> >          /* We're ready to process the command in FIS byte 2. */
> >          ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
> >  
> > -        if (s->dev[port].port.ifs[0].status & READY_STAT) {
> > +        if ((s->dev[port].port.ifs[0].status & (READY_STAT|DRQ_STAT|BUSY_STAT)) ==
> > +            READY_STAT) {
> >              ahci_write_fis_d2h(&s->dev[port], cmd_fis);
> >          }
> >      }
> > 
> 
> While the patch fixes the symptom, I think it is only a bandaid.
> 
> There is no reason why the async_cmd_done should be restricted to
> asynchronous commands.  If synchronous commands are made to go through
> the async_cmd_done callback, you'll automatically get the D2H FIS
> written for all commands.

I suggested this to Kevin offline but he prefers it like this.

> It's good for 1.7, but let's revisit it for 1.8.
> 
> Paolo

Fine with me.