* Re: [Qemu-devel] [PATCH] ahci: Fix FLUSH command
2013-07-15 9:34 [Qemu-devel] [PATCH] ahci: Fix FLUSH command Kevin Wolf
@ 2013-07-15 9:47 ` Stefan Hajnoczi
2013-07-15 21:26 ` Alex Williamson
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2013-07-15 9:47 UTC (permalink / raw)
To: Kevin Wolf; +Cc: famz, mst, qemu-devel, qemu-stable, alex.williamson, afaerber
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>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] ahci: Fix FLUSH command
2013-07-15 9:34 [Qemu-devel] [PATCH] ahci: Fix FLUSH command Kevin Wolf
2013-07-15 9:47 ` Stefan Hajnoczi
@ 2013-07-15 21:26 ` Alex Williamson
2013-07-16 6:50 ` Fam Zheng
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Alex Williamson @ 2013-07-15 21:26 UTC (permalink / raw)
To: Kevin Wolf; +Cc: famz, mst, qemu-stable, qemu-devel, stefanha, afaerber
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;
> };
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] ahci: Fix FLUSH command
2013-07-15 9:34 [Qemu-devel] [PATCH] ahci: Fix FLUSH command Kevin Wolf
2013-07-15 9:47 ` Stefan Hajnoczi
2013-07-15 21:26 ` Alex Williamson
@ 2013-07-16 6:50 ` Fam Zheng
2013-07-16 12:30 ` Andreas Färber
2013-07-16 12:39 ` Michael S. Tsirkin
4 siblings, 0 replies; 7+ messages in thread
From: Fam Zheng @ 2013-07-16 6:50 UTC (permalink / raw)
To: Kevin Wolf
Cc: mst, qemu-stable, qemu-devel, alex.williamson, stefanha, afaerber
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] ahci: Fix FLUSH command
2013-07-15 9:34 [Qemu-devel] [PATCH] ahci: Fix FLUSH command Kevin Wolf
` (2 preceding siblings ...)
2013-07-16 6:50 ` Fam Zheng
@ 2013-07-16 12:30 ` Andreas Färber
2013-07-16 12:45 ` Kevin Wolf
2013-07-16 12:39 ` Michael S. Tsirkin
4 siblings, 1 reply; 7+ messages in thread
From: Andreas Färber @ 2013-07-16 12:30 UTC (permalink / raw)
To: Kevin Wolf; +Cc: famz, mst, qemu-stable, qemu-devel, alex.williamson, stefanha
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] ahci: Fix FLUSH command
2013-07-16 12:30 ` Andreas Färber
@ 2013-07-16 12:45 ` Kevin Wolf
0 siblings, 0 replies; 7+ messages in thread
From: Kevin Wolf @ 2013-07-16 12:45 UTC (permalink / raw)
To: Andreas Färber
Cc: famz, mst, qemu-stable, qemu-devel, alex.williamson, stefanha
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] ahci: Fix FLUSH command
2013-07-15 9:34 [Qemu-devel] [PATCH] ahci: Fix FLUSH command Kevin Wolf
` (3 preceding siblings ...)
2013-07-16 12:30 ` Andreas Färber
@ 2013-07-16 12:39 ` Michael S. Tsirkin
4 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2013-07-16 12:39 UTC (permalink / raw)
To: Kevin Wolf
Cc: famz, qemu-stable, qemu-devel, alex.williamson, stefanha,
afaerber
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
^ permalink raw reply [flat|nested] 7+ messages in thread