This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


Re: [Xen-devel] [amd iommu] [patch 2/2]Add APCI tables support for AMD I

To: xen-devel@xxxxxxxxxxxxxxxxxxx
Subject: Re: [Xen-devel] [amd iommu] [patch 2/2]Add APCI tables support for AMD IOMMU
From: Mark Williamson <mark.williamson@xxxxxxxxxxxx>
Date: Thu, 28 Feb 2008 18:12:53 +0000
Cc: Wei Wang2 <wei.wang2@xxxxxxx>, Keir Fraser <keir.fraser@xxxxxxxxxxxx>
Delivery-date: Thu, 28 Feb 2008 10:13:21 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <1204203102.8542.56.camel@xxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1204203102.8542.56.camel@xxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: KMail/1.9.6 (enterprise 0.20070907.709405)
Hi there,

Some picky comments inline, most of them are formatting but one semantic 
question too.  I've removed most of the diff lines but left a little context 
before each comment.

-            cap_ptr + PCI_CAP_MMIO_BAR_LOW_OFFSET) &
-    iommu->mmio_base_phys = (unsigned long)mmio_bar;
-    if ( (mmio_bar == 0) || ( (mmio_bar & 0x3FFF) != 0 ) ) {
+            cap_ptr + PCI_CAP_MMIO_BAR_LOW_OFFSET); 

Tiny trailing whitespace here.

+static void __init register_iommu_exclusion_range(struct amd_iommu *iommu)
+    u64 addr_lo, addr_hi;
+    u32 entry;
+    addr_lo = iommu->exclusion_limit & DMA_32BIT_MASK;
+    addr_hi = iommu->exclusion_limit >> 32;

iommu->exclusion_limit is an unsigned long, so on x86_32 you are bit shifting 
exclusion_limit by a value equal to its width.  I would guess that gcc 
probably does the right thing in this case, but I don't know if this is 
strictly allowed by the C standard?

A question leading on from that, I guess, is whether exclusion_limit ought to 
be a u64 like addr_lo and addr_hi?

+    spin_lock_irqsave(&hd->mapping_lock, flags);
+    for ( i = 0; i < npages; ++i )
+    {
+        pte = get_pte_from_page_tables(hd->root_table,
+           hd->paging_mode, phys_addr>>PAGE_SHIFT);
+        if ( pte == 0 )

Most of the Xen codebase uses NULL rather than 0 to indicate a null pointer.  
Your use seems to be consistent with the rest of this file, though, so maybe 
that's acceptable here...

+        /* allocate 'ivrs mappings' table */
+        /* note: the table has entries to accomodate all IOMMUs */
+        last_bus = 0;
+        for_each_amd_iommu (iommu)
+           if (iommu->last_downstream_bus > last_bus)

Needs spaces between the brackets and expression ;-)

 int amd_iommu_assign_device(struct domain *d, u8 bus, u8 devfn)
+    int bdf = (bus << 8) | devfn;
+    int req_id;
+    req_id = ivrs_mappings[bdf].dte_requestor_id;
+    if (ivrs_mappings[req_id].unity_map_enable)

Bracket spacing again ;-)


Push Me Pull You - Distributed SCM tool (http://www.cl.cam.ac.uk/~maw48/pmpu/)

Xen-devel mailing list