[PATCH 3/4 RFC] fsl/msi: Add MSI bank allocation for kernel owned devices

[Bharat.Bhushan at freescale.com]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, March 12, 2015 4:53 AM
> To: Bhushan Bharat-R65777
> Cc: linuxppc-dev at ozlabs.org
> Subject: Re: [PATCH 3/4 RFC] fsl/msi: Add MSI bank allocation for kernel
> owned devices
> 
> On Tue, 2015-03-03 at 10:47 +0530, Bharat Bhushan wrote:
> > With this patch a "context" can allocate a MSI bank and use the
> > allocated MSI-bank for the devices in that "context".
> >
> > kernel/host "context" is "NULL", So all devices owned by kernel will
> > share a MSI bank allocated with "context = NULL.
> >
> > This patch is in direction to have separate MSI bank for kernel
> > context and userspace/VM context. We do not want two software context
> > (kernel and VMs) to share a MSI bank for safe/reliable interrupts with
> > full isolation. Follow up patch will add interface to allocate a MSI
> > bank for userspace/VM context.
> >
> > NOTE: This RFC patch allows only one MSI bank to be allocated for
> > kernel context. Which seems to be sufficient to me. But if we see this
> > is limiting some real usecase scanerio then this limitation can be
> > removed
> >
> > One issue which still need to addressed is when to free kernel context
> > allocated MSI bank? Say all MSI capable devices are assigned to
> > VM/userspace then there is no need to have any MSI bank reserved for
> > kernel context.
> >
> > Signed-off-by: Bharat Bhushan <Bharat.Bhushan at freescale.com>
> > ---
> >  arch/powerpc/sysdev/fsl_msi.c | 88
> > ++++++++++++++++++++++++++++++++++++++-----
> >  arch/powerpc/sysdev/fsl_msi.h |  4 ++
> >  2 files changed, 83 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/powerpc/sysdev/fsl_msi.c
> > b/arch/powerpc/sysdev/fsl_msi.c index 32ba1e3..027aeeb 100644
> > --- a/arch/powerpc/sysdev/fsl_msi.c
> > +++ b/arch/powerpc/sysdev/fsl_msi.c
> > @@ -142,6 +142,79 @@ static void fsl_teardown_msi_irqs(struct pci_dev
> *pdev)
> >  	return;
> >  }
> >
> > +/*
> > + * Allocate a MSI Bank for the requested "context".
> > + * NULL "context" means that this request is to allocate
> > + * MSI bank for kernel owned devices. And currently we
> > + * assume that one MSI bank is sufficient for kernel.
> > + */
> > +static struct fsl_msi *fsl_msi_allocate_msi_bank(void *context) {
> > +	struct fsl_msi *msi_data;
> > +
> > +	/* Kernel context (NULL) can reserve only one msi bank */
> > +	if (!context) {
> > +		list_for_each_entry(msi_data, &msi_head, list) {
> > +			if ((msi_data->reserved == MSI_RESERVED) &&
> > +			    (msi_data->context == NULL))
> > +				return NULL;
> > +		}
> 
> Unnecessary parentheses
> 
> Is there any reason why the kernel bank needs to participate in this
> mechanism at all?  Set it aside at MSI driver init, and don't put it on
> the list.  I know you've previously said that you want to support configs
> where the kernel doesn't get any banks, but that can just be a boot
> option that tells the MSI code to not set aside a bank for the kernel.
> It would simplify the code.

I think we cannot completely remove the MSI bank from kernel practically. So I can make the kernel msi bank out of list.

> 
> With the patchset as is, how would one indicate whether kernel devices
> should get a bank?

For kernel owned device, in fsl_setup_msi_irqs() we check if there is a reserved MSI bank, if not then reserve a msi bank. If reserve fails then setup_msi_irq() fails. I think there is no fallback to Legacy interrupt.

>  Specifically, when the kernel does have an MSI-
> capable device but we'd prefer to use legacy interrupts to keep MSIs
> available to VFIO.

Do we want this?

> 
> > +	}
> > +
> > +	list_for_each_entry(msi_data, &msi_head, list) {
> > +		if (msi_data->reserved == MSI_FREE) {
> > +			msi_data->reserved = MSI_RESERVED;
> > +			msi_data->context = context;
> > +			return msi_data;
> > +		}
> > +	}
> 
> What prevents races from parallel callers?

Will add a lock.

> 
> > +/* FIXME: Assumption that host kernel will allocate only one MSI bank
> > +*/
> 
> It's not a FIXME if we think the limitation is not burdensome.

Ok

> 
> > + __attribute__ ((unused)) static int fsl_msi_free_msi_bank(void
> > + *context)
> 
> static int __maybe_unused fsl_msi_free_msi_bank(void *context)
> 
> Why are you adding this function then removing it in the next patch?

Will fix this

> 
> > +	/* 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;
> > +
> > +	return NULL;
> 
> return fsl_msi_allocate_msi_bank(NULL);

Ok

> 
> 
> > +}
> > +
> >  static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq,
> >  				struct msi_msg *msg,
> >  				struct fsl_msi *fsl_msi_data)
> > @@ -174,7 +247,7 @@ static int fsl_setup_msi_irqs(struct pci_dev *pdev,
> int nvec, int type)
> >  	struct pci_controller *hose = pci_bus_to_host(pdev->bus);
> >  	struct device_node *np;
> >  	phandle phandle = 0;
> > -	int rc, hwirq = -ENOMEM;
> > +	int rc = -ENODEV, hwirq = -ENOMEM;
> 
> Initialize rc when you detect the error instead of here (it'd be OK to
> initialize it to zero here, to mitigate potential uninitialized value
> bugs).

Ok

> 
> >  	unsigned int virq;
> >  	struct msi_desc *entry;
> >  	struct msi_msg msg;
> > @@ -231,15 +304,12 @@ static int fsl_setup_msi_irqs(struct pci_dev
> *pdev, int nvec, int type)
> >  		if (specific_msi_bank) {
> >  			hwirq = msi_bitmap_alloc_hwirqs(&msi_data->bitmap, 1);
> >  		} else {
> > -			/*
> > -			 * Loop over all the MSI devices until we find one that
> has an
> > -			 * available interrupt.
> > -			 */
> > -			list_for_each_entry(msi_data, &msi_head, list) {
> > -				hwirq = msi_bitmap_alloc_hwirqs(&msi_data-
> >bitmap, 1);
> > -				if (hwirq >= 0)
> > -					break;
> > +			msi_data = fsl_msi_get_reserved_msi_bank(pdev);
> > +			if (!msi_data) {
> > +				dev_err(&pdev->dev, "No MSI Bank allocated\n");
> > +				goto out_free;
> 
> Is this really an error?  Seems like dev_info() would be more appropriate
> to indicate the absence of a resource where you can fall back to another
> option.

There is no fallback in fsl_setup_msi_irqs(), We have to error out from fsl_setup_msi_irqs(), no?

> 
> > diff --git a/arch/powerpc/sysdev/fsl_msi.h
> > b/arch/powerpc/sysdev/fsl_msi.h index 9b0ab84..c69702b 100644
> > --- a/arch/powerpc/sysdev/fsl_msi.h
> > +++ b/arch/powerpc/sysdev/fsl_msi.h
> > @@ -46,6 +46,10 @@ struct fsl_msi {
> >  	struct list_head list;          /* support multiple MSI banks */
> >
> >  	phandle phandle;
> > +#define MSI_FREE		0
> > +#define MSI_RESERVED		1
> > +        int reserved;
> > +        void *context;
> 
> Whitespace
> 
> How about just "bool reserved"?  Or if the kernel bank is kept off the
> list, just check context for NULL.

I will try to make kernel bank out of list and see how much this simplifies the code.

Thanks
-Bharat

> 
> -Scott
>