On Wed, 11 Jan 2023, Xu Yilun wrote:
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.
OK, (void *) + size in bytes is fine.
And why users/drivers need to care about the alignment of the parameter
block?
Consumers of the parameter block data might try access data that is
unaligned for a particular CPU. The good news is that the definition of
the parameter blocks ensures the data is u64 aligned.
Thanks,
Matthew Gerlach
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
