| Message ID | 20220914190330.60779-2-kim.phillips@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] iommu/amd: Fix ivrs_acpihid cmdline parsing code | expand |
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;
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
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;
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(-)