Wei Wang2
2010-Aug-11 13:43 UTC
[Xen-devel] [PATCH] msi: Avoid uninitialized msi descriptors
Hi, When __pci_enable_msix() returns early, output parameter (struct msi_desc **desc) will not be initialized. On my machine, a Broadcom BCM5709 nic has both MSI and MSIX capability blocks and when guest tries to enable msix interrupts but __pci_enable_msix() returns early for encountering a msi block, the whole system will crash for fatal page fault immediately. Thanks, Wei Signed-off-by: Wei Wang <wei.wang2@amd.com> -- AMD GmbH, Germany Operating System Research Center Legal Information: Advanced Micro Devices GmbH Karl-Hammerschmidt-Str. 34 85609 Dornach b. München Geschäftsführer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni Sitz: Dornach, Gemeinde Aschheim, Landkreis München Registergericht München, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Dante Cinco
2010-Aug-11 17:09 UTC
RE: [Xen-devel] [PATCH] msi: Avoid uninitialized msi descriptors
I''m glad that somebody who is familiar with MSI has resolved this problem although I haven''t applied and verified the proposed patch yet on my system. I posted the Xen crash I experienced related to this issue on xen-devel a couple of weeks ago (see link below) but wasn''t familiar enough with the MSI/MSI-X code to propose a solution other than to back out changelist 21778. http://old.nabble.com/Re:-(XEN)-RIP:-e008:-%3Cffff82c48015564b%3E---write_msi_msg%2B0x2b-0x150-p29301832.html Dante -----Original Message----- From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Wei Wang2 Sent: Wednesday, August 11, 2010 6:43 AM To: xen-devel@lists.xensource.com Subject: [Xen-devel] [PATCH] msi: Avoid uninitialized msi descriptors Hi, When __pci_enable_msix() returns early, output parameter (struct msi_desc **desc) will not be initialized. On my machine, a Broadcom BCM5709 nic has both MSI and MSIX capability blocks and when guest tries to enable msix interrupts but __pci_enable_msix() returns early for encountering a msi block, the whole system will crash for fatal page fault immediately. Thanks, Wei Signed-off-by: Wei Wang <wei.wang2@amd.com> -- AMD GmbH, Germany Operating System Research Center Legal Information: Advanced Micro Devices GmbH Karl-Hammerschmidt-Str. 34 85609 Dornach b. München Geschäftsführer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni Sitz: Dornach, Gemeinde Aschheim, Landkreis München Registergericht München, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Aug-11 17:36 UTC
Re: [Xen-devel] [PATCH] msi: Avoid uninitialized msi descriptors
On 11/08/2010 18:09, "Dante Cinco" <dantecinco@gmail.com> wrote:> I''m glad that somebody who is familiar with MSI has resolved this > problem although I haven''t applied and verified the proposed patch yet > on my system. I posted the Xen crash I experienced related to this > issue on xen-devel a couple of weeks ago (see link below) but wasn''t > familiar enough with the MSI/MSI-X code to propose a solution other > than to back out changelist 21778.Let me know when you confirm. I backported 21778 for 4.0.1, perhaps in too much haste. If this is a good fix then I should backport it as well. Thanks, Keir> http://old.nabble.com/Re:-(XEN)-RIP:-e008:-%3Cffff82c48015564b%3E---write_msi_ > msg%2B0x2b-0x150-p29301832.html > > Dante > > > -----Original Message----- > From: xen-devel-bounces@lists.xensource.com > [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Wei Wang2 > Sent: Wednesday, August 11, 2010 6:43 AM > To: xen-devel@lists.xensource.com > Subject: [Xen-devel] [PATCH] msi: Avoid uninitialized msi descriptors > > Hi, > When __pci_enable_msix() returns early, output parameter (struct msi_desc > **desc) will not be initialized. On my machine, a Broadcom BCM5709 > nic has both MSI and MSIX capability blocks and when guest tries to > enable msix interrupts but __pci_enable_msix() returns early for > encountering a msi block, the whole system will crash for fatal page > fault immediately. > > Thanks, > Wei > > Signed-off-by: Wei Wang <wei.wang2@amd.com> > -- > AMD GmbH, Germany > Operating System Research Center > > Legal Information: > Advanced Micro Devices GmbH > Karl-Hammerschmidt-Str. 34 > 85609 Dornach b. München > > Geschäftsführer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni > Sitz: Dornach, Gemeinde Aschheim, Landkreis München Registergericht > München, HRB Nr. 43632 > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei Wang2
2010-Aug-12 13:30 UTC
Re: [Xen-devel] [PATCH] msi: Avoid uninitialized msi descriptors
Keir, I would suggest to backport this patch. I saw the same issue on both Xen-4.0.1 RC6 and xen-unstable. Actually, some MSI-X capable devices will also offer MSI capabilities. For such device, MSI will be enabled first by OS. When guest driver (like Broadcom driver) detects multiple vcpus, it will disable MSI and enable MSI-X block for better cpu-interrupt affinity. In this case, the whole system crashes. But on UP system, only MSI block will be enabled, it might still work even without the fix. Thanks, Wei On Wednesday 11 August 2010 19:36:47 Keir Fraser wrote:> On 11/08/2010 18:09, "Dante Cinco" <dantecinco@gmail.com> wrote: > > I''m glad that somebody who is familiar with MSI has resolved this > > problem although I haven''t applied and verified the proposed patch yet > > on my system. I posted the Xen crash I experienced related to this > > issue on xen-devel a couple of weeks ago (see link below) but wasn''t > > familiar enough with the MSI/MSI-X code to propose a solution other > > than to back out changelist 21778. > > Let me know when you confirm. I backported 21778 for 4.0.1, perhaps in too > much haste. If this is a good fix then I should backport it as well. > > Thanks, > Keir > > > http://old.nabble.com/Re:-(XEN)-RIP:-e008:-%3Cffff82c48015564b%3E---write > >_msi_ msg%2B0x2b-0x150-p29301832.html > > > > Dante > > > > > > -----Original Message----- > > From: xen-devel-bounces@lists.xensource.com > > [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Wei Wang2 > > Sent: Wednesday, August 11, 2010 6:43 AM > > To: xen-devel@lists.xensource.com > > Subject: [Xen-devel] [PATCH] msi: Avoid uninitialized msi descriptors > > > > Hi, > > When __pci_enable_msix() returns early, output parameter (struct msi_desc > > **desc) will not be initialized. On my machine, a Broadcom BCM5709 > > nic has both MSI and MSIX capability blocks and when guest tries to > > enable msix interrupts but __pci_enable_msix() returns early for > > encountering a msi block, the whole system will crash for fatal page > > fault immediately. > > > > Thanks, > > Wei > > > > Signed-off-by: Wei Wang <wei.wang2@amd.com> > > -- > > AMD GmbH, Germany > > Operating System Research Center > > > > Legal Information: > > Advanced Micro Devices GmbH > > Karl-Hammerschmidt-Str. 34 > > 85609 Dornach b. München > > > > Geschäftsführer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni > > Sitz: Dornach, Gemeinde Aschheim, Landkreis München Registergericht > > München, HRB Nr. 43632 > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xensource.com > > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2010-Aug-13 12:07 UTC
Re: [Xen-devel] [PATCH] msi: Avoid uninitialized msi descriptors
>>> On 11.08.10 at 15:43, Wei Wang2 <wei.wang2@amd.com> wrote: > static int __pci_enable_msi(struct msi_info *msi, struct msi_desc **desc) > { > int status; > struct pci_dev *pdev; >+ struct msi_desc *old_desc; > > ASSERT(spin_is_locked(&pcidevs_lock)); > pdev = pci_get_pdev(msi->bus, msi->devfn); > if ( !pdev ) > return -ENODEV; > >- if ( find_msi_entry(pdev, msi->irq, PCI_CAP_ID_MSI) ) >+ old_desc = find_msi_entry(pdev, msi->irq, PCI_CAP_ID_MSI); >+ if ( old_desc ) > { > dprintk(XENLOG_WARNING, "irq %d has already mapped to MSI on " > "device %02x:%02x.%01x.\n", msi->irq, msi->bus, > PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn)); >+ *desc = old_desc;While I agree to this part, ...> return 0; > } > >- if ( find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX) ) >+ old_desc = find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX); >+ if ( old_desc ) > { > dprintk(XENLOG_WARNING, "MSI-X is already in use on " > "device %02x:%02x.%01x\n", msi->bus, > PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn)); >- return 0; >+ pci_disable_msi(old_desc);... I don''t think this one''s right: Admittedly I should have changed the return value from 0 to an actual error (e.g. -EBUSY) - I simply overlooked that after doing the copy-and-paste operation. Whether disabling and switching to the alternative mechanism is the right thing to do here I don''t know. But I''m pretty certain that old_desc may now be leaked, as msi_free_irq() isn''t being called on it and set_irq_msi() also doesn''t check whether irq_desc[].msi_desc is already non-NULL. Same thing (obviously) for the second part of the changes. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel