Re: [git pull] AMD-IOMMU (AMD-Vi) updates for 2.6.32

From: Ingo Molnar
Date: Fri Sep 04 2009 - 09:06:58 EST


* Joerg Roedel <joerg.roedel@xxxxxxx> wrote:

> Hi Ingo,
>
> The following changes since commit 37d0892c5a94e208cf863e3b7bac014edee4346d:
> Ian Kent (1):
> autofs4 - fix missed case when changing to use struct path
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/joro/linux-2.6-iommu.git amd-iommu/2.6.32
>
> Joerg Roedel (37):
> x86/amd-iommu: Dump fault entry on DTE error
> x86/amd-iommu: Dump illegal command on ILLEGAL_COMMAND_ERROR
> x86/amd-iommu: Introduce function for iommu-local domain flush
> x86/amd-iommu: Remove some merge helper code
> x86/amd-iommu: replace "AMD IOMMU" by "AMD-Vi"
> x86/amd-iommu: Remove redundant 'IOMMU' string
> x86/amd-iommu: fix broken check in amd_iommu_flush_all_devices
> x86/amd-iommu: Add function to flush all DTEs on one IOMMU
> x86/amd-iommu: Add reset function for command buffers
> x86/amd-iommu: Reset command buffer on ILLEGAL_COMMAND_ERROR
> x86/amd-iommu: Panic if IOMMU command buffer reset fails
> x86/amd-iommu: Reset command buffer if wait loop fails
> x86/amd-iommu: Make fetch_pte aware of dynamic mapping levels
> x86/amd-iommu: Use fetch_pte in iommu_unmap_page
> x86/amd-iommu: Use fetch_pte in amd_iommu_iova_to_phys
> x86/amd-iommu: Add a gneric version of amd_iommu_flush_all_devices
> x86/amd-iommu: Introduce set_dte_entry function
> x86/amd-iommu: Flush domains if address space size was increased
> x86/amd-iommu: Introduce increase_address_space function
> x86/amd-iommu: Change alloc_pte to support 64 bit address space
> x86/amd-iommu: Remove last usages of IOMMU_PTE_L0_INDEX
> x86/amd-iommu: Remove bus_addr check in iommu_map_page
> x86/amd-iommu: Use 2-level page tables for dma_ops domains
> x86/amd-iommu: Remove old page table handling macros
> x86/amd-iommu: Support higher level PTEs in iommu_page_unmap
> x86/amd-iommu: Change iommu_map_page to support multiple page sizes
> x86/dma: Mark iommu_pass_through as __read_mostly
> x86/amd-iommu: Add core functions for pd allocation/freeing
> x86/amd-iommu: Add passthrough mode initialization functions
> x86/amd-iommu: Fix device table write order
> x86/amd-iommu: Align locking between attach_device and detach_device
> x86/amd-iommu: Make sure a device is assigned in passthrough mode
> x86/amd-iommu: Don't detach device from pt domain on driver unbind
> x86/amd-iommu: Initialize passthrough mode when requested
> Merge branches 'gart/fixes', 'amd-iommu/fixes+cleanups' and 'amd-iommu/fault-handling' into amd-iommu/2.6.32
> Merge branch 'amd-iommu/passthrough' into amd-iommu/2.6.32
> Merge branch 'amd-iommu/pagetable' into amd-iommu/2.6.32
>
> Pavel Vasilyev (1):
> x86/gart: Do not select AGP for GART_IOMMU
>
> arch/x86/Kconfig | 1 -
> arch/x86/include/asm/amd_iommu.h | 1 +
> arch/x86/include/asm/amd_iommu_types.h | 50 ++--
> arch/x86/kernel/amd_iommu.c | 489 +++++++++++++++++++++++---------
> arch/x86/kernel/amd_iommu_init.c | 42 ++-
> arch/x86/kernel/pci-dma.c | 9 +-
> 6 files changed, 429 insertions(+), 163 deletions(-)
>
> These changes include updates and improvements in the page table code,
> in fault handling and they implement iommu=pt mode for AMD-Vi too.
> Last but not least there are also some minor cleanups and fixes for
> AMD-Vi and GART included in this pull-request. All these changes are
> compile-, boot- and load-tested.
>
> I had to do three merge commits for this pull request because the
> amd-iommu/passthrough and amd-iommu/pagetable branches touch the same
> code in some parts and the octopus merge of these branches failed. I had
> to merge these two branches seperatly.
> Please consider to pull.

Pulled, thanks a lot Joerg!

A few (small) comments:

+static void dump_dte_entry(u16 devid)
+{
+ int i;
+
+ for (i = 0; i < 8; ++i)
+ pr_err("AMD-Vi: DTE[%d]: %08x\n", i,
+ amd_iommu_dev_table[devid].data[i]);

that 8 is not very obvious - it deserves a comment. (we allocate at
least one page to amd_iommu_dev_table[] so it's safe - but where the
8 comes from is not very clear.)

Also, we tend to write 'i++' not '++i' in loops. (there's other
examples of this too in the iommu files)

This log printing pattern:

printk(KERN_ERR "AMD-Vi: Event logged [");

switch (type) {
case EVENT_TYPE_ILL_DEV:
printk("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
"address=0x%016llx flags=0x%04x]\n",

is now broken (produces an unexpected newline) due to:

5fd29d6: printk: clean up handling of log-levels and newlines

you probably want to convert all printk's to pr_*() calls, and use
pr_cont() in the above place.

Similar comments hold for dump_command() as well.

+++ b/arch/x86/include/asm/amd_iommu_types.h
@@ -457,4 +457,7 @@ static inline void amd_iommu_stats_init(void) {
}

#endif /* CONFIG_AMD_IOMMU_STATS */

+/* some function prototypes */
+extern void amd_iommu_reset_cmd_buffer(struct amd_iommu *iommu);
+

function prototypes belong into amd_iommu.h, not amd_iommu_types.h.

there's also a lot of function prototypes declared in the .c file:

+++ b/arch/x86/kernel/amd_iommu.c
@@ -61,6 +61,7 @@ static u64* alloc_pte(struct protection_domain
*dom,
static void dma_ops_reserve_addresses(struct dma_ops_domain *dom,
unsigned long start_page,
unsigned int pages);
+static void reset_iommu_command_buffer(struct amd_iommu *iommu);

These can generally be avoided via cleaner ordering: first define
simpler functions then more complex ones (which rely on simpler
functions).

+ if (unlikely(i == EXIT_LOOP_COUNT)) {
+ spin_unlock(&iommu->lock);
+ reset_iommu_command_buffer(iommu);
+ spin_lock(&iommu->lock);
+ }

What did iommu->lock protect it here, why do we drop it, and why is
it necessary to take it again? This pattern shows that either
there's too much or too little locking.

+ /* increase reference counter */
+ domain->dev_cnt += 1;

use ++?

+ * If a device is not yet associated with a domain, this function does
+ * assigns it visible for the hardware

typo.

+static void update_domain(struct protection_domain *domain)
+{
+ if (!domain->updated)
+ return;
+
+ update_device_table(domain);
+ flush_devices_by_domain(domain);
+ iommu_flush_domain(domain->id);
+
+ domain->updated = false;
+}

the domain->updated is a bit weird naming. Perhaps
domain->update_needed?

+ iommu_unmap_page(domain, iova, PM_MAP_4k);

small nit: mixed-case letters - PM_MAP_4K is better i guess.

+#define PM_ALIGNED(lvl, addr) ((PM_MAP_MASK(lvl) & (addr)) == (addr))

macro with two uses of its parameter - this is side-effect-unsafe.
Safer would be to turn this into an inline function. That would also
do proper type checking.

+ /* allocate passthroug domain */

typo.

+/*****************************************************************************
+ *
+ * The next functions do a basic initialization of IOMMU for pass through
+ * mode
+ *
+ * In passthrough mode the IOMMU is initialized and enabled but not used for
+ * DMA-API translation.
+ *
+
*****************************************************************************/

please use the customary (multi-line) comment style:

/*
* Comment .....
* ...... goes here.
*/

specified in Documentation/CodingStyle.

+#define PD_PASSTHROUGH_MASK (1UL << 2) /* domain has no page
+ translation */

ditto.

+ domain->dev_cnt += 1;

Could use '++' here too.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/