Re: [PATCH v8 00/14] arm: rework id register storage

[Cornelia Huck <cohuck@redhat.com>]

On Wed, Jun 25 2025, Peter Maydell <peter.maydell@linaro.org> wrote:

> On Wed, 25 Jun 2025 at 10:10, Eric Auger <eric.auger@redhat.com> wrote:
>> However there are other checkpatch errors besides the one you reported, in
>> 52873a54ad arm/cpu: Store aa64isar0/aa64zfr0 into the idregs arrays
>> ERROR: line over 90 characters
>> #388: FILE: target/arm/kvm.c:225:
>> +    return ARM64_SYS_REG((sysreg & CP_REG_ARM64_SYSREG_OP0_MASK) >>
>> CP_REG_ARM64_SYSREG_OP0_SHIFT,
>>
>> ERROR: line over 90 characters
>> #389: FILE: target/arm/kvm.c:226:
>> +                         (sysreg & CP_REG_ARM64_SYSREG_OP1_MASK) >>
>> CP_REG_ARM64_SYSREG_OP1_SHIFT,
>>
>> ERROR: line over 90 characters
>> #390: FILE: target/arm/kvm.c:227:
>> +                         (sysreg & CP_REG_ARM64_SYSREG_CRN_MASK) >>
>> CP_REG_ARM64_SYSREG_CRN_SHIFT,
>>
>> ERROR: line over 90 characters
>> #391: FILE: target/arm/kvm.c:228:
>> +                         (sysreg & CP_REG_ARM64_SYSREG_CRM_MASK) >>
>> CP_REG_ARM64_SYSREG_CRM_SHIFT,
>>
>> ERROR: line over 90 characters
>> #392: FILE: target/arm/kvm.c:229:
>> +                         (sysreg & CP_REG_ARM64_SYSREG_OP2_MASK) >>
>> CP_REG_ARM64_SYSREG_OP2_SHIFT);
>>
>> WARNING: line over 80 characters
>> #396: FILE: target/arm/kvm.c:233:
>> +static int get_host_cpu_reg(int fd, ARMHostCPUFeatures *ahcf,
>> ARMIDRegisterIdx index)
>
> The last one of those is probably easily fixed, but note the general
> remark in style.rst that it's better to have an overlong line than
> one with an awkward and unreadable wrapping. (We ought to fix
> checkpatch and style.rst to agree on whether to warn or error and
> whether that should be at 90 chars or 100 chars, but that would require
> reopening an old style argument and it's too much effort.)

I would not disagree with fixing the last one if wanted, but I think the
others are more readable if we do not try to break them up.

>
>> 5f15ebdf3f arm/cpu: Add sysreg definitions in cpu-sysregs.h
>> ERROR: Macros with complex values should be enclosed in parenthesis
>> #56: FILE: target/arm/cpu-sysregs.h:21:
>> +#define DEF(NAME, OP0, OP1, CRN, CRM, OP2) NAME##_IDX,
>>
>> ERROR: Macros with complex values should be enclosed in parenthesis
>> #64: FILE: target/arm/cpu-sysregs.h:29:
>> +#define DEF(NAME, OP0, OP1, CRN, CRM, OP2) \
>> +    SYS_##NAME = ENCODE_ID_REG(OP0, OP1, CRN, CRM, OP2),
>>
>> ERROR: Macros with complex values should be enclosed in parenthesis
>> #203: FILE: target/arm/cpu64.c:40:
>> +#define DEF(NAME, OP0, OP1, CRN, CRM, OP2)      \
>> +    [NAME##_IDX] = SYS_##NAME,
>
> This is checkpatch not being able to cope with more complex
> uses of the preprocessor; the warning only makes sense for
> "acts like a function" macros.

Yep, that's why I ignored it.

In general, should we mention that we intentionally ignored
checkpatch.pl feedback, or would that be just noise?