diff mbox

AHCI broken in current git, bisected.

Message ID 20110515163105.GG24932@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin May 15, 2011, 4:32 p.m. UTC 
On Sun, May 15, 2011 at 07:58:23PM +0400, Alexey Zaytsev wrote:
> Hi.
> 
> The commit 667bb59d2358daeef179583c944becba3f1f9680
> Author: Avi Kivity <avi@redhat.com>
> Date:   Mon Apr 4 18:28:02 2011 +0300
> 
>     ich/ahci: convert to pci_register_bar_simple()
> 
>     Signed-off-by: Avi Kivity <avi@redhat.com>
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> breaks AHCI to the point the disks are not detected by both seabios and Linux:
> 
> [    8.582220] ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xc000 irq 14
> [    8.582646] ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xc008 irq 15
> [    8.603979] ACPI: PCI Interrupt Link [LNKD] enabled at IRQ 10
> [    8.605151] ahci 0000:00:04.0: PCI INT A -> Link[LNKD] -> GSI 10
> (level, high) -> IRQ 10
> [    9.608243] ahci 0000:00:04.0: controller reset failed (0xf000ff53)
> [    9.609948] ahci 0000:00:04.0: PCI INT A disabled
> [    9.610267] ahci: probe of 0000:00:04.0 failed with error -5

Sure enough,

    /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now */
    pci_register_bar_simple(&d->card, 5, 0x1000, 0, d->ahci.mem);
    msi_init(dev, 0x50, 1, true, false);
    ahci_init(&d->ahci, &dev->qdev, 6);

where ahci_init initializes d->ahci.mem.

Coul;d you try out the following please
(untested, a bit busy now)?

--->

    ich/ahci: fix use of uninitialized memory
    
    The commit 667bb59d2358daeef179583c944becba3f1f9680
    uses d->ahci.mem before it is initialized by
    ahci_init(). Fix this by calling ahci_init() first thing
    so that it's safe to use all fields in the ahci state struct.
    
    Reported-by: Alexey Zaytsev <alexey.zaytsev@gmail.com>
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Comments

Alexey Zaytsev May 15, 2011, 5:06 p.m. UTC | #1 
On Sun, May 15, 2011 at 20:32, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, May 15, 2011 at 07:58:23PM +0400, Alexey Zaytsev wrote:
>> Hi.
>>
>> The commit 667bb59d2358daeef179583c944becba3f1f9680
>> Author: Avi Kivity <avi@redhat.com>
>> Date:   Mon Apr 4 18:28:02 2011 +0300
>>
>>     ich/ahci: convert to pci_register_bar_simple()
>>
>>     Signed-off-by: Avi Kivity <avi@redhat.com>
>>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> breaks AHCI to the point the disks are not detected by both seabios and Linux:
>>
>> [    8.582220] ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xc000 irq 14
>> [    8.582646] ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xc008 irq 15
>> [    8.603979] ACPI: PCI Interrupt Link [LNKD] enabled at IRQ 10
>> [    8.605151] ahci 0000:00:04.0: PCI INT A -> Link[LNKD] -> GSI 10
>> (level, high) -> IRQ 10
>> [    9.608243] ahci 0000:00:04.0: controller reset failed (0xf000ff53)
>> [    9.609948] ahci 0000:00:04.0: PCI INT A disabled
>> [    9.610267] ahci: probe of 0000:00:04.0 failed with error -5
>
> Sure enough,
>
>    /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now */
>    pci_register_bar_simple(&d->card, 5, 0x1000, 0, d->ahci.mem);
>    msi_init(dev, 0x50, 1, true, false);
>    ahci_init(&d->ahci, &dev->qdev, 6);
>
> where ahci_init initializes d->ahci.mem.
>
> Coul;d you try out the following please
> (untested, a bit busy now)?
>
> --->

Works, thank you.
Jan Kiszka May 15, 2011, 5:14 p.m. UTC | #2 
On 2011-05-15 18:32, Michael S. Tsirkin wrote:
> On Sun, May 15, 2011 at 07:58:23PM +0400, Alexey Zaytsev wrote:
>> Hi.
>>
>> The commit 667bb59d2358daeef179583c944becba3f1f9680
>> Author: Avi Kivity <avi@redhat.com>
>> Date:   Mon Apr 4 18:28:02 2011 +0300
>>
>>     ich/ahci: convert to pci_register_bar_simple()
>>
>>     Signed-off-by: Avi Kivity <avi@redhat.com>
>>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> breaks AHCI to the point the disks are not detected by both seabios and Linux:
>>
>> [    8.582220] ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xc000 irq 14
>> [    8.582646] ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xc008 irq 15
>> [    8.603979] ACPI: PCI Interrupt Link [LNKD] enabled at IRQ 10
>> [    8.605151] ahci 0000:00:04.0: PCI INT A -> Link[LNKD] -> GSI 10
>> (level, high) -> IRQ 10
>> [    9.608243] ahci 0000:00:04.0: controller reset failed (0xf000ff53)
>> [    9.609948] ahci 0000:00:04.0: PCI INT A disabled
>> [    9.610267] ahci: probe of 0000:00:04.0 failed with error -5
> 
> Sure enough,
> 
>     /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now */
>     pci_register_bar_simple(&d->card, 5, 0x1000, 0, d->ahci.mem);
>     msi_init(dev, 0x50, 1, true, false);
>     ahci_init(&d->ahci, &dev->qdev, 6);
> 
> where ahci_init initializes d->ahci.mem.
> 
> Coul;d you try out the following please
> (untested, a bit busy now)?

See also http://permalink.gmane.org/gmane.comp.emulators.qemu/101978

Jan
Alexey Zaytsev May 15, 2011, 5:28 p.m. UTC | #3 
On Sun, May 15, 2011 at 21:14, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-05-15 18:32, Michael S. Tsirkin wrote:
>> On Sun, May 15, 2011 at 07:58:23PM +0400, Alexey Zaytsev wrote:
>>> Hi.
>>>
>>> The commit 667bb59d2358daeef179583c944becba3f1f9680
>>> Author: Avi Kivity <avi@redhat.com>
>>> Date:   Mon Apr 4 18:28:02 2011 +0300
>>>
>>>     ich/ahci: convert to pci_register_bar_simple()
>>>
>>>     Signed-off-by: Avi Kivity <avi@redhat.com>
>>>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>
>>> breaks AHCI to the point the disks are not detected by both seabios and Linux:
>>>
>>> [    8.582220] ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xc000 irq 14
>>> [    8.582646] ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xc008 irq 15
>>> [    8.603979] ACPI: PCI Interrupt Link [LNKD] enabled at IRQ 10
>>> [    8.605151] ahci 0000:00:04.0: PCI INT A -> Link[LNKD] -> GSI 10
>>> (level, high) -> IRQ 10
>>> [    9.608243] ahci 0000:00:04.0: controller reset failed (0xf000ff53)
>>> [    9.609948] ahci 0000:00:04.0: PCI INT A disabled
>>> [    9.610267] ahci: probe of 0000:00:04.0 failed with error -5
>>
>> Sure enough,
>>
>>     /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now */
>>     pci_register_bar_simple(&d->card, 5, 0x1000, 0, d->ahci.mem);
>>     msi_init(dev, 0x50, 1, true, false);
>>     ahci_init(&d->ahci, &dev->qdev, 6);
>>
>> where ahci_init initializes d->ahci.mem.
>>
>> Coul;d you try out the following please
>> (untested, a bit busy now)?
>
> See also http://permalink.gmane.org/gmane.comp.emulators.qemu/101978
>

Hey, thanks! I should look harder next time. ;)
Michael S. Tsirkin May 15, 2011, 7:20 p.m. UTC | #4 
On Sun, May 15, 2011 at 07:14:42PM +0200, Jan Kiszka wrote:
> On 2011-05-15 18:32, Michael S. Tsirkin wrote:
> > On Sun, May 15, 2011 at 07:58:23PM +0400, Alexey Zaytsev wrote:
> >> Hi.
> >>
> >> The commit 667bb59d2358daeef179583c944becba3f1f9680
> >> Author: Avi Kivity <avi@redhat.com>
> >> Date:   Mon Apr 4 18:28:02 2011 +0300
> >>
> >>     ich/ahci: convert to pci_register_bar_simple()
> >>
> >>     Signed-off-by: Avi Kivity <avi@redhat.com>
> >>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >> breaks AHCI to the point the disks are not detected by both seabios and Linux:
> >>
> >> [    8.582220] ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xc000 irq 14
> >> [    8.582646] ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xc008 irq 15
> >> [    8.603979] ACPI: PCI Interrupt Link [LNKD] enabled at IRQ 10
> >> [    8.605151] ahci 0000:00:04.0: PCI INT A -> Link[LNKD] -> GSI 10
> >> (level, high) -> IRQ 10
> >> [    9.608243] ahci 0000:00:04.0: controller reset failed (0xf000ff53)
> >> [    9.609948] ahci 0000:00:04.0: PCI INT A disabled
> >> [    9.610267] ahci: probe of 0000:00:04.0 failed with error -5
> > 
> > Sure enough,
> > 
> >     /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now */
> >     pci_register_bar_simple(&d->card, 5, 0x1000, 0, d->ahci.mem);
> >     msi_init(dev, 0x50, 1, true, false);
> >     ahci_init(&d->ahci, &dev->qdev, 6);
> > 
> > where ahci_init initializes d->ahci.mem.
> > 
> > Coul;d you try out the following please
> > (untested, a bit busy now)?
> 
> See also http://permalink.gmane.org/gmane.comp.emulators.qemu/101978
> 
> Jan
> 

Pity I missed this the first time. Thanks!
Michael S. Tsirkin May 15, 2011, 8:03 p.m. UTC | #5 
On Sun, May 15, 2011 at 10:20:23PM +0300, Michael S. Tsirkin wrote:
> On Sun, May 15, 2011 at 07:14:42PM +0200, Jan Kiszka wrote:
> > On 2011-05-15 18:32, Michael S. Tsirkin wrote:
> > > On Sun, May 15, 2011 at 07:58:23PM +0400, Alexey Zaytsev wrote:
> > >> Hi.
> > >>
> > >> The commit 667bb59d2358daeef179583c944becba3f1f9680
> > >> Author: Avi Kivity <avi@redhat.com>
> > >> Date:   Mon Apr 4 18:28:02 2011 +0300
> > >>
> > >>     ich/ahci: convert to pci_register_bar_simple()
> > >>
> > >>     Signed-off-by: Avi Kivity <avi@redhat.com>
> > >>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >>
> > >> breaks AHCI to the point the disks are not detected by both seabios and Linux:
> > >>
> > >> [    8.582220] ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xc000 irq 14
> > >> [    8.582646] ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xc008 irq 15
> > >> [    8.603979] ACPI: PCI Interrupt Link [LNKD] enabled at IRQ 10
> > >> [    8.605151] ahci 0000:00:04.0: PCI INT A -> Link[LNKD] -> GSI 10
> > >> (level, high) -> IRQ 10
> > >> [    9.608243] ahci 0000:00:04.0: controller reset failed (0xf000ff53)
> > >> [    9.609948] ahci 0000:00:04.0: PCI INT A disabled
> > >> [    9.610267] ahci: probe of 0000:00:04.0 failed with error -5
> > > 
> > > Sure enough,
> > > 
> > >     /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now */
> > >     pci_register_bar_simple(&d->card, 5, 0x1000, 0, d->ahci.mem);
> > >     msi_init(dev, 0x50, 1, true, false);
> > >     ahci_init(&d->ahci, &dev->qdev, 6);
> > > 
> > > where ahci_init initializes d->ahci.mem.
> > > 
> > > Coul;d you try out the following please
> > > (untested, a bit busy now)?
> > 
> > See also http://permalink.gmane.org/gmane.comp.emulators.qemu/101978
> > 
> > Jan
> > 
> 
> Pity I missed this the first time. Thanks!

Ah, I see, I didn't miss it, just back from vacation and didn't get so
far back in the backlog yet.

> -- 
> MST
Anthony Liguori May 16, 2011, 4:20 p.m. UTC | #6 
On 05/15/2011 03:03 PM, Michael S. Tsirkin wrote:
> On Sun, May 15, 2011 at 10:20:23PM +0300, Michael S. Tsirkin wrote:
>> On Sun, May 15, 2011 at 07:14:42PM +0200, Jan Kiszka wrote:
>>
>> Pity I missed this the first time. Thanks!
>
> Ah, I see, I didn't miss it, just back from vacation and didn't get so
> far back in the backlog yet.

Applied Jan's patch.

Regards,

Anthony Liguori

>> --
>> MST
>
diff mbox

Patch

diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index e44339b..5ce0a88 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -77,6 +77,8 @@  static int pci_ich9_ahci_init(PCIDevice *dev)
     struct AHCIPCIState *d;
     d = DO_UPCAST(struct AHCIPCIState, card, dev);
 
+    ahci_init(&d->ahci, &dev->qdev, 6);
+
     pci_config_set_vendor_id(d->card.config, PCI_VENDOR_ID_INTEL);
     pci_config_set_device_id(d->card.config, PCI_DEVICE_ID_INTEL_82801IR);
 
@@ -97,8 +99,6 @@  static int pci_ich9_ahci_init(PCIDevice *dev)
     pci_register_bar_simple(&d->card, 5, 0x1000, 0, d->ahci.mem);
 
     msi_init(dev, 0x50, 1, true, false);
-
-    ahci_init(&d->ahci, &dev->qdev, 6);
     d->ahci.irq = d->card.irq[0];
 
     return 0;