> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, March 12, 2015 5:48 AM
> To: Bhushan Bharat-R65777
> Cc: linuxppc-...@ozlabs.org
> Subject: Re: [PATCH 4/4 RFC] fsl/msi: Add interface to reserve/free msi
> bank
>
> 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.bhus...@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.
Ok
>
> > @@ -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 {}
Ok, leftover of print debugging :)
>
> >
> > 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?
No, will remove later "msi".
>
> > +
> > +/* 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.
ok
>
> > +static int is_msi_bank_reserved(struct fsl_msi *msi)
>
> s/int/bool/
Ok
>
>
> > +/*
> > + * 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?
Not needed, will use the msi_data->msiir as per patch 1/4;
>
> > @@ -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().
Ok,
Thanks a lot
-Bharat
>
> -Scott
>
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
