To be honest I didn't look all that closely at these patches, beyond
observing that they largely modify AMD-IOMMU-specific files. It would be
nice to fix up the coding style -- I tend to do this myself only when I
already have the offending files loaded into an Emacs buffer for some other
reason.
-- Keir
On 28/2/08 18:12, "Mark Williamson" <mark.williamson@xxxxxxxxxxxx> wrote:
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home