From mboxrd@z Thu Jan  1 00:00:00 1970
From: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
Subject: Re: [PATCH] KVM: IOAPIC: only access APIC registers one dword at
 a time
Date: Fri, 02 Jul 2010 15:38:02 +0800
Message-ID: <4C2D975A.2050704@cn.fujitsu.com>
References: <4C2D6D4B.5060309@cn.fujitsu.com> <4C2D95EA.6080209@np.css.fujitsu.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Cc: Avi Kivity <avi@redhat.com>, Marcelo Tosatti <mtosatti@redhat.com>,
	LKML <linux-kernel@vger.kernel.org>,
	KVM list <kvm@vger.kernel.org>
To: Jin Dongming <jin.dongming@np.css.fujitsu.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from cn.fujitsu.com ([222.73.24.84]:53019 "EHLO song.cn.fujitsu.com"
	rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP
	id S1752202Ab0GBHlv (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 2 Jul 2010 03:41:51 -0400
In-Reply-To: <4C2D95EA.6080209@np.css.fujitsu.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>



Jin Dongming wrote:

>>  
>>  	switch (len) {
>> -	case 8:
>> -		*(u64 *) val = result;
>> -		break;
>> -	case 1:
>> -	case 2:
>>  	case 4:
>> -		memcpy(val, (char *)&result, len);
> 
> Here the parameter len is not used for reading data from ioapic register, before switch case
> the value of ioapic register has been read by ioapic_read_indirect().
> 

Yeah, it's right, but it's rarely operation that guest using incorrect width to access
the registers, so i don't think it's worthy to move the width judgment before the real
registers read.

> 
>> +		*(u32 *) val = result;
>>  		break;
>> +
>>  	default:
>>  		printk(KERN_WARNING "ioapic: wrong length %d\n", len);
> 
> And I think here is not good to print warning message. Maybe it is better to do
> such kind of checking at the first of this function before ioapic_read_indirect().
> 

ditto

Thanks for your review, Jin!