[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] ahci: properly reset PxCMD on HBA reset
From: |
Jason Baron |
Subject: |
Re: [Qemu-devel] [PATCH] ahci: properly reset PxCMD on HBA reset |
Date: |
Tue, 4 Sep 2012 16:12:10 -0400 |
User-agent: |
Mutt/1.5.20 (2009-12-10) |
On Fri, Aug 31, 2012 at 11:25:52PM -0700, Alexander Graf wrote:
> On 31.08.2012, at 13:54, Jason Baron <address@hidden> wrote:
>
> > 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.
> >>
> >
> > 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