RE: [PATCH v9 01/13] KVM: PPC: POWERNV: move iommu_add_device earlier
Hi Alex,
Looks like this patch is not picked by anyone, Are you going to pick this patch?
My vfio/iommu patches have dependency on this patch (this is already tested by
me).
Thanks
-Bharat
> -Original Message-
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+bharat.bhushan=freescale@lists.ozlabs.org] On Behalf Of Alexey
> Kardashevskiy
> Sent: Wednesday, August 28, 2013 2:08 PM
> To: linuxppc-...@lists.ozlabs.org
> Cc: k...@vger.kernel.org; Gleb Natapov; Alexey Kardashevskiy; Alexander Graf;
> kvm-...@vger.kernel.org; linux-kernel@vger.kernel.org; linux...@kvack.org;
> Paul
> Mackerras; Paolo Bonzini; David Gibson
> Subject: [PATCH v9 01/13] KVM: PPC: POWERNV: move iommu_add_device earlier
>
> The current implementation of IOMMU on sPAPR does not use iommu_ops and
> therefore does not call IOMMU API's bus_set_iommu() which
> 1) sets iommu_ops for a bus
> 2) registers a bus notifier
> Instead, PCI devices are added to IOMMU groups from
> subsys_initcall_sync(tce_iommu_init) which does basically the same thing
> without
> using iommu_ops callbacks.
>
> However Freescale PAMU driver (https://lkml.org/lkml/2013/7/1/158)
> implements iommu_ops and when tce_iommu_init is called, every PCI device is
> already added to some group so there is a conflict.
>
> This patch does 2 things:
> 1. removes the loop in which PCI devices were added to groups and adds
> explicit
> iommu_add_device() calls to add devices as soon as they get the iommu_table
> pointer assigned to them.
> 2. moves a bus notifier to powernv code in order to avoid conflict with the
> notifier from Freescale driver.
>
> iommu_add_device() and iommu_del_device() are public now.
>
> Signed-off-by: Alexey Kardashevskiy
> ---
> Changes:
> v8:
> * added the check for iommu_group!=NULL before removing device from a group as
> suggested by Wei Yang
>
> v2:
> * added a helper - set_iommu_table_base_and_group - which does
> set_iommu_table_base() and iommu_add_device()
> ---
> arch/powerpc/include/asm/iommu.h| 9 +++
> arch/powerpc/kernel/iommu.c | 41
> +++--
> arch/powerpc/platforms/powernv/pci-ioda.c | 8 +++---
> arch/powerpc/platforms/powernv/pci-p5ioc2.c | 2 +-
> arch/powerpc/platforms/powernv/pci.c| 33 ++-
> arch/powerpc/platforms/pseries/iommu.c | 8 +++---
> 6 files changed, 55 insertions(+), 46 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/iommu.h
> b/arch/powerpc/include/asm/iommu.h
> index c34656a..19ad77f 100644
> --- a/arch/powerpc/include/asm/iommu.h
> +++ b/arch/powerpc/include/asm/iommu.h
> @@ -103,6 +103,15 @@ extern struct iommu_table *iommu_init_table(struct
> iommu_table * tbl,
> int nid);
> extern void iommu_register_group(struct iommu_table *tbl,
>int pci_domain_number, unsigned long pe_num);
> +extern int iommu_add_device(struct device *dev); extern void
> +iommu_del_device(struct device *dev);
> +
> +static inline void set_iommu_table_base_and_group(struct device *dev,
> + void *base)
> +{
> + set_iommu_table_base(dev, base);
> + iommu_add_device(dev);
> +}
>
> extern int iommu_map_sg(struct device *dev, struct iommu_table *tbl,
> struct scatterlist *sglist, int nelems, diff --git
> a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c index
> b20ff17..15f8ca8 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -1105,7 +1105,7 @@ void iommu_release_ownership(struct iommu_table *tbl) }
> EXPORT_SYMBOL_GPL(iommu_release_ownership);
>
> -static int iommu_add_device(struct device *dev)
> +int iommu_add_device(struct device *dev)
> {
> struct iommu_table *tbl;
> int ret = 0;
> @@ -1134,46 +1134,13 @@ static int iommu_add_device(struct device *dev)
>
> return ret;
> }
> +EXPORT_SYMBOL_GPL(iommu_add_device);
>
> -static void iommu_del_device(struct device *dev)
> +void iommu_del_device(struct device *dev)
> {
> iommu_group_remove_device(dev);
> }
> -
> -static int iommu_bus_notifier(struct notifier_block *nb,
> - unsigned long action, void *data)
> -{
> - struct device *dev = data;
> -
> - switch (action) {
> - case BUS_NOTIFY_ADD_DEVICE:
> - return iommu_add_device(dev);
> - case BUS_NOTIFY_DEL_DEVICE:
> - iommu_del_device(dev);
> - return 0;
> - default:
> - return 0;
> - }
> -}
> -
> -static struct notifier_block tce_iommu_bus_nb = {
> - .notifier_call = iommu_bus_notifier,
> -};
> -
> -static int __init tce_iommu_init(void)
> -{
> - struct pci_dev *pdev = NULL;
> -
> - BUILD_BUG_ON(PAGE_SIZE < IOMMU_PAGE_SIZE);
> -
> - for_each_pci_dev(pdev)
> - iommu_add_device(&pdev->dev);
> -
> - bus_register_notifier(&pci_bus_type, &tce_iommu_bus_nb);
> - return
RE: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via sysfs only
> -Original Message-
> From: Wood Scott-B07421
> Sent: Tuesday, October 29, 2013 10:25 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder Stuart-B08248;
> christoffer.d...@linaro.org; linux-kernel@vger.kernel.org;
> a.mota...@virtualopensystems.com; ag...@suse.de; Sethi Varun-B16395;
> peter.mayd...@linaro.org; santosh.shu...@linaro.org; k...@vger.kernel.org;
> gre...@linuxfoundation.org
> Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via
> sysfs only
>
> On Mon, 2013-10-28 at 23:45 -0500, Bhushan Bharat-R65777 wrote:
> >
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, October 29, 2013 10:05 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder
> > > Stuart-B08248; christoffer.d...@linaro.org;
> > > linux-kernel@vger.kernel.org; a.mota...@virtualopensystems.com;
> > > ag...@suse.de; Sethi Varun-B16395; peter.mayd...@linaro.org;
> > > santosh.shu...@linaro.org; k...@vger.kernel.org;
> > > gre...@linuxfoundation.org
> > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit
> > > binding via sysfs only
> > >
> > > On Mon, 2013-10-28 at 23:31 -0500, Bhushan Bharat-R65777 wrote:
> > > >
> > > > > -Original Message-
> > > > > From: Wood Scott-B07421
> > > > > Sent: Tuesday, October 29, 2013 10:00 AM
> > > > > To: Bhushan Bharat-R65777
> > > > > Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder
> > > > > Stuart-B08248; christoffer.d...@linaro.org;
> > > > > linux-kernel@vger.kernel.org; a.mota...@virtualopensystems.com;
> > > > > ag...@suse.de; Sethi Varun-B16395; peter.mayd...@linaro.org;
> > > > > santosh.shu...@linaro.org; k...@vger.kernel.org;
> > > > > gre...@linuxfoundation.org
> > > > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit
> > > > > binding via sysfs only
> > > > >
> > > > > On Mon, 2013-10-28 at 22:52 -0500, Bhushan Bharat-R65777 wrote:
> > > > > > So when ids == NULL it does not check of vendor etc and calls
> > > > > > pci_add_dynid()
> > > > > which in turn calls driver_attach().
> > > > > >
> > > > > > If we change the above loop to break if ids->vendor ==
> > > > > >PCI_ANY_ID && ids- subvendor == PCI_ANY_ID then also we will call
> pci_add_dyids().
> > > > >
> > > > > What problem are you trying to solve?
> > > >
> > > > new_id interface to continue working as before.
> > >
> > > In what specific way does this allow new_id to continue working as
> > > before? Be verbose.
> >
> >
> > What I observed that this patch (kim's patch) new_id interface stops
> > working.
>
> Yes.
>
> > This is found to be because store_new_id() checks for pdrv->id_table
> > which is no more NULL, so the below check fails
>
> I do not think that is the reason. The reason is because sysfs_bind_only is
> set, and this is not a direct sysfs bind.
>
> > if (ids) {
> > ^^
> > This is no more NULL, so enter inside the loop
> >
> > retval = -EINVAL;
> > while (ids->vendor || ids->subvendor || ids->class_mask) {
> > if (driver_data == ids->driver_data) {
> > retval = 0;
> > break;
> > }
> > ids++;
> > }
> > if (retval) /* No match */
> > return retval; ^ This is where it returns
> > as -EINVAL
>
> Why wouldn't it have broken out of the loop earlier, since driver_data and
> ids-
> >driver_data should both be zero? I assume this is with a patch to do
> PCI_ANY_ID in vfio-pci.
hmmm, I am pretty sure I have seen that issue a few time (below is command line
output) but now I am not getting any error reported. Although device is not
binding to driver because of sysfs_bind_only as you mentioned (I thought of
this as a second issue). If I will be able to reproduce the first issue then I
will let you guys know otherwise there was no first issue :(
root@p5040ds:/sys/bus/pci# echo :01:00.0 >
devices/\:01\:00.0/driver/unbind
e1000e :01:00.0 eth0: removed PHC
root@p5040ds:/sys/bus/pci# echo 8086 10d3 > drivers/vfio-pci/new_id
-sh: echo: write error: Invalid argument
root@p5040ds:/sys/bus/pci# echo :01:00.0 > drivers/vfio-pci/bind
-Bharat
>
> -Scott
>
RE: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via sysfs only
> -Original Message- > From: Wood Scott-B07421 > Sent: Tuesday, October 29, 2013 10:05 AM > To: Bhushan Bharat-R65777 > Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder Stuart-B08248; > christoffer.d...@linaro.org; linux-kernel@vger.kernel.org; > a.mota...@virtualopensystems.com; ag...@suse.de; Sethi Varun-B16395; > peter.mayd...@linaro.org; santosh.shu...@linaro.org; k...@vger.kernel.org; > gre...@linuxfoundation.org > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via > sysfs only > > On Mon, 2013-10-28 at 23:31 -0500, Bhushan Bharat-R65777 wrote: > > > > > -Original Message- > > > From: Wood Scott-B07421 > > > Sent: Tuesday, October 29, 2013 10:00 AM > > > To: Bhushan Bharat-R65777 > > > Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder > > > Stuart-B08248; christoffer.d...@linaro.org; > > > linux-kernel@vger.kernel.org; a.mota...@virtualopensystems.com; > > > ag...@suse.de; Sethi Varun-B16395; peter.mayd...@linaro.org; > > > santosh.shu...@linaro.org; k...@vger.kernel.org; > > > gre...@linuxfoundation.org > > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit > > > binding via sysfs only > > > > > > On Mon, 2013-10-28 at 22:52 -0500, Bhushan Bharat-R65777 wrote: > > > > > > > > > -Original Message- > > > > > From: Wood Scott-B07421 > > > > > Sent: Tuesday, October 29, 2013 9:11 AM > > > > > To: Bhushan Bharat-R65777 > > > > > Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder > > > > > Stuart-B08248; christoffer.d...@linaro.org; > > > > > linux-kernel@vger.kernel.org; a.mota...@virtualopensystems.com; > > > > > ag...@suse.de; Sethi Varun-B16395; peter.mayd...@linaro.org; > > > > > santosh.shu...@linaro.org; k...@vger.kernel.org; > > > > > gre...@linuxfoundation.org > > > > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit > > > > > binding via sysfs only > > > > > > > > > > On Mon, 2013-10-28 at 22:38 -0500, Bhushan Bharat-R65777 wrote: > > > > > > > > > > > > > -Original Message- > > > > > > > From: Wood Scott-B07421 > > > > > > > Sent: Monday, October 28, 2013 11:40 PM > > > > > > > To: Alex Williamson > > > > > > > Cc: Kim Phillips; Bhushan Bharat-R65777; Wood Scott-B07421; > > > > > > > Yoder Stuart-B08248; christoffer.d...@linaro.org; > > > > > > > linux-kernel@vger.kernel.org; > > > > > > > a.mota...@virtualopensystems.com; ag...@suse.de; Sethi > > > > > > > Varun-B16395; peter.mayd...@linaro.org; > > > > > > > santosh.shu...@linaro.org; k...@vger.kernel.org; > > > > > > > gre...@linuxfoundation.org > > > > > > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for > > > > > > > explicit binding via sysfs only > > > > > > > > > > > > > > On Mon, 2013-10-28 at 13:00 -0500, Scott Wood wrote: > > > > > > > > On Mon, 2013-10-28 at 11:47 -0600, Alex Williamson wrote: > > > > > > > > > On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote: > > > > > > > > > > Force the vfio-pci driver to only be bound explicitly > > > > > > > > > > via sysfs to avoid conflics with other drivers in the > > > > > > > > > > event of a > > > hotplug. > > > > > > > > > > > > > > > > > > We can't break userspace, so we can't disable the > > > > > > > > > current method of binding devices to vfio-pci. We can > > > > > > > > > add a new method and perhaps deprecate the existing > > > > > > > > > mechanism to be removed at some point in the future. > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > I thought the existing method involved using sysfs bind, > > > > > > > > and this was just eliminating a race. How does the bind > > > > > > > > get triggered > > > currently? > > > > > > > > > > > > > > OK, so it seems it's relying on the write to new_id calling > > > driver_attach(). > > > > > >
RE: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via sysfs only
> -Original Message-
> From: Wood Scott-B07421
> Sent: Tuesday, October 29, 2013 10:00 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder Stuart-B08248;
> christoffer.d...@linaro.org; linux-kernel@vger.kernel.org;
> a.mota...@virtualopensystems.com; ag...@suse.de; Sethi Varun-B16395;
> peter.mayd...@linaro.org; santosh.shu...@linaro.org; k...@vger.kernel.org;
> gre...@linuxfoundation.org
> Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via
> sysfs only
>
> On Mon, 2013-10-28 at 22:52 -0500, Bhushan Bharat-R65777 wrote:
> >
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, October 29, 2013 9:11 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder
> > > Stuart-B08248; christoffer.d...@linaro.org;
> > > linux-kernel@vger.kernel.org; a.mota...@virtualopensystems.com;
> > > ag...@suse.de; Sethi Varun-B16395; peter.mayd...@linaro.org;
> > > santosh.shu...@linaro.org; k...@vger.kernel.org;
> > > gre...@linuxfoundation.org
> > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit
> > > binding via sysfs only
> > >
> > > On Mon, 2013-10-28 at 22:38 -0500, Bhushan Bharat-R65777 wrote:
> > > >
> > > > > -Original Message-
> > > > > From: Wood Scott-B07421
> > > > > Sent: Monday, October 28, 2013 11:40 PM
> > > > > To: Alex Williamson
> > > > > Cc: Kim Phillips; Bhushan Bharat-R65777; Wood Scott-B07421;
> > > > > Yoder Stuart-B08248; christoffer.d...@linaro.org;
> > > > > linux-kernel@vger.kernel.org; a.mota...@virtualopensystems.com;
> > > > > ag...@suse.de; Sethi Varun-B16395; peter.mayd...@linaro.org;
> > > > > santosh.shu...@linaro.org; k...@vger.kernel.org;
> > > > > gre...@linuxfoundation.org
> > > > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit
> > > > > binding via sysfs only
> > > > >
> > > > > On Mon, 2013-10-28 at 13:00 -0500, Scott Wood wrote:
> > > > > > On Mon, 2013-10-28 at 11:47 -0600, Alex Williamson wrote:
> > > > > > > On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> > > > > > > > Force the vfio-pci driver to only be bound explicitly via
> > > > > > > > sysfs to avoid conflics with other drivers in the event of a
> hotplug.
> > > > > > >
> > > > > > > We can't break userspace, so we can't disable the current
> > > > > > > method of binding devices to vfio-pci. We can add a new
> > > > > > > method and perhaps deprecate the existing mechanism to be
> > > > > > > removed at some point in the future. Thanks,
> > > > > >
> > > > > > I thought the existing method involved using sysfs bind, and
> > > > > > this was just eliminating a race. How does the bind get triggered
> currently?
> > > > >
> > > > > OK, so it seems it's relying on the write to new_id calling
> driver_attach().
> > > > > Sigh. I guess we could make driver-sysfs-bind-only be settable
> > > > > via sysfs, and have new-userspace set both that and PCI_ANY_ID
> > > > > (or the specific ID if userspace
> > > > > prefers) via new_id. The platform bus patches could continue as
> > > > > is, since there's no existing mechanism to break.
> > > >
> > > > What about changing the store_new_id() to bypass exact ids check
> > > > if driver
> > > have PCI_ANY_ID?
> > >
> > > I don't follow.
> >
> > store_new_id() function id defined as:
> >
> > static ssize_t store_new_id(struct device_driver *driver, const char
> > *buf, size_t count) {
> > struct pci_driver *pdrv = to_pci_driver(driver);
> > const struct pci_device_id *ids = pdrv->id_table;
> >
> >
> > /* Only accept driver_data values that match an existing id_table
> >entry */
> > if (ids) {
> > retval = -EINVAL;
> > while (ids->vendor || ids->subvendor || ids->class_mask) {
> > if (driver_data == ids->driver_data) {
> > retval = 0;
> > break;
> > }
> > ids++;
> > }
> > if (retval) /* No match */
> > return retval;
> > }
> >
> > retval = pci_add_dynid(pdrv, vendor, device, subvendor, subdevice,
> >class, class_mask, driver_data);
> >
> >
> > So when ids == NULL it does not check of vendor etc and calls
> > pci_add_dynid()
> which in turn calls driver_attach().
> >
> > If we change the above loop to break if ids->vendor == PCI_ANY_ID && ids-
> >subvendor == PCI_ANY_ID then also we will call pci_add_dyids().
>
> What problem are you trying to solve?
new_id interface to continue working as before.
-Bharat
>
> -Scott
>
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via sysfs only
> -Original Message-
> From: Wood Scott-B07421
> Sent: Tuesday, October 29, 2013 9:11 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder Stuart-B08248;
> christoffer.d...@linaro.org; linux-kernel@vger.kernel.org;
> a.mota...@virtualopensystems.com; ag...@suse.de; Sethi Varun-B16395;
> peter.mayd...@linaro.org; santosh.shu...@linaro.org; k...@vger.kernel.org;
> gre...@linuxfoundation.org
> Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via
> sysfs only
>
> On Mon, 2013-10-28 at 22:38 -0500, Bhushan Bharat-R65777 wrote:
> >
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Monday, October 28, 2013 11:40 PM
> > > To: Alex Williamson
> > > Cc: Kim Phillips; Bhushan Bharat-R65777; Wood Scott-B07421; Yoder
> > > Stuart-B08248; christoffer.d...@linaro.org;
> > > linux-kernel@vger.kernel.org; a.mota...@virtualopensystems.com;
> > > ag...@suse.de; Sethi Varun-B16395; peter.mayd...@linaro.org;
> > > santosh.shu...@linaro.org; k...@vger.kernel.org;
> > > gre...@linuxfoundation.org
> > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit
> > > binding via sysfs only
> > >
> > > On Mon, 2013-10-28 at 13:00 -0500, Scott Wood wrote:
> > > > On Mon, 2013-10-28 at 11:47 -0600, Alex Williamson wrote:
> > > > > On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> > > > > > Force the vfio-pci driver to only be bound explicitly via
> > > > > > sysfs to avoid conflics with other drivers in the event of a
> > > > > > hotplug.
> > > > >
> > > > > We can't break userspace, so we can't disable the current method
> > > > > of binding devices to vfio-pci. We can add a new method and
> > > > > perhaps deprecate the existing mechanism to be removed at some
> > > > > point in the future. Thanks,
> > > >
> > > > I thought the existing method involved using sysfs bind, and this
> > > > was just eliminating a race. How does the bind get triggered currently?
> > >
> > > OK, so it seems it's relying on the write to new_id calling
> > > driver_attach().
> > > Sigh. I guess we could make driver-sysfs-bind-only be settable via
> > > sysfs, and have new-userspace set both that and PCI_ANY_ID (or the
> > > specific ID if userspace
> > > prefers) via new_id. The platform bus patches could continue as is,
> > > since there's no existing mechanism to break.
> >
> > What about changing the store_new_id() to bypass exact ids check if driver
> have PCI_ANY_ID?
>
> I don't follow.
store_new_id() function id defined as:
static ssize_t store_new_id(struct device_driver *driver, const char *buf,
size_t count)
{
struct pci_driver *pdrv = to_pci_driver(driver);
const struct pci_device_id *ids = pdrv->id_table;
/* Only accept driver_data values that match an existing id_table
entry */
if (ids) {
retval = -EINVAL;
while (ids->vendor || ids->subvendor || ids->class_mask) {
if (driver_data == ids->driver_data) {
retval = 0;
break;
}
ids++;
}
if (retval) /* No match */
return retval;
}
retval = pci_add_dynid(pdrv, vendor, device, subvendor, subdevice,
class, class_mask, driver_data);
So when ids == NULL it does not check of vendor etc and calls pci_add_dynid()
which in turn calls driver_attach().
If we change the above loop to break if ids->vendor == PCI_ANY_ID &&
ids->subvendor == PCI_ANY_ID then also we will call pci_add_dyids().
-Bharat
>
> -Scott
>
RE: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via sysfs only
> -Original Message- > From: Wood Scott-B07421 > Sent: Monday, October 28, 2013 11:40 PM > To: Alex Williamson > Cc: Kim Phillips; Bhushan Bharat-R65777; Wood Scott-B07421; Yoder > Stuart-B08248; > christoffer.d...@linaro.org; linux-kernel@vger.kernel.org; > a.mota...@virtualopensystems.com; ag...@suse.de; Sethi Varun-B16395; > peter.mayd...@linaro.org; santosh.shu...@linaro.org; k...@vger.kernel.org; > gre...@linuxfoundation.org > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via > sysfs only > > On Mon, 2013-10-28 at 13:00 -0500, Scott Wood wrote: > > On Mon, 2013-10-28 at 11:47 -0600, Alex Williamson wrote: > > > On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote: > > > > Force the vfio-pci driver to only be bound explicitly via sysfs to > > > > avoid conflics with other drivers in the event of a hotplug. > > > > > > We can't break userspace, so we can't disable the current method of > > > binding devices to vfio-pci. We can add a new method and perhaps > > > deprecate the existing mechanism to be removed at some point in the > > > future. Thanks, > > > > I thought the existing method involved using sysfs bind, and this was > > just eliminating a race. How does the bind get triggered currently? > > OK, so it seems it's relying on the write to new_id calling driver_attach(). > Sigh. I guess we could make driver-sysfs-bind-only be settable via sysfs, and > have new-userspace set both that and PCI_ANY_ID (or the specific ID if > userspace > prefers) via new_id. The platform bus patches could continue as is, since > there's no existing mechanism to break. What about changing the store_new_id() to bypass exact ids check if driver have PCI_ANY_ID? -Bharat > > -Scott >
RE: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via sysfs only
> -Original Message-
> From: Kim Phillips [mailto:kim.phill...@linaro.org]
> Sent: Saturday, October 12, 2013 4:47 AM
> To: Wood Scott-B07421
> Cc: Bhushan Bharat-R65777; Wood Scott-B07421; Yoder Stuart-B08248;
> christoffer.d...@linaro.org; alex.william...@redhat.com; linux-
> ker...@vger.kernel.org; a.mota...@virtualopensystems.com; ag...@suse.de; Sethi
> Varun-B16395; peter.mayd...@linaro.org; santosh.shu...@linaro.org;
> k...@vger.kernel.org; gre...@linuxfoundation.org
> Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via
> sysfs only
>
> On Fri, 11 Oct 2013 15:43:40 -0500
> Scott Wood wrote:
>
> > On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> > > Force the vfio-pci driver to only be bound explicitly via sysfs to avoid
> > > conflics with other drivers in the event of a hotplug.
> > >
> > > Signed-off-by: Kim Phillips
> > > ---
> > > drivers/vfio/pci/vfio_pci.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > > index 6ab71b9..bdd7833 100644
> > > --- a/drivers/vfio/pci/vfio_pci.c
> > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > @@ -901,6 +901,9 @@ static struct pci_driver vfio_pci_driver = {
> > > .probe = vfio_pci_probe,
> > > .remove = vfio_pci_remove,
> > > .err_handler= &vfio_err_handlers,
> > > + .driver = {
> > > + .sysfs_bind_only = true,
> > > + },
> > > };
> > >
> > > static void __exit vfio_pci_cleanup(void)
> >
> > You also need to add a PCI_ANY_ID match in order to be able to get rid
> > of the new_id usage.
>
> thanks - see below.
>
> Can someone with a PCI bus test this? Bharat?
Hello Kim,
I can test that we can get rid of new_id and use "bind" to bind the device to
vfio_pci.
Other thing is generating hotplug, or reorder the driver registration by
tweaking Makefile to test sysfs_bind_only way to bind is not yet tested.
Thanks
-Bharat
>
> Kim
>
> From a8d6c12f2ec763c2ac7fd384a3397c370cc1b932 Mon Sep 17 00:00:00 2001
> From: Kim Phillips
> Date: Thu, 10 Oct 2013 22:16:34 -0500
> Subject: [PATCH 3/4 v2] VFIO: pci: amend vfio-pci for explicit binding via
> sysfs
> only
>
> Force the vfio-pci driver to only be bound explicitly via sysfs to avoid
> conflics with other drivers in the event of a hotplug. Also replace
> the only dynamic ids assignment with a table with a single PCI_ANY_ID
> entry since writing the sysfs bind file without having to specify ids
> via the new_id file first should no longer be necessary.
>
> Signed-off-by: Kim Phillips
> ---
> drivers/vfio/pci/vfio_pci.c | 12 +++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 6ab71b9..c5b434f 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -895,12 +895,22 @@ static struct pci_error_handlers vfio_err_handlers = {
> .error_detected = vfio_pci_aer_err_detected,
> };
>
> +static DEFINE_PCI_DEVICE_TABLE(vfio_pci_id_table) = {
> +{ PCI_DEVICE(PCI_ANY_ID, PCI_ANY_ID) },
> +{ 0 }
> +};
> +
> +MODULE_DEVICE_TABLE(pci, vfio_pci_id_table);
> +
> static struct pci_driver vfio_pci_driver = {
> .name = "vfio-pci",
> - .id_table = NULL, /* only dynamic ids */
> + .id_table = vfio_pci_id_table, /* no dynamic ids */
> .probe = vfio_pci_probe,
> .remove = vfio_pci_remove,
> .err_handler= &vfio_err_handlers,
> + .driver = {
> + .sysfs_bind_only = true, /* bind only via sysfs */
> + },
> };
>
> static void __exit vfio_pci_cleanup(void)
> --
> 1.8.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe devices
> >
> > > >
> > > >
> > > > > -Original Message-
> > > > > From: Sethi Varun-B16395
> > > > > Sent: Wednesday, October 16, 2013 4:53 PM
> > > > > To: j...@8bytes.org; io...@lists.linux-foundation.org; linuxppc-
> > > > > d...@lists.ozlabs.org; linux-kernel@vger.kernel.org; Yoder
> > > > > Stuart-B08248; Wood Scott-B07421; alex.william...@redhat.com;
> > > > > Bhushan
> > > > > Bharat-R65777
> > > > > Cc: Sethi Varun-B16395
> > > > > Subject: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for
> > > > > PCIe devices
> > > > >
> > > > > Once the PCIe device assigned to a guest VM (via VFIO) gets
> > > > > detached from the iommu domain (when guest terminates), its PAMU
> > > > > table entry is disabled. So, this would prevent the device from
> > > > > being used once it's
> > > > assigned back to the host.
> > > > >
> > > > > This patch allows for creation of a default DMA window
> > > > > corresponding to the device and subsequently enabling the PAMU
> > > > > table entry. Before we enable the entry, we ensure that the
> > > > > device's bus master capability is disabled (device quiesced).
> > > > >
> > > > > Signed-off-by: Varun Sethi
> > > > > ---
> > > > > drivers/iommu/fsl_pamu.c| 43
> > ---
> > > > -
> > > > > drivers/iommu/fsl_pamu.h|1 +
> > > > > drivers/iommu/fsl_pamu_domain.c | 46
> > > > ---
> > > > > 3 files changed, 78 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c
> > > > > index
> > > > > cba0498..fb4a031 100644
> > > > > --- a/drivers/iommu/fsl_pamu.c
> > > > > +++ b/drivers/iommu/fsl_pamu.c
> > > > > @@ -225,6 +225,21 @@ static struct paace *pamu_get_spaace(struct
> > > > > paace *paace,
> > > > > u32 wnum)
> > > > > return spaace;
> > > > > }
> > > > >
> > > > > +/*
> > > > > + * Defaul PPAACE settings for an LIODN.
> > > > > + */
> > > > > +static void setup_default_ppaace(struct paace *ppaace) {
> > > > > + pamu_init_ppaace(ppaace);
> > > > > + /* window size is 2^(WSE+1) bytes */
> > > > > + set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> > > > > + ppaace->wbah = 0;
> > > > > + set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> > > > > + set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> > > > > + PAACE_ATM_NO_XLATE);
> > > > > + set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> > > > > + PAACE_AP_PERMS_ALL);
> > > > > +}
> > > > > /**
> > > > > * pamu_get_fspi_and_allocate() - Allocates fspi index and
> > > > > reserves
> > > > subwindows
> > > > > *required for primary PAACE in
> > the
> > > > secondary
> > > > > @@ -253,6 +268,24 @@ static unsigned long
> > > > > pamu_get_fspi_and_allocate(u32
> > > > > subwin_cnt)
> > > > > return (spaace_addr - (unsigned long)spaact) / (sizeof(struct
> > > > > paace)); }
> > > > >
> > > > > +/* Reset the PAACE entry to the default state */ void
> > > > > +enable_default_dma_window(int liodn) {
> > > > > + struct paace *ppaace;
> > > > > +
> > > > > + ppaace = pamu_get_ppaace(liodn);
> > > > > + if (!ppaace) {
> > > > > + pr_debug("Invalid liodn entry\n");
> > > > > + return;
> > > > > + }
> > > > > +
> > > > > + memset(ppaace, 0, sizeof(struct paace));
> > > > > +
> > > > > + setup_default_ppaace(ppaace);
> > > > > + mb();
> > > > > + pamu_enable_liodn(liodn);
> > > > > +}
> > > > > +
> > > > > /* Release the subwindows reserved for a particular LIODN */
> > > >
RE: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe devices
> >
> >
> > > -Original Message-
> > > From: Sethi Varun-B16395
> > > Sent: Wednesday, October 16, 2013 4:53 PM
> > > To: j...@8bytes.org; io...@lists.linux-foundation.org; linuxppc-
> > > d...@lists.ozlabs.org; linux-kernel@vger.kernel.org; Yoder
> > > Stuart-B08248; Wood Scott-B07421; alex.william...@redhat.com;
> > > Bhushan
> > > Bharat-R65777
> > > Cc: Sethi Varun-B16395
> > > Subject: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for
> > > PCIe devices
> > >
> > > Once the PCIe device assigned to a guest VM (via VFIO) gets detached
> > > from the iommu domain (when guest terminates), its PAMU table entry
> > > is disabled. So, this would prevent the device from being used once
> > > it's
> > assigned back to the host.
> > >
> > > This patch allows for creation of a default DMA window corresponding
> > > to the device and subsequently enabling the PAMU table entry. Before
> > > we enable the entry, we ensure that the device's bus master
> > > capability is disabled (device quiesced).
> > >
> > > Signed-off-by: Varun Sethi
> > > ---
> > > drivers/iommu/fsl_pamu.c| 43 ---
> > -
> > > drivers/iommu/fsl_pamu.h|1 +
> > > drivers/iommu/fsl_pamu_domain.c | 46
> > ---
> > > 3 files changed, 78 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c
> > > index
> > > cba0498..fb4a031 100644
> > > --- a/drivers/iommu/fsl_pamu.c
> > > +++ b/drivers/iommu/fsl_pamu.c
> > > @@ -225,6 +225,21 @@ static struct paace *pamu_get_spaace(struct
> > > paace *paace,
> > > u32 wnum)
> > > return spaace;
> > > }
> > >
> > > +/*
> > > + * Defaul PPAACE settings for an LIODN.
> > > + */
> > > +static void setup_default_ppaace(struct paace *ppaace) {
> > > + pamu_init_ppaace(ppaace);
> > > + /* window size is 2^(WSE+1) bytes */
> > > + set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> > > + ppaace->wbah = 0;
> > > + set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> > > + set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> > > + PAACE_ATM_NO_XLATE);
> > > + set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> > > + PAACE_AP_PERMS_ALL);
> > > +}
> > > /**
> > > * pamu_get_fspi_and_allocate() - Allocates fspi index and reserves
> > subwindows
> > > *required for primary PAACE in the
> > secondary
> > > @@ -253,6 +268,24 @@ static unsigned long
> > > pamu_get_fspi_and_allocate(u32
> > > subwin_cnt)
> > > return (spaace_addr - (unsigned long)spaact) / (sizeof(struct
> > > paace)); }
> > >
> > > +/* Reset the PAACE entry to the default state */ void
> > > +enable_default_dma_window(int liodn) {
> > > + struct paace *ppaace;
> > > +
> > > + ppaace = pamu_get_ppaace(liodn);
> > > + if (!ppaace) {
> > > + pr_debug("Invalid liodn entry\n");
> > > + return;
> > > + }
> > > +
> > > + memset(ppaace, 0, sizeof(struct paace));
> > > +
> > > + setup_default_ppaace(ppaace);
> > > + mb();
> > > + pamu_enable_liodn(liodn);
> > > +}
> > > +
> > > /* Release the subwindows reserved for a particular LIODN */ void
> > > pamu_free_subwins(int liodn) { @@ -752,15 +785,7 @@ static void
> > > __init
> > > setup_liodns(void)
> > > continue;
> > > }
> > > ppaace = pamu_get_ppaace(liodn);
> > > - pamu_init_ppaace(ppaace);
> > > - /* window size is 2^(WSE+1) bytes */
> > > - set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> > > - ppaace->wbah = 0;
> > > - set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> > > - set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> > > - PAACE_ATM_NO_XLATE);
> > > - set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> > > - PAACE_AP_PERMS_ALL);
> > > + setup_default_ppaace(ppaace);
> > > if (of_device_is_
RE: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe devices
> -Original Message-
> From: Sethi Varun-B16395
> Sent: Wednesday, October 16, 2013 4:53 PM
> To: j...@8bytes.org; io...@lists.linux-foundation.org; linuxppc-
> d...@lists.ozlabs.org; linux-kernel@vger.kernel.org; Yoder Stuart-B08248; Wood
> Scott-B07421; alex.william...@redhat.com; Bhushan Bharat-R65777
> Cc: Sethi Varun-B16395
> Subject: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe devices
>
> Once the PCIe device assigned to a guest VM (via VFIO) gets detached from the
> iommu domain (when guest terminates), its PAMU table entry is disabled. So,
> this
> would prevent the device from being used once it's assigned back to the host.
>
> This patch allows for creation of a default DMA window corresponding to the
> device and subsequently enabling the PAMU table entry. Before we enable the
> entry, we ensure that the device's bus master capability is disabled (device
> quiesced).
>
> Signed-off-by: Varun Sethi
> ---
> drivers/iommu/fsl_pamu.c| 43
> drivers/iommu/fsl_pamu.h|1 +
> drivers/iommu/fsl_pamu_domain.c | 46
> ---
> 3 files changed, 78 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c index
> cba0498..fb4a031 100644
> --- a/drivers/iommu/fsl_pamu.c
> +++ b/drivers/iommu/fsl_pamu.c
> @@ -225,6 +225,21 @@ static struct paace *pamu_get_spaace(struct paace *paace,
> u32 wnum)
> return spaace;
> }
>
> +/*
> + * Defaul PPAACE settings for an LIODN.
> + */
> +static void setup_default_ppaace(struct paace *ppaace) {
> + pamu_init_ppaace(ppaace);
> + /* window size is 2^(WSE+1) bytes */
> + set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> + ppaace->wbah = 0;
> + set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> + set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> + PAACE_ATM_NO_XLATE);
> + set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> + PAACE_AP_PERMS_ALL);
> +}
> /**
> * pamu_get_fspi_and_allocate() - Allocates fspi index and reserves
> subwindows
> *required for primary PAACE in the secondary
> @@ -253,6 +268,24 @@ static unsigned long pamu_get_fspi_and_allocate(u32
> subwin_cnt)
> return (spaace_addr - (unsigned long)spaact) / (sizeof(struct paace));
> }
>
> +/* Reset the PAACE entry to the default state */ void
> +enable_default_dma_window(int liodn) {
> + struct paace *ppaace;
> +
> + ppaace = pamu_get_ppaace(liodn);
> + if (!ppaace) {
> + pr_debug("Invalid liodn entry\n");
> + return;
> + }
> +
> + memset(ppaace, 0, sizeof(struct paace));
> +
> + setup_default_ppaace(ppaace);
> + mb();
> + pamu_enable_liodn(liodn);
> +}
> +
> /* Release the subwindows reserved for a particular LIODN */ void
> pamu_free_subwins(int liodn) { @@ -752,15 +785,7 @@ static void __init
> setup_liodns(void)
> continue;
> }
> ppaace = pamu_get_ppaace(liodn);
> - pamu_init_ppaace(ppaace);
> - /* window size is 2^(WSE+1) bytes */
> - set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> - ppaace->wbah = 0;
> - set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> - set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> - PAACE_ATM_NO_XLATE);
> - set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> - PAACE_AP_PERMS_ALL);
> + setup_default_ppaace(ppaace);
> if (of_device_is_compatible(node, "fsl,qman-portal"))
> setup_qbman_paace(ppaace, QMAN_PORTAL_PAACE);
> if (of_device_is_compatible(node, "fsl,qman")) diff
> --git
> a/drivers/iommu/fsl_pamu.h b/drivers/iommu/fsl_pamu.h index 8fc1a12..0edc
> 100644
> --- a/drivers/iommu/fsl_pamu.h
> +++ b/drivers/iommu/fsl_pamu.h
> @@ -406,5 +406,6 @@ void get_ome_index(u32 *omi_index, struct device *dev);
> int
> pamu_update_paace_stash(int liodn, u32 subwin, u32 value); int
> pamu_disable_spaace(int liodn, u32 subwin);
> u32 pamu_get_max_subwin_cnt(void);
> +void enable_default_dma_window(int liodn);
>
> #endif /* __FSL_PAMU_H */
> diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
> index 966ae70..dd6cafc 100644
> --- a/drivers/iommu/fsl_pamu_domain.c
> +++ b/drivers/iommu/fsl_p
RE: RFC: (re-)binding the VFIO platform driver to a platform device
> -Original Message- > From: Wood Scott-B07421 > Sent: Thursday, October 10, 2013 8:53 PM > To: Bhushan Bharat-R65777 > Cc: Wood Scott-B07421; Yoder Stuart-B08248; Kim Phillips; Christoffer Dall; > Alex > Williamson; linux-kernel@vger.kernel.org; a.mota...@virtualopensystems.com; > ag...@suse.de; Sethi Varun-B16395; peter.mayd...@linaro.org; > santosh.shu...@linaro.org; k...@vger.kernel.org; gre...@linuxfoundation.org > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform device > > On Thu, 2013-10-10 at 02:45 -0500, Bhushan Bharat-R65777 wrote: > > > > > -Original Message- > > > From: Wood Scott-B07421 > > > Sent: Thursday, October 10, 2013 1:33 AM > > > To: Yoder Stuart-B08248 > > > Cc: Wood Scott-B07421; Kim Phillips; Christoffer Dall; Alex > > > Williamson; linux- ker...@vger.kernel.org; > > > a.mota...@virtualopensystems.com; ag...@suse.de; Sethi Varun-B16395; > > > Bhushan Bharat-R65777; peter.mayd...@linaro.org; > > > santosh.shu...@linaro.org; k...@vger.kernel.org; > > > gre...@linuxfoundation.org > > > Subject: Re: RFC: (re-)binding the VFIO platform driver to a > > > platform device > > > > > > On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote: > > > > Ah, think I understand now...yes that works as well, and would be > > > > less intrustive. So are you writing a patch? :) > > > > > > I've been meaning to since the previous round of discussion, but I've been > busy. > > > Would someone else be able to test it in the context of using it for VFIO? > > > > I wish I could have but I do not have vfio-platform stuff. > > VFIO PCI without new_id would also be a useful test. I will do that :) -Bharat > > -Scott >
RE: RFC: (re-)binding the VFIO platform driver to a platform device
> -Original Message- > From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On Behalf > Of > Kim Phillips > Sent: Thursday, October 10, 2013 8:36 AM > To: Wood Scott-B07421 > Cc: Yoder Stuart-B08248; Wood Scott-B07421; christoffer.d...@linaro.org; > alex.william...@redhat.com; linux-kernel@vger.kernel.org; > a.mota...@virtualopensystems.com; ag...@suse.de; Sethi Varun-B16395; Bhushan > Bharat-R65777; peter.mayd...@linaro.org; santosh.shu...@linaro.org; > k...@vger.kernel.org; gre...@linuxfoundation.org > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform device > > On Wed, 9 Oct 2013 15:03:19 -0500 > Scott Wood wrote: > > > On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote: > > > > From: Wood Scott-B07421 > > > > Sent: Wednesday, October 09, 2013 2:22 PM > > > > > > > > On Wed, 2013-10-09 at 14:02 -0500, Yoder Stuart-B08248 wrote: > > > > > Have been thinking about this issue some more. As Scott > > > > > mentioned, > > thanks for bringing this up again. > > > > > There's already a "bool suppress_bind_attrs" to prevent sysfs > > > > bind/unbind. I suggested a similar flag to mean the oppsosite -- > > > > bind > > > > *only* through sysfs. Greg KH was skeptical and wanted to see a > > > > patch before any further discussion. > > > > > > Ah, think I understand now...yes that works as well, and would be > > > less intrustive. So are you writing a patch? :) > > > > I've been meaning to since the previous round of discussion, but I've > > been busy. Would someone else be able to test it in the context of > > using it for VFIO? > > yes - see below. > > > Otherwise, that looks about right, for the driver side (though > > driver_attach could error out earlier rather than testing it inside > > the loop). > > I've made the changes you suggested and tested the resulting diff below on an > arndale board. I successfully performed the following sequence of commands > after first changing the i2c@12C8 node in the device tree to be > exclusively > compatible with "vfio": > > === > # ls -l /sys/bus/platform/drivers/vfio-platform/ > total 0 > --w--- 1 root root 4096 Sep 24 19:17 bind > --w--- 1 root root 4096 Sep 24 19:13 uevent > --w--- 1 root root 4096 Sep 24 19:18 unbind # ls -l > /sys/bus/platform/drivers/s3c-i2c total 0 > lrwxrwxrwx 1 root root0 Sep 24 19:11 12c6.i2c -> > ../../../../devices/12c6.i2c > lrwxrwxrwx 1 root root0 Sep 24 19:11 12c9.i2c -> > ../../../../devices/12c9.i2c > lrwxrwxrwx 1 root root0 Sep 24 19:20 12ce.i2c -> > ../../../../devices/12ce.i2c > --w--- 1 root root 4096 Sep 24 19:18 bind > --w--- 1 root root 4096 Sep 24 19:11 uevent > --w--- 1 root root 4096 Sep 24 19:17 unbind # ls -l > /sys/devices/12c8.i2c/driver # this is the one with the 'vfio' compatible > ls: cannot access /sys/devices/12c8.i2c/driver: No such file or directory > # > ls -l /sys/devices/12ce.i2c/driver lrwxrwxrwx 1 root root 0 Sep 24 19:18 > /sys/devices/12ce.i2c/driver -> ../../bus/platform/drivers/s3c-i2c > # echo 12ce.i2c > /sys/bus/platform/drivers/s3c-i2c/unbind > # ls -l /sys/devices/12ce.i2c/driver > ls: cannot access /sys/devices/12ce.i2c/driver: No such file or directory > # > echo 12ce.i2c > /sys/bus/platform/drivers/vfio-platform/bind > # ls -l /sys/devices/12ce.i2c/driver lrwxrwxrwx 1 root root 0 Sep 24 19:21 > /sys/devices/12ce.i2c/driver -> ../../bus/platform/drivers/vfio-platform > # echo 12ce.i2c > /sys/bus/platform/drivers/vfio-platform/unbind > # ls -l /sys/devices/12ce.i2c/driver # echo 12ce.i2c > > /sys/bus/platform/drivers/s3c-i2c/bind > [ 722.137524] s3c-i2c 12ce.i2c: slave address 0x38 [ 722.141037] s3c-i2c > 12ce.i2c: bus frequency set to 65 KHz [ 722.150605] s3c-i2c 12ce.i2c: > i2c-8: S3C I2C adapter # ls -l /sys/devices/12ce.i2c/driver lrwxrwxrwx 1 > root root 0 Sep 24 19:21 /sys/devices/12ce.i2c/driver -> > ../../bus/platform/drivers/s3c-i2c > # > > > so it's correctly not allowing 'vfio' driver to bind to a device tree > compatible > it's declared, and it then can bind the i2c @ 12ce device to the vfio- > platform driver, and unbind and bind it back to the i2c driver. > > For clarity's sake, before this diff, the command: > > echo 12ce.i2c > /sys/bus/platform/drivers/vfio-platform/bind > > would error with: > > echo: writ
RE: RFC: (re-)binding the VFIO platform driver to a platform device
> -Original Message-
> From: Wood Scott-B07421
> Sent: Thursday, October 10, 2013 1:33 AM
> To: Yoder Stuart-B08248
> Cc: Wood Scott-B07421; Kim Phillips; Christoffer Dall; Alex Williamson; linux-
> ker...@vger.kernel.org; a.mota...@virtualopensystems.com; ag...@suse.de; Sethi
> Varun-B16395; Bhushan Bharat-R65777; peter.mayd...@linaro.org;
> santosh.shu...@linaro.org; k...@vger.kernel.org; gre...@linuxfoundation.org
> Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform device
>
> On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote:
> >
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Wednesday, October 09, 2013 2:22 PM
> > > To: Yoder Stuart-B08248
> > > Cc: Wood Scott-B07421; Kim Phillips; Christoffer Dall; Alex
> > > Williamson; linux-kernel@vger.kernel.org;
> > > a.mota...@virtualopensystems.com; ag...@suse.de; Sethi Varun-B16395;
> > > Bhushan Bharat-R65777; peter.mayd...@linaro.org;
> > > santosh.shu...@linaro.org; k...@vger.kernel.org;
> > > gre...@linuxfoundation.org
> > > Subject: Re: RFC: (re-)binding the VFIO platform driver to a
> > > platform device
> > >
> > > On Wed, 2013-10-09 at 14:02 -0500, Yoder Stuart-B08248 wrote:
> > > > Have been thinking about this issue some more. As Scott
> > > > mentioned, 'wildcard' matching for a driver can be fairly done in
> > > > the platform bus driver. We could add a new flag to the platform driver
> struct:
> > > >
> > > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > > > index 4f8bef3..4d6cf14 100644
> > > > --- a/drivers/base/platform.c
> > > > +++ b/drivers/base/platform.c
> > > > @@ -727,6 +727,10 @@ static int platform_match(struct device *dev,
> > > struct device_driver *drv)
> > > > struct platform_device *pdev = to_platform_device(dev);
> > > > struct platform_driver *pdrv = to_platform_driver(drv);
> > > >
> > > > + /* the driver matches any device */
> > > > + if (pdrv->match_any)
> > > > + return 1;
> > > > +
> > > > /* Attempt an OF style match first */
> > > > if (of_driver_match_device(dev, drv))
> > > > return 1;
> > > >
> > > > However, the more problematic issue is that a bus driver has no
> > > > way to differentiate from an explicit bind request via sysfs and a
> > > > bind that happened through bus probing.
> > >
> > > Again, I think the wildcard match should be orthogonal to "don't
> > > bind by default" as far as the mechanism goes.
> > >
> > > There's already a "bool suppress_bind_attrs" to prevent sysfs
> > > bind/unbind. I suggested a similar flag to mean the oppsosite --
> > > bind
> > > *only* through sysfs. Greg KH was skeptical and wanted to see a
> > > patch before any further discussion.
> >
> > Ah, think I understand now...yes that works as well, and would be
> > less intrustive. So are you writing a patch? :)
>
> I've been meaning to since the previous round of discussion, but I've been
> busy.
> Would someone else be able to test it in the context of using it for VFIO?
I wish I could have but I do not have vfio-platform stuff.
>
> > It would be something like this, right?
> >
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c index
> > 35fa368..c9a61ea 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -389,7 +389,7 @@ static int __device_attach(struct device_driver
> > *drv, void *data) {
> > struct device *dev = data;
> >
> > - if (!driver_match_device(drv, dev))
> > + if (!drv->explicit_bind_only && !driver_match_device(drv,
> > + dev))
> > return 0;
>
> if (drv->explicit_bind_only || !driver_match_device(drv, dev))
> return 0;
Scott,
I am trying to understand what you are proposing here (example "DEVICE" can be
handled by "DRIVER1" and "VFIO-PLATFORM-DRIVER"):
- By default drv->explicit_bind_only will be clear in all drivers.
- By default device->explicit_bind_only will also be clear for all devices.
- On boot, matching devices will bound to the respective driver (DEVICE >==>
DRIVER1).
This will never bound with VFIO-PLATFORM-DRIVER. So far same as before.
- Via Sysfs interface set drv->explicit_bind_only
RE: [PATCH 1/7] powerpc: Add interface to get msi region information
> -Original Message-
> From: Wood Scott-B07421
> Sent: Wednesday, October 09, 2013 4:27 AM
> To: Bhushan Bharat-R65777
> Cc: alex.william...@redhat.com; j...@8bytes.org; b...@kernel.crashing.org;
> ga...@kernel.crashing.org; linux-kernel@vger.kernel.org; linuxppc-
> d...@lists.ozlabs.org; linux-...@vger.kernel.org; ag...@suse.de;
> io...@lists.linux-foundation.org; Bhushan Bharat-R65777
> Subject: Re: [PATCH 1/7] powerpc: Add interface to get msi region information
>
> On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> > @@ -376,6 +405,7 @@ static int fsl_of_msi_probe(struct platform_device *dev)
> > int len;
> > u32 offset;
> > static const u32 all_avail[] = { 0, NR_MSI_IRQS };
> > + static int bank_index;
> >
> > match = of_match_device(fsl_of_msi_ids, &dev->dev);
> > if (!match)
> > @@ -419,8 +449,8 @@ static int fsl_of_msi_probe(struct platform_device *dev)
> > dev->dev.of_node->full_name);
> > goto error_out;
> > }
> > - msi->msiir_offset =
> > - features->msiir_offset + (res.start & 0xf);
> > + msi->msiir = res.start + features->msiir_offset;
> > + printk("msi->msiir = %llx\n", msi->msiir);
>
> dev_dbg or remove
Oops, sorry it was leftover of debugging :(
>
> > }
> >
> > msi->feature = features->fsl_pic_ip; @@ -470,6 +500,7 @@ static int
> > fsl_of_msi_probe(struct platform_device *dev)
> > }
> > }
> >
> > + msi->bank_index = bank_index++;
>
> What if multiple MSIs are boing probed in parallel?
Ohh, I have not thought that it can be called in parallel
> bank_index is not atomic.
Will declare bank_intex as atomic_t and use atomic_inc_return(&bank_index)
>
> > diff --git a/arch/powerpc/sysdev/fsl_msi.h
> > b/arch/powerpc/sysdev/fsl_msi.h index 8225f86..6bd5cfc 100644
> > --- a/arch/powerpc/sysdev/fsl_msi.h
> > +++ b/arch/powerpc/sysdev/fsl_msi.h
> > @@ -29,12 +29,19 @@ struct fsl_msi {
> > struct irq_domain *irqhost;
> >
> > unsigned long cascade_irq;
> > -
> > - u32 msiir_offset; /* Offset of MSIIR, relative to start of CCSR */
> > + dma_addr_t msiir; /* MSIIR Address in CCSR */
>
> Are you sure dma_addr_t is right here, versus phys_addr_t? It implies that
> it's
> the output of the DMA API, but I don't think the DMA API is used in the MSI
> driver. Perhaps it should be, but we still want the raw physical address to
> pass on to VFIO.
Looking through the conversation I will make this phys_addr_t
>
> > void __iomem *msi_regs;
> > u32 feature;
> > int msi_virqs[NR_MSI_REG];
> >
> > + /*
> > +* During probe each bank is assigned a index number.
> > +* index number ranges from 0 to 2^32.
> > +* Example MSI bank 1 = 0
> > +* MSI bank 2 = 1, and so on.
> > +*/
> > + int bank_index;
>
> 2^32 doesn't fit in "int" (nor does 2^32 - 1).
Right :(
>
> Just say that indices start at 0.
Will correct this
Thanks
-Bharat
>
> -Scott
>
RE: [PATCH 1/7] powerpc: Add interface to get msi region information
> -Original Message- > From: j...@8bytes.org [mailto:j...@8bytes.org] > Sent: Tuesday, October 08, 2013 10:32 PM > To: Bjorn Helgaas > Cc: Bhushan Bharat-R65777; alex.william...@redhat.com; > b...@kernel.crashing.org; > ga...@kernel.crashing.org; linux-kernel@vger.kernel.org; linuxppc- > d...@lists.ozlabs.org; linux-...@vger.kernel.org; ag...@suse.de; Wood Scott- > B07421; io...@lists.linux-foundation.org > Subject: Re: [PATCH 1/7] powerpc: Add interface to get msi region information > > On Tue, Oct 08, 2013 at 10:47:49AM -0600, Bjorn Helgaas wrote: > > I still have no idea what an "aperture type IOMMU" is, other than that > > it is "different." > > An aperture based IOMMU is basically any GART-like IOMMU which can only remap > a > small window (the aperture) of the DMA address space. DMA outside of that > window > is either blocked completly or passed through untranslated. It is completely blocked for Freescale PAMU. So for this type of iommu what we have to do is to create a MSI mapping just after guest physical address, Example: guest have a 512M of memory then we create window of 1G (because of power of 2 requirement), then we have to FIT MSI just after 512M of guest. And for that we need 1) to know the physical address of MSI's in interrupt controller (for that this patch was all about of). 2) When guest enable MSI interrupt then we write MSI-address and MSI-DATA in device. The discussion with Alex Williamson is about that interface. Thanks -Bharat > > > Joerg > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/7] iommu: add api to get iommu_domain of a device
> > > Do you really want module dependencies between vfio and your core
> > > kernel MSI setup? Look at the vfio external user interface that we've
> already defined.
> > > That allows other components of the kernel to get a proper reference
> > > to a vfio group. From there you can work out how to get what you
> > > want. Another alternative is that vfio could register an MSI to
> > > IOVA mapping with architecture code when the mapping is created.
> > > The MSI setup path could then do a lookup in architecture code for
> > > the mapping. You could even store the MSI to IOVA mapping in VFIO
> > > and create an interface where SET_IRQ passes that mapping into setup code.
> >
> > Ok, What I want is to get IOVA associated with a physical address
> > (physical address of MSI-bank).
> > And currently I do not see a way to know IOVA of a physical address
> > and doing all this domain get and then search through all of
> > iommu-windows of that domain.
> >
> > What if we add an iommu-API which can return the IOVA mapping of a
> > physical address. Current use case is setting up MSI's for aperture
> > type of IOMMU also getting a phys_to_iova() mapping is independent of
> > VFIO, your thought?
>
> A physical address can be mapped to multiple IOVAs, so the interface seems
> flawed by design. It also has the same problem as above, it's a backdoor that
> can be called asynchronous to the owner of the domain, so what reason is there
> to believe the result? It just replaces an iommu_domain pointer with an IOVA.
> VFIO knows this mapping, so why are we trying to go behind its back and ask
> the
> IOMMU?
IOMMU is the final place where mapping is created, so may be today it is
calling on behalf of VFIO, tomorrow it can be for normal Linux or some other
interface. But I am fine to directly talk to vfio and will not try to solve a
problem which does not exists today.
MSI subsystem knows pdev (pci device) and physical address, then what interface
it will use to get the IOVA from VFIO?
Thanks
-Bharat
> Thanks,
>
> Alex
>
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH 2/7] iommu: add api to get iommu_domain of a device
> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Friday, October 04, 2013 11:42 PM
> To: Bhushan Bharat-R65777
> Cc: j...@8bytes.org; b...@kernel.crashing.org; ga...@kernel.crashing.org;
> linux-
> ker...@vger.kernel.org; linuxppc-...@lists.ozlabs.org; linux-
> p...@vger.kernel.org; ag...@suse.de; Wood Scott-B07421; iommu@lists.linux-
> foundation.org
> Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device
>
> On Fri, 2013-10-04 at 17:23 +, Bhushan Bharat-R65777 wrote:
> >
> > > -Original Message-
> > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > Sent: Friday, October 04, 2013 10:43 PM
> > > To: Bhushan Bharat-R65777
> > > Cc: j...@8bytes.org; b...@kernel.crashing.org;
> > > ga...@kernel.crashing.org; linux- ker...@vger.kernel.org;
> > > linuxppc-...@lists.ozlabs.org; linux- p...@vger.kernel.org;
> > > ag...@suse.de; Wood Scott-B07421; iommu@lists.linux- foundation.org
> > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > device
> > >
> > > On Fri, 2013-10-04 at 16:47 +, Bhushan Bharat-R65777 wrote:
> > > >
> > > > > -----Original Message-
> > > > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > > > Sent: Friday, October 04, 2013 9:15 PM
> > > > > To: Bhushan Bharat-R65777
> > > > > Cc: j...@8bytes.org; b...@kernel.crashing.org;
> > > > > ga...@kernel.crashing.org; linux- ker...@vger.kernel.org;
> > > > > linuxppc-...@lists.ozlabs.org; linux- p...@vger.kernel.org;
> > > > > ag...@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > > > > foundation.org
> > > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > > > device
> > > > >
> > > > > On Fri, 2013-10-04 at 09:54 +, Bhushan Bharat-R65777 wrote:
> > > > > >
> > > > > > > -Original Message-
> > > > > > > From: linux-pci-ow...@vger.kernel.org
> > > > > > > [mailto:linux-pci-ow...@vger.kernel.org]
> > > > > > > On Behalf Of Alex Williamson
> > > > > > > Sent: Wednesday, September 25, 2013 10:16 PM
> > > > > > > To: Bhushan Bharat-R65777
> > > > > > > Cc: j...@8bytes.org; b...@kernel.crashing.org;
> > > > > > > ga...@kernel.crashing.org; linux- ker...@vger.kernel.org;
> > > > > > > linuxppc-...@lists.ozlabs.org; linux- p...@vger.kernel.org;
> > > > > > > ag...@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > > > > > > foundation.org; Bhushan Bharat-R65777
> > > > > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain
> > > > > > > of a device
> > > > > > >
> > > > > > > On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> > > > > > > > This api return the iommu domain to which the device is
> > > > > > > > attached.
> > > > > > > > The iommu_domain is required for making API calls related to
> iommu.
> > > > > > > > Follow up patches which use this API to know iommu maping.
> > > > > > > >
> > > > > > > > Signed-off-by: Bharat Bhushan
> > > > > > > >
> > > > > > > > ---
> > > > > > > > drivers/iommu/iommu.c | 10 ++
> > > > > > > > include/linux/iommu.h |7 +++
> > > > > > > > 2 files changed, 17 insertions(+), 0 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > > > > > index
> > > > > > > > fbe9ca7..6ac5f50 100644
> > > > > > > > --- a/drivers/iommu/iommu.c
> > > > > > > > +++ b/drivers/iommu/iommu.c
> > > > > > > > @@ -696,6 +696,16 @@ void iommu_detach_device(struct
> > > > > > > > iommu_domain *domain, struct device *dev) }
> > > > > > > > EXPORT_SYMBOL_GPL(iommu_detach_device);
> > > > > > > >
> > > > > > > > +struct iommu_domain *iommu_get_dev_domain(struct device *dev) {
> > > > > > > > + struct iommu_ops *ops = dev->bus->iommu_ops;
>
RE: [PATCH 2/7] iommu: add api to get iommu_domain of a device
> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Friday, October 04, 2013 10:43 PM
> To: Bhushan Bharat-R65777
> Cc: j...@8bytes.org; b...@kernel.crashing.org; ga...@kernel.crashing.org;
> linux-
> ker...@vger.kernel.org; linuxppc-...@lists.ozlabs.org; linux-
> p...@vger.kernel.org; ag...@suse.de; Wood Scott-B07421; iommu@lists.linux-
> foundation.org
> Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device
>
> On Fri, 2013-10-04 at 16:47 +, Bhushan Bharat-R65777 wrote:
> >
> > > -Original Message-
> > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > Sent: Friday, October 04, 2013 9:15 PM
> > > To: Bhushan Bharat-R65777
> > > Cc: j...@8bytes.org; b...@kernel.crashing.org;
> > > ga...@kernel.crashing.org; linux- ker...@vger.kernel.org;
> > > linuxppc-...@lists.ozlabs.org; linux- p...@vger.kernel.org;
> > > ag...@suse.de; Wood Scott-B07421; iommu@lists.linux- foundation.org
> > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > device
> > >
> > > On Fri, 2013-10-04 at 09:54 +, Bhushan Bharat-R65777 wrote:
> > > >
> > > > > -Original Message-
> > > > > From: linux-pci-ow...@vger.kernel.org
> > > > > [mailto:linux-pci-ow...@vger.kernel.org]
> > > > > On Behalf Of Alex Williamson
> > > > > Sent: Wednesday, September 25, 2013 10:16 PM
> > > > > To: Bhushan Bharat-R65777
> > > > > Cc: j...@8bytes.org; b...@kernel.crashing.org;
> > > > > ga...@kernel.crashing.org; linux- ker...@vger.kernel.org;
> > > > > linuxppc-...@lists.ozlabs.org; linux- p...@vger.kernel.org;
> > > > > ag...@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > > > > foundation.org; Bhushan Bharat-R65777
> > > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > > > device
> > > > >
> > > > > On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> > > > > > This api return the iommu domain to which the device is attached.
> > > > > > The iommu_domain is required for making API calls related to iommu.
> > > > > > Follow up patches which use this API to know iommu maping.
> > > > > >
> > > > > > Signed-off-by: Bharat Bhushan
> > > > > > ---
> > > > > > drivers/iommu/iommu.c | 10 ++
> > > > > > include/linux/iommu.h |7 +++
> > > > > > 2 files changed, 17 insertions(+), 0 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > > > index
> > > > > > fbe9ca7..6ac5f50 100644
> > > > > > --- a/drivers/iommu/iommu.c
> > > > > > +++ b/drivers/iommu/iommu.c
> > > > > > @@ -696,6 +696,16 @@ void iommu_detach_device(struct
> > > > > > iommu_domain *domain, struct device *dev) }
> > > > > > EXPORT_SYMBOL_GPL(iommu_detach_device);
> > > > > >
> > > > > > +struct iommu_domain *iommu_get_dev_domain(struct device *dev) {
> > > > > > + struct iommu_ops *ops = dev->bus->iommu_ops;
> > > > > > +
> > > > > > + if (unlikely(ops == NULL || ops->get_dev_iommu_domain == NULL))
> > > > > > + return NULL;
> > > > > > +
> > > > > > + return ops->get_dev_iommu_domain(dev); }
> > > > > > +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);
> > > > >
> > > > > What prevents this from racing iommu_domain_free()? There's no
> > > > > references acquired, so there's no reason for the caller to
> > > > > assume the
> > > pointer is valid.
> > > >
> > > > Sorry for late query, somehow this email went into a folder and
> > > > escaped;
> > > >
> > > > Just to be sure, there is not lock at generic "struct
> > > > iommu_domain", but IP
> > > specific structure (link FSL domain) linked in iommu_domain->priv
> > > have a lock, so we need to ensure this race in FSL iommu code (say
> > > drivers/iommu/fsl_pamu_domain.c), right?
> > >
> > > No, it's not sufficient to make sure that your use of the interface
> > > is race free. The interface itself needs to be designed
RE: [PATCH 2/7] iommu: add api to get iommu_domain of a device
> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Friday, October 04, 2013 9:15 PM
> To: Bhushan Bharat-R65777
> Cc: j...@8bytes.org; b...@kernel.crashing.org; ga...@kernel.crashing.org;
> linux-
> ker...@vger.kernel.org; linuxppc-...@lists.ozlabs.org; linux-
> p...@vger.kernel.org; ag...@suse.de; Wood Scott-B07421; iommu@lists.linux-
> foundation.org
> Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device
>
> On Fri, 2013-10-04 at 09:54 +, Bhushan Bharat-R65777 wrote:
> >
> > > -Original Message-
> > > From: linux-pci-ow...@vger.kernel.org
> > > [mailto:linux-pci-ow...@vger.kernel.org]
> > > On Behalf Of Alex Williamson
> > > Sent: Wednesday, September 25, 2013 10:16 PM
> > > To: Bhushan Bharat-R65777
> > > Cc: j...@8bytes.org; b...@kernel.crashing.org;
> > > ga...@kernel.crashing.org; linux- ker...@vger.kernel.org;
> > > linuxppc-...@lists.ozlabs.org; linux- p...@vger.kernel.org;
> > > ag...@suse.de; Wood Scott-B07421; iommu@lists.linux- foundation.org;
> > > Bhushan Bharat-R65777
> > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > device
> > >
> > > On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> > > > This api return the iommu domain to which the device is attached.
> > > > The iommu_domain is required for making API calls related to iommu.
> > > > Follow up patches which use this API to know iommu maping.
> > > >
> > > > Signed-off-by: Bharat Bhushan
> > > > ---
> > > > drivers/iommu/iommu.c | 10 ++
> > > > include/linux/iommu.h |7 +++
> > > > 2 files changed, 17 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index
> > > > fbe9ca7..6ac5f50 100644
> > > > --- a/drivers/iommu/iommu.c
> > > > +++ b/drivers/iommu/iommu.c
> > > > @@ -696,6 +696,16 @@ void iommu_detach_device(struct iommu_domain
> > > > *domain, struct device *dev) }
> > > > EXPORT_SYMBOL_GPL(iommu_detach_device);
> > > >
> > > > +struct iommu_domain *iommu_get_dev_domain(struct device *dev) {
> > > > + struct iommu_ops *ops = dev->bus->iommu_ops;
> > > > +
> > > > + if (unlikely(ops == NULL || ops->get_dev_iommu_domain == NULL))
> > > > + return NULL;
> > > > +
> > > > + return ops->get_dev_iommu_domain(dev); }
> > > > +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);
> > >
> > > What prevents this from racing iommu_domain_free()? There's no
> > > references acquired, so there's no reason for the caller to assume the
> pointer is valid.
> >
> > Sorry for late query, somehow this email went into a folder and
> > escaped;
> >
> > Just to be sure, there is not lock at generic "struct iommu_domain", but IP
> specific structure (link FSL domain) linked in iommu_domain->priv have a lock,
> so we need to ensure this race in FSL iommu code (say
> drivers/iommu/fsl_pamu_domain.c), right?
>
> No, it's not sufficient to make sure that your use of the interface is race
> free. The interface itself needs to be designed so that it's difficult to use
> incorrectly.
So we can define iommu_get_dev_domain()/iommu_put_dev_domain();
iommu_get_dev_domain() will return domain with the lock held, and
iommu_put_dev_domain() will release the lock? And iommu_get_dev_domain() must
always be followed by iommu_get_dev_domain().
> That's not the case here. This is a backdoor to get the iommu
> domain from the iommu driver regardless of who is using it or how. The iommu
> domain is created and managed by vfio, so shouldn't we be looking at how to do
> this through vfio?
Let me first describe what we are doing here:
During initialization:-
- vfio talks to MSI system to know the MSI-page and size
- vfio then interacts with iommu to map the MSI-page in iommu (IOVA is decided
by userspace and physical address is the MSI-page)
- So the IOVA subwindow mapping is created in iommu and yes VFIO know about
this mapping.
Now do SET_IRQ(MSI/MSIX) ioctl:
- calls pci_enable_msix()/pci_enable_msi_block(): which is supposed to set MSI
address/data in device.
- So in current implementation (this patchset) msi-subsystem gets the IOVA
from iommu via this defined interface.
- Are you saying that rather than getting this from iommu, we should get this
from vfio? What difference does this make?
Thanks
-Bharat
>
RE: [PATCH 2/7] iommu: add api to get iommu_domain of a device
> -Original Message-
> From: Bhushan Bharat-R65777
> Sent: Friday, October 04, 2013 3:24 PM
> To: 'Alex Williamson'
> Cc: j...@8bytes.org; b...@kernel.crashing.org; ga...@kernel.crashing.org;
> linux-
> ker...@vger.kernel.org; linuxppc-...@lists.ozlabs.org; linux-
> p...@vger.kernel.org; ag...@suse.de; Wood Scott-B07421; iommu@lists.linux-
> foundation.org
> Subject: RE: [PATCH 2/7] iommu: add api to get iommu_domain of a device
>
>
>
> > -Original Message-
> > From: linux-pci-ow...@vger.kernel.org
> > [mailto:linux-pci-ow...@vger.kernel.org]
> > On Behalf Of Alex Williamson
> > Sent: Wednesday, September 25, 2013 10:16 PM
> > To: Bhushan Bharat-R65777
> > Cc: j...@8bytes.org; b...@kernel.crashing.org;
> > ga...@kernel.crashing.org; linux- ker...@vger.kernel.org;
> > linuxppc-...@lists.ozlabs.org; linux- p...@vger.kernel.org;
> > ag...@suse.de; Wood Scott-B07421; iommu@lists.linux- foundation.org;
> > Bhushan Bharat-R65777
> > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > device
> >
> > On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> > > This api return the iommu domain to which the device is attached.
> > > The iommu_domain is required for making API calls related to iommu.
> > > Follow up patches which use this API to know iommu maping.
> > >
> > > Signed-off-by: Bharat Bhushan
> > > ---
> > > drivers/iommu/iommu.c | 10 ++
> > > include/linux/iommu.h |7 +++
> > > 2 files changed, 17 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index
> > > fbe9ca7..6ac5f50 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -696,6 +696,16 @@ void iommu_detach_device(struct iommu_domain
> > > *domain, struct device *dev) }
> > > EXPORT_SYMBOL_GPL(iommu_detach_device);
> > >
> > > +struct iommu_domain *iommu_get_dev_domain(struct device *dev) {
> > > + struct iommu_ops *ops = dev->bus->iommu_ops;
> > > +
> > > + if (unlikely(ops == NULL || ops->get_dev_iommu_domain == NULL))
> > > + return NULL;
> > > +
> > > + return ops->get_dev_iommu_domain(dev); }
> > > +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);
> >
> > What prevents this from racing iommu_domain_free()? There's no
> > references acquired, so there's no reason for the caller to assume the
> > pointer
> is valid.
>
> Sorry for late query, somehow this email went into a folder and escaped;
>
> Just to be sure, there is not lock at generic "struct iommu_domain", but IP
> specific structure (link FSL domain) linked in iommu_domain->priv have a lock,
> so we need to ensure this race in FSL iommu code (say
> drivers/iommu/fsl_pamu_domain.c), right?
Further thinking of this, there are more problems here:
- Like MSI subsystem will call iommu_get_dev_domain(), which will take a lock,
find the domain pointer, release the lock, and return the domain
- Now if domain in freed up
- While MSI subsystem tries to do work on domain (like
get_attribute/set_attribute etc) ???
So can we do like iommu_get_dev_domain() will return domain with the lock held,
and iommu_put_dev_domain() will release the lock? And iommu_get_dev_domain()
must always be followed by iommu_get_dev_domain()
Thanks
-Bharat
>
> Thanks
> -Bharat
>
> >
> > > /*
> > > * IOMMU groups are really the natrual working unit of the IOMMU, but
> > > * the IOMMU API works on domains and devices. Bridge that gap by
> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h index
> > > 7ea319e..fa046bd 100644
> > > --- a/include/linux/iommu.h
> > > +++ b/include/linux/iommu.h
> > > @@ -127,6 +127,7 @@ struct iommu_ops {
> > > int (*domain_set_windows)(struct iommu_domain *domain, u32 w_count);
> > > /* Get the numer of window per domain */
> > > u32 (*domain_get_windows)(struct iommu_domain *domain);
> > > + struct iommu_domain *(*get_dev_iommu_domain)(struct device *dev);
> > >
> > > unsigned long pgsize_bitmap;
> > > };
> > > @@ -190,6 +191,7 @@ extern int iommu_domain_window_enable(struct
> > > iommu_domain
> > *domain, u32 wnd_nr,
> > > phys_addr_t offset, u64 size,
> > > int prot);
> > > extern void iommu_domain_window_disable(struct iommu_domain
> > > *domain,
> > > u32 wnd_nr);
> >
RE: [PATCH 2/7] iommu: add api to get iommu_domain of a device
> -Original Message-
> From: linux-pci-ow...@vger.kernel.org [mailto:linux-pci-ow...@vger.kernel.org]
> On Behalf Of Alex Williamson
> Sent: Wednesday, September 25, 2013 10:16 PM
> To: Bhushan Bharat-R65777
> Cc: j...@8bytes.org; b...@kernel.crashing.org; ga...@kernel.crashing.org;
> linux-
> ker...@vger.kernel.org; linuxppc-...@lists.ozlabs.org; linux-
> p...@vger.kernel.org; ag...@suse.de; Wood Scott-B07421; iommu@lists.linux-
> foundation.org; Bhushan Bharat-R65777
> Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device
>
> On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> > This api return the iommu domain to which the device is attached.
> > The iommu_domain is required for making API calls related to iommu.
> > Follow up patches which use this API to know iommu maping.
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > drivers/iommu/iommu.c | 10 ++
> > include/linux/iommu.h |7 +++
> > 2 files changed, 17 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index
> > fbe9ca7..6ac5f50 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -696,6 +696,16 @@ void iommu_detach_device(struct iommu_domain
> > *domain, struct device *dev) }
> > EXPORT_SYMBOL_GPL(iommu_detach_device);
> >
> > +struct iommu_domain *iommu_get_dev_domain(struct device *dev) {
> > + struct iommu_ops *ops = dev->bus->iommu_ops;
> > +
> > + if (unlikely(ops == NULL || ops->get_dev_iommu_domain == NULL))
> > + return NULL;
> > +
> > + return ops->get_dev_iommu_domain(dev); }
> > +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);
>
> What prevents this from racing iommu_domain_free()? There's no references
> acquired, so there's no reason for the caller to assume the pointer is valid.
Sorry for late query, somehow this email went into a folder and escaped;
Just to be sure, there is not lock at generic "struct iommu_domain", but IP
specific structure (link FSL domain) linked in iommu_domain->priv have a lock,
so we need to ensure this race in FSL iommu code (say
drivers/iommu/fsl_pamu_domain.c), right?
Thanks
-Bharat
>
> > /*
> > * IOMMU groups are really the natrual working unit of the IOMMU, but
> > * the IOMMU API works on domains and devices. Bridge that gap by
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h index
> > 7ea319e..fa046bd 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -127,6 +127,7 @@ struct iommu_ops {
> > int (*domain_set_windows)(struct iommu_domain *domain, u32 w_count);
> > /* Get the numer of window per domain */
> > u32 (*domain_get_windows)(struct iommu_domain *domain);
> > + struct iommu_domain *(*get_dev_iommu_domain)(struct device *dev);
> >
> > unsigned long pgsize_bitmap;
> > };
> > @@ -190,6 +191,7 @@ extern int iommu_domain_window_enable(struct
> > iommu_domain
> *domain, u32 wnd_nr,
> > phys_addr_t offset, u64 size,
> > int prot);
> > extern void iommu_domain_window_disable(struct iommu_domain *domain,
> > u32 wnd_nr);
> > +extern struct iommu_domain *iommu_get_dev_domain(struct device *dev);
> > /**
> > * report_iommu_fault() - report about an IOMMU fault to the IOMMU
> > framework
> > * @domain: the iommu domain where the fault has happened @@ -284,6
> > +286,11 @@ static inline void iommu_domain_window_disable(struct
> > iommu_domain *domain, { }
> >
> > +static inline struct iommu_domain *iommu_get_dev_domain(struct device
> > +*dev) {
> > + return NULL;
> > +}
> > +
> > static inline phys_addr_t iommu_iova_to_phys(struct iommu_domain
> > *domain, dma_addr_t iova) {
> > return 0;
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in the
> body
> of a message to majord...@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/7] powerpc: Add interface to get msi region information
> -Original Message-
> From: linux-pci-ow...@vger.kernel.org [mailto:linux-pci-ow...@vger.kernel.org]
> On Behalf Of Bjorn Helgaas
> Sent: Wednesday, September 25, 2013 5:28 AM
> To: Bhushan Bharat-R65777
> Cc: alex.william...@redhat.com; j...@8bytes.org; b...@kernel.crashing.org;
> ga...@kernel.crashing.org; linux-kernel@vger.kernel.org; linuxppc-
> d...@lists.ozlabs.org; linux-...@vger.kernel.org; ag...@suse.de; Wood Scott-
> B07421; io...@lists.linux-foundation.org; Bhushan Bharat-R65777
> Subject: Re: [PATCH 1/7] powerpc: Add interface to get msi region information
>
> On Thu, Sep 19, 2013 at 12:59:17PM +0530, Bharat Bhushan wrote:
> > This patch adds interface to get following information
> > - Number of MSI regions (which is number of MSI banks for powerpc).
> > - Get the region address range: Physical page which have the
> > address/addresses used for generating MSI interrupt
> > and size of the page.
> >
> > These are required to create IOMMU (Freescale PAMU) mapping for
> > devices which are directly assigned using VFIO.
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > arch/powerpc/include/asm/machdep.h |8 +++
> > arch/powerpc/include/asm/pci.h |2 +
> > arch/powerpc/kernel/msi.c | 18
> > arch/powerpc/sysdev/fsl_msi.c | 39
> > +--
> > arch/powerpc/sysdev/fsl_msi.h | 11 -
> > drivers/pci/msi.c | 26
> > include/linux/msi.h|8 +++
> > include/linux/pci.h| 13
> > 8 files changed, 120 insertions(+), 5 deletions(-)
> >
> > ...
>
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index
> > aca7578..6d85c15 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -30,6 +30,20 @@ static int pci_msi_enable = 1;
> >
> > /* Arch hooks */
> >
> > +#ifndef arch_msi_get_region_count
> > +int arch_msi_get_region_count(void)
> > +{
> > + return 0;
> > +}
> > +#endif
> > +
> > +#ifndef arch_msi_get_region
> > +int arch_msi_get_region(int region_num, struct msi_region *region) {
> > + return 0;
> > +}
> > +#endif
>
> This #define strategy is gone; see 4287d824 ("PCI: use weak functions for MSI
> arch-specific functions"). Please use the weak function strategy for your new
> MSI region functions.
ok
>
> > +
> > #ifndef arch_msi_check_device
> > int arch_msi_check_device(struct pci_dev *dev, int nvec, int type) {
> > @@ -903,6 +917,18 @@ void pci_disable_msi(struct pci_dev *dev) }
> > EXPORT_SYMBOL(pci_disable_msi);
> >
> > +int msi_get_region_count(void)
> > +{
> > + return arch_msi_get_region_count();
> > +}
> > +EXPORT_SYMBOL(msi_get_region_count);
> > +
> > +int msi_get_region(int region_num, struct msi_region *region) {
> > + return arch_msi_get_region(region_num, region); }
> > +EXPORT_SYMBOL(msi_get_region);
>
> Please split these interface additions, i.e., the drivers/pci/msi.c,
> include/linux/msi.h, and include/linux/pci.h changes, into a separate patch.
ok
>
> I don't know enough about VFIO to understand why these new interfaces are
> needed. Is this the first VFIO IOMMU driver? I see vfio_iommu_spapr_tce.c
> and
> vfio_iommu_type1.c but I don't know if they're comparable to the Freescale
> PAMU.
> Do other VFIO IOMMU implementations support MSI? If so, do they handle the
> problem of mapping the MSI regions in a different way?
PAMU is an aperture type of IOMMU while other are paging type, So they are
completely different from what PAMU is and handle that differently.
>
> > /**
> > * pci_msix_table_size - return the number of device's MSI-X table entries
> > * @dev: pointer to the pci_dev data structure of MSI-X device
> > function diff --git a/include/linux/msi.h b/include/linux/msi.h index
> > ee66f3a..ae32601 100644
> > --- a/include/linux/msi.h
> > +++ b/include/linux/msi.h
> > @@ -50,6 +50,12 @@ struct msi_desc {
> > struct kobject kobj;
> > };
> >
> > +struct msi_region {
> > + int region_num;
> > + dma_addr_t addr;
> > + size_t size;
> > +};
>
> This needs some sort of explanatory comment.
Ok
-Bharat
>
> > /*
> > * The arch hook for setup up msi irqs
> > */
> > @@ -58,5 +64,7 @@ void arch_teardown_msi_irq(unsigned int irq); int
> > arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type); void
>
RE: [PATCH 7/7] vfio pci: Add vfio iommu implementation for FSL_PAMU
> -Original Message- > From: Alex Williamson [mailto:alex.william...@redhat.com] > Sent: Thursday, September 26, 2013 12:37 AM > To: Bhushan Bharat-R65777 > Cc: j...@8bytes.org; b...@kernel.crashing.org; ga...@kernel.crashing.org; > linux- > ker...@vger.kernel.org; linuxppc-...@lists.ozlabs.org; linux- > p...@vger.kernel.org; ag...@suse.de; Wood Scott-B07421; iommu@lists.linux- > foundation.org; Bhushan Bharat-R65777 > Subject: Re: [PATCH 7/7] vfio pci: Add vfio iommu implementation for FSL_PAMU > > On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote: > > This patch adds vfio iommu support for Freescale IOMMU (PAMU - > > Peripheral Access Management Unit). > > > > The Freescale PAMU is an aperture-based IOMMU with the following > > characteristics. Each device has an entry in a table in memory > > describing the iova->phys mapping. The mapping has: > > -an overall aperture that is power of 2 sized, and has a start iova that > >is naturally aligned > > -has 1 or more windows within the aperture > > -number of windows must be power of 2, max is 256 > > -size of each window is determined by aperture size / # of windows > > -iova of each window is determined by aperture start iova / # of windows > > -the mapped region in each window can be different than > >the window size...mapping must power of 2 > > -physical address of the mapping must be naturally aligned > >with the mapping size > > > > Some of the code is derived from TYPE1 iommu > > (driver/vfio/vfio_iommu_type1.c). > > > > Signed-off-by: Bharat Bhushan > > --- > > drivers/vfio/Kconfig |6 + > > drivers/vfio/Makefile |1 + > > drivers/vfio/vfio_iommu_fsl_pamu.c | 952 > > > include/uapi/linux/vfio.h | 100 > > 4 files changed, 1059 insertions(+), 0 deletions(-) create mode > > 100644 drivers/vfio/vfio_iommu_fsl_pamu.c > > > > diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig index > > 26b3d9d..7d1da26 100644 > > --- a/drivers/vfio/Kconfig > > +++ b/drivers/vfio/Kconfig > > @@ -8,11 +8,17 @@ config VFIO_IOMMU_SPAPR_TCE > > depends on VFIO && SPAPR_TCE_IOMMU > > default n > > > > +config VFIO_IOMMU_FSL_PAMU > > + tristate > > + depends on VFIO > > + default n > > + > > menuconfig VFIO > > tristate "VFIO Non-Privileged userspace driver framework" > > depends on IOMMU_API > > select VFIO_IOMMU_TYPE1 if X86 > > select VFIO_IOMMU_SPAPR_TCE if (PPC_POWERNV || PPC_PSERIES) > > + select VFIO_IOMMU_FSL_PAMU if FSL_PAMU > > help > > VFIO provides a framework for secure userspace device drivers. > > See Documentation/vfio.txt for more details. > > diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile index > > c5792ec..7461350 100644 > > --- a/drivers/vfio/Makefile > > +++ b/drivers/vfio/Makefile > > @@ -1,4 +1,5 @@ > > obj-$(CONFIG_VFIO) += vfio.o > > obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_common.o > > vfio_iommu_type1.o > > obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_common.o > > vfio_iommu_spapr_tce.o > > +obj-$(CONFIG_VFIO_IOMMU_FSL_PAMU) += vfio_iommu_common.o > > +vfio_iommu_fsl_pamu.o > > obj-$(CONFIG_VFIO_PCI) += pci/ > > diff --git a/drivers/vfio/vfio_iommu_fsl_pamu.c > > b/drivers/vfio/vfio_iommu_fsl_pamu.c > > new file mode 100644 > > index 000..b29365f > > --- /dev/null > > +++ b/drivers/vfio/vfio_iommu_fsl_pamu.c > > @@ -0,0 +1,952 @@ > > +/* > > + * VFIO: IOMMU DMA mapping support for FSL PAMU IOMMU > > + * > > + * This program is free software; you can redistribute it and/or > > +modify > > + * it under the terms of the GNU General Public License, version 2, > > +as > > + * published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program; if not, write to the Free Software > > + * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, > > USA. > > + * > > + * Copyright (C) 2013 Freescale Semiconductor, Inc. > > + * > > + * Author: Bharat Bhushan >
RE: [PATCH 6/7] vfio: moving some functions in common file
> -Original Message-
> From: linux-pci-ow...@vger.kernel.org [mailto:linux-pci-ow...@vger.kernel.org]
> On Behalf Of Alex Williamson
> Sent: Wednesday, September 25, 2013 10:33 PM
> To: Bhushan Bharat-R65777
> Cc: j...@8bytes.org; b...@kernel.crashing.org; ga...@kernel.crashing.org;
> linux-
> ker...@vger.kernel.org; linuxppc-...@lists.ozlabs.org; linux-
> p...@vger.kernel.org; ag...@suse.de; Wood Scott-B07421; iommu@lists.linux-
> foundation.org; Bhushan Bharat-R65777
> Subject: Re: [PATCH 6/7] vfio: moving some functions in common file
>
> On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> > Some function defined in vfio_iommu_type1.c were common and we want to
> > use these for FSL IOMMU (PAMU) and iommu-none driver.
> > So some of them are moved to vfio_iommu_common.c
> >
> > I think we can do more of that but we will take this step by step.
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > drivers/vfio/Makefile|4 +-
> > drivers/vfio/vfio_iommu_common.c | 235
> ++
> > drivers/vfio/vfio_iommu_common.h | 30 +
> > drivers/vfio/vfio_iommu_type1.c | 206
> > +-
> > 4 files changed, 268 insertions(+), 207 deletions(-) create mode
> > 100644 drivers/vfio/vfio_iommu_common.c create mode 100644
> > drivers/vfio/vfio_iommu_common.h
> >
> > diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile index
> > 72bfabc..c5792ec 100644
> > --- a/drivers/vfio/Makefile
> > +++ b/drivers/vfio/Makefile
> > @@ -1,4 +1,4 @@
> > obj-$(CONFIG_VFIO) += vfio.o
> > -obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
> > -obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
> > +obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_common.o
> > +vfio_iommu_type1.o
> > +obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_common.o
> > +vfio_iommu_spapr_tce.o
> > obj-$(CONFIG_VFIO_PCI) += pci/
> > diff --git a/drivers/vfio/vfio_iommu_common.c
> > b/drivers/vfio/vfio_iommu_common.c
> > new file mode 100644
> > index 000..8bdc0ea
> > --- /dev/null
> > +++ b/drivers/vfio/vfio_iommu_common.c
> > @@ -0,0 +1,235 @@
> > +/*
> > + * VFIO: Common code for vfio IOMMU support
> > + *
> > + * Copyright (C) 2012 Red Hat, Inc. All rights reserved.
> > + * Author: Alex Williamson
> > + * Author: Bharat Bhushan
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * Derived from original vfio:
> > + * Copyright 2010 Cisco Systems, Inc. All rights reserved.
> > + * Author: Tom Lyon, p...@cisco.com
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include /* pci_bus_type */
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> Please cleanup includes on both the source and target files. You obviously
> don't need linux/pci.h here for one.
Will do.
>
> > +
> > +static bool disable_hugepages;
> > +module_param_named(disable_hugepages,
> > + disable_hugepages, bool, S_IRUGO | S_IWUSR);
> > +MODULE_PARM_DESC(disable_hugepages,
> > +"Disable VFIO IOMMU support for IOMMU hugepages.");
> > +
> > +struct vwork {
> > + struct mm_struct*mm;
> > + longnpage;
> > + struct work_struct work;
> > +};
> > +
> > +/* delayed decrement/increment for locked_vm */ void
> > +vfio_lock_acct_bg(struct work_struct *work) {
> > + struct vwork *vwork = container_of(work, struct vwork, work);
> > + struct mm_struct *mm;
> > +
> > + mm = vwork->mm;
> > + down_write(&mm->mmap_sem);
> > + mm->locked_vm += vwork->npage;
> > + up_write(&mm->mmap_sem);
> > + mmput(mm);
> > + kfree(vwork);
> > +}
> > +
> > +void vfio_lock_acct(long npage)
> > +{
> > + struct vwork *vwork;
> > + struct mm_struct *mm;
> > +
> > + if (!current->mm || !npage)
> > + return; /* process exited or nothing to do */
> > +
> > + if (down_write_trylock(¤t->mm->mmap_sem)) {
> > + current->mm->locked_vm += npage;
> > + up_write(¤t->mm->mmap_sem);
>
RE: [PATCH 5/7] iommu: supress loff_t compilation error on powerpc
> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Wednesday, September 25, 2013 10:10 PM
> To: Bhushan Bharat-R65777
> Cc: j...@8bytes.org; b...@kernel.crashing.org; ga...@kernel.crashing.org;
> linux-
> ker...@vger.kernel.org; linuxppc-...@lists.ozlabs.org; linux-
> p...@vger.kernel.org; ag...@suse.de; Wood Scott-B07421; iommu@lists.linux-
> foundation.org; Bhushan Bharat-R65777
> Subject: Re: [PATCH 5/7] iommu: supress loff_t compilation error on powerpc
>
> On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> > Signed-off-by: Bharat Bhushan
> > ---
> > drivers/vfio/pci/vfio_pci_rdwr.c |3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c
> > b/drivers/vfio/pci/vfio_pci_rdwr.c
> > index 210db24..8a8156a 100644
> > --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> > +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
> > @@ -181,7 +181,8 @@ ssize_t vfio_pci_vga_rw(struct vfio_pci_device *vdev,
> > char
> __user *buf,
> >size_t count, loff_t *ppos, bool iswrite) {
> > int ret;
> > - loff_t off, pos = *ppos & VFIO_PCI_OFFSET_MASK;
> > + loff_t off;
> > + u64 pos = (u64 )(*ppos & VFIO_PCI_OFFSET_MASK);
> > void __iomem *iomem = NULL;
> > unsigned int rsrc;
> > bool is_ioport;
>
> What's the compile error that this fixes?
I was getting below error; and after some googling I came to know that this is
how it is fixed by other guys.
/home/r65777/linux-vfio/drivers/vfio/pci/vfio_pci_rdwr.c:193: undefined
reference to `__cmpdi2'
/home/r65777/linux-vfio/drivers/vfio/pci/vfio_pci_rdwr.c:193: undefined
reference to `__cmpdi2'
Thanks
-Bharat
>
RE: [PATCH] KVM: PPC: POWERNV: move iommu_add_device earlier
> -Original Message-
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+bharat.bhushan=freescale@lists.ozlabs.org] On Behalf Of Alexey
> Kardashevskiy
> Sent: Wednesday, August 14, 2013 2:55 PM
> To: linuxppc-...@lists.ozlabs.org
> Cc: Alexey Kardashevskiy; Paul Mackerras; linux-kernel@vger.kernel.org
> Subject: [PATCH] KVM: PPC: POWERNV: move iommu_add_device earlier
>
> The current implementation of IOMMU on sPAPR does not use iommu_ops
> and therefore does not call IOMMU API's bus_set_iommu() which
> 1) sets iommu_ops for a bus
> 2) registers a bus notifier
> Instead, PCI devices are added to IOMMU groups from
> subsys_initcall_sync(tce_iommu_init) which does basically the same
> thing without using iommu_ops callbacks.
>
> However Freescale PAMU driver (https://lkml.org/lkml/2013/7/1/158)
> implements iommu_ops and when tce_iommu_init is called, every PCI device
> is already added to some group so there is a conflict.
>
> This patch does 2 things:
> 1. removes the loop in which PCI devices were added to groups and
> adds explicit iommu_add_device() calls to add devices as soon as they get
> the iommu_table pointer assigned to them.
> 2. moves a bus notifier to powernv code in order to avoid conflict with
> the notifier from Freescale driver.
>
> iommu_add_device() and iommu_del_device() are public now.
This works for me (able to boot Linux, as expected) :-)
But a question, why not move arch/powerpc/kernel/iommu.c in platform/ ? or use
this for book3s or not_book3e only?
Thanks
-Bharat
>
> Signed-off-by: Alexey Kardashevskiy
> ---
> arch/powerpc/include/asm/iommu.h| 2 ++
> arch/powerpc/kernel/iommu.c | 41
> +++--
> arch/powerpc/platforms/powernv/pci-ioda.c | 12 ++---
> arch/powerpc/platforms/powernv/pci-p5ioc2.c | 1 +
> arch/powerpc/platforms/powernv/pci.c| 31 ++
> arch/powerpc/platforms/pseries/iommu.c | 7 +++--
> 6 files changed, 51 insertions(+), 43 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/iommu.h
> b/arch/powerpc/include/asm/iommu.h
> index c34656a..ba74329 100644
> --- a/arch/powerpc/include/asm/iommu.h
> +++ b/arch/powerpc/include/asm/iommu.h
> @@ -103,6 +103,8 @@ extern struct iommu_table *iommu_init_table(struct
> iommu_table * tbl,
> int nid);
> extern void iommu_register_group(struct iommu_table *tbl,
>int pci_domain_number, unsigned long pe_num);
> +extern int iommu_add_device(struct device *dev);
> +extern void iommu_del_device(struct device *dev);
>
> extern int iommu_map_sg(struct device *dev, struct iommu_table *tbl,
> struct scatterlist *sglist, int nelems,
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index b20ff17..15f8ca8 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -1105,7 +1105,7 @@ void iommu_release_ownership(struct iommu_table *tbl)
> }
> EXPORT_SYMBOL_GPL(iommu_release_ownership);
>
> -static int iommu_add_device(struct device *dev)
> +int iommu_add_device(struct device *dev)
> {
> struct iommu_table *tbl;
> int ret = 0;
> @@ -1134,46 +1134,13 @@ static int iommu_add_device(struct device *dev)
>
> return ret;
> }
> +EXPORT_SYMBOL_GPL(iommu_add_device);
>
> -static void iommu_del_device(struct device *dev)
> +void iommu_del_device(struct device *dev)
> {
> iommu_group_remove_device(dev);
> }
> -
> -static int iommu_bus_notifier(struct notifier_block *nb,
> - unsigned long action, void *data)
> -{
> - struct device *dev = data;
> -
> - switch (action) {
> - case BUS_NOTIFY_ADD_DEVICE:
> - return iommu_add_device(dev);
> - case BUS_NOTIFY_DEL_DEVICE:
> - iommu_del_device(dev);
> - return 0;
> - default:
> - return 0;
> - }
> -}
> -
> -static struct notifier_block tce_iommu_bus_nb = {
> - .notifier_call = iommu_bus_notifier,
> -};
> -
> -static int __init tce_iommu_init(void)
> -{
> - struct pci_dev *pdev = NULL;
> -
> - BUILD_BUG_ON(PAGE_SIZE < IOMMU_PAGE_SIZE);
> -
> - for_each_pci_dev(pdev)
> - iommu_add_device(&pdev->dev);
> -
> - bus_register_notifier(&pci_bus_type, &tce_iommu_bus_nb);
> - return 0;
> -}
> -
> -subsys_initcall_sync(tce_iommu_init);
> +EXPORT_SYMBOL_GPL(iommu_del_device);
>
> #else
>
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c
> b/arch/powerpc/platforms/powernv/pci-ioda.c
> index d8140b1..a9f8fef 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -441,6 +441,7 @@ static void pnv_pci_ioda_dma_dev_setup(struct pnv_phb
> *phb,
> struct pci_dev *pdev
>
> pe = &phb->ioda.pe_array[pdn->pe_number];
> set_iommu_table_base(&pdev->dev, &pe->tce32_table);
> + iommu_add_device(&pdev->dev);
> }
>
> static void
RE: [v3][PATCH 7/8] book3e/kexec/kdump: redefine VIRT_PHYS_OFFSET
> -Original Message- > From: Linuxppc-dev [mailto:linuxppc-dev- > bounces+bharat.bhushan=freescale@lists.ozlabs.org] On Behalf Of Tiejun > Chen > Sent: Tuesday, July 09, 2013 1:33 PM > To: b...@kernel.crashing.org > Cc: linuxppc-...@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: [v3][PATCH 7/8] book3e/kexec/kdump: redefine VIRT_PHYS_OFFSET > > Book3e is always aligned 1GB to create TLB so we should > use (KERNELBASE - MEMORY_START) as VIRT_PHYS_OFFSET to > get __pa/__va properly while boot kdump. > > Signed-off-by: Tiejun Chen > --- > arch/powerpc/include/asm/page.h |2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h > index 988c812..5b00081 100644 > --- a/arch/powerpc/include/asm/page.h > +++ b/arch/powerpc/include/asm/page.h > @@ -112,6 +112,8 @@ extern long long virt_phys_offset; > /* See Description below for VIRT_PHYS_OFFSET */ > #ifdef CONFIG_RELOCATABLE_PPC32 > #define VIRT_PHYS_OFFSET virt_phys_offset > +#elif defined(CONFIG_PPC_BOOK3E_64) > +#define VIRT_PHYS_OFFSET (KERNELBASE - MEMORY_START) Can you please explain this code a bit more. I am not understanding this part:) -Bharat > #else > #define VIRT_PHYS_OFFSET (KERNELBASE - PHYSICAL_START) > #endif > -- > 1.7.9.5 > > ___ > Linuxppc-dev mailing list > linuxppc-...@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [v3][PATCH 1/8] powerpc/book3e: rename interrupt_end_book3e with __end_interrupts
> -Original Message- > From: Linuxppc-dev [mailto:linuxppc-dev- > bounces+bharat.bhushan=freescale@lists.ozlabs.org] On Behalf Of Tiejun > Chen > Sent: Tuesday, July 09, 2013 1:33 PM > To: b...@kernel.crashing.org > Cc: linuxppc-...@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: [v3][PATCH 1/8] powerpc/book3e: rename interrupt_end_book3e with > __end_interrupts > > We can rename 'interrupt_end_book3e' with '__end_interrupts' then > book3s/book3e > can share this unique label to make sure we can use this conveniently. I think we can be consistent with start and end names, no? -Bharat > > Signed-off-by: Tiejun Chen > --- > arch/powerpc/kernel/exceptions-64e.S |8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/kernel/exceptions-64e.S > b/arch/powerpc/kernel/exceptions-64e.S > index 645170a..a518e48 100644 > --- a/arch/powerpc/kernel/exceptions-64e.S > +++ b/arch/powerpc/kernel/exceptions-64e.S > @@ -309,8 +309,8 @@ interrupt_base_book3e: > /* fake > trap */ > EXCEPTION_STUB(0x300, hypercall) > EXCEPTION_STUB(0x320, ehpriv) > > - .globl interrupt_end_book3e > -interrupt_end_book3e: > + .globl __end_interrupts > +__end_interrupts: > > /* Critical Input Interrupt */ > START_EXCEPTION(critical_input); > @@ -493,7 +493,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC) > beq+1f > > LOAD_REG_IMMEDIATE(r14,interrupt_base_book3e) > - LOAD_REG_IMMEDIATE(r15,interrupt_end_book3e) > + LOAD_REG_IMMEDIATE(r15,__end_interrupts) > cmpld cr0,r10,r14 > cmpld cr1,r10,r15 > blt+cr0,1f > @@ -559,7 +559,7 @@ kernel_dbg_exc: > beq+1f > > LOAD_REG_IMMEDIATE(r14,interrupt_base_book3e) > - LOAD_REG_IMMEDIATE(r15,interrupt_end_book3e) > + LOAD_REG_IMMEDIATE(r15,__end_interrupts) > cmpld cr0,r10,r14 > cmpld cr1,r10,r15 > blt+cr0,1f > -- > 1.7.9.5 > > ___ > Linuxppc-dev mailing list > linuxppc-...@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [v2][PATCH 4/7] book3e/kexec/kdump: introduce a kexec kernel flag
> -Original Message-
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+bharat.bhushan=freescale@lists.ozlabs.org] On Behalf Of Tiejun
> Chen
> Sent: Thursday, June 20, 2013 1:23 PM
> To: b...@kernel.crashing.org
> Cc: linuxppc-...@lists.ozlabs.org; linux-kernel@vger.kernel.org
> Subject: [v2][PATCH 4/7] book3e/kexec/kdump: introduce a kexec kernel flag
>
> We need to introduce a flag to indicate we're already running
> a kexec kernel then we can go proper path. For example, We
> shouldn't access spin_table from the bootloader to up any secondary
> cpu for kexec kernel, and kexec kernel already know how to jump to
> generic_secondary_smp_init.
>
> Signed-off-by: Tiejun Chen
> ---
> arch/powerpc/include/asm/smp.h|3 +++
> arch/powerpc/kernel/head_64.S | 12
> arch/powerpc/kernel/misc_64.S |6 ++
> arch/powerpc/platforms/85xx/smp.c | 14 ++
> 4 files changed, 35 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
> index ffbaabe..fbc3d9b 100644
> --- a/arch/powerpc/include/asm/smp.h
> +++ b/arch/powerpc/include/asm/smp.h
> @@ -200,6 +200,9 @@ extern void generic_secondary_thread_init(void);
> extern unsigned long __secondary_hold_spinloop;
> extern unsigned long __secondary_hold_acknowledge;
> extern char __secondary_hold;
> +#if defined(CONFIG_KEXEC) || defined(CONFIG_CRASH_DUMP)
> +extern unsigned long __run_at_kexec;
> +#endif
>
> extern void __early_start(void);
> #endif /* __ASSEMBLY__ */
> diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
> index 3e19ba2..ffa4b18 100644
> --- a/arch/powerpc/kernel/head_64.S
> +++ b/arch/powerpc/kernel/head_64.S
> @@ -89,6 +89,12 @@ __secondary_hold_spinloop:
> __secondary_hold_acknowledge:
> .llong 0x0
>
> +#if defined(CONFIG_KEXEC) || defined(CONFIG_CRASH_DUMP)
> + .globl __run_at_kexec
> +__run_at_kexec:
> + .llong 0x0 /* Flag for the secondary kernel from kexec. */
> +#endif
> +
> #ifdef CONFIG_RELOCATABLE
> /* This flag is set to 1 by a loader if the kernel should run
>* at the loaded address instead of the linked address. This
> @@ -417,6 +423,12 @@ _STATIC(__after_prom_start)
> #if defined(CONFIG_PPC_BOOK3E)
> tovirt(r26,r26) /* on booke, we already run at
> PAGE_OFFSET */
> #endif
> +#if defined(CONFIG_KEXEC) || defined(CONFIG_CRASH_DUMP)
> + /* If relocated we need to restore this flag on that relocated address.
> */
> + ld r7,__run_at_kexec-_stext(r26)
> + std r7,__run_at_kexec-_stext(r26)
> +#endif
> +
> lwz r7,__run_at_load-_stext(r26)
> #if defined(CONFIG_PPC_BOOK3E)
> tophys(r26,r26) /* Restore for the remains. */
> diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
> index 20cbb98..c89aead 100644
> --- a/arch/powerpc/kernel/misc_64.S
> +++ b/arch/powerpc/kernel/misc_64.S
> @@ -619,6 +619,12 @@ _GLOBAL(kexec_sequence)
> bl .copy_and_flush /* (dest, src, copy limit, start offset) */
> 1: /* assume normal blr return */
>
> + /* notify we're going into kexec kernel for SMP. */
> + LOAD_REG_ADDR(r3,__run_at_kexec)
> + li r4,1
> + std r4,0(r3)
> + sync
> +
> /* release other cpus to the new kernel secondary start at 0x60 */
> mflrr5
> li r6,1
> diff --git a/arch/powerpc/platforms/85xx/smp.c
> b/arch/powerpc/platforms/85xx/smp.c
> index 6a17599..b308373 100644
> --- a/arch/powerpc/platforms/85xx/smp.c
> +++ b/arch/powerpc/platforms/85xx/smp.c
> @@ -150,6 +150,9 @@ static int __cpuinit smp_85xx_kick_cpu(int nr)
> int hw_cpu = get_hard_smp_processor_id(nr);
> int ioremappable;
> int ret = 0;
> +#if defined(CONFIG_KEXEC) || defined(CONFIG_CRASH_DUMP)
> + unsigned long *ptr;
> +#endif
What about if we can remove the ifdef around *ptr ...
>
> WARN_ON(nr < 0 || nr >= NR_CPUS);
> WARN_ON(hw_cpu < 0 || hw_cpu >= NR_CPUS);
> @@ -238,11 +241,22 @@ out:
> #else
> smp_generic_kick_cpu(nr);
>
> +#if defined(CONFIG_KEXEC) || defined(CONFIG_CRASH_DUMP)
> + ptr = (unsigned long *)((unsigned long)&__run_at_kexec);
... #endif here ...
> + /* We shouldn't access spin_table from the bootloader to up any
> + * secondary cpu for kexec kernel, and kexec kernel already
> + * know how to jump to generic_secondary_smp_init.
> + */
> + if (!*ptr) {
> +#endif
... remove #endif ...
> flush_spin_table(spin_table);
> out_be32(&spin_table->pir, hw_cpu);
> out_be64((u64 *)(&spin_table->addr_h),
> __pa((u64)*((unsigned long long *)generic_secondary_smp_init)));
> flush_spin_table(spin_table);
> +#if defined(CONFIG_KEXEC) || defined(CONFIG_CRASH_DUMP)
> + }
> +#endif
--- remove above 3 lines
-Bharat
> #endif
>
> local_irq_restore(flags);
> --
> 1.7.9.5
>
> ___
> Linuxppc-dev
RE: [v2][PATCH 2/7] book3e/kexec/kdump: enable kexec for kernel
> -Original Message-
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+bharat.bhushan=freescale@lists.ozlabs.org] On Behalf Of Tiejun
> Chen
> Sent: Thursday, June 20, 2013 1:23 PM
> To: b...@kernel.crashing.org
> Cc: linuxppc-...@lists.ozlabs.org; linux-kernel@vger.kernel.org
> Subject: [v2][PATCH 2/7] book3e/kexec/kdump: enable kexec for kernel
>
> We need to active KEXEC for book3e and bypass or convert non-book3e stuff
> in kexec coverage.
>
> Signed-off-by: Tiejun Chen
> ---
> arch/powerpc/Kconfig |2 +-
> arch/powerpc/kernel/machine_kexec_64.c |6 ++
> arch/powerpc/kernel/misc_64.S |6 ++
> 3 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index c33e3ad..6ecf3c9 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -364,7 +364,7 @@ config ARCH_ENABLE_MEMORY_HOTREMOVE
>
> config KEXEC
> bool "kexec system call"
> - depends on (PPC_BOOK3S || FSL_BOOKE || (44x && !SMP))
> + depends on (PPC_BOOK3S || FSL_BOOKE || (44x && !SMP)) || PPC_BOOK3E
> help
> kexec is a system call that implements the ability to shutdown your
> current kernel, and to start another kernel. It is like a reboot
> diff --git a/arch/powerpc/kernel/machine_kexec_64.c
> b/arch/powerpc/kernel/machine_kexec_64.c
> index 611acdf..ef39271 100644
> --- a/arch/powerpc/kernel/machine_kexec_64.c
> +++ b/arch/powerpc/kernel/machine_kexec_64.c
> @@ -33,6 +33,7 @@
> int default_machine_kexec_prepare(struct kimage *image)
> {
> int i;
> +#ifndef CONFIG_PPC_BOOK3E
> unsigned long begin, end; /* limits of segment */
> unsigned long low, high;/* limits of blocked memory range */
> struct device_node *node;
> @@ -41,6 +42,7 @@ int default_machine_kexec_prepare(struct kimage *image)
>
> if (!ppc_md.hpte_clear_all)
> return -ENOENT;
> +#endif
Do we really need this function for book3e? can we have a separate function
rather than multiple confusing ifdef?
-Bharat
>
> /*
>* Since we use the kernel fault handlers and paging code to
> @@ -51,6 +53,7 @@ int default_machine_kexec_prepare(struct kimage *image)
> if (image->segment[i].mem < __pa(_end))
> return -ETXTBSY;
>
> +#ifndef CONFIG_PPC_BOOK3E
> /*
>* For non-LPAR, we absolutely can not overwrite the mmu hash
>* table, since we are still using the bolted entries in it to
> @@ -92,6 +95,7 @@ int default_machine_kexec_prepare(struct kimage *image)
> return -ETXTBSY;
> }
> }
> +#endif
>
> return 0;
> }
> @@ -367,6 +371,7 @@ void default_machine_kexec(struct kimage *image)
> /* NOTREACHED */
> }
>
> +#ifndef CONFIG_PPC_BOOK3E
> /* Values we need to export to the second kernel via the device tree. */
> static unsigned long htab_base;
>
> @@ -411,3 +416,4 @@ static int __init export_htab_values(void)
> return 0;
> }
> late_initcall(export_htab_values);
> +#endif
> diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
> index 6820e45..f1a7ce7 100644
> --- a/arch/powerpc/kernel/misc_64.S
> +++ b/arch/powerpc/kernel/misc_64.S
> @@ -543,9 +543,13 @@ _GLOBAL(kexec_sequence)
> lhz r25,PACAHWCPUID(r13)/* get our phys cpu from paca */
>
> /* disable interrupts, we are overwriting kernel data next */
> +#ifndef CONFIG_PPC_BOOK3E
> mfmsr r3
> rlwinm r3,r3,0,17,15
> mtmsrd r3,1
> +#else
> + wrteei 0
> +#endif
>
> /* copy dest pages, flush whole dest image */
> mr r3,r29
> @@ -567,10 +571,12 @@ _GLOBAL(kexec_sequence)
> li r6,1
> stw r6,kexec_flag-1b(5)
>
> +#ifndef CONFIG_PPC_BOOK3E
> /* clear out hardware hash page table and tlb */
> ld r5,0(r27) /* deref function descriptor */
> mtctr r5
> bctrl /* ppc_md.hpte_clear_all(void); */
> +#endif
>
> /*
> * kexec image calling is:
> --
> 1.7.9.5
>
> ___
> Linuxppc-dev mailing list
> linuxppc-...@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [v2][PATCH 1/7] powerpc/book3e: support CONFIG_RELOCATABLE
> -Original Message- > From: Linuxppc-dev [mailto:linuxppc-dev- > bounces+bharat.bhushan=freescale@lists.ozlabs.org] On Behalf Of Tiejun > Chen > Sent: Thursday, June 20, 2013 1:23 PM > To: b...@kernel.crashing.org > Cc: linuxppc-...@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: [v2][PATCH 1/7] powerpc/book3e: support CONFIG_RELOCATABLE > > book3e is different with book3s since 3s includes the exception > vectors code in head_64.S as it relies on absolute addressing > which is only possible within this compilation unit. So we have > to get that label address with got. > > And when boot a relocated kernel, we should reset ipvr properly again > after .relocate. > > Signed-off-by: Tiejun Chen > --- > arch/powerpc/include/asm/exception-64e.h |8 > arch/powerpc/kernel/exceptions-64e.S | 15 ++- > arch/powerpc/kernel/head_64.S| 22 ++ > arch/powerpc/lib/feature-fixups.c|7 +++ > 4 files changed, 51 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/include/asm/exception-64e.h > b/arch/powerpc/include/asm/exception-64e.h > index 51fa43e..89e940d 100644 > --- a/arch/powerpc/include/asm/exception-64e.h > +++ b/arch/powerpc/include/asm/exception-64e.h > @@ -214,10 +214,18 @@ exc_##label##_book3e: > #define TLB_MISS_STATS_SAVE_INFO_BOLTED > #endif > > +#ifndef CONFIG_RELOCATABLE > #define SET_IVOR(vector_number, vector_offset) \ > li r3,vector_offset@l; \ > ori r3,r3,interrupt_base_book3e@l; \ > mtspr SPRN_IVOR##vector_number,r3; > +#else > +#define SET_IVOR(vector_number, vector_offset) \ > + LOAD_REG_ADDR(r3,interrupt_base_book3e);\ > + rlwinm r3,r3,0,15,0; \ > + ori r3,r3,vector_offset@l; \ > + mtspr SPRN_IVOR##vector_number,r3; > +#endif > > #endif /* _ASM_POWERPC_EXCEPTION_64E_H */ > > diff --git a/arch/powerpc/kernel/exceptions-64e.S > b/arch/powerpc/kernel/exceptions-64e.S > index 645170a..4b23119 100644 > --- a/arch/powerpc/kernel/exceptions-64e.S > +++ b/arch/powerpc/kernel/exceptions-64e.S > @@ -1097,7 +1097,15 @@ skpinv:addir6,r6,1 > /* > Increment */ > * r4 = MAS0 w/TLBSEL & ESEL for the temp mapping > */ > /* Now we branch the new virtual address mapped by this entry */ > +#ifdef CONFIG_RELOCATABLE > + /* We have to find out address from lr. */ > + bl 1f /* Find our address */ > +1: mflrr6 > + addir6,r6,(2f - 1b) > + tovirt(r6,r6) > +#else > LOAD_REG_IMMEDIATE(r6,2f) > +#endif > lis r7,MSR_KERNEL@h > ori r7,r7,MSR_KERNEL@l > mtspr SPRN_SRR0,r6 > @@ -1348,9 +1356,14 @@ _GLOBAL(book3e_secondary_thread_init) > mflrr28 > b 3b > > -_STATIC(init_core_book3e) > +_GLOBAL(init_core_book3e) > /* Establish the interrupt vector base */ > +#ifdef CONFIG_RELOCATABLE > + tovirt(r2,r2) > + LOAD_REG_ADDR(r3, interrupt_base_book3e) > +#else > LOAD_REG_IMMEDIATE(r3, interrupt_base_book3e) > +#endif > mtspr SPRN_IVPR,r3 > sync > blr > diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S > index b61363d..0942f3a 100644 > --- a/arch/powerpc/kernel/head_64.S > +++ b/arch/powerpc/kernel/head_64.S > @@ -414,12 +414,22 @@ _STATIC(__after_prom_start) > /* process relocations for the final address of the kernel */ > lis r25,PAGE_OFFSET@highest /* compute virtual base of kernel */ > sldir25,r25,32 > +#if defined(CONFIG_PPC_BOOK3E) > + tovirt(r26,r26) /* on booke, we already run at > PAGE_OFFSET */ > +#endif > lwz r7,__run_at_load-_stext(r26) > +#if defined(CONFIG_PPC_BOOK3E) > + tophys(r26,r26) /* Restore for the remains. */ > +#endif > cmplwi cr0,r7,1/* flagged to stay where we are ? */ > bne 1f > add r25,r25,r26 > 1: mr r3,r25 > bl .relocate > +#if defined(CONFIG_PPC_BOOK3E) > + /* We should set ivpr again after .relocate. */ > + bl .init_core_book3e > +#endif > #endif > > /* > @@ -447,12 +457,24 @@ _STATIC(__after_prom_start) > * variable __run_at_load, if it is set the kernel is treated as relocatable > * kernel, otherwise it will be moved to PHYSICAL_START > */ > +#if defined(CONFIG_PPC_BOOK3E) > + tovirt(r26,r26) /* on booke, we already run at > PAGE_OFFSET */ > +#endif > lwz r7,__run_at_load-_stext(r26) > +#if defined(CONFIG_PPC_BOOK3E) > + tophys(r26,r26) /* Restore for the remains. */ > +#endif > cmplwi cr0,r7,1 > bne 3f > > +#ifdef CONFIG_PPC_BOOK3E > + LOAD_REG_ADDR(r5, interrupt_end_book3e) > + LOAD_REG_ADDR(r11, _stext) > + sub r5,r5,r11 > +#else > /* just copy interrupts */ > LOAD_REG_IMMEDIATE(r5, __end_interrupts - _stext) > +#endif >
