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

[Mark Williamson <mark.williamson@xxxxxxxxxxxx>]

> 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@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



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

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