| Message ID | 20180531004323.4611-3-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ahci: fix completion race condition | expand |
From comments on the cover letter, I am adding: Fixes: https://bugs.launchpad.net/qemu/+bug/1769189 On 05/30/2018 08:43 PM, John Snow wrote: > AHCI presently signals completion prior to the PxCI register being > cleared to indicate completion. If a guest driver attempts to issue > a new command in its IRQ handler, it might be surprised to learn there > is still a command pending. > > In the case of Windows 10's boot driver, it will actually poll the IRQ > register hoping to find out when the command is done running -- which > will never happen, as there isn't a command running. > > Fix this: clear PxCI in ahci_cmd_done and not in the asynchronous BH. > Because it now runs synchronously, we don't need to check if the command > is actually done by spying on the ATA registers. We know it's done. > > Signed-off-by: John Snow <jsnow@redhat.com> and: CC: qemu-stable <qemu-stable@nongnu.org> Reported-by: François Guerraz <kubrick@fgv6.net> Tested-by: Bruce Rogers <brogers@suse.com> (Stable note: only patch #2 here is strictly required.) > --- > hw/ide/ahci.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > index b7a6f68790..a9558e45e7 100644 > --- a/hw/ide/ahci.c > +++ b/hw/ide/ahci.c > @@ -532,13 +532,6 @@ static void ahci_check_cmd_bh(void *opaque) > qemu_bh_delete(ad->check_bh); > ad->check_bh = NULL; > > - if ((ad->busy_slot != -1) && > - !(ad->port.ifs[0].status & (BUSY_STAT|DRQ_STAT))) { > - /* no longer busy */ > - ad->port_regs.cmd_issue &= ~(1 << ad->busy_slot); > - ad->busy_slot = -1; > - } > - > check_cmd(ad->hba, ad->port_no); > } > > @@ -1425,6 +1418,12 @@ static void ahci_cmd_done(IDEDMA *dma) > > trace_ahci_cmd_done(ad->hba, ad->port_no); > > + /* no longer busy */ > + if (ad->busy_slot != -1) { > + ad->port_regs.cmd_issue &= ~(1 << ad->busy_slot); > + ad->busy_slot = -1; > + } > + > /* update d2h status */ > ahci_write_fis_d2h(ad); > >
On Wed, May 30, 2018 at 08:43:22PM -0400, John Snow wrote: > AHCI presently signals completion prior to the PxCI register being > cleared to indicate completion. If a guest driver attempts to issue > a new command in its IRQ handler, it might be surprised to learn there > is still a command pending. > > In the case of Windows 10's boot driver, it will actually poll the IRQ > register hoping to find out when the command is done running -- which > will never happen, as there isn't a command running. > > Fix this: clear PxCI in ahci_cmd_done and not in the asynchronous BH. > Because it now runs synchronously, we don't need to check if the command > is actually done by spying on the ATA registers. We know it's done. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > hw/ide/ahci.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > index b7a6f68790..a9558e45e7 100644 > --- a/hw/ide/ahci.c > +++ b/hw/ide/ahci.c > @@ -532,13 +532,6 @@ static void ahci_check_cmd_bh(void *opaque) > qemu_bh_delete(ad->check_bh); > ad->check_bh = NULL; > > - if ((ad->busy_slot != -1) && > - !(ad->port.ifs[0].status & (BUSY_STAT|DRQ_STAT))) { > - /* no longer busy */ > - ad->port_regs.cmd_issue &= ~(1 << ad->busy_slot); > - ad->busy_slot = -1; > - } > - > check_cmd(ad->hba, ad->port_no); > } > > @@ -1425,6 +1418,12 @@ static void ahci_cmd_done(IDEDMA *dma) > > trace_ahci_cmd_done(ad->hba, ad->port_no); > > + /* no longer busy */ > + if (ad->busy_slot != -1) { > + ad->port_regs.cmd_issue &= ~(1 << ad->busy_slot); > + ad->busy_slot = -1; > + } > + > /* update d2h status */ > ahci_write_fis_d2h(ad); > > -- > 2.14.3 > > Reviewed-by: Jeff Cody <jcody@redhat.com>
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index b7a6f68790..a9558e45e7 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -532,13 +532,6 @@ static void ahci_check_cmd_bh(void *opaque) qemu_bh_delete(ad->check_bh); ad->check_bh = NULL; - if ((ad->busy_slot != -1) && - !(ad->port.ifs[0].status & (BUSY_STAT|DRQ_STAT))) { - /* no longer busy */ - ad->port_regs.cmd_issue &= ~(1 << ad->busy_slot); - ad->busy_slot = -1; - } - check_cmd(ad->hba, ad->port_no); } @@ -1425,6 +1418,12 @@ static void ahci_cmd_done(IDEDMA *dma) trace_ahci_cmd_done(ad->hba, ad->port_no); + /* no longer busy */ + if (ad->busy_slot != -1) { + ad->port_regs.cmd_issue &= ~(1 << ad->busy_slot); + ad->busy_slot = -1; + } + /* update d2h status */ ahci_write_fis_d2h(ad);
AHCI presently signals completion prior to the PxCI register being cleared to indicate completion. If a guest driver attempts to issue a new command in its IRQ handler, it might be surprised to learn there is still a command pending. In the case of Windows 10's boot driver, it will actually poll the IRQ register hoping to find out when the command is done running -- which will never happen, as there isn't a command running. Fix this: clear PxCI in ahci_cmd_done and not in the asynchronous BH. Because it now runs synchronously, we don't need to check if the command is actually done by spying on the ATA registers. We know it's done. Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/ahci.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)