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

[Mark Williamson <mark.williamson@xxxxxxxxxxxx>]

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) &
-            PCI_CAP_MMIO_BAR_LOW_MASK;
-    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 ;-)

Cheers,
Mark

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel