Re: [PATCH] KVM: x86: Save bits by merging Mmx/Sse/Avx bits

[Paolo Bonzini <pbonzini@redhat.com>]

On 07/11/2014 18:37, Radim Krčmář wrote:
> 2014-11-06 11:15+0200, Nadav Amit:
>> As we run out of bits in the KVM emulator instruction flags, we can merge
>> together the Mmx/Sse/Avx bits. These bits are mutual exclusive (i.e., each
>> instruction is either MMX, SSE, AVX, or none), so we can save one bit in the
>> flags by merging them.
>>
>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>> ---
> 
> It looks that Avx behaves a bit differently that legacy Sse, so having
> it exclusive is better.
> 
> I'd make changes, but the behavior doesn't look wrong now, so

Thanks for the review, Radim.

I think we have no clear idea of what Avx would do (I have one---same as
Sse but make VEX prefix mandatory, see VBROADCASTSS---but I'm not sure
it's the right one either).  Let's keep these patches on hold.

Paolo

> Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
> 
>>  arch/x86/kvm/emulate.c | 44 ++++++++++++++++++++++++++++----------------
>>  1 file changed, 28 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index cd2029b..f98ead7 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -123,7 +123,6 @@
>>  #define Prefix      (3<<15)     /* Instruction varies with 66/f2/f3 prefix */
>>  #define RMExt       (4<<15)     /* Opcode extension in ModRM r/m if mod == 3 */
>>  #define Escape      (5<<15)     /* Escape to coprocessor instruction */
>> -#define Sse         (1<<18)     /* SSE Vector instruction */
> 
> (I liked that Paolo moved something to the empty spot.)
> 
>>  /* Generic ModRM decode. */
>>  #define ModRM       (1<<19)
>>  /* Destination is only written; never read. */
>> @@ -155,9 +154,11 @@
>>  #define Src2GS      (OpGS << Src2Shift)
>>  #define Src2Mask    (OpMask << Src2Shift)
>>  #define Mmx         ((u64)1 << 40)  /* MMX Vector instruction */
>> -#define Aligned     ((u64)1 << 41)  /* Explicitly aligned (e.g. MOVDQA) */
>> -#define Unaligned   ((u64)1 << 42)  /* Explicitly unaligned (e.g. MOVDQU) */
>> -#define Avx         ((u64)1 << 43)  /* Advanced Vector Extensions */
> 
> #define OpExtShift 40
> 
>> +#define Sse         ((u64)2 << 40)  /* SSE Vector instruction */
>> +#define Avx         ((u64)3 << 40)  /* Advanced Vector Extensions */
> 
> (Precedents set OpExt{None,Mmx,Sse,Avx}.)
> 
>> +#define OpExtMask   ((u64)3 << 40)
> 
> (Wouldn't Ext be enough?)
> 
>> +#define Aligned     ((u64)1 << 42)  /* Explicitly aligned (e.g. MOVDQA) */
>> +#define Unaligned   ((u64)1 << 43)  /* Explicitly unaligned (e.g. MOVDQU) */
>>  #define Fastop      ((u64)1 << 44)  /* Use opcode::u.fastop */
>>  #define NoWrite     ((u64)1 << 45)  /* No writeback */
>>  #define SrcWrite    ((u64)1 << 46)  /* Write back src operand */
>> @@ -1082,18 +1083,19 @@ static void decode_register_operand(struct x86_emulate_ctxt *ctxt,
>>  				    struct operand *op)
>>  {
>>  	unsigned reg = ctxt->modrm_reg;
>> +	u64 op_ext = ctxt->d & OpExtMask;
> 
> (We kept it inline.)
> 
>>  
>>  	if (!(ctxt->d & ModRM))
>>  		reg = (ctxt->b & 7) | ((ctxt->rex_prefix & 1) << 3);
>>  
>> -	if (ctxt->d & Sse) {
>> +	if (op_ext == Sse) {
>>  		op->type = OP_XMM;
>>  		op->bytes = 16;
>>  		op->addr.xmm = reg;
>>  		read_sse_reg(ctxt, &op->vec_val, reg);
>>  		return;
>>  	}
>> -	if (ctxt->d & Mmx) {
>> +	if (op_ext == Mmx) {
>>  		reg &= 7;
>>  		op->type = OP_MM;
>>  		op->bytes = 8;
> [...]
>> @@ -1137,14 +1140,15 @@ static int decode_modrm(struct x86_emulate_ctxt *ctxt,
>> -		if ((ctxt->d & (Sse|Mmx)) && (ops->get_cr(ctxt, 0) & X86_CR0_TS)) {
>> +		if ((op_ext == Sse || op_ext == Mmx) &&
> 
> It could be just op_ext here -- Avx doesn't differ.
> 
>> +		    (ops->get_cr(ctxt, 0) & X86_CR0_TS)) {
>>  			rc = emulate_nm(ctxt);
>>  			goto done;
>>  		}
>>  
>> -		if (ctxt->d & Mmx) {
>> +		if (op_ext == Mmx) {
>>  			rc = flush_pending_x87_faults(ctxt);
>>  			if (rc != X86EMUL_CONTINUE)
>>  				goto done;
>> -- 
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html