From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33023) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T7WgS-0003Q6-B6 for qemu-devel@nongnu.org; Fri, 31 Aug 2012 15:15:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T7WgQ-0003jZ-UM for qemu-devel@nongnu.org; Fri, 31 Aug 2012 15:15:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47707) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T7WgQ-0003he-LW for qemu-devel@nongnu.org; Fri, 31 Aug 2012 15:15:10 -0400 Date: Fri, 31 Aug 2012 15:15:08 -0400 From: Jason Baron Message-ID: <20120831191507.GG12212@redhat.com> References: <201208232109.q7NL9P9t016424@int-mx09.intmail.prod.int.phx2.redhat.com> <50408E06.1070009@redhat.com> <20120831142305.GA12212@redhat.com> <128F51EA-270A-41FA-A284-530EB6599C41@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <128F51EA-270A-41FA-A284-530EB6599C41@suse.de> Subject: Re: [Qemu-devel] [PATCH] ahci: properly reset PxCMD on HBA reset List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Kevin Wolf , aliguori@us.ibm.com, mst@redhat.com, jan.kiszka@siemens.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, yamahata@valinux.co.jp, alex.williamson@redhat.com, pbonzini@redhat.com, afaerber@suse.de, armbru@redhat.com 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 > >>> --- > >>> 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. Thanks, -Jason