From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:56490) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R8MMe-000547-4T for qemu-devel@nongnu.org; Mon, 26 Sep 2011 21:21:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R8MMc-0002IW-TR for qemu-devel@nongnu.org; Mon, 26 Sep 2011 21:21:40 -0400 Received: from e23smtp03.au.ibm.com ([202.81.31.145]:35320) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R8MMc-0002IC-88 for qemu-devel@nongnu.org; Mon, 26 Sep 2011 21:21:38 -0400 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [202.81.31.247]) by e23smtp03.au.ibm.com (8.14.4/8.13.1) with ESMTP id p8R1FsDN025146 for ; Tue, 27 Sep 2011 11:15:54 +1000 Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p8R1JLL12363438 for ; Tue, 27 Sep 2011 11:19:21 +1000 Received: from d23av02.au.ibm.com (loopback [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p8R1LOj3015560 for ; Tue, 27 Sep 2011 11:21:24 +1000 Message-ID: <4E8124E3.7020100@linux.vnet.ibm.com> Date: Tue, 27 Sep 2011 09:20:35 +0800 From: Ray Wang MIME-Version: 1.0 References: <1317018152-2574-1-git-send-email-raywang@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] Changed the type of val argument of the function gt64120_writel() from uint32_t to uint64_t, so change the corresponding bswap32() to bswap64(). List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-devel@nongnu.org On 9/26/2011 5:46 PM, Peter Maydell wrote: > On 26 September 2011 07:22, Ray Wang wrote: >> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c >> index 1c34253..d0a31d2 100644 >> --- a/hw/gt64xxx.c >> +++ b/hw/gt64xxx.c >> @@ -312,7 +312,7 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr, >> uint32_t saddr; >> >> if (!(s->regs[GT_CPU]& 0x00001000)) >> - val = bswap32(val); >> + val = bswap64(val); >> >> saddr = (addr& 0xfff)>> 2; >> switch (saddr) { >> @@ -533,7 +533,7 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr, >> break; >> case GT_PCI0_CFGDATA: >> if (!(s->regs[GT_PCI0_CMD]& 1)&& (s->pci.config_reg& 0x00fff800)) >> - val = bswap32(val); >> + val = bswap64(val); >> if (s->pci.config_reg& (1u<< 31)) >> pci_data_write(s->pci.bus, s->pci.config_reg, val, 4); >> break; > I don't know this device, but this looks a bit suspicious. If > you do a bswap64() on the value for a 32-bit write this will put > the data into the high 32 bits and zeros into the low half; then > storing into s->regs[] will just write a zero (since regs[] is > 32 bits), won't it? Changing only writel and not readl also looks > odd. Yes, you're right. However, if a 64-bit value with effective high 32 bits is considered as a 32-bit one, some significant information will be lost. > > What is the bug this change is trying to fix? It is always a potential bug to process a 64-bit value as 32-bit one, isn't it? > > -- PMM > -- Best Regards, ------------------------------------------------- Ray Wang Linux Technology Center, KVM China IBM Corp., Beijing, China xianleiw@cn.ibm.com