* [PATCH v4 0/3] powerpc/85xx: introduce corenet_generic machine
From: Kevin Hao @ 2013-09-26 1:42 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc
v4:
- Update the patch 3 according to Scott's comment.
v3:
- Update the patch 1 according to Scott's comment.
- Introduce one new patch to combine the board kernel options into one.
v2:
- Fold the original patch 2 into patch 1.
- Update the patch 1 according to Scott and Kumar's comments.
- Introduce a new patch to rename the corenet_ds.c to corenet_generic.c.
v1:
This patch series introduces a common machine to support p2041rdb, p3041ds,
p4080ds, p5020ds, p5040ds, t4240qds and b4qds to avoid the code duplication.
Boot test on p5020ds and p4080ds.
Kevin Hao (3):
powerpc/85xx: introduce corenet_generic machine
powerpc/85xx: rename the corenet_ds.c to corenet_generic.c
powerpc/85xx: use one kernel option for all the CoreNet_Generic boards
arch/powerpc/configs/corenet32_smp_defconfig | 6 +-
arch/powerpc/configs/corenet64_smp_defconfig | 5 +-
arch/powerpc/configs/ppc64e_defconfig | 2 +-
arch/powerpc/platforms/85xx/Kconfig | 101 ++------------
arch/powerpc/platforms/85xx/Makefile | 8 +-
arch/powerpc/platforms/85xx/b4_qds.c | 97 --------------
arch/powerpc/platforms/85xx/corenet_ds.c | 96 --------------
arch/powerpc/platforms/85xx/corenet_ds.h | 19 ---
arch/powerpc/platforms/85xx/corenet_generic.c | 182 ++++++++++++++++++++++++++
arch/powerpc/platforms/85xx/p2041_rdb.c | 87 ------------
arch/powerpc/platforms/85xx/p3041_ds.c | 89 -------------
arch/powerpc/platforms/85xx/p4080_ds.c | 87 ------------
arch/powerpc/platforms/85xx/p5020_ds.c | 93 -------------
arch/powerpc/platforms/85xx/p5040_ds.c | 84 ------------
arch/powerpc/platforms/85xx/t4240_qds.c | 93 -------------
15 files changed, 196 insertions(+), 853 deletions(-)
delete mode 100644 arch/powerpc/platforms/85xx/b4_qds.c
delete mode 100644 arch/powerpc/platforms/85xx/corenet_ds.c
delete mode 100644 arch/powerpc/platforms/85xx/corenet_ds.h
create mode 100644 arch/powerpc/platforms/85xx/corenet_generic.c
delete mode 100644 arch/powerpc/platforms/85xx/p2041_rdb.c
delete mode 100644 arch/powerpc/platforms/85xx/p3041_ds.c
delete mode 100644 arch/powerpc/platforms/85xx/p4080_ds.c
delete mode 100644 arch/powerpc/platforms/85xx/p5020_ds.c
delete mode 100644 arch/powerpc/platforms/85xx/p5040_ds.c
delete mode 100644 arch/powerpc/platforms/85xx/t4240_qds.c
--
1.8.3.1
^ permalink raw reply
* Re: [PATCH v3] powerpc/p1010rdb-pb:make a new dts for p1010rdb-pb
From: Scott Wood @ 2013-09-26 1:42 UTC (permalink / raw)
To: Liu Shengzhou-B36685; +Cc: Zhao Qiang-B45475, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <3F453DDFF675A64A89321A1F35281021ADAA42@039-SN1MPN1-003.039d.mgd.msft.net>
On Wed, 2013-09-18 at 05:43 +0000, Liu Shengzhou-B36685 wrote:
>
> > -----Original Message-----
> > From: Zhao Qiang-B45475
> > Sent: Wednesday, September 18, 2013 12:55 PM
> > To: linuxppc-dev@lists.ozlabs.org
> > Cc: Zhao Qiang-B45475; Liu Shengzhou-B36685
> > Subject: [PATCH v3] powerpc/p1010rdb-pb:make a new dts for p1010rdb-pb
> >
> > P1010RDB-PA and P1010RDB-PB boards use different external PHY interrupt signals.
> > So make a new dts for P1010RDB-PB.
> >
> > Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com>
> > Signed-off-by: Zhao Qiang <B45475@freescale.com>
> > ---
> > Changes for v2:
> > -Remove phy interrupts for p1010rdb-pb
> > Changes for v3:
> > -Maintain the phy interrupts p1010rdb-pa and make a new device
> > tree for p1010rdb-pb
> >
> > arch/powerpc/boot/dts/p1010rdb-pa.dtsi | 11 ++++++
> > arch/powerpc/boot/dts/p1010rdb-pb.dts | 67 ++++++++++++++++++++++++++++++++++
> > arch/powerpc/boot/dts/p1010rdb-pb.dtsi | 11 ++++++
> > arch/powerpc/boot/dts/p1010rdb.dts | 1 +
> > arch/powerpc/boot/dts/p1010rdb.dtsi | 3 --
> > arch/powerpc/platforms/85xx/p1010rdb.c | 29 +++++++++++++++
> > 6 files changed, 119 insertions(+), 3 deletions(-) create mode 100644
> > arch/powerpc/boot/dts/p1010rdb-pa.dtsi
> > create mode 100644 arch/powerpc/boot/dts/p1010rdb-pb.dts
> > create mode 100644 arch/powerpc/boot/dts/p1010rdb-pb.dtsi
> >
>
> Don't touch p1010rdb.c, create two p1010rdb-pa.dts and p1010rdb-pb.dts, which include the common p1010rdb.dtsi.
> Combine original p1010rdb.dts and p1010rdb.dtsi into new p1010rdb.dtsi with removing compatible part.
> Place phy node and following part in p1010rdb-pa.dts and p1010rdb-pb.dts.
> / {
> model = "fsl,P1010RDB-PA";
> compatible = "fsl,P1010RDB";
> }
>
> / {
> model = "fsl,P1010RDB-PB";
> compatible = "fsl,P1010RDB";
> }
Model is mainly for human consumption and generally redundant on the
toplevel node... These boards are obviously not fully compatible and
thus there should be a new "fsl,p1010rdb-pb" with the existing
"fsl,P1010RDB" used to refer to PA since we can't change it now.
-Scott
^ permalink raw reply
* Re: Device tree node for Freescale Gianfar PTP reference clock source selection
From: Scott Wood @ 2013-09-26 1:30 UTC (permalink / raw)
To: Aida Mynzhasova
Cc: netdev, Richard Cochran, linuxppc-dev, Claudiu Manoil, devicetree
In-Reply-To: <523FEFCD.70409@skitlab.ru>
On Mon, 2013-09-23 at 11:37 +0400, Aida Mynzhasova wrote:
> Hi,
>
> Currently, Freescale Gianfar PTP reference clock source is determined
> through hard-coded value in gianfar_ptp driver. I don't think that
> recompilation of the entire module (or even worse - the kernel) is a god
> idea when we want to change one clock source to another. So, I want to
> add new device tree binding, which can be used as:
Is this describing the hardware or how you're using it? If the latter,
it should be a module parameter or some sort of runtime knob instead.
> ptp_clock@24E00 {
> compatible = "fsl,etsec-ptp";
> reg = <0x24E00 0xB0>;
> interrupts = <12 0x8 13 0x8>;
> interrupt-parent = < &ipic >;
> fsl,cksel = <0>; /* <-- New entry */
> fsl,tclk-period = <10>;
> fsl,tmr-prsc = <100>;
> fsl,tmr-add = <0x999999A4>;
> fsl,tmr-fiper1 = <0x3B9AC9F6>;
> fsl,tmr-fiper2 = <0x00018696>;
> fsl,max-adj = <659999998>;
> };
>
> fsl,cksel acceptable values:
>
> <0> for external clock;
> <1> for eTSEC system clock;
> <2> for eTSEC1 transmit clock;
> <3> for RTC clock input.
>
> I am new in this mailing list, and as far as I know, I have to discuss
> all updates for device tree files here before sending patch, which uses
> new attributes.
>
> Also, should I define new bindings in some special way? I want to add
> description of cksel attribute in
> /Documentation/devicetree/bindings/net/fsl-tsec-phy.txt. Is it enough or
> not?
Assuming this is actually describing how the hardware is wired up, yes,
that's how you'd document it (making sure that device trees without that
property are interpreted the same as today).
-Scott
^ permalink raw reply
* Re: [PATCH v3 3/3] powerpc/85xx: use one kernel option for all the CoreNet_Generic boards
From: Kevin Hao @ 2013-09-26 1:29 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc
In-Reply-To: <1380158760.24959.240.camel@snotra.buserror.net>
[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]
On Wed, Sep 25, 2013 at 08:26:00PM -0500, Scott Wood wrote:
> On Thu, 2013-09-26 at 09:23 +0800, Kevin Hao wrote:
> > On Wed, Sep 25, 2013 at 06:11:23PM -0500, Scott Wood wrote:
> > > On Wed, 2013-09-25 at 08:58 +0800, Kevin Hao wrote:
> > > > On Tue, Sep 24, 2013 at 05:41:32PM -0500, Scott Wood wrote:
> > > > > On Tue, 2013-09-24 at 11:03 +0800, Kevin Hao wrote:
> > > > > > + and B4 QDS boards
> > > > [...]
> > > > >
> > > > > Is there any difference between the 32-bit and 64-bit versions of this
> > > > > config symbol, other than the help text?
> > > >
> > > > No. As you know some of these boards only support 32bit kernel and some of
> > > > them only support 64bit kernel. It will definitely cause confusion when using
> > > > only one kernel option for all these boards. So I divide this into two options
> > > > (even the same name) for 32bit and 64bit respectively.
> > >
> > > You could just mention in the help text which boards are 32-bit, which
> > > are 64-bit, and which can work with either.
> >
> > OK. How about this?
> >
> > arch/powerpc/platforms/85xx/Kconfig | 26 +++++++-------------------
> > 1 file changed, 7 insertions(+), 19 deletions(-)
>
> Looks good.
OK. I will fold these changes into the original patch and make a new respin.
Thanks,
Kevin
>
> -Scott
>
>
>
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCH v3 3/3] powerpc/85xx: use one kernel option for all the CoreNet_Generic boards
From: Scott Wood @ 2013-09-26 1:26 UTC (permalink / raw)
To: Kevin Hao; +Cc: linuxppc
In-Reply-To: <20130926012310.GB9209@pek-khao-d1.corp.ad.wrs.com>
On Thu, 2013-09-26 at 09:23 +0800, Kevin Hao wrote:
> On Wed, Sep 25, 2013 at 06:11:23PM -0500, Scott Wood wrote:
> > On Wed, 2013-09-25 at 08:58 +0800, Kevin Hao wrote:
> > > On Tue, Sep 24, 2013 at 05:41:32PM -0500, Scott Wood wrote:
> > > > On Tue, 2013-09-24 at 11:03 +0800, Kevin Hao wrote:
> > > > > + and B4 QDS boards
> > > [...]
> > > >
> > > > Is there any difference between the 32-bit and 64-bit versions of this
> > > > config symbol, other than the help text?
> > >
> > > No. As you know some of these boards only support 32bit kernel and some of
> > > them only support 64bit kernel. It will definitely cause confusion when using
> > > only one kernel option for all these boards. So I divide this into two options
> > > (even the same name) for 32bit and 64bit respectively.
> >
> > You could just mention in the help text which boards are 32-bit, which
> > are 64-bit, and which can work with either.
>
> OK. How about this?
>
> arch/powerpc/platforms/85xx/Kconfig | 26 +++++++-------------------
> 1 file changed, 7 insertions(+), 19 deletions(-)
Looks good.
-Scott
^ permalink raw reply
* Re: [PATCH] powerpc/fsl/defconfig: enable CONFIG_AT803X_PHY
From: Scott Wood @ 2013-09-26 1:24 UTC (permalink / raw)
To: Shengzhou Liu; +Cc: linuxppc-dev
In-Reply-To: <1378196906-9850-1-git-send-email-Shengzhou.Liu@freescale.com>
On Tue, 2013-09-03 at 16:28 +0800, Shengzhou Liu wrote:
> Enable CONFIG_AT803X_PHY to support AR8030/8033/8035 PHY.
>
> Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com>
> ---
> arch/powerpc/configs/corenet32_smp_defconfig | 1 +
> arch/powerpc/configs/mpc85xx_defconfig | 1 +
> arch/powerpc/configs/mpc85xx_smp_defconfig | 1 +
> 3 files changed, 3 insertions(+), 0 deletions(-)
Why not corenet64_smp_defconfig? Which boards have this PHY?
-Scott
^ permalink raw reply
* Re: [PATCH v3 3/3] powerpc/85xx: use one kernel option for all the CoreNet_Generic boards
From: Kevin Hao @ 2013-09-26 1:23 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc
In-Reply-To: <1380150683.24959.215.camel@snotra.buserror.net>
[-- Attachment #1: Type: text/plain, Size: 2670 bytes --]
On Wed, Sep 25, 2013 at 06:11:23PM -0500, Scott Wood wrote:
> On Wed, 2013-09-25 at 08:58 +0800, Kevin Hao wrote:
> > On Tue, Sep 24, 2013 at 05:41:32PM -0500, Scott Wood wrote:
> > > On Tue, 2013-09-24 at 11:03 +0800, Kevin Hao wrote:
> > > > + and B4 QDS boards
> > [...]
> > >
> > > Is there any difference between the 32-bit and 64-bit versions of this
> > > config symbol, other than the help text?
> >
> > No. As you know some of these boards only support 32bit kernel and some of
> > them only support 64bit kernel. It will definitely cause confusion when using
> > only one kernel option for all these boards. So I divide this into two options
> > (even the same name) for 32bit and 64bit respectively.
>
> You could just mention in the help text which boards are 32-bit, which
> are 64-bit, and which can work with either.
OK. How about this?
arch/powerpc/platforms/85xx/Kconfig | 26 +++++++-------------------
1 file changed, 7 insertions(+), 19 deletions(-)
diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
index 04456fb..4d46349 100644
--- a/arch/powerpc/platforms/85xx/Kconfig
+++ b/arch/powerpc/platforms/85xx/Kconfig
@@ -218,20 +218,6 @@ config GE_IMP3A
This board is a 3U CompactPCI Single Board Computer with a Freescale
P2020 processor.
-config CORENET_GENERIC
- bool "Freescale CoreNet Generic"
- select DEFAULT_UIMAGE
- select PPC_E500MC
- select PHYS_64BIT
- select SWIOTLB
- select ARCH_REQUIRE_GPIOLIB
- select GPIO_MPC8XXX
- select HAS_RAPIDIO
- select PPC_EPAPR_HV_PIC
- help
- This option enables support for the P2041 RDB, P3041 DS, P4080 DS, P5020 DS
- and P5040 DS boards
-
config SGY_CTS1000
tristate "Servergy CTS-1000 support"
select GPIOLIB
@@ -257,8 +243,6 @@ config PPC_QEMU_E500
unset based on the emulated CPU (or actual host CPU in the case
of KVM).
-if PPC64
-
config CORENET_GENERIC
bool "Freescale CoreNet Generic"
select DEFAULT_UIMAGE
@@ -271,10 +255,14 @@ config CORENET_GENERIC
select HAS_RAPIDIO
select PPC_EPAPR_HV_PIC
help
- This option enables support for the P5020 DS, P5040 DS, T4240 QDS
- and B4 QDS boards
+ This option enables support for the FSL CoreNet based boards.
+ For 32bit kernel, the following boards are supported:
+ P2041 RDB, P3041 DS and P4080 DS
+ For 64bit kernel, the following boards are supported:
+ T4240 QDS and B4 QDS
+ The following boards are supported for both 32bit and 64bit kernel:
+ P5020 DS and P5040 DS
-endif
endif # FSL_SOC_BOOKE
config TQM85xx
Thanks,
Kevin
>
> -Scott
>
>
>
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply related
* Re: [PATCH v3 3/3] powerpc/85xx: use one kernel option for all the CoreNet_Generic boards
From: Scott Wood @ 2013-09-25 23:11 UTC (permalink / raw)
To: Kevin Hao; +Cc: linuxppc
In-Reply-To: <20130925005843.GA20572@pek-khao-d1.corp.ad.wrs.com>
On Wed, 2013-09-25 at 08:58 +0800, Kevin Hao wrote:
> On Tue, Sep 24, 2013 at 05:41:32PM -0500, Scott Wood wrote:
> > On Tue, 2013-09-24 at 11:03 +0800, Kevin Hao wrote:
> > > + and B4 QDS boards
> [...]
> >
> > Is there any difference between the 32-bit and 64-bit versions of this
> > config symbol, other than the help text?
>
> No. As you know some of these boards only support 32bit kernel and some of
> them only support 64bit kernel. It will definitely cause confusion when using
> only one kernel option for all these boards. So I divide this into two options
> (even the same name) for 32bit and 64bit respectively.
You could just mention in the help text which boards are 32-bit, which
are 64-bit, and which can work with either.
-Scott
^ permalink raw reply
* Re: [PATCH V4 3/3] powerpc/85xx: Add TWR-P1025 board support
From: Scott Wood @ 2013-09-25 23:09 UTC (permalink / raw)
To: Xie Xiaobo-R63061
Cc: Wood Scott-B07421, linuxppc-dev@lists.ozlabs.org,
Johnston Michael-R49610
In-Reply-To: <69EC9ED88E3CC04094A78F8074A7986D5FF91C@039-SN1MPN1-003.039d.mgd.msft.net>
On Wed, 2013-09-25 at 04:50 -0500, Xie Xiaobo-R63061 wrote:
> Hi Scott,
>
> See the reply inline.
>
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, September 25, 2013 7:22 AM
> > To: Xie Xiaobo-R63061
> > Cc: linuxppc-dev@lists.ozlabs.org; Johnston Michael-R49610
> > Subject: Re: [PATCH V4 3/3] powerpc/85xx: Add TWR-P1025 board support
> >
> > On Tue, 2013-09-24 at 18:48 +0800, Xie Xiaobo wrote:
> > > + partition@80000 {
> > > + /* 3.5 MB for Linux Kernel Image */
> > > + reg = <0x00080000 0x00380000>;
> > > + label = "NOR Linux Kernel Image";
> > > + };
> >
> > Is this enough?
>
> I will enlarge it to 6MB.
>
> >
> > > + partition@400000 {
> > > + /* 58.75MB for JFFS2 based Root file System */
> > > + reg = <0x00400000 0x03ac0000>;
> > > + label = "NOR Root File System";
> > > + };
> >
> > Don't specify jffs2.
>
> OK, I will remove "jffs2"
>
> >
> > > + /* CS2 for Display */
> > > + ssd1289@2,0 {
> > > + #address-cells = <1>;
> > > + #size-cells = <1>;
> > > + compatible = "ssd1289";
> > > + reg = <0x2 0x0000 0x0002
> > > + 0x2 0x0002 0x0002>;
> > > + };
> >
> > Node names should be generic. What does ssd1289 do? If this is actually
> > the display device, then it should be called "display@2,0".
>
> OK. The ssd1289 is a LCD controller.
>
> >
> > How about a vendor prefix on that compatible? Why #address-cells/#size-
> > cells despite no child nodes? Where is a binding that says what each of
> > those two reg resources mean?
>
> I will add the vendor prefix. I review the ssd1289 driver, and the #address-cells/#size-cells were un-used. I will remove them.
And a binding?
Why do you need two separate reg resources rather than just <2 0 4>?
Will they ever be discontiguous?
-Scott
^ permalink raw reply
* Re: [PATCH] powerpc/kvmbook3s_hv: propagate H_SET_MODE to the host
From: Paul Mackerras @ 2013-09-25 22:31 UTC (permalink / raw)
To: Laurent Dufour; +Cc: linuxppc-dev, Anton Blanchard
In-Reply-To: <20130925121027.29504.19269.stgit@nimbus>
On Wed, Sep 25, 2013 at 02:10:27PM +0200, Laurent Dufour wrote:
> Follow-up to Anton's H_SET_MODE patch, the host should be taken aware of
> guest endianess change.
>
> The hcall H_SET_MODE is processed in kvm then in the host.
>
> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> ---
> arch/powerpc/kvm/book3s_hv.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 998cad3..4a47c74 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -599,6 +599,14 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
> kvmppc_get_gpr(vcpu, 5),
> kvmppc_get_gpr(vcpu, 6),
> kvmppc_get_gpr(vcpu, 7));
> + /*
> + * If the hcall succeeded, we propagate it to the host.
> + * This way, it will be aware of the endianess's change too.
> + * The assumption is made that the hcall will succeed in the
> + * host.
> + */
> + if (ret == H_SUCCESS)
> + return RESUME_HOST;
> break;
The problem with this is that H_SET_MODE isn't just used for setting
endianness; it also does breakpoint setting (DAWR/X and CIABR), which
might happen very frequently, so we don't want them being punted up to
userspace.
Paul.
^ permalink raw reply
* Re: linux-next: build failure after merge of the akpm tree
From: Andrew Morton @ 2013-09-25 21:43 UTC (permalink / raw)
To: Hugh Dickins
Cc: Stephen Rothwell, Frederic Weisbecker, linux-kernel,
Sergei Trofimovich, linux-next, Greg KH, ppc-dev, Timur Tabi
In-Reply-To: <alpine.LNX.2.00.1309251426150.2479@eggly.anvils>
On Wed, 25 Sep 2013 14:32:14 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
> On Wed, 25 Sep 2013, Andrew Morton wrote:
> > On Wed, 25 Sep 2013 11:06:43 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > > Hi Andrew,
> > >
> > > After merging the akpm tree, linux-next builds (powerpc allmodconfig)
> > > fail like this:
> >
> > I can't get powerpc to build at all at present:
> >
> > CHK include/config/kernel.release
> > CHK include/generated/uapi/linux/version.h
> > CHK include/generated/utsrelease.h
> > CC arch/powerpc/kernel/asm-offsets.s
> > In file included from include/linux/vtime.h:6,
> > from include/linux/hardirq.h:7,
> > from include/linux/memcontrol.h:24,
> > from include/linux/swap.h:8,
> > from include/linux/suspend.h:4,
> > from arch/powerpc/kernel/asm-offsets.c:24:
> > arch/powerpc/include/generated/asm/vtime.h:1:31: error: asm-generic/vtime.h: No such file or directory
>
> That caught me too: include/asm-generic/vtime.h is a patch-unfriendly
> 0-length file in the git tree;
hm, this?
From: Andrew Morton <akpm@linux-foundation.org>
Subject: include/asm-generic/vtime.h: avoid zero-length file
patch(1) can't handle zero-length files - it appears to simply not create
the file, so my powerpc build fails.
Put something in here to make life easier.
Cc: Hugh Dickins <hughd@google.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
include/asm-generic/vtime.h | 1 +
1 file changed, 1 insertion(+)
diff -puN /dev/null include/asm-generic/vtime.h
--- /dev/null
+++ a/include/asm-generic/vtime.h
@@ -0,0 +1 @@
+/* no content, but patch(1) dislikes empty files */
_
> I wonder what use it's supposed to have.
Frederic, can you please confirm that include/asm-generic/vtime.h is
supposed to be empty?
> (And I'm not very keen on the growing trend for symlinks in the git tree.)
ooh, that explains why I lost my arch/microblaze/boot/dts/system.dts.
^ permalink raw reply
* Re: linux-next: build failure after merge of the akpm tree
From: Hugh Dickins @ 2013-09-25 21:32 UTC (permalink / raw)
To: Andrew Morton
Cc: Stephen Rothwell, Frederic Weisbecker, linux-kernel,
Sergei Trofimovich, linux-next, Greg KH, ppc-dev, Timur Tabi
In-Reply-To: <20130925132612.d1685bc8fb72558eef6fb09d@linux-foundation.org>
On Wed, 25 Sep 2013, Andrew Morton wrote:
> On Wed, 25 Sep 2013 11:06:43 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > Hi Andrew,
> >
> > After merging the akpm tree, linux-next builds (powerpc allmodconfig)
> > fail like this:
>
> I can't get powerpc to build at all at present:
>
> CHK include/config/kernel.release
> CHK include/generated/uapi/linux/version.h
> CHK include/generated/utsrelease.h
> CC arch/powerpc/kernel/asm-offsets.s
> In file included from include/linux/vtime.h:6,
> from include/linux/hardirq.h:7,
> from include/linux/memcontrol.h:24,
> from include/linux/swap.h:8,
> from include/linux/suspend.h:4,
> from arch/powerpc/kernel/asm-offsets.c:24:
> arch/powerpc/include/generated/asm/vtime.h:1:31: error: asm-generic/vtime.h: No such file or directory
That caught me too: include/asm-generic/vtime.h is a patch-unfriendly
0-length file in the git tree; I wonder what use it's supposed to have.
(And I'm not very keen on the growing trend for symlinks in the git tree.)
Hugh
^ permalink raw reply
* [PATCH RESEND 4/4] hotplug, powerpc, x86: Remove cpu_hotplug_driver_lock()
From: Toshi Kani @ 2013-09-25 21:08 UTC (permalink / raw)
To: rjw
Cc: fenghua.yu, bp, Toshi Kani, gregkh, x86, linux-kernel, linux-acpi,
isimatu.yasuaki, mingo, srivatsa.bhat, nfont, tglx, hpa,
linuxppc-dev
cpu_hotplug_driver_lock() serializes CPU online/offline operations
when ARCH_CPU_PROBE_RELEASE is set. This lock interface is no longer
necessary with the following reason:
- lock_device_hotplug() now protects CPU online/offline operations,
including the probe & release interfaces enabled by
ARCH_CPU_PROBE_RELEASE. The use of cpu_hotplug_driver_lock() is
redundant.
- cpu_hotplug_driver_lock() is only valid when ARCH_CPU_PROBE_RELEASE
is defined, which is misleading and is only enabled on powerpc.
This patch removes the cpu_hotplug_driver_lock() interface. As
a result, ARCH_CPU_PROBE_RELEASE only enables / disables the cpu
probe & release interface as intended. There is no functional change
in this patch.
Signed-off-by: Toshi Kani <toshi.kani@hp.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
---
Performed build test only on powerpc.
Resend: Rebased to linux-next of the PM tree (3.12-rc2).
---
arch/powerpc/kernel/smp.c | 12 ---------
arch/powerpc/platforms/pseries/dlpar.c | 43 ++++++++++++--------------------
arch/x86/kernel/topology.c | 2 -
drivers/base/cpu.c | 15 ++---------
include/linux/cpu.h | 13 ----------
5 files changed, 19 insertions(+), 66 deletions(-)
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 8e59abc..930cd8a 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -844,18 +844,6 @@ void __cpu_die(unsigned int cpu)
smp_ops->cpu_die(cpu);
}
-static DEFINE_MUTEX(powerpc_cpu_hotplug_driver_mutex);
-
-void cpu_hotplug_driver_lock()
-{
- mutex_lock(&powerpc_cpu_hotplug_driver_mutex);
-}
-
-void cpu_hotplug_driver_unlock()
-{
- mutex_unlock(&powerpc_cpu_hotplug_driver_mutex);
-}
-
void cpu_die(void)
{
if (ppc_md.cpu_die)
diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 7cfdaae..a8fe5aa 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -404,46 +404,38 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
unsigned long drc_index;
int rc;
- cpu_hotplug_driver_lock();
rc = strict_strtoul(buf, 0, &drc_index);
- if (rc) {
- rc = -EINVAL;
- goto out;
- }
+ if (rc)
+ return -EINVAL;
parent = of_find_node_by_path("/cpus");
- if (!parent) {
- rc = -ENODEV;
- goto out;
- }
+ if (!parent)
+ return -ENODEV;
dn = dlpar_configure_connector(drc_index, parent);
- if (!dn) {
- rc = -EINVAL;
- goto out;
- }
+ if (!dn)
+ return -EINVAL;
of_node_put(parent);
rc = dlpar_acquire_drc(drc_index);
if (rc) {
dlpar_free_cc_nodes(dn);
- rc = -EINVAL;
- goto out;
+ return -EINVAL;
}
rc = dlpar_attach_node(dn);
if (rc) {
dlpar_release_drc(drc_index);
dlpar_free_cc_nodes(dn);
- goto out;
+ return rc;
}
rc = dlpar_online_cpu(dn);
-out:
- cpu_hotplug_driver_unlock();
+ if (rc)
+ return rc;
- return rc ? rc : count;
+ return count;
}
static int dlpar_offline_cpu(struct device_node *dn)
@@ -516,30 +508,27 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t count)
return -EINVAL;
}
- cpu_hotplug_driver_lock();
rc = dlpar_offline_cpu(dn);
if (rc) {
of_node_put(dn);
- rc = -EINVAL;
- goto out;
+ return -EINVAL;
}
rc = dlpar_release_drc(*drc_index);
if (rc) {
of_node_put(dn);
- goto out;
+ return rc;
}
rc = dlpar_detach_node(dn);
if (rc) {
dlpar_acquire_drc(*drc_index);
- goto out;
+ return rc;
}
of_node_put(dn);
-out:
- cpu_hotplug_driver_unlock();
- return rc ? rc : count;
+
+ return count;
}
static int __init pseries_dlpar_init(void)
diff --git a/arch/x86/kernel/topology.c b/arch/x86/kernel/topology.c
index a3f35eb..649b010 100644
--- a/arch/x86/kernel/topology.c
+++ b/arch/x86/kernel/topology.c
@@ -66,7 +66,6 @@ int __ref _debug_hotplug_cpu(int cpu, int action)
return -EINVAL;
lock_device_hotplug();
- cpu_hotplug_driver_lock();
switch (action) {
case 0:
@@ -91,7 +90,6 @@ int __ref _debug_hotplug_cpu(int cpu, int action)
ret = -EINVAL;
}
- cpu_hotplug_driver_unlock();
unlock_device_hotplug();
return ret;
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 51f5d7f..f48370d 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -44,13 +44,11 @@ static int __ref cpu_subsys_online(struct device *dev)
struct cpu *cpu = container_of(dev, struct cpu, dev);
int cpuid = dev->id;
int from_nid, to_nid;
- int ret = -ENODEV;
-
- cpu_hotplug_driver_lock();
+ int ret;
from_nid = cpu_to_node(cpuid);
if (from_nid == NUMA_NO_NODE)
- goto out;
+ return -ENODEV;
ret = cpu_up(cpuid);
/*
@@ -61,19 +59,12 @@ static int __ref cpu_subsys_online(struct device *dev)
if (from_nid != to_nid)
change_cpu_under_node(cpu, from_nid, to_nid);
- out:
- cpu_hotplug_driver_unlock();
return ret;
}
static int cpu_subsys_offline(struct device *dev)
{
- int ret;
-
- cpu_hotplug_driver_lock();
- ret = cpu_down(dev->id);
- cpu_hotplug_driver_unlock();
- return ret;
+ return cpu_down(dev->id);
}
void unregister_cpu(struct cpu *cpu)
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 801ff9e..3434ef7 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -185,19 +185,6 @@ extern void cpu_hotplug_enable(void);
void clear_tasks_mm_cpumask(int cpu);
int cpu_down(unsigned int cpu);
-#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
-extern void cpu_hotplug_driver_lock(void);
-extern void cpu_hotplug_driver_unlock(void);
-#else
-static inline void cpu_hotplug_driver_lock(void)
-{
-}
-
-static inline void cpu_hotplug_driver_unlock(void)
-{
-}
-#endif
-
#else /* CONFIG_HOTPLUG_CPU */
static inline void cpu_hotplug_begin(void) {}
^ permalink raw reply related
* Re: [PATCH v2 2/6] PCI/MSI: Factor out pci_get_msi_cap() interface
From: Tejun Heo @ 2013-09-25 21:00 UTC (permalink / raw)
To: Alexander Gordeev
Cc: linux-pci@vger.kernel.org, Joerg Roedel, x86@kernel.org,
linux-kernel@vger.kernel.org, linux-ide@vger.kernel.org,
Jan Beulich, Bjorn Helgaas, linuxppc-dev, Ingo Molnar
In-Reply-To: <20130925205804.GA21737@dhcp-26-207.brq.redhat.com>
Hello,
On Wed, Sep 25, 2013 at 10:58:05PM +0200, Alexander Gordeev wrote:
> Unfortunately, pSeries is a shows-topper here :( It seems we have to
> introduce pci_get_msi{,x}_limit() interfaces to honour the quota
> thing. I just hope the hardware set for pSeries is limited and we
> won't need to use it for all drivers.
Can you please go into a bit of detail on that? Why does it matter?
Is it because you're worried you might cause performance regression by
forcing prevoius partial multiple allocations to single interrupt
operation?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 2/6] PCI/MSI: Factor out pci_get_msi_cap() interface
From: Alexander Gordeev @ 2013-09-25 20:58 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci@vger.kernel.org, Joerg Roedel, x86@kernel.org,
linux-kernel@vger.kernel.org, linux-ide@vger.kernel.org,
Jan Beulich, Tejun Heo, linuxppc-dev, Ingo Molnar
In-Reply-To: <20130925180220.GB26273@google.com>
On Wed, Sep 25, 2013 at 12:02:20PM -0600, Bjorn Helgaas wrote:
> On Fri, Sep 20, 2013 at 07:27:36AM -0500, Tejun Heo wrote:
> > On Fri, Sep 20, 2013 at 10:24:59AM +0200, Alexander Gordeev wrote:
> > > * Make pci_enable_msix() return 0/-errno
> >
> > My choice would be this one.
>
> I agree; it sounds like you've identified several bugs related to the
> current confusing interface, so fixing that seems like the first step.
Yeah, I am trying to. Turns out to be a nice exercise ;)
> I hope we can avoid adding a plethora of interfaces to address unusual
> corner cases. But if we do the above and it turns out not to be enough,
> we can always extend it later.
Unfortunately, pSeries is a shows-topper here :( It seems we have to
introduce pci_get_msi{,x}_limit() interfaces to honour the quota
thing. I just hope the hardware set for pSeries is limited and we
won't need to use it for all drivers.
> Bjorn
--
Regards,
Alexander Gordeev
agordeev@redhat.com
^ permalink raw reply
* Re: [PATCH v2 4/4] hotplug, powerpc, x86: Remove cpu_hotplug_driver_lock()
From: Toshi Kani @ 2013-09-25 20:44 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: fenghua.yu@intel.com, bp@suse.de, gregkh@linuxfoundation.org,
x86@kernel.org, linux-kernel@vger.kernel.org,
linux-acpi@vger.kernel.org, isimatu.yasuaki@jp.fujitsu.com,
mingo@redhat.com, srivatsa.bhat@linux.vnet.ibm.com,
nfont@linux.vnet.ibm.com, tglx@linutronix.de, hpa@linux.intel.com,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <7232059.d6LGFPRAoP@vostro.rjw.lan>
On Wed, 2013-09-25 at 08:27 +0000, Rafael J. Wysocki wrote:
> Hi,
>
> On Thursday, August 29, 2013 06:22:09 PM Toshi Kani wrote:
> > cpu_hotplug_driver_lock() serializes CPU online/offline operations
> > when ARCH_CPU_PROBE_RELEASE is set. This lock interface is no longer
> > necessary with the following reason:
> >
> > - lock_device_hotplug() now protects CPU online/offline operations,
> > including the probe & release interfaces enabled by
> > ARCH_CPU_PROBE_RELEASE. The use of cpu_hotplug_driver_lock() is
> > redundant.
> > - cpu_hotplug_driver_lock() is only valid when ARCH_CPU_PROBE_RELEASE
> > is defined, which is misleading and is only enabled on powerpc.
> >
> > This patch removes the cpu_hotplug_driver_lock() interface. As
> > a result, ARCH_CPU_PROBE_RELEASE only enables / disables the cpu
> > probe & release interface as intended. There is no functional change
> > in this patch.
> >
> > Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Reviewed-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
>
> Can you please rebase this patch on top of 3.12-rc2? It doesn't apply for
> me any more.
Yes, I will send this patch on top of 3.12-rc2.
Thanks,
-Toshi
^ permalink raw reply
* Re: linux-next: build failure after merge of the akpm tree
From: Andrew Morton @ 2013-09-25 20:26 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Greg KH, linux-kernel, Sergei Trofimovich, linux-next, ppc-dev,
Timur Tabi
In-Reply-To: <20130925110643.db5fa154bea3838ed6affa45@canb.auug.org.au>
On Wed, 25 Sep 2013 11:06:43 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Hi Andrew,
>
> After merging the akpm tree, linux-next builds (powerpc allmodconfig)
> fail like this:
I can't get powerpc to build at all at present:
CHK include/config/kernel.release
CHK include/generated/uapi/linux/version.h
CHK include/generated/utsrelease.h
CC arch/powerpc/kernel/asm-offsets.s
In file included from include/linux/vtime.h:6,
from include/linux/hardirq.h:7,
from include/linux/memcontrol.h:24,
from include/linux/swap.h:8,
from include/linux/suspend.h:4,
from arch/powerpc/kernel/asm-offsets.c:24:
arch/powerpc/include/generated/asm/vtime.h:1:31: error: asm-generic/vtime.h: No such file or directory
> drivers/tty/ehv_bytechan.c:362:1: error: type defaults to 'int' in declaration of 'console_initcall' [-Werror=implicit-int]
>
> Caused by commit 0f01cf96c2d4 ("./Makefile: enable -Werror=implicit-int
> and -Werror=strict-prototypes by default") which has bee in linux-next
> since Aug 16. This commit exposed that fact that
> drivers/tty/ehv_bytechan.c can be built as a module, but has a
> console_initcall (which is not available to modules). This was
> originally introduced in commit dcd83aaff1c8 ("tty/powerpc: introduce the
> ePAPR embedded hypervisor byte channel driver") in v3.2.
>
> Anyone got a good solution?
console_initcall() is a macro defined in init.h. But we forgot to
provide a version for #ifdef MODULE.
At include/linux/init.h line 284 we see:
/* Don't use these in loadable modules, but some people do... */
#define early_initcall(fn) module_init(fn)
#define core_initcall(fn) module_init(fn)
...
So we *could* add console_initcall() there. But the problem is that it
won't work as desired - when the driver is loaded as a module,
ehv_bc_console_init() will be called at modprobe time, which is far far
later than console_initcall-time.
So the ehv_bytechan.c developers need to work out what they want to do
here. Do we disallow building that driver as a module? Or do we
permit that, and run ehv_bc_console_init() at modprobe time (needs
testing!).
If the latter then I'd be reluctant to add a modular version of
console_initcall() because the thing's very presence is misleading.
otoh, drivers which use such a console_initcall() _might_ work, and
everyone tests their drivers both built-in and as modules, don't they?
Don't they?
^ permalink raw reply
* Re: [PATCH v2] powerpc/83xx: gianfar_ptp: select 1588 clock source through dts file
From: Kumar Gala @ 2013-09-25 19:38 UTC (permalink / raw)
To: Aida Mynzhasova; +Cc: devicetree, richardcochran, linuxppc-dev, netdev
In-Reply-To: <1380093863-5388-1-git-send-email-aida.mynzhasova@skitlab.ru>
On Sep 25, 2013, at 2:24 AM, Aida Mynzhasova wrote:
> Currently IEEE 1588 timer reference clock source is determined through
> hard-coded value in gianfar_ptp driver. This patch allows to select =
ptp
> clock source by means of device tree file node.
>=20
> For instance:
>=20
> fsl,cksel =3D <0>;
>=20
> for using external (TSEC_TMR_CLK input) high precision timer
> reference clock.
>=20
> Other acceptable values:
>=20
> <1> : eTSEC system clock
> <2> : eTSEC1 transmit clock
> <3> : RTC clock input
Do these value match some register field to select which clk? If so =
please add that to the document.
- k
>=20
> When this attribute isn't used, eTSEC system clock will serve as
> IEEE 1588 timer reference clock.
>=20
> Signed-off-by: Aida Mynzhasova <aida.mynzhasova@skitlab.ru>
> ---
> Documentation/devicetree/bindings/net/fsl-tsec-phy.txt | 16 =
+++++++++++++++-
> drivers/net/ethernet/freescale/gianfar_ptp.c | 4 +++-
> 2 files changed, 18 insertions(+), 2 deletions(-)
>=20
> diff --git a/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt =
b/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
> index 2c6be03..eb06059 100644
> --- a/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
> +++ b/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
> @@ -86,6 +86,7 @@ General Properties:
>=20
> Clock Properties:
>=20
> + - fsl,cksel Timer reference clock source.
> - fsl,tclk-period Timer reference clock period in nanoseconds.
> - fsl,tmr-prsc Prescaler, divides the output clock.
> - fsl,tmr-add Frequency compensation value.
> @@ -97,7 +98,7 @@ Clock Properties:
> clock. You must choose these carefully for the clock to work right.
> Here is how to figure good values:
>=20
> - TimerOsc =3D system clock MHz
> + TimerOsc =3D selected reference clock MHz
> tclk_period =3D desired clock period nanoseconds
> NominalFreq =3D 1000 / tclk_period MHz
> FreqDivRatio =3D TimerOsc / NominalFreq (must be greater that =
1.0)
> @@ -114,6 +115,18 @@ Clock Properties:
> Pulse Per Second (PPS) signal, since this will be offered to the PPS
> subsystem to synchronize the Linux clock.
>=20
> + "fsl,cksel" property allows to select different reference clock
> + sources:
> +
> + <0> - external high precision timer reference clock (TSEC_TMR_CLK
> + input is used for this purpose);
> + <1> - eTSEC system clock;
> + <2> - eTSEC1 transmit clock;
> + <3> - RTC clock input.
> +
> + When this attribute is not used, eTSEC system clock will serve as
> + IEEE 1588 timer reference clock.
> +
> Example:
>=20
> ptp_clock@24E00 {
> @@ -121,6 +134,7 @@ Example:
> reg =3D <0x24E00 0xB0>;
> interrupts =3D <12 0x8 13 0x8>;
> interrupt-parent =3D < &ipic >;
> + fsl,cksel =3D <1>;
> fsl,tclk-period =3D <10>;
> fsl,tmr-prsc =3D <100>;
> fsl,tmr-add =3D <0x999999A4>;
> diff --git a/drivers/net/ethernet/freescale/gianfar_ptp.c =
b/drivers/net/ethernet/freescale/gianfar_ptp.c
> index 098f133..e006a09 100644
> --- a/drivers/net/ethernet/freescale/gianfar_ptp.c
> +++ b/drivers/net/ethernet/freescale/gianfar_ptp.c
> @@ -452,7 +452,9 @@ static int gianfar_ptp_probe(struct =
platform_device *dev)
> err =3D -ENODEV;
>=20
> etsects->caps =3D ptp_gianfar_caps;
> - etsects->cksel =3D DEFAULT_CKSEL;
> +
> + if (get_of_u32(node, "fsl,cksel", &etsects->cksel))
> + etsects->cksel =3D DEFAULT_CKSEL;
>=20
> if (get_of_u32(node, "fsl,tclk-period", &etsects->tclk_period) =
||
> get_of_u32(node, "fsl,tmr-prsc", &etsects->tmr_prsc) ||
> --=20
> 1.8.1.2
>=20
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" =
in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--=20
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, =
hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH 7/7] vfio pci: Add vfio iommu implementation for FSL_PAMU
From: Alex Williamson @ 2013-09-25 19:06 UTC (permalink / raw)
To: Bharat Bhushan
Cc: agraf, linux-pci, joro, linux-kernel, Bharat Bhushan, iommu,
scottwood, linuxppc-dev
In-Reply-To: <1379575763-2091-8-git-send-email-Bharat.Bhushan@freescale.com>
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 <bharat.bhushan@freescale.com>
> ---
> 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 0000000..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 <bharat.bhushan@freescale.com>
> + *
> + * This file is derived from driver/vfio/vfio_iommu_type1.c
> + *
> + * 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
> + */
> +
> +#include <linux/compat.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/iommu.h>
> +#include <linux/module.h>
> +#include <linux/mm.h>
> +#include <linux/pci.h> /* pci_bus_type */
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/vfio.h>
> +#include <linux/workqueue.h>
> +#include <linux/hugetlb.h>
> +#include <linux/msi.h>
> +#include <asm/fsl_pamu_stash.h>
> +
> +#include "vfio_iommu_common.h"
> +
> +#define DRIVER_VERSION "0.1"
> +#define DRIVER_AUTHOR "Bharat Bhushan <bharat.bhushan@freescale.com>"
> +#define DRIVER_DESC "FSL PAMU IOMMU driver for VFIO"
> +
> +struct vfio_iommu {
> + struct iommu_domain *domain;
> + struct mutex lock;
> + dma_addr_t aperture_start;
> + dma_addr_t aperture_end;
> + dma_addr_t page_size; /* Maximum mapped Page size */
> + int nsubwindows; /* Number of subwindows */
> + struct rb_root dma_list;
> + struct list_head msi_dma_list;
> + struct list_head group_list;
> +};
> +
> +struct vfio_dma {
> + struct rb_node node;
> + dma_addr_t iova; /* Device address */
> + unsigned long vaddr; /* Process virtual addr */
> + size_t size; /* Number of pages */
Is this really pages?
> + int prot; /* IOMMU_READ/WRITE */
> +};
> +
> +struct vfio_msi_dma {
> + struct list_head next;
> + dma_addr_t iova; /* Device address */
> + int bank_id;
> + int prot; /* IOMMU_READ/WRITE */
> +};
> +
> +struct vfio_group {
> + struct iommu_group *iommu_group;
> + struct list_head next;
> +};
> +
> +static struct vfio_dma *vfio_find_dma(struct vfio_iommu *iommu,
> + dma_addr_t start, size_t size)
> +{
> + struct rb_node *node = iommu->dma_list.rb_node;
> +
> + while (node) {
> + struct vfio_dma *dma = rb_entry(node, struct vfio_dma, node);
> +
> + if (start + size <= dma->iova)
> + node = node->rb_left;
> + else if (start >= dma->iova + dma->size)
because this looks more like it's bytes...
> + node = node->rb_right;
> + else
> + return dma;
> + }
> +
> + return NULL;
> +}
> +
> +static void vfio_insert_dma(struct vfio_iommu *iommu, struct vfio_dma *new)
> +{
> + struct rb_node **link = &iommu->dma_list.rb_node, *parent = NULL;
> + struct vfio_dma *dma;
> +
> + while (*link) {
> + parent = *link;
> + dma = rb_entry(parent, struct vfio_dma, node);
> +
> + if (new->iova + new->size <= dma->iova)
so does this...
> + link = &(*link)->rb_left;
> + else
> + link = &(*link)->rb_right;
> + }
> +
> + rb_link_node(&new->node, parent, link);
> + rb_insert_color(&new->node, &iommu->dma_list);
> +}
> +
> +static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *old)
> +{
> + rb_erase(&old->node, &iommu->dma_list);
> +}
So if your vfio_dma.size is actually in bytes, why isn't all this code
in common?
> +
> +static int iova_to_win(struct vfio_iommu *iommu, dma_addr_t iova)
> +{
> + u64 offset = iova - iommu->aperture_start;
> + do_div(offset, iommu->page_size);
> + return (int) offset;
> +}
> +
> +static int vfio_disable_iommu_domain(struct vfio_iommu *iommu)
> +{
> + int enable = 0;
> + return iommu_domain_set_attr(iommu->domain,
> + DOMAIN_ATTR_FSL_PAMU_ENABLE, &enable);
> +}
This is never called?!
> +
> +static int vfio_enable_iommu_domain(struct vfio_iommu *iommu)
> +{
> + int enable = 1;
> + return iommu_domain_set_attr(iommu->domain,
> + DOMAIN_ATTR_FSL_PAMU_ENABLE, &enable);
> +}
> +
> +/* Unmap DMA region */
> +static int vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
> + dma_addr_t iova, size_t *size)
> +{
> + dma_addr_t start = iova;
> + int win, win_start, win_end;
> + long unlocked = 0;
> + unsigned int nr_pages;
> +
> + nr_pages = iommu->page_size / PAGE_SIZE;
> + win_start = iova_to_win(iommu, iova);
> + win_end = iova_to_win(iommu, iova + *size - 1);
> +
> + /* Release the pinned pages */
> + for (win = win_start; win <= win_end; iova += iommu->page_size, win++) {
> + unsigned long pfn;
> +
> + pfn = iommu_iova_to_phys(iommu->domain, iova) >> PAGE_SHIFT;
> + if (!pfn)
> + continue;
> +
> + iommu_domain_window_disable(iommu->domain, win);
> +
> + unlocked += vfio_unpin_pages(pfn, nr_pages, dma->prot, 1);
> + }
> +
> + vfio_lock_acct(-unlocked);
> + *size = iova - start;
> + return 0;
> +}
> +
> +static int vfio_remove_dma_overlap(struct vfio_iommu *iommu, dma_addr_t start,
> + size_t *size, struct vfio_dma *dma)
> +{
> + size_t offset, overlap, tmp;
> + struct vfio_dma *split;
> + int ret;
> +
> + if (!*size)
> + return 0;
> +
> + /*
> + * Existing dma region is completely covered, unmap all. This is
> + * the likely case since userspace tends to map and unmap buffers
> + * in one shot rather than multiple mappings within a buffer.
> + */
> + if (likely(start <= dma->iova &&
> + start + *size >= dma->iova + dma->size)) {
> + *size = dma->size;
> + ret = vfio_unmap_unpin(iommu, dma, dma->iova, size);
> + if (ret)
> + return ret;
> +
> + /*
> + * Did we remove more than we have? Should never happen
> + * since a vfio_dma is contiguous in iova and vaddr.
> + */
> + WARN_ON(*size != dma->size);
> +
> + vfio_remove_dma(iommu, dma);
> + kfree(dma);
> + return 0;
> + }
> +
> + /* Overlap low address of existing range */
> + if (start <= dma->iova) {
> + overlap = start + *size - dma->iova;
> + ret = vfio_unmap_unpin(iommu, dma, dma->iova, &overlap);
> + if (ret)
> + return ret;
> +
> + vfio_remove_dma(iommu, dma);
> +
> + /*
> + * Check, we may have removed to whole vfio_dma. If not
> + * fixup and re-insert.
> + */
> + if (overlap < dma->size) {
> + dma->iova += overlap;
> + dma->vaddr += overlap;
> + dma->size -= overlap;
> + vfio_insert_dma(iommu, dma);
> + } else
> + kfree(dma);
> +
> + *size = overlap;
> + return 0;
> + }
> +
> + /* Overlap high address of existing range */
> + if (start + *size >= dma->iova + dma->size) {
> + offset = start - dma->iova;
> + overlap = dma->size - offset;
> +
> + ret = vfio_unmap_unpin(iommu, dma, start, &overlap);
> + if (ret)
> + return ret;
> +
> + dma->size -= overlap;
> + *size = overlap;
> + return 0;
> + }
> +
> + /* Split existing */
> +
> + /*
> + * Allocate our tracking structure early even though it may not
> + * be used. An Allocation failure later loses track of pages and
> + * is more difficult to unwind.
> + */
> + split = kzalloc(sizeof(*split), GFP_KERNEL);
> + if (!split)
> + return -ENOMEM;
> +
> + offset = start - dma->iova;
> +
> + ret = vfio_unmap_unpin(iommu, dma, start, size);
> + if (ret || !*size) {
> + kfree(split);
> + return ret;
> + }
> +
> + tmp = dma->size;
> +
> + /* Resize the lower vfio_dma in place, before the below insert */
> + dma->size = offset;
> +
> + /* Insert new for remainder, assuming it didn't all get unmapped */
> + if (likely(offset + *size < tmp)) {
> + split->size = tmp - offset - *size;
> + split->iova = dma->iova + offset + *size;
> + split->vaddr = dma->vaddr + offset + *size;
> + split->prot = dma->prot;
> + vfio_insert_dma(iommu, split);
> + } else
> + kfree(split);
> +
> + return 0;
> +}
Hmm, this looks identical to type1, can we share more?
> +
> +/* Map DMA region */
> +static int vfio_dma_map(struct vfio_iommu *iommu, dma_addr_t iova,
> + unsigned long vaddr, long npage, int prot)
> +{
> + int ret = 0, i;
> + size_t size;
> + unsigned int win, nr_subwindows;
> + dma_addr_t iovamap;
> +
> + /* total size to be mapped */
> + size = npage << PAGE_SHIFT;
> + do_div(size, iommu->page_size);
> + nr_subwindows = size;
> + size = npage << PAGE_SHIFT;
Is all this do_div() stuff necessary? If page_size is a power of two,
just shift it.
> + iovamap = iova;
> + for (i = 0; i < nr_subwindows; i++) {
> + unsigned long pfn;
> + unsigned long nr_pages;
> + dma_addr_t mapsize;
> + struct vfio_dma *dma = NULL;
> +
> + win = iova_to_win(iommu, iovamap);
Aren't these consecutive, why can't we just increment?
> + if (iovamap != iommu->aperture_start + iommu->page_size * win) {
> + pr_err("%s iova(%llx) unalligned to window size %llx\n",
> + __func__, iovamap, iommu->page_size);
> + ret = -EINVAL;
> + break;
> + }
Can't this only happen on the first one? Seems like it should be
outside of the loop. What about alignment with the end of the window,
do you care? Check spelling in your warning, but better yet, get rid of
it, this doesn't seem like something we need to error on.
> +
> + mapsize = min(iova + size - iovamap, iommu->page_size);
> + /*
> + * FIXME: Currently we only support mapping page-size
> + * of subwindow-size.
> + */
> + if (mapsize < iommu->page_size) {
> + pr_err("%s iova (%llx) not alligned to window size %llx\n",
> + __func__, iovamap, iommu->page_size);
> + ret = -EINVAL;
> + break;
> + }
So you do care about the end alignment, but why can't we error for both
of these in advance?
> +
> + nr_pages = mapsize >> PAGE_SHIFT;
> +
> + /* Pin a contiguous chunk of memory */
> + ret = vfio_pin_pages(vaddr, nr_pages, prot, &pfn);
> + if (ret != nr_pages) {
> + pr_err("%s unable to pin pages = %lx, pinned(%lx/%lx)\n",
> + __func__, vaddr, npage, nr_pages);
> + ret = -EINVAL;
> + break;
> + }
How likely is this to succeed? It seems like we're relying on userspace
to use hugepages to make this work.
> +
> + ret = iommu_domain_window_enable(iommu->domain, win,
> + (phys_addr_t)pfn << PAGE_SHIFT,
> + mapsize, prot);
> + if (ret) {
> + pr_err("%s unable to iommu_map()\n", __func__);
> + ret = -EINVAL;
> + break;
> + }
You might consider how many cases you're returning EINVAL and think
about how difficult this will be to debug. I don't think we can leave
all these pr_err()s since it gives userspace a trivial way to spam log
files.
> +
> + /*
> + * Check if we abut a region below - nothing below 0.
> + * This is the most likely case when mapping chunks of
> + * physically contiguous regions within a virtual address
> + * range. Update the abutting entry in place since iova
> + * doesn't change.
> + */
> + if (likely(iovamap)) {
> + struct vfio_dma *tmp;
> + tmp = vfio_find_dma(iommu, iovamap - 1, 1);
> + if (tmp && tmp->prot == prot &&
> + tmp->vaddr + tmp->size == vaddr) {
> + tmp->size += mapsize;
> + dma = tmp;
> + }
> + }
> +
> + /*
> + * Check if we abut a region above - nothing above ~0 + 1.
> + * If we abut above and below, remove and free. If only
> + * abut above, remove, modify, reinsert.
> + */
> + if (likely(iovamap + mapsize)) {
> + struct vfio_dma *tmp;
> + tmp = vfio_find_dma(iommu, iovamap + mapsize, 1);
> + if (tmp && tmp->prot == prot &&
> + tmp->vaddr == vaddr + mapsize) {
> + vfio_remove_dma(iommu, tmp);
> + if (dma) {
> + dma->size += tmp->size;
> + kfree(tmp);
> + } else {
> + tmp->size += mapsize;
> + tmp->iova = iovamap;
> + tmp->vaddr = vaddr;
> + vfio_insert_dma(iommu, tmp);
> + dma = tmp;
> + }
> + }
> + }
> +
> + if (!dma) {
> + dma = kzalloc(sizeof(*dma), GFP_KERNEL);
> + if (!dma) {
> + iommu_unmap(iommu->domain, iovamap, mapsize);
> + vfio_unpin_pages(pfn, npage, prot, true);
> + ret = -ENOMEM;
> + break;
> + }
> +
> + dma->size = mapsize;
> + dma->iova = iovamap;
> + dma->vaddr = vaddr;
> + dma->prot = prot;
> + vfio_insert_dma(iommu, dma);
> + }
> +
> + iovamap += mapsize;
> + vaddr += mapsize;
Another chunk that looks like it's probably identical to type1. Can we
rip this out to another function and add it to common?
> + }
> +
> + if (ret) {
> + struct vfio_dma *tmp;
> + while ((tmp = vfio_find_dma(iommu, iova, size))) {
> + int r = vfio_remove_dma_overlap(iommu, iova,
> + &size, tmp);
> + if (WARN_ON(r || !size))
> + break;
> + }
> + }
Broken whitespace, please run scripts/checkpatch.pl before posting.
> +
> + vfio_enable_iommu_domain(iommu);
I don't quite understand your semantics here since you never use the
disable version, is this just redundant after the first mapping? When
dma_list is empty should it be disabled? Is there a bug here that an
error will enable the iommu domain even if there are no entries?
> + return 0;
> +}
> +
> +static int vfio_dma_do_map(struct vfio_iommu *iommu,
> + struct vfio_iommu_type1_dma_map *map)
> +{
> + dma_addr_t iova = map->iova;
> + size_t size = map->size;
> + unsigned long vaddr = map->vaddr;
> + int ret = 0, prot = 0;
> + long npage;
> +
> + /* READ/WRITE from device perspective */
> + if (map->flags & VFIO_DMA_MAP_FLAG_WRITE)
> + prot |= IOMMU_WRITE;
> + if (map->flags & VFIO_DMA_MAP_FLAG_READ)
> + prot |= IOMMU_READ;
> +
> + if (!prot)
> + return -EINVAL; /* No READ/WRITE? */
> +
> + /* Don't allow IOVA wrap */
> + if (iova + size && iova + size < iova)
> + return -EINVAL;
> +
> + /* Don't allow virtual address wrap */
> + if (vaddr + size && vaddr + size < vaddr)
> + return -EINVAL;
> +
> + /*
> + * FIXME: Currently we only support mapping page-size
> + * of subwindow-size.
> + */
> + if (size < iommu->page_size)
> + return -EINVAL;
> +
I'd think the start and end alignment could be tested here.
> + npage = size >> PAGE_SHIFT;
> + if (!npage)
> + return -EINVAL;
> +
> + mutex_lock(&iommu->lock);
> +
> + if (vfio_find_dma(iommu, iova, size)) {
> + ret = -EEXIST;
> + goto out_lock;
> + }
> +
> + vfio_dma_map(iommu, iova, vaddr, npage, prot);
> +
> +out_lock:
> + mutex_unlock(&iommu->lock);
> + return ret;
> +}
> +
> +static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> + struct vfio_iommu_type1_dma_unmap *unmap)
> +{
> + struct vfio_dma *dma;
> + size_t unmapped = 0, size;
> + int ret = 0;
> +
> + mutex_lock(&iommu->lock);
> +
> + while ((dma = vfio_find_dma(iommu, unmap->iova, unmap->size))) {
> + size = unmap->size;
> + ret = vfio_remove_dma_overlap(iommu, unmap->iova, &size, dma);
> + if (ret || !size)
> + break;
> + unmapped += size;
> + }
> +
> + mutex_unlock(&iommu->lock);
> +
> + /*
> + * We may unmap more than requested, update the unmap struct so
> + * userspace can know.
> + */
> + unmap->size = unmapped;
> +
> + return ret;
> +}
> +
> +static int vfio_handle_get_attr(struct vfio_iommu *iommu,
> + struct vfio_pamu_attr *pamu_attr)
> +{
> + switch (pamu_attr->attribute) {
> + case VFIO_ATTR_GEOMETRY: {
> + struct iommu_domain_geometry geom;
> + if (iommu_domain_get_attr(iommu->domain,
> + DOMAIN_ATTR_GEOMETRY, &geom)) {
> + pr_err("%s Error getting domain geometry\n",
> + __func__);
> + return -EFAULT;
> + }
> +
> + pamu_attr->attr_info.attr.aperture_start = geom.aperture_start;
> + pamu_attr->attr_info.attr.aperture_end = geom.aperture_end;
> + break;
> + }
> + case VFIO_ATTR_WINDOWS: {
> + u32 count;
> + if (iommu_domain_get_attr(iommu->domain,
> + DOMAIN_ATTR_WINDOWS, &count)) {
> + pr_err("%s Error getting domain windows\n",
> + __func__);
> + return -EFAULT;
> + }
> +
> + pamu_attr->attr_info.windows = count;
> + break;
> + }
> + case VFIO_ATTR_PAMU_STASH: {
> + struct pamu_stash_attribute stash;
> + if (iommu_domain_get_attr(iommu->domain,
> + DOMAIN_ATTR_FSL_PAMU_STASH, &stash)) {
> + pr_err("%s Error getting domain windows\n",
> + __func__);
> + return -EFAULT;
> + }
> +
> + pamu_attr->attr_info.stash.cpu = stash.cpu;
> + pamu_attr->attr_info.stash.cache = stash.cache;
> + break;
> + }
> +
> + default:
> + pr_err("%s Error: Invalid attribute (%d)\n",
> + __func__, pamu_attr->attribute);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int vfio_handle_set_attr(struct vfio_iommu *iommu,
> + struct vfio_pamu_attr *pamu_attr)
> +{
> + switch (pamu_attr->attribute) {
> + case VFIO_ATTR_GEOMETRY: {
> + struct iommu_domain_geometry geom;
> +
> + geom.aperture_start = pamu_attr->attr_info.attr.aperture_start;
> + geom.aperture_end = pamu_attr->attr_info.attr.aperture_end;
> + iommu->aperture_start = geom.aperture_start;
> + iommu->aperture_end = geom.aperture_end;
> + geom.force_aperture = 1;
> + if (iommu_domain_set_attr(iommu->domain,
> + DOMAIN_ATTR_GEOMETRY, &geom)) {
> + pr_err("%s Error setting domain geometry\n", __func__);
> + return -EFAULT;
> + }
> +
> + break;
> + }
> + case VFIO_ATTR_WINDOWS: {
> + u32 count = pamu_attr->attr_info.windows;
> + u64 size;
> + if (count > 256) {
> + pr_err("Number of subwindows requested (%d) is 256\n",
> + count);
> + return -EINVAL;
> + }
> + iommu->nsubwindows = pamu_attr->attr_info.windows;
> + size = iommu->aperture_end - iommu->aperture_start + 1;
> + do_div(size, count);
> + iommu->page_size = size;
> + if (iommu_domain_set_attr(iommu->domain,
> + DOMAIN_ATTR_WINDOWS, &count)) {
> + pr_err("%s Error getting domain windows\n",
> + __func__);
> + return -EFAULT;
> + }
> +
> + break;
> + }
> + case VFIO_ATTR_PAMU_STASH: {
> + struct pamu_stash_attribute stash;
> +
> + stash.cpu = pamu_attr->attr_info.stash.cpu;
> + stash.cache = pamu_attr->attr_info.stash.cache;
> + if (iommu_domain_set_attr(iommu->domain,
> + DOMAIN_ATTR_FSL_PAMU_STASH, &stash)) {
> + pr_err("%s Error getting domain windows\n",
> + __func__);
> + return -EFAULT;
> + }
> + break;
Why do we throw away the return value of iommu_domain_set_attr and
replace it with EFAULT in all these cases? I assume all these pr_err()s
are leftover debug. Can the user do anything they shouldn't through
these? How do we guarantee that?
> + }
> +
> + default:
> + pr_err("%s Error: Invalid attribute (%d)\n",
> + __func__, pamu_attr->attribute);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int vfio_msi_map(struct vfio_iommu *iommu,
> + struct vfio_pamu_msi_bank_map *msi_map, int prot)
> +{
> + struct msi_region region;
> + int window;
> + int ret;
> +
> + ret = msi_get_region(msi_map->msi_bank_index, ®ion);
> + if (ret) {
> + pr_err("%s MSI region (%d) not found\n", __func__,
> + msi_map->msi_bank_index);
> + return ret;
> + }
> +
> + window = iova_to_win(iommu, msi_map->iova);
> + ret = iommu_domain_window_enable(iommu->domain, window, region.addr,
> + region.size, prot);
> + if (ret) {
> + pr_err("%s Error: unable to map msi region\n", __func__);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int vfio_do_msi_map(struct vfio_iommu *iommu,
> + struct vfio_pamu_msi_bank_map *msi_map)
> +{
> + struct vfio_msi_dma *msi_dma;
> + int ret, prot = 0;
> +
> + /* READ/WRITE from device perspective */
> + if (msi_map->flags & VFIO_DMA_MAP_FLAG_WRITE)
> + prot |= IOMMU_WRITE;
> + if (msi_map->flags & VFIO_DMA_MAP_FLAG_READ)
> + prot |= IOMMU_READ;
> +
> + if (!prot)
> + return -EINVAL; /* No READ/WRITE? */
> +
> + ret = vfio_msi_map(iommu, msi_map, prot);
> + if (ret)
> + return ret;
> +
> + msi_dma = kzalloc(sizeof(*msi_dma), GFP_KERNEL);
> + if (!msi_dma)
> + return -ENOMEM;
> +
> + msi_dma->iova = msi_map->iova;
> + msi_dma->bank_id = msi_map->msi_bank_index;
> + list_add(&msi_dma->next, &iommu->msi_dma_list);
> + return 0;
What happens when the user creates multiple MSI mappings at the same
iova? What happens when DMA mappings overlap MSI mappings? Shouldn't
there be some locking around list manipulation?
> +}
> +
> +static void vfio_msi_unmap(struct vfio_iommu *iommu, dma_addr_t iova)
> +{
> + int window;
> + window = iova_to_win(iommu, iova);
> + iommu_domain_window_disable(iommu->domain, window);
> +}
> +
> +static int vfio_do_msi_unmap(struct vfio_iommu *iommu,
> + struct vfio_pamu_msi_bank_unmap *msi_unmap)
> +{
> + struct vfio_msi_dma *mdma, *mdma_tmp;
> +
> + list_for_each_entry_safe(mdma, mdma_tmp, &iommu->msi_dma_list, next) {
> + if (mdma->iova == msi_unmap->iova) {
> + vfio_msi_unmap(iommu, mdma->iova);
> + list_del(&mdma->next);
> + kfree(mdma);
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +static void *vfio_iommu_fsl_pamu_open(unsigned long arg)
> +{
> + struct vfio_iommu *iommu;
> +
> + if (arg != VFIO_FSL_PAMU_IOMMU)
> + return ERR_PTR(-EINVAL);
> +
> + iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
> + if (!iommu)
> + return ERR_PTR(-ENOMEM);
> +
> + INIT_LIST_HEAD(&iommu->group_list);
> + iommu->dma_list = RB_ROOT;
> + INIT_LIST_HEAD(&iommu->msi_dma_list);
> + mutex_init(&iommu->lock);
> +
> + /*
> + * Wish we didn't have to know about bus_type here.
> + */
> + iommu->domain = iommu_domain_alloc(&pci_bus_type);
> + if (!iommu->domain) {
> + kfree(iommu);
> + return ERR_PTR(-EIO);
> + }
> +
> + return iommu;
> +}
> +
> +static void vfio_iommu_fsl_pamu_release(void *iommu_data)
> +{
> + struct vfio_iommu *iommu = iommu_data;
> + struct vfio_group *group, *group_tmp;
> + struct vfio_msi_dma *mdma, *mdma_tmp;
> + struct rb_node *node;
> +
> + list_for_each_entry_safe(group, group_tmp, &iommu->group_list, next) {
> + iommu_detach_group(iommu->domain, group->iommu_group);
> + list_del(&group->next);
> + kfree(group);
> + }
> +
> + while ((node = rb_first(&iommu->dma_list))) {
> + struct vfio_dma *dma = rb_entry(node, struct vfio_dma, node);
> + size_t size = dma->size;
> + vfio_remove_dma_overlap(iommu, dma->iova, &size, dma);
> + if (WARN_ON(!size))
> + break;
> + }
> +
> + list_for_each_entry_safe(mdma, mdma_tmp, &iommu->msi_dma_list, next) {
> + vfio_msi_unmap(iommu, mdma->iova);
> + list_del(&mdma->next);
> + kfree(mdma);
> + }
> +
> + iommu_domain_free(iommu->domain);
> + iommu->domain = NULL;
> + kfree(iommu);
> +}
> +
> +static long vfio_iommu_fsl_pamu_ioctl(void *iommu_data,
> + unsigned int cmd, unsigned long arg)
> +{
> + struct vfio_iommu *iommu = iommu_data;
> + unsigned long minsz;
> +
> + if (cmd == VFIO_CHECK_EXTENSION) {
> + switch (arg) {
> + case VFIO_FSL_PAMU_IOMMU:
> + return 1;
> + default:
> + return 0;
> + }
> + } else if (cmd == VFIO_IOMMU_MAP_DMA) {
> + struct vfio_iommu_type1_dma_map map;
> + uint32_t mask = VFIO_DMA_MAP_FLAG_READ |
> + VFIO_DMA_MAP_FLAG_WRITE;
> +
> + minsz = offsetofend(struct vfio_iommu_type1_dma_map, size);
> +
> + if (copy_from_user(&map, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (map.argsz < minsz || map.flags & ~mask)
> + return -EINVAL;
> +
> + return vfio_dma_do_map(iommu, &map);
> +
> + } else if (cmd == VFIO_IOMMU_UNMAP_DMA) {
> + struct vfio_iommu_type1_dma_unmap unmap;
> + long ret;
> +
> + minsz = offsetofend(struct vfio_iommu_type1_dma_unmap, size);
> +
> + if (copy_from_user(&unmap, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (unmap.argsz < minsz || unmap.flags)
> + return -EINVAL;
> +
> + ret = vfio_dma_do_unmap(iommu, &unmap);
> + if (ret)
> + return ret;
> +
> + return copy_to_user((void __user *)arg, &unmap, minsz);
> + } else if (cmd == VFIO_IOMMU_PAMU_GET_ATTR) {
> + struct vfio_pamu_attr pamu_attr;
> +
> + minsz = offsetofend(struct vfio_pamu_attr, attr_info);
> + if (copy_from_user(&pamu_attr, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (pamu_attr.argsz < minsz)
> + return -EINVAL;
> +
> + vfio_handle_get_attr(iommu, &pamu_attr);
> +
> + copy_to_user((void __user *)arg, &pamu_attr, minsz);
> + return 0;
> + } else if (cmd == VFIO_IOMMU_PAMU_SET_ATTR) {
> + struct vfio_pamu_attr pamu_attr;
> +
> + minsz = offsetofend(struct vfio_pamu_attr, attr_info);
> + if (copy_from_user(&pamu_attr, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (pamu_attr.argsz < minsz)
> + return -EINVAL;
> +
> + vfio_handle_set_attr(iommu, &pamu_attr);
> + return 0;
> + } else if (cmd == VFIO_IOMMU_PAMU_GET_MSI_BANK_COUNT) {
> + return msi_get_region_count();
> + } else if (cmd == VFIO_IOMMU_PAMU_MAP_MSI_BANK) {
> + struct vfio_pamu_msi_bank_map msi_map;
> +
> + minsz = offsetofend(struct vfio_pamu_msi_bank_map, iova);
> + if (copy_from_user(&msi_map, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (msi_map.argsz < minsz)
> + return -EINVAL;
> +
> + vfio_do_msi_map(iommu, &msi_map);
> + return 0;
> + } else if (cmd == VFIO_IOMMU_PAMU_UNMAP_MSI_BANK) {
> + struct vfio_pamu_msi_bank_unmap msi_unmap;
> +
> + minsz = offsetofend(struct vfio_pamu_msi_bank_unmap, iova);
> + if (copy_from_user(&msi_unmap, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (msi_unmap.argsz < minsz)
> + return -EINVAL;
> +
> + vfio_do_msi_unmap(iommu, &msi_unmap);
> + return 0;
> +
> + }
> +
> + return -ENOTTY;
> +}
> +
> +static int vfio_iommu_fsl_pamu_attach_group(void *iommu_data,
> + struct iommu_group *iommu_group)
> +{
> + struct vfio_iommu *iommu = iommu_data;
> + struct vfio_group *group, *tmp;
> + int ret;
> +
> + group = kzalloc(sizeof(*group), GFP_KERNEL);
> + if (!group)
> + return -ENOMEM;
> +
> + mutex_lock(&iommu->lock);
> +
> + list_for_each_entry(tmp, &iommu->group_list, next) {
> + if (tmp->iommu_group == iommu_group) {
> + mutex_unlock(&iommu->lock);
> + kfree(group);
> + return -EINVAL;
> + }
> + }
> +
> + ret = iommu_attach_group(iommu->domain, iommu_group);
> + if (ret) {
> + mutex_unlock(&iommu->lock);
> + kfree(group);
> + return ret;
> + }
> +
> + group->iommu_group = iommu_group;
> + list_add(&group->next, &iommu->group_list);
> +
> + mutex_unlock(&iommu->lock);
> +
> + return 0;
> +}
> +
> +static void vfio_iommu_fsl_pamu_detach_group(void *iommu_data,
> + struct iommu_group *iommu_group)
> +{
> + struct vfio_iommu *iommu = iommu_data;
> + struct vfio_group *group;
> +
> + mutex_lock(&iommu->lock);
> +
> + list_for_each_entry(group, &iommu->group_list, next) {
> + if (group->iommu_group == iommu_group) {
> + iommu_detach_group(iommu->domain, iommu_group);
> + list_del(&group->next);
> + kfree(group);
> + break;
> + }
> + }
> +
> + mutex_unlock(&iommu->lock);
> +}
> +
> +static const struct vfio_iommu_driver_ops vfio_iommu_driver_ops_fsl_pamu = {
> + .name = "vfio-iommu-fsl_pamu",
> + .owner = THIS_MODULE,
> + .open = vfio_iommu_fsl_pamu_open,
> + .release = vfio_iommu_fsl_pamu_release,
> + .ioctl = vfio_iommu_fsl_pamu_ioctl,
> + .attach_group = vfio_iommu_fsl_pamu_attach_group,
> + .detach_group = vfio_iommu_fsl_pamu_detach_group,
> +};
> +
> +static int __init vfio_iommu_fsl_pamu_init(void)
> +{
> + if (!iommu_present(&pci_bus_type))
> + return -ENODEV;
> +
> + return vfio_register_iommu_driver(&vfio_iommu_driver_ops_fsl_pamu);
> +}
> +
> +static void __exit vfio_iommu_fsl_pamu_cleanup(void)
> +{
> + vfio_unregister_iommu_driver(&vfio_iommu_driver_ops_fsl_pamu);
> +}
> +
> +module_init(vfio_iommu_fsl_pamu_init);
> +module_exit(vfio_iommu_fsl_pamu_cleanup);
> +
> +MODULE_VERSION(DRIVER_VERSION);
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 0fd47f5..d359055 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -23,6 +23,7 @@
>
> #define VFIO_TYPE1_IOMMU 1
> #define VFIO_SPAPR_TCE_IOMMU 2
> +#define VFIO_FSL_PAMU_IOMMU 3
>
> /*
> * The IOCTL interface is designed for extensibility by embedding the
> @@ -451,4 +452,103 @@ struct vfio_iommu_spapr_tce_info {
>
> /* ***************************************************************** */
>
> +/*********** APIs for VFIO_PAMU type only ****************/
> +/*
> + * VFIO_IOMMU_PAMU_GET_ATTR - _IO(VFIO_TYPE, VFIO_BASE + 17,
> + * struct vfio_pamu_attr)
> + *
> + * Gets the iommu attributes for the current vfio container.
> + * Caller sets argsz and attribute. The ioctl fills in
> + * the provided struct vfio_pamu_attr based on the attribute
> + * value that was set.
> + * Return: 0 on success, -errno on failure
> + */
> +struct vfio_pamu_attr {
> + __u32 argsz;
> + __u32 flags; /* no flags currently */
> +#define VFIO_ATTR_GEOMETRY 0
> +#define VFIO_ATTR_WINDOWS 1
> +#define VFIO_ATTR_PAMU_STASH 2
> + __u32 attribute;
> +
> + union {
> + /* VFIO_ATTR_GEOMETRY */
> + struct {
> + /* first addr that can be mapped */
> + __u64 aperture_start;
> + /* last addr that can be mapped */
> + __u64 aperture_end;
> + } attr;
> +
> + /* VFIO_ATTR_WINDOWS */
> + __u32 windows; /* number of windows in the aperture
> + * initially this will be the max number
> + * of windows that can be set
> + */
> + /* VFIO_ATTR_PAMU_STASH */
> + struct {
> + __u32 cpu; /* CPU number for stashing */
> + __u32 cache; /* cache ID for stashing */
> + } stash;
> + } attr_info;
> +};
> +#define VFIO_IOMMU_PAMU_GET_ATTR _IO(VFIO_TYPE, VFIO_BASE + 17)
> +
> +/*
> + * VFIO_IOMMU_PAMU_SET_ATTR - _IO(VFIO_TYPE, VFIO_BASE + 18,
> + * struct vfio_pamu_attr)
> + *
> + * Sets the iommu attributes for the current vfio container.
> + * Caller sets struct vfio_pamu attr, including argsz and attribute and
> + * setting any fields that are valid for the attribute.
> + * Return: 0 on success, -errno on failure
> + */
> +#define VFIO_IOMMU_PAMU_SET_ATTR _IO(VFIO_TYPE, VFIO_BASE + 18)
> +
> +/*
> + * VFIO_IOMMU_PAMU_GET_MSI_BANK_COUNT - _IO(VFIO_TYPE, VFIO_BASE + 19, __u32)
> + *
> + * Returns the number of MSI banks for this platform. This tells user space
> + * how many aperture windows should be reserved for MSI banks when setting
> + * the PAMU geometry and window count.
> + * Return: __u32 bank count on success, -errno on failure
> + */
> +#define VFIO_IOMMU_PAMU_GET_MSI_BANK_COUNT _IO(VFIO_TYPE, VFIO_BASE + 19)
> +
> +/*
> + * VFIO_IOMMU_PAMU_MAP_MSI_BANK - _IO(VFIO_TYPE, VFIO_BASE + 20,
> + * struct vfio_pamu_msi_bank_map)
> + *
> + * Maps the MSI bank at the specified index and iova. User space must
> + * call this ioctl once for each MSI bank (count of banks is returned by
> + * VFIO_IOMMU_PAMU_GET_MSI_BANK_COUNT).
> + * Caller provides struct vfio_pamu_msi_bank_map with all fields set.
> + * Return: 0 on success, -errno on failure
> + */
> +
> +struct vfio_pamu_msi_bank_map {
> + __u32 argsz;
> + __u32 flags; /* no flags currently */
> + __u32 msi_bank_index; /* the index of the MSI bank */
> + __u64 iova; /* the iova the bank is to be mapped to */
> +};
> +#define VFIO_IOMMU_PAMU_MAP_MSI_BANK _IO(VFIO_TYPE, VFIO_BASE + 20)
> +
> +/*
> + * VFIO_IOMMU_PAMU_UNMAP_MSI_BANK - _IO(VFIO_TYPE, VFIO_BASE + 21,
> + * struct vfio_pamu_msi_bank_unmap)
> + *
> + * Unmaps the MSI bank at the specified iova.
> + * Caller provides struct vfio_pamu_msi_bank_unmap with all fields set.
> + * Operates on VFIO file descriptor (/dev/vfio/vfio).
> + * Return: 0 on success, -errno on failure
> + */
> +
> +struct vfio_pamu_msi_bank_unmap {
> + __u32 argsz;
> + __u32 flags; /* no flags currently */
> + __u64 iova; /* the iova to be unmapped to */
> +};
> +#define VFIO_IOMMU_PAMU_UNMAP_MSI_BANK _IO(VFIO_TYPE, VFIO_BASE + 21)
> +
> #endif /* _UAPIVFIO_H */
^ permalink raw reply
* Re: [PATCH v2 2/6] PCI/MSI: Factor out pci_get_msi_cap() interface
From: Bjorn Helgaas @ 2013-09-25 18:02 UTC (permalink / raw)
To: Tejun Heo
Cc: Joerg Roedel, x86@kernel.org, linux-kernel@vger.kernel.org,
linux-ide@vger.kernel.org, Alexander Gordeev, Jan Beulich,
linux-pci@vger.kernel.org, linuxppc-dev, Ingo Molnar
In-Reply-To: <20130920122736.GD7630@mtj.dyndns.org>
On Fri, Sep 20, 2013 at 07:27:36AM -0500, Tejun Heo wrote:
> On Fri, Sep 20, 2013 at 10:24:59AM +0200, Alexander Gordeev wrote:
> > * Make pci_enable_msix() return 0/-errno
>
> My choice would be this one.
I agree; it sounds like you've identified several bugs related to the
current confusing interface, so fixing that seems like the first step.
I hope we can avoid adding a plethora of interfaces to address unusual
corner cases. But if we do the above and it turns out not to be enough,
we can always extend it later.
Bjorn
^ permalink raw reply
* Re: [PATCH V4 1/3] powerpc/85xx: Add QE common init functions
From: Scott Wood @ 2013-09-25 18:01 UTC (permalink / raw)
To: Xie Xiaobo-R63061; +Cc: Wood Scott-B07421, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <69EC9ED88E3CC04094A78F8074A7986D5FF934@039-SN1MPN1-003.039d.mgd.msft.net>
On Wed, 2013-09-25 at 04:51 -0500, Xie Xiaobo-R63061 wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, September 25, 2013 7:13 AM
> > To: Xie Xiaobo-R63061
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Subject: Re: [PATCH V4 1/3] powerpc/85xx: Add QE common init functions
> >
> > On Tue, 2013-09-24 at 18:48 +0800, Xie Xiaobo wrote:
> > > Define two QE init functions in common file, and avoid the same codes
> > > being duplicated in board files.
> > >
> > > Signed-off-by: Xie Xiaobo <X.Xie@freescale.com>
> > > ---
> > > V4 -> V3: Nochange
> > >
> > > arch/powerpc/platforms/85xx/common.c | 51
> > > +++++++++++++++++++++++++++++++++++
> > > arch/powerpc/platforms/85xx/mpc85xx.h | 8 ++++++
> > > 2 files changed, 59 insertions(+)
> > >
> > > diff --git a/arch/powerpc/platforms/85xx/common.c
> > > b/arch/powerpc/platforms/85xx/common.c
> > > index d0861a0..08fff48 100644
> > > --- a/arch/powerpc/platforms/85xx/common.c
> > > +++ b/arch/powerpc/platforms/85xx/common.c
> > > @@ -7,6 +7,9 @@
> > > */
> > > #include <linux/of_platform.h>
> > >
> > > +#include <asm/machdep.h>
> > > +#include <asm/qe.h>
> > > +#include <asm/qe_ic.h>
> > > #include <sysdev/cpm2_pic.h>
> > >
> > > #include "mpc85xx.h"
> > > @@ -80,3 +83,51 @@ void __init mpc85xx_cpm2_pic_init(void)
> > > irq_set_chained_handler(irq, cpm2_cascade); } #endif
> > > +
> > > +#ifdef CONFIG_QUICC_ENGINE
> > > +void __init mpc85xx_qe_pic_init(void) {
> > > + struct device_node *np;
> > > +
> > > + np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic");
> > > + if (np) {
> > > + if (machine_is(mpc8568_mds) || machine_is(mpc8569_mds))
> > > + qe_ic_init(np, 0, qe_ic_cascade_muxed_mpic, NULL);
> > > + else
> > > + qe_ic_init(np, 0, qe_ic_cascade_low_mpic,
> > > + qe_ic_cascade_high_mpic);
> > > + of_node_put(np);
> > > + } else
> > > + pr_err("%s: Could not find qe-ic node\n", __func__); }
> >
> > Have the caller pass in a flag indicating the type of cascade. Or,
> > perhaps this function isn't worth factoring out. Where is the check for
> > p1021_mds? Where did 8568/9 MDS come from? I don't see those checks
> > removed in patch 2.
>
> [Xie] The qe_pic_init just call one function qe_ic_init(), So I just need factor out the qe_init function, Is it feasible?
"Or, perhaps this function isn't worth factoring out." :-)
-Scott
^ permalink raw reply
* Re: [PATCH v4 4/4] powerpc/85xx: add sysfs for pw20 state and altivec idle
From: Scott Wood @ 2013-09-25 17:56 UTC (permalink / raw)
To: Wang Dongsheng-B40534
Cc: Wood Scott-B07421, linuxppc-dev@lists.ozlabs.org,
Bhushan Bharat-R65777
In-Reply-To: <ABB05CD9C9F68C46A5CEDC7F154392590108FF41@039-SN2MPN1-021.039d.mgd.msft.net>
On Wed, 2013-09-25 at 03:10 -0500, Wang Dongsheng-B40534 wrote:
>
> > -----Original Message-----
> > From: Bhushan Bharat-R65777
> > Sent: Wednesday, September 25, 2013 2:23 PM
> > To: Wang Dongsheng-B40534; Wood Scott-B07421
> > Cc: linuxppc-dev@lists.ozlabs.org; Wang Dongsheng-B40534
> > Subject: RE: [PATCH v4 4/4] powerpc/85xx: add sysfs for pw20 state and
> > altivec idle
> >
> >
> >
> > > -----Original Message-----
> > > From: Linuxppc-dev [mailto:linuxppc-dev-
> > > bounces+bharat.bhushan=freescale.com@lists.ozlabs.org] On Behalf Of
> > > bounces+Dongsheng
> > > Wang
> > > Sent: Tuesday, September 24, 2013 2:59 PM
> > > To: Wood Scott-B07421
> > > Cc: linuxppc-dev@lists.ozlabs.org; Wang Dongsheng-B40534
> > > Subject: [PATCH v4 4/4] powerpc/85xx: add sysfs for pw20 state and
> > > altivec idle
> > >
> > > From: Wang Dongsheng <dongsheng.wang@freescale.com>
> > >
> > > Add a sys interface to enable/diable pw20 state or altivec idle, and
> > > control the wait entry time.
> > >
> > > Enable/Disable interface:
> > > 0, disable. 1, enable.
> > > /sys/devices/system/cpu/cpuX/pw20_state
> > > /sys/devices/system/cpu/cpuX/altivec_idle
> > >
> > > Set wait time interface:(Nanosecond)
> > > /sys/devices/system/cpu/cpuX/pw20_wait_time
> > > /sys/devices/system/cpu/cpuX/altivec_idle_wait_time
> > > Example: Base on TBfreq is 41MHZ.
> > > 1~47(ns): TB[63]
> > > 48~95(ns): TB[62]
> > > 96~191(ns): TB[61]
> > > 192~383(ns): TB[62]
> > > 384~767(ns): TB[60]
> > > ...
> > >
> > > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> > > ---
> > > *v4:
> > > Move code from 85xx/common.c to kernel/sysfs.c.
> > >
> > > Remove has_pw20_altivec_idle function.
> > >
> > > Change wait "entry_bit" to wait time.
> > >
> > > arch/powerpc/kernel/sysfs.c | 291
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 291 insertions(+)
> > >
> > > diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
> > > index 27a90b9..23fece6 100644
> > > --- a/arch/powerpc/kernel/sysfs.c
> > > +++ b/arch/powerpc/kernel/sysfs.c
> > > @@ -85,6 +85,279 @@ __setup("smt-snooze-delay=",
> > > setup_smt_snooze_delay);
> > >
> > > #endif /* CONFIG_PPC64 */
> > >
> > > +#ifdef CONFIG_FSL_SOC
> > > +#define MAX_BIT 63
> > > +
> > > +static u64 pw20_wt;
> > > +static u64 altivec_idle_wt;
> > > +
> > > +static unsigned int get_idle_ticks_bit(u64 ns) {
> > > + u64 cycle;
> > > +
> > > + cycle = div_u64(ns, 1000 / tb_ticks_per_usec);
> >
> > When tb_ticks_per_usec > 1000 (timebase frequency > 1GHz) then this will
> > always be ns, which is not correct, no?
Actually it'll be a divide by zero in that case.
> "1000 / tb_ticks_per_usec" means nsec_ticks_per_tb
>
> If timebase frequency > 1GHz, this should be "tb_ticks_per_usec / 1000" and to get tb_ticks_per_nsec.
> This should be changed to "cycle = ns * tb_ticks_per_nsec;"
>
> But at present we do not have such a platform that timebase frequency
> more than 1GHz. And I think it is not need to support such a situation.
> Because we have no environment to test it.
You can test it by hacking a wrong timebase frequency in and seeing what
the calculation does.
Or do something like this:
if (ns >= 10000)
cycle = ((ns + 500) / 1000) * tb_ticks_per_usec;
else
cycle = div_u64((u64)ns * tb_ticks_per_usec, 1000);
...which can be tested just by varying ns.
> If later there will be more than 1GHZ platform at that time to add this support.
There almost certainly won't be timebases that run that fast, but divide
by zero is a rather nasty way of responding if such a thing does happen.
-Scott
^ permalink raw reply
* Re: mm: insure topdown mmap chooses addresses above security minimum
From: Ingo Molnar @ 2013-09-25 17:44 UTC (permalink / raw)
To: Timothy Pepper
Cc: linux-mips, linux-sh, linux-mm, Paul Mackerras, H. Peter Anvin,
sparclinux, Michel Lespinasse, Russell King, x86, Ingo Molnar,
Rik van Riel, Al Viro, James Morris, Thomas Gleixner,
linux-arm-kernel, linuxppc-dev, Ralf Baechle, Paul Mundt,
Andrew Morton, Linus Torvalds, David S. Miller
In-Reply-To: <20130925171243.GA7428@tcpepper-desk.jf.intel.com>
* Timothy Pepper <timothy.c.pepper@linux.intel.com> wrote:
> On Wed 25 Sep at 09:30:49 +0200 mingo@kernel.org said:
> > > info.flags = VM_UNMAPPED_AREA_TOPDOWN;
> > > info.length = len;
> > > - info.low_limit = PAGE_SIZE;
> > > + info.low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
> > > info.high_limit = mm->mmap_base;
> > > info.align_mask = filp ? get_align_mask() : 0;
> > > info.align_offset = pgoff << PAGE_SHIFT;
> >
> > There appears to be a lot of repetition in these methods - instead of
> > changing 6 places it would be more future-proof to first factor out the
> > common bits and then to apply the fix to the shared implementation.
>
> Besides that existing redundancy in the multiple somewhat similar
> arch_get_unmapped_area_topdown() functions, I was expecting people might
> question the added redundancy of the six instances of:
>
> max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
That redundancy would be automatically addressed by my suggestion.
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH v2] powerpc/83xx: gianfar_ptp: select 1588 clock source through dts file
From: Richard Cochran @ 2013-09-25 17:13 UTC (permalink / raw)
To: Aida Mynzhasova; +Cc: devicetree, linuxppc-dev, netdev
In-Reply-To: <1380093863-5388-1-git-send-email-aida.mynzhasova@skitlab.ru>
On Wed, Sep 25, 2013 at 11:24:23AM +0400, Aida Mynzhasova wrote:
> Currently IEEE 1588 timer reference clock source is determined through
> hard-coded value in gianfar_ptp driver. This patch allows to select ptp
> clock source by means of device tree file node.
Looks okay to me now.
Acked-by: Richard Cochran <richardcochran@gmail.com>
^ permalink raw reply
* Re: mm: insure topdown mmap chooses addresses above security minimum
From: Timothy Pepper @ 2013-09-25 17:12 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-mips, linux-sh, linux-mm, Paul Mackerras, H. Peter Anvin,
sparclinux, Michel Lespinasse, Russell King, x86, Ingo Molnar,
Rik van Riel, Al Viro, James Morris, Thomas Gleixner,
linux-arm-kernel, linuxppc-dev, Ralf Baechle, Paul Mundt,
Andrew Morton, Linus Torvalds, David S. Miller
In-Reply-To: <20130925073048.GB27960@gmail.com>
On Wed 25 Sep at 09:30:49 +0200 mingo@kernel.org said:
> > info.flags = VM_UNMAPPED_AREA_TOPDOWN;
> > info.length = len;
> > - info.low_limit = PAGE_SIZE;
> > + info.low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
> > info.high_limit = mm->mmap_base;
> > info.align_mask = filp ? get_align_mask() : 0;
> > info.align_offset = pgoff << PAGE_SHIFT;
>
> There appears to be a lot of repetition in these methods - instead of
> changing 6 places it would be more future-proof to first factor out the
> common bits and then to apply the fix to the shared implementation.
Besides that existing redundancy in the multiple somewhat similar
arch_get_unmapped_area_topdown() functions, I was expecting people might
question the added redundancy of the six instances of:
max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr));
There's also a seventh similar instance if you consider
mm/mmap.c:round_hint_to_min() and its call stack. I'm inclined to
think mmap_min_addr should be validated/aligned in one place, namely on
initialization and input in security/min_addr.c:update_mmap_min_addr(),
with mmap_min_addr always stored as an aligned value.
In the past commit 40401530 Al Viro arguably moved that checking out
of the security code and toward the mmap code. Granted at that point
though there was only the round_hint_to_min() insuring the value in
mmap_min_addr was page aligned before use in that call path. I'm thinking
something like:
diff --git a/security/min_addr.c b/security/min_addr.c
--- a/security/min_addr.c
+++ b/security/min_addr.c
@@ -14,14 +14,16 @@ unsigned long dac_mmap_min_addr = CONFIG_DEFAULT_MMAP_MIN_ADDR;
*/
static void update_mmap_min_addr(void)
{
+ unsigned long addr;
#ifdef CONFIG_LSM_MMAP_MIN_ADDR
if (dac_mmap_min_addr > CONFIG_LSM_MMAP_MIN_ADDR)
- mmap_min_addr = dac_mmap_min_addr;
+ addr = dac_mmap_min_addr;
else
- mmap_min_addr = CONFIG_LSM_MMAP_MIN_ADDR;
+ addr = CONFIG_LSM_MMAP_MIN_ADDR;
#else
- mmap_min_addr = dac_mmap_min_addr;
+ addr = dac_mmap_min_addr;
#endif
+ mmap_min_addr = max(PAGE_SIZE, PAGE_ALIGN(addr));
}
/*
But this possibly has implications beyond the mmap code.
Al Viro, James Morris: any thoughts on the above?
Michel, Rik: what do you think of common helpers called by
ARM, MIPS, SH, Sparc, x86_64 arch_get_unmapped_area_topdown()
and arch_get_unmapped_area() to handle initialization of struct
vm_unmapped_area_info info fields which are currently mostly common?
Given the nuances of "mostly common" I'm not sure the result would
actually be positive for overall readability / self-documenting-ness of
the per arch files.
--
Tim Pepper <timothy.c.pepper@linux.intel.com>
Intel Open Source Technology Center
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox