From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Graf Subject: Re: [PATCH 5/5] megasas: LSI Megaraid SAS emulation Date: Tue, 05 Jul 2011 16:05:40 +0200 Message-ID: <4E131A34.4020601@suse.de> References: <1309863815-28236-1-git-send-email-hare@suse.de> <1309863815-28236-2-git-send-email-hare@suse.de> <1309863815-28236-3-git-send-email-hare@suse.de> <1309863815-28236-4-git-send-email-hare@suse.de> <1309863815-28236-5-git-send-email-hare@suse.de> <1309863815-28236-6-git-send-email-hare@suse.de> <4E1313E2.30400@suse.de> <4E1318DD.10703@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Hannes Reinecke , qemu-devel@nongnu.org, Stefan Haynoczi , kvm@vger.kernel.org, Christoph Hellwig To: Paolo Bonzini Return-path: Received: from cantor2.suse.de ([195.135.220.15]:39278 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751971Ab1GEOFl (ORCPT ); Tue, 5 Jul 2011 10:05:41 -0400 In-Reply-To: <4E1318DD.10703@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 07/05/2011 03:59 PM, Paolo Bonzini wrote: > On 07/05/2011 03:38 PM, Alexander Graf wrote: >>> + if (is_sgl64) { >>> + iov_pa = ldq_phys(pa); >>> + } else { >>> + iov_pa = ldl_phys(pa); >> These load data from memory in target endianness. Are you sure that's >> what you want? I'd expect this to be defined as little endian >> (especially given that ldq and ldl on the same address work). > Seems to be target endianness from the corresponding Linux code: > > if (sge_count) { > scsi_for_each_sg(scp, os_sgl, sge_count, i) { > mfi_sgl->sge32[i].length = sg_dma_len(os_sgl); > mfi_sgl->sge32[i].phys_addr = sg_dma_address(os_sgl); > } > } > > ... > > if (sge_count) { > scsi_for_each_sg(scp, os_sgl, sge_count, i) { > mfi_sgl->sge64[i].length = sg_dma_len(os_sgl); > mfi_sgl->sge64[i].phys_addr = sg_dma_address(os_sgl); > } > } > > Note that this is _either_ a ldq or a ldl depending on what the driver told > the device. It is not accessing a 64-bit value as 32-bit. So how would the device know which endianness the target is then? This looks like broken Linux code to me then. Christoph, is the above correct for big endian systems? Btw, reading through the Qemu sources, there is ldl_le_p() to read little endian values from memory. I haven't quite found one for _phys though. We can just add that however. Alex From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:41102) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qe6G0-0005Kg-FA for qemu-devel@nongnu.org; Tue, 05 Jul 2011 10:05:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qe6Fy-0003FH-NA for qemu-devel@nongnu.org; Tue, 05 Jul 2011 10:05:44 -0400 Received: from cantor2.suse.de ([195.135.220.15]:39279 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qe6Fy-0003F4-5w for qemu-devel@nongnu.org; Tue, 05 Jul 2011 10:05:42 -0400 Message-ID: <4E131A34.4020601@suse.de> Date: Tue, 05 Jul 2011 16:05:40 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1309863815-28236-1-git-send-email-hare@suse.de> <1309863815-28236-2-git-send-email-hare@suse.de> <1309863815-28236-3-git-send-email-hare@suse.de> <1309863815-28236-4-git-send-email-hare@suse.de> <1309863815-28236-5-git-send-email-hare@suse.de> <1309863815-28236-6-git-send-email-hare@suse.de> <4E1313E2.30400@suse.de> <4E1318DD.10703@redhat.com> In-Reply-To: <4E1318DD.10703@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/5] megasas: LSI Megaraid SAS emulation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kvm@vger.kernel.org, Christoph Hellwig , Hannes Reinecke , Stefan Haynoczi , qemu-devel@nongnu.org On 07/05/2011 03:59 PM, Paolo Bonzini wrote: > On 07/05/2011 03:38 PM, Alexander Graf wrote: >>> + if (is_sgl64) { >>> + iov_pa = ldq_phys(pa); >>> + } else { >>> + iov_pa = ldl_phys(pa); >> These load data from memory in target endianness. Are you sure that's >> what you want? I'd expect this to be defined as little endian >> (especially given that ldq and ldl on the same address work). > Seems to be target endianness from the corresponding Linux code: > > if (sge_count) { > scsi_for_each_sg(scp, os_sgl, sge_count, i) { > mfi_sgl->sge32[i].length = sg_dma_len(os_sgl); > mfi_sgl->sge32[i].phys_addr = sg_dma_address(os_sgl); > } > } > > ... > > if (sge_count) { > scsi_for_each_sg(scp, os_sgl, sge_count, i) { > mfi_sgl->sge64[i].length = sg_dma_len(os_sgl); > mfi_sgl->sge64[i].phys_addr = sg_dma_address(os_sgl); > } > } > > Note that this is _either_ a ldq or a ldl depending on what the driver told > the device. It is not accessing a 64-bit value as 32-bit. So how would the device know which endianness the target is then? This looks like broken Linux code to me then. Christoph, is the above correct for big endian systems? Btw, reading through the Qemu sources, there is ldl_le_p() to read little endian values from memory. I haven't quite found one for _phys though. We can just add that however. Alex