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 > > ~Andrew
