On 2023-01-10 at 14:07:16 -0800, matthew.gerlach@xxxxxxxxxxxxxxx wrote: > > > On Tue, 10 Jan 2023, Andy Shevchenko wrote: > > > On Mon, Jan 09, 2023 at 04:30:28PM -0800, matthew.gerlach@xxxxxxxxxxxxxxx wrote: > > > From: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx> > > > > > > Version 1 of the Device Feature Header (DFH) definition adds > > > functionality to the Device Feature List (DFL) bus. > > > > > > A DFHv1 header may have one or more parameter blocks that > > > further describes the HW to SW. Add support to the DFL bus > > > to parse the MSI-X parameter. > > > > > > The location of a feature's register set is explicitly > > > described in DFHv1 and can be relative to the base of the DFHv1 > > > or an absolute address. Parse the location and pass the information > > > to DFL driver. > > > > ... > > > > > v10: change dfh_find_param to return size of parameter data in bytes > > > > The problem that might occur with this approach is byte ordering. > > When we have u64 items, we know that they all are placed in CPU > > ordering by the bottom layer. What's the contract now? Can it be > > a problematic? Please double check this (always keep in mind BE32 > > as most interesting case for u64/unsigned long representation and > > other possible byte ordering outcomes). > > A number of u64 items certainly states explicit alignment of the memory, but > I think byte ordering is a different issue. > > The bottom layer, by design, is still enforcing a number u64 items under the > hood. So the contract has not changed. Changing units of size from u64s to > bytes was suggested to match the general practice of size of memory being in > bytes. I think the suggestion was made because the return type for > dfh_find_param() changed from u64* to void* in version 9, when indirectly > returning the size of the parameter data was introduced. So a void * with a > size in bytes makes sense. On the other hand, returning a u64 * is a more > precise reflection of the data alignment. I think the API should be as I prefer (void *) + bytes. The properties in the parameter block are not guarateed to be u64 for each, e.g. the REG_LAYOUT, so (void *) could better indicate it is not. It is just a block of data unknown to DFL core and to be parsed by drivers. And why users/drivers need to care about the alignment of the parameter block? Thanks, Yilun > follows: > > /** > * dfh_find_param() - find parameter block for the given parameter id > * @dfl_dev: dfl device > * @param_id: id of dfl parameter > * @pcount: destination to store size of parameter data in u64 bit words > * > * Return: pointer to start of parameter data, PTR_ERR otherwise. > */ > u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id, size_t > *pcount) > > Regarding byte ordering, Documentation/fpga/dfl.rst does not currently > mention endianness. All current HW implementations of DFL are little-endian. > I should add a statement in Documentation/fpga/dfl.rst that fields in the > Device Feature Header are little-endian. > > Thanks for the feedback, > Matthew Gerlach > > > > > -- > > With Best Regards, > > Andy Shevchenko > > > > > >
