[v4,3/4] fpga: dfl: add basic support DFHv1

Commit Message

Matthew Gerlach Oct. 20, 2022, 9:26 p.m. UTC

From: Matthew Gerlach <[e-mail]>

Add generic support for MSI-X interrupts for DFL devices.

The location of a feature's registers 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.

Signed-off-by: Matthew Gerlach <[e-mail]>
---
v4: s/MSIX/MSI_X
    move kernel doc to implementation
    use structure assignment
    fix decode of absolute address
    clean up comment in parse_feature_irqs
    remove use of csr_res

v3: remove unneeded blank line
    use clearer variable name
    pass finfo into parse_feature_irqs()
    refactor code for better indentation
    use switch statement for irq parsing
    squash in code parsing register location

v2: fix kernel doc
    clarify use of DFH_VERSION field
---
 drivers/fpga/dfl.c  | 234 ++++++++++++++++++++++++++++++++++----------
 drivers/fpga/dfl.h  |   5 +
 include/linux/dfl.h |   4 +
 3 files changed, 194 insertions(+), 49 deletions(-)

Comments

Andy Shevchenko Oct. 20, 2022, 10:07 p.m. UTC | #1

On Thu, Oct 20, 2022 at 02:26:09PM -0700, [e-mail] wrote:
> From: Matthew Gerlach <[e-mail]>
> 
> Add generic support for MSI-X interrupts for DFL devices.
> 
> The location of a feature's registers 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.

...

> +static void *find_param(void *base, resource_size_t max, int param)

Why base can't be u64 * to begin with?

> +{
> +	int off = 0;
> +	u64 v, next;
> +
> +	while (off < max) {

Maybe you need a comment somewhere to tell that the caller guarantees that max
won't provoke OOB accesses.

> +		v = *(u64 *)(base + off);

Okay, if offset is not multiple of at least 4, how do you guarantee no
exception on the architectures with disallowed misaligned accesses?

Making base to be u64 * solves this, but you need to take care to provide
offset in terms of u64 words.

> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> +			return base + off + DFHv1_PARAM_DATA;
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
> +			break;
> +
> +	}
> +
> +	return NULL;
> +}

...

> +		/*
> +		 * DFHv0 only provides mmio resource information for each feature

MMIO

> +		 * in the DFL header.  There is no generic interrupt information.
> +		 * Instead, features with interrupt functionality provide
> +		 * the information in feature specific registers.
> +		 */

...

> +		if (!finfo->param_size)
>  			break;

This is redundant as it's implied by find_param().

> +		p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
> +		if (!p)
>  			break;

...

> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
> +{
> +	int size = 0;
> +	u64 v, next;
> +
> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
> +		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
> +		return 0;
> +
> +	while (size + DFHv1_PARAM_HDR < max) {
> +		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
> +			return -EINVAL;
> +
> +		size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
> +
> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
> +			return size;

These 3 looks like they deserve different fields and hence separate FIELD_GET()
will return exactly what we need without additional masking, right?

> +	}
> +
> +	return -ENOENT;
> +}

...

> +	if (dfh_psize > 0) {

Isn't this implied by memcpy_fromio()? I mean if it's 0, nothing bad will
happen if you call the above directly.

> +		memcpy_fromio(finfo->params,
> +			      binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
> +		finfo->param_size = dfh_psize;
> +	}

...

>  	finfo->mmio_res.flags = IORESOURCE_MEM;
> +	if (dfh_ver == 1) {
> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> +		if (v & DFHv1_CSR_ADDR_REL)
> +			finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
> +		else
> +			finfo->mmio_res.start = binfo->start + ofst +
> +					       FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +
> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> +		finfo->mmio_res.end = finfo->mmio_res.start +
> +				      FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
> +	} else {
> +		finfo->mmio_res.start = binfo->start + ofst;
> +		finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> +	}

You may define

	resource_size_t start, end;

locally and simplify above quite a bit.

...

> +void *dfh_find_param(struct dfl_device *dfl_dev, int param);

+ Blank line.

>  #endif /* __LINUX_DFL_H */

Ilpo Järvinen Oct. 21, 2022, 8:58 a.m. UTC | #2

On Thu, 20 Oct 2022, [e-mail] wrote:

> From: Matthew Gerlach <[e-mail]>
> 
> Add generic support for MSI-X interrupts for DFL devices.
> 
> The location of a feature's registers 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.
> 
> Signed-off-by: Matthew Gerlach <[e-mail]>
> ---
> v4: s/MSIX/MSI_X
>     move kernel doc to implementation
>     use structure assignment
>     fix decode of absolute address
>     clean up comment in parse_feature_irqs
>     remove use of csr_res
> 
> v3: remove unneeded blank line
>     use clearer variable name
>     pass finfo into parse_feature_irqs()
>     refactor code for better indentation
>     use switch statement for irq parsing
>     squash in code parsing register location
> 
> v2: fix kernel doc
>     clarify use of DFH_VERSION field
> ---

> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
> +{
> +	int size = 0;
> +	u64 v, next;
> +
> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
> +		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
> +		return 0;
> +
> +	while (size + DFHv1_PARAM_HDR < max) {
> +		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))

In general, try to not use inverse logic for defining masks. However here, 
just change DFHv1_PARAM_HDR_NEXT_OFFSET to not include any extra bits 
(no rsvd nor eop) and you no longer need this extra masking.

> +			return -EINVAL;
> +
> +		size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;

...Then you can drop this anding too.

> +
> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)

Your docs say EOP, but here you use EOL.

Change DFHv1_PARAM_HDR_NEXT_EOL such that this is extracted directly from 
v.

Ilpo Järvinen Oct. 21, 2022, 9:07 a.m. UTC | #3

On Thu, 20 Oct 2022, [e-mail] wrote:

> From: Matthew Gerlach <[e-mail]>
> 
> Add generic support for MSI-X interrupts for DFL devices.
> 
> The location of a feature's registers 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.
> 
> Signed-off-by: Matthew Gerlach <[e-mail]>
> ---

> @@ -934,56 +962,108 @@ static u16 feature_id(u64 value)
>  	return 0;
>  }
>  
> +static void *find_param(void *base, resource_size_t max, int param)
> +{
> +	int off = 0;
> +	u64 v, next;
> +
> +	while (off < max) {
> +		v = *(u64 *)(base + off);
> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> +			return base + off + DFHv1_PARAM_DATA;
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
> +			break;
> +
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * dfh_find_param() - find data for the given parameter id
> + * @dfl_dev: dfl device
> + * @param: id of dfl parameter
> + *
> + * Return: pointer to parameter data on success, NULL otherwise.
> + */
> +void *dfh_find_param(struct dfl_device *dfl_dev, int param)
> +{
> +	return find_param(dfl_dev->params, dfl_dev->param_size, param);
> +}
> +EXPORT_SYMBOL_GPL(dfh_find_param);

Do you expect this split between dfh_find_param() and find_param() to
be useful in the future? If no other callers are expected, I'd just pull 
find_param() into dfh_find_param() and create local variables for base and 
max.

Matthew Gerlach Oct. 24, 2022, 2:56 p.m. UTC | #4

On Fri, 21 Oct 2022, Andy Shevchenko wrote:

> On Thu, Oct 20, 2022 at 02:26:09PM -0700, [e-mail] wrote:
>> From: Matthew Gerlach <[e-mail]>
>>
>> Add generic support for MSI-X interrupts for DFL devices.
>>
>> The location of a feature's registers 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.
>
> ...
>
>> +static void *find_param(void *base, resource_size_t max, int param)
>
> Why base can't be u64 * to begin with?

It can be u64, and I will consider it for the next iteration.
>
>> +{
>> +	int off = 0;
>> +	u64 v, next;
>> +
>> +	while (off < max) {
>
> Maybe you need a comment somewhere to tell that the caller guarantees that max
> won't provoke OOB accesses.
>
>> +		v = *(u64 *)(base + off);
>
> Okay, if offset is not multiple of at least 4, how do you guarantee no
> exception on the architectures with disallowed misaligned accesses?
>
> Making base to be u64 * solves this, but you need to take care to provide
> offset in terms of u64 words.

The masking of next below ensures that the offset it at least 4 byte 
aligned, but it might make sense to define the next field in terms of 8 
byte words.

>
>> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
>> +			return base + off + DFHv1_PARAM_DATA;
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
>> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
>> +			break;
>> +
>> +	}
>> +
>> +	return NULL;
>> +}
>
> ...
>
>> +		/*
>> +		 * DFHv0 only provides mmio resource information for each feature
>
> MMIO

I'll change mmio to MMIO here and a place in the documentation that I 
noticed.

>
>> +		 * in the DFL header.  There is no generic interrupt information.
>> +		 * Instead, features with interrupt functionality provide
>> +		 * the information in feature specific registers.
>> +		 */
>
> ...
>
>> +		if (!finfo->param_size)
>>  			break;
>
> This is redundant as it's implied by find_param().

I will remove the redundant code.

>
>> +		p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
>> +		if (!p)
>>  			break;
>
> ...
>
>> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
>> +{
>> +	int size = 0;
>> +	u64 v, next;
>> +
>> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
>> +		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
>> +		return 0;
>> +
>> +	while (size + DFHv1_PARAM_HDR < max) {
>> +		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
>> +			return -EINVAL;
>> +
>> +		size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
>> +
>> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
>> +			return size;
>
> These 3 looks like they deserve different fields and hence separate FIELD_GET()
> will return exactly what we need without additional masking, right?

I agree separate FIELD_GET() calls will be cleaner.

>
>> +	}
>> +
>> +	return -ENOENT;
>> +}
>
> ...
>
>> +	if (dfh_psize > 0) {
>
> Isn't this implied by memcpy_fromio()? I mean if it's 0, nothing bad will
> happen if you call the above directly.
>
>> +		memcpy_fromio(finfo->params,
>> +			      binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
>> +		finfo->param_size = dfh_psize;
>> +	}
>
> ...
>
>>  	finfo->mmio_res.flags = IORESOURCE_MEM;
>> +	if (dfh_ver == 1) {
>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
>> +		if (v & DFHv1_CSR_ADDR_REL)
>> +			finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
>> +		else
>> +			finfo->mmio_res.start = binfo->start + ofst +
>> +					       FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
>> +
>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
>> +		finfo->mmio_res.end = finfo->mmio_res.start +
>> +				      FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
>> +	} else {
>> +		finfo->mmio_res.start = binfo->start + ofst;
>> +		finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>> +	}
>
> You may define
>
> 	resource_size_t start, end;
>
> locally and simplify above quite a bit.

That is a good suggestion that should clean up the code quite a bit.

>
> ...
>
>> +void *dfh_find_param(struct dfl_device *dfl_dev, int param);
>
> + Blank line.
>
>>  #endif /* __LINUX_DFL_H */
>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>

Matthew Gerlach Oct. 24, 2022, 3:09 p.m. UTC | #5

On Fri, 21 Oct 2022, Ilpo Järvinen wrote:

> On Thu, 20 Oct 2022, [e-mail] wrote:
>
>> From: Matthew Gerlach <[e-mail]>
>>
>> Add generic support for MSI-X interrupts for DFL devices.
>>
>> The location of a feature's registers 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.
>>
>> Signed-off-by: Matthew Gerlach <[e-mail]>
>> ---
>> v4: s/MSIX/MSI_X
>>     move kernel doc to implementation
>>     use structure assignment
>>     fix decode of absolute address
>>     clean up comment in parse_feature_irqs
>>     remove use of csr_res
>>
>> v3: remove unneeded blank line
>>     use clearer variable name
>>     pass finfo into parse_feature_irqs()
>>     refactor code for better indentation
>>     use switch statement for irq parsing
>>     squash in code parsing register location
>>
>> v2: fix kernel doc
>>     clarify use of DFH_VERSION field
>> ---
>
>> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
>> +{
>> +	int size = 0;
>> +	u64 v, next;
>> +
>> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
>> +		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
>> +		return 0;
>> +
>> +	while (size + DFHv1_PARAM_HDR < max) {
>> +		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
>
> In general, try to not use inverse logic for defining masks. However here,
> just change DFHv1_PARAM_HDR_NEXT_OFFSET to not include any extra bits
> (no rsvd nor eop) and you no longer need this extra masking.

I agree that defining the fields better and using FIELD_GET would make 
this code cleaner.

>
>> +			return -EINVAL;
>> +
>> +		size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
>
> ...Then you can drop this anding too.
>
>> +
>> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
>
> Your docs say EOP, but here you use EOL.

Thanks for catching the inconsistency.

>
> Change DFHv1_PARAM_HDR_NEXT_EOL such that this is extracted directly from
> v.
>
> -- 
> i.
>

Xu Yilun Oct. 29, 2022, 1:08 p.m. UTC | #6

On 2022-10-20 at 14:26:09 -0700, [e-mail] wrote:
> From: Matthew Gerlach <[e-mail]>
> 
> Add generic support for MSI-X interrupts for DFL devices.

The first paragraph of the commit message should be the summary of the
whole change. But this seems one of the changes. Please add the proper
summary at the beginning.

> 
> The location of a feature's registers 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.
> 
> Signed-off-by: Matthew Gerlach <[e-mail]>
> ---
> v4: s/MSIX/MSI_X
>     move kernel doc to implementation
>     use structure assignment
>     fix decode of absolute address
>     clean up comment in parse_feature_irqs
>     remove use of csr_res
> 
> v3: remove unneeded blank line
>     use clearer variable name
>     pass finfo into parse_feature_irqs()
>     refactor code for better indentation
>     use switch statement for irq parsing
>     squash in code parsing register location
> 
> v2: fix kernel doc
>     clarify use of DFH_VERSION field
> ---
>  drivers/fpga/dfl.c  | 234 ++++++++++++++++++++++++++++++++++----------
>  drivers/fpga/dfl.h  |   5 +
>  include/linux/dfl.h |   4 +
>  3 files changed, 194 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index b9aae85ba930..37f995e66436 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -342,6 +342,8 @@ static void release_dfl_dev(struct device *dev)
>  	if (ddev->mmio_res.parent)
>  		release_resource(&ddev->mmio_res);
>  
> +	kfree(ddev->params);
> +
>  	ida_free(&dfl_device_ida, ddev->id);
>  	kfree(ddev->irqs);
>  	kfree(ddev);
> @@ -380,7 +382,16 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
>  	ddev->type = feature_dev_id_type(pdev);
>  	ddev->feature_id = feature->id;
>  	ddev->revision = feature->revision;
> +	ddev->dfh_version = feature->dfh_version;
>  	ddev->cdev = pdata->dfl_cdev;
> +	if (feature->param_size) {
> +		ddev->params = kmemdup(feature->params, feature->param_size, GFP_KERNEL);
> +		if (!ddev->params) {
> +			ret = -ENOMEM;
> +			goto put_dev;
> +		}
> +		ddev->param_size = feature->param_size;
> +	}
>  
>  	/* add mmio resource */
>  	parent_res = &pdev->resource[feature->resource_index];
> @@ -708,20 +719,27 @@ struct build_feature_devs_info {
>   * struct dfl_feature_info - sub feature info collected during feature dev build
>   *
>   * @fid: id of this sub feature.
> + * @revision: revision of this sub feature
> + * @dfh_version: version of Device Feature Header (DFH)
>   * @mmio_res: mmio resource of this sub feature.
>   * @ioaddr: mapped base address of mmio resource.
>   * @node: node in sub_features linked list.
>   * @irq_base: start of irq index in this sub feature.
>   * @nr_irqs: number of irqs of this sub feature.
> + * @param_size: size DFH parameters.
> + * @params: DFH parameter data.
>   */
>  struct dfl_feature_info {
>  	u16 fid;
>  	u8 revision;
> +	u8 dfh_version;
>  	struct resource mmio_res;
>  	void __iomem *ioaddr;
>  	struct list_head node;
>  	unsigned int irq_base;
>  	unsigned int nr_irqs;
> +	unsigned int param_size;
> +	u64 params[];
>  };
>  
>  static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
> @@ -797,7 +815,17 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>  		feature->dev = fdev;
>  		feature->id = finfo->fid;
>  		feature->revision = finfo->revision;
> +		feature->dfh_version = finfo->dfh_version;
>  
> +		if (finfo->param_size) {
> +			feature->params = devm_kmemdup(binfo->dev,
> +						       finfo->params, finfo->param_size,
> +						       GFP_KERNEL);
> +			if (!feature->params)
> +				return -ENOMEM;
> +
> +			feature->param_size = finfo->param_size;
> +		}
>  		/*
>  		 * the FIU header feature has some fundamental functions (sriov
>  		 * set, port enable/disable) needed for the dfl bus device and
> @@ -934,56 +962,108 @@ static u16 feature_id(u64 value)
>  	return 0;
>  }
>  
> +static void *find_param(void *base, resource_size_t max, int param)
> +{
> +	int off = 0;
> +	u64 v, next;
> +
> +	while (off < max) {
> +		v = *(u64 *)(base + off);
> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> +			return base + off + DFHv1_PARAM_DATA;
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
> +			break;
> +
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * dfh_find_param() - find data for the given parameter id
> + * @dfl_dev: dfl device
> + * @param: id of dfl parameter
> + *
> + * Return: pointer to parameter data on success, NULL otherwise.
> + */
> +void *dfh_find_param(struct dfl_device *dfl_dev, int param)
> +{
> +	return find_param(dfl_dev->params, dfl_dev->param_size, param);
> +}
> +EXPORT_SYMBOL_GPL(dfh_find_param);
> +
>  static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> -			      resource_size_t ofst, u16 fid,
> -			      unsigned int *irq_base, unsigned int *nr_irqs)
> +			      resource_size_t ofst, struct dfl_feature_info *finfo)
>  {
>  	void __iomem *base = binfo->ioaddr + ofst;
>  	unsigned int i, ibase, inr = 0;
> +	void *params = finfo->params;
>  	enum dfl_id_type type;
> +	u16 fid = finfo->fid;
>  	int virq;
> +	u32 *p;
>  	u64 v;
>  
> -	type = feature_dev_id_type(binfo->feature_dev);
> +	switch (finfo->dfh_version) {
> +	case 0:
> +		/*
> +		 * DFHv0 only provides mmio resource information for each feature
> +		 * in the DFL header.  There is no generic interrupt information.
> +		 * Instead, features with interrupt functionality provide
> +		 * the information in feature specific registers.
> +		 */
> +		type = feature_dev_id_type(binfo->feature_dev);
> +		if (type == PORT_ID) {
> +			switch (fid) {
> +			case PORT_FEATURE_ID_UINT:
> +				v = readq(base + PORT_UINT_CAP);
> +				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> +				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> +				break;
> +			case PORT_FEATURE_ID_ERROR:
> +				v = readq(base + PORT_ERROR_CAP);
> +				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> +				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> +				break;
> +			}
> +		} else if (type == FME_ID) {
> +			switch (fid) {
> +			case FME_FEATURE_ID_GLOBAL_ERR:
> +				v = readq(base + FME_ERROR_CAP);
> +				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> +				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> +				break;
> +			}
> +		}
> +		break;
>  
> -	/*
> -	 * Ideally DFL framework should only read info from DFL header, but
> -	 * current version DFL only provides mmio resources information for
> -	 * each feature in DFL Header, no field for interrupt resources.
> -	 * Interrupt resource information is provided by specific mmio
> -	 * registers of each private feature which supports interrupt. So in
> -	 * order to parse and assign irq resources, DFL framework has to look
> -	 * into specific capability registers of these private features.
> -	 *
> -	 * Once future DFL version supports generic interrupt resource
> -	 * information in common DFL headers, the generic interrupt parsing
> -	 * code will be added. But in order to be compatible to old version
> -	 * DFL, the driver may still fall back to these quirks.
> -	 */
> -	if (type == PORT_ID) {
> -		switch (fid) {
> -		case PORT_FEATURE_ID_UINT:
> -			v = readq(base + PORT_UINT_CAP);
> -			ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> -			inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> +	case 1:
> +		/*
> +		 * DFHv1 provides interrupt resource information in DFHv1
> +		 * parameter blocks.
> +		 */
> +		if (!finfo->param_size)
>  			break;
> -		case PORT_FEATURE_ID_ERROR:
> -			v = readq(base + PORT_ERROR_CAP);
> -			ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> -			inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> +
> +		p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
> +		if (!p)
>  			break;
> -		}
> -	} else if (type == FME_ID) {
> -		if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
> -			v = readq(base + FME_ERROR_CAP);
> -			ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> -			inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> -		}
> +
> +		ibase = *p++;
> +		inr = *p;
> +		break;
> +
> +	default:
> +		dev_warn(binfo->dev, "unexpected DFH version %d\n", finfo->dfh_version);
> +		break;
>  	}
>  
>  	if (!inr) {
> -		*irq_base = 0;
> -		*nr_irqs = 0;
> +		finfo->irq_base = 0;
> +		finfo->nr_irqs = 0;
>  		return 0;
>  	}
>  
> @@ -1006,12 +1086,37 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>  		}
>  	}
>  
> -	*irq_base = ibase;
> -	*nr_irqs = inr;
> +	finfo->irq_base = ibase;
> +	finfo->nr_irqs = inr;
>  
>  	return 0;
>  }
>  
> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
> +{
> +	int size = 0;
> +	u64 v, next;
> +
> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
> +		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
> +		return 0;
> +
> +	while (size + DFHv1_PARAM_HDR < max) {
> +		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
> +			return -EINVAL;
> +
> +		size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
> +
> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
> +			return size;
> +	}
> +
> +	return -ENOENT;
> +}
> +
>  /*
>   * when create sub feature instances, for private features, it doesn't need
>   * to provide resource size and feature id as they could be read from DFH
> @@ -1023,39 +1128,70 @@ static int
>  create_feature_instance(struct build_feature_devs_info *binfo,
>  			resource_size_t ofst, resource_size_t size, u16 fid)
>  {
> -	unsigned int irq_base, nr_irqs;
>  	struct dfl_feature_info *finfo;
> +	int dfh_psize = 0;
>  	u8 revision = 0;
> +	u8 dfh_ver = 0;
>  	int ret;
>  	u64 v;
>  
>  	if (fid != FEATURE_ID_AFU) {
>  		v = readq(binfo->ioaddr + ofst);
>  		revision = FIELD_GET(DFH_REVISION, v);
> -
> +		dfh_ver = FIELD_GET(DFH_VERSION, v);
>  		/* read feature size and id if inputs are invalid */
>  		size = size ? size : feature_size(v);
>  		fid = fid ? fid : feature_id(v);
> +		if (dfh_ver == 1) {
> +			dfh_psize = dfh_get_psize(binfo->ioaddr + ofst, size);
> +			if (dfh_psize < 0) {
> +				dev_err(binfo->dev,
> +					"failed to read size of DFHv1 parameters %d\n",
> +					dfh_psize);
> +				return dfh_psize;
> +			}
> +			dev_dbg(binfo->dev, "dfhv1_psize %d\n", dfh_psize);
> +		}
>  	}
>  
>  	if (binfo->len - ofst < size)
>  		return -EINVAL;
>  
> -	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
> -	if (ret)
> -		return ret;
> -
> -	finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
> +	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);

The u64 flexible array in the structure, but seems dfh_get_psize could
not garantee 64bit aligned size.

What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
u32[].

Thanks,
Yilun

>  	if (!finfo)
>  		return -ENOMEM;
>  
> +	if (dfh_psize > 0) {
> +		memcpy_fromio(finfo->params,
> +			      binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
> +		finfo->param_size = dfh_psize;
> +	}
> +
>  	finfo->fid = fid;
>  	finfo->revision = revision;
> -	finfo->mmio_res.start = binfo->start + ofst;
> -	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> +	finfo->dfh_version = dfh_ver;
>  	finfo->mmio_res.flags = IORESOURCE_MEM;
> -	finfo->irq_base = irq_base;
> -	finfo->nr_irqs = nr_irqs;
> +	if (dfh_ver == 1) {
> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> +		if (v & DFHv1_CSR_ADDR_REL)
> +			finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
> +		else
> +			finfo->mmio_res.start = binfo->start + ofst +
> +					       FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +
> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> +		finfo->mmio_res.end = finfo->mmio_res.start +
> +				      FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;

So for dflv1, no feature header resource for dfl_device, is it a problem
for dfl_uio? Does userspace driver need the raw feature header?

> +	} else {
> +		finfo->mmio_res.start = binfo->start + ofst;
> +		finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> +	}
> +
> +	ret = parse_feature_irqs(binfo, ofst, finfo);
> +	if (ret) {
> +		kfree(finfo);
> +		return ret;
> +	}
>  
>  	list_add_tail(&finfo->node, &binfo->sub_features);
>  	binfo->feature_num++;
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 45e6e1359a67..1ea7f40c1af6 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -273,11 +273,14 @@ struct dfl_feature_irq_ctx {
>   * @ops: ops of this sub feature.
>   * @ddev: ptr to the dfl device of this sub feature.
>   * @priv: priv data of this feature.
> + * @param_size: size of dfh parameters
> + * @params: point to memory copy of dfh parameters
>   */
>  struct dfl_feature {
>  	struct platform_device *dev;
>  	u16 id;
>  	u8 revision;
> +	u8 dfh_version;
>  	int resource_index;
>  	void __iomem *ioaddr;
>  	struct dfl_feature_irq_ctx *irq_ctx;
> @@ -285,6 +288,8 @@ struct dfl_feature {
>  	const struct dfl_feature_ops *ops;
>  	struct dfl_device *ddev;
>  	void *priv;
> +	unsigned int param_size;
> +	void *params;
>  };
>  
>  #define FEATURE_DEV_ID_UNUSED	(-1)
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index fea9e16d35b6..d00787e870b7 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -50,9 +50,12 @@ struct dfl_device {
>  	u16 type;
>  	u16 feature_id;
>  	u8 revision;
> +	u8 dfh_version;
>  	struct resource mmio_res;
>  	int *irqs;
>  	unsigned int num_irqs;
> +	unsigned int param_size;
> +	void *params;
>  	struct dfl_fpga_cdev *cdev;
>  	const struct dfl_device_id *id_entry;
>  };
> @@ -95,4 +98,5 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
>  	module_driver(__dfl_driver, dfl_driver_register, \
>  		      dfl_driver_unregister)
>  
> +void *dfh_find_param(struct dfl_device *dfl_dev, int param);

int param_id is better?

Thanks,
Yilun

>  #endif /* __LINUX_DFL_H */
> -- 
> 2.25.1
>

Matthew Gerlach Oct. 29, 2022, 2:47 p.m. UTC | #7

On Sat, 29 Oct 2022, Xu Yilun wrote:

> On 2022-10-20 at 14:26:09 -0700, [e-mail] wrote:
>> From: Matthew Gerlach <[e-mail]>
>>
>> Add generic support for MSI-X interrupts for DFL devices.
>
> The first paragraph of the commit message should be the summary of the
> whole change. But this seems one of the changes. Please add the proper
> summary at the beginning.

This is a very good suggestion.  It will be addressed in the next 
revision.

>
>>
>> The location of a feature's registers 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.
>>
>> Signed-off-by: Matthew Gerlach <[e-mail]>
>> ---
>> v4: s/MSIX/MSI_X
>>     move kernel doc to implementation
>>     use structure assignment
>>     fix decode of absolute address
>>     clean up comment in parse_feature_irqs
>>     remove use of csr_res
>>
>> v3: remove unneeded blank line
>>     use clearer variable name
>>     pass finfo into parse_feature_irqs()
>>     refactor code for better indentation
>>     use switch statement for irq parsing
>>     squash in code parsing register location
>>
>> v2: fix kernel doc
>>     clarify use of DFH_VERSION field
>> ---
>>  drivers/fpga/dfl.c  | 234 ++++++++++++++++++++++++++++++++++----------
>>  drivers/fpga/dfl.h  |   5 +
>>  include/linux/dfl.h |   4 +
>>  3 files changed, 194 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>> index b9aae85ba930..37f995e66436 100644
>> --- a/drivers/fpga/dfl.c
>> +++ b/drivers/fpga/dfl.c
>> @@ -342,6 +342,8 @@ static void release_dfl_dev(struct device *dev)
>>  	if (ddev->mmio_res.parent)
>>  		release_resource(&ddev->mmio_res);
>>
>> +	kfree(ddev->params);
>> +
>>  	ida_free(&dfl_device_ida, ddev->id);
>>  	kfree(ddev->irqs);
>>  	kfree(ddev);
>> @@ -380,7 +382,16 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
>>  	ddev->type = feature_dev_id_type(pdev);
>>  	ddev->feature_id = feature->id;
>>  	ddev->revision = feature->revision;
>> +	ddev->dfh_version = feature->dfh_version;
>>  	ddev->cdev = pdata->dfl_cdev;
>> +	if (feature->param_size) {
>> +		ddev->params = kmemdup(feature->params, feature->param_size, GFP_KERNEL);
>> +		if (!ddev->params) {
>> +			ret = -ENOMEM;
>> +			goto put_dev;
>> +		}
>> +		ddev->param_size = feature->param_size;
>> +	}
>>
>>  	/* add mmio resource */
>>  	parent_res = &pdev->resource[feature->resource_index];
>> @@ -708,20 +719,27 @@ struct build_feature_devs_info {
>>   * struct dfl_feature_info - sub feature info collected during feature dev build
>>   *
>>   * @fid: id of this sub feature.
>> + * @revision: revision of this sub feature
>> + * @dfh_version: version of Device Feature Header (DFH)
>>   * @mmio_res: mmio resource of this sub feature.
>>   * @ioaddr: mapped base address of mmio resource.
>>   * @node: node in sub_features linked list.
>>   * @irq_base: start of irq index in this sub feature.
>>   * @nr_irqs: number of irqs of this sub feature.
>> + * @param_size: size DFH parameters.
>> + * @params: DFH parameter data.
>>   */
>>  struct dfl_feature_info {
>>  	u16 fid;
>>  	u8 revision;
>> +	u8 dfh_version;
>>  	struct resource mmio_res;
>>  	void __iomem *ioaddr;
>>  	struct list_head node;
>>  	unsigned int irq_base;
>>  	unsigned int nr_irqs;
>> +	unsigned int param_size;
>> +	u64 params[];
>>  };
>>
>>  static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
>> @@ -797,7 +815,17 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>>  		feature->dev = fdev;
>>  		feature->id = finfo->fid;
>>  		feature->revision = finfo->revision;
>> +		feature->dfh_version = finfo->dfh_version;
>>
>> +		if (finfo->param_size) {
>> +			feature->params = devm_kmemdup(binfo->dev,
>> +						       finfo->params, finfo->param_size,
>> +						       GFP_KERNEL);
>> +			if (!feature->params)
>> +				return -ENOMEM;
>> +
>> +			feature->param_size = finfo->param_size;
>> +		}
>>  		/*
>>  		 * the FIU header feature has some fundamental functions (sriov
>>  		 * set, port enable/disable) needed for the dfl bus device and
>> @@ -934,56 +962,108 @@ static u16 feature_id(u64 value)
>>  	return 0;
>>  }
>>
>> +static void *find_param(void *base, resource_size_t max, int param)
>> +{
>> +	int off = 0;
>> +	u64 v, next;
>> +
>> +	while (off < max) {
>> +		v = *(u64 *)(base + off);
>> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
>> +			return base + off + DFHv1_PARAM_DATA;
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
>> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
>> +			break;
>> +
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * dfh_find_param() - find data for the given parameter id
>> + * @dfl_dev: dfl device
>> + * @param: id of dfl parameter
>> + *
>> + * Return: pointer to parameter data on success, NULL otherwise.
>> + */
>> +void *dfh_find_param(struct dfl_device *dfl_dev, int param)
>> +{
>> +	return find_param(dfl_dev->params, dfl_dev->param_size, param);
>> +}
>> +EXPORT_SYMBOL_GPL(dfh_find_param);
>> +
>>  static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>> -			      resource_size_t ofst, u16 fid,
>> -			      unsigned int *irq_base, unsigned int *nr_irqs)
>> +			      resource_size_t ofst, struct dfl_feature_info *finfo)
>>  {
>>  	void __iomem *base = binfo->ioaddr + ofst;
>>  	unsigned int i, ibase, inr = 0;
>> +	void *params = finfo->params;
>>  	enum dfl_id_type type;
>> +	u16 fid = finfo->fid;
>>  	int virq;
>> +	u32 *p;
>>  	u64 v;
>>
>> -	type = feature_dev_id_type(binfo->feature_dev);
>> +	switch (finfo->dfh_version) {
>> +	case 0:
>> +		/*
>> +		 * DFHv0 only provides mmio resource information for each feature
>> +		 * in the DFL header.  There is no generic interrupt information.
>> +		 * Instead, features with interrupt functionality provide
>> +		 * the information in feature specific registers.
>> +		 */
>> +		type = feature_dev_id_type(binfo->feature_dev);
>> +		if (type == PORT_ID) {
>> +			switch (fid) {
>> +			case PORT_FEATURE_ID_UINT:
>> +				v = readq(base + PORT_UINT_CAP);
>> +				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>> +				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>> +				break;
>> +			case PORT_FEATURE_ID_ERROR:
>> +				v = readq(base + PORT_ERROR_CAP);
>> +				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
>> +				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>> +				break;
>> +			}
>> +		} else if (type == FME_ID) {
>> +			switch (fid) {
>> +			case FME_FEATURE_ID_GLOBAL_ERR:
>> +				v = readq(base + FME_ERROR_CAP);
>> +				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>> +				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>> +				break;
>> +			}
>> +		}
>> +		break;
>>
>> -	/*
>> -	 * Ideally DFL framework should only read info from DFL header, but
>> -	 * current version DFL only provides mmio resources information for
>> -	 * each feature in DFL Header, no field for interrupt resources.
>> -	 * Interrupt resource information is provided by specific mmio
>> -	 * registers of each private feature which supports interrupt. So in
>> -	 * order to parse and assign irq resources, DFL framework has to look
>> -	 * into specific capability registers of these private features.
>> -	 *
>> -	 * Once future DFL version supports generic interrupt resource
>> -	 * information in common DFL headers, the generic interrupt parsing
>> -	 * code will be added. But in order to be compatible to old version
>> -	 * DFL, the driver may still fall back to these quirks.
>> -	 */
>> -	if (type == PORT_ID) {
>> -		switch (fid) {
>> -		case PORT_FEATURE_ID_UINT:
>> -			v = readq(base + PORT_UINT_CAP);
>> -			ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>> -			inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>> +	case 1:
>> +		/*
>> +		 * DFHv1 provides interrupt resource information in DFHv1
>> +		 * parameter blocks.
>> +		 */
>> +		if (!finfo->param_size)
>>  			break;
>> -		case PORT_FEATURE_ID_ERROR:
>> -			v = readq(base + PORT_ERROR_CAP);
>> -			ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
>> -			inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>> +
>> +		p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
>> +		if (!p)
>>  			break;
>> -		}
>> -	} else if (type == FME_ID) {
>> -		if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
>> -			v = readq(base + FME_ERROR_CAP);
>> -			ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>> -			inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>> -		}
>> +
>> +		ibase = *p++;
>> +		inr = *p;
>> +		break;
>> +
>> +	default:
>> +		dev_warn(binfo->dev, "unexpected DFH version %d\n", finfo->dfh_version);
>> +		break;
>>  	}
>>
>>  	if (!inr) {
>> -		*irq_base = 0;
>> -		*nr_irqs = 0;
>> +		finfo->irq_base = 0;
>> +		finfo->nr_irqs = 0;
>>  		return 0;
>>  	}
>>
>> @@ -1006,12 +1086,37 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>>  		}
>>  	}
>>
>> -	*irq_base = ibase;
>> -	*nr_irqs = inr;
>> +	finfo->irq_base = ibase;
>> +	finfo->nr_irqs = inr;
>>
>>  	return 0;
>>  }
>>
>> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
>> +{
>> +	int size = 0;
>> +	u64 v, next;
>> +
>> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
>> +		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
>> +		return 0;
>> +
>> +	while (size + DFHv1_PARAM_HDR < max) {
>> +		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
>> +			return -EINVAL;
>> +
>> +		size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
>> +
>> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
>> +			return size;
>> +	}
>> +
>> +	return -ENOENT;
>> +}
>> +
>>  /*
>>   * when create sub feature instances, for private features, it doesn't need
>>   * to provide resource size and feature id as they could be read from DFH
>> @@ -1023,39 +1128,70 @@ static int
>>  create_feature_instance(struct build_feature_devs_info *binfo,
>>  			resource_size_t ofst, resource_size_t size, u16 fid)
>>  {
>> -	unsigned int irq_base, nr_irqs;
>>  	struct dfl_feature_info *finfo;
>> +	int dfh_psize = 0;
>>  	u8 revision = 0;
>> +	u8 dfh_ver = 0;
>>  	int ret;
>>  	u64 v;
>>
>>  	if (fid != FEATURE_ID_AFU) {
>>  		v = readq(binfo->ioaddr + ofst);
>>  		revision = FIELD_GET(DFH_REVISION, v);
>> -
>> +		dfh_ver = FIELD_GET(DFH_VERSION, v);
>>  		/* read feature size and id if inputs are invalid */
>>  		size = size ? size : feature_size(v);
>>  		fid = fid ? fid : feature_id(v);
>> +		if (dfh_ver == 1) {
>> +			dfh_psize = dfh_get_psize(binfo->ioaddr + ofst, size);
>> +			if (dfh_psize < 0) {
>> +				dev_err(binfo->dev,
>> +					"failed to read size of DFHv1 parameters %d\n",
>> +					dfh_psize);
>> +				return dfh_psize;
>> +			}
>> +			dev_dbg(binfo->dev, "dfhv1_psize %d\n", dfh_psize);
>> +		}
>>  	}
>>
>>  	if (binfo->len - ofst < size)
>>  		return -EINVAL;
>>
>> -	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
>> -	if (ret)
>> -		return ret;
>> -
>> -	finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
>> +	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
>
> The u64 flexible array in the structure, but seems dfh_get_psize could
> not garantee 64bit aligned size.
>
> What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
> of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
> u32[].
>
> Thanks,
> Yilun

The mandatory alignment alignment is 64.  The documentation and field 
definitions will be updated accordingly in the next revision.

>
>>  	if (!finfo)
>>  		return -ENOMEM;
>>
>> +	if (dfh_psize > 0) {
>> +		memcpy_fromio(finfo->params,
>> +			      binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
>> +		finfo->param_size = dfh_psize;
>> +	}
>> +
>>  	finfo->fid = fid;
>>  	finfo->revision = revision;
>> -	finfo->mmio_res.start = binfo->start + ofst;
>> -	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>> +	finfo->dfh_version = dfh_ver;
>>  	finfo->mmio_res.flags = IORESOURCE_MEM;
>> -	finfo->irq_base = irq_base;
>> -	finfo->nr_irqs = nr_irqs;
>> +	if (dfh_ver == 1) {
>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
>> +		if (v & DFHv1_CSR_ADDR_REL)
>> +			finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
>> +		else
>> +			finfo->mmio_res.start = binfo->start + ofst +
>> +					       FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
>> +
>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
>> +		finfo->mmio_res.end = finfo->mmio_res.start +
>> +				      FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
>
> So for dflv1, no feature header resource for dfl_device, is it a problem
> for dfl_uio? Does userspace driver need the raw feature header?
These are two very good questions.  The dfl_uio driver question is 
particularly relevent because user space is looking at the GUIDs.

>
>> +	} else {
>> +		finfo->mmio_res.start = binfo->start + ofst;
>> +		finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>> +	}
>> +
>> +	ret = parse_feature_irqs(binfo, ofst, finfo);
>> +	if (ret) {
>> +		kfree(finfo);
>> +		return ret;
>> +	}
>>
>>  	list_add_tail(&finfo->node, &binfo->sub_features);
>>  	binfo->feature_num++;
>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>> index 45e6e1359a67..1ea7f40c1af6 100644
>> --- a/drivers/fpga/dfl.h
>> +++ b/drivers/fpga/dfl.h
>> @@ -273,11 +273,14 @@ struct dfl_feature_irq_ctx {
>>   * @ops: ops of this sub feature.
>>   * @ddev: ptr to the dfl device of this sub feature.
>>   * @priv: priv data of this feature.
>> + * @param_size: size of dfh parameters
>> + * @params: point to memory copy of dfh parameters
>>   */
>>  struct dfl_feature {
>>  	struct platform_device *dev;
>>  	u16 id;
>>  	u8 revision;
>> +	u8 dfh_version;
>>  	int resource_index;
>>  	void __iomem *ioaddr;
>>  	struct dfl_feature_irq_ctx *irq_ctx;
>> @@ -285,6 +288,8 @@ struct dfl_feature {
>>  	const struct dfl_feature_ops *ops;
>>  	struct dfl_device *ddev;
>>  	void *priv;
>> +	unsigned int param_size;
>> +	void *params;
>>  };
>>
>>  #define FEATURE_DEV_ID_UNUSED	(-1)
>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>> index fea9e16d35b6..d00787e870b7 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -50,9 +50,12 @@ struct dfl_device {
>>  	u16 type;
>>  	u16 feature_id;
>>  	u8 revision;
>> +	u8 dfh_version;
>>  	struct resource mmio_res;
>>  	int *irqs;
>>  	unsigned int num_irqs;
>> +	unsigned int param_size;
>> +	void *params;
>>  	struct dfl_fpga_cdev *cdev;
>>  	const struct dfl_device_id *id_entry;
>>  };
>> @@ -95,4 +98,5 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
>>  	module_driver(__dfl_driver, dfl_driver_register, \
>>  		      dfl_driver_unregister)
>>
>> +void *dfh_find_param(struct dfl_device *dfl_dev, int param);
>
> int param_id is better?

param_id would be better.


>
> Thanks,
> Yilun
>
>>  #endif /* __LINUX_DFL_H */
>> --
>> 2.25.1
>>
>

Andy Shevchenko Oct. 30, 2022, 10:06 p.m. UTC | #8

On Sat, Oct 29, 2022 at 09:08:44PM +0800, Xu Yilun wrote:
> On 2022-10-20 at 14:26:09 -0700, [e-mail] wrote:

> >  struct dfl_feature_info {
> >  	u16 fid;
> >  	u8 revision;
> > +	u8 dfh_version;
> >  	struct resource mmio_res;
> >  	void __iomem *ioaddr;
> >  	struct list_head node;
> >  	unsigned int irq_base;
> >  	unsigned int nr_irqs;
> > +	unsigned int param_size;
> > +	u64 params[];
> >  };

...

> > +	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);


This probably may use something from overflow.h.

> The u64 flexible array in the structure, but seems dfh_get_psize could
> not garantee 64bit aligned size.
> 
> What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
> of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
> u32[].

Isn't it guaranteed by the C standard / architecture ABI?

Xu Yilun Oct. 31, 2022, 1:16 a.m. UTC | #9

On 2022-10-31 at 00:06:28 +0200, Andy Shevchenko wrote:
> On Sat, Oct 29, 2022 at 09:08:44PM +0800, Xu Yilun wrote:
> > On 2022-10-20 at 14:26:09 -0700, [e-mail] wrote:
> 
> > >  struct dfl_feature_info {
> > >  	u16 fid;
> > >  	u8 revision;
> > > +	u8 dfh_version;
> > >  	struct resource mmio_res;
> > >  	void __iomem *ioaddr;
> > >  	struct list_head node;
> > >  	unsigned int irq_base;
> > >  	unsigned int nr_irqs;
> > > +	unsigned int param_size;
> > > +	u64 params[];
> > >  };
> 
> ...
> 
> > > +	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
> 
> 
> This probably may use something from overflow.h.
> 
> > The u64 flexible array in the structure, but seems dfh_get_psize could
> > not garantee 64bit aligned size.
> > 
> > What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
> > of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
> > u32[].
> 
> Isn't it guaranteed by the C standard / architecture ABI?

I'm referring to the malloc size of the structure. It reserved dfh_psize
bytes for this u64 array, but there is no garantee dfh_psize should be a
multiple of 8. So there may be memory leak when accessing the last
array element?

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Andy Shevchenko Oct. 31, 2022, 3:34 p.m. UTC | #10

On Mon, Oct 31, 2022 at 09:16:19AM +0800, Xu Yilun wrote:
> On 2022-10-31 at 00:06:28 +0200, Andy Shevchenko wrote:
> > On Sat, Oct 29, 2022 at 09:08:44PM +0800, Xu Yilun wrote:
> > > On 2022-10-20 at 14:26:09 -0700, [e-mail] wrote:
> > 
> > > >  struct dfl_feature_info {
> > > >  	u16 fid;
> > > >  	u8 revision;
> > > > +	u8 dfh_version;
> > > >  	struct resource mmio_res;
> > > >  	void __iomem *ioaddr;
> > > >  	struct list_head node;
> > > >  	unsigned int irq_base;
> > > >  	unsigned int nr_irqs;
> > > > +	unsigned int param_size;
> > > > +	u64 params[];
> > > >  };
> > 
> > ...
> > 
> > > > +	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
> > 
> > 
> > This probably may use something from overflow.h.
> > 
> > > The u64 flexible array in the structure, but seems dfh_get_psize could
> > > not garantee 64bit aligned size.
> > > 
> > > What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
> > > of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
> > > u32[].
> > 
> > Isn't it guaranteed by the C standard / architecture ABI?
> 
> I'm referring to the malloc size of the structure. It reserved dfh_psize
> bytes for this u64 array, but there is no garantee dfh_psize should be a
> multiple of 8. So there may be memory leak when accessing the last
> array element?

Have you looked at macros in the overflow.h? Would the use of it solve your
concern?

Matthew Gerlach Oct. 31, 2022, 8:15 p.m. UTC | #11

On Mon, 31 Oct 2022, Andy Shevchenko wrote:

> On Mon, Oct 31, 2022 at 09:16:19AM +0800, Xu Yilun wrote:
>> On 2022-10-31 at 00:06:28 +0200, Andy Shevchenko wrote:
>>> On Sat, Oct 29, 2022 at 09:08:44PM +0800, Xu Yilun wrote:
>>>> On 2022-10-20 at 14:26:09 -0700, [e-mail] wrote:
>>>
>>>>>  struct dfl_feature_info {
>>>>>  	u16 fid;
>>>>>  	u8 revision;
>>>>> +	u8 dfh_version;
>>>>>  	struct resource mmio_res;
>>>>>  	void __iomem *ioaddr;
>>>>>  	struct list_head node;
>>>>>  	unsigned int irq_base;
>>>>>  	unsigned int nr_irqs;
>>>>> +	unsigned int param_size;
>>>>> +	u64 params[];
>>>>>  };
>>>
>>> ...
>>>
>>>>> +	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
>>>
>>>
>>> This probably may use something from overflow.h.
>>>
>>>> The u64 flexible array in the structure, but seems dfh_get_psize could
>>>> not garantee 64bit aligned size.
>>>>
>>>> What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
>>>> of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
>>>> u32[].
>>>
>>> Isn't it guaranteed by the C standard / architecture ABI?
>>
>> I'm referring to the malloc size of the structure. It reserved dfh_psize
>> bytes for this u64 array, but there is no garantee dfh_psize should be a
>> multiple of 8. So there may be memory leak when accessing the last
>> array element?
>
> Have you looked at macros in the overflow.h? Would the use of it solve your
> concern?

By clarifying the definition of the next field in the parameter header 
as the number of 8-byte words, dfh_get_psize is guaranteed to be a multiple of 8.
This is fixed in the next revision of patches.

Matthew Gerlach

>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>

Xu Yilun Nov. 1, 2022, 1:55 a.m. UTC | #12

On 2022-10-31 at 17:34:20 +0200, Andy Shevchenko wrote:
> On Mon, Oct 31, 2022 at 09:16:19AM +0800, Xu Yilun wrote:
> > On 2022-10-31 at 00:06:28 +0200, Andy Shevchenko wrote:
> > > On Sat, Oct 29, 2022 at 09:08:44PM +0800, Xu Yilun wrote:
> > > > On 2022-10-20 at 14:26:09 -0700, [e-mail] wrote:
> > > 
> > > > >  struct dfl_feature_info {
> > > > >  	u16 fid;
> > > > >  	u8 revision;
> > > > > +	u8 dfh_version;
> > > > >  	struct resource mmio_res;
> > > > >  	void __iomem *ioaddr;
> > > > >  	struct list_head node;
> > > > >  	unsigned int irq_base;
> > > > >  	unsigned int nr_irqs;
> > > > > +	unsigned int param_size;
> > > > > +	u64 params[];
> > > > >  };
> > > 
> > > ...
> > > 
> > > > > +	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
> > > 
> > > 
> > > This probably may use something from overflow.h.
> > > 
> > > > The u64 flexible array in the structure, but seems dfh_get_psize could
> > > > not garantee 64bit aligned size.
> > > > 
> > > > What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
> > > > of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
> > > > u32[].
> > > 
> > > Isn't it guaranteed by the C standard / architecture ABI?
> > 
> > I'm referring to the malloc size of the structure. It reserved dfh_psize
> > bytes for this u64 array, but there is no garantee dfh_psize should be a
> > multiple of 8. So there may be memory leak when accessing the last
> > array element?
> 
> Have you looked at macros in the overflow.h? Would the use of it solve your
> concern?

Yes, struct_size() or array_size() specifies the element size & count,
which solve the concern at the root.

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Matthew Gerlach Nov. 1, 2022, 10:37 p.m. UTC | #13

On Sat, 29 Oct 2022, [e-mail] wrote:

>
>>
>>>  	if (!finfo)
>>>  		return -ENOMEM;
>>> 
>>> +	if (dfh_psize > 0) {
>>> +		memcpy_fromio(finfo->params,
>>> +			      binfo->ioaddr + ofst + DFHv1_PARAM_HDR, 
>>> dfh_psize);
>>> +		finfo->param_size = dfh_psize;
>>> +	}
>>> +
>>>  	finfo->fid = fid;
>>>  	finfo->revision = revision;
>>> -	finfo->mmio_res.start = binfo->start + ofst;
>>> -	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>>> +	finfo->dfh_version = dfh_ver;
>>>  	finfo->mmio_res.flags = IORESOURCE_MEM;
>>> -	finfo->irq_base = irq_base;
>>> -	finfo->nr_irqs = nr_irqs;
>>> +	if (dfh_ver == 1) {
>>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
>>> +		if (v & DFHv1_CSR_ADDR_REL)
>>> +			finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
>>> +		else
>>> +			finfo->mmio_res.start = binfo->start + ofst +
>>> +					       FIELD_GET(DFHv1_CSR_ADDR_MASK, 
>>> v);
>>> +
>>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
>>> +		finfo->mmio_res.end = finfo->mmio_res.start +
>>> +				      FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 
>>> 1;
>> 
>> So for dflv1, no feature header resource for dfl_device, is it a problem
>> for dfl_uio? Does userspace driver need the raw feature header?
> These are two very good questions.  The dfl_uio driver question is 
> particularly relevent because user space is looking at the GUIDs.
>

In the case of dfl_uio driver, user space will definitely want to look at 
the feature header for the GUID and the parameters.  Since DFHv1 can have 
the DFH header and the feature registers in non-contiguous memory 
locations, a resource for the dfl_device will be required.  In earlier
revisions of this patch set, a second resource was added called csr_res 
pointing to the feature's register while mmio_res pointed at the header.
Do we just need better names or do we need an array of named resources?

>> 
>>> +	} else {
>>> +		finfo->mmio_res.start = binfo->start + ofst;
>>> +		finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>>> +	}
>>> +
>>> +	ret = parse_feature_irqs(binfo, ofst, finfo);
>>> +	if (ret) {
>>> +		kfree(finfo);
>>> +		return ret;
>>> +	}
>>>

Xu Yilun Nov. 3, 2022, 1:36 a.m. UTC | #14

On 2022-11-01 at 15:37:19 -0700, [e-mail] wrote:
> 
> 
> On Sat, 29 Oct 2022, [e-mail] wrote:
> 
> > 
> > > 
> > > >  	if (!finfo)
> > > >  		return -ENOMEM;
> > > > 
> > > > +	if (dfh_psize > 0) {
> > > > +		memcpy_fromio(finfo->params,
> > > > +			      binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
> > > > +		finfo->param_size = dfh_psize;
> > > > +	}
> > > > +
> > > >  	finfo->fid = fid;
> > > >  	finfo->revision = revision;
> > > > -	finfo->mmio_res.start = binfo->start + ofst;
> > > > -	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> > > > +	finfo->dfh_version = dfh_ver;
> > > >  	finfo->mmio_res.flags = IORESOURCE_MEM;
> > > > -	finfo->irq_base = irq_base;
> > > > -	finfo->nr_irqs = nr_irqs;
> > > > +	if (dfh_ver == 1) {
> > > > +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> > > > +		if (v & DFHv1_CSR_ADDR_REL)
> > > > +			finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
> > > > +		else
> > > > +			finfo->mmio_res.start = binfo->start + ofst +
> > > > +					       FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> > > > +
> > > > +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> > > > +		finfo->mmio_res.end = finfo->mmio_res.start +
> > > > +				      FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
> > > 
> > > So for dflv1, no feature header resource for dfl_device, is it a problem
> > > for dfl_uio? Does userspace driver need the raw feature header?
> > These are two very good questions.  The dfl_uio driver question is
> > particularly relevent because user space is looking at the GUIDs.
> > 
> 
> In the case of dfl_uio driver, user space will definitely want to look at
> the feature header for the GUID and the parameters.  Since DFHv1 can have
> the DFH header and the feature registers in non-contiguous memory locations,
> a resource for the dfl_device will be required.  In earlier
> revisions of this patch set, a second resource was added called csr_res
> pointing to the feature's register while mmio_res pointed at the header.
> Do we just need better names or do we need an array of named resources?

Either is OK, you could also name a resource element in an array by
struct resource:name. But my concern is still no overlapping.

Thanks,
Yilun

Patch

diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index b9aae85ba930..37f995e66436 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -342,6 +342,8 @@  static void release_dfl_dev(struct device *dev)
 	if (ddev->mmio_res.parent)
 		release_resource(&ddev->mmio_res);
 
+	kfree(ddev->params);
+
 	ida_free(&dfl_device_ida, ddev->id);
 	kfree(ddev->irqs);
 	kfree(ddev);
@@ -380,7 +382,16 @@  dfl_dev_add(struct dfl_feature_platform_data *pdata,
 	ddev->type = feature_dev_id_type(pdev);
 	ddev->feature_id = feature->id;
 	ddev->revision = feature->revision;
+	ddev->dfh_version = feature->dfh_version;
 	ddev->cdev = pdata->dfl_cdev;
+	if (feature->param_size) {
+		ddev->params = kmemdup(feature->params, feature->param_size, GFP_KERNEL);
+		if (!ddev->params) {
+			ret = -ENOMEM;
+			goto put_dev;
+		}
+		ddev->param_size = feature->param_size;
+	}
 
 	/* add mmio resource */
 	parent_res = &pdev->resource[feature->resource_index];
@@ -708,20 +719,27 @@  struct build_feature_devs_info {
  * struct dfl_feature_info - sub feature info collected during feature dev build
  *
  * @fid: id of this sub feature.
+ * @revision: revision of this sub feature
+ * @dfh_version: version of Device Feature Header (DFH)
  * @mmio_res: mmio resource of this sub feature.
  * @ioaddr: mapped base address of mmio resource.
  * @node: node in sub_features linked list.
  * @irq_base: start of irq index in this sub feature.
  * @nr_irqs: number of irqs of this sub feature.
+ * @param_size: size DFH parameters.
+ * @params: DFH parameter data.
  */
 struct dfl_feature_info {
 	u16 fid;
 	u8 revision;
+	u8 dfh_version;
 	struct resource mmio_res;
 	void __iomem *ioaddr;
 	struct list_head node;
 	unsigned int irq_base;
 	unsigned int nr_irqs;
+	unsigned int param_size;
+	u64 params[];
 };
 
 static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
@@ -797,7 +815,17 @@  static int build_info_commit_dev(struct build_feature_devs_info *binfo)
 		feature->dev = fdev;
 		feature->id = finfo->fid;
 		feature->revision = finfo->revision;
+		feature->dfh_version = finfo->dfh_version;
 
+		if (finfo->param_size) {
+			feature->params = devm_kmemdup(binfo->dev,
+						       finfo->params, finfo->param_size,
+						       GFP_KERNEL);
+			if (!feature->params)
+				return -ENOMEM;
+
+			feature->param_size = finfo->param_size;
+		}
 		/*
 		 * the FIU header feature has some fundamental functions (sriov
 		 * set, port enable/disable) needed for the dfl bus device and
@@ -934,56 +962,108 @@  static u16 feature_id(u64 value)
 	return 0;
 }
 
+static void *find_param(void *base, resource_size_t max, int param)
+{
+	int off = 0;
+	u64 v, next;
+
+	while (off < max) {
+		v = *(u64 *)(base + off);
+		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
+			return base + off + DFHv1_PARAM_DATA;
+
+		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+		off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
+		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
+			break;
+
+	}
+
+	return NULL;
+}
+
+/**
+ * dfh_find_param() - find data for the given parameter id
+ * @dfl_dev: dfl device
+ * @param: id of dfl parameter
+ *
+ * Return: pointer to parameter data on success, NULL otherwise.
+ */
+void *dfh_find_param(struct dfl_device *dfl_dev, int param)
+{
+	return find_param(dfl_dev->params, dfl_dev->param_size, param);
+}
+EXPORT_SYMBOL_GPL(dfh_find_param);
+
 static int parse_feature_irqs(struct build_feature_devs_info *binfo,
-			      resource_size_t ofst, u16 fid,
-			      unsigned int *irq_base, unsigned int *nr_irqs)
+			      resource_size_t ofst, struct dfl_feature_info *finfo)
 {
 	void __iomem *base = binfo->ioaddr + ofst;
 	unsigned int i, ibase, inr = 0;
+	void *params = finfo->params;
 	enum dfl_id_type type;
+	u16 fid = finfo->fid;
 	int virq;
+	u32 *p;
 	u64 v;
 
-	type = feature_dev_id_type(binfo->feature_dev);
+	switch (finfo->dfh_version) {
+	case 0:
+		/*
+		 * DFHv0 only provides mmio resource information for each feature
+		 * in the DFL header.  There is no generic interrupt information.
+		 * Instead, features with interrupt functionality provide
+		 * the information in feature specific registers.
+		 */
+		type = feature_dev_id_type(binfo->feature_dev);
+		if (type == PORT_ID) {
+			switch (fid) {
+			case PORT_FEATURE_ID_UINT:
+				v = readq(base + PORT_UINT_CAP);
+				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
+				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
+				break;
+			case PORT_FEATURE_ID_ERROR:
+				v = readq(base + PORT_ERROR_CAP);
+				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
+				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
+				break;
+			}
+		} else if (type == FME_ID) {
+			switch (fid) {
+			case FME_FEATURE_ID_GLOBAL_ERR:
+				v = readq(base + FME_ERROR_CAP);
+				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
+				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
+				break;
+			}
+		}
+		break;
 
-	/*
-	 * Ideally DFL framework should only read info from DFL header, but
-	 * current version DFL only provides mmio resources information for
-	 * each feature in DFL Header, no field for interrupt resources.
-	 * Interrupt resource information is provided by specific mmio
-	 * registers of each private feature which supports interrupt. So in
-	 * order to parse and assign irq resources, DFL framework has to look
-	 * into specific capability registers of these private features.
-	 *
-	 * Once future DFL version supports generic interrupt resource
-	 * information in common DFL headers, the generic interrupt parsing
-	 * code will be added. But in order to be compatible to old version
-	 * DFL, the driver may still fall back to these quirks.
-	 */
-	if (type == PORT_ID) {
-		switch (fid) {
-		case PORT_FEATURE_ID_UINT:
-			v = readq(base + PORT_UINT_CAP);
-			ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
-			inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
+	case 1:
+		/*
+		 * DFHv1 provides interrupt resource information in DFHv1
+		 * parameter blocks.
+		 */
+		if (!finfo->param_size)
 			break;
-		case PORT_FEATURE_ID_ERROR:
-			v = readq(base + PORT_ERROR_CAP);
-			ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
-			inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
+
+		p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
+		if (!p)
 			break;
-		}
-	} else if (type == FME_ID) {
-		if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
-			v = readq(base + FME_ERROR_CAP);
-			ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
-			inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
-		}
+
+		ibase = *p++;
+		inr = *p;
+		break;
+
+	default:
+		dev_warn(binfo->dev, "unexpected DFH version %d\n", finfo->dfh_version);
+		break;
 	}
 
 	if (!inr) {
-		*irq_base = 0;
-		*nr_irqs = 0;
+		finfo->irq_base = 0;
+		finfo->nr_irqs = 0;
 		return 0;
 	}
 
@@ -1006,12 +1086,37 @@  static int parse_feature_irqs(struct build_feature_devs_info *binfo,
 		}
 	}
 
-	*irq_base = ibase;
-	*nr_irqs = inr;
+	finfo->irq_base = ibase;
+	finfo->nr_irqs = inr;
 
 	return 0;
 }
 
+static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
+{
+	int size = 0;
+	u64 v, next;
+
+	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
+		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
+		return 0;
+
+	while (size + DFHv1_PARAM_HDR < max) {
+		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
+
+		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+		if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
+			return -EINVAL;
+
+		size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
+
+		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
+			return size;
+	}
+
+	return -ENOENT;
+}
+
 /*
  * when create sub feature instances, for private features, it doesn't need
  * to provide resource size and feature id as they could be read from DFH
@@ -1023,39 +1128,70 @@  static int
 create_feature_instance(struct build_feature_devs_info *binfo,
 			resource_size_t ofst, resource_size_t size, u16 fid)
 {
-	unsigned int irq_base, nr_irqs;
 	struct dfl_feature_info *finfo;
+	int dfh_psize = 0;
 	u8 revision = 0;
+	u8 dfh_ver = 0;
 	int ret;
 	u64 v;
 
 	if (fid != FEATURE_ID_AFU) {
 		v = readq(binfo->ioaddr + ofst);
 		revision = FIELD_GET(DFH_REVISION, v);
-
+		dfh_ver = FIELD_GET(DFH_VERSION, v);
 		/* read feature size and id if inputs are invalid */
 		size = size ? size : feature_size(v);
 		fid = fid ? fid : feature_id(v);
+		if (dfh_ver == 1) {
+			dfh_psize = dfh_get_psize(binfo->ioaddr + ofst, size);
+			if (dfh_psize < 0) {
+				dev_err(binfo->dev,
+					"failed to read size of DFHv1 parameters %d\n",
+					dfh_psize);
+				return dfh_psize;
+			}
+			dev_dbg(binfo->dev, "dfhv1_psize %d\n", dfh_psize);
+		}
 	}
 
 	if (binfo->len - ofst < size)
 		return -EINVAL;
 
-	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
-	if (ret)
-		return ret;
-
-	finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
+	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
 	if (!finfo)
 		return -ENOMEM;
 
+	if (dfh_psize > 0) {
+		memcpy_fromio(finfo->params,
+			      binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
+		finfo->param_size = dfh_psize;
+	}
+
 	finfo->fid = fid;
 	finfo->revision = revision;
-	finfo->mmio_res.start = binfo->start + ofst;
-	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
+	finfo->dfh_version = dfh_ver;
 	finfo->mmio_res.flags = IORESOURCE_MEM;
-	finfo->irq_base = irq_base;
-	finfo->nr_irqs = nr_irqs;
+	if (dfh_ver == 1) {
+		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
+		if (v & DFHv1_CSR_ADDR_REL)
+			finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
+		else
+			finfo->mmio_res.start = binfo->start + ofst +
+					       FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
+
+		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
+		finfo->mmio_res.end = finfo->mmio_res.start +
+				      FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
+	} else {
+		finfo->mmio_res.start = binfo->start + ofst;
+		finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
+	}
+
+	ret = parse_feature_irqs(binfo, ofst, finfo);
+	if (ret) {
+		kfree(finfo);
+		return ret;
+	}
 
 	list_add_tail(&finfo->node, &binfo->sub_features);
 	binfo->feature_num++;
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 45e6e1359a67..1ea7f40c1af6 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -273,11 +273,14 @@  struct dfl_feature_irq_ctx {
  * @ops: ops of this sub feature.
  * @ddev: ptr to the dfl device of this sub feature.
  * @priv: priv data of this feature.
+ * @param_size: size of dfh parameters
+ * @params: point to memory copy of dfh parameters
  */
 struct dfl_feature {
 	struct platform_device *dev;
 	u16 id;
 	u8 revision;
+	u8 dfh_version;
 	int resource_index;
 	void __iomem *ioaddr;
 	struct dfl_feature_irq_ctx *irq_ctx;
@@ -285,6 +288,8 @@  struct dfl_feature {
 	const struct dfl_feature_ops *ops;
 	struct dfl_device *ddev;
 	void *priv;
+	unsigned int param_size;
+	void *params;
 };
 
 #define FEATURE_DEV_ID_UNUSED	(-1)
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index fea9e16d35b6..d00787e870b7 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -50,9 +50,12 @@  struct dfl_device {
 	u16 type;
 	u16 feature_id;
 	u8 revision;
+	u8 dfh_version;
 	struct resource mmio_res;
 	int *irqs;
 	unsigned int num_irqs;
+	unsigned int param_size;
+	void *params;
 	struct dfl_fpga_cdev *cdev;
 	const struct dfl_device_id *id_entry;
 };
@@ -95,4 +98,5 @@  void dfl_driver_unregister(struct dfl_driver *dfl_drv);
 	module_driver(__dfl_driver, dfl_driver_register, \
 		      dfl_driver_unregister)
 
+void *dfh_find_param(struct dfl_device *dfl_dev, int param);
 #endif /* __LINUX_DFL_H */