From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: [PATCH 1/3] KVM: x86 emulator: fix negative bit offset BitOp
 instruction emulation
Date: Sun, 08 Aug 2010 16:28:41 -0400
Message-ID: <4C5F1379.5040401@redhat.com>
References: <4C5BB725.3060609@cn.fujitsu.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=GB2312
Content-Transfer-Encoding: 7bit
Cc: kvm@vger.kernel.org
To: Wei Yongjun <yjwei@cn.fujitsu.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:1317 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1754374Ab0HHU2u (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 8 Aug 2010 16:28:50 -0400
In-Reply-To: <4C5BB725.3060609@cn.fujitsu.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

 On 08/06/2010 03:17 AM, Wei Yongjun wrote:
> If bit offset operands is a negative number, BitOp instruction
> will return wrong value. This patch fix it.
>
> +static void fetch_bit_operand(struct decode_cache *c)
> +{
> +	unsigned long mask, byte_offset;
> +
> +	if (c->dst.type == OP_MEM) {
> +		if (c->src.bytes == 2)
> +			c->src.val = (s16)c->src.val;
> +		else if (c->src.bytes == 4)
> +			c->src.val = (s32)c->src.val;

Better not to update in place, but instead use a local signed variable.

> +
> +		mask = ~(c->dst.bytes * 8 - 1);
> +
> +		if ((long)c->src.val < 0) {
> +			/* negative bit offset */
> +			byte_offset = c->dst.bytes +
> +				      ((-c->src.val - 1) & mask) / 8;
> +			c->dst.addr.mem -= byte_offset;
> +		} else {
> +			/* positive bit offset */
> +			c->dst.addr.mem += (c->src.val & mask) / 8;
> +		}
> +	}

Is the if () really necessary? If division translates to arithmetic
shift right, it might not be needed.

> +}
> +

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.