Re: [Qemu-devel] [PATCH] ahci: properly reset PxCMD on HBA reset

[Jason Baron <jbaron@redhat.com>]

On Fri, Aug 31, 2012 at 12:12:22PM +0200, Kevin Wolf wrote:
> Am 23.08.2012 23:09, schrieb Jason Baron:
> > While testing q35, I found that windows 7 (specifically, windows 7 ultimate
> > with sp1 x64), wouldn't install because it can't find the cdrom or disk drive.
> > The failure message is: 'A required cd/dvd device driver is missing. If you
> > have a driver floppy disk, CD, DVD, or USB flash drive, please insert it now.'
> > This can also be reproduced on piix by adding an ahci controller, and
> > observing that windows 7 does not see any devices behind it.
> > 
> > The problem is that when windows issues a HBA reset, qemu does not reset the
> > individual ports' PxCMD register. Windows 7 then reads back the PxCMD register
> > and presumably assumes that the ahci controller has already been initialized.
> > Windows then never sets up the PxIE register to enable interrupts, and thus it
> > never gets irqs back when it sends ata device inquiry commands.
> > 
> > I believe this change brings qemu into ahci 1.3 specification compliance.
> > 
> > Section 10.4.3 HBA Reset:
> > 
> > "
> > When GHC.HR is set to '1', GHC.AE, GHC.IE, the IS register, and all port
> > register fields (except PxFB/PxFBU/PxCLB/PxCLBU) that are not HwInit in the
> > HBA's register memory space are reset.
> > "
> > 
> > I've also re-tested Fedora 16 and 17 to verify that they continue to work with
> > this change.
> > 
> > Signed-off-by: Jason Baron <jbaron@redhat.com>
> > ---
> >  hw/ide/ahci.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> > index 9fe89a5..06c236f 100644
> > --- a/hw/ide/ahci.c
> > +++ b/hw/ide/ahci.c
> > @@ -1199,6 +1199,7 @@ void ahci_reset(AHCIState *s)
> >          pr->irq_stat = 0;
> >          pr->irq_mask = 0;
> >          pr->scr_ctl = 0;
> > +        pr->cmd = 0;
> >          ahci_reset_port(s, i);
> >      }
> >  }
> > 
> 
> In ahci_init() we have:
> 
>   ad->port_regs.cmd = PORT_CMD_SPIN_UP | PORT_CMD_POWER_ON;
> 
> Why is resetting to 0 in ahci_reset() correct? I think we can still get
> this part committed for 1.2 after you either explain why 0 is right or
> send a fixed version.
> 

Good catch. I just tried out the following instead in ahci_reset():

pr->cmd = PORT_CMD_SPIN_UP | PORT_CMD_POWER_ON;

And it continues to make Windows7 find the ahci disks. I'd like to do a
little more testing, but I will send an updated path.

> I think for 1.3 there's still a bit more left. We're missing some more
> fields in a reset: tfdata isn't set at all, should be set to 0x7f in
> ahci_reset_port(), and cmd_issue should be set to 0. And shouldn't
> ahci_init() actually call ahci_reset() in order to ensure consistent
> results?
> 

Agreed, those all look like good additional cleanups for 1.3. I will add
them to my q35 treee for testing.

Thanks,

-Jason