Re: [Xen-devel] [for-4.7] x86/emulate: synchronize LOCKed instruction emulation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.05.16 at 16:20, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> I've kept experimenting with the patch but can't quite figure out why
> minimizing the lock scope to the writeback part would not be sufficient,
> but it isn't.
> 
> I.e. with this code:
> 
> 3824  writeback:
> 3825     ops->smp_lock(lock_prefix);
> 3826     switch ( dst.type )
> 3827     {
> 3828     case OP_REG:
> 3829         /* The 4-byte case *is* correct: in 64-bit mode we zero-extend. 
> */
> 3830         switch ( dst.bytes )
> 3831         {
> 3832         case 1: *(uint8_t  *)dst.reg = (uint8_t)dst.val; break;
> 3833         case 2: *(uint16_t *)dst.reg = (uint16_t)dst.val; break;
> 3834         case 4: *dst.reg = (uint32_t)dst.val; break; /* 64b: zero-ext */
> 3835         case 8: *dst.reg = dst.val; break;
> 3836         }
> 3837         break;
> 3838     case OP_MEM:
> 3839         if ( !(d & Mov) && (dst.orig_val == dst.val) &&
> 3840              !ctxt->force_writeback )
> 3841             /* nothing to do */;
> 3842         else if ( lock_prefix )
> 3843             rc = ops->cmpxchg(
> 3844                 dst.mem.seg, dst.mem.off, &dst.orig_val,
> 3845                 &dst.val, dst.bytes, ctxt);
> 3846         else
> 3847             rc = ops->write(
> 3848                 dst.mem.seg, dst.mem.off, &dst.val, dst.bytes, ctxt);
> 3849         if ( rc != 0 )
> 3850         {
> 3851             ops->smp_unlock(lock_prefix);
> 3852             goto done;
> 3853         }
> 3854     default:
> 3855         break;
> 3856     }
> 3857     ops->smp_unlock(lock_prefix);
> 
> I can still reproduce the guest hang. But if I lock at the very
> beginning of x86_emulate() and unlock before each return, no more hangs.

Isn't that obvious? Locked instructions are necessarily
read-modify-write ones, and hence the lock needs to be taken
before the read, and dropped after the write. But remember, I'll
continue to show opposition to this getting "fixed" this way (in the
emulator itself), as long as no proper explanation can be given
why making hvmemul_cmpxchg() do what its name says isn't all
we need (and hence why i386-like bus lock behavior is needed).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel