[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 22/25] x86/HVM: do actual CMPXCHG in hvmemul_cmpxchg()
On 05/02/18 08:32, Jan Beulich wrote: >>>> On 02.02.18 at 17:36, <andrew.cooper3@xxxxxxxxxx> wrote: >> On 07/12/17 14:16, Jan Beulich wrote: >>> --- a/xen/arch/x86/hvm/emulate.c >>> +++ b/xen/arch/x86/hvm/emulate.c >>> @@ -1296,8 +1296,83 @@ static int hvmemul_cmpxchg( >>> bool lock, >>> struct x86_emulate_ctxt *ctxt) >>> { >>> - /* Fix this in case the guest is really relying on r-m-w atomicity. */ >>> - return hvmemul_write(seg, offset, p_new, bytes, ctxt); >>> + struct hvm_emulate_ctxt *hvmemul_ctxt = >>> + container_of(ctxt, struct hvm_emulate_ctxt, ctxt); >>> + struct vcpu *curr = current; >>> + unsigned long addr, reps = 1; >>> + uint32_t pfec = PFEC_page_present | PFEC_write_access; >> I'm fairly certain from my pagetable work that passing PFEC_page_present >> here is bogus, and I do have (eventual) plans to make the pagewalk >> reject such values. > Both here and in the subsequent RMW patch I'm simply following > what hvmemul_write() does. I'd prefer all three to stay in sync. Fair enough. > >>> + case 16: >>> + if ( cpu_has_cx16 ) >>> + { >>> + __uint128_t *old = p_old, cur; >>> + >>> + if ( lock ) >>> + cur = __cmpxchg16b(mapping, old, p_new); >>> + else >>> + cur = cmpxchg16b_local_(mapping, old, p_new); >>> + if ( cur != *old ) >>> + { >>> + *old = cur; >>> + rc = X86EMUL_CMPXCHG_FAILED; >>> + } >>> + break; >>> + } >>> + /* fall through */ >>> + default: >> ASSERT_UNREACHABLE() ? >> >>> + rc = X86EMUL_UNHANDLEABLE; >>> + break; >>> + } > I'm not sure - from an abstract POV cpu_has_cx16 and the guest > seeing the feature available in its CPUID policy could differ. Granted > we're unlikely to want to try to emulate CMPXCHG16B without having > the instruction available ourselves, but it still wouldn't seem entirely > correct to assert here. I could remove the fall-through and _then_ > assert in the default case only. Let me know. The point was to catch bad sizes from being passed in. There is only a single ancient range of 64bit processors which don't have CX16, but I'd still argue that it would be a bug for the emulator to pass 16 down in such cases. > >>> --- a/xen/include/asm-x86/system.h >>> +++ b/xen/include/asm-x86/system.h >>> @@ -110,6 +110,38 @@ static always_inline unsigned long __cmp >>> return old; >>> } >>> >>> +static always_inline unsigned long cmpxchg_local_( >> unlocked_cmpxchg() ? > Not in line with our current naming scheme. Its rather more in line than introducing a local_ suffix. Trailing underscores are almost non-existant, and as far as I can tell, used exclusively in the internals of the compat code. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |