[ofw] ib_portinfo_record_t padding history?

[Hal Rosenstock]

On Tue, May 18, 2010 at 5:18 PM, Smith, Stan <stan.smith at intel.com> wrote:
> Fab Tillier wrote:
>> Sean Hefty wrote on Tue, 18 May 2010 at 12:08:08
>>
>>>> Thoughts?
>>>
>>> The padding looks wrong based on the IB spec.  PortInfoRecord should
>>> have PortInfo as its last field.  The size of PortInfoRecord should
>>> be 58 bytes.
>>
>> AttributeOffset in the MAD is defined as number of 8-byte words
>> between attributes.  I believe the extra pad is there so that you can
>> treat the records in a GET_TABLE response as an array of
>> PortInfoRecords.  I don't know if any code actually uses it this way,
>> but that's what I guess the intent is.
>>
>>>> #include <complib/cl_packon.h>
>>>> typedef struct _ib_portinfo_record
>>>> {
>>>>        ib_net16_t              lid;
>>>>        uint8_t         port_num;
>>>>        uint8_t         resv;
>>>>        ib_port_info_t  port_info;
>>>>        uint8_t         pad[6];         <-- ??
>>>> }       PACK_SUFFIX ib_portinfo_record_t;
>>>> #include <complib/cl_packoff.h>
>>>
>>> My guess (and this is only a guess) is that the padding was added to
>>> make PortInfoRecord align on a 64-bit boundary, probably in case it
>>> were sent as part of an RMPP mad.  The PortInfo structure contains
>>> 64- bit values at the top of the structure.
>>
>> Nah, the attributes end up unaligned anyway thanks to the stellar
>> layout of the SA MADs.  I'm pretty sure it's there to allow indexing
>> as an array.  While the pad isn't formally documented, it will always
>> be there.  It should be harmless.
>>
>> -Fab
>
> Indeed a problem arises in osmtest when building a fabric inventory file 'osmtest.dat' via 'osmtest -f c'.
> The problem is exactly that of an SA response containing an array of ib_portinfo_records.
> With the padding the 2nd ib_portinfo_record[1] index computation is incorrectly offset by 2 bytes.
> Without the padding the index calculation is correct. This observation led me to the comparison of the Linux vs. Windows ib_portinfo_record_t definitions and subsequent removal of the padding.
> I agree the padding should not have been an issue; all things coded correctly.
>
> Digging further into the issue, I find the following Windows vs. Linux definition:
>
> AL_INLINE ib_net16_t AL_API
> ib_get_attr_offset(
>        IN              const   uint32_t                                        attr_size )
> {
>        if( attr_size & 0x07 )
>                return( cl_hton16( (uint16_t)(attr_size >> 3) + 1 ) );
>        else
>                return( cl_hton16( (uint16_t)(attr_size >> 3) ) );
> }
>
> Linux
>
> static inline ib_net16_t AL_API
> ib_get_attr_offset(const uint32_t attr_size )
> {
>        return( cl_hton16( (uint16_t)(attr_size >> 3) ) );
> }

Why does Windows have the if clause in ib_get_attr_off (and not match
the "Linux" version) ?

-- Hal


> 'attr_size' is passed in osmtest.c as 'sizeof(ib_portinfo_record_t)' when building an SA query.
> I suspect this is the root of the problem, as osmtest.c code which builds the SA query for all known portinfo records sets the madw attr_size as '(attr_size >> 3)'.
> The question becomes 'which is the correct version of ib_get_attr_offset()?'.
>
>
> _______________________________________________
> ofw mailing list
> ofw at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
>