From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH] KVM: x86: Save bits by merging Mmx/Sse/Avx bits Date: Fri, 07 Nov 2014 18:39:54 +0100 Message-ID: <545D03EA.4040005@redhat.com> References: <1415265301-16746-1-git-send-email-namit@cs.technion.ac.il> <20141107173719.GA18235@potion.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Nadav Amit Return-path: Received: from mx1.redhat.com ([209.132.183.28]:45757 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751437AbaKGRkB (ORCPT ); Fri, 7 Nov 2014 12:40:01 -0500 In-Reply-To: <20141107173719.GA18235@potion.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 07/11/2014 18:37, Radim Kr=C4=8Dm=C3=A1=C5=99 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 bi= t in the >> flags by merging them. >> >> Signed-off-by: Nadav Amit >> --- >=20 > It looks that Avx behaves a bit differently that legacy Sse, so havin= g > it exclusive is better. >=20 > 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 a= s 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=C4=8Dm=C3=A1=C5=99 >=20 >> 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 =3D=3D 3 */ >> #define Escape (5<<15) /* Escape to coprocessor instructio= n */ >> -#define Sse (1<<18) /* SSE Vector instruction */ >=20 > (I liked that Paolo moved something to the empty spot.) >=20 >> /* 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. MOV= DQA) */ >> -#define Unaligned ((u64)1 << 42) /* Explicitly unaligned (e.g. M= OVDQU) */ >> -#define Avx ((u64)1 << 43) /* Advanced Vector Extensions *= / >=20 > #define OpExtShift 40 >=20 >> +#define Sse ((u64)2 << 40) /* SSE Vector instruction */ >> +#define Avx ((u64)3 << 40) /* Advanced Vector Extensions *= / >=20 > (Precedents set OpExt{None,Mmx,Sse,Avx}.) >=20 >> +#define OpExtMask ((u64)3 << 40) >=20 > (Wouldn't Ext be enough?) >=20 >> +#define Aligned ((u64)1 << 42) /* Explicitly aligned (e.g. MOV= DQA) */ >> +#define Unaligned ((u64)1 << 43) /* Explicitly unaligned (e.g. M= OVDQU) */ >> #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 x= 86_emulate_ctxt *ctxt, >> struct operand *op) >> { >> unsigned reg =3D ctxt->modrm_reg; >> + u64 op_ext =3D ctxt->d & OpExtMask; >=20 > (We kept it inline.) >=20 >> =20 >> if (!(ctxt->d & ModRM)) >> reg =3D (ctxt->b & 7) | ((ctxt->rex_prefix & 1) << 3); >> =20 >> - if (ctxt->d & Sse) { >> + if (op_ext =3D=3D Sse) { >> op->type =3D OP_XMM; >> op->bytes =3D 16; >> op->addr.xmm =3D reg; >> read_sse_reg(ctxt, &op->vec_val, reg); >> return; >> } >> - if (ctxt->d & Mmx) { >> + if (op_ext =3D=3D Mmx) { >> reg &=3D 7; >> op->type =3D OP_MM; >> op->bytes =3D 8; > [...] >> @@ -1137,14 +1140,15 @@ static int decode_modrm(struct x86_emulate_c= txt *ctxt, >> - if ((ctxt->d & (Sse|Mmx)) && (ops->get_cr(ctxt, 0) & X86_CR0_TS))= { >> + if ((op_ext =3D=3D Sse || op_ext =3D=3D Mmx) && >=20 > It could be just op_ext here -- Avx doesn't differ. >=20 >> + (ops->get_cr(ctxt, 0) & X86_CR0_TS)) { >> rc =3D emulate_nm(ctxt); >> goto done; >> } >> =20 >> - if (ctxt->d & Mmx) { >> + if (op_ext =3D=3D Mmx) { >> rc =3D flush_pending_x87_faults(ctxt); >> if (rc !=3D X86EMUL_CONTINUE) >> goto done; >> --=20 >> 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