[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 4/5] x86emul: make write and cmpxchg hooks optional
>>> On 07.12.16 at 14:36, <andrew.cooper3@xxxxxxxxxx> wrote: > On 07/12/16 08:32, Jan Beulich wrote: >> And I don't see why the ->cpuid() hook would be required all of the >> sudden - all its uses are guarded by a NULL check. > > You made it convincing argument in c/s 043ad80 "x86: always supply > .cpuid() handler to x86_emulate()", as to why the ->cpuid() hook should > be expected. > > Especially if we are going to be adding more CPUID checks going forward, > I would take this opportunity to make it mandatory. But not in this patch, no. The more that such a patch then should also remove all the respective NULL checks. Plus at least x86_decode_insn() invokes x86_decode() without cpuid hook, so further care would be needed. Overall I'm not convinced it is a good idea to close the road for x86_emulate() uses without that hook - see the "as long as respective instructions may get used in that case" in the description of the commit you point out. >>>> @@ -2624,13 +2626,18 @@ x86_emulate( >>>> } >>>> else if ( !(d & Mov) ) /* optimisation - avoid slow emulated read >>>> */ >>>> { >>>> + fail_if(lock_prefix ? !ops->cmpxchg : !ops->write); >>>> if ( (rc = read_ulong(dst.mem.seg, dst.mem.off, >>>> &dst.val, dst.bytes, ctxt, ops)) ) >>>> goto done; >>>> dst.orig_val = dst.val; >>>> } >>>> - else /* Lock prefix is allowed only on RMW instructions. */ >>>> + else >>>> + { >>>> + /* Lock prefix is allowed only on RMW instructions. */ >>>> generate_exception_if(lock_prefix, EXC_UD); >>>> + fail_if(!ops->write); >>> I am not sure that these two new fail_if()'s are sensibly placed here, >>> remote from the use of the hooks they are protecting against. >> Well - I don't see a point continuing the emulation attempt in that >> case. They're being duplicated in the writeback code already >> anyway, for safety reasons. > > Ok. How about some comment indicating "bail early if we won't be able > to complete the operation" ? I can add a comment, but the wording you suggest doesn't seem appropriate, due to my further earlier argumentation (it being a bad idea to update other guest visible machine state). As I now realize that wouldn't in fact be a problem here, as the instructions writing to memory we emulate so far don't update other machine state, but it would still set a bad precedent, as there are others which do. And I really hope to get at least a good part of SSE and AVX done by the time 4.9 freezes. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |