Re: [PATCH 4/4 RFC] fsl/msi: Add interface to reserve/free msi bank

[Scott Wood <scottwood@freescale.com>]

On Tue, 2015-03-03 at 10:47 +0530, Bharat Bhushan wrote:
> This patch allows a context (different from kernel context)
> to reserve a MSI bank for itself. And then the devices in the
> context will share the MSI bank.
> 
> VFIO meta driver is one of typical user of these APIs. It will
> reserve a MSI bank for MSI interrupt support of direct assignment
> PCI devices to a Guest. Patches for same will follow this patch.
> 
> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> ---
>  arch/powerpc/include/asm/device.h  |   2 +
>  arch/powerpc/include/asm/fsl_msi.h |  26 ++++++
>  arch/powerpc/sysdev/fsl_msi.c      | 169 +++++++++++++++++++++++++++++++------
>  arch/powerpc/sysdev/fsl_msi.h      |   1 +
>  4 files changed, 173 insertions(+), 25 deletions(-)
>  create mode 100644 arch/powerpc/include/asm/fsl_msi.h
> 
> diff --git a/arch/powerpc/include/asm/device.h b/arch/powerpc/include/asm/device.h
> index 38faede..1c2bfd7 100644
> --- a/arch/powerpc/include/asm/device.h
> +++ b/arch/powerpc/include/asm/device.h
> @@ -40,6 +40,8 @@ struct dev_archdata {
>  #ifdef CONFIG_FAIL_IOMMU
>  	int fail_iommu;
>  #endif
> +
> +	void *context;
>  };

This needs a comment and probably a more specific name.

> @@ -200,6 +185,12 @@ static struct fsl_msi *fsl_msi_get_reserved_msi_bank(struct pci_dev *pdev)
>  {
>  	struct fsl_msi *msi_data = NULL;
>  	void *context = NULL;
> +	struct device *dev = &pdev->dev;
> +
> +	/* Device assigned to userspace if there is valid context */
> +	if (dev->archdata.context) {
> +		context = dev->archdata.context;
> +	}

No {}

>  
>  	list_for_each_entry(msi_data, &msi_head, list) {
>  		if ((msi_data->reserved == MSI_RESERVED) &&
> @@ -208,13 +199,133 @@ static struct fsl_msi *fsl_msi_get_reserved_msi_bank(struct pci_dev *pdev)
>  	}
>  
>  	/* If no MSI bank allocated for kernel owned device, allocate one */
> -	msi_data = fsl_msi_allocate_msi_bank(NULL);
> -	if (msi_data)
> -		return msi_data;
> +	if (!context) {
> +		msi_data = fsl_msi_allocate_msi_bank(NULL);
> +		if (msi_data)
> +			return msi_data;
> +	}
>  
>  	return NULL;
>  }
>  
> +/* API to set "context" to which the device belongs */
> +int fsl_msi_set_msi_bank_in_dev(struct device *dev, void *data)
> +{
> +	dev->archdata.context = data;
> +	return 0;
> +}

Do we really need "msi" to appear twice in every function name?

> +
> +/*  This API Allows a MSI bank to be reserved for a "context".
> + *  All devices in same "context" will share the allocated
> + *  MSI bank.
> + *  Typically this function will be called from meta
> + *  driver like VFIO with a valid "context".
> + */
> +struct fsl_msi *fsl_msi_reserve_msi_bank(void *context)
> +{
> +	struct fsl_msi *msi_data;
> +
> +	if (!context)
> +		return NULL;
> +
> +	/* Check if msi-bank already allocated for the context */
> +	list_for_each_entry(msi_data, &msi_head, list) {
> +		if (msi_data->reserved == MSI_FREE)
> +			continue;
> +
> +		if (context == msi_data->context)
> +			return msi_data;

The reserved == MSI_FREE check doesn't add anything because if it's free
then the context check will fail.

> +static int is_msi_bank_reserved(struct fsl_msi *msi)

s/int/bool/


> +/*
> + * This function configures PAMU window for MSI page with
> + * given iova. Also same iova will be used as "msi-address"
> + * when configuring msi-message in the devices using this
> + * msi bank.
> + */
> +int fsl_msi_set_msi_bank_region(struct iommu_domain *domain,
> +				void *context , int win,

Whitespace

> @@ -225,12 +336,17 @@ static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq,
>  	int len;
>  	const __be64 *reg;
>  
> -	/* If the msi-address-64 property exists, then use it */
> -	reg = of_get_property(hose->dn, "msi-address-64", &len);
> -	if (reg && (len == sizeof(u64)))
> -		address = be64_to_cpup(reg);
> -	else
> -		address = msi_data->msiir;
> +	if (pdev->dev.archdata.context) {
> +		address = msi_data->iova;
> +	} else {
> +		/* If the msi-address-64 property exists, then use it */
> +		reg = of_get_property(hose->dn, "msi-address-64", &len);
> +		if (reg && (len == sizeof(u64)))
> +			address = be64_to_cpup(reg);
> +		else
> +			address = fsl_pci_immrbar_base(hose) +
> +					(msi_data->msiir & 0xfffff);
> +	}

You removed the call to fsl_pci_immrbar_base in patch 1/4 and are
reintroducing it here.  If this call is needed, how does it work in
between those two patches?

> @@ -401,6 +517,9 @@ static int fsl_of_msi_remove(struct platform_device *ofdev)
>  	struct fsl_msi *msi = platform_get_drvdata(ofdev);
>  	int virq, i;
>  
> +	if (is_msi_bank_reserved(msi))
> +		return -EBUSY;

As I mentioned in discussion of a prior version of this, the driver core
ignores error codes in .remove().  Since there's no reason to remove
this piece of platform infrastructure, and the remove path has probably
never been tested, I'd just replace the whole thing with BUG().

-Scott