Re: [Xen-devel] [PATCH] AMD IOMMU: fix debug console IOMMU intremap output

["Woods, Brian" <Brian.Woods@xxxxxxx>]

On Wed, Dec 05, 2018 at 02:00:43AM -0700, Jan Beulich wrote:
> >>> On 04.12.18 at 22:47, <Brian.Woods@xxxxxxx> wrote:
> > --- a/xen/drivers/passthrough/amd/iommu_intr.c
> > +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> > @@ -665,6 +665,24 @@ int __init amd_setup_hpet_msi(struct msi_desc 
> > *msi_desc)
> >      return rc;
> >  }
> >  
> > +
> > +static bool intremap_table_empty(const u32 *table)
> 
> uint32_t here please and ...
> 
> > +{
> > +    u32 count;
> 
> ... since a fixed width type isn't needed here in the first place,
> unsigned int here. (This is notwithstanding the fact that I
> assume you've merely cloned dump_intremap_table().)

Gah, I did copy/clone dump_intremap_table, if I keep the same code
strucutre I'll use you ahd Paul's suggestions.

> > +    if ( !table )
> > +        return true;
> > +
> > +    for ( count = 0; count < INTREMAP_ENTRIES; count++ )
> > +    {
> > +        if ( table[count] )
> > +            return false;
> > +    }
> > +    return true;
> 
> Blank line above here please.
> 
> > +}
> > +
> > +
> > +
> >  static void dump_intremap_table(const u32 *table)
> 
> No multiple consecutive blank lines in general please (there may
> be extremely limited cases where exceptions are possible).
> 
> > @@ -687,13 +705,17 @@ static int dump_intremap_mapping(u16 seg, struct 
> > ivrs_mappings *ivrs_mapping)
> >      if ( !ivrs_mapping )
> >          return 0;
> >  
> > -    printk("  %04x:%02x:%02x:%u:\n", seg,
> > -           PCI_BUS(ivrs_mapping->dte_requestor_id),
> > -           PCI_SLOT(ivrs_mapping->dte_requestor_id),
> > -           PCI_FUNC(ivrs_mapping->dte_requestor_id));
> > -
> >      spin_lock_irqsave(&(ivrs_mapping->intremap_lock), flags);
> > -    dump_intremap_table(ivrs_mapping->intremap_table);
> > +
> > +    if ( !intremap_table_empty(ivrs_mapping->intremap_table) ) {
> 
> Brace on its own line please.
> 
> > +        printk("  %04x:%02x:%02x:%u:\n", seg,
> > +               PCI_BUS(ivrs_mapping->dte_requestor_id),
> > +               PCI_SLOT(ivrs_mapping->dte_requestor_id),
> > +               PCI_FUNC(ivrs_mapping->dte_requestor_id));
> > +
> > +        dump_intremap_table(ivrs_mapping->intremap_table);
> > +    }
> 
> dump_intremap_table() already skips empty entries, so aiui it
> is just the headline above you omit. How much of a savings is
> this really?
> 
> Furthermore, instead of adding a second function with a second
> loop, did you consider moving the logging of the headline into
> dump_intremap_table(), issuing the line the first time you hit a
> non-empty entry?
> 
> Jan
> 

I did think about doing that also, but ended up going with this route.
What I'll do is move printing the headline intp dump_intremap_table and
see what you guys think (since it's such a small patch, it'll be easier
to do that than talk about it).


-- 
Brian Woods

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel