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

[Jason Baron]

On Fri, Aug 31, 2012 at 01:29:46PM -0700, Alexander Graf wrote:
> On 31.08.2012, at 12:15, Jason Baron <address@hidden> wrote:
> 
> > On Fri, Aug 31, 2012 at 12:04:59PM -0700, Alexander Graf wrote:
> >> On 31.08.2012, at 07:23, Jason Baron wrote:
> >> 
> >>> 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 <address@hidden>
> >>>>> ---
> >>>>> 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.
> >> 
> >> Since we reset on init, we can just shove it there completely, right?
> >> 
> >> 
> > 
> > Actually, we don't call reset on init. I'm thinking for 1.2, we can just
> > add the fix to the reset path, and as Kevin suggested in 1.3, we will
> > look to add a call to ahci_reset() in the init path.
> 
> We register the reset handler as generic reset path, no? It should be called 
> on machine init then. Every reset handler gets called on machine boot.
> 

Right, I guess I was concerned that there might be a path where init is
called but not reset. I also was being cautious as I wanted this for
1.2. That said, it does seem safe to remove it from init.

Thanks,

-Jason