Re: [Qemu-devel] [PATCH] hw: Add support for new LSI Logic devices.

[Don Slutz <Don@CloudSwitch.com>]

On 09/13/12 16:57, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> 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