Re: [XEN PATCH] xen: Add SAF deviations for MISRA C:2012 Rule 7.1

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 04/10/2023 12:06, Luca Fancellu wrote:
> Hi Nicola,
> 
>> On 4 Oct 2023, at 10:56, andrew.cooper3@citrix.com wrote:
>> 
>> On 03/10/2023 9:46 pm, Stefano Stabellini wrote:
>>> On Tue, 3 Oct 2023, Nicola Vetrini wrote:
>>> 
>>>> As specified in rules.rst, these constants can be used
>>>> in the code.
>>>> Their deviation is now accomplished by using a SAF comment,
>>>> rather than an ECLAIR configuration.
>>>> 
>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>> 
>>> "SAF" discussion aside that can be resolved elsewhere:
>>> 
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>> 
>> Well no.  "SAF" aside (and SAF does need fixing before reposting this 
>> patch, otherwise it's just unnecessary churn), ...
>> 
>>>> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
>>>> index d2a781fc3fb5..d0623b72ccfa 100644
>>>> --- a/xen/arch/x86/hvm/svm/svm.h
>>>> +++ b/xen/arch/x86/hvm/svm/svm.h
>>>> @@ -57,14 +57,23 @@ static inline void svm_invlpga(unsigned long 
>>>> linear, uint32_t asid)
>>>> #define INSTR_INT3 INSTR_ENC(X86EMUL_OPC( 0, 0xcc), 0)
>>>> #define INSTR_ICEBP INSTR_ENC(X86EMUL_OPC( 0, 0xf1), 0)
>>>> #define INSTR_HLT INSTR_ENC(X86EMUL_OPC( 0, 0xf4), 0)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_XSETBV INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371)
>>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
>>>> #define INSTR_WBINVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)
>> 
>> ... this has broken a tabulated structure to have comments ahead of 
>> lines with octal numbers, while ...
>> 
>>>> diff --git a/xen/arch/x86/hvm/svm/emulate.c 
>>>> b/xen/arch/x86/hvm/svm/emulate.c
>>>> index aa2c61c433b3..c5e3341c6316 100644
>>>> --- a/xen/arch/x86/hvm/svm/emulate.c
>>>> +++ b/xen/arch/x86/hvm/svm/emulate.c
>>>> @@ -90,9 +90,9 @@ unsigned int svm_get_insn_len(struct vcpu *v, 
>>>> unsigned int instr_enc)
>>>> if ( !instr_modrm )
>>>> return emul_len;
>>>> 
>>>> - if ( modrm_mod == MASK_EXTR(instr_modrm, 0300) &&
>>>> - (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
>>>> - (modrm_rm & 7) == MASK_EXTR(instr_modrm, 0007) )
>>>> + if ( modrm_mod == MASK_EXTR(instr_modrm, 0300) && /* SAF-2-safe */
>>>> + (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && /* SAF-2-safe 
>>>> */
>>>> + (modrm_rm & 7) == MASK_EXTR(instr_modrm, 0007) ) /* SAF-2-safe */
>>>> return emul_len;
>>>> }
>> 
>> ... this has comments at the end of lines with octal numbers.
>> 
>> So which is it?
> 
> I agree with Andrew here in this sense: the in-code comment is
> supposed to be on the line *before* the violation,
> not on the same line, so I’m also wondering how it is fixing the very
> first violation.
> 
> Cheers,
> Luca
> 

Actually it justifies what is on either the previous line or the same 
because it's
translated to /* -E> safe MC3R1.R7.1 1 */, where the last number is how 
many lines besides
the current one are to be deviated (e.g. you can have 0 deviate only the 
current line).
Most of the times the current form is what's needed, as you would put 
the comment on a line
of its own. In the case of the if that would break the formatting. The 
downside of doing the same thing on the table is that the first entry 
not to be deviated would actually be deviated.

#define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)

This may not be problematic, since 0 could be considered an octal 
constant, but is an
exception explicitly listed in the MISRA rule.
For the same reason the line

return emul_len;

is deviated by the above comment, but putting an octal constant there 
would for sure
be the result of a deliberate choice. There's the alternative of:

                           /* SAF-2-safe */
    if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
                           /* SAF-2-safe */
        (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
                           /* SAF-2-safe */
        (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )

to make it consistent with the table and avoid any "hidden" deviated 
line or, again,
the modification of the translation script so that it doesn't use a 
fixed "1" offset, which
is motivated by what you wrote on the thread of the modification of 
xen_analysis.py.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)