Messages in this thread | | | Date | Thu, 12 Jan 2023 07:30:54 -0800 (PST) | From | matthew.gerlach@linux ... | Subject | Re: [PATCH v10 3/4] fpga: dfl: add basic support for DFHv1 |
| |
On Wed, 11 Jan 2023, Xu Yilun wrote:
> On 2023-01-10 at 14:07:16 -0800, matthew.gerlach@linux.intel.com wrote: >> >> >> On Tue, 10 Jan 2023, Andy Shevchenko wrote: >> >>> On Mon, Jan 09, 2023 at 04:30:28PM -0800, matthew.gerlach@linux.intel.com wrote: >>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> >>>> >>>> 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 >>> >>> >>> >
| |