On 06.02.2025 16:00, Oleksii Kurochko wrote:
>
> On 2/6/25 12:10 PM, Jan Beulich wrote:
>>>>> + case 's':
>>>>> + /*
>>>>> + * Workaround for invalid single-letter 's' & 'u' (QEMU):
>>>>> + * Before QEMU 7.1 it was an issue with misa to ISA string
>>>>> + * conversion:
>>>>> +
>>>>> *https://patchwork.kernel.org/project/qemu-devel/patch/dee09d708405075420b29115c1e9e87910b8da55.1648270894.git.research_tra...@irq.a4lg.com/#24792587
>>>>> + * Additional details of the workaround on Linux kernel
>>>>> side:
>>>>> +
>>>>> *https://lore.kernel.org/linux-riscv/[email protected]/#t
>>>>> + *
>>>>> + * No need to set the bit in riscv_isa as 's' & 'u' are
>>>>> + * not valid ISA extensions. It works unless the first
>>>>> + * multi-letter extension in the ISA string begins with
>>>>> + * "Su" and is not prefixed with an underscore.
>>>>> + */
>>>>> + if ( ext[-1] != '_' && ext[1] == 'u' )
>>>>> + {
>>>>> + ++isa;
>>>>> + ext_err = true;
>>>>> + break;
>>>>> + }
>>>>> + fallthrough;
>>>>> + case 'z':
>>>>> + /*
>>>>> + * Before attempting to parse the extension itself, we find
>>>>> its end.
>>>>> + * As multi-letter extensions must be split from other
>>>>> multi-letter
>>>>> + * extensions with an "_", the end of a multi-letter
>>>>> extension will
>>>>> + * either be the null character or the "_" at the start of
>>>>> the next
>>>>> + * multi-letter extension.
>>>>> + *
>>>>> + * Next, as the extensions version is currently ignored, we
>>>>> + * eliminate that portion. This is done by parsing backwards
>>>>> from
>>>>> + * the end of the extension, removing any numbers. This may
>>>>> be a
>>>>> + * major or minor number however, so the process is repeated
>>>>> if a
>>>>> + * minor number was found.
>>>>> + *
>>>>> + * ext_end is intended to represent the first character
>>>>> *after* the
>>>>> + * name portion of an extension, but will be decremented to
>>>>> the last
>>>>> + * character itself while eliminating the extensions version
>>>>> number.
>>>>> + * A simple re-increment solves this problem.
>>>>> + */
>>>>> + for ( ; *isa && *isa != '_'; ++isa )
>>>>> + if ( unlikely(!isalnum(*isa)) )
>>>>> + ext_err = true;
>>>>> +
>>>>> + ext_end = isa;
>>>>> + if ( unlikely(ext_err) )
>>>>> + break;
>>>> This, otoh, is an error. Which probably shouldn't go silently.
>>> It is actually not an error, what it means that if version is mentioned or
>>> not, so
>>> (1) rv32..._zbb_zbc
>>> (2) rv32..._zbb1p2_zbc
>>>
>>> For (1), ext_err = true and it means that version isn't mentioned for _zbb
>>> and after it
>>> immediately another extension name is going, so there is no any sense in
>>> ignoring (or parsing) version
>>> numbers.
>>> For (2), ext_err = false (because it ends on number ) so we have to ignore
>>> (or parse) version nubmers.
>> I don't follow. Why would ext_err be true for (1)? That's a well-formed
>> specifier, isn't it? rv32..._zbb.zbc (with the last dot meaning a literal
>> one, unlike the earlier ...) is an example of what would cause ext_err to
>> be true.
>
> My fault, you are right, ext_err will be false for (1).
>
> For your example, rv32..._zbb.zbc we have to do panic(), haven't we?
That's what I was trying to suggest earlier on. From anything we can't parse
we can't possibly infer whether we're able to run with the properties the
system has.
>>>>> + default:
>>>>> + /*
>>>>> + * Things are a little easier for single-letter extensions,
>>>>> as they
>>>>> + * are parsed forwards.
>>>>> + *
>>>>> + * After checking that our starting position is valid, we
>>>>> need to
>>>>> + * ensure that, when isa was incremented at the start of the
>>>>> loop,
>>>>> + * that it arrived at the start of the next extension.
>>>>> + *
>>>>> + * If we are already on a non-digit, there is nothing to do.
>>>>> Either
>>>>> + * we have a multi-letter extension's _, or the start of an
>>>>> + * extension.
>>>>> + *
>>>>> + * Otherwise we have found the current extension's major
>>>>> version
>>>>> + * number. Parse past it, and a subsequent p/minor version
>>>>> number
>>>>> + * if present. The `p` extension must not appear immediately
>>>>> after
>>>>> + * a number, so there is no fear of missing it.
>>>>> + */
>>>>> + if ( unlikely(!isalpha(*ext)) )
>>>>> + {
>>>>> + ext_err = true;
>>>>> + break;
>>>>> + }
>>>> Like above this also looks to be a situation that maybe better would be
>>>> left go entirely silently. I even wonder whether you can safely continue
>>>> the parsing in that case: How do you know whether what follows is indeed
>>>> the start of an extension name?
>>> Lets consider examples:
>>> (1) riscv,isa=im
>>> (2) riscv,isa=i1p2m
>>> (3) riscv,isa=i1m
>>>
>>> (4) riscv,isa=i1_m1
>>>
>>> Note: Underscores "_" may be used to separate ISA extensions to improve
>>> readability
>>> and to provide disambiguation, e.g., "RV32I2_M2_A2".
>>>
>>> (1) isa="i" so ext = "m" => no minor and major version between "i" and "m"
>>> => `ext` contains the name
>>> of the next extension name, thereby we will break a loop and go for
>>> parsing of the next extension
>>> which is "m" in this example
>>> (2) isa="i" => ext="1" => algorithm will go further and will just skip
>>> minor and major version for
>>> this extension (1p2 => 1.2 version of extension "i")
>>> (3) just the same as (2) but will stop earlier as minor version isn't set.
>>>
>>> Note: having "_" is legal from specification point of view, but it is
>>> illegal to use it between single letter
>>> extension from device tree binding point of view.
>>> (4) just the same as (2) and (3) but using "_" separator, so the if above
>>> will just skip "_" and the next
>>> after "_" is an extension name again.
>>>
>>> Considering that "_" is illegal from device tree point of view between
>>> single letter extensions we should have
>>> ASSERT(*ext != "_") and then only cases (1) - (3) will be possible to have.
>>> Probably it also will make a sense to have an array with legal single
>>> letter extensions and check that *ext is
>>> in this array.
>>>
>>> Interesting that device tree binding doesn't cover this case:
>>> ```
>>> Because the "P" extension for Packed SIMD can be confused for the decimal
>>> point in a version number,
>>> it must be preceded by an underscore if it follows a number. For example,
>>> "rv32i2p2" means version
>>> 2.2 of RV32I, whereas "rv32i2_p2" means version 2.0 of RV32I with version
>>> 2.0 of the P extension.
>>> ```
>>> if I correctly interpreted the
>>> pattern:pattern:^rv(?:64|32)imaf?d?q?c?b?k?j?p?v?h?(?:[hsxz](?:[0-9a-z])+)?(?:_[hsxz](?:[0-9a-z])+)*$
>>> And it also doesn't support versions for single letter extensions.
>>> So "rv32i2_m2_a2" or with using "p" is impossible?
>>> Then riscv_isa_parse_string() could be simplified ( probably when it was
>>> implemented in Linux kernel they don't have the binding for riscv,isa and
>>> they
>>> just tried to follow RISC-V specification ) for the case of single letter
>>> extensions (or just keep it as is to be in sync with Linux kernel).
>> All fine, but what about e.g.
>>
>> (5) riscv,isa=i?m1
>
> It is impossible from device tree point of view:
> 1. According to DT's patter after single letter extension couldn't be version.
> 2. Between "ima" can't be anything.
>
> But it shouldn't break an algorithm because:
> (1) parse "i" and nothing wrong with that.
> (2) "?" will be ignored because we have a check at the start of single letter
> extension case:
> if ( unlikely(!isalpha(*ext)) )
> so ext_err will be set to true
> (3) "?" will be ignored and go just to "m" because of set ext_err=true at the
> step (2).
> (4) Then "m" will be parsed successfully and 1 will be just ignored.
>
> Does it make sense?
See above - I don't think we should continue to run if parsing fails. Of
course we might, after tainting the system, in a best effort manner.
Jan
