Re: [PATCH 2/2] KVM: x86 emulator: add decoding of CMPXCHG8B dst operand.

[Avi Kivity <avi@redhat.com>]

On 03/21/2010 04:44 PM, Gleb Natapov wrote:
> On Sun, Mar 21, 2010 at 04:41:24PM +0200, Avi Kivity wrote:
>    
>> On 03/21/2010 01:08 PM, Gleb Natapov wrote:
>>      
>>> Decode CMPXCHG8B destination operand in decoding stage. Fixes regression
>>> introduced by "If LOCK prefix is used dest arg should be memory" commit.
>>> This commit relies on dst operand be decoded at the beginning of an
>>> instruction emulation.
>>>        
>>      
>>> @@ -1719,15 +1719,12 @@ static inline int emulate_grp9(struct x86_emulate_ctxt *ctxt,
>>>   		c->regs[VCPU_REGS_RAX] = (u32) (old>>   0);
>>>   		c->regs[VCPU_REGS_RDX] = (u32) (old>>   32);
>>>   		ctxt->eflags&= ~EFLG_ZF;
>>> -
>>>   	} else {
>>> -		new = ((u64)c->regs[VCPU_REGS_RCX]<<   32) |
>>> +		c->dst.val = ((u64)c->regs[VCPU_REGS_RCX]<<   32) |
>>>   		       (u32) c->regs[VCPU_REGS_RBX];
>>>
>>> -		rc = ops->cmpxchg_emulated(c->modrm_ea,&old,&new, 8, ctxt->vcpu);
>>> -		if (rc != X86EMUL_CONTINUE)
>>> -			return rc;
>>>   		ctxt->eflags |= EFLG_ZF;
>>> +		c->lock_prefix = 1;
>>>        
>> Why is this bit needed?  cmpxchg64b without lock is valid and racy,
>> but the guest may know it is safe.
>>
>>      
> Agree. Before this patch cmpxchg8b emulation always called
> cmpxchg_emulated(), so to be extra careful I wanted to preserve old
> behaviour. Resend the patch without this line?
>    

Better a 3/2 that removes it.  So we have a large patch that just 
transforms code, and a small patch that corrects an earlier bug.  May 
help a bisector one day.

-- 
error compiling committee.c: too many arguments to function