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

Signed-off-by: Wei Wang <wei.wang2@amd.com>

-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy


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

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@lists.xensource.com
http://lists.xensource.com/xen-devel

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@cl.cam.ac.uk> 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@lists.xensource.com
http://lists.xensource.com/xen-devel

> 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.
ok.

FWIW, my checker tree has a Xen coding style checker plus a writeup of some  
key points of the coding style as a document.  Would you take a patch to add 
these to the tree for future contributors to refer to?

Cheers,
Mark
>  -- Keir
>
> On 28/2/08 18:12, "Mark Williamson"
<mark.williamson@cl.cam.ac.uk> 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


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

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

On 28/2/08 19:10, "Mark Williamson"
<mark.williamson@cl.cam.ac.uk> wrote:
>> 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.
> 
> ok.
> 
> FWIW, my checker tree has a Xen coding style checker plus a writeup of some
> key points of the coding style as a document.  Would you take a patch to
add
> these to the tree for future contributors to refer to?
I''d certainly strongly consider it.

 -- Keir



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

Wei Wang just left for a much deserved vacation, although out of email
contact. So, he''ll certainly get to the refactoring when he gets back.
I''ll recommend that he fixup all of the AMD IOMMU code, referring to
Mark''s nice coding style checker + doc.
tom 
> -----Original Message-----
> From: xen-devel-bounces@lists.xensource.com 
> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of 
> Keir Fraser
> Sent: Thursday, February 28, 2008 3:22 PM
> To: Mark Williamson
> Cc: Wang2, Wei; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [amd iommu] [patch 2/2]Add APCI 
> tables supportfor AMD IOMMU
> 
> On 28/2/08 19:10, "Mark Williamson" 
> <mark.williamson@cl.cam.ac.uk> wrote:
> 
> >> 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.
> > 
> > ok.
> > 
> > FWIW, my checker tree has a Xen coding style checker plus a 
> writeup of 
> > some key points of the coding style as a document.  Would 
> you take a 
> > patch to add these to the tree for future contributors to refer to?
> 
> I''d certainly strongly consider it.
> 
>  -- Keir
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 
> 

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