[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [XEN PATCH] xen: Add SAF deviations for MISRA C:2012 Rule 7.1
- To: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Luca Fancellu <Luca.Fancellu@xxxxxxx>
- From: Julien Grall <julien@xxxxxxx>
- Date: Mon, 9 Oct 2023 09:51:57 +0100
- Cc: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "michal.orzel@xxxxxxx" <michal.orzel@xxxxxxx>, "xenia.ragiadakou@xxxxxxx" <xenia.ragiadakou@xxxxxxx>, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>, "consulting@xxxxxxxxxxx" <consulting@xxxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Henry Wang <Henry.Wang@xxxxxxx>, Simone Ballarin <simone.ballarin@xxxxxxxxxxx>, Doug Goldstein <cardoe@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
- Delivery-date: Mon, 09 Oct 2023 08:52:11 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
Hi,
On 07/10/2023 01:43, Stefano Stabellini wrote:
On Fri, 6 Oct 2023, Luca Fancellu wrote:
On 6 Oct 2023, at 02:02, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
On Thu, 5 Oct 2023, Luca Fancellu wrote:
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
*/
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:
I am not sure this is better. This is a long line to read. But this is a
personal opinion.
On the technical side, can we easily teach a tool to format this kind of
exception? If not, then this should not be an exception we should implement.
/* 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?
Could we have a number in the comment to indicate the number of lines
the comment applies to?
Cheers,
--
Julien Grall
|
