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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 04/10/2023 12:52, 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.
Yes, line A is untouched.

<line A>
/* -E> safe MC3R1.R7.1 1 */ (deviated)
<line B>                    (deviated)



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