lkml.org 
[lkml]   [2006]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 2/3] x86-64: Calgary IOMMU - Calgary specific bits
Date
On Monday 20 March 2006 09:54, Muli Ben-Yehuda wrote:

> +/* register offsets inside the PHB space */


Call it host bridge here.

> +void* tce_table_kva[MAX_NUM_OF_PHBS * MAX_NUM_NODES];
> +unsigned int specified_table_size = TCE_TABLE_SIZE_UNSPECIFIED;

Why are these not static too?

> +static int translate_empty_slots __read_mostly = 0;
> +static int calgary_detected __read_mostly = 0;
> +
> +static void tce_cache_blast(struct iommu_table *tbl);
> +
> +/* enable this to stress test the chip's TCE cache */
> +#undef BLAST_TCE_CACHE_ON_UNMAP

This should be probably hooked into iommu=debug / CONFIG_IOMMU_DEBUG

> +static inline int valid_dma_direction(int dma_direction)
> +{
> + return ((dma_direction == DMA_BIDIRECTIONAL) ||
> + (dma_direction == DMA_TO_DEVICE) ||
> + (dma_direction == DMA_FROM_DEVICE));
> +}

Don't we already check that in the callers?


> +
> + offset = find_next_zero_string(tbl->it_map, tbl->it_hint,
> + tbl->it_size, npages);
> + if (offset == ~0UL) {
> + tce_cache_blast(tbl);
> + offset = find_next_zero_string(tbl->it_map, 0,
> + tbl->it_size, npages);
> + if (offset == ~0UL)
> + panic("Calgary: IOMMU full, fix the allocator.\n");

This should properly error out, not panic.


> +static dma_addr_t iommu_alloc(struct iommu_table *tbl, void *vaddr,
> + unsigned int npages, int direction)
> +{
> + unsigned long entry, flags;
> + dma_addr_t ret = bad_dma_address;
> +
> + spin_lock_irqsave(&tbl->it_lock, flags);
> +
> + entry = iommu_range_alloc(tbl, npages);
> +
> + ret = entry << PAGE_SHIFT; /* set the return dma address */
> +
> + if (unlikely(ret == bad_dma_address))
> + goto error;
> +
> + /* put the TCEs in the HW table */
> + tce_build(tbl, entry, npages, (unsigned long)vaddr & PAGE_MASK,
> + direction);
> + /* make sure HW sees the new TCEs */
> + mb();
> +
> + spin_unlock_irqrestore(&tbl->it_lock, flags);
> +
> + return ret;

Does this imply vaddr is always page aligned? Otherwise you would need
to add the offset into the page.


> + BUG_ON(!tbl);
> +
> + spin_lock_irqsave(&tbl->it_lock, flags);

That is a useless BUG_ON. More occurrences.

> + /* make sure updates are seen by hardware */
> + mb();

Doesn't make sense on x86-64.


> + dma_handle = iommu_alloc(tbl, vaddr, npages, direction);
> + if (dma_handle != bad_dma_address)
> + dma_handle |= (uaddr & ~PAGE_MASK);

Would be cleaner to do it in iommu_alloc



+
> +static inline unsigned long split_queue_offset(unsigned char num)
etc.

Looks like these all should be array lookups.
+
> + /* avoid the BIOS/VGA first 640KB-1MB region */
> + start = (640 * 1024);
> + npages = ((1024 - 640) * 1024) >> PAGE_SHIFT;
> + iommu_range_reserve(tbl, start, npages);

Why only those and not the other PCI holes? And why at all?


> +static int __init calgary_setup_tar(struct pci_dev *dev, void __iomem *bbar)
> +{
> + u64 val64;
> + u64 table_phys;
> + void __iomem *target;
> + int ret;
> + struct iommu_table *tbl;
> +
> + /* build TCE tables for each PHB */
> + ret = build_tce_table(dev, bbar);
> + if (ret)
> + goto done;

That's a useless goto

>
> +done:
> + return ret;
> +}
> +
> +static void __init calgary_free_tar(struct pci_dev *dev)
> +{
> + u64 val64;
> + struct iommu_table *tbl = dev->sysdata;
> + void __iomem *target;
> +
> + target = calgary_reg(tbl->bbar, tar_offset(dev->bus->number));
> + val64 = be64_to_cpu(readq(target));
> + val64 &= ~TAR_SW_BITS;
> + writeq(cpu_to_be64(val64), target);
> + readq(target); /* flush */
> +
> + kfree(tbl);
> + dev->sysdata = NULL;

We already use dev->sysdata for the NUMA topology information from ACPI.
I think that will conflict. You need to define a new structure for
all users.

> + /*
> + * FE0MB-8MB*OneBasedChassisNumber+1MB*(RioNodeId-ChassisBase)
> + * ChassisBase is always zero for x366/x260/x460
> + * RioNodeId is 2 for first Calgary, 3 for second Calgary
> + */
> + address = 0xfe000000 - (0x800000 * (1 + dev->bus->number / 15)) +
> + (0x100000) * (nodeid - 0);


Nasty magic numbers all over.

> + return address;
> +}
> +
> +static int __init calgary_init_one(struct pci_dev *dev)
> +{
> + u32 address;
> + void __iomem *bbar;
> + int ret;
> +
> + address = locate_register_space(dev);
> + /* map entire 1MB of Calgary config space */
> + bbar = ioremap(address, 1024 * 1024);

ioremap_nocache

Where exactly is the isolation enabled?

> + printk(KERN_INFO "PCI-DMA: detecting Calgary chipset...\n");

I want it to be quiet if Calgary is not there please.

> + for (bus = 0, calgary = 0;
> + bus < MAX_NUM_OF_PHBS * num_online_nodes() * 2;
> + bus++) {

Yuck another full scan. We're nearly over the threshold where whoever
adds more of these has to write a proper function to encapsulate and possibly
cache all that.

> + BUG_ON(bus >= MAX_PHB_BUS_NUM * MAX_NUM_NODES);
> + if (read_pci_config(bus, 0, 0, 0) != PCI_VENDOR_DEVICE_ID_CALGARY)
> + continue;

> + printk(KERN_ERR "PCI-DMA: Calgary init failed %x, "
> + "falling back to no_iommu\n", ret);

Shouldn't it rather fall back to swiotlb?

> + if (end_pfn > MAX_DMA32_PFN)
> + printk(KERN_ERR "WARNING more than 4GB of memory, "
> + "32bit PCI may malfunction.\n");
> + return ret;
> + }
> +
> + force_iommu = 1;

Why that?

> + dma_ops = &calgary_dma_ops;
> +
> + return ret;
> +}
> +
> +void __init calgary_parse_options(char *p)
> +{
> + while (*p) {
> + if (!strncmp(p, "64k", 3))
> + specified_table_size = TCE_TABLE_SIZE_64K;
> + else if (!strncmp(p, "128k", 4))
> + specified_table_size = TCE_TABLE_SIZE_128K;
> + else if (!strncmp(p, "256k", 4))
> + specified_table_size = TCE_TABLE_SIZE_256K;
> + else if (!strncmp(p, "512k", 4))
> + specified_table_size = TCE_TABLE_SIZE_512K;
> + else if (!strncmp(p, "1M", 2))
> + specified_table_size = TCE_TABLE_SIZE_1M;
> + else if (!strncmp(p, "2M", 2))
> + specified_table_size = TCE_TABLE_SIZE_2M;
> + else if (!strncmp(p, "4M", 2))
> + specified_table_size = TCE_TABLE_SIZE_4M;
> + else if (!strncmp(p, "8M", 2))
> + specified_table_size = TCE_TABLE_SIZE_8M;

You forgot to document all these in Documentation/x86_64


> + tce_free(tbl, 0, tbl->it_size);
> + mb(); /* flush TCE table update */

I don't think it will do anything on x86-64


> + p = __alloc_bootmem_low(size, size, 0);
> + if (!p)
> + printk(KERN_ERR "Calgary: cannot allocate TCE table of "
> + "size 0x%x\n", size);

It won't - it will panic. I have a patch to add alloc_bootmem_nopanic queued
though. You should use that later.

I would like to see a printk and some comments about the full isolation
because it's a big change. How does it interact with the X server for once?

The patch is not self contained because it doesn't have Kconfig?

-Andi
-
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/

\
 
 \ /
  Last update: 2006-03-23 18:23    [W:1.787 / U:0.632 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site