From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45579) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1THy8I-0004AZ-G9 for qemu-devel@nongnu.org; Sat, 29 Sep 2012 10:35:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1THy8H-0002RM-H3 for qemu-devel@nongnu.org; Sat, 29 Sep 2012 10:35:06 -0400 Received: from hub021-nj-7.exch021.serverdata.net ([206.225.164.223]:54094) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1THy8H-0002KO-Cc for qemu-devel@nongnu.org; Sat, 29 Sep 2012 10:35:05 -0400 Message-ID: <50670716.8050604@CloudSwitch.com> Date: Sat, 29 Sep 2012 10:35:02 -0400 From: Don Slutz MIME-Version: 1.0 References: <1347382813-5662-1-git-send-email-Don@CloudSwitch.com> <20120913135426.GB7640@redhat.com> <87ligdbpzr.fsf@codemonkey.ws> In-Reply-To: <87ligdbpzr.fsf@codemonkey.ws> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] hw: Add support for new LSI Logic devices. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" On 09/13/12 16:57, Anthony Liguori wrote: > "Michael S. Tsirkin" writes: > >> On Tue, Sep 11, 2012 at 01:00:13PM -0400, Don Slutz wrote: >>> + if (next_chain_offset) { >>> + MptSGEntryChain sgec; >>> + cpu_physical_memory_read(seg_start_pa + next_chain_offset, >>> + &sgec, sizeof(MptSGEntryChain)); >>> + assert(sgec.u2ElementType == MPTSGENTRYTYPE_CHAIN); >>> + next_sge_pa = sgec.u32SegmentAddressLow; >>> + if (sgec.f64BitAddress) { >>> + next_sge_pa |= >>> + ((uint64_t)sgec.u32SegmentAddressHigh) << 32; >>> + } >>> + seg_start_pa = next_sge_pa; >>> + next_chain_offset = sgec.u8NextChainOffset * sizeof(uint32_t); >> BTW all this logic seems wrong on big endian. >> Maybe we don't care short term but we do long term. > I think we care short term. It's easy enough to copy and past but i'm > not inclined to believe this will be maintained longer term if someone > makes the investment to de-uglify things. How important is the big endian support? lsi53c895a.c says: /* ??? Need to check if the {read,write}[wl] routines work properly on big-endian targets. */ I could do the same, I.E. code it up and submit it un-tested on big endian soon. Or since I currently do not have access to any real big endian hardware; do all testing on QEMU on QEMU. > I can tolerate a lot, but I'm not going to pull something with variable > names of 'u8NextChainOffset' :-) Changing this in tree is just > unnecessary churn. I have re-worked the variable names. For example: if (next_chain_offset) { MptSGEntryChain sgec; cpu_physical_memory_read(seg_start_pa + next_chain_offset, &sgec, sizeof(MptSGEntryChain)); assert(sgec.element_type == MPTSGENTRYTYPE_CHAIN); next_sge_pa = sgec.segment_address_low; if (sgec.bit_address) { next_sge_pa |= ((uint64_t)sgec.segment_address_high) << 32; } seg_start_pa = next_sge_pa; next_chain_offset = sgec.next_chain_offset * sizeof(uint32_t); Does this look better? > Regards, > > Anthony Liguori > >> I think you need to fix it up using le_to_cpu or something. >> And in particular this likely means bitfields can not be used cleanly, >> so you will not be able to resync lsilogic.h from virtualbox. >> The implication I guess is that we should just fix up the style >> to match qemu. >> >> -- >> MST -Don Slutz