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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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

> 
> ~Andrew