linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Dey, Megha" <megha.dey@linux.intel.com>
To: Thomas Gleixner <tglx@linutronix.de>,
	Dave Jiang <dave.jiang@intel.com>,
	vkoul@kernel.org, maz@kernel.org, bhelgaas@google.com,
	rafael@kernel.org, gregkh@linuxfoundation.org, hpa@zytor.com,
	alex.williamson@redhat.com, jacob.jun.pan@intel.com,
	ashok.raj@intel.com, jgg@mellanox.com, yi.l.liu@intel.com,
	baolu.lu@intel.com, kevin.tian@intel.com,
	sanjay.k.kumar@intel.com, tony.luck@intel.com,
	jing.lin@intel.com, dan.j.williams@intel.com,
	kwankhede@nvidia.com, eric.auger@redhat.com, parav@mellanox.com
Cc: dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org,
	x86@kernel.org, linux-pci@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH RFC 04/15] drivers/base: Add support for a new IMS irq domain
Date: Sun, 3 May 2020 17:11:58 -0700	[thread overview]
Message-ID: <53df1de7-ca25-e1be-b31c-e57bd95f0564@linux.intel.com> (raw)
In-Reply-To: <87pnbvtfdc.fsf@nanos.tec.linutronix.de>

Hi Thomas,

On 4/25/2020 2:38 PM, Thomas Gleixner wrote:
> Dave Jiang <dave.jiang@intel.com> writes:
>> From: Megha Dey <megha.dey@linux.intel.com>
>>
>> Add support for the creation of a new IMS irq domain. It creates a new
>> irq chip associated with the IMS domain and adds the necessary domain
>> operations to it.
> 
> And how is a X86 specific thingy related to drivers/base?

Well, clearly this file has both arch independent sand dependent code 
which is incorrect. From various discussions, we have now concluded that 
IMS is after all not a X86 specific thingy after all. IMS is just a name 
intel came up with, all it really means is device managed addr/data 
writes to generate interrupts.

> 
>> diff --git a/drivers/base/ims-msi.c b/drivers/base/ims-msi.c
> 
> This sits in drivers base because IMS is architecture independent, right?

Per my above comment, technically we can call something IMS even if 
device has its own location to store interrupts in non-pci standard 
mechanism, much like platform-msi indeed. We simply need to extend 
platform-msi to its address some of its shortcomings: increase number of 
interrupts to > 2048, enable dynamic allocation of interrupts, add 
mask/unmask callbacks in addition to write_msg etc.

I will be sending out an email shortly outlining the new design for IMS 
and what are the improvements we want to add to the already exisitng 
platform-msi infrastructure.

> 
>> new file mode 100644
>> index 000000000000..738f6d153155
>> --- /dev/null
>> +++ b/drivers/base/ims-msi.c
>> @@ -0,0 +1,100 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Support for Device Specific IMS interrupts.
>> + *
>> + * Copyright © 2019 Intel Corporation.
>> + *
>> + * Author: Megha Dey <megha.dey@intel.com>
>> + */
>> +
>> +#include <linux/dmar.h>
>> +#include <linux/irq.h>
>> +#include <linux/mdev.h>
>> +#include <linux/pci.h>
>> +
>> +/*
>> + * Determine if a dev is mdev or not. Return NULL if not mdev device.
>> + * Return mdev's parent dev if success.
>> + */
>> +static inline struct device *mdev_to_parent(struct device *dev)
>> +{
>> +	struct device *ret = NULL;
>> +	struct device *(*fn)(struct device *dev);
>> +	struct bus_type *bus = symbol_get(mdev_bus_type);
> 
> symbol_get()?

mdev_bus_type is defined in driver/vfio/mdev/ directory. The entire 
vfio-mdev can be compiled as a module and if so, then this symbol is not 
visible outside of that directory and there are some linker errors. 
Currently, there these symbols sare self-contained and are not used 
outside of the directory where they are defined. I did not know earlier 
that is not advisible to use symbol_get() for this. I will try to come 
up with a better approach.

> 
>> +
>> +	if (bus && dev->bus == bus) {
>> +		fn = symbol_get(mdev_dev_to_parent_dev);
> 
> What's wrong with simple function calls?

Hmmm, same reason as above..
> 
>> +		ret = fn(dev);
>> +		symbol_put(mdev_dev_to_parent_dev);
>> +		symbol_put(mdev_bus_type);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static irq_hw_number_t dev_ims_get_hwirq(struct msi_domain_info *info,
>> +					 msi_alloc_info_t *arg)
>> +{
>> +	return arg->ims_hwirq;
>> +}
>> +
>> +static int dev_ims_prepare(struct irq_domain *domain, struct device *dev,
>> +			   int nvec, msi_alloc_info_t *arg)
>> +{
>> +	if (dev_is_mdev(dev))
>> +		dev = mdev_to_parent(dev);
> 
> This makes absolutely no sense. Somewhere you claimed that this is
> solely for mdev. Now this interface takes both a regular device and mdev.
> 
> Lack of explanation seems to be a common scheme here.

IMS can be used for mdev or a regular device. I do not think it is 
claimed anywhere that IMS is solely for mdev. In the current use case 
for DSA, IMS is used only by the guest (mdev) although it can very well 
be used by the host driver as well.

> 
>> +	init_irq_alloc_info(arg, NULL);
>> +	arg->dev = dev;
>> +	arg->type = X86_IRQ_ALLOC_TYPE_IMS;
>> +
>> +	return 0;
>> +}
>> +
>> +static void dev_ims_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
>> +{
>> +	arg->ims_hwirq = platform_msi_calc_hwirq(desc);
>> +}
>> +
>> +static struct msi_domain_ops dev_ims_domain_ops = {
>> +	.get_hwirq	= dev_ims_get_hwirq,
>> +	.msi_prepare	= dev_ims_prepare,
>> +	.set_desc	= dev_ims_set_desc,
>> +};
>> +
>> +static struct irq_chip dev_ims_ir_controller = {
>> +	.name			= "IR-DEV-IMS",
>> +	.irq_ack		= irq_chip_ack_parent,
>> +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
>> +	.irq_set_vcpu_affinity	= irq_chip_set_vcpu_affinity_parent,
>> +	.flags			= IRQCHIP_SKIP_SET_WAKE,
>> +	.irq_write_msi_msg	= platform_msi_write_msg,
>> +};
>> +
>> +static struct msi_domain_info ims_ir_domain_info = {
>> +	.flags		= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS,
>> +	.ops		= &dev_ims_domain_ops,
>> +	.chip		= &dev_ims_ir_controller,
>> +	.handler	= handle_edge_irq,
>> +	.handler_name	= "edge",
>> +};
>> +
>> +struct irq_domain *arch_create_ims_irq_domain(struct irq_domain *parent,
>> +					      const char *name)
> 
> arch_create_ ???? In drivers/base ???

Needs to go away. On second thought, per Jason Gunthorpe's comment, this 
is not even required. We can simply use the existing 
platform_msi_create_irq_domain API itself.
> 
>> +{
>> +	struct fwnode_handle *fn;
>> +	struct irq_domain *domain;
>> +
>> +	fn = irq_domain_alloc_named_fwnode(name);
>> +	if (!fn)
>> +		return NULL;
>> +
>> +	domain = msi_create_irq_domain(fn, &ims_ir_domain_info, parent);
>> +	if (!domain)
>> +		return NULL;
>> +
>> +	irq_domain_update_bus_token(domain, DOMAIN_BUS_PLATFORM_MSI);
>> +	irq_domain_free_fwnode(fn);
>> +
>> +	return domain;
>> +}
>> diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c
>> index 2696aa75983b..59160e8cbfb1 100644
>> --- a/drivers/base/platform-msi.c
>> +++ b/drivers/base/platform-msi.c
>> @@ -31,12 +31,11 @@ struct platform_msi_priv_data {
>>   /* The devid allocator */
>>   static DEFINE_IDA(platform_msi_devid_ida);
>>   
>> -#ifdef GENERIC_MSI_DOMAIN_OPS
>>   /*
>>    * Convert an msi_desc to a globaly unique identifier (per-device
>>    * devid + msi_desc position in the msi_list).
>>    */
>> -static irq_hw_number_t platform_msi_calc_hwirq(struct msi_desc *desc)
>> +irq_hw_number_t platform_msi_calc_hwirq(struct msi_desc *desc)
>>   {
>>   	u32 devid;
>>   
>> @@ -45,6 +44,7 @@ static irq_hw_number_t platform_msi_calc_hwirq(struct msi_desc *desc)
>>   	return (devid << (32 - DEV_ID_SHIFT)) | desc->platform.msi_index;
>>   }
>>   
>> +#ifdef GENERIC_MSI_DOMAIN_OPS
>>   static void platform_msi_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
>>   {
>>   	arg->desc = desc;
>> @@ -76,7 +76,7 @@ static void platform_msi_update_dom_ops(struct msi_domain_info *info)
>>   		ops->set_desc = platform_msi_set_desc;
>>   }
>>   
>> -static void platform_msi_write_msg(struct irq_data *data, struct msi_msg *msg)
>> +void platform_msi_write_msg(struct irq_data *data, struct msi_msg *msg)
>>   {
>>   	struct msi_desc *desc = irq_data_get_msi_desc(data);
>>   	struct platform_msi_priv_data *priv_data;
>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
>> index b558d4cfd082..cecc6a6bdbef 100644
>> --- a/drivers/vfio/mdev/mdev_core.c
>> +++ b/drivers/vfio/mdev/mdev_core.c
>> @@ -33,6 +33,12 @@ struct device *mdev_parent_dev(struct mdev_device *mdev)
>>   }
>>   EXPORT_SYMBOL(mdev_parent_dev);
>>   
>> +struct device *mdev_dev_to_parent_dev(struct device *dev)
>> +{
>> +	return to_mdev_device(dev)->parent->dev;
>> +}
>> +EXPORT_SYMBOL(mdev_dev_to_parent_dev);
> 
> And this needs to be EXPORT_SYMBOL because this is designed to support
> non GPL drivers from the very beginning, right? Ditto for the other
> exports in this file.

Hmm, I followed the same convention as the other exports here. Guess I 
would have to change all other exports to EXPORT_SYMBOL_GPL as well.

> 
>> diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
>> index 7d922950caaf..c21f1305a76b 100644
>> --- a/drivers/vfio/mdev/mdev_private.h
>> +++ b/drivers/vfio/mdev/mdev_private.h
>> @@ -36,7 +36,6 @@ struct mdev_device {
>>   };
>>   
>>   #define to_mdev_device(dev)	container_of(dev, struct mdev_device, dev)
>> -#define dev_is_mdev(d)		((d)->bus == &mdev_bus_type)
> 
> Moving stuff around 3 patches later makes tons of sense.

ok will add it earlier then.
>    
> Thanks,
> 
>          tglx
> 

  reply	other threads:[~2020-05-04  0:12 UTC|newest]

Thread overview: 89+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-21 23:33 [PATCH RFC 00/15] Add VFIO mediated device support and IMS support for the idxd driver Dave Jiang
2020-04-21 23:33 ` [PATCH RFC 01/15] drivers/base: Introduce platform_msi_ops Dave Jiang
2020-04-26  7:01   ` Greg KH
2020-04-27 21:38     ` Dave Jiang
2020-04-28  7:34       ` Greg KH
2020-04-21 23:33 ` [PATCH RFC 02/15] drivers/base: Introduce a new platform-msi list Dave Jiang
2020-04-25 21:13   ` Thomas Gleixner
2020-05-04  0:08     ` Dey, Megha
2020-04-21 23:34 ` [PATCH RFC 03/15] drivers/base: Allocate/free platform-msi interrupts by group Dave Jiang
2020-04-25 21:23   ` Thomas Gleixner
2020-05-04  0:08     ` Dey, Megha
2020-04-21 23:34 ` [PATCH RFC 04/15] drivers/base: Add support for a new IMS irq domain Dave Jiang
2020-04-23 20:11   ` Jason Gunthorpe
2020-05-01 22:30     ` Dey, Megha
2020-05-03 22:25       ` Jason Gunthorpe
2020-05-03 22:40         ` Dey, Megha
2020-05-03 22:46           ` Jason Gunthorpe
2020-05-04  0:25             ` Dey, Megha
2020-05-04 12:14               ` Jason Gunthorpe
2020-05-06 10:27                 ` Tian, Kevin
2020-04-25 21:38   ` Thomas Gleixner
2020-05-04  0:11     ` Dey, Megha [this message]
2020-04-21 23:34 ` [PATCH RFC 05/15] ims-msi: Add mask/unmask routines Dave Jiang
2020-04-25 21:49   ` Thomas Gleixner
2020-05-04  0:16     ` Dey, Megha
2020-04-21 23:34 ` [PATCH RFC 06/15] ims-msi: Enable IMS interrupts Dave Jiang
2020-04-25 22:13   ` Thomas Gleixner
2020-05-04  0:17     ` Dey, Megha
2020-04-21 23:34 ` [PATCH RFC 07/15] Documentation: Interrupt Message store Dave Jiang
2020-04-23 20:04   ` Jason Gunthorpe
2020-05-01 22:32     ` Dey, Megha
2020-05-03 22:28       ` Jason Gunthorpe
2020-05-03 22:41         ` Dey, Megha
2020-04-21 23:34 ` [PATCH RFC 08/15] vfio/mdev: Add a member for iommu domain in mdev_device Dave Jiang
2020-04-21 23:34 ` [PATCH RFC 09/15] vfio/type1: Save domain when attach domain to mdev Dave Jiang
2020-04-21 23:34 ` [PATCH RFC 10/15] dmaengine: idxd: add config support for readonly devices Dave Jiang
2020-04-21 23:34 ` [PATCH RFC 11/15] dmaengine: idxd: add IMS support in base driver Dave Jiang
2020-04-21 23:35 ` [PATCH RFC 12/15] dmaengine: idxd: add device support functions in prep for mdev Dave Jiang
2020-04-21 23:35 ` [PATCH RFC 13/15] dmaengine: idxd: add support for VFIO mediated device Dave Jiang
2020-04-21 23:35 ` [PATCH RFC 14/15] dmaengine: idxd: add error notification from host driver to " Dave Jiang
2020-04-21 23:35 ` [PATCH RFC 15/15] dmaengine: idxd: add ABI documentation for mediated device support Dave Jiang
2020-04-21 23:54 ` [PATCH RFC 00/15] Add VFIO mediated device support and IMS support for the idxd driver Jason Gunthorpe
2020-04-22  0:53   ` Tian, Kevin
2020-04-22 11:50     ` Jason Gunthorpe
2020-04-22 21:14       ` Raj, Ashok
2020-04-23 19:12         ` Jason Gunthorpe
2020-04-24  3:27           ` Tian, Kevin
2020-04-24 12:44             ` Jason Gunthorpe
2020-04-24 16:25               ` Tian, Kevin
2020-04-24 18:12                 ` Jason Gunthorpe
2020-04-26  5:18                   ` Tian, Kevin
2020-04-26 19:13                     ` Jason Gunthorpe
2020-04-27  3:43                       ` Alex Williamson
2020-04-27 11:58                         ` Jason Gunthorpe
2020-04-27 13:19                           ` Alex Williamson
2020-04-27 13:22                             ` Jason Gunthorpe
2020-04-27 14:18                               ` Alex Williamson
2020-04-27 14:25                                 ` Jason Gunthorpe
2020-04-27 15:41                                   ` Alex Williamson
2020-04-27 16:16                                     ` Jason Gunthorpe
2020-04-27 16:25                                       ` Dave Jiang
2020-04-27 21:56                                         ` Jason Gunthorpe
2020-04-29  9:42                               ` Tian, Kevin
2020-05-08 20:47                                 ` Raj, Ashok
2020-05-08 23:16                                   ` Jason Gunthorpe
2020-05-08 23:52                                     ` Dave Jiang
2020-05-09  0:09                                     ` Raj, Ashok
2020-05-09 12:21                                       ` Jason Gunthorpe
2020-05-13  2:29                                         ` Jason Wang
2020-05-13  8:30                                         ` Tian, Kevin
2020-05-13 12:40                                           ` Jason Gunthorpe
2020-04-27 12:13                       ` Tian, Kevin
2020-04-27 12:55                         ` Jason Gunthorpe
2020-04-22 21:24   ` Dan Williams
2020-04-23 19:17     ` Dan Williams
2020-04-23 19:49       ` Jason Gunthorpe
2020-05-01 22:31         ` Dey, Megha
2020-05-03 22:21           ` Jason Gunthorpe
2020-05-03 22:32             ` Dey, Megha
2020-04-23 19:18     ` Jason Gunthorpe
2020-05-01 22:31       ` Dey, Megha
2020-05-03 22:22         ` Jason Gunthorpe
2020-05-03 22:31           ` Dey, Megha
2020-05-03 22:36             ` Jason Gunthorpe
2020-05-04  0:20               ` Dey, Megha
2020-04-22 23:04   ` Dey, Megha
2020-04-23 19:44     ` Jason Gunthorpe
2020-05-01 22:32       ` Dey, Megha
2020-04-24  6:31   ` Jason Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=53df1de7-ca25-e1be-b31c-e57bd95f0564@linux.intel.com \
    --to=megha.dey@linux.intel.com \
    --cc=alex.williamson@redhat.com \
    --cc=ashok.raj@intel.com \
    --cc=baolu.lu@intel.com \
    --cc=bhelgaas@google.com \
    --cc=dan.j.williams@intel.com \
    --cc=dave.jiang@intel.com \
    --cc=dmaengine@vger.kernel.org \
    --cc=eric.auger@redhat.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hpa@zytor.com \
    --cc=jacob.jun.pan@intel.com \
    --cc=jgg@mellanox.com \
    --cc=jing.lin@intel.com \
    --cc=kevin.tian@intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=kwankhede@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=parav@mellanox.com \
    --cc=rafael@kernel.org \
    --cc=sanjay.k.kumar@intel.com \
    --cc=tglx@linutronix.de \
    --cc=tony.luck@intel.com \
    --cc=vkoul@kernel.org \
    --cc=x86@kernel.org \
    --cc=yi.l.liu@intel.com \
    --subject='Re: [PATCH RFC 04/15] drivers/base: Add support for a new IMS irq domain' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).