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

[Don Slutz]

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