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

[Kim Phillips <kim.phillips@amd.com>]

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