Mailing List Archive

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