| Message ID | 1457435357-34073-2-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2016-03-08 at 19:09 +0800, Quan Xu wrote: > Doing what we do serves as a fix for a bug found in AMD IOMMU > initialization. > This first line is rather useless, if not worse. :-) I don't know (provided a new version is not necessary, and provided maintainers agree with me :-)) whether you need to repost or it can be removed when code is committed. > Signed-off-by: Quan Xu <quan.xu@intel.com> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> > get_maintainer.pl gives me only Suravee, as Aravind stepped down a few days ago, so he shouldn't be bothered (and, in fact, I'm moving him from Cc to Bcc). > CC: Dario Faggioli <dario.faggioli@citrix.com> > CC: Jan Beulich <jbeulich@suse.com> > (BTW, it's of course fine to include me and Jan, despite what get_maintainer.pl's output, as we've been involved in previous rounds of review.) All that being said: Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com> Thanks and Regards, Dario
On March 08, 2016 8:13pm, <dario.faggioli@citrix.com> wrote: > On Tue, 2016-03-08 at 19:09 +0800, Quan Xu wrote: > > Doing what we do serves as a fix for a bug found in AMD IOMMU > > initialization. > > > This first line is rather useless, if not worse. :-) > I will remove it in next v2. :) > I don't know (provided a new version is not necessary, and provided maintainers > agree with me :-)) whether you need to repost or it can be removed when code > is committed. > I think I'd better send out v2. :) > > Signed-off-by: Quan Xu <quan.xu@intel.com> > > CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > > CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> > > > get_maintainer.pl gives me only Suravee, as Aravind stepped down a few days > ago, so he shouldn't be bothered (and, in fact, I'm moving him from Cc to Bcc). > Got it, thanks for your advice. > > CC: Dario Faggioli <dario.faggioli@citrix.com> > > CC: Jan Beulich <jbeulich@suse.com> > > > (BTW, it's of course fine to include me and Jan, despite what get_maintainer.pl's > output, as we've been involved in previous rounds of review.) > > All that being said: > > Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com> > Dario, thanks. Quan
>>> On 08.03.16 at 12:09, <quan.xu@intel.com> wrote: > Doing what we do serves as a fix for a bug found in AMD IOMMU initialization. > > The current code is using spin_lock{_irqsave(), _irqrestore()} to > protect pci_get_dev() in the set_iommu_interrupt_handler(). However, > this can only be called during AMD IOMMU initialization, with interrupt > enabled, so at least it is not necessary to disable interrupts, or > save/restore interrupt flag. On top of what Dario said: This description isn't very accurate either: If the code in question runs with interrupts enabled (which I'm not sure it does; would take me following back up the call chain to see whether this happens before or after interrupts get enabled for the first time), then it may very well be necessary to disable interrupts for a particular purpose - from an abstract pov. That's not the case here, but the description of such a change shouldn't make incorrect claims or statements. Jan
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c index d90a2d2..a400497 100644 --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -778,7 +778,6 @@ static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu) { int irq, ret; hw_irq_controller *handler; - unsigned long flags; u16 control; irq = create_irq(NUMA_NO_NODE); @@ -788,10 +787,10 @@ static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu) return 0; } - spin_lock_irqsave(&pcidevs_lock, flags); + spin_lock(&pcidevs_lock); iommu->msi.dev = pci_get_pdev(iommu->seg, PCI_BUS(iommu->bdf), PCI_DEVFN2(iommu->bdf)); - spin_unlock_irqrestore(&pcidevs_lock, flags); + spin_unlock(&pcidevs_lock); if ( !iommu->msi.dev ) { AMD_IOMMU_DEBUG("IOMMU: no pdev for %04x:%02x:%02x.%u\n",
Doing what we do serves as a fix for a bug found in AMD IOMMU initialization. The current code is using spin_lock{_irqsave(), _irqrestore()} to protect pci_get_dev() in the set_iommu_interrupt_handler(). However, this can only be called during AMD IOMMU initialization, with interrupt enabled, so at least it is not necessary to disable interrupts, or save/restore interrupt flag. In order to fix this, we can use just plain spin{_lock(),_unlock()}, instead of spin_lock{_irqsave(),_irqrestore()}. Signed-off-by: Quan Xu <quan.xu@intel.com> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> CC: Dario Faggioli <dario.faggioli@citrix.com> CC: Jan Beulich <jbeulich@suse.com> --- xen/drivers/passthrough/amd/iommu_init.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)