* [PATCH] AMD IOMMU: fix debug console IOMMU intremap output
@ 2018-12-04 21:47 Woods, Brian
2018-12-05 8:50 ` Paul Durrant
2018-12-05 9:00 ` Jan Beulich
0 siblings, 2 replies; 4+ messages in thread
From: Woods, Brian @ 2018-12-04 21:47 UTC (permalink / raw)
To: xen-devel@lists.xen.org
Cc: Paul Durrant, Woods, Brian, Suthikulpanit, Suravee,
Roger Pau Monne
When using the Xen debug console and printing the IOMMU intremap tables,
it prints everything in the IVRS range regardless if it has an intr
remap or not. Add some logic to cause an entry to only be printed if
the intr remap table isn't empty.
Signed-off-by: Brian Woods <brian.woods@amd.com>
---
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>
xen/drivers/passthrough/amd/iommu_intr.c | 34 ++++++++++++++++++++++++++------
1 file changed, 28 insertions(+), 6 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c
index dad2d1e5ab..e86300b57f 100644
--- 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)
+{
+ u32 count;
+
+ if ( !table )
+ return true;
+
+ for ( count = 0; count < INTREMAP_ENTRIES; count++ )
+ {
+ if ( table[count] )
+ return false;
+ }
+ return true;
+}
+
+
+
static void dump_intremap_table(const u32 *table)
{
u32 count;
@@ -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) ) {
+ 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);
+ }
+
spin_unlock_irqrestore(&(ivrs_mapping->intremap_lock), flags);
return 0;
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] AMD IOMMU: fix debug console IOMMU intremap output
2018-12-04 21:47 [PATCH] AMD IOMMU: fix debug console IOMMU intremap output Woods, Brian
@ 2018-12-05 8:50 ` Paul Durrant
2018-12-05 9:00 ` Jan Beulich
1 sibling, 0 replies; 4+ messages in thread
From: Paul Durrant @ 2018-12-05 8:50 UTC (permalink / raw)
To: 'Woods, Brian', xen-devel@lists.xen.org
Cc: Suthikulpanit, Suravee, Roger Pau Monne
> -----Original Message-----
> From: Woods, Brian [mailto:Brian.Woods@amd.com]
> Sent: 04 December 2018 21:47
> To: xen-devel@lists.xen.org
> Cc: Suthikulpanit, Suravee <Suravee.Suthikulpanit@amd.com>; Woods, Brian
> <Brian.Woods@amd.com>; Paul Durrant <Paul.Durrant@citrix.com>; Roger Pau
> Monne <roger.pau@citrix.com>
> Subject: [PATCH] AMD IOMMU: fix debug console IOMMU intremap output
>
> When using the Xen debug console and printing the IOMMU intremap tables,
> it prints everything in the IVRS range regardless if it has an intr
> remap or not. Add some logic to cause an entry to only be printed if
> the intr remap table isn't empty.
>
> Signed-off-by: Brian Woods <brian.woods@amd.com>
> ---
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Roger Pau Monne <roger.pau@citrix.com>
>
> xen/drivers/passthrough/amd/iommu_intr.c | 34 ++++++++++++++++++++++++++-
> -----
> 1 file changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_intr.c
> b/xen/drivers/passthrough/amd/iommu_intr.c
> index dad2d1e5ab..e86300b57f 100644
> --- 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)
> +{
> + u32 count;
s/u32/uint32_t in both cases above.
> +
> + if ( !table )
> + return true;
> +
> + for ( count = 0; count < INTREMAP_ENTRIES; count++ )
> + {
Unnecessary braces.
> + if ( table[count] )
> + return false;
> + }
> + return true;
> +}
> +
> +
> +
> static void dump_intremap_table(const u32 *table)
> {
> u32 count;
> @@ -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) ) {
Style. The open brace should be on the line below.
The functionality looks fine though, so with those fixed...
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> + 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);
> + }
> +
> spin_unlock_irqrestore(&(ivrs_mapping->intremap_lock), flags);
>
> return 0;
> --
> 2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] AMD IOMMU: fix debug console IOMMU intremap output
2018-12-04 21:47 [PATCH] AMD IOMMU: fix debug console IOMMU intremap output Woods, Brian
2018-12-05 8:50 ` Paul Durrant
@ 2018-12-05 9:00 ` Jan Beulich
2018-12-05 19:56 ` Woods, Brian
1 sibling, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2018-12-05 9:00 UTC (permalink / raw)
To: Brian Woods
Cc: Paul Durrant, xen-devel, Suravee Suthikulpanit, Roger Pau Monne
>>> On 04.12.18 at 22:47, <Brian.Woods@amd.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] AMD IOMMU: fix debug console IOMMU intremap output
2018-12-05 9:00 ` Jan Beulich
@ 2018-12-05 19:56 ` Woods, Brian
0 siblings, 0 replies; 4+ messages in thread
From: Woods, Brian @ 2018-12-05 19:56 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel@lists.xen.org, Paul Durrant, Woods, Brian,
Suthikulpanit, Suravee, Roger Pau Monne
On Wed, Dec 05, 2018 at 02:00:43AM -0700, Jan Beulich wrote:
> >>> On 04.12.18 at 22:47, <Brian.Woods@amd.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-12-05 19:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-04 21:47 [PATCH] AMD IOMMU: fix debug console IOMMU intremap output Woods, Brian
2018-12-05 8:50 ` Paul Durrant
2018-12-05 9:00 ` Jan Beulich
2018-12-05 19:56 ` Woods, Brian
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.