Re: [PATCH v3 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len()

["Jan Beulich" <JBeulich@suse.com>]

>>> On 31.01.19 at 19:07, <andrew.cooper3@citrix.com> wrote:
> On 07/01/2019 10:30, Jan Beulich wrote:
>>>>> On 31.12.18 at 12:37, <andrew.cooper3@citrix.com> wrote:
>>> Passing a 32-bit integer index into an array with entries containing less than
>>> 32 bits of data is wasteful, and creates an unnecessary error condition of
>>> passing an out-of-range index.
>>>
>>> The width of the X86EMUL_OPC() encoding is at most 24 bits, which leaves room
>>> for a modrm byte.
>> That's true for the 0x0f-prefix-space insns (and it's just 20 bits in that
>> case), but going this route we'd paint ourselves into a corner if we'd
>> ever have to add 0x0f38-, 0x0f3a-, or 0x8f0?-prefix-space insns.
> 
> We only need constants here for instructions which have intercepts.  I
> doubt any SIMD instructions are going to enter that category, but we can
> always reconsider the encoding scheme (probably to unsigned long) if
> this becomes an issue.

Okay then. Nevetheless one reference point regarding this: Recall
that not all insns in these two opcode spaces are SIMD. And as an
aside recall also that insns like {LD,ST}MXCSR are only half-way SIMD,
but have VEX encoded counterparts. I'm also not sure whether you'd
call {,F}X{SAVE,RSTOR} SIMD insns, yet XSAVES does have an
intercept (of course without exceeding the 24-bit encoding scheme).

>> Furthermore someone adjusting the encoding layout in x86_emulate.h
>> is very unlikely to notice breakage here until trying the resulting
>> binary - I strongly think some BUILD_BUG_ON() should be added to
>> make this apparent at build time.
> 
> It turns out that BUILD_BUG_ON() doesn't work, because the macro
> truncates at 32 bits due to types.
> 
> Using
> 
> #define INSTR_PAUSE       INSTR_ENC(X86EMUL_OPC_F3(0l, 0x90), 0)
> 
> i.e. using a long constant for for the ext field does end up triggering
> -Woverflow when I artificially set a high bit inside X86EMUL_OPC() 
> However, this isn't viable protection, as internal changes in
> X86EMUL_OPC() would render it useless.
> 
> 
> Given that a build time check is proving complicated, and that encoding
> errors will be blindingly obvious in debug builds (as the diagnostics
> will trigger and we'll hand #GP to the guest), and that it is unlikely
> that we're going to vastly change the encoding scheme, I think it will
> be fine to go without a build-time check.

Well, okay then.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel