linux-kernel - Re: [PATCH v2] iommu/amd: Fix extended features logging

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 11 Apr 2021 15:21:49 -0700
From:   Joe Perches <joe@...ches.com>
To:     Alexander Monakov <amonakov@...ras.ru>,
        linux-kernel@...r.kernel.org
Cc:     Paul Menzel <pmenzel@...gen.mpg.de>,
        Joerg Roedel <jroedel@...e.de>,
        Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
        iommu@...ts.linux-foundation.org
Subject: Re: [PATCH v2] iommu/amd: Fix extended features logging

On Mon, 2021-04-12 at 00:13 +0300, Alexander Monakov wrote:
> print_iommu_info prints the EFR register and then the decoded list of
> features on a separate line:
> 
> pci 0000:00:00.2: AMD-Vi: Extended features (0x206d73ef22254ade):
>  PPR X2APIC NX GT IA GA PC GA_vAPIC
> 
> The second line is emitted via 'pr_cont', which causes it to have a
> different ('warn') loglevel compared to the previous line ('info').
> 
> Commit 9a295ff0ffc9 attempted to rectify this by removing the newline
> from the pci_info format string, but this doesn't work, as pci_info
> calls implicitly append a newline anyway.
> 
> Printing the decoded features on the same line would make it quite long.
> Instead, change pci_info() to pr_info() to omit PCI bus location info,
> which is shown in the preceding message anyway. This results in:
> 
> pci 0000:00:00.2: AMD-Vi: Found IOMMU cap 0x40
> AMD-Vi: Extended features (0x206d73ef22254ade): PPR X2APIC NX GT IA GA PC GA_vAPIC
> AMD-Vi: Interrupt remapping enabled
> 
> Fixes: 9a295ff0ffc9 ("iommu/amd: Print extended features in one line to fix divergent log levels")
> Link: https://lore.kernel.org/lkml/alpine.LNX.2.20.13.2104112326460.11104@monopod.intra.ispras.ru
> Signed-off-by: Alexander Monakov <amonakov@...ras.ru>
> Cc: Paul Menzel <pmenzel@...gen.mpg.de>
> Cc: Joerg Roedel <jroedel@...e.de>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
> Cc: iommu@...ts.linux-foundation.org
> ---
> 
> v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer
> solution
> 
>  drivers/iommu/amd/init.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 596d0c413473..62913f82a21f 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -1929,8 +1929,8 @@ static void print_iommu_info(void)
>  		pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr);
>  
> 
>  		if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
> -			pci_info(pdev, "Extended features (%#llx):",
> -				 iommu->features);
> +			pr_info("Extended features (%#llx):", iommu->features);
> +
>  			for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
>  				if (iommu_feature(iommu, (1ULL << i)))
>  					pr_cont(" %s", feat_str[i]);

How about avoiding all of this by using a temporary buffer
and a single pci_info.

Miscellanea:
o Move the feat_str and i declarations into the if block for locality

---
 drivers/iommu/amd/init.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 321f5906e6ed..0d219044726e 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1943,30 +1943,37 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
 
 static void print_iommu_info(void)
 {
-	static const char * const feat_str[] = {
-		"PreF", "PPR", "X2APIC", "NX", "GT", "[5]",
-		"IA", "GA", "HE", "PC"
-	};
 	struct amd_iommu *iommu;
 
 	for_each_iommu(iommu) {
 		struct pci_dev *pdev = iommu->dev;
-		int i;
 
 		pci_info(pdev, "Found IOMMU cap 0x%x\n", iommu->cap_ptr);
 
 		if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
-			pci_info(pdev, "Extended features (%#llx):",
-				 iommu->features);
+			static const char * const feat_str[] = {
+				"PreF", "PPR", "X2APIC", "NX", "GT", "[5]",
+				"IA", "GA", "HE", "PC"
+			};
+			int i;
+			char features[128] = "";
+			int len = 0;
+
 			for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
-				if (iommu_feature(iommu, (1ULL << i)))
-					pr_cont(" %s", feat_str[i]);
+				if (!iommu_feature(iommu, BIT_ULL(i)))
+					continue;
+				len += scnprintf(features + len,
+						 sizeof(features) - len,
+						 " %s", feat_str[i]);
 			}
 
 			if (iommu->features & FEATURE_GAM_VAPIC)
-				pr_cont(" GA_vAPIC");
+				len += scnprintf(features + len,
+						 sizeof(features) - len,
+						 " %s", "GA_vPIC");
 
-			pr_cont("\n");
+			pci_info(pdev, "Extended features (%#llx):%s\n",
+				 iommu->features, features);
 		}
 	}
 	if (irq_remapping_enabled) {