| Message ID | 571A4E9F02000078000E4D15@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 22, 2016 at 08:17:35AM -0600, Jan Beulich wrote: > As both INVLPG and INVLPGA have basically the same exception rules > (leaving aside that INVLPGA requires SVME enabled, which so far isn't > being taken care of, and that INVLPG requires ModRM.mod != 3), fold > the handling of the two as much as possible alongside achieving the > goal of the patch (at once doing the #UD checks pror to the #GP one, > which ought to be more in line with how hardware does things). > > But please note that AMD and Intel disagree on what exceptions INVLPG > may raise, and the more reasonable Intel variant is being followed. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > So this patch supersedes Andrew's patch to fix invlpg on AMD with shadow paging? Ask because I have a list of things to track and not sure if this is something new. Wei.
>>> On 22.04.16 at 16:40, <wei.liu2@citrix.com> wrote: > On Fri, Apr 22, 2016 at 08:17:35AM -0600, Jan Beulich wrote: >> As both INVLPG and INVLPGA have basically the same exception rules >> (leaving aside that INVLPGA requires SVME enabled, which so far isn't >> being taken care of, and that INVLPG requires ModRM.mod != 3), fold >> the handling of the two as much as possible alongside achieving the >> goal of the patch (at once doing the #UD checks pror to the #GP one, >> which ought to be more in line with how hardware does things). >> >> But please note that AMD and Intel disagree on what exceptions INVLPG >> may raise, and the more reasonable Intel variant is being followed. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> > > So this patch supersedes Andrew's patch to fix invlpg on AMD with shadow > paging? No, it is addressing an issue I found while reviewing Andrew's. Jan
On 22/04/16 15:17, Jan Beulich wrote: > As both INVLPG and INVLPGA have basically the same exception rules > (leaving aside that INVLPGA requires SVME enabled, which so far isn't > being taken care of, We also don't appear to handle the ASID in %ecx correctly either. Yet another item on the TODO list for nested virt. > and that INVLPG requires ModRM.mod != 3), fold > the handling of the two as much as possible alongside achieving the > goal of the patch (at once doing the #UD checks pror to the #GP one, > which ought to be more in line with how hardware does things). > > But please note that AMD and Intel disagree on what exceptions INVLPG > may raise, and the more reasonable Intel variant is being followed. Which differences? You introduce the !in_protmode() check to the `invlpg` path. However, both manuals agree that it should work in real mode (subject to the lock prefix not being in use). I presume the difference is to whether it is eligible for use in vm86 mode, where indeed Intel is stricter than AMD. ~Andrew
>>> On 22.04.16 at 18:08, <andrew.cooper3@citrix.com> wrote: > On 22/04/16 15:17, Jan Beulich wrote: >> As both INVLPG and INVLPGA have basically the same exception rules >> (leaving aside that INVLPGA requires SVME enabled, which so far isn't >> being taken care of, > > We also don't appear to handle the ASID in %ecx correctly either. Yet > another item on the TODO list for nested virt. > >> and that INVLPG requires ModRM.mod != 3), fold >> the handling of the two as much as possible alongside achieving the >> goal of the patch (at once doing the #UD checks pror to the #GP one, >> which ought to be more in line with how hardware does things). >> >> But please note that AMD and Intel disagree on what exceptions INVLPG >> may raise, and the more reasonable Intel variant is being followed. > > Which differences? > > You introduce the !in_protmode() check to the `invlpg` path. However, > both manuals agree that it should work in real mode (subject to the lock > prefix not being in use). I presume the difference is to whether it is > eligible for use in vm86 mode, where indeed Intel is stricter than AMD. Oh, indeed - I must have mixed things up badly. Withdrawing the patch. Jan
--- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -3880,13 +3880,14 @@ x86_emulate( switch( modrm ) { case 0xdf: /* invlpga */ - generate_exception_if(!in_protmode(ctxt, ops), EXC_UD, -1); - generate_exception_if(!mode_ring0(), EXC_GP, 0); - fail_if(ops->invlpg == NULL); - if ( (rc = ops->invlpg(x86_seg_none, truncate_ea(_regs.eax), - ctxt)) ) - goto done; - goto no_writeback; + ea.mem.off = truncate_ea(_regs.eax); + /* + * This is questionable: AMD's PM calls the address used here both + * virtual and linear. They've informally indicated "linear" is + * meant there. + */ + ea.mem.seg = x86_seg_none; + goto invlpg; case 0xf9: /* rdtscp */ { uint64_t tsc_aux; fail_if(ops->read_msr == NULL); @@ -4006,8 +4007,10 @@ x86_emulate( goto done; break; case 7: /* invlpg */ - generate_exception_if(!mode_ring0(), EXC_GP, 0); generate_exception_if(ea.type != OP_MEM, EXC_UD, -1); + invlpg: + generate_exception_if(!in_protmode(ctxt, ops), EXC_UD, -1); + generate_exception_if(!mode_ring0(), EXC_GP, 0); fail_if(ops->invlpg == NULL); if ( (rc = ops->invlpg(ea.mem.seg, ea.mem.off, ctxt)) ) goto done;