On Fri, 6 Oct 2023, Luca Fancellu wrote: > > On 6 Oct 2023, at 02:02, Stefano Stabellini <sstabell...@kernel.org> wrote: > > > > On Thu, 5 Oct 2023, Luca Fancellu wrote: > >>> On 5 Oct 2023, at 00:32, Stefano Stabellini <sstabell...@kernel.org> > >>> wrote: > >>> > >>> On Wed, 4 Oct 2023, Luca Fancellu wrote: > >>>>> On 4 Oct 2023, at 11:29, Nicola Vetrini <nicola.vetr...@bugseng.com> > >>>>> wrote: > >>>>> On 04/10/2023 12:06, Luca Fancellu wrote: > >>>>>> Hi Nicola, > >>>>>>> On 4 Oct 2023, at 10:56, andrew.coop...@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.vetr...@bugseng.com> > >>>>>>>> "SAF" discussion aside that can be resolved elsewhere: > >>>>>>>> Reviewed-by: Stefano Stabellini <sstabell...@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 > >>>>> > >>>> > >>>> 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 */ > > > > Right so the results would be all off by a few lines of code so when > > you go to read the report generated by cppcheck, the references > > wouldn't match anymore. > > > > Before giving up and accepting that we are constrained to only formats > > that don't change the LOC numbers, can we check what Coverity supports? > > > > I am asking because we could get away with implementing the formats > > above in cppcheck, given that cppcheck is open source. But for Coverity > > we need to stay with what is already supported by it. > > > > Does Coverity support anything other than: > > > > <tag on previous line> > > <next line is code with deviation> > > Unfortunately not, from its documentation I can’t see anything apart from the > above, > I can ask someone from synopsys though to double check.
I wonder how people would feel to have an exception to our coding style in these cases and have longer than 80 chars lines. I am asking because this is better than many of the other options above: /* SAF-x-safe */ if ( modrm_mod == MASK_EXTR(instr_modrm, 0300) && (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && (modrm_rm & 7) == MASK_EXTR(instr_modrm, 0007) ) Any other ideas?
