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

[Keir Fraser <keir.fraser@xxxxxxxxxxxxx>]

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