Re: [Qemu-devel] [PATCH 12/23] ide: Convert FLUSH CACHE to ide_cmd_table handler

[Alex Williamson]

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;
 }