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

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

[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);
     }
 }
-- 
1.7.1

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

[Alexander Graf]

On 23.08.2012, at 23:09, Jason Baron wrote:

> 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.

What a nasty little bug. If it makes it work for you, the change is all fine from my POV (and should go into 1.2).

Thanks a lot for pulling off the effort to debug through all this. It sure must've been terribly hard.


Alex

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

[Jason Baron]

On Fri, Aug 24, 2012 at 06:39:02AM +0200, Alexander Graf wrote:
> > 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.
> 
> What a nasty little bug. If it makes it work for you, the change is all fine from my POV (and should go into 1.2).
> 

Ok, I don't see it 1.2.0-rc2. If others are ok with this for 1.2, whose
tree should this go through?

Thanks,

-Jason

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

[Alexander Graf]

On 30.08.2012, at 12:59, Jason Baron <jbaron@redhat.com> wrote:

> On Fri, Aug 24, 2012 at 06:39:02AM +0200, Alexander Graf wrote:
>>> 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.
>> 
>> What a nasty little bug. If it makes it work for you, the change is all fine from my POV (and should go into 1.2).
>> 
> 
> Ok, I don't see it 1.2.0-rc2. If others are ok with this for 1.2, whose
> tree should this go through?

Kevin :)

Alex

> 
> Thanks,
> 
> -Jason

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

[Luiz Capitulino]

On Thu, 23 Aug 2012 17:09:25 -0400
Jason Baron <jbaron@redhat.com> wrote:

> 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.

I've tested this but unfortunately it doesn't fix the issues I'm getting with
OpenBSD 5.1 and NetBSD 5.1 as reported here (section 'master with ahci'):

 http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg03329.html

PS: Those could be kernel issues, of course.

> 
> 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);
>      }
>  }

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

[Alexander Graf]

Am 24.08.2012 um 07:18 schrieb Luiz Capitulino <lcapitulino@redhat.com>:

> On Thu, 23 Aug 2012 17:09:25 -0400
> Jason Baron <jbaron@redhat.com> wrote:
> 
>> 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.
> 
> I've tested this but unfortunately it doesn't fix the issues I'm getting with
> OpenBSD 5.1 and NetBSD 5.1 as reported here (section 'master with ahci'):
> 
> http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg03329.html
> 
> PS: Those could be kernel issues, of course.

The BSDs already fail in detecting the cotroller as a whole, right? Not just disks on it.

Alex

> 
>> 
>> 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);
>>     }
>> }
> 

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

[Luiz Capitulino]

On Fri, 24 Aug 2012 07:34:51 -0700
Alexander Graf <agraf@suse.de> wrote:

> 
> 
> Am 24.08.2012 um 07:18 schrieb Luiz Capitulino <lcapitulino@redhat.com>:
> 
> > On Thu, 23 Aug 2012 17:09:25 -0400
> > Jason Baron <jbaron@redhat.com> wrote:
> > 
> >> 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.
> > 
> > I've tested this but unfortunately it doesn't fix the issues I'm getting with
> > OpenBSD 5.1 and NetBSD 5.1 as reported here (section 'master with ahci'):
> > 
> > http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg03329.html
> > 
> > PS: Those could be kernel issues, of course.
> 
> The BSDs already fail in detecting the cotroller as a whole, right? Not just disks on it.

Thanks for the clarification. Didn't try to understand the patch, automatically
thought it could be related to my last testing report.

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

[Kevin Wolf]

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.

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?

Kevin

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

[Jason Baron]

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

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

[Alexander Graf]

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 <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.

Since we reset on init, we can just shove it there completely, right?


Alex

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

[Jason Baron]

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 <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.
> 
> 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

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

[Alexander Graf]

On 31.08.2012, at 12:15, Jason Baron <jbaron@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 <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.
>> 
>> 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.

Alex

> 
> Thanks,
> 
> -Jason

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 <jbaron@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 <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.
> >> 
> >> 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

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 <jbaron@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 <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.
> >> 
> >> 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?

-Jason

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

[Alexander Graf]

On 31.08.2012, at 13:54, Jason Baron <jbaron@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 <jbaron@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 <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.
>>>> 
>>>> 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?

Alex

> 
> -Jason

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

[Jason Baron]

On Fri, Aug 31, 2012 at 11:25:52PM -0700, Alexander Graf wrote:
> On 31.08.2012, at 13:54, Jason Baron <jbaron@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 <jbaron@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 <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.
> >>>> 
> >>>> 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