RE: [PATCH 0/9 v2] vfio-pci: add support for Freescale IOMMU (PAMU)
> -Original Message- > From: Wood Scott-B07421 > Sent: Saturday, December 07, 2013 12:55 AM > To: Bhushan Bharat-R65777 > Cc: Alex Williamson; linux-...@vger.kernel.org; ag...@suse.de; Yoder Stuart- > B08248; io...@lists.linux-foundation.org; bhelg...@google.com; linuxppc- > d...@lists.ozlabs.org; linux-ker...@vger.kernel.org > Subject: Re: [PATCH 0/9 v2] vfio-pci: add support for Freescale IOMMU (PAMU) > > On Thu, 2013-12-05 at 22:17 -0600, Bharat Bhushan wrote: > > > > > -Original Message- > > > From: Wood Scott-B07421 > > > Sent: Friday, December 06, 2013 5:31 AM > > > To: Bhushan Bharat-R65777 > > > Cc: Alex Williamson; linux-...@vger.kernel.org; ag...@suse.de; Yoder > > > Stuart- B08248; io...@lists.linux-foundation.org; > > > bhelg...@google.com; linuxppc- d...@lists.ozlabs.org; > > > linux-ker...@vger.kernel.org > > > Subject: Re: [PATCH 0/9 v2] vfio-pci: add support for Freescale > > > IOMMU (PAMU) > > > > > > On Sun, 2013-11-24 at 23:33 -0600, Bharat Bhushan wrote: > > > > > > > > > -Original Message- > > > > > From: Alex Williamson [mailto:alex.william...@redhat.com] > > > > > Sent: Friday, November 22, 2013 2:31 AM > > > > > To: Wood Scott-B07421 > > > > > Cc: Bhushan Bharat-R65777; linux-...@vger.kernel.org; > > > > > ag...@suse.de; Yoder Stuart-B08248; > > > > > io...@lists.linux-foundation.org; bhelg...@google.com; linuxppc- > > > > > d...@lists.ozlabs.org; linux-ker...@vger.kernel.org > > > > > Subject: Re: [PATCH 0/9 v2] vfio-pci: add support for Freescale > > > > > IOMMU (PAMU) > > > > > > > > > > On Thu, 2013-11-21 at 14:47 -0600, Scott Wood wrote: > > > > > > They can interfere. > > > > > > > > Want to be sure of how they can interfere? > > > > > > If more than one VFIO user shares the same MSI group, one of the > > > users can send MSIs to another user, by using the wrong interrupt > > > within the bank. Unexpected MSIs could cause misbehavior or denial of > service. > > > > > > > >> With this hardware, the only way to prevent that > > > > > > is to make sure that a bank is not shared by multiple protection > contexts. > > > > > > For some of our users, though, I believe preventing this is > > > > > > less important than the performance benefit. > > > > > > > > So should we let this patch series in without protection? > > > > > > No, there should be some sort of opt-in mechanism similar to > > > IOMMU-less VFIO -- but not the same exact one, since one is a much > > > more serious loss of isolation than the other. > > > > Can you please elaborate "opt-in mechanism"? > > The system should be secure by default. If the administrator wants to relax > protection in order to accomplish some functionality, that should require an > explicit request such as a write to a sysfs file. > > > > > > I think we need some sort of ownership model around the msi banks > > > > > then. > > > > > Otherwise there's nothing preventing another userspace from > > > > > attempting an MSI based attack on other users, or perhaps even > > > > > on the host. VFIO can't allow that. Thanks, > > > > > > > > We have very few (3 MSI bank on most of chips), so we can not > > > > assign one to each userspace. > > > > > > That depends on how many users there are. > > > > What I think we can do is: > > - Reserve one MSI region for host. Host will not share MSI region with > > Guest. > > - For upto 2 Guest (MAX msi with host - 1) give then separate MSI sub > > regions > > - Additional Guest will share MSI region with other guest. > > > > Any better suggestion are most welcome. > > If the administrator does not opt into this partial loss of isolation, then > once > you run out of MSI groups, new users should not be able to set up MSIs. So mean vfio should use Legacy when out of MSI banks? Thanks -Bharat > > -Scott > ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 0/9 v2] vfio-pci: add support for Freescale IOMMU (PAMU)
> -Original Message- > From: Alex Williamson [mailto:alex.william...@redhat.com] > Sent: Saturday, December 07, 2013 1:00 AM > To: Wood Scott-B07421 > Cc: Bhushan Bharat-R65777; linux-...@vger.kernel.org; ag...@suse.de; Yoder > Stuart-B08248; io...@lists.linux-foundation.org; bhelg...@google.com; > linuxppc- > d...@lists.ozlabs.org; linux-ker...@vger.kernel.org > Subject: Re: [PATCH 0/9 v2] vfio-pci: add support for Freescale IOMMU (PAMU) > > On Fri, 2013-12-06 at 12:59 -0600, Scott Wood wrote: > > On Thu, 2013-12-05 at 22:11 -0600, Bharat Bhushan wrote: > > > > > > > -Original Message- > > > > From: Wood Scott-B07421 > > > > Sent: Friday, December 06, 2013 5:52 AM > > > > To: Bhushan Bharat-R65777 > > > > Cc: Alex Williamson; linux-...@vger.kernel.org; ag...@suse.de; > > > > Yoder Stuart- B08248; io...@lists.linux-foundation.org; > > > > bhelg...@google.com; linuxppc- d...@lists.ozlabs.org; > > > > linux-ker...@vger.kernel.org > > > > Subject: Re: [PATCH 0/9 v2] vfio-pci: add support for Freescale > > > > IOMMU (PAMU) > > > > > > > > On Thu, 2013-11-28 at 03:19 -0600, Bharat Bhushan wrote: > > > > > > > > > > > -Original Message- > > > > > > From: Bhushan Bharat-R65777 > > > > > > Sent: Wednesday, November 27, 2013 9:39 PM > > > > > > To: 'Alex Williamson' > > > > > > Cc: Wood Scott-B07421; linux-...@vger.kernel.org; > > > > > > ag...@suse.de; Yoder Stuart- B08248; > > > > > > io...@lists.linux-foundation.org; bhelg...@google.com; > > > > > > linuxppc- d...@lists.ozlabs.org; linux-ker...@vger.kernel.org > > > > > > Subject: RE: [PATCH 0/9 v2] vfio-pci: add support for > > > > > > Freescale IOMMU (PAMU) > > > > > > > > > > > > If we just provide the size of MSI bank to userspace then > > > > > > userspace cannot do anything wrong. > > > > > > > > > > So userspace does not know address, so it cannot mmap and cause > > > > > any > > > > interference by directly reading/writing. > > > > > > > > That's security through obscurity... Couldn't the malicious user > > > > find out the address via other means, such as experimentation on > > > > another system over which they have full control? What would > > > > happen if the user reads from their device's PCI config space? Or > > > > gets the information via some back door in the PCI device they > > > > own? Or pokes throughout the address space looking for something that > generates an interrupt to its own device? > > > > > > So how to solve this problem, Any suggestion ? > > > > > > We have to map one window in PAMU for MSIs and a malicious user can > > > ask its device to do DMA to MSI window region with any pair of > > > address and data, which can lead to unexpected MSIs in system? > > > > I don't think there are any solutions other than to limit each bank to > > one user, unless the admin turns some knob that says they're OK with > > the partial loss of isolation. > > Even if the admin does opt-in to an allow_unsafe_interrupts options, it should > still be reasonably difficult for one guest to interfere with the other. I > don't think we want to rely on the blind luck of making the full MSI bank > accessible to multiple guests and hoping they don't step on each other. Not sure how to solve in this case (sharing MSI page) > That probably means that vfio needs to manage the space rather than the > guest. What you mean by " vfio needs to manage the space rather than the guest"? Thanks -Bharat > Thanks, > > Alex > ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 0/9 v2] vfio-pci: add support for Freescale IOMMU (PAMU)
> -Original Message- > From: Alex Williamson [mailto:alex.william...@redhat.com] > Sent: Tuesday, December 10, 2013 11:23 AM > To: Bhushan Bharat-R65777 > Cc: Wood Scott-B07421; linux-...@vger.kernel.org; ag...@suse.de; Yoder Stuart- > B08248; io...@lists.linux-foundation.org; bhelg...@google.com; linuxppc- > d...@lists.ozlabs.org; linux-ker...@vger.kernel.org > Subject: Re: [PATCH 0/9 v2] vfio-pci: add support for Freescale IOMMU (PAMU) > > On Tue, 2013-12-10 at 05:37 +, bharat.bhus...@freescale.com wrote: > > > > > -Original Message- > > > From: Alex Williamson [mailto:alex.william...@redhat.com] > > > Sent: Saturday, December 07, 2013 1:00 AM > > > To: Wood Scott-B07421 > > > Cc: Bhushan Bharat-R65777; linux-...@vger.kernel.org; ag...@suse.de; > > > Yoder Stuart-B08248; io...@lists.linux-foundation.org; > > > bhelg...@google.com; linuxppc- d...@lists.ozlabs.org; > > > linux-ker...@vger.kernel.org > > > Subject: Re: [PATCH 0/9 v2] vfio-pci: add support for Freescale > > > IOMMU (PAMU) > > > > > > On Fri, 2013-12-06 at 12:59 -0600, Scott Wood wrote: > > > > On Thu, 2013-12-05 at 22:11 -0600, Bharat Bhushan wrote: > > > > > > > > > > > -Original Message- > > > > > > From: Wood Scott-B07421 > > > > > > Sent: Friday, December 06, 2013 5:52 AM > > > > > > To: Bhushan Bharat-R65777 > > > > > > Cc: Alex Williamson; linux-...@vger.kernel.org; ag...@suse.de; > > > > > > Yoder Stuart- B08248; io...@lists.linux-foundation.org; > > > > > > bhelg...@google.com; linuxppc- d...@lists.ozlabs.org; > > > > > > linux-ker...@vger.kernel.org > > > > > > Subject: Re: [PATCH 0/9 v2] vfio-pci: add support for > > > > > > Freescale IOMMU (PAMU) > > > > > > > > > > > > On Thu, 2013-11-28 at 03:19 -0600, Bharat Bhushan wrote: > > > > > > > > > > > > > > > -Original Message- > > > > > > > > From: Bhushan Bharat-R65777 > > > > > > > > Sent: Wednesday, November 27, 2013 9:39 PM > > > > > > > > To: 'Alex Williamson' > > > > > > > > Cc: Wood Scott-B07421; linux-...@vger.kernel.org; > > > > > > > > ag...@suse.de; Yoder Stuart- B08248; > > > > > > > > io...@lists.linux-foundation.org; bhelg...@google.com; > > > > > > > > linuxppc- d...@lists.ozlabs.org; > > > > > > > > linux-ker...@vger.kernel.org > > > > > > > > Subject: RE: [PATCH 0/9 v2] vfio-pci: add support for > > > > > > > > Freescale IOMMU (PAMU) > > > > > > > > > > > > > > > > If we just provide the size of MSI bank to userspace then > > > > > > > > userspace cannot do anything wrong. > > > > > > > > > > > > > > So userspace does not know address, so it cannot mmap and > > > > > > > cause any > > > > > > interference by directly reading/writing. > > > > > > > > > > > > That's security through obscurity... Couldn't the malicious > > > > > > user find out the address via other means, such as > > > > > > experimentation on another system over which they have full > > > > > > control? What would happen if the user reads from their > > > > > > device's PCI config space? Or gets the information via some > > > > > > back door in the PCI device they own? Or pokes throughout the > > > > > > address space looking for something that > > > generates an interrupt to its own device? > > > > > > > > > > So how to solve this problem, Any suggestion ? > > > > > > > > > > We have to map one window in PAMU for MSIs and a malicious user > > > > > can ask its device to do DMA to MSI window region with any pair > > > > > of address and data, which can lead to unexpected MSIs in system? > > > > > > > > I don't think there are any solutions other than to limit each > > > > bank to one user, unless the admin turns some knob that says > > > > they're OK with the partial loss of isolation. > > > > > > Even if the admin does opt-in to an allow_unsafe_interrupts options, > > > it should still be reasona
RE: [1/3] powerpc/vfio: Enable on POWERNV platform
> -Original Message-
> From: Wood Scott-B07421
> Sent: Saturday, December 14, 2013 2:33 AM
> To: Alexey Kardashevskiy
> Cc: linuxppc-dev@lists.ozlabs.org; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Alex Williamson; Paul Mackerras; David Gibson; Sethi
> Varun-B16395; Bhushan Bharat-R65777
> Subject: Re: [1/3] powerpc/vfio: Enable on POWERNV platform
>
> On Fri, 2013-12-13 at 14:02 +1100, Alexey Kardashevskiy wrote:
> > On 12/13/2013 10:35 AM, Scott Wood wrote:
> > > On Tue, May 21, 2013 at 01:33:09PM +1000, Alexey Kardashevskiy wrote:
> > >> +static int iommu_add_device(struct device *dev) {
> > >> +struct iommu_table *tbl;
> > >> +int ret = 0;
> > >> +
> > >> +if (WARN_ON(dev->iommu_group)) {
> > >> +pr_warn("iommu_tce: device %s is already in iommu group
> > >> %d,
> skipping\n",
> > >> +dev_name(dev),
> > >> +iommu_group_id(dev->iommu_group));
> > >> +return -EBUSY;
> > >> +}
> > > [snip]
> > >> +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);
> > >
> > > This is missing a check to see whether the appropriate hardware is
> > > present. This file should also be renamed to something less
> > > generic, and depend on a kconfig symbol more specific than CONFIG_PPC64.
> > >
> > > When this is combined with CONFIG_FSL_PAMU on hardware with a PAMU,
> > > I get a bunch of those "WARN_ON(dev->iommu_group)" dumps because
> > > PAMU already got to them. Presumably without PAMU it silently (or
> > > with just pr_debug) bails out at some other point.
> >
> >
> > I posted (yet again) yesterday "[PATCH v11] PPC: POWERNV: move
> > iommu_add_device earlier" which should fix this. And Bharat asked many
> > times for this to get accepted :)
>
> I still get the WARN_ONs even with that patch. You're still registering the
> bus
> notifier unconditionally.
I have not tried v11 but tested V9 version of that patch. And yes, in that
version the bus notifier was not registered unconditionally in kernel/iommu.c .
Thanks
-Bharat
>
> -Scott
>
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v11] PPC: POWERNV: move iommu_add_device earlier
> -Original Message-
> From: Alexey Kardashevskiy [mailto:a...@ozlabs.ru]
> Sent: Thursday, December 12, 2013 1:24 PM
> To: linuxppc-dev@lists.ozlabs.org
> Cc: Alexey Kardashevskiy; Benjamin Herrenschmidt; Bhushan Bharat-R65777; Alex
> Graf; linux-ker...@vger.kernel.org
> Subject: [PATCH v11] 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:
> v11:
> * rebased on upstream
>
> v10:
> * fixed linker error when IOMMU_API is not enabled
>
> v9:
> * removed "KVM" from the subject as it is not really a KVM patch so
> PPC mainainter (hi Ben!) can review/include it into his tree
>
> 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| 26
> arch/powerpc/kernel/iommu.c | 11 --
> arch/powerpc/platforms/powernv/pci-ioda.c | 8
> arch/powerpc/platforms/powernv/pci-p5ioc2.c | 2 +-
> arch/powerpc/platforms/powernv/pci.c| 31
> -
> arch/powerpc/platforms/pseries/iommu.c | 8 +---
> 6 files changed, 70 insertions(+), 16 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/iommu.h
> b/arch/powerpc/include/asm/iommu.h
> index c34656a..774fa27 100644
> --- a/arch/powerpc/include/asm/iommu.h
> +++ b/arch/powerpc/include/asm/iommu.h
> @@ -101,8 +101,34 @@ extern void iommu_free_table(struct iommu_table *tbl,
> const
> char *node_name);
> */
> extern struct iommu_table *iommu_init_table(struct iommu_table * tbl,
> int nid);
> +#ifdef CONFIG_IOMMU_API
> 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);
> +#else
> +static inline void iommu_register_group(struct iommu_table *tbl,
> + int pci_domain_number,
> + unsigned long pe_num)
> +{
> +}
> +
> +static inline int iommu_add_device(struct device *dev)
> +{
> + return 0;
> +}
> +
> +static inline void iommu_del_device(struct device *dev)
> +{
> +}
> +#endif /* !CONFIG_IOMMU_API */
> +
> +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 572bb5b..818a092 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,11 +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);
> }
> +EXPORT_SYMBOL_GPL(iommu_del_device);
>
> static int iommu_bus_notifier(struct notifier_block *nb,
> unsigned long action, void *data)
> @@ -1162,13 +1164,8 @@ static struct notifier_block tce_iommu_bus_nb = {
>
> 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
RE: [RESEND PATCH] memory: Freescale CoreNet Coherency Fabric error reporting driver
> -Original Message-
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+bharat.bhushan=freescale@lists.ozlabs.org] On Behalf Of Scott Wood
> Sent: Saturday, May 31, 2014 3:58 AM
> To: Greg Kroah-Hartman
> Cc: linuxppc-dev@lists.ozlabs.org; linux-ker...@vger.kernel.org
> Subject: [RESEND PATCH] memory: Freescale CoreNet Coherency Fabric error
> reporting driver
>
> The CoreNet Coherency Fabric is part of the memory subsystem on some Freescale
> QorIQ chips. It can report coherency violations (e.g.
> due to misusing memory that is mapped noncoherent) as well as transactions
> that
> do not hit any local access window, or which hit a local access window with an
> invalid target ID.
>
> Signed-off-by: Scott Wood
> ---
> Resending to the proper list addresses -- sorry for the duplicate.
>
> arch/powerpc/configs/corenet32_smp_defconfig | 1 +
> arch/powerpc/configs/corenet64_smp_defconfig | 1 +
> drivers/memory/Kconfig | 10 ++
> drivers/memory/Makefile | 1 +
> drivers/memory/fsl-corenet-cf.c | 246
> +++
> 5 files changed, 259 insertions(+)
> create mode 100644 drivers/memory/fsl-corenet-cf.c
>
> diff --git a/arch/powerpc/configs/corenet32_smp_defconfig
> b/arch/powerpc/configs/corenet32_smp_defconfig
> index c19ff05..0c99d7e 100644
> --- a/arch/powerpc/configs/corenet32_smp_defconfig
> +++ b/arch/powerpc/configs/corenet32_smp_defconfig
> @@ -179,3 +179,4 @@ CONFIG_CRYPTO_SHA512=y CONFIG_CRYPTO_AES=y #
> CONFIG_CRYPTO_ANSI_CPRNG is not set CONFIG_CRYPTO_DEV_FSL_CAAM=y
> +CONFIG_FSL_CORENET_CF=y
> diff --git a/arch/powerpc/configs/corenet64_smp_defconfig
> b/arch/powerpc/configs/corenet64_smp_defconfig
> index 5c7fa19..8fb616d 100644
> --- a/arch/powerpc/configs/corenet64_smp_defconfig
> +++ b/arch/powerpc/configs/corenet64_smp_defconfig
> @@ -175,3 +175,4 @@ CONFIG_CRYPTO_SHA256=y CONFIG_CRYPTO_SHA512=y #
> CONFIG_CRYPTO_ANSI_CPRNG is not set CONFIG_CRYPTO_DEV_FSL_CAAM=y
> +CONFIG_FSL_CORENET_CF=y
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig index
> c59e9c9..fab81a1 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -61,6 +61,16 @@ config TEGRA30_MC
> analysis, especially for IOMMU/SMMU(System Memory Management
> Unit) module.
>
> +config FSL_CORENET_CF
> + tristate "Freescale CoreNet Error Reporting"
> + depends on FSL_SOC_BOOKE
> + help
> + Say Y for reporting of errors from the Freescale CoreNet
> + Coherency Fabric. Errors reported include accesses to
> + physical addresses that mapped by no local access window
> + (LAW) or an invalid LAW, as well as bad cache state that
> + represents a coherency violation.
> +
> config FSL_IFC
> bool
> depends on FSL_SOC
> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile index
> 71160a2..4055c47 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_OF) += of_memory.o
> endif
> obj-$(CONFIG_TI_AEMIF) += ti-aemif.o
> obj-$(CONFIG_TI_EMIF)+= emif.o
> +obj-$(CONFIG_FSL_CORENET_CF) += fsl-corenet-cf.o
> obj-$(CONFIG_FSL_IFC)+= fsl_ifc.o
> obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o
> obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o
> diff --git a/drivers/memory/fsl-corenet-cf.c b/drivers/memory/fsl-corenet-cf.c
> new file mode 100644 index 000..a57a614
> --- /dev/null
> +++ b/drivers/memory/fsl-corenet-cf.c
> @@ -0,0 +1,246 @@
> +/*
> + * CoreNet Coherency Fabric error reporting
> + *
> + * Copyright 2014 Freescale Semiconductor Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> +modify it
> + * under the terms of the GNU General Public License as published by
> +the
> + * Free Software Foundation; either version 2 of the License, or (at
> +your
> + * option) any later version.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +enum ccf_version {
> + CCF1,
> + CCF2,
> +};
> +
> +struct ccf_info {
> + enum ccf_version version;
> + int err_reg_offs;
> +};
> +
> +static const struct ccf_info ccf1_info = {
> + .version = CCF1,
> + .err_reg_offs = 0xa00,
> +};
> +
> +static const struct ccf_info ccf2_info = {
> + .version = CCF2,
> + .err_reg_offs = 0xe40,
> +};
> +
> +static const struct of_device_id ccf_matches[] = {
> + {
> + .compatible = "fsl,corenet1-cf",
> + .data = &ccf1_info,
> + },
> + {
> + .compatible = "fsl,corenet2-cf",
> + .data = &ccf2_info,
> + },
> + {}
> +};
> +
> +struct ccf_err_regs {
> + u32 errdet; /* 0x00 Error Detect Register */
> + /* 0x04 Error Enable (ccf1)/Disable (ccf2) Register */
> + u32 errdis;
> + /* 0x08 Error Interrupt Enable Register (ccf2 only
RE: [RESEND PATCH] memory: Freescale CoreNet Coherency Fabric error reporting driver
> -Original Message-
> From: Wood Scott-B07421
> Sent: Wednesday, June 04, 2014 10:12 PM
> To: Bhushan Bharat-R65777
> Cc: Greg Kroah-Hartman; linuxppc-dev@lists.ozlabs.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [RESEND PATCH] memory: Freescale CoreNet Coherency Fabric error
> reporting driver
>
> On Wed, 2014-06-04 at 03:17 -0500, Bhushan Bharat-R65777 wrote:
> > > +struct ccf_err_regs {
> > > + u32 errdet; /* 0x00 Error Detect Register */
> > > + /* 0x04 Error Enable (ccf1)/Disable (ccf2) Register */
> > > + u32 errdis;
> > > + /* 0x08 Error Interrupt Enable Register (ccf2 only) */
> > > + u32 errinten;
> > > + u32 cecar; /* 0x0c Error Capture Attribute Register */
> > > + u32 cecadrh;/* 0x10 Error Capture Address High */
> >
> > s/cecadrh/cecaddrh/g
> > This way we will be consistent with Reference manual.
>
> It's "cecadrh" in ccf1 and "cecaddrh" in ccf2. I suppose I should use the
> latter since "errdet/errdis/errinten" are the ccf2 names.
>
> > > + u32 cecadrl;/* 0x14 Error Capture Address Low */
> >
> > s/cecadrl/cecaddrl/g
> >
> > > + u32 cecar2; /* 0x18 Error Capture Attribute Register 2 */
> > > +};
> > > +
> > > +/* LAE/CV also valid for errdis and errinten */
> > > +#define ERRDET_LAE (1 << 0) /* Local Access Error */
> > > +#define ERRDET_CV(1 << 1) /* Coherency Violation */
> > > +#define ERRDET_CTYPE_SHIFT 26/* Capture Type (ccf2 only) */
> > > +#define ERRDET_CTYPE_MASK(0x3f << ERRDET_CTYPE_SHIFT)
> >
> > Should not this be (0x1f << ERRDET_CTYPE_SHIFT)
>
> Yes, thanks for catching that.
>
> > > +#define ERRDET_CAP (1 << 31) /* Capture Valid (ccf2 only)
> > > */
> > > +
> > > +#define CECAR_VAL(1 << 0) /* Valid (ccf1 only) */
> > > +#define CECAR_UVT(1 << 15) /* Unavailable target ID
> > > (ccf1) */
> > > +#define CECAR_SRCID_SHIFT_CCF1 24
> > > +#define CECAR_SRCID_MASK_CCF1(0xff << CECAR_SRCID_SHIFT_CCF1)
> > > +#define CECAR_SRCID_SHIFT_CCF2 18
> > > +#define CECAR_SRCID_MASK_CCF2(0xff << CECAR_SRCID_SHIFT_CCF2)
> > > +
> > > +#define CECADRH_ADDRH0xf
> >
> > On ccf2 this id 0xff.
>
> OK. I think we can get away with using 0xff on both.
>
> > > +static int ccf_remove(struct platform_device *pdev) {
> > > + struct ccf_private *ccf = dev_get_drvdata(&pdev->dev);
> > > +
> > > + switch (ccf->info->version) {
> > > + case CCF1:
> > > + iowrite32be(0, &ccf->err_regs->errdis);
> > > + break;
> > > +
> > > + case CCF2:
> > > + iowrite32be(0, &ccf->err_regs->errinten);
> >
> > Do you think it is same to disable detection bits in ccf->err_regs->errdis?
>
> Disabling the interrupt is what we're aiming for here, but ccf1 doesn't
> provide
> a way to do that separate from disabling detection.
What I wanted to say that do we also need to disable detection (set ERRDET_LAE
| ERRDET_CV bits in errdis) apart from clearing errinten on ccf2 ?
Thanks
-Bharat
>
> -Scott
>
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] KVM: PPC: Book3E: Unlock mmu_lock when setting caching atttribute
> -Original Message-
> From: Mihai Caraman [mailto:mihai.cara...@freescale.com]
> Sent: Wednesday, June 18, 2014 9:15 PM
> To: kvm-...@vger.kernel.org
> Cc: k...@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; Caraman Mihai
> Claudiu-
> B02008; Bhushan Bharat-R65777
> Subject: [PATCH] KVM: PPC: Book3E: Unlock mmu_lock when setting caching
> atttribute
>
> The patch 08c9a188d0d0fc0f0c5e17d89a06bb59c493110f
> kvm: powerpc: use caching attributes as per linux pte
> do not handle properly the error case, letting mmu_lock locked. The lock
> will further generate a RCU stall from kvmppc_e500_emul_tlbwe() caller.
>
> In case of an error go to out label.
>
> Signed-off-by: Mihai Caraman
> Cc: Bharat Bhushan
Thanks mike for fixing this; I am curious to know how you reached to this point
:)
Reviewed-by: Bharat Bhushan
Regards
-Bharat
> ---
> arch/powerpc/kvm/e500_mmu_host.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kvm/e500_mmu_host.c
> b/arch/powerpc/kvm/e500_mmu_host.c
> index 0528fe5..54144c7 100644
> --- a/arch/powerpc/kvm/e500_mmu_host.c
> +++ b/arch/powerpc/kvm/e500_mmu_host.c
> @@ -473,7 +473,8 @@ static inline int kvmppc_e500_shadow_map(struct
> kvmppc_vcpu_e500 *vcpu_e500,
> if (printk_ratelimit())
> pr_err("%s: pte not present: gfn %lx, pfn %lx\n",
> __func__, (long)gfn, pfn);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
> kvmppc_e500_ref_setup(ref, gtlbe, pfn, wimg);
>
> --
> 1.7.11.7
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] vfio: Fix endianness handling for emulated BARs
> -Original Message- > From: Linuxppc-dev [mailto:linuxppc-dev- > bounces+bharat.bhushan=freescale@lists.ozlabs.org] On Behalf Of Alexey > Kardashevskiy > Sent: Thursday, June 19, 2014 9:18 AM > To: Alex Williamson > Cc: k...@vger.kernel.org; Nikunj A Dadhania; linux-ker...@vger.kernel.org; > Alexander Graf; linuxppc-dev@lists.ozlabs.org > Subject: Re: [PATCH] vfio: Fix endianness handling for emulated BARs > > On 06/19/2014 11:50 AM, Alexey Kardashevskiy wrote: > > On 06/19/2014 10:50 AM, Alexey Kardashevskiy wrote: > >> On 06/19/2014 04:35 AM, Alex Williamson wrote: > >>> On Wed, 2014-06-18 at 21:36 +1000, Alexey Kardashevskiy wrote: > VFIO exposes BARs to user space as a byte stream so userspace can > read it using pread()/pwrite(). Since this is a byte stream, VFIO > should not do byte swapping and simply return values as it gets > them from PCI device. > > Instead, the existing code assumes that byte stream in read/write > is little-endian and it fixes endianness for values which it passes > to ioreadXX/iowriteXX helpers. This works for little-endian as PCI > is little endian and le32_to_cpu/... are stubs. > >>> > >>> vfio read32: > >>> > >>> val = cpu_to_le32(ioread32(io + off)); > >>> > >>> Where the typical x86 case, ioread32 is: > >>> > >>> #define ioread32(addr) readl(addr) > >>> > >>> and readl is: > >>> > >>> __le32_to_cpu(__raw_readl(addr)); > >>> > >>> So we do canceling byte swaps, which are both nops on x86, and end > >>> up returning device endian, which we assume is little endian. > >>> > >>> vfio write32 is similar: > >>> > >>> iowrite32(le32_to_cpu(val), io + off); > >>> > >>> The implicit cpu_to_le32 of iowrite32() and our explicit swap cancel > >>> out, so input data is device endian, which is assumed little. > >>> > This also works for big endian but rather by an accident: it reads > 4 bytes from the stream (@val is big endian), converts to CPU > format (which should be big endian) as it was little endian (@val > becomes actually little > endian) and calls iowrite32() which does not do swapping on big > endian system. > >>> > >>> Really? > >>> > >>> In arch/powerpc/kernel/iomap.c iowrite32() is just a wrapper around > >>> writel(), which seems to use the generic implementation, which does > >>> include a cpu_to_le32. > >> > >> > >> Ouch, wrong comment. iowrite32() does swapping. My bad. > >> > >> > >>> > >>> I also see other big endian archs like parisc doing cpu_to_le32 on > >>> iowrite32, so I don't think this statement is true. I imagine it's > >>> probably working for you because the swap cancel. > >>> > This removes byte swapping and makes use ioread32be/iowrite32be > (and 16bit versions) on big-endian systems. The "be" helpers take > native endian values and do swapping at the moment of writing to a > PCI register using one of "store byte-reversed" instructions. > >>> > >>> So now you want iowrite32() on little endian and iowrite32be() on > >>> big endian, the former does a cpu_to_le32 (which is a nop on little > >>> endian) and the latter does a cpu_to_be32 (which is a nop on big > >>> endian)... > >>> should we just be using __raw_writel() on both? > >> > >> > >> We can do that too. The beauty of iowrite32be on ppc64 is that it > >> does not swap and write separately, it is implemented via the "Store > >> Word Byte-Reverse Indexed X-form" single instruction. > >> > >> And some archs (do not know which ones) may add memory barriers in > >> their implementations of ioread/iowrite. __raw_writel is too raw :) > >> > >>> There doesn't actually > >>> seem to be any change in behavior here, it just eliminates > >>> back-to-back byte swaps, which are a nop on x86, but not power, right? > >> > >> Exactly. No dependency for QEMU. > > > > How about that: > > === > > > > VFIO exposes BARs to user space as a byte stream so userspace can read > > it using pread()/pwrite(). Since this is a byte stream, VFIO should > > not do byte swapping and simply return values as it gets them from PCI > > device. > > > > Instead, the existing code assumes that byte stream in read/write is > > little-endian and it fixes endianness for values which it passes to > > ioreadXX/iowriteXX helpers in native format. The IO helpers do > > swapping again. Since both byte swaps are nops on little-endian host, this > works. > > > > This also works for big endian but rather by an accident: it reads 4 > > bytes from the stream (@val is big endian), converts to CPU format > > (which should be big endian) as it was little endian (and @val becomes > > actually little > > endian) and calls iowrite32() which does swapping on big endian system > > again. So byte swap gets cancelled, __raw_writel() receives a native > > value and then *(volatile unsigned int __force *)PCI_FIX_ADDR(addr) = > > v; just does the right thing. > > I am wrong here, sorry. This is what happens when you watch soccer between 2am >
RE: [RESEND PATCH] memory: Freescale CoreNet Coherency Fabric error reporting driver
> -Original Message-
> From: Wood Scott-B07421
> Sent: Wednesday, June 04, 2014 10:38 PM
> To: Bhushan Bharat-R65777
> Cc: Greg Kroah-Hartman; linuxppc-dev@lists.ozlabs.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [RESEND PATCH] memory: Freescale CoreNet Coherency Fabric error
> reporting driver
>
> On Wed, 2014-06-04 at 12:04 -0500, Bhushan Bharat-R65777 wrote:
> >
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Wednesday, June 04, 2014 10:12 PM
> > > To: Bhushan Bharat-R65777
> > > Cc: Greg Kroah-Hartman; linuxppc-dev@lists.ozlabs.org; linux-
> > > ker...@vger.kernel.org
> > > Subject: Re: [RESEND PATCH] memory: Freescale CoreNet Coherency
> > > Fabric error reporting driver
> > >
> > > On Wed, 2014-06-04 at 03:17 -0500, Bhushan Bharat-R65777 wrote:
> > > > > +static int ccf_remove(struct platform_device *pdev) {
> > > > > + struct ccf_private *ccf = dev_get_drvdata(&pdev->dev);
> > > > > +
> > > > > + switch (ccf->info->version) {
> > > > > + case CCF1:
> > > > > + iowrite32be(0, &ccf->err_regs->errdis);
> > > > > + break;
> > > > > +
> > > > > + case CCF2:
> > > > > + iowrite32be(0, &ccf->err_regs->errinten);
> > > >
> > > > Do you think it is same to disable detection bits in ccf->err_regs-
> >errdis?
> > >
> > > Disabling the interrupt is what we're aiming for here, but ccf1
> > > doesn't provide a way to do that separate from disabling detection.
> >
> > What I wanted to say that do we also need to disable detection (set
> > ERRDET_LAE | ERRDET_CV bits in errdis) apart from clearing errinten on
> > ccf2 ?
>
> I don't think we "need" to. You could argue that we should for consistency,
> though I think there's value in errors continuing to be detected even without
> the driver (e.g. can dump the registers in a debugger).
Yes this comment was for consistency. Also IIUC, the state which is left when
the driver is removed is not default reset behavior.
If we want errors to be detected then should not we have a sysfs interface?
Thanks
-Bharat
>
> -Scott
>
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [RESEND PATCH] memory: Freescale CoreNet Coherency Fabric error reporting driver
> -Original Message-
> From: Wood Scott-B07421
> Sent: Tuesday, July 01, 2014 2:30 AM
> To: Bhushan Bharat-R65777
> Cc: Greg Kroah-Hartman; linuxppc-dev@lists.ozlabs.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [RESEND PATCH] memory: Freescale CoreNet Coherency Fabric error
> reporting driver
>
> On Sun, 2014-06-29 at 23:58 -0500, Bhushan Bharat-R65777 wrote:
> >
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Wednesday, June 04, 2014 10:38 PM
> > > To: Bhushan Bharat-R65777
> > > Cc: Greg Kroah-Hartman; linuxppc-dev@lists.ozlabs.org; linux-
> > > ker...@vger.kernel.org
> > > Subject: Re: [RESEND PATCH] memory: Freescale CoreNet Coherency
> > > Fabric error reporting driver
> > >
> > > On Wed, 2014-06-04 at 12:04 -0500, Bhushan Bharat-R65777 wrote:
> > > >
> > > > > -Original Message-
> > > > > From: Wood Scott-B07421
> > > > > Sent: Wednesday, June 04, 2014 10:12 PM
> > > > > To: Bhushan Bharat-R65777
> > > > > Cc: Greg Kroah-Hartman; linuxppc-dev@lists.ozlabs.org; linux-
> > > > > ker...@vger.kernel.org
> > > > > Subject: Re: [RESEND PATCH] memory: Freescale CoreNet Coherency
> > > > > Fabric error reporting driver
> > > > >
> > > > > On Wed, 2014-06-04 at 03:17 -0500, Bhushan Bharat-R65777 wrote:
> > > > > > > +static int ccf_remove(struct platform_device *pdev) {
> > > > > > > + struct ccf_private *ccf = dev_get_drvdata(&pdev->dev);
> > > > > > > +
> > > > > > > + switch (ccf->info->version) {
> > > > > > > + case CCF1:
> > > > > > > + iowrite32be(0, &ccf->err_regs->errdis);
> > > > > > > + break;
> > > > > > > +
> > > > > > > + case CCF2:
> > > > > > > + iowrite32be(0, &ccf->err_regs->errinten);
> > > > > >
> > > > > > Do you think it is same to disable detection bits in
> > > > > > ccf->err_regs-
> > > >errdis?
> > > > >
> > > > > Disabling the interrupt is what we're aiming for here, but ccf1
> > > > > doesn't provide a way to do that separate from disabling detection.
> > > >
> > > > What I wanted to say that do we also need to disable detection
> > > > (set ERRDET_LAE | ERRDET_CV bits in errdis) apart from clearing
> > > > errinten on
> > > > ccf2 ?
> > >
> > > I don't think we "need" to. You could argue that we should for
> > > consistency, though I think there's value in errors continuing to be
> > > detected even without the driver (e.g. can dump the registers in a
> debugger).
> >
> > Yes this comment was for consistency. Also IIUC, the state which is left
> > when
> the driver is removed is not default reset behavior.
>
> How many drivers leave the hardware in pristine reset state when exiting?
I do not know :)
> And
> you could argue that having detection off by default is poor hardware design
> (enabling interrupts is another matter of course).
Ok, then can you please add a comment in _remove() function describing why
detection is still enabled.
Thanks
-Bharat
>
> > If we want errors to be detected then should not we have a sysfs interface?
>
> That may be useful but it's beyond the scope of what I'm doing with this
> patch.
> We currently don't log machine checks anywhere but via printk either.
>
> BTW, I thought I had sent v2 of this, but I don't see it anywhere...
> I'll respin soon.
>
> -Scott
>
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v2] memory: Freescale CoreNet Coherency Fabric error reporting driver
> -Original Message-
> From: Wood Scott-B07421
> Sent: Thursday, July 03, 2014 5:22 AM
> To: linuxppc-dev@lists.ozlabs.org
> Cc: linux-ker...@vger.kernel.org; Bhushan Bharat-R65777
> Subject: [PATCH v2] memory: Freescale CoreNet Coherency Fabric error reporting
> driver
>
> The CoreNet Coherency Fabric is part of the memory subsystem on
> some Freescale QorIQ chips. It can report coherency violations (e.g.
> due to misusing memory that is mapped noncoherent) as well as
> transactions that do not hit any local access window, or which hit a
> local access window with an invalid target ID.
>
> Signed-off-by: Scott Wood
Reviewed-by: Bharat Bhushan
Regards
-Bharat
> ---
> v2:
> - s/cecadr/cecaddr/ to consistently use ccf2 names
> - fix ERRDET_CTYPE_MASK value
> - use the ccf2 value for CECADDRH_ADDRH (harmless on ccf1)
> - add comment explaining why we disable detection on remove for
>ccf1 but not ccf2
> ---
> arch/powerpc/configs/corenet32_smp_defconfig | 1 +
> arch/powerpc/configs/corenet64_smp_defconfig | 1 +
> drivers/memory/Kconfig | 10 ++
> drivers/memory/Makefile | 1 +
> drivers/memory/fsl-corenet-cf.c | 251
> +++
> 5 files changed, 264 insertions(+)
> create mode 100644 drivers/memory/fsl-corenet-cf.c
>
> diff --git a/arch/powerpc/configs/corenet32_smp_defconfig
> b/arch/powerpc/configs/corenet32_smp_defconfig
> index 7d0c837..6a3c58a 100644
> --- a/arch/powerpc/configs/corenet32_smp_defconfig
> +++ b/arch/powerpc/configs/corenet32_smp_defconfig
> @@ -180,3 +180,4 @@ CONFIG_CRYPTO_SHA512=y
> CONFIG_CRYPTO_AES=y
> # CONFIG_CRYPTO_ANSI_CPRNG is not set
> CONFIG_CRYPTO_DEV_FSL_CAAM=y
> +CONFIG_FSL_CORENET_CF=y
> diff --git a/arch/powerpc/configs/corenet64_smp_defconfig
> b/arch/powerpc/configs/corenet64_smp_defconfig
> index 6ae07e1..4b07bad 100644
> --- a/arch/powerpc/configs/corenet64_smp_defconfig
> +++ b/arch/powerpc/configs/corenet64_smp_defconfig
> @@ -179,3 +179,4 @@ CONFIG_CRYPTO_SHA256=y
> CONFIG_CRYPTO_SHA512=y
> # CONFIG_CRYPTO_ANSI_CPRNG is not set
> CONFIG_CRYPTO_DEV_FSL_CAAM=y
> +CONFIG_FSL_CORENET_CF=y
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index c59e9c9..fab81a1 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -61,6 +61,16 @@ config TEGRA30_MC
> analysis, especially for IOMMU/SMMU(System Memory Management
> Unit) module.
>
> +config FSL_CORENET_CF
> + tristate "Freescale CoreNet Error Reporting"
> + depends on FSL_SOC_BOOKE
> + help
> + Say Y for reporting of errors from the Freescale CoreNet
> + Coherency Fabric. Errors reported include accesses to
> + physical addresses that mapped by no local access window
> + (LAW) or an invalid LAW, as well as bad cache state that
> + represents a coherency violation.
> +
> config FSL_IFC
> bool
> depends on FSL_SOC
> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index 71160a2..4055c47 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_OF) += of_memory.o
> endif
> obj-$(CONFIG_TI_AEMIF) += ti-aemif.o
> obj-$(CONFIG_TI_EMIF)+= emif.o
> +obj-$(CONFIG_FSL_CORENET_CF) += fsl-corenet-cf.o
> obj-$(CONFIG_FSL_IFC)+= fsl_ifc.o
> obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o
> obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o
> diff --git a/drivers/memory/fsl-corenet-cf.c b/drivers/memory/fsl-corenet-cf.c
> new file mode 100644
> index 000..c9443fc
> --- /dev/null
> +++ b/drivers/memory/fsl-corenet-cf.c
> @@ -0,0 +1,251 @@
> +/*
> + * CoreNet Coherency Fabric error reporting
> + *
> + * Copyright 2014 Freescale Semiconductor Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +enum ccf_version {
> + CCF1,
> + CCF2,
> +};
> +
> +struct ccf_info {
> + enum ccf_version version;
> + int err_reg_offs;
> +};
> +
> +static const struct ccf_info ccf1_info = {
> + .version = CCF1,
> + .err_reg_offs = 0xa00,
> +};
> +
> +static const struct ccf_info ccf2_info = {
> + .version = CCF2,
> + .err_reg_offs = 0xe40,
> +};
> +
> +static const struct of_device_id ccf_matches[] = {
> + {
> + .compatible = "fsl,corenet1-cf",
> + .data = &ccf1_info,
> + },
> + {
> + .compatible = "fsl,corenet2-cf",
> + .data = &ccf2_info,
> + },
> + {}
> +};
> +
> +struct ccf_err_regs {
> + u32 errdet; /* 0x00 Error Detect Register */
> + /* 0x
RE: [PATCH v2 2/2] PCI/MSI: Remove arch_msi_check_device()
> -Original Message-
> From: linux-pci-ow...@vger.kernel.org [mailto:linux-pci-ow...@vger.kernel.org]
> On Behalf Of Alexander Gordeev
> Sent: Monday, August 11, 2014 5:16 PM
> To: Bjorn Helgaas
> Cc: linux-ker...@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> p...@vger.kernel.org
> Subject: [PATCH v2 2/2] PCI/MSI: Remove arch_msi_check_device()
>
> There are no archs that override arch_msi_check_device() hook. Remove it as it
> is completely redundant.
Is not we are still overriding this in powerpc (not sure I missed some patch ,
if so please point to that).
$ grep -r arch_msi_check_device arch/powerpc/
Binary file arch/powerpc/kernel/msi.o matches
arch/powerpc/kernel/msi.c:int arch_msi_check_device(struct pci_dev* dev, int
nvec, int type)
Binary file arch/powerpc/kernel/built-in.o matches
Thanks
-Bharat
>
> If an arch would need to check MSI/MSI-X possibility for a device it should
> make
> it within arch_setup_msi_irqs() hook.
>
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Alexander Gordeev
> ---
> drivers/pci/msi.c | 49 +
> include/linux/msi.h | 3 ---
> 2 files changed, 13 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index 5a40516..6c2cc41
> 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -56,16 +56,6 @@ void __weak arch_teardown_msi_irq(unsigned int irq)
> chip->teardown_irq(chip, irq);
> }
>
> -int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type) -{
> - struct msi_chip *chip = dev->bus->msi;
> -
> - if (!chip || !chip->check_device)
> - return 0;
> -
> - return chip->check_device(chip, dev, nvec, type);
> -}
> -
> int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) {
> struct msi_desc *entry;
> @@ -806,22 +796,23 @@ out_free:
> }
>
> /**
> - * pci_msi_check_device - check whether MSI may be enabled on a device
> + * pci_msi_supported - check whether MSI may be enabled on a device
> * @dev: pointer to the pci_dev data structure of MSI device function
> * @nvec: how many MSIs have been requested ?
> - * @type: are we checking for MSI or MSI-X ?
> *
> * Look at global flags, the device itself, and its parent buses
> * to determine if MSI/-X are supported for the device. If MSI/-X is
> * supported return 0, else return an error code.
> **/
> -static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
> +static int pci_msi_supported(struct pci_dev *dev, int nvec)
> {
> struct pci_bus *bus;
> - int ret;
>
> /* MSI must be globally enabled and supported by the device */
> - if (!pci_msi_enable || !dev || dev->no_msi)
> + if (!pci_msi_enable)
> + return -EINVAL;
> +
> + if (!dev || dev->no_msi || dev->current_state != PCI_D0)
> return -EINVAL;
>
> /*
> @@ -843,10 +834,6 @@ static int pci_msi_check_device(struct pci_dev *dev, int
> nvec, int type)
> if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
> return -EINVAL;
>
> - ret = arch_msi_check_device(dev, nvec, type);
> - if (ret)
> - return ret;
> -
> return 0;
> }
>
> @@ -949,13 +936,13 @@ int pci_enable_msix(struct pci_dev *dev, struct
> msix_entry
> *entries, int nvec)
> int status, nr_entries;
> int i, j;
>
> - if (!entries || !dev->msix_cap || dev->current_state != PCI_D0)
> - return -EINVAL;
> -
> - status = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSIX);
> + status = pci_msi_supported(dev, nvec);
> if (status)
> return status;
>
> + if (!entries)
> + return -EINVAL;
> +
> nr_entries = pci_msix_vec_count(dev);
> if (nr_entries < 0)
> return nr_entries;
> @@ -1062,8 +1049,9 @@ int pci_enable_msi_range(struct pci_dev *dev, int
> minvec,
> int maxvec)
> int nvec;
> int rc;
>
> - if (dev->current_state != PCI_D0)
> - return -EINVAL;
> + rc = pci_msi_supported(dev, minvec);
> + if (rc)
> + return rc;
>
> WARN_ON(!!dev->msi_enabled);
>
> @@ -1086,17 +1074,6 @@ int pci_enable_msi_range(struct pci_dev *dev, int
> minvec,
> int maxvec)
> nvec = maxvec;
>
> do {
> - rc = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSI);
> - if (rc < 0) {
> - return rc;
> - } else if (rc > 0) {
> - if (rc < minvec)
> - return -ENOSPC;
> - nvec = rc;
> - }
> - } while (rc);
> -
> - do {
> rc = msi_capability_init(dev, nvec);
> if (rc < 0) {
> return rc;
> diff --git a/include/linux/msi.h b/include/linux/msi.h index 8103f32..dbf7cc9
> 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -6
RE: [PATCH 3/4 RFC] fsl/msi: Add MSI bank allocation for kernel owned devices
> -Original Message-
> From: Wood Scott-B07421
> Sent: Thursday, March 12, 2015 4:53 AM
> To: Bhushan Bharat-R65777
> Cc: linuxppc-...@ozlabs.org
> Subject: Re: [PATCH 3/4 RFC] fsl/msi: Add MSI bank allocation for kernel
> owned devices
>
> On Tue, 2015-03-03 at 10:47 +0530, Bharat Bhushan wrote:
> > With this patch a "context" can allocate a MSI bank and use the
> > allocated MSI-bank for the devices in that "context".
> >
> > kernel/host "context" is "NULL", So all devices owned by kernel will
> > share a MSI bank allocated with "context = NULL.
> >
> > This patch is in direction to have separate MSI bank for kernel
> > context and userspace/VM context. We do not want two software context
> > (kernel and VMs) to share a MSI bank for safe/reliable interrupts with
> > full isolation. Follow up patch will add interface to allocate a MSI
> > bank for userspace/VM context.
> >
> > NOTE: This RFC patch allows only one MSI bank to be allocated for
> > kernel context. Which seems to be sufficient to me. But if we see this
> > is limiting some real usecase scanerio then this limitation can be
> > removed
> >
> > One issue which still need to addressed is when to free kernel context
> > allocated MSI bank? Say all MSI capable devices are assigned to
> > VM/userspace then there is no need to have any MSI bank reserved for
> > kernel context.
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > arch/powerpc/sysdev/fsl_msi.c | 88
> > ++-
> > arch/powerpc/sysdev/fsl_msi.h | 4 ++
> > 2 files changed, 83 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/powerpc/sysdev/fsl_msi.c
> > b/arch/powerpc/sysdev/fsl_msi.c index 32ba1e3..027aeeb 100644
> > --- a/arch/powerpc/sysdev/fsl_msi.c
> > +++ b/arch/powerpc/sysdev/fsl_msi.c
> > @@ -142,6 +142,79 @@ static void fsl_teardown_msi_irqs(struct pci_dev
> *pdev)
> > return;
> > }
> >
> > +/*
> > + * Allocate a MSI Bank for the requested "context".
> > + * NULL "context" means that this request is to allocate
> > + * MSI bank for kernel owned devices. And currently we
> > + * assume that one MSI bank is sufficient for kernel.
> > + */
> > +static struct fsl_msi *fsl_msi_allocate_msi_bank(void *context) {
> > + struct fsl_msi *msi_data;
> > +
> > + /* Kernel context (NULL) can reserve only one msi bank */
> > + if (!context) {
> > + list_for_each_entry(msi_data, &msi_head, list) {
> > + if ((msi_data->reserved == MSI_RESERVED) &&
> > + (msi_data->context == NULL))
> > + return NULL;
> > + }
>
> Unnecessary parentheses
>
> Is there any reason why the kernel bank needs to participate in this
> mechanism at all? Set it aside at MSI driver init, and don't put it on
> the list. I know you've previously said that you want to support configs
> where the kernel doesn't get any banks, but that can just be a boot
> option that tells the MSI code to not set aside a bank for the kernel.
> It would simplify the code.
I think we cannot completely remove the MSI bank from kernel practically. So I
can make the kernel msi bank out of list.
>
> With the patchset as is, how would one indicate whether kernel devices
> should get a bank?
For kernel owned device, in fsl_setup_msi_irqs() we check if there is a
reserved MSI bank, if not then reserve a msi bank. If reserve fails then
setup_msi_irq() fails. I think there is no fallback to Legacy interrupt.
> Specifically, when the kernel does have an MSI-
> capable device but we'd prefer to use legacy interrupts to keep MSIs
> available to VFIO.
Do we want this?
>
> > + }
> > +
> > + list_for_each_entry(msi_data, &msi_head, list) {
> > + if (msi_data->reserved == MSI_FREE) {
> > + msi_data->reserved = MSI_RESERVED;
> > + msi_data->context = context;
> > + return msi_data;
> > + }
> > + }
>
> What prevents races from parallel callers?
Will add a lock.
>
> > +/* FIXME: Assumption that host kernel will allocate only one MSI bank
> > +*/
>
> It's not a FIXME if we think the limitation is not burdensome.
Ok
>
> > + __attribute__ ((unused)) static int fsl_msi_free_msi_bank(void
> > + *context)
>
> static int __maybe_unused fsl_msi_free_msi_bank(void *context)
>
> Why are you adding this function then removing it in the next patch?
Will fix this
>
> > + /* If no MSI bank allocated for kernel owned device, allocate one
> */
> > + msi_data = fsl_msi_allocate_msi_bank(NULL);
> > + if (msi_data)
> > + return msi_data;
> > +
> > + return NULL;
>
> return fsl_msi_allocate_msi_bank(NULL);
Ok
>
>
> > +}
> > +
> > static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq,
> > struct msi_msg *msg,
> > struct fsl_msi *fsl_msi_data)
> > @@ -174,7 +247,7 @@ static int fsl_setup_msi_irqs(struct pci_dev *pdev,
> int nvec, int type)
>
RE: [PATCH 4/4 RFC] fsl/msi: Add interface to reserve/free msi bank
> -Original Message-
> From: Wood Scott-B07421
> Sent: Thursday, March 12, 2015 5:48 AM
> To: Bhushan Bharat-R65777
> Cc: linuxppc-...@ozlabs.org
> Subject: Re: [PATCH 4/4 RFC] fsl/msi: Add interface to reserve/free msi
> bank
>
> On Tue, 2015-03-03 at 10:47 +0530, Bharat Bhushan wrote:
> > This patch allows a context (different from kernel context) to reserve
> > a MSI bank for itself. And then the devices in the context will share
> > the MSI bank.
> >
> > VFIO meta driver is one of typical user of these APIs. It will reserve
> > a MSI bank for MSI interrupt support of direct assignment PCI devices
> > to a Guest. Patches for same will follow this patch.
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > arch/powerpc/include/asm/device.h | 2 +
> > arch/powerpc/include/asm/fsl_msi.h | 26 ++
> > arch/powerpc/sysdev/fsl_msi.c | 169
> +++--
> > arch/powerpc/sysdev/fsl_msi.h | 1 +
> > 4 files changed, 173 insertions(+), 25 deletions(-) create mode
> > 100644 arch/powerpc/include/asm/fsl_msi.h
> >
> > diff --git a/arch/powerpc/include/asm/device.h
> > b/arch/powerpc/include/asm/device.h
> > index 38faede..1c2bfd7 100644
> > --- a/arch/powerpc/include/asm/device.h
> > +++ b/arch/powerpc/include/asm/device.h
> > @@ -40,6 +40,8 @@ struct dev_archdata { #ifdef CONFIG_FAIL_IOMMU
> > int fail_iommu;
> > #endif
> > +
> > + void *context;
> > };
>
> This needs a comment and probably a more specific name.
Ok
>
> > @@ -200,6 +185,12 @@ static struct fsl_msi
> > *fsl_msi_get_reserved_msi_bank(struct pci_dev *pdev) {
> > struct fsl_msi *msi_data = NULL;
> > void *context = NULL;
> > + struct device *dev = &pdev->dev;
> > +
> > + /* Device assigned to userspace if there is valid context */
> > + if (dev->archdata.context) {
> > + context = dev->archdata.context;
> > + }
>
> No {}
Ok, leftover of print debugging :)
>
> >
> > list_for_each_entry(msi_data, &msi_head, list) {
> > if ((msi_data->reserved == MSI_RESERVED) && @@ -208,13
> +199,133 @@
> > static struct fsl_msi *fsl_msi_get_reserved_msi_bank(struct pci_dev
> *pdev)
> > }
> >
> > /* If no MSI bank allocated for kernel owned device, allocate one
> */
> > - msi_data = fsl_msi_allocate_msi_bank(NULL);
> > - if (msi_data)
> > - return msi_data;
> > + if (!context) {
> > + msi_data = fsl_msi_allocate_msi_bank(NULL);
> > + if (msi_data)
> > + return msi_data;
> > + }
> >
> > return NULL;
> > }
> >
> > +/* API to set "context" to which the device belongs */ int
> > +fsl_msi_set_msi_bank_in_dev(struct device *dev, void *data) {
> > + dev->archdata.context = data;
> > + return 0;
> > +}
>
> Do we really need "msi" to appear twice in every function name?
No, will remove later "msi".
>
> > +
> > +/* This API Allows a MSI bank to be reserved for a "context".
> > + * All devices in same "context" will share the allocated
> > + * MSI bank.
> > + * Typically this function will be called from meta
> > + * driver like VFIO with a valid "context".
> > + */
> > +struct fsl_msi *fsl_msi_reserve_msi_bank(void *context) {
> > + struct fsl_msi *msi_data;
> > +
> > + if (!context)
> > + return NULL;
> > +
> > + /* Check if msi-bank already allocated for the context */
> > + list_for_each_entry(msi_data, &msi_head, list) {
> > + if (msi_data->reserved == MSI_FREE)
> > + continue;
> > +
> > + if (context == msi_data->context)
> > + return msi_data;
>
> The reserved == MSI_FREE check doesn't add anything because if it's free
> then the context check will fail.
ok
>
> > +static int is_msi_bank_reserved(struct fsl_msi *msi)
>
> s/int/bool/
Ok
>
>
> > +/*
> > + * This function configures PAMU window for MSI page with
> > + * given iova. Also same iova will be used as "msi-address"
> > + * when configuring msi-message in the devices using this
> > + * msi bank.
> > + */
> > +int fsl_msi_set_msi_bank_region(struct iommu_domain *domain,
> > + void *context , int win,
>
> Whitespace
>
> > @@ -225,12 +336,17 @@ static void fsl_compose_msi_msg(struct pci_dev
> *pdev, int hwirq,
> > int len;
> > const __be64 *reg;
> >
> > - /* If the msi-address-64 property exists, then use it */
> > - reg = of_get_property(hose->dn, "msi-address-64", &len);
> > - if (reg && (len == sizeof(u64)))
> > - address = be64_to_cpup(reg);
> > - else
> > - address = msi_data->msiir;
> > + if (pdev->dev.archdata.context) {
> > + address = msi_data->iova;
> > + } else {
> > + /* If the msi-address-64 property exists, then use it */
> > + reg = of_get_property(hose->dn, "msi-address-64", &len);
> > + if (reg && (len == sizeof(u64)))
> > + address = be64_to_cpup(reg);
> > + else
> > + address
