[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 12/23] ide: Convert FLUSH CACHE to ide_cmd_table
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 12/23] ide: Convert FLUSH CACHE to ide_cmd_table handler |
Date: |
Thu, 4 Jul 2013 09:58:46 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 03.07.2013 um 23:51 hat Alex Williamson geschrieben:
> On Wed, 2013-07-03 at 15:41 -0600, Alex Williamson wrote:
> > On Mon, 2013-06-24 at 11:10 +0200, Stefan Hajnoczi wrote:
> > > From: Kevin Wolf <address@hidden>
> > >
> > > Signed-off-by: Kevin Wolf <address@hidden>
> > > Signed-off-by: Stefan Hajnoczi <address@hidden>
> > > ---
> > > hw/ide/core.c | 14 ++++++++------
> > > 1 file changed, 8 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > > index 8789758..83e86aa 100644
> > > --- a/hw/ide/core.c
> > > +++ b/hw/ide/core.c
> > > @@ -1184,6 +1184,12 @@ static bool cmd_write_dma(IDEState *s, uint8_t cmd)
> > > return false;
> > > }
> > >
> > > +static bool cmd_flush_cache(IDEState *s, uint8_t cmd)
> > > +{
> > > + ide_flush_cache(s);
> > > + return false;
> > > +}
> > > +
> > > static bool cmd_read_native_max(IDEState *s, uint8_t cmd)
> > > {
> > > bool lba48 = (cmd == WIN_READ_NATIVE_MAX_EXT);
> > > @@ -1345,8 +1351,8 @@ static const struct {
> > > [WIN_SETIDLE1] = { cmd_nop, ALL_OK },
> > > [WIN_CHECKPOWERMODE1] = { cmd_check_power_mode, ALL_OK |
> > > SET_DSC },
> > > [WIN_SLEEPNOW1] = { cmd_nop, ALL_OK },
> > > - [WIN_FLUSH_CACHE] = { NULL, ALL_OK },
> > > - [WIN_FLUSH_CACHE_EXT] = { NULL, HD_CFA_OK },
> > > + [WIN_FLUSH_CACHE] = { cmd_flush_cache, ALL_OK },
> > > + [WIN_FLUSH_CACHE_EXT] = { cmd_flush_cache, HD_CFA_OK },
> > > [WIN_IDENTIFY] = { cmd_identify, ALL_OK },
> > > [WIN_SETFEATURES] = { cmd_set_features, ALL_OK | SET_DSC
> > > },
> > > [IBM_SENSE_CONDITION] = { NULL, CFA_OK },
> > > @@ -1403,10 +1409,6 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
> > > }
> > >
> > > switch(val) {
> > > - case WIN_FLUSH_CACHE:
> > > - case WIN_FLUSH_CACHE_EXT:
> > > - ide_flush_cache(s);
> > > - break;
> > > case WIN_SEEK:
> > > /* XXX: Check that seek is within bounds */
> > > s->status = READY_STAT | SEEK_STAT;
> >
> > This also breaks win7 x64 q35 IDE. Note that while this change looks
> > like a no-op, filling in a handler now means that we do:
> >
> > s->status = READY_STAT | BUSY_STAT;
> >
> > before calling the handler and don't clear it on the way out since the
> > function statically returns false. This then introduces the same bug as
> > f68ec837. Thanks,
>
> This seems to work around the bug, but I'll leave it to those of you who
> actually know how IDE works for a proper fix:
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 96b468c..8893849 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1186,6 +1186,7 @@ static bool cmd_write_dma(IDEState *s, uint8_t cmd)
> static bool cmd_flush_cache(IDEState *s, uint8_t cmd)
> {
> ide_flush_cache(s);
> + s->status &= ~BUSY_STAT;
> return false;
> }
This is wrong, the BSY bit must remain set while the FLUSH command is
running. As I said in the other thread, the real problem is that AHCI
isn't notified about the command completion for flushes.
Kevin