Re: [PATCH v4 07/10] PCI/IDE: Add IDE establishment helpers

[Xu Yilun <yilun.xu@linux.intel.com>]

> This is the change I am adding
> 
>  drivers/pci/ide.c                        | 128 ++++++++++++++++++++++-
>  drivers/virt/coco/arm-cca-host/arm-cca.c |  13 +++
>  include/linux/pci-ide.h                  |   7 ++
>  3 files changed, 147 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/ide.c b/drivers/pci/ide.c
> index 3f772979eacb..23d1712ba97a 100644
> --- a/drivers/pci/ide.c
> +++ b/drivers/pci/ide.c
> @@ -101,7 +101,7 @@ void pci_ide_init(struct pci_dev *pdev)
>  	pdev->ide_cap = ide_cap;
>  	pdev->nr_link_ide = nr_link_ide;
>  	pdev->nr_sel_ide = nr_streams;
> -	pdev->nr_ide_mem = nr_ide_mem;
> +	pdev->nr_ide_mem = min(nr_ide_mem, PCI_IDE_AASOC_REG_MAX);
>  }
>  
>  struct stream_index {
> @@ -213,11 +213,13 @@ struct pci_ide *pci_ide_stream_alloc(struct pci_dev *pdev)
>  				.rid_start = pci_dev_id(rp),
>  				.rid_end = pci_dev_id(rp),
>  				.stream_index = no_free_ptr(ep_stream)->stream_index,
> +				.nr_mem = 0,
>  			},
>  			[PCI_IDE_RP] = {
>  				.rid_start = pci_dev_id(pdev),
>  				.rid_end = rid_end,
>  				.stream_index = no_free_ptr(rp_stream)->stream_index,
> +				.nr_mem = 0,
>  			},
>  		},
>  		.host_bridge_stream = no_free_ptr(hb_stream)->stream_index,
> @@ -228,6 +230,109 @@ struct pci_ide *pci_ide_stream_alloc(struct pci_dev *pdev)
>  }
>  EXPORT_SYMBOL_GPL(pci_ide_stream_alloc);
>  
> +static int add_range_merge_overlap(struct range *range, int az, int nr_range,
> +				   u64 start, u64 end)
> +{
> +	int i;
> +
> +	if (start >= end)
> +		return nr_range;
> +
> +	/* get new start/end: */
> +	for (i = 0; i < nr_range; i++) {
> +
> +		if (!range[i].end)
> +			continue;
> +
> +		/* Try to add to the end */
> +		if (range[i].end + 1 == start) {
> +			range[i].end = end;
> +			return nr_range;
> +		}
> +
> +		/* Try to add to the start */
> +		if (range[i].start == end + 1) {
> +			range[i].start = start;
> +			return nr_range;
> +		}
> +	}
> +
> +	/* Need to add it: */
> +	return add_range(range, az, nr_range, start, end);
> +}

Can add_range_with_merge() serve your purpose?

> +
> +int pci_ide_add_address_assoc_block(struct pci_dev *pdev,
> +				    struct pci_ide *ide,
> +				    u64 start, u64 end)
> +{
> +	struct pci_ide_partner *partner;
> +
> +	if (!pci_is_pcie(pdev)) {
> +		pci_warn_once(pdev, "not a PCIe device\n");
> +		return -EINVAL;
> +	}
> +
> +	switch (pci_pcie_type(pdev)) {
> +	case PCI_EXP_TYPE_ENDPOINT:
> +
> +		if (pdev != ide->pdev)
> +			return -EINVAL;
> +		partner = &ide->partner[PCI_IDE_RP];
> +		break;

IIUC, you want to program the RP is it? So is it better we input the to
be programmed device (RP), not the target device (EP) with the range.

> +	default:
> +		pci_warn_once(pdev, "invalid device type\n");
> +		return -EINVAL;
> +	}
> +
> +	if (partner->nr_mem >= pdev->nr_ide_mem)
> +		return -ENOMEM;

I don't get why the desired number blocks for RP is related to the
supported number blocks for EP.

Apart from this, I don't see the necessary to input the EP device.

> +
> +	partner->nr_mem = add_range_merge_overlap(partner->mem,
> +					   PCI_IDE_AASOC_REG_MAX, partner->nr_mem,
> +					   start, end);
> +	return 0;
> +}
> +
> +
> +int pci_ide_merge_address_assoc_block(struct pci_dev *pdev,
> +				      struct pci_ide *ide, u64 start, u64 end)
> +{
> +	struct pci_ide_partner *partner;
> +
> +	if (!pci_is_pcie(pdev)) {
> +		pci_warn_once(pdev, "not a PCIe device\n");
> +		return -EINVAL;
> +	}
> +
> +	switch (pci_pcie_type(pdev)) {
> +	case PCI_EXP_TYPE_ENDPOINT:
> +
> +		if (pdev != ide->pdev)
> +			return -EINVAL;
> +		partner = &ide->partner[PCI_IDE_RP];
> +		break;
> +	default:
> +		pci_warn_once(pdev, "invalid device type\n");
> +		return -EINVAL;
> +	}
> +
> +	for (int i = 0; i < PCI_IDE_AASOC_REG_MAX; i++) {
> +		struct range *r = &partner->mem[i];
> +
> +		if (r->start < start)
> +			start = r->start;
> +		if (r->end > end)
> +			end = r->end;
> +		r->start = 0;
> +		r->end = 0;
> +	}
> +	partner->mem[0].start = start;
> +	partner->mem[0].end = end;
> +	partner->nr_mem = 1;

IIUC, this function will merge previously added ranges, and the newly
input range, into one, which is wield to me as a kAPI.

> +
> +	return 0;
> +}

I noticed Arto has a good idea that there needs at most 2 blocks no
matter how the mmio layout is for PF/VF/MFD..., one for 32 bit, one for
64 bit. And the direct connected upstream bridge to the DSM device has
already aggregated the 2 ranges on enumeration. [1] That greatly reduces
the complexity. No need for callers to iterate the devices/resources to
collect the ranges again.

For TDX, the firmware enforces to setup only one addr block for RP, no
matter how many supported blocks the RP actually has. That means TDX
could only support 64 bit IDE ranges. I'd like to require an input
parameter like "max_nr_mem_rp" for this purpose.

Based on the above, I've found the only input from the caller is the
max_nr_mem_rp, how about we just add it in pci_ide_stream_alloc(),
input 0 if you don't need the addr block setup.

[...]

>  
> @@ -163,9 +164,21 @@ static int cca_tsm_connect(struct pci_dev *pdev)
>  	if (rc)
>  		goto err_stream;
>  
> +	/*
> +	 * Try to use the available address assoc register blocks.
> +	 * If we fail with ENOMEM, create one block covering the entire
> +	 * address range. (Should work for arm64)
> +	 */
> +	pci_dev_for_each_resource(pdev, res) {
> +		rc = pci_ide_add_address_assoc_block(pdev, ide, res->start, res->end);
> +		if (rc == -ENOMEM)
> +			pci_ide_merge_address_assoc_block(pdev, ide, res->start, res->end);
> +	}

You just input the addr of PF0, not VF/MFD... any limitation? If we
switch to get ranges from direct upstream bridge, are you still OK?

Thanks,
Yilun