Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754765AbYJBPcN (ORCPT ); Thu, 2 Oct 2008 11:32:13 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753833AbYJBPb4 (ORCPT ); Thu, 2 Oct 2008 11:31:56 -0400 Received: from g1t0027.austin.hp.com ([15.216.28.34]:24056 "EHLO g1t0027.austin.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754566AbYJBPby (ORCPT ); Thu, 2 Oct 2008 11:31:54 -0400 From: Bjorn Helgaas To: Fenghua Yu Subject: Re: [PATCH 1/2]Add Variable Page Size and IA64 Support in Intel IOMMU: Generic Part Date: Thu, 2 Oct 2008 09:31:47 -0600 User-Agent: KMail/1.9.9 Cc: "Luck, Tony" , Jesse Barnes , David Woodhouse , Ingo Molnar , Avi Kivity , Stephen Rothwell , Andrew Morton , LKML , linux-ia64@vger.kernel.org References: <617E1C2C70743745A92448908E030B2A02AD5AD7@scsmsx411.amr.corp.intel.com> <20081001165725.GA21559@linux-os.sc.intel.com> In-Reply-To: <20081001165725.GA21559@linux-os.sc.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200810020931.48952.bjorn.helgaas@hp.com> X-Brightmail-Tracker: AAAAAQAAAAI= X-Whitelist: TRUE Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6475 Lines: 185 On Wednesday 01 October 2008 10:57:25 am Fenghua Yu wrote: > The current Intel IOMMU code assumes that both host page size and Intel IOMMU page size are 4K. The first patch supports variable page size. This provides support for IA64 which has multiple page sizes. > > This patch also adds some other code hooks for IA64 platform including DMAR_OPERATION_TIMEOUT definition, . Can you split this patch up? It contains several logically separate changes: - casting things to unsigned long long - adding stuff under #ifdef CONFIG_IA64 - page size changes - whitespace changes > @@ -510,7 +514,7 @@ int alloc_iommu(struct dmar_drhd_unit *drhd) > > iommu->seq_id = iommu_allocated++; > > - iommu->reg = ioremap(drhd->reg_base_addr, PAGE_SIZE_4K); > + iommu->reg = ioremap(drhd->reg_base_addr, IOMMU_PAGE_SIZE); > if (!iommu->reg) { > printk(KERN_ERR "IOMMU: can't map the region\n"); This printk should include a clue, like the IOMMU ID and/or address we tried to map. > +#ifdef CONFIG_IA64 > +static inline void *ia64_get_zeroed_page(gfp_t gfp_mask) > +{ > + struct page *page; > + > + page = alloc_pages(gfp_mask | __GFP_ZERO, 0); > + if (page) > + return (void *)page_address(page); > + return 0; > +} > +#endif > > static inline void *alloc_pgtable_page(void) > { > @@ -141,7 +155,11 @@ static inline void *alloc_pgtable_page(void) > /* trying to avoid low memory issues */ > flags = current->flags & PF_MEMALLOC; > current->flags |= PF_MEMALLOC; > +#ifdef CONFIG_IA64 > + vaddr = ia64_get_zeroed_page(GFP_ATOMIC); > +#else > vaddr = (void *)get_zeroed_page(GFP_ATOMIC); > +#endif Why does ia64 need a special case here? > @@ -655,7 +673,8 @@ static int __iommu_flush_iotlb(struct intel_iommu *iommu, u16 did, > printk(KERN_ERR"IOMMU: flush IOTLB failed\n"); > if (DMA_TLB_IAIG(val) != DMA_TLB_IIRG(type)) > pr_debug("IOMMU: tlb flush request %Lx, actual %Lx\n", > - DMA_TLB_IIRG(type), DMA_TLB_IAIG(val)); > + (unsigned long long)DMA_TLB_IIRG(type), > + (unsigned long long) DMA_TLB_IAIG(val)); These printks should include an IOMMU ID also (I assume a system can contain multiple IOMMUs). > @@ -1490,9 +1513,9 @@ static int iommu_prepare_identity_map(struct pci_dev *pdev, u64 start, u64 end) > return -ENOMEM; > > /* The address might not be aligned */ > - base = start & PAGE_MASK_4K; > + base = start & PAGE_MASK; > size = end - base; > - size = PAGE_ALIGN_4K(size); > + size = PAGE_ALIGN(size); > if (!reserve_iova(&domain->iovad, IOVA_PFN(base), > IOVA_PFN(base + size) - 1)) { > printk(KERN_ERR "IOMMU: reserve iova failed\n"); This should probably be a "dev_err(&pdev->dev," and include the IOMMU ID. > @@ -1855,27 +1879,23 @@ intel_map_single(struct device *hwdev, phys_addr_t paddr, size_t size, int dir) > * is not a big problem > */ > ret = domain_page_mapping(domain, start_paddr, > - ((u64)paddr) & PAGE_MASK_4K, size, prot); > + ((u64)paddr) & PAGE_MASK, size, prot); > if (ret) > goto error; > > - pr_debug("Device %s request: %lx@%llx mapping: %lx@%llx, dir %d\n", > - pci_name(pdev), size, (u64)paddr, > - size, (u64)start_paddr, dir); > - > /* it's a non-present to present mapping */ > ret = iommu_flush_iotlb_psi(domain->iommu, domain->id, > - start_paddr, size >> PAGE_SHIFT_4K, 1); > + start_paddr, size >> IOMMU_PAGE_SHIFT, 1); > if (ret) > iommu_flush_write_buffer(domain->iommu); > > - return (start_paddr + ((u64)paddr & (~PAGE_MASK_4K))); > + return start_paddr + ((u64)paddr & (~PAGE_MASK)); > > error: > if (iova) > __free_iova(&domain->iovad, iova); > printk(KERN_ERR"Device %s request: %lx@%llx dir %d --- failed\n", > - pci_name(pdev), size, (u64)paddr, dir); > + pci_name(pdev), size, (unsigned long long)paddr, dir); Use dev_err() here and include IOMMU ID. > @@ -1953,11 +1974,11 @@ static void intel_unmap_single(struct device *dev, dma_addr_t dev_addr, > if (!iova) > return; > > - start_addr = iova->pfn_lo << PAGE_SHIFT_4K; > + start_addr = iova->pfn_lo << PAGE_SHIFT; > size = aligned_size((u64)dev_addr, size); > > pr_debug("Device %s unmapping: %lx@%llx\n", > - pci_name(pdev), size, (u64)start_addr); > + pci_name(pdev), size, (unsigned long long)start_addr); Use dev_dbg() here. > diff --git a/include/linux/dma_remapping.h b/include/linux/dma_remapping.h > index bff5c65..218598d 100644 > --- a/include/linux/dma_remapping.h > +++ b/include/linux/dma_remapping.h > @@ -2,15 +2,15 @@ > #define _DMA_REMAPPING_H > > /* > - * We need a fixed PAGE_SIZE of 4K irrespective of > - * arch PAGE_SIZE for IOMMU page tables. > + * VT-d hardware uses 4K page size regardless host page size. > */ > -#define PAGE_SHIFT_4K (12) > -#define PAGE_SIZE_4K (1UL << PAGE_SHIFT_4K) > -#define PAGE_MASK_4K (((u64)-1) << PAGE_SHIFT_4K) > -#define PAGE_ALIGN_4K(addr) (((addr) + PAGE_SIZE_4K - 1) & PAGE_MASK_4K) > +#define IOMMU_PAGE_SHIFT (12) > +#define IOMMU_PAGE_SIZE (1UL << IOMMU_PAGE_SHIFT) > +#define IOMMU_PAGE_MASK (((u64)-1) << IOMMU_PAGE_SHIFT) > +#define IOMMU_PAGE_ALIGN(addr) \ > + (((addr) + IOMMU_PAGE_SIZE - 1) & IOMMU_PAGE_MASK) > > -#define IOVA_PFN(addr) ((addr) >> PAGE_SHIFT_4K) > +#define IOVA_PFN(addr) ((addr) >> PAGE_SHIFT) These are pretty generic names (IOMMU_PAGE_SHIFT, IOVA_PFN, etc), but the definitions seem to be specific to VT-d. I can't tell if this file is supposed to be sort of generic, or if it's Intel-specific. > diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h > index e7b196b..d84612a 100644 > --- a/include/linux/intel-iommu.h > +++ b/include/linux/intel-iommu.h > @@ -67,6 +67,13 @@ > hi = readl(dmar + reg + 4); \ > (((u64) hi) << 32) + lo; }) > */ > +#ifdef CONFIG_IA64 > +#define dmar_readq readq > +static inline void dmar_writeq(void __iomem *addr, u64 val) > +{ > + writeq(val, addr); > +} > +#else > static inline u64 dmar_readq(void __iomem *addr) > { > u32 lo, hi; > @@ -80,6 +87,7 @@ static inline void dmar_writeq(void __iomem *addr, u64 val) > writel((u32)val, addr); > writel((u32)(val >> 32), addr + 4); > } > +#endif What's this all about? Why do we need #ifdef CONFIG_IA64 here? Doesn't x86 provide its own readq/writeq implementation? Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/