From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ingo Molnar Subject: Re: [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking Date: Tue, 19 May 2009 11:32:27 +0200 Message-ID: <20090519093227.GF31404@elte.hu> References: <1241676996-27406-1-git-send-email-weidong.han@intel.com> <1241676996-27406-2-git-send-email-weidong.han@intel.com> <1241720381.27006.8582.camel@localhost.localdomain> <715D42877B251141A38726ABF5CABF2C01A6DD4900@pdsmsx503.ccr.corp.intel.com> <20090511132027.GC32693@elte.hu> <715D42877B251141A38726ABF5CABF2C054568FA16@pdsmsx503.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "Siddha, Suresh B" , "'dwmw2@infradead.org'" , "'linux-kernel@vger.kernel.org'" , "'iommu@lists.linux-foundation.org'" , "'kvm@vger.kernel.org'" To: "Han, Weidong" Return-path: Content-Disposition: inline In-Reply-To: <715D42877B251141A38726ABF5CABF2C054568FA16@pdsmsx503.ccr.corp.intel.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org * Han, Weidong wrote: > Ingo Molnar wrote: > > * Han, Weidong wrote: > > > >> Siddha, Suresh B wrote: > >>> On Wed, 2009-05-06 at 23:16 -0700, Han, Weidong wrote: > >>>> @@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct > >>>> acpi_dmar_header *header, " 0x%Lx\n", > >>>> scope->enumeration_id, drhd->address); > >>>> > >>>> + bus = pci_find_bus(drhd->segment, scope->bus); > >>>> + path = (struct acpi_dmar_pci_path *)(scope + 1); + count = > >>>> (scope->length - + sizeof(struct acpi_dmar_device_scope)) > >>>> + / sizeof(struct acpi_dmar_pci_path); > >>>> + > >>>> + while (count) { > >>>> + if (pdev) > >>>> + pci_dev_put(pdev); > >>>> + > >>>> + if (!bus) > >>>> + break; > >>>> + > >>>> + pdev = pci_get_slot(bus, > >>>> + PCI_DEVFN(path->dev, path->fn)); > >>>> + if (!pdev) > >>>> + break; > >>> > >>> ir_parse_ioapic_scope() happens very early in the boot. So, I > >>> don't think we can do the pci related discovery here. > >>> > >> > >> Thanks for your pointing it out. It should enable the source-id > >> checking for io-apic's after the pci subsystem is up. I will > >> change it. > > > > Note, there's ways to do early PCI quirks too, check > > arch/x86/kernel/early-quirks.c. It's done by reading the PCI > > configuration space directly via a careful early-capable subset of > > the PCI config space APIs. > > > > But it's a method of last resort. > > > > Thanks for your reminder. It can use direct PCI access here as follows. It's easy and clean. I think it's better than adding the source-id checking for io-apic's after the pci subsystem is up. I will send out updated patches after some tests. > > @@ -634,6 +695,24 @@ static int ir_parse_ioapic_scope(struct acpi_dmar_header *header, > " 0x%Lx\n", scope->enumeration_id, > drhd->address); > > + bus = scope->bus; > + path = (struct acpi_dmar_pci_path *)(scope + 1); > + count = (scope->length - > + sizeof(struct acpi_dmar_device_scope)) > + / sizeof(struct acpi_dmar_pci_path); > + > + while (--count > 0) { > + /* Access PCI directly due to the PCI > + * subsystem isn't initialized yet. > + */ > + bus = read_pci_config_byte(bus, path->dev, > + path->fn, PCI_SECONDARY_BUS); > + path++; > + } > + > + ir_ioapic[ir_ioapic_num].bus = bus; > + ir_ioapic[ir_ioapic_num].devfn = > + PCI_DEVFN(path->dev, path->fn); looks good IMO, beyond the obligatory comment-style nitpick [*] :-) Also, the function above seems to be way too large - please split it into a couple of natural helper functions. Thanks, Ingo [*] Please use the customary comment style: /* * Comment ..... * ...... goes here: */ specified in Documentation/CodingStyle.