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@... 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;