From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: KVM: Fix the invlpg instruction emulation on AMD64 Date: Wed, 17 Oct 2007 17:40:45 +0200 Message-ID: <47162CFD.3060504@qumranet.com> References: <20071015190823.GA11333@hall.aurel32.net> <47148403.6010603@qumranet.com> <47148867.9070600@aurel32.net> <47148E82.9090103@qumranet.com> <20071017135021.GA32185@hall.aurel32.net> <47161E2B.1000006@qumranet.com> <47162AD3.6070602@aurel32.net> <47162B4F.3050002@aurel32.net> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: Aurelien Jarno Return-path: In-Reply-To: <47162B4F.3050002-rXXEIb44qovR7s880joybQ@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org Aurelien Jarno wrote: > Aurelien Jarno a =E9crit : > = >> Avi Kivity a =E9crit : >> = >>> Aurelien Jarno wrote: >>> = >>>> On Tue, Oct 16, 2007 at 12:12:18PM +0200, Avi Kivity wrote: >>>> = >>>> = >>>>> Aurelien Jarno wrote: >>>>> = >>>>> = >>>>>> I have marked the invlpg instruction the same way as it is done in >>>>>> kvm-37 to know what happens. I get either modrm_reg =3D 4 or =3D 6 w= hen the >>>>>> invlpg instruction is executed, but never =3D 7. >>>>>> >>>>>> = >>>>>> = >>>>>> = >>>>> Then it isn't the invlpg instruction at all. Rather smsw (modrm_reg = =3D=3D = >>>>> 4) or lmsw ( =3D=3D 6). >>>>> >>>>> I'm confused. >>>>> >>>>> (looks) >>>>> >>>>> Okay. What we have here is total breakage when emulating an instruct= ion = >>>>> that uses a mod r/m encoding that actually refers to a register = >>>>> (modrm_mod =3D=3D 3). In x86_decode_insn() we set src.type as OP_MEM= , and = >>>>> in x86_emulate_insn() we happily fetch it even though it's a register= , = >>>>> generating a fault. >>>>> >>>>> It usually doesn't bite us because these instructions are directly ex= ecuted. >>>>> >>>>> The fix is probably to switch to OP_REG if SrcMem and ModRM and = >>>>> modrm_mod =3D=3D 3 (similarly for DstMem). >>>>> >>>>> = >>>>> = >>>> Ok, I have tried to implement that for SrcMem, and it works in my case. >>>> I am able to boot FreeBSD, and I am seeing no regression for Linux or >>>> Hurd VMs. Does the patch below looks ok to you? >>>> >>>> >>>> >>>> KVM: Fix the invlpg instruction emulation on AMD64 >>>> >>>> The patch below correctly detects the invlpg instruction, and switch = >>>> src.type to OP_REG. This fixes the boot of FreeBSD on an AMD64 CPU, it= has >>>> been broken since commit aa38840d3d2e0a804e628077df8d8879b496d741. >>>> >>>> It also moves the assignation of c->src.bytes after the test as it is >>>> not needed for the invlpg instruction. >>>> >>>> Signed-off-by: Aurelien Jarno >>>> >>>> diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c >>>> index e974ace..06e183b 100644 >>>> --- a/drivers/kvm/x86_emulate.c >>>> +++ b/drivers/kvm/x86_emulate.c >>>> @@ -825,12 +825,12 @@ modrm_done: >>>> c->src.bytes =3D 4; >>>> goto srcmem_common; >>>> case SrcMem: >>>> - c->src.bytes =3D (c->d & ByteOp) ? 1 : >>>> - c->op_bytes; >>>> /* Don't fetch the address for invlpg: it could be unmapped. */ >>>> - if (c->twobyte && c->b =3D=3D 0x01 >>>> - && c->modrm_reg =3D=3D 7) >>>> + if ((c->d & ModRM) && c->modrm_mod =3D=3D 3) { >>>> + c->src.type =3D OP_REG; >>>> break; >>>> + } >>>> + c->src.bytes =3D (c->d & ByteOp) ? 1 : c->op_bytes; >>>> srcmem_common: >>>> c->src.type =3D OP_MEM; >>>> break; >>>> = >>>> = >>> Why did you remove the check for invlpg? It need not be executed with >>> modrm_mod =3D=3D 3 (in fact invlpg with modrm_mod =3D=3D 3 is useless),= so it >>> won't be caught by this. >>> = >> Because I really thing this check is buggy. On my tests, modrm_reg is >> always different than 7 for the invlpg instruction, so the test is >> always false. >> = > > JFTR, in kvm-37 (the latest version that work out of the box for > FreeBSD), SrcMem is removed from twobyte[1] on AMD. > > = It had this little beauty: > /* > * Tell the emulator that of the Group 7 instructions (sgdt, lidt, > etc.) we > * are interested only in invlpg and not in any of the rest. > * > * invlpg is a special instruction in that the data it references may not > * be mapped. > */ > void kvm_emulator_want_group7_invlpg(void) > { > twobyte_table[1] &=3D ~SrcMem; > } > EXPORT_SYMBOL_GPL(kvm_emulator_want_group7_invlpg); It took advantage of the fact that AMD doesn't need to emulate sgdt and lidt in real mode, so it wouldn't get called for these instructions. = But they are called on Intel so we can't break them. -- = Do not meddle in the internals of kernels, for they are subtle and quick to= panic. ------------------------------------------------------------------------- This SF.net email is sponsored by: Splunk Inc. Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/