[Qemu-devel] [PATCH] ahci: Fix FLUSH command

[Qemu-devel] [PATCH] ahci: Fix FLUSH command

[Kevin Wolf]

AHCI couldn't cope with asynchronous commands that aren't doing DMA, it
simply wouldn't complete them. Due to the bug fixed in commit f68ec837,
FLUSH commands would seem to have completed immediately even if they
were still running on the host. After the commit, they would simply hang
and never unset the BSY bit, rendering AHCI unusable on any OS sending
flushes.

This patch adds another callback for the completion of asynchronous
commands. This is what AHCI really wants to use for its command
completion logic rather than an DMA completion callback.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/ahci.c     | 8 +++++++-
 hw/ide/core.c     | 9 +++++++++
 hw/ide/internal.h | 1 +
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 97eddec..1d863b5 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1107,9 +1107,14 @@ static int ahci_dma_add_status(IDEDMA *dma, int status)
 
 static int ahci_dma_set_inactive(IDEDMA *dma)
 {
+    return 0;
+}
+
+static int ahci_async_cmd_done(IDEDMA *dma)
+{
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
 
-    DPRINTF(ad->port_no, "dma done\n");
+    DPRINTF(ad->port_no, "async cmd done\n");
 
     /* update d2h status */
     ahci_write_fis_d2h(ad, NULL);
@@ -1144,6 +1149,7 @@ static const IDEDMAOps ahci_dma_ops = {
     .set_unit = ahci_dma_set_unit,
     .add_status = ahci_dma_add_status,
     .set_inactive = ahci_dma_set_inactive,
+    .async_cmd_done = ahci_async_cmd_done,
     .restart_cb = ahci_dma_restart_cb,
     .reset = ahci_dma_reset,
 };
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 03d1cfa..a73af72 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -568,10 +568,18 @@ static void dma_buf_commit(IDEState *s)
     qemu_sglist_destroy(&s->sg);
 }
 
+static void ide_async_cmd_done(IDEState *s)
+{
+    if (s->bus->dma->ops->async_cmd_done) {
+        s->bus->dma->ops->async_cmd_done(s->bus->dma);
+    }
+}
+
 void ide_set_inactive(IDEState *s)
 {
     s->bus->dma->aiocb = NULL;
     s->bus->dma->ops->set_inactive(s->bus->dma);
+    ide_async_cmd_done(s);
 }
 
 void ide_dma_error(IDEState *s)
@@ -804,6 +812,7 @@ static void ide_flush_cb(void *opaque, int ret)
 
     bdrv_acct_done(s->bs, &s->acct);
     s->status = READY_STAT | SEEK_STAT;
+    ide_async_cmd_done(s);
     ide_set_irq(s->bus);
 }
 
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 03f1489..048a052 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -433,6 +433,7 @@ struct IDEDMAOps {
     DMAIntFunc *set_unit;
     DMAIntFunc *add_status;
     DMAFunc *set_inactive;
+    DMAFunc *async_cmd_done;
     DMARestartFunc *restart_cb;
     DMAFunc *reset;
 };
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] ahci: Fix FLUSH command

[Stefan Hajnoczi]

On Mon, Jul 15, 2013 at 11:34:31AM +0200, Kevin Wolf wrote:
> AHCI couldn't cope with asynchronous commands that aren't doing DMA, it
> simply wouldn't complete them. Due to the bug fixed in commit f68ec837,
> FLUSH commands would seem to have completed immediately even if they
> were still running on the host. After the commit, they would simply hang
> and never unset the BSY bit, rendering AHCI unusable on any OS sending
> flushes.
> 
> This patch adds another callback for the completion of asynchronous
> commands. This is what AHCI really wants to use for its command
> completion logic rather than an DMA completion callback.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/ide/ahci.c     | 8 +++++++-
>  hw/ide/core.c     | 9 +++++++++
>  hw/ide/internal.h | 1 +
>  3 files changed, 17 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH] ahci: Fix FLUSH command

[Alex Williamson]

On Mon, 2013-07-15 at 11:34 +0200, Kevin Wolf wrote:
> AHCI couldn't cope with asynchronous commands that aren't doing DMA, it
> simply wouldn't complete them. Due to the bug fixed in commit f68ec837,
> FLUSH commands would seem to have completed immediately even if they
> were still running on the host. After the commit, they would simply hang
> and never unset the BSY bit, rendering AHCI unusable on any OS sending
> flushes.
> 
> This patch adds another callback for the completion of asynchronous
> commands. This is what AHCI really wants to use for its command
> completion logic rather than an DMA completion callback.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Works!

Tested-by: Alex Williamson <alex.williamson@redhat.com>

> ---
>  hw/ide/ahci.c     | 8 +++++++-
>  hw/ide/core.c     | 9 +++++++++
>  hw/ide/internal.h | 1 +
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 97eddec..1d863b5 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1107,9 +1107,14 @@ static int ahci_dma_add_status(IDEDMA *dma, int status)
>  
>  static int ahci_dma_set_inactive(IDEDMA *dma)
>  {
> +    return 0;
> +}
> +
> +static int ahci_async_cmd_done(IDEDMA *dma)
> +{
>      AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
>  
> -    DPRINTF(ad->port_no, "dma done\n");
> +    DPRINTF(ad->port_no, "async cmd done\n");
>  
>      /* update d2h status */
>      ahci_write_fis_d2h(ad, NULL);
> @@ -1144,6 +1149,7 @@ static const IDEDMAOps ahci_dma_ops = {
>      .set_unit = ahci_dma_set_unit,
>      .add_status = ahci_dma_add_status,
>      .set_inactive = ahci_dma_set_inactive,
> +    .async_cmd_done = ahci_async_cmd_done,
>      .restart_cb = ahci_dma_restart_cb,
>      .reset = ahci_dma_reset,
>  };
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 03d1cfa..a73af72 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -568,10 +568,18 @@ static void dma_buf_commit(IDEState *s)
>      qemu_sglist_destroy(&s->sg);
>  }
>  
> +static void ide_async_cmd_done(IDEState *s)
> +{
> +    if (s->bus->dma->ops->async_cmd_done) {
> +        s->bus->dma->ops->async_cmd_done(s->bus->dma);
> +    }
> +}
> +
>  void ide_set_inactive(IDEState *s)
>  {
>      s->bus->dma->aiocb = NULL;
>      s->bus->dma->ops->set_inactive(s->bus->dma);
> +    ide_async_cmd_done(s);
>  }
>  
>  void ide_dma_error(IDEState *s)
> @@ -804,6 +812,7 @@ static void ide_flush_cb(void *opaque, int ret)
>  
>      bdrv_acct_done(s->bs, &s->acct);
>      s->status = READY_STAT | SEEK_STAT;
> +    ide_async_cmd_done(s);
>      ide_set_irq(s->bus);
>  }
>  
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 03f1489..048a052 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -433,6 +433,7 @@ struct IDEDMAOps {
>      DMAIntFunc *set_unit;
>      DMAIntFunc *add_status;
>      DMAFunc *set_inactive;
> +    DMAFunc *async_cmd_done;
>      DMARestartFunc *restart_cb;
>      DMAFunc *reset;
>  };

Re: [Qemu-devel] [PATCH] ahci: Fix FLUSH command

[Fam Zheng]

On Mon, 07/15 11:34, Kevin Wolf wrote:
> AHCI couldn't cope with asynchronous commands that aren't doing DMA, it
> simply wouldn't complete them. Due to the bug fixed in commit f68ec837,
> FLUSH commands would seem to have completed immediately even if they
> were still running on the host. After the commit, they would simply hang
> and never unset the BSY bit, rendering AHCI unusable on any OS sending
> flushes.
> 
> This patch adds another callback for the completion of asynchronous
> commands. This is what AHCI really wants to use for its command
> completion logic rather than an DMA completion callback.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Works for me, thanks.

Tested-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH] ahci: Fix FLUSH command

[Andreas Färber]

Am 15.07.2013 11:34, schrieb Kevin Wolf:
> AHCI couldn't cope with asynchronous commands that aren't doing DMA, it
> simply wouldn't complete them. Due to the bug fixed in commit f68ec837,
> FLUSH commands would seem to have completed immediately even if they
> were still running on the host. After the commit, they would simply hang
> and never unset the BSY bit, rendering AHCI unusable on any OS sending
> flushes.
> 
> This patch adds another callback for the completion of asynchronous
> commands. This is what AHCI really wants to use for its command
> completion logic rather than an DMA completion callback.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/ide/ahci.c     | 8 +++++++-
>  hw/ide/core.c     | 9 +++++++++
>  hw/ide/internal.h | 1 +
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 97eddec..1d863b5 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1107,9 +1107,14 @@ static int ahci_dma_add_status(IDEDMA *dma, int status)
>  
>  static int ahci_dma_set_inactive(IDEDMA *dma)
>  {
> +    return 0;

Is it intentional that this is now no-op rather than calling
dma->ops->set_inactive(dma) like core IDE does below?

Other than that it looks sensible to me.

Andreas

> +}
> +
> +static int ahci_async_cmd_done(IDEDMA *dma)
> +{
>      AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
>  
> -    DPRINTF(ad->port_no, "dma done\n");
> +    DPRINTF(ad->port_no, "async cmd done\n");
>  
>      /* update d2h status */
>      ahci_write_fis_d2h(ad, NULL);
> @@ -1144,6 +1149,7 @@ static const IDEDMAOps ahci_dma_ops = {
>      .set_unit = ahci_dma_set_unit,
>      .add_status = ahci_dma_add_status,
>      .set_inactive = ahci_dma_set_inactive,
> +    .async_cmd_done = ahci_async_cmd_done,
>      .restart_cb = ahci_dma_restart_cb,
>      .reset = ahci_dma_reset,
>  };
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 03d1cfa..a73af72 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -568,10 +568,18 @@ static void dma_buf_commit(IDEState *s)
>      qemu_sglist_destroy(&s->sg);
>  }
>  
> +static void ide_async_cmd_done(IDEState *s)
> +{
> +    if (s->bus->dma->ops->async_cmd_done) {
> +        s->bus->dma->ops->async_cmd_done(s->bus->dma);
> +    }
> +}
> +
>  void ide_set_inactive(IDEState *s)
>  {
>      s->bus->dma->aiocb = NULL;
>      s->bus->dma->ops->set_inactive(s->bus->dma);
> +    ide_async_cmd_done(s);
>  }
>  
>  void ide_dma_error(IDEState *s)
> @@ -804,6 +812,7 @@ static void ide_flush_cb(void *opaque, int ret)
>  
>      bdrv_acct_done(s->bs, &s->acct);
>      s->status = READY_STAT | SEEK_STAT;
> +    ide_async_cmd_done(s);
>      ide_set_irq(s->bus);
>  }
>  
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 03f1489..048a052 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -433,6 +433,7 @@ struct IDEDMAOps {
>      DMAIntFunc *set_unit;
>      DMAIntFunc *add_status;
>      DMAFunc *set_inactive;
> +    DMAFunc *async_cmd_done;
>      DMARestartFunc *restart_cb;
>      DMAFunc *reset;
>  };
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] ahci: Fix FLUSH command

[Michael S. Tsirkin]

On Mon, Jul 15, 2013 at 11:34:31AM +0200, Kevin Wolf wrote:
> AHCI couldn't cope with asynchronous commands that aren't doing DMA, it
> simply wouldn't complete them. Due to the bug fixed in commit f68ec837,
> FLUSH commands would seem to have completed immediately even if they
> were still running on the host. After the commit, they would simply hang
> and never unset the BSY bit, rendering AHCI unusable on any OS sending
> flushes.
> 
> This patch adds another callback for the completion of asynchronous
> commands. This is what AHCI really wants to use for its command
> completion logic rather than an DMA completion callback.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>


Tested-by: "Michael S. Tsirkin" <mst@redhat.com>

> ---
>  hw/ide/ahci.c     | 8 +++++++-
>  hw/ide/core.c     | 9 +++++++++
>  hw/ide/internal.h | 1 +
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 97eddec..1d863b5 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1107,9 +1107,14 @@ static int ahci_dma_add_status(IDEDMA *dma, int status)
>  
>  static int ahci_dma_set_inactive(IDEDMA *dma)
>  {
> +    return 0;
> +}
> +
> +static int ahci_async_cmd_done(IDEDMA *dma)
> +{
>      AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
>  
> -    DPRINTF(ad->port_no, "dma done\n");
> +    DPRINTF(ad->port_no, "async cmd done\n");
>  
>      /* update d2h status */
>      ahci_write_fis_d2h(ad, NULL);
> @@ -1144,6 +1149,7 @@ static const IDEDMAOps ahci_dma_ops = {
>      .set_unit = ahci_dma_set_unit,
>      .add_status = ahci_dma_add_status,
>      .set_inactive = ahci_dma_set_inactive,
> +    .async_cmd_done = ahci_async_cmd_done,
>      .restart_cb = ahci_dma_restart_cb,
>      .reset = ahci_dma_reset,
>  };
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 03d1cfa..a73af72 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -568,10 +568,18 @@ static void dma_buf_commit(IDEState *s)
>      qemu_sglist_destroy(&s->sg);
>  }
>  
> +static void ide_async_cmd_done(IDEState *s)
> +{
> +    if (s->bus->dma->ops->async_cmd_done) {
> +        s->bus->dma->ops->async_cmd_done(s->bus->dma);
> +    }
> +}
> +
>  void ide_set_inactive(IDEState *s)
>  {
>      s->bus->dma->aiocb = NULL;
>      s->bus->dma->ops->set_inactive(s->bus->dma);
> +    ide_async_cmd_done(s);
>  }
>  
>  void ide_dma_error(IDEState *s)
> @@ -804,6 +812,7 @@ static void ide_flush_cb(void *opaque, int ret)
>  
>      bdrv_acct_done(s->bs, &s->acct);
>      s->status = READY_STAT | SEEK_STAT;
> +    ide_async_cmd_done(s);
>      ide_set_irq(s->bus);
>  }
>  
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 03f1489..048a052 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -433,6 +433,7 @@ struct IDEDMAOps {
>      DMAIntFunc *set_unit;
>      DMAIntFunc *add_status;
>      DMAFunc *set_inactive;
> +    DMAFunc *async_cmd_done;
>      DMARestartFunc *restart_cb;
>      DMAFunc *reset;
>  };
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH] ahci: Fix FLUSH command

[Kevin Wolf]

Am 16.07.2013 um 14:30 hat Andreas Färber geschrieben:
> Am 15.07.2013 11:34, schrieb Kevin Wolf:
> > AHCI couldn't cope with asynchronous commands that aren't doing DMA, it
> > simply wouldn't complete them. Due to the bug fixed in commit f68ec837,
> > FLUSH commands would seem to have completed immediately even if they
> > were still running on the host. After the commit, they would simply hang
> > and never unset the BSY bit, rendering AHCI unusable on any OS sending
> > flushes.
> > 
> > This patch adds another callback for the completion of asynchronous
> > commands. This is what AHCI really wants to use for its command
> > completion logic rather than an DMA completion callback.
> > 
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  hw/ide/ahci.c     | 8 +++++++-
> >  hw/ide/core.c     | 9 +++++++++
> >  hw/ide/internal.h | 1 +
> >  3 files changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> > index 97eddec..1d863b5 100644
> > --- a/hw/ide/ahci.c
> > +++ b/hw/ide/ahci.c
> > @@ -1107,9 +1107,14 @@ static int ahci_dma_add_status(IDEDMA *dma, int status)
> >  
> >  static int ahci_dma_set_inactive(IDEDMA *dma)
> >  {
> > +    return 0;
> 
> Is it intentional that this is now no-op rather than calling
> dma->ops->set_inactive(dma) like core IDE does below?

This _is_ the dma->ops->set_inactive() callback of AHCI that is called
by the IDE code. For plain IDE commands the same IDEDMAOps function is
implemented by ide_nop().

Kevin

> Other than that it looks sensible to me.
> 
> Andreas
> 
> > +}
> > +
> > +static int ahci_async_cmd_done(IDEDMA *dma)
> > +{
> >      AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
> >  
> > -    DPRINTF(ad->port_no, "dma done\n");
> > +    DPRINTF(ad->port_no, "async cmd done\n");
> >  
> >      /* update d2h status */
> >      ahci_write_fis_d2h(ad, NULL);
> > @@ -1144,6 +1149,7 @@ static const IDEDMAOps ahci_dma_ops = {
> >      .set_unit = ahci_dma_set_unit,
> >      .add_status = ahci_dma_add_status,
> >      .set_inactive = ahci_dma_set_inactive,
> > +    .async_cmd_done = ahci_async_cmd_done,
> >      .restart_cb = ahci_dma_restart_cb,
> >      .reset = ahci_dma_reset,
> >  };
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index 03d1cfa..a73af72 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -568,10 +568,18 @@ static void dma_buf_commit(IDEState *s)
> >      qemu_sglist_destroy(&s->sg);
> >  }
> >  
> > +static void ide_async_cmd_done(IDEState *s)
> > +{
> > +    if (s->bus->dma->ops->async_cmd_done) {
> > +        s->bus->dma->ops->async_cmd_done(s->bus->dma);
> > +    }
> > +}
> > +
> >  void ide_set_inactive(IDEState *s)
> >  {
> >      s->bus->dma->aiocb = NULL;
> >      s->bus->dma->ops->set_inactive(s->bus->dma);
> > +    ide_async_cmd_done(s);
> >  }
> >  
> >  void ide_dma_error(IDEState *s)
> > @@ -804,6 +812,7 @@ static void ide_flush_cb(void *opaque, int ret)
> >  
> >      bdrv_acct_done(s->bs, &s->acct);
> >      s->status = READY_STAT | SEEK_STAT;
> > +    ide_async_cmd_done(s);
> >      ide_set_irq(s->bus);
> >  }
> >  
> > diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> > index 03f1489..048a052 100644
> > --- a/hw/ide/internal.h
> > +++ b/hw/ide/internal.h
> > @@ -433,6 +433,7 @@ struct IDEDMAOps {
> >      DMAIntFunc *set_unit;
> >      DMAIntFunc *add_status;
> >      DMAFunc *set_inactive;
> > +    DMAFunc *async_cmd_done;
> >      DMARestartFunc *restart_cb;
> >      DMAFunc *reset;
> >  };
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg