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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 5 Oct 2023, at 00:32, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Wed, 4 Oct 2023, Luca Fancellu wrote:
>>> On 4 Oct 2023, at 11:29, Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx> wrote:
>>> On 04/10/2023 12:06, Luca Fancellu wrote:
>>>> Hi Nicola,
>>>>> On 4 Oct 2023, at 10:56, andrew.cooper3@xxxxxxxxxx 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@xxxxxxxxxxx>
>>>>>> "SAF" discussion aside that can be resolved elsewhere:
>>>>>> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>>>> 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
>>> 
>> 
>> Hi Nicola,
>> 
>>> 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).
>> 
>> Just to understand, does this way:
>> 
>> <line A>
>> /* -E> safe MC3R1.R7.1 1 */
>> <line B>
>> 
>> Justifies only line B? Because I thought so, but now I want to be sure, 
>> otherwise it doesn’t act
>> as intended.
>> 
>> 
>>> 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.
>> 
>> From the documentation:
>> 
>>    In the Xen codebase, these tags will be used to document and suppress 
>> findings:
>> 
>>    - SAF-X-safe: This tag means that the next line of code contains a 
>> finding, but
>>      the non compliance to the checker is analysed and demonstrated to be 
>> safe.
>> 
>> I understand that Eclair is capable of suppressing also the line in which 
>> the in-code suppression
>> comment resides, but these generic Xen in-code suppression comment are meant 
>> to be used
>> by multiple static analysis tools and many of them suppress only the line 
>> next to the comment
>> (Coverity, cppcheck).
> 
> As we see more realistic examples, it turns out that this is limiting.
> 
> Given that the SAF-2-safe comment needs to go through xen-analysis.py
> translations anyway, could we implement something a bit more flexible in
> xen-analysis.py?
> 
> For instance, could we implement a format with the number of lines of
> code like this as we discussed in a previous thread?
> 
> /* SAF-2-safe start */
> if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
>    (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
>    (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
> /* SAF-2-safe end */
> 
> Firstly, let ask Andrew, do you prefer this?
> 
> 
> And also this second format:
> 
> 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 */
> 
> 
> Could we implement in xen-analysis.py a conversion that would turn the
> two formats above that are not understood by cppcheck into:
> 
> /* cppcheck tag */
> if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
> /* cppcheck tag */
>    (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
> /* cppcheck tag */
>    (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
> 
> Or this is a problem because it would end up changing lines of code
> numbers in the source file?

Yes this is the real issue why we didn’t do the /* ... start */ code /* ... end 
*/


> 
> If we can hide the "ugliness" behind the xen-analysis conversion tool we
> could have a clean codebase and still be compatible with multiple tools.