[PATCH 1/2] iommu/amd: Fix ivrs_acpihid cmdline parsing code

[PATCH 1/2] iommu/amd: Fix ivrs_acpihid cmdline parsing code

[Kim Phillips]

The second (UID) strcmp in acpi_dev_hid_uid_match considers
"0" and "00" different, which can prevent device registration.

Have the AMD IOMMU driver's ivrs_acpihid parsing code remove
any leading zeroes to make the UID strcmp succeed.  Now users
can safely specify "AMDxxxxx:00" or "AMDxxxxx:0" and expect
the same behaviour.

Fixes: ca3bf5d47cec ("iommu/amd: Introduces ivrs_acpihid kernel parameter")
Signed-off-by: Kim Phillips <kim.phillips@amd.com>
Cc: stable@vger.kernel.org
Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Cc: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd/init.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index fdc642362c14..ef0e1a4b5a11 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3471,6 +3471,13 @@ static int __init parse_ivrs_acpihid(char *str)
 		return 1;
 	}
 
+	/*
+	 * Ignore leading zeroes after ':', so e.g., AMDI0095:00
+	 * will match AMDI0095:0 in the second strcmp in acpi_dev_hid_uid_match
+	 */
+	while (*uid == '0' && *(uid + 1))
+		uid++;
+
 	i = early_acpihid_map_size++;
 	memcpy(early_acpihid_map[i].hid, hid, strlen(hid));
 	memcpy(early_acpihid_map[i].uid, uid, strlen(uid));
-- 
2.34.1

[PATCH 2/2] iommu/amd: Fix ill-formed ivrs_ioapic, ivrs_hpet and ivrs_acpihid options

[Kim Phillips]

Currently, these options cause the following libkmod error:

libkmod: ERROR ../libkmod/libkmod-config.c:489 kcmdline_parse_result: \
	Ignoring bad option on kernel command line while parsing module \
	name: 'ivrs_xxxx[XX:XX'

Fix by introducing a new parameter format for these options and
throw a warning for the deprecated format.

Users are still allowed to omit the PCI Segment if zero.

Adding a Link: to the reason why we're modding the syntax parsing
in the driver and not in libkmod.

Fixes: ca3bf5d47cec ("iommu/amd: Introduces ivrs_acpihid kernel parameter")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/linux-modules/20200310082308.14318-2-lucas.demarchi@intel.com/
Reported-by: Kim Phillips <kim.phillips@amd.com>
Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Kim Phillips <kim.phillips@amd.com>
---
 .../admin-guide/kernel-parameters.txt         | 27 +++++--
 drivers/iommu/amd/init.c                      | 79 +++++++++++++------
 2 files changed, 76 insertions(+), 30 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index d7f30902fda0..23666104ab9b 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2294,7 +2294,13 @@
 			Provide an override to the IOAPIC-ID<->DEVICE-ID
 			mapping provided in the IVRS ACPI table.
 			By default, PCI segment is 0, and can be omitted.
-			For example:
+
+			For example, to map IOAPIC-ID decimal 10 to
+			PCI segment 0x1 and PCI device 00:14.0,
+			write the parameter as:
+				ivrs_ioapic=10@0001:00:14.0
+
+			Deprecated formats:
 			* To map IOAPIC-ID decimal 10 to PCI device 00:14.0
 			  write the parameter as:
 				ivrs_ioapic[10]=00:14.0
@@ -2306,7 +2312,13 @@
 			Provide an override to the HPET-ID<->DEVICE-ID
 			mapping provided in the IVRS ACPI table.
 			By default, PCI segment is 0, and can be omitted.
-			For example:
+
+			For example, to map HPET-ID decimal 10 to
+			PCI segment 0x1 and PCI device 00:14.0,
+			write the parameter as:
+				ivrs_ioapic=10@0001:00:14.0
+
+			Deprecated formats:
 			* To map HPET-ID decimal 0 to PCI device 00:14.0
 			  write the parameter as:
 				ivrs_hpet[0]=00:14.0
@@ -2317,15 +2329,20 @@
 	ivrs_acpihid	[HW,X86-64]
 			Provide an override to the ACPI-HID:UID<->DEVICE-ID
 			mapping provided in the IVRS ACPI table.
+			By default, PCI segment is 0, and can be omitted.
 
 			For example, to map UART-HID:UID AMD0020:0 to
 			PCI segment 0x1 and PCI device ID 00:14.5,
 			write the parameter as:
-				ivrs_acpihid[0001:00:14.5]=AMD0020:0
+				ivrs_acpihid=AMD0020:0@0001:00:14.5
 
-			By default, PCI segment is 0, and can be omitted.
-			For example, PCI device 00:14.5 write the parameter as:
+			Deprecated formats:
+			* To map UART-HID:UID AMD0020:0 to PCI segment is 0,
+			  PCI device ID 00:14.5, write the parameter as:
 				ivrs_acpihid[00:14.5]=AMD0020:0
+			* To map UART-HID:UID AMD0020:0 to PCI segment 0x1 and
+			  PCI device ID 00:14.5, write the parameter as:
+				ivrs_acpihid[0001:00:14.5]=AMD0020:0
 
 	js=		[HW,JOY] Analog joystick
 			See Documentation/input/joydev/joystick.rst.
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index ef0e1a4b5a11..13c6b581549c 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3385,18 +3385,24 @@ static int __init parse_amd_iommu_options(char *str)
 static int __init parse_ivrs_ioapic(char *str)
 {
 	u32 seg = 0, bus, dev, fn;
-	int ret, id, i;
+	int id, i;
 	u32 devid;
 
-	ret = sscanf(str, "[%d]=%x:%x.%x", &id, &bus, &dev, &fn);
-	if (ret != 4) {
-		ret = sscanf(str, "[%d]=%x:%x:%x.%x", &id, &seg, &bus, &dev, &fn);
-		if (ret != 5) {
-			pr_err("Invalid command line: ivrs_ioapic%s\n", str);
-			return 1;
-		}
+	if (sscanf(str, "=%d@%x:%x.%x", &id, &bus, &dev, &fn) == 4 ||
+	    sscanf(str, "=%d@%x:%x:%x.%x", &id, &seg, &bus, &dev, &fn) == 5)
+		goto found;
+
+	if (sscanf(str, "[%d]=%x:%x.%x", &id, &bus, &dev, &fn) == 4 ||
+	    sscanf(str, "[%d]=%x:%x:%x.%x", &id, &seg, &bus, &dev, &fn) == 5) {
+		pr_warn("Deprecated option : ivrs_ioapic%s\n", str);
+		pr_warn("Please see kernel parameters document and update the option.\n");
+		goto found;
 	}
 
+	pr_err("Invalid command line: ivrs_ioapic%s\n", str);
+	return 1;
+
+found:
 	if (early_ioapic_map_size == EARLY_MAP_SIZE) {
 		pr_err("Early IOAPIC map overflow - ignoring ivrs_ioapic%s\n",
 			str);
@@ -3417,18 +3423,24 @@ static int __init parse_ivrs_ioapic(char *str)
 static int __init parse_ivrs_hpet(char *str)
 {
 	u32 seg = 0, bus, dev, fn;
-	int ret, id, i;
+	int id, i;
 	u32 devid;
 
-	ret = sscanf(str, "[%d]=%x:%x.%x", &id, &bus, &dev, &fn);
-	if (ret != 4) {
-		ret = sscanf(str, "[%d]=%x:%x:%x.%x", &id, &seg, &bus, &dev, &fn);
-		if (ret != 5) {
-			pr_err("Invalid command line: ivrs_hpet%s\n", str);
-			return 1;
-		}
+	if (sscanf(str, "=%d@%x:%x.%x", &id, &bus, &dev, &fn) == 4 ||
+	    sscanf(str, "=%d@%x:%x:%x.%x", &id, &seg, &bus, &dev, &fn) == 5)
+		goto found;
+
+	if (sscanf(str, "[%d]=%x:%x.%x", &id, &bus, &dev, &fn) == 4 ||
+	    sscanf(str, "[%d]=%x:%x:%x.%x", &id, &seg, &bus, &dev, &fn) == 5) {
+		pr_warn("Deprecated option : ivrs_hpet%s\n", str);
+		pr_warn("Please see kernel parameters document and update the option.\n");
+		goto found;
 	}
 
+	pr_err("Invalid command line: ivrs_hpet%s\n", str);
+	return 1;
+
+found:
 	if (early_hpet_map_size == EARLY_MAP_SIZE) {
 		pr_err("Early HPET map overflow - ignoring ivrs_hpet%s\n",
 			str);
@@ -3449,19 +3461,36 @@ static int __init parse_ivrs_hpet(char *str)
 static int __init parse_ivrs_acpihid(char *str)
 {
 	u32 seg = 0, bus, dev, fn;
-	char *hid, *uid, *p;
+	char *hid, *uid, *p, *addr;
 	char acpiid[ACPIHID_UID_LEN + ACPIHID_HID_LEN] = {0};
-	int ret, i;
-
-	ret = sscanf(str, "[%x:%x.%x]=%s", &bus, &dev, &fn, acpiid);
-	if (ret != 4) {
-		ret = sscanf(str, "[%x:%x:%x.%x]=%s", &seg, &bus, &dev, &fn, acpiid);
-		if (ret != 5) {
-			pr_err("Invalid command line: ivrs_acpihid(%s)\n", str);
-			return 1;
+	int i;
+
+	addr = strchr(str, '@');
+	if (!addr) {
+		if (sscanf(str, "[%x:%x.%x]=%s", &bus, &dev, &fn, acpiid) == 4 ||
+		    sscanf(str, "[%x:%x:%x.%x]=%s", &seg, &bus, &dev, &fn, acpiid) == 5) {
+			pr_warn("Deprecated option: ivrs_acpihid%s\n", str);
+			pr_warn("Please see kernel parameters document and update the option.\n");
+			goto found;
 		}
+		goto not_found;
 	}
 
+	/* We have the '@', make it the terminator to get just the acpiid */
+	*addr++ = 0;
+
+	if (sscanf(str, "=%s", acpiid) != 1)
+		goto not_found;
+
+	if (sscanf(addr, "%x:%x.%x", &bus, &dev, &fn) == 3 ||
+	    sscanf(addr, "%x:%x:%x.%x", &seg, &bus, &dev, &fn) == 4)
+		goto found;
+
+not_found:
+	pr_err("Invalid command line: ivrs_acpihid%s\n", str);
+	return 1;
+
+found:
 	p = acpiid;
 	hid = strsep(&p, ":");
 	uid = p;
-- 
2.34.1

Re: [PATCH 2/2] iommu/amd: Fix ill-formed ivrs_ioapic, ivrs_hpet and ivrs_acpihid options

[Robin Murphy]

On 2022-09-14 20:03, Kim Phillips wrote:
> Currently, these options cause the following libkmod error:
> 
> libkmod: ERROR ../libkmod/libkmod-config.c:489 kcmdline_parse_result: \
> 	Ignoring bad option on kernel command line while parsing module \
> 	name: 'ivrs_xxxx[XX:XX'
> 
> Fix by introducing a new parameter format for these options and
> throw a warning for the deprecated format.
> 
> Users are still allowed to omit the PCI Segment if zero.
> 
> Adding a Link: to the reason why we're modding the syntax parsing
> in the driver and not in libkmod.
> 
> Fixes: ca3bf5d47cec ("iommu/amd: Introduces ivrs_acpihid kernel parameter")
> Cc: stable@vger.kernel.org
> Link: https://lore.kernel.org/linux-modules/20200310082308.14318-2-lucas.demarchi@intel.com/
> Reported-by: Kim Phillips <kim.phillips@amd.com>
> Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Kim Phillips <kim.phillips@amd.com>
> ---
>   .../admin-guide/kernel-parameters.txt         | 27 +++++--
>   drivers/iommu/amd/init.c                      | 79 +++++++++++++------
>   2 files changed, 76 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index d7f30902fda0..23666104ab9b 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2294,7 +2294,13 @@
>   			Provide an override to the IOAPIC-ID<->DEVICE-ID
>   			mapping provided in the IVRS ACPI table.
>   			By default, PCI segment is 0, and can be omitted.
> -			For example:

I wonder if it might be helpful to cross-reference the "pci=" option and 
spell out the general "<id>@<pci_dev>" format?

> +
> +			For example, to map IOAPIC-ID decimal 10 to
> +			PCI segment 0x1 and PCI device 00:14.0,
> +			write the parameter as:
> +				ivrs_ioapic=10@0001:00:14.0
> +
> +			Deprecated formats:
>   			* To map IOAPIC-ID decimal 10 to PCI device 00:14.0
>   			  write the parameter as:
>   				ivrs_ioapic[10]=00:14.0

...then we could just say that there's also a deprecated 
"ivrs_ioapic[<id>]=<pci_dev>" form. But then maybe it's hard to 
concisely express that the [] are literal here rather than denoting an 
optional value like everywhere else, Hmm...

Anyway, my underlying thought here is that providing an equally detailed 
example of what people shouldn't use as of what they should seems 
somewhat at odds with the message that they shouldn't be using it.

> @@ -2306,7 +2312,13 @@
>   			Provide an override to the HPET-ID<->DEVICE-ID
>   			mapping provided in the IVRS ACPI table.
>   			By default, PCI segment is 0, and can be omitted.
> -			For example:
> +
> +			For example, to map HPET-ID decimal 10 to
> +			PCI segment 0x1 and PCI device 00:14.0,
> +			write the parameter as:
> +				ivrs_ioapic=10@0001:00:14.0
> +
> +			Deprecated formats:
>   			* To map HPET-ID decimal 0 to PCI device 00:14.0
>   			  write the parameter as:
>   				ivrs_hpet[0]=00:14.0
> @@ -2317,15 +2329,20 @@
>   	ivrs_acpihid	[HW,X86-64]
>   			Provide an override to the ACPI-HID:UID<->DEVICE-ID
>   			mapping provided in the IVRS ACPI table.
> +			By default, PCI segment is 0, and can be omitted.
>   
>   			For example, to map UART-HID:UID AMD0020:0 to
>   			PCI segment 0x1 and PCI device ID 00:14.5,
>   			write the parameter as:
> -				ivrs_acpihid[0001:00:14.5]=AMD0020:0
> +				ivrs_acpihid=AMD0020:0@0001:00:14.5
>   
> -			By default, PCI segment is 0, and can be omitted.
> -			For example, PCI device 00:14.5 write the parameter as:
> +			Deprecated formats:
> +			* To map UART-HID:UID AMD0020:0 to PCI segment is 0,
> +			  PCI device ID 00:14.5, write the parameter as:
>   				ivrs_acpihid[00:14.5]=AMD0020:0
> +			* To map UART-HID:UID AMD0020:0 to PCI segment 0x1 and
> +			  PCI device ID 00:14.5, write the parameter as:
> +				ivrs_acpihid[0001:00:14.5]=AMD0020:0
>   
>   	js=		[HW,JOY] Analog joystick
>   			See Documentation/input/joydev/joystick.rst.
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index ef0e1a4b5a11..13c6b581549c 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -3385,18 +3385,24 @@ static int __init parse_amd_iommu_options(char *str)
>   static int __init parse_ivrs_ioapic(char *str)
>   {
>   	u32 seg = 0, bus, dev, fn;
> -	int ret, id, i;
> +	int id, i;
>   	u32 devid;
>   
> -	ret = sscanf(str, "[%d]=%x:%x.%x", &id, &bus, &dev, &fn);
> -	if (ret != 4) {
> -		ret = sscanf(str, "[%d]=%x:%x:%x.%x", &id, &seg, &bus, &dev, &fn);
> -		if (ret != 5) {
> -			pr_err("Invalid command line: ivrs_ioapic%s\n", str);
> -			return 1;
> -		}
> +	if (sscanf(str, "=%d@%x:%x.%x", &id, &bus, &dev, &fn) == 4 ||
> +	    sscanf(str, "=", &id, &seg, &bus, &dev, &fn) == 5)
> +		goto found;
> +
> +	if (sscanf(str, "[%d]=%x:%x.%x", &id, &bus, &dev, &fn) == 4 ||
> +	    sscanf(str, "[%d]=%x:%x:%x.%x", &id, &seg, &bus, &dev, &fn) == 5) {
> +		pr_warn("Deprecated option : ivrs_ioapic%s\n", str);

 From a user PoV this message seems unfairly confusing, since it's not 
actually the option that's deprecated, it's the format of the value of 
the option...

> +		pr_warn("Please see kernel parameters document and update the option.\n");

...and having messages split across multiple lines that get interleaved 
with other output, and are twice as wordy to be unhelpful than to simply 
say what was expected, is even less pleasant.

I'd suggest:

		pr_warn("ivrs_ioapic%s option format deprecated; use 
ivrs_ioapic=%d@%04x:%02x:%02x.%d instead\n",
			str, id, seg, bus, dev, fn);

which is concise* and consistent with how other deprecated IOMMU options 
are reported; It's not like we didn't understand what was passed, so we 
may as well make it as easy as copy-paste for the user to do what we're 
asking. Similarly for the others below.

Thanks,
Robin.


*Yes, it's a fairly long line. Screens are wide these days.

> +		goto found;
>   	}
>   
> +	pr_err("Invalid command line: ivrs_ioapic%s\n", str);
> +	return 1;
> +
> +found:
>   	if (early_ioapic_map_size == EARLY_MAP_SIZE) {
>   		pr_err("Early IOAPIC map overflow - ignoring ivrs_ioapic%s\n",
>   			str);
> @@ -3417,18 +3423,24 @@ static int __init parse_ivrs_ioapic(char *str)
>   static int __init parse_ivrs_hpet(char *str)
>   {
>   	u32 seg = 0, bus, dev, fn;
> -	int ret, id, i;
> +	int id, i;
>   	u32 devid;
>   
> -	ret = sscanf(str, "[%d]=%x:%x.%x", &id, &bus, &dev, &fn);
> -	if (ret != 4) {
> -		ret = sscanf(str, "[%d]=%x:%x:%x.%x", &id, &seg, &bus, &dev, &fn);
> -		if (ret != 5) {
> -			pr_err("Invalid command line: ivrs_hpet%s\n", str);
> -			return 1;
> -		}
> +	if (sscanf(str, "=%d@%x:%x.%x", &id, &bus, &dev, &fn) == 4 ||
> +	    sscanf(str, "=%d@%x:%x:%x.%x", &id, &seg, &bus, &dev, &fn) == 5)
> +		goto found;
> +
> +	if (sscanf(str, "[%d]=%x:%x.%x", &id, &bus, &dev, &fn) == 4 ||
> +	    sscanf(str, "[%d]=%x:%x:%x.%x", &id, &seg, &bus, &dev, &fn) == 5) {
> +		pr_warn("Deprecated option : ivrs_hpet%s\n", str);
> +		pr_warn("Please see kernel parameters document and update the option.\n");
> +		goto found;
>   	}
>   
> +	pr_err("Invalid command line: ivrs_hpet%s\n", str);
> +	return 1;
> +
> +found:
>   	if (early_hpet_map_size == EARLY_MAP_SIZE) {
>   		pr_err("Early HPET map overflow - ignoring ivrs_hpet%s\n",
>   			str);
> @@ -3449,19 +3461,36 @@ static int __init parse_ivrs_hpet(char *str)
>   static int __init parse_ivrs_acpihid(char *str)
>   {
>   	u32 seg = 0, bus, dev, fn;
> -	char *hid, *uid, *p;
> +	char *hid, *uid, *p, *addr;
>   	char acpiid[ACPIHID_UID_LEN + ACPIHID_HID_LEN] = {0};
> -	int ret, i;
> -
> -	ret = sscanf(str, "[%x:%x.%x]=%s", &bus, &dev, &fn, acpiid);
> -	if (ret != 4) {
> -		ret = sscanf(str, "[%x:%x:%x.%x]=%s", &seg, &bus, &dev, &fn, acpiid);
> -		if (ret != 5) {
> -			pr_err("Invalid command line: ivrs_acpihid(%s)\n", str);
> -			return 1;
> +	int i;
> +
> +	addr = strchr(str, '@');
> +	if (!addr) {
> +		if (sscanf(str, "[%x:%x.%x]=%s", &bus, &dev, &fn, acpiid) == 4 ||
> +		    sscanf(str, "[%x:%x:%x.%x]=%s", &seg, &bus, &dev, &fn, acpiid) == 5) {
> +			pr_warn("Deprecated option: ivrs_acpihid%s\n", str);
> +			pr_warn("Please see kernel parameters document and update the option.\n");
> +			goto found;
>   		}
> +		goto not_found;
>   	}
>   
> +	/* We have the '@', make it the terminator to get just the acpiid */
> +	*addr++ = 0;
> +
> +	if (sscanf(str, "=%s", acpiid) != 1)
> +		goto not_found;
> +
> +	if (sscanf(addr, "%x:%x.%x", &bus, &dev, &fn) == 3 ||
> +	    sscanf(addr, "%x:%x:%x.%x", &seg, &bus, &dev, &fn) == 4)
> +		goto found;
> +
> +not_found:
> +	pr_err("Invalid command line: ivrs_acpihid%s\n", str);
> +	return 1;
> +
> +found:
>   	p = acpiid;
>   	hid = strsep(&p, ":");
>   	uid = p;

Re: [PATCH 2/2] iommu/amd: Fix ill-formed ivrs_ioapic, ivrs_hpet and ivrs_acpihid options

[Kim Phillips]

On 9/14/22 4:04 PM, Robin Murphy wrote:
> On 2022-09-14 20:03, Kim Phillips wrote:
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index d7f30902fda0..23666104ab9b 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -2294,7 +2294,13 @@
>>               Provide an override to the IOAPIC-ID<->DEVICE-ID
>>               mapping provided in the IVRS ACPI table.
>>               By default, PCI segment is 0, and can be omitted.
>> -            For example:
> 
> I wonder if it might be helpful to cross-reference the "pci=" option and spell out the general "<id>@<pci_dev>" format?

I wouldn't want to reference pci= because pci= contents
suggest a whole slew of other options that may make users
wonder about what may also be available under ivrs_*=.
Also, pci= syntax may change without ivrs_*= syntax
changing, and since they have 0 code in common, that
would be bad.

>> +
>> +            For example, to map IOAPIC-ID decimal 10 to
>> +            PCI segment 0x1 and PCI device 00:14.0,
>> +            write the parameter as:
>> +                ivrs_ioapic=10@0001:00:14.0
>> +
>> +            Deprecated formats:
>>               * To map IOAPIC-ID decimal 10 to PCI device 00:14.0
>>                 write the parameter as:
>>                   ivrs_ioapic[10]=00:14.0
> 
> ...then we could just say that there's also a deprecated "ivrs_ioapic[<id>]=<pci_dev>" form. But then maybe it's hard to concisely express that the [] are literal here rather than denoting an optional value like everywhere else, Hmm...

Yeah, in ivrs_*[<id>]=, <id> is now optional, too.

> Anyway, my underlying thought here is that providing an equally detailed example of what people shouldn't use as of what they should seems somewhat at odds with the message that they shouldn't be using it.

The old syntax has been around since 2016, and because
of the Fixes: and stable, I think the rationale here is
we want to make sure users of the old syntax that the
old syntax is still possible.

Maybe stable is too much for patch 2/2 though.

>> @@ -2306,7 +2312,13 @@
>>               Provide an override to the HPET-ID<->DEVICE-ID
>>               mapping provided in the IVRS ACPI table.
>>               By default, PCI segment is 0, and can be omitted.
>> -            For example:
>> +
>> +            For example, to map HPET-ID decimal 10 to
>> +            PCI segment 0x1 and PCI device 00:14.0,
>> +            write the parameter as:
>> +                ivrs_ioapic=10@0001:00:14.0
>> +
>> +            Deprecated formats:
>>               * To map HPET-ID decimal 0 to PCI device 00:14.0
>>                 write the parameter as:
>>                   ivrs_hpet[0]=00:14.0

[There's a cut-n-paste bug here: I made ivrs_ioapic= -> ivrs_hpet=.]

>> +++ b/drivers/iommu/amd/init.c
>> @@ -3385,18 +3385,24 @@ static int __init parse_amd_iommu_options(char *str)
>>   static int __init parse_ivrs_ioapic(char *str)
>>   {
>>       u32 seg = 0, bus, dev, fn;
>> -    int ret, id, i;
>> +    int id, i;
>>       u32 devid;
>> -    ret = sscanf(str, "[%d]=%x:%x.%x", &id, &bus, &dev, &fn);
>> -    if (ret != 4) {
>> -        ret = sscanf(str, "[%d]=%x:%x:%x.%x", &id, &seg, &bus, &dev, &fn);
>> -        if (ret != 5) {
>> -            pr_err("Invalid command line: ivrs_ioapic%s\n", str);
>> -            return 1;
>> -        }
>> +    if (sscanf(str, "=%d@%x:%x.%x", &id, &bus, &dev, &fn) == 4 ||
>> +        sscanf(str, "=", &id, &seg, &bus, &dev, &fn) == 5)
>> +        goto found;
>> +
>> +    if (sscanf(str, "[%d]=%x:%x.%x", &id, &bus, &dev, &fn) == 4 ||
>> +        sscanf(str, "[%d]=%x:%x:%x.%x", &id, &seg, &bus, &dev, &fn) == 5) {
>> +        pr_warn("Deprecated option : ivrs_ioapic%s\n", str);
> 
>  From a user PoV this message seems unfairly confusing, since it's not actually the option that's deprecated, it's the format of the value of the option...

That could depend on what one considers is on the LHS vs RHS of the '='...

>> +        pr_warn("Please see kernel parameters document and update the option.\n");
> 
> ...and having messages split across multiple lines that get interleaved with other output, and are twice as wordy to be unhelpful than to simply say what was expected, is even less pleasant.

This isn't common, but yes.

> I'd suggest:
> 
>          pr_warn("ivrs_ioapic%s option format deprecated; use ivrs_ioapic=%d@%04x:%02x:%02x.%d instead\n",
>              str, id, seg, bus, dev, fn);
> 
> which is concise* and consistent with how other deprecated IOMMU options are reported; It's not like we didn't understand what was passed, so we may as well make it as easy as copy-paste for the user to do what we're asking. Similarly for the others below.

Granted, done.

Thanks,

Kim