On Fri, Aug 31, 2012 at 11:25:52PM -0700, Alexander Graf wrote: > On 31.08.2012, at 13:54, Jason Baron <jba...@redhat.com> wrote: > > > On Fri, Aug 31, 2012 at 01:29:46PM -0700, Alexander Graf wrote: > >> On 31.08.2012, at 12:15, Jason Baron <jba...@redhat.com> 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 <jba...@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. > >>>> > >>>> 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. > >> > > > > what about hotplug? There we call init, but not reset? > > Really? It would be broken then already, no? >
Yes, you are right - hotplug calls reset. I've thus dropped the clear from the init path. Please see my v2 posting. Thanks, -Jason
