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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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().)

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



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