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