WARNING - OLD ARCHIVES

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/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Mark Williamson <mark.williamson@xxxxxxxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [amd iommu] [patch 2/2]Add APCI tables support for AMD IOMMU
From: Keir Fraser <keir.fraser@xxxxxxxxxxxxx>
Date: Thu, 28 Feb 2008 18:42:13 +0000
Cc: Wei Wang2 <wei.wang2@xxxxxxx>
Delivery-date: Thu, 28 Feb 2008 10:43:25 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <200802281812.54240.mark.williamson@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>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Thread-index: Ach6OaIE4FxvAOYsEdygygAX8io7RQ==
Thread-topic: [Xen-devel] [amd iommu] [patch 2/2]Add APCI tables support for AMD IOMMU
User-agent: Microsoft-Entourage/11.3.6.070618
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