* Re: [PATCH -next] ocxl: simplify the return expression of free_function_dev()
From: Andrew Donnellan @ 2020-09-28 1:24 UTC (permalink / raw)
To: Qinglang Miao, Frederic Barrat, Arnd Bergmann, Greg Kroah-Hartman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20200921131047.92526-1-miaoqinglang@huawei.com>
On 21/9/20 11:10 pm, Qinglang Miao wrote:
> Simplify the return expression.
>
> Signed-off-by: Qinglang Miao <miaoqinglang@huawei.com>
Looks good
Acked-by: Andrew Donnellan <ajd@linux.ibm.com>
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply
* Re: [PATCH] tpm: of: avoid __va() translation for event log address
From: Christophe Leroy @ 2020-09-28 5:56 UTC (permalink / raw)
To: Jarkko Sakkinen, Jason Gunthorpe, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras
Cc: linux-integrity, linuxppc-dev, Ard Biesheuvel, Peter Huewe,
Linux Kernel Mailing List
In-Reply-To: <20200927234434.GA5283@linux.intel.com>
Le 28/09/2020 à 01:44, Jarkko Sakkinen a écrit :
> On Fri, Sep 25, 2020 at 09:00:18AM -0300, Jason Gunthorpe wrote:
>> On Fri, Sep 25, 2020 at 01:29:20PM +0300, Jarkko Sakkinen wrote:
>>> On Fri, Sep 25, 2020 at 09:00:56AM +0200, Ard Biesheuvel wrote:
>>>> On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen
>>>> <jarkko.sakkinen@linux.intel.com> wrote:
>>>>>
>>>>> On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote:
>>>>>> The TPM event log is provided to the OS by the firmware, by loading
>>>>>> it into an area in memory and passing the physical address via a node
>>>>>> in the device tree.
>>>>>>
>>>>>> Currently, we use __va() to access the memory via the kernel's linear
>>>>>> map: however, it is not guaranteed that the linear map covers this
>>>>>> particular address, as we may be running under HIGHMEM on a 32-bit
>>>>>> architecture, or running firmware that uses a memory type for the
>>>>>> event log that is omitted from the linear map (such as EfiReserved).
>>>>>
>>>>> Makes perfect sense to the level that I wonder if this should have a
>>>>> fixes tag and/or needs to be backported to the stable kernels?
>>>>>
>>>>
>>>> AIUI, the code was written specifically for ppc64, which is a
>>>> non-highmem, non-EFI architecture. However, when we start reusing this
>>>> driver for ARM, this issue could pop up.
>>>>
>>>> The code itself has been refactored a couple of times, so I think it
>>>> will require different versions of the patch for different generations
>>>> of stable kernels.
>>>>
>>>> So perhaps just add Cc: <stable@vger.kernel.org>, and wait and see how
>>>> far back it applies cleanly?
>>>
>>> Yeah, I think I'll cc it with some note before the diffstat.
>>>
>>> I'm thinking to cap it to only 5.x kernels (at least first) unless it is
>>> dead easy to backport below that.
>>
>> I have this vauge recollection of pointing at this before and being
>> told that it had to be __va for some PPC reason?
>>
>> Do check with the PPC people first, I see none on the CC list.
>>
>> Jason
>
> Thanks, added arch/powerpc maintainers.
>
As far as I can see, memremap() won't work on PPC32 at least:
IIUC, memremap() calls arch_memremap_wb()
arch_memremap_wb() calls ioremap_cache()
In case of failure, then ioremap_wt() and ioremap_wc() are tried.
All ioremap calls end up in __ioremap_caller() which will return NULL in case you try to ioremap RAM.
So the statement "So instead, use memremap(), which will reuse the linear mapping if
it is valid, or create another mapping otherwise." seems to be wrong, at least for PPC32.
Even for PPC64 which doesn't seem to have the RAM check, I can't see that it will "reuse the linear
mapping".
Christophe
^ permalink raw reply
* Re: [PATCH] tpm: of: avoid __va() translation for event log address
From: Ard Biesheuvel @ 2020-09-28 6:20 UTC (permalink / raw)
To: Christophe Leroy
Cc: Linux Kernel Mailing List, Jarkko Sakkinen, Jason Gunthorpe,
Paul Mackerras, linux-integrity,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), Peter Huewe
In-Reply-To: <9be9c7e7-c424-d241-2255-ad854221bd2e@csgroup.eu>
On Mon, 28 Sep 2020 at 07:56, Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 28/09/2020 à 01:44, Jarkko Sakkinen a écrit :
> > On Fri, Sep 25, 2020 at 09:00:18AM -0300, Jason Gunthorpe wrote:
> >> On Fri, Sep 25, 2020 at 01:29:20PM +0300, Jarkko Sakkinen wrote:
> >>> On Fri, Sep 25, 2020 at 09:00:56AM +0200, Ard Biesheuvel wrote:
> >>>> On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen
> >>>> <jarkko.sakkinen@linux.intel.com> wrote:
> >>>>>
> >>>>> On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote:
> >>>>>> The TPM event log is provided to the OS by the firmware, by loading
> >>>>>> it into an area in memory and passing the physical address via a node
> >>>>>> in the device tree.
> >>>>>>
> >>>>>> Currently, we use __va() to access the memory via the kernel's linear
> >>>>>> map: however, it is not guaranteed that the linear map covers this
> >>>>>> particular address, as we may be running under HIGHMEM on a 32-bit
> >>>>>> architecture, or running firmware that uses a memory type for the
> >>>>>> event log that is omitted from the linear map (such as EfiReserved).
> >>>>>
> >>>>> Makes perfect sense to the level that I wonder if this should have a
> >>>>> fixes tag and/or needs to be backported to the stable kernels?
> >>>>>
> >>>>
> >>>> AIUI, the code was written specifically for ppc64, which is a
> >>>> non-highmem, non-EFI architecture. However, when we start reusing this
> >>>> driver for ARM, this issue could pop up.
> >>>>
> >>>> The code itself has been refactored a couple of times, so I think it
> >>>> will require different versions of the patch for different generations
> >>>> of stable kernels.
> >>>>
> >>>> So perhaps just add Cc: <stable@vger.kernel.org>, and wait and see how
> >>>> far back it applies cleanly?
> >>>
> >>> Yeah, I think I'll cc it with some note before the diffstat.
> >>>
> >>> I'm thinking to cap it to only 5.x kernels (at least first) unless it is
> >>> dead easy to backport below that.
> >>
> >> I have this vauge recollection of pointing at this before and being
> >> told that it had to be __va for some PPC reason?
> >>
> >> Do check with the PPC people first, I see none on the CC list.
> >>
> >> Jason
> >
> > Thanks, added arch/powerpc maintainers.
> >
>
> As far as I can see, memremap() won't work on PPC32 at least:
>
> IIUC, memremap() calls arch_memremap_wb()
> arch_memremap_wb() calls ioremap_cache()
> In case of failure, then ioremap_wt() and ioremap_wc() are tried.
>
> All ioremap calls end up in __ioremap_caller() which will return NULL in case you try to ioremap RAM.
>
> So the statement "So instead, use memremap(), which will reuse the linear mapping if
> it is valid, or create another mapping otherwise." seems to be wrong, at least for PPC32.
>
> Even for PPC64 which doesn't seem to have the RAM check, I can't see that it will "reuse the linear
> mapping".
>
It is there, please look again. Before any of the above happens,
memremap() will call try_ram_remap() for regions that are covered by a
IORESOURCE_SYSTEM_RAM, and map it using __va() if its PFN is valid and
it is not highmem.
So as far as I can tell, this change has no effect on PPC at all
unless its RAM is not described as IORESOURCE_SYSTEM_RAM.
^ permalink raw reply
* [PATCH] powerpc: net: bpf_jit_comp: Fix misuse of fallthrough
From: zhe.he @ 2020-09-28 9:00 UTC (permalink / raw)
To: gustavo, netdev, bpf, linuxppc-dev, linux-kernel, zhe.he
From: He Zhe <zhe.he@windriver.com>
The user defined label following "fallthrough" is not considered by GCC
and causes build failure.
kernel-source/include/linux/compiler_attributes.h:208:41: error: attribute
'fallthrough' not preceding a case label or default label [-Werror]
208 define fallthrough _attribute((fallthrough_))
^~~~~~~~~~~~~
Signed-off-by: He Zhe <zhe.he@windriver.com>
---
arch/powerpc/net/bpf_jit_comp.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index 78d61f97371e..e809cb5a1631 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -475,7 +475,6 @@ static int bpf_jit_build_body(struct bpf_prog *fp, u32 *image,
case BPF_JMP | BPF_JSET | BPF_K:
case BPF_JMP | BPF_JSET | BPF_X:
true_cond = COND_NE;
- fallthrough;
cond_branch:
/* same targets, can avoid doing the test :) */
if (filter[i].jt == filter[i].jf) {
--
2.26.2
^ permalink raw reply related
* Re: [PATCH] powerpc/papr_scm: Add PAPR command family to pass-through command-set
From: Vaibhav Jain @ 2020-09-28 13:14 UTC (permalink / raw)
To: linuxppc-dev, linux-nvdimm
Cc: Santosh Sivaraj, Aneesh Kumar K . V, Verma, Vishal L,
Oliver O'Halloran, Dan Williams, Ira Weiny
In-Reply-To: <20200913211904.24472-1-vaibhav@linux.ibm.com>
Hi Dan, Ira and Vishal,
Can you please take a look at this patch. Without it the functionality
to report nvdimm health via ndctl breaks on 5.9
Thanks,
~ Vaibhav
Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> Add NVDIMM_FAMILY_PAPR to the list of valid 'dimm_family_mask'
> acceptable by papr_scm. This is needed as since commit
> 92fe2aa859f5 ("libnvdimm: Validate command family indices") libnvdimm
> performs a validation of 'nd_cmd_pkg.nd_family' received as part of
> ND_CMD_CALL processing to ensure only known command families can use
> the general ND_CMD_CALL pass-through functionality.
>
> Without this change the ND_CMD_CALL pass-through targeting
> NVDIMM_FAMILY_PAPR error out with -EINVAL.
>
> Fixes: 92fe2aa859f5 ("libnvdimm: Validate command family indices")
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> arch/powerpc/platforms/pseries/papr_scm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 5493bc847bd08..27268370dee00 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -898,6 +898,9 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
> p->bus_desc.of_node = p->pdev->dev.of_node;
> p->bus_desc.provider_name = kstrdup(p->pdev->name, GFP_KERNEL);
>
> + /* Set the dimm command family mask to accept PDSMs */
> + set_bit(NVDIMM_FAMILY_PAPR, &p->bus_desc.dimm_family_mask);
> +
> if (!p->bus_desc.provider_name)
> return -ENOMEM;
>
> --
> 2.26.2
>
^ permalink raw reply
* Re: [PATCH 1/5] Documentation: dt: binding: fsl: Add 'fsl,ippdexpcr1-alt-addr' property
From: Rob Herring @ 2020-09-28 13:56 UTC (permalink / raw)
To: Ran Wang
Cc: devicetree@vger.kernel.org, Biwen Li, Shawn Guo,
linux-kernel@vger.kernel.org, Leo Li,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <AM6PR04MB5413BB2F8D044B2312DAEC4FF1380@AM6PR04MB5413.eurprd04.prod.outlook.com>
On Wed, Sep 23, 2020 at 1:44 AM Ran Wang <ran.wang_1@nxp.com> wrote:
>
> Hi Rob,
>
> On Wednesday, September 23, 2020 10:33 AM, Rob Herring wrote:
> >
> > On Wed, Sep 16, 2020 at 04:18:27PM +0800, Ran Wang wrote:
> > > From: Biwen Li <biwen.li@nxp.com>
> > >
> > > The 'fsl,ippdexpcr1-alt-addr' property is used to handle an errata
> > > A-008646 on LS1021A
> > >
> > > Signed-off-by: Biwen Li <biwen.li@nxp.com>
> > > Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
> > > ---
> > > Documentation/devicetree/bindings/soc/fsl/rcpm.txt | 19
> > > +++++++++++++++++++
> > > 1 file changed, 19 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> > > b/Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> > > index 5a33619..1be58a3 100644
> > > --- a/Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> > > +++ b/Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> > > @@ -34,6 +34,11 @@ Chassis Version Example Chips
> > > Optional properties:
> > > - little-endian : RCPM register block is Little Endian. Without it RCPM
> > > will be Big Endian (default case).
> > > + - fsl,ippdexpcr1-alt-addr : The property is related to a hardware issue
> > > + on SoC LS1021A and only needed on SoC LS1021A.
> > > + Must include 2 entries:
> > > + The first entry must be a link to the SCFG device node.
> > > + The 2nd entry must be offset of register IPPDEXPCR1 in SCFG.
> >
> > You don't need a DT change for this. You can find SCFG node by its compatible
> > string and then the offset should be known given this issue is only on 1 SoC.
>
> Did you mean that RCPM driver just to access IPPDEXPCR1 shadowed register in SCFG
> directly without fetching it's offset info. from DT?
Yes. There's only 1 possible value of the offset because there's only
one SoC, so the driver can hardcode the offset. And I assume there's
only one SCFG node, so you can find it by its compatible string
(of_find_compatible_node).
Rob
^ permalink raw reply
* Re: [PATCH] tpm: of: avoid __va() translation for event log address
From: Jarkko Sakkinen @ 2020-09-28 14:09 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Linux Kernel Mailing List, Jason Gunthorpe, Paul Mackerras,
linux-integrity, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
Peter Huewe
In-Reply-To: <CAMj1kXGxNgixUEocma-9F3fYgdJJJADh=bvyrCziXkuArErWdA@mail.gmail.com>
On Mon, Sep 28, 2020 at 08:20:18AM +0200, Ard Biesheuvel wrote:
> On Mon, 28 Sep 2020 at 07:56, Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
> >
> >
> >
> > Le 28/09/2020 à 01:44, Jarkko Sakkinen a écrit :
> > > On Fri, Sep 25, 2020 at 09:00:18AM -0300, Jason Gunthorpe wrote:
> > >> On Fri, Sep 25, 2020 at 01:29:20PM +0300, Jarkko Sakkinen wrote:
> > >>> On Fri, Sep 25, 2020 at 09:00:56AM +0200, Ard Biesheuvel wrote:
> > >>>> On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen
> > >>>> <jarkko.sakkinen@linux.intel.com> wrote:
> > >>>>>
> > >>>>> On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote:
> > >>>>>> The TPM event log is provided to the OS by the firmware, by loading
> > >>>>>> it into an area in memory and passing the physical address via a node
> > >>>>>> in the device tree.
> > >>>>>>
> > >>>>>> Currently, we use __va() to access the memory via the kernel's linear
> > >>>>>> map: however, it is not guaranteed that the linear map covers this
> > >>>>>> particular address, as we may be running under HIGHMEM on a 32-bit
> > >>>>>> architecture, or running firmware that uses a memory type for the
> > >>>>>> event log that is omitted from the linear map (such as EfiReserved).
> > >>>>>
> > >>>>> Makes perfect sense to the level that I wonder if this should have a
> > >>>>> fixes tag and/or needs to be backported to the stable kernels?
> > >>>>>
> > >>>>
> > >>>> AIUI, the code was written specifically for ppc64, which is a
> > >>>> non-highmem, non-EFI architecture. However, when we start reusing this
> > >>>> driver for ARM, this issue could pop up.
> > >>>>
> > >>>> The code itself has been refactored a couple of times, so I think it
> > >>>> will require different versions of the patch for different generations
> > >>>> of stable kernels.
> > >>>>
> > >>>> So perhaps just add Cc: <stable@vger.kernel.org>, and wait and see how
> > >>>> far back it applies cleanly?
> > >>>
> > >>> Yeah, I think I'll cc it with some note before the diffstat.
> > >>>
> > >>> I'm thinking to cap it to only 5.x kernels (at least first) unless it is
> > >>> dead easy to backport below that.
> > >>
> > >> I have this vauge recollection of pointing at this before and being
> > >> told that it had to be __va for some PPC reason?
> > >>
> > >> Do check with the PPC people first, I see none on the CC list.
> > >>
> > >> Jason
> > >
> > > Thanks, added arch/powerpc maintainers.
> > >
> >
> > As far as I can see, memremap() won't work on PPC32 at least:
> >
> > IIUC, memremap() calls arch_memremap_wb()
> > arch_memremap_wb() calls ioremap_cache()
> > In case of failure, then ioremap_wt() and ioremap_wc() are tried.
> >
> > All ioremap calls end up in __ioremap_caller() which will return NULL in case you try to ioremap RAM.
> >
> > So the statement "So instead, use memremap(), which will reuse the linear mapping if
> > it is valid, or create another mapping otherwise." seems to be wrong, at least for PPC32.
> >
> > Even for PPC64 which doesn't seem to have the RAM check, I can't see that it will "reuse the linear
> > mapping".
> >
>
> It is there, please look again. Before any of the above happens,
> memremap() will call try_ram_remap() for regions that are covered by a
> IORESOURCE_SYSTEM_RAM, and map it using __va() if its PFN is valid and
> it is not highmem.
>
> So as far as I can tell, this change has no effect on PPC at all
> unless its RAM is not described as IORESOURCE_SYSTEM_RAM.
Any chance for someone to test this on PPC32?
/Jarkko
^ permalink raw reply
* Re: [PATCH v8 2/8] powerpc/vdso: Remove __kernel_datapage_offset and simplify __get_datapage()
From: Dmitry Safonov @ 2020-09-28 15:08 UTC (permalink / raw)
To: Christophe Leroy, Will Deacon, Michael Ellerman
Cc: nathanl, linux-arch, Arnd Bergmann, open list, Paul Mackerras,
Andy Lutomirski, Thomas Gleixner, Vincenzo Frascino, linuxppc-dev
In-Reply-To: <ad72ffd3-a552-cc98-7545-d30285fd5219@csgroup.eu>
On 9/27/20 8:43 AM, Christophe Leroy wrote:
>
>
> Le 21/09/2020 à 13:26, Will Deacon a écrit :
>> On Fri, Aug 28, 2020 at 12:14:28PM +1000, Michael Ellerman wrote:
>>> Dmitry Safonov <0x7f454c46@gmail.com> writes:
[..]
>>>> I'll cook a patch for vm_special_mapping if you don't mind :-)
>>>
>>> That would be great, thanks!
>>
>> I lost track of this one. Is there a patch kicking around to resolve
>> this,
>> or is the segfault expected behaviour?
>>
>
> IIUC dmitry said he will cook a patch. I have not seen any patch yet.
Yes, sorry about the delay - I was a bit busy with xfrm patches.
I'll send patches for .close() this week, working on them now.
> AFAIKS, among the architectures having VDSO sigreturn trampolines, only
> SH, X86 and POWERPC provide alternative trampoline on stack when VDSO is
> not there.
>
> All other architectures just having a VDSO don't expect VDSO to not be
> mapped.
>
> As far as nowadays stacks are mapped non-executable, getting a segfaut
> is expected behaviour. However, I think we should really make it
> cleaner. Today it segfaults because it is still pointing to the VDSO
> trampoline that has been unmapped. But should the user map some other
> code at the same address, we'll run in the weed on signal return instead
> of segfaulting.
+1.
> So VDSO unmapping should really be properly managed, the reference
> should be properly cleared in order to segfault in a controllable manner.
>
> Only powerpc has a hook to properly clear the VDSO pointer when VDSO is
> unmapped.
Thanks,
Dmitry
^ permalink raw reply
* Re: [PATCH] powerpc/papr_scm: Add PAPR command family to pass-through command-set
From: Ira Weiny @ 2020-09-28 15:45 UTC (permalink / raw)
To: Vaibhav Jain
Cc: Santosh Sivaraj, linux-nvdimm, Aneesh Kumar K . V,
Verma, Vishal L, Oliver O'Halloran, Dan Williams,
linuxppc-dev
In-Reply-To: <87pn662gc3.fsf@vajain21.in.ibm.com>
On Mon, Sep 28, 2020 at 06:44:52PM +0530, Vaibhav Jain wrote:
> Hi Dan, Ira and Vishal,
>
> Can you please take a look at this patch. Without it the functionality
> to report nvdimm health via ndctl breaks on 5.9
Sorry...
>
> Thanks,
> ~ Vaibhav
>
> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
>
> > Add NVDIMM_FAMILY_PAPR to the list of valid 'dimm_family_mask'
> > acceptable by papr_scm. This is needed as since commit
> > 92fe2aa859f5 ("libnvdimm: Validate command family indices") libnvdimm
> > performs a validation of 'nd_cmd_pkg.nd_family' received as part of
> > ND_CMD_CALL processing to ensure only known command families can use
> > the general ND_CMD_CALL pass-through functionality.
> >
> > Without this change the ND_CMD_CALL pass-through targeting
> > NVDIMM_FAMILY_PAPR error out with -EINVAL.
> >
> > Fixes: 92fe2aa859f5 ("libnvdimm: Validate command family indices")
> > Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
LGTM
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> > arch/powerpc/platforms/pseries/papr_scm.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> > index 5493bc847bd08..27268370dee00 100644
> > --- a/arch/powerpc/platforms/pseries/papr_scm.c
> > +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> > @@ -898,6 +898,9 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
> > p->bus_desc.of_node = p->pdev->dev.of_node;
> > p->bus_desc.provider_name = kstrdup(p->pdev->name, GFP_KERNEL);
> >
> > + /* Set the dimm command family mask to accept PDSMs */
> > + set_bit(NVDIMM_FAMILY_PAPR, &p->bus_desc.dimm_family_mask);
> > +
> > if (!p->bus_desc.provider_name)
> > return -ENOMEM;
> >
> > --
> > 2.26.2
> >
^ permalink raw reply
* Re: [PATCH] powerpc/papr_scm: Support dynamic enable/disable of performance statistics
From: Ira Weiny @ 2020-09-28 16:04 UTC (permalink / raw)
To: Vaibhav Jain
Cc: Santosh Sivaraj, linux-nvdimm, Aneesh Kumar K . V,
Oliver O'Halloran, Dan Williams, linuxppc-dev
In-Reply-To: <20200913212115.24958-1-vaibhav@linux.ibm.com>
On Mon, Sep 14, 2020 at 02:51:15AM +0530, Vaibhav Jain wrote:
> Collection of performance statistics of an NVDIMM can be dynamically
> enabled/disabled from the Hypervisor Management Console even when the
> guest lpar is running. The current implementation however will check if
> the performance statistics collection is supported during NVDIMM probe
> and if yes will assume that to be the case afterwards.
>
> Hence we update papr_scm to remove this assumption from the code by
> eliminating the 'stat_buffer_len' member from 'struct papr_scm_priv'
> that was used to cache the max buffer size needed to fetch NVDIMM
> performance stats from PHYP. With that struct member gone, various
> functions that depended on it are updated. Specifically
> perf_stats_show() is updated to query the PHYP first for the size of
> buffer needed to hold all performance statistics instead of relying on
> 'stat_buffer_len'
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> arch/powerpc/platforms/pseries/papr_scm.c | 53 +++++++++++------------
> 1 file changed, 25 insertions(+), 28 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 27268370dee00..6697e1c3b9ebe 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -112,9 +112,6 @@ struct papr_scm_priv {
>
> /* Health information for the dimm */
> u64 health_bitmap;
> -
> - /* length of the stat buffer as expected by phyp */
> - size_t stat_buffer_len;
> };
>
> static LIST_HEAD(papr_nd_regions);
> @@ -230,14 +227,15 @@ static int drc_pmem_query_n_bind(struct papr_scm_priv *p)
> * - If buff_stats == NULL the return value is the size in byes of the buffer
> * needed to hold all supported performance-statistics.
> * - If buff_stats != NULL and num_stats == 0 then we copy all known
> - * performance-statistics to 'buff_stat' and expect to be large enough to
> - * hold them.
> + * performance-statistics to 'buff_stat' and expect it to be large enough to
> + * hold them. The 'buff_size' args contains the size of the 'buff_stats'
> * - if buff_stats != NULL and num_stats > 0 then copy the requested
> * performance-statistics to buff_stats.
> */
> static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
> struct papr_scm_perf_stats *buff_stats,
> - unsigned int num_stats)
> + unsigned int num_stats,
> + size_t buff_size)
> {
> unsigned long ret[PLPAR_HCALL_BUFSIZE];
> size_t size;
> @@ -261,12 +259,18 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
> size = sizeof(struct papr_scm_perf_stats) +
> num_stats * sizeof(struct papr_scm_perf_stat);
> else
> - size = p->stat_buffer_len;
> + size = buff_size;
> } else {
> /* In case of no out buffer ignore the size */
> size = 0;
> }
>
> + /* verify that the buffer size needed is sufficient */
> + if (size > buff_size) {
> + __WARN();
> + return -EINVAL;
> + }
> +
> /* Do the HCALL asking PHYP for info */
> rc = plpar_hcall(H_SCM_PERFORMANCE_STATS, ret, p->drc_index,
> buff_stats ? virt_to_phys(buff_stats) : 0,
> @@ -277,6 +281,10 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
> dev_err(&p->pdev->dev,
> "Unknown performance stats, Err:0x%016lX\n", ret[0]);
> return -ENOENT;
> + } else if (rc == H_AUTHORITY) {
> + dev_dbg(&p->pdev->dev,
> + "Performance stats in-accessible\n");
> + return -EPERM;
> } else if (rc != H_SUCCESS) {
> dev_err(&p->pdev->dev,
> "Failed to query performance stats, Err:%lld\n", rc);
> @@ -526,10 +534,6 @@ static int papr_pdsm_fuel_gauge(struct papr_scm_priv *p,
> struct papr_scm_perf_stat *stat;
> struct papr_scm_perf_stats *stats;
>
> - /* Silently fail if fetching performance metrics isn't supported */
> - if (!p->stat_buffer_len)
> - return 0;
> -
> /* Allocate request buffer enough to hold single performance stat */
> size = sizeof(struct papr_scm_perf_stats) +
> sizeof(struct papr_scm_perf_stat);
> @@ -543,9 +547,11 @@ static int papr_pdsm_fuel_gauge(struct papr_scm_priv *p,
> stat->stat_val = 0;
>
> /* Fetch the fuel gauge and populate it in payload */
> - rc = drc_pmem_query_stats(p, stats, 1);
> + rc = drc_pmem_query_stats(p, stats, 1, size);
> if (rc < 0) {
> dev_dbg(&p->pdev->dev, "Err(%d) fetching fuel gauge\n", rc);
> + /* Silently fail if unable to fetch performance metric */
> + rc = 0;
> goto free_stats;
> }
>
> @@ -786,23 +792,25 @@ static ssize_t perf_stats_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> int index;
> - ssize_t rc;
> + ssize_t rc, buff_len;
> struct seq_buf s;
> struct papr_scm_perf_stat *stat;
> struct papr_scm_perf_stats *stats;
> struct nvdimm *dimm = to_nvdimm(dev);
> struct papr_scm_priv *p = nvdimm_provider_data(dimm);
>
> - if (!p->stat_buffer_len)
> - return -ENOENT;
> + /* fetch the length of buffer needed to get all stats */
> + buff_len = drc_pmem_query_stats(p, NULL, 0, 0);
> + if (buff_len <= 0)
> + return buff_len;
Generally I can't find anything wrong with this patch technically but the
architecture of drc_pmem_query_stats() seems overly complicated.
IOW, I feel like you are overloading drc_pmem_query_stats() in an odd way which
makes it and the callers code confusing. Why can't you have a separate
function which returns the max buffer length and separate out the logic within
drc_pmem_query_stats() to make it clear how to call plpar_hcall() to get this
information?
Ira
>
> /* Allocate the buffer for phyp where stats are written */
> - stats = kzalloc(p->stat_buffer_len, GFP_KERNEL);
> + stats = kzalloc(buff_len, GFP_KERNEL);
> if (!stats)
> return -ENOMEM;
>
> /* Ask phyp to return all dimm perf stats */
> - rc = drc_pmem_query_stats(p, stats, 0);
> + rc = drc_pmem_query_stats(p, stats, 0, buff_len);
> if (rc)
> goto free_stats;
> /*
> @@ -891,7 +899,6 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
> struct nd_region_desc ndr_desc;
> unsigned long dimm_flags;
> int target_nid, online_nid;
> - ssize_t stat_size;
>
> p->bus_desc.ndctl = papr_scm_ndctl;
> p->bus_desc.module = THIS_MODULE;
> @@ -962,16 +969,6 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
> list_add_tail(&p->region_list, &papr_nd_regions);
> mutex_unlock(&papr_ndr_lock);
>
> - /* Try retriving the stat buffer and see if its supported */
> - stat_size = drc_pmem_query_stats(p, NULL, 0);
> - if (stat_size > 0) {
> - p->stat_buffer_len = stat_size;
> - dev_dbg(&p->pdev->dev, "Max perf-stat size %lu-bytes\n",
> - p->stat_buffer_len);
> - } else {
> - dev_info(&p->pdev->dev, "Dimm performance stats unavailable\n");
> - }
> -
> return 0;
>
> err: nvdimm_bus_unregister(p->bus);
> --
> 2.26.2
>
^ permalink raw reply
* Re: [PATCH v2 2/3] lkdtm/powerpc: Add SLB multihit test
From: Ganesh @ 2020-09-28 17:32 UTC (permalink / raw)
To: Kees Cook; +Cc: msuchanek, linuxppc-dev, mahesh, npiggin
In-Reply-To: <202009251244.A4396AFF@keescook>
On 9/26/20 1:27 AM, Kees Cook wrote:
> On Fri, Sep 25, 2020 at 04:01:22PM +0530, Ganesh Goudar wrote:
>> Add support to inject slb multihit errors, to test machine
>> check handling.
> Thank you for more tests in here!
>
>> Based on work by Mahesh Salgaonkar and Michal Suchánek.
>>
>> Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
>> Cc: Michal Suchánek <msuchanek@suse.de>
> Should these be Co-developed-by: with S-o-b?
Sure, ill add.
>> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
>> ---
>> drivers/misc/lkdtm/Makefile | 4 ++
>> drivers/misc/lkdtm/core.c | 3 +
>> drivers/misc/lkdtm/lkdtm.h | 3 +
>> drivers/misc/lkdtm/powerpc.c | 132 +++++++++++++++++++++++++++++++++++
>> 4 files changed, 142 insertions(+)
>> create mode 100644 drivers/misc/lkdtm/powerpc.c
>>
>> diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile
>> index c70b3822013f..6a82f407fbcd 100644
>> --- a/drivers/misc/lkdtm/Makefile
>> +++ b/drivers/misc/lkdtm/Makefile
>> @@ -11,6 +11,10 @@ lkdtm-$(CONFIG_LKDTM) += usercopy.o
>> lkdtm-$(CONFIG_LKDTM) += stackleak.o
>> lkdtm-$(CONFIG_LKDTM) += cfi.o
>>
>> +ifeq ($(CONFIG_PPC64),y)
>> +lkdtm-$(CONFIG_LKDTM) += powerpc.o
>> +endif
> This can just be:
>
> lkdtm-$(CONFIG_PPC64) += powerpc.o
ok
>> +
>> KASAN_SANITIZE_stackleak.o := n
>> KCOV_INSTRUMENT_rodata.o := n
>>
>> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
>> index a5e344df9166..8d5db42baa90 100644
>> --- a/drivers/misc/lkdtm/core.c
>> +++ b/drivers/misc/lkdtm/core.c
>> @@ -178,6 +178,9 @@ static const struct crashtype crashtypes[] = {
>> #ifdef CONFIG_X86_32
>> CRASHTYPE(DOUBLE_FAULT),
>> #endif
>> +#ifdef CONFIG_PPC64
>> + CRASHTYPE(PPC_SLB_MULTIHIT),
>> +#endif
>> };
>>
>>
>> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
>> index 8878538b2c13..b305bd511ee5 100644
>> --- a/drivers/misc/lkdtm/lkdtm.h
>> +++ b/drivers/misc/lkdtm/lkdtm.h
>> @@ -104,4 +104,7 @@ void lkdtm_STACKLEAK_ERASING(void);
>> /* cfi.c */
>> void lkdtm_CFI_FORWARD_PROTO(void);
>>
>> +/* powerpc.c */
>> +void lkdtm_PPC_SLB_MULTIHIT(void);
>> +
>> #endif
>> diff --git a/drivers/misc/lkdtm/powerpc.c b/drivers/misc/lkdtm/powerpc.c
>> new file mode 100644
>> index 000000000000..d6db18444757
>> --- /dev/null
>> +++ b/drivers/misc/lkdtm/powerpc.c
>> @@ -0,0 +1,132 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
> Please #include "lkdtm.h" here to get the correct pr_fmt heading (and
> any future header adjustments).
Sure
>> +#include <linux/slab.h>
>> +#include <linux/vmalloc.h>
>> +
>> +static inline unsigned long get_slb_index(void)
>> +{
>> + unsigned long index;
>> +
>> + index = get_paca()->stab_rr;
>> +
>> + /*
>> + * simple round-robin replacement of slb starting at SLB_NUM_BOLTED.
>> + */
>> + if (index < (mmu_slb_size - 1))
>> + index++;
>> + else
>> + index = SLB_NUM_BOLTED;
>> + get_paca()->stab_rr = index;
>> + return index;
>> +}
>> +
>> +#define slb_esid_mask(ssize) \
>> + (((ssize) == MMU_SEGSIZE_256M) ? ESID_MASK : ESID_MASK_1T)
>> +
>> +static inline unsigned long mk_esid_data(unsigned long ea, int ssize,
>> + unsigned long slot)
>> +{
>> + return (ea & slb_esid_mask(ssize)) | SLB_ESID_V | slot;
>> +}
>> +
>> +#define slb_vsid_shift(ssize) \
>> + ((ssize) == MMU_SEGSIZE_256M ? SLB_VSID_SHIFT : SLB_VSID_SHIFT_1T)
>> +
>> +static inline unsigned long mk_vsid_data(unsigned long ea, int ssize,
>> + unsigned long flags)
>> +{
>> + return (get_kernel_vsid(ea, ssize) << slb_vsid_shift(ssize)) | flags |
>> + ((unsigned long)ssize << SLB_VSID_SSIZE_SHIFT);
>> +}
>> +
>> +static void insert_slb_entry(char *p, int ssize)
>> +{
>> + unsigned long flags, entry;
>> +
>> + flags = SLB_VSID_KERNEL | mmu_psize_defs[MMU_PAGE_64K].sllp;
>> + preempt_disable();
>> +
>> + entry = get_slb_index();
>> + asm volatile("slbmte %0,%1" :
>> + : "r" (mk_vsid_data((unsigned long)p, ssize, flags)),
>> + "r" (mk_esid_data((unsigned long)p, ssize, entry))
>> + : "memory");
>> +
>> + entry = get_slb_index();
>> + asm volatile("slbmte %0,%1" :
>> + : "r" (mk_vsid_data((unsigned long)p, ssize, flags)),
>> + "r" (mk_esid_data((unsigned long)p, ssize, entry))
>> + : "memory");
>> + preempt_enable();
>> + p[0] = '!';
>> +}
> Can you add some comments to these helpers? It'll help people (me) with
> understanding what is actually being done here (and more importantly,
> what is _expected_ to happen).
Sure, ill add comments.
>> +
>> +static void inject_vmalloc_slb_multihit(void)
>> +{
>> + char *p;
>> +
>> + p = vmalloc(2048);
>> + if (!p)
>> + return;
>> +
>> + insert_slb_entry(p, MMU_SEGSIZE_1T);
>> + vfree(p);
>> +}
>> +
>> +static void inject_kmalloc_slb_multihit(void)
>> +{
>> + char *p;
>> +
>> + p = kmalloc(2048, GFP_KERNEL);
>> + if (!p)
>> + return;
>> +
>> + insert_slb_entry(p, MMU_SEGSIZE_1T);
>> + kfree(p);
>> +}
> It looks like the expected failure injection is actually the "p[0] = '!'" line in the
> "insert" helper? I would expect pr_info/pr_err wrappers, etc, as in
> other lkdtm tests.
Yes "p[0] = '!'" is the final step in this error injection, ill add
comments.
> If this is the negative test, what does the positive test look like?
> e.g. in other lkdtm tests, first a positive test is done, then a
> negative: first run a legit function, then run a function from a bad
> location.
Yes, this is negative test, As SLB search is very fundamental thing in
address translation and
used all the time, no positive test required here.
>> +
>> +static void insert_dup_slb_entry_0(void)
>> +{
>> + unsigned long test_address = 0xC000000000000000;
>> + volatile unsigned long *test_ptr;
>> + unsigned long entry, i = 0;
>> + unsigned long esid, vsid;
>> +
>> + test_ptr = (unsigned long *)test_address;
>> + preempt_disable();
>> +
>> + asm volatile("slbmfee %0,%1" : "=r" (esid) : "r" (i));
>> + asm volatile("slbmfev %0,%1" : "=r" (vsid) : "r" (i));
>> + entry = get_slb_index();
>> +
>> + /* for i !=0 we would need to mask out the old entry number */
>> + asm volatile("slbmte %0,%1" :
>> + : "r" (vsid),
>> + "r" (esid | entry)
>> + : "memory");
>> +
>> + asm volatile("slbmfee %0,%1" : "=r" (esid) : "r" (i));
>> + asm volatile("slbmfev %0,%1" : "=r" (vsid) : "r" (i));
>> + entry = get_slb_index();
>> +
>> + /* for i !=0 we would need to mask out the old entry number */
>> + asm volatile("slbmte %0,%1" :
>> + : "r" (vsid),
>> + "r" (esid | entry)
>> + : "memory");
>> +
>> + pr_info("lkdtm: %s accessing test address 0x%lx: 0x%lx\n",
>> + __func__, test_address, *test_ptr);
>> +
>> + preempt_enable();
>> +}
> What does this do?
It tries to inject error in address range where most important kernel
data structure may fall.
Mahesh and Michal Suchánek correct me if I am wrong.
>> +
>> +void lkdtm_PPC_SLB_MULTIHIT(void)
>> +{
>> + if (mmu_has_feature(MMU_FTR_HPTE_TABLE)) {
>> + inject_vmalloc_slb_multihit();
>> + inject_kmalloc_slb_multihit();
>> + insert_dup_slb_entry_0();
>> + }
>> + pr_info("Recovered from SLB multihit. (Ignore this message on non HPTE machines)");
> Is this bad? If so, I'd expect pr_err("FAIL: ...") Can HPTE machines be
> detected so that an XFAIL can be emitted instead?
Sure, ill add XFAIL print in non HPTE case. And in case of failure we
will not return
here, so if we hit this print line we are good.
> Since there are three (two?) distinct regions being tested, should these
> be separate tests? Right now there is no way to separate a vmalloc
> failure from a kmalloc failure, and no way to fail the first without
> hiding the result from the latter (or maybe the machine cannot survive
> this test? ... which should also be a comment.)
Sure, ill comment, And yes machine cannot survive these tests in case of
failure to handle.
> And finally, assuming a successful test (or testing from a separate
> thread later), so there any state that needs to be restored (or cleaned
> up before doing the "insert" calls)?
No, there is nothing to be restored here, Thanks!
> Thanks!
>
^ permalink raw reply
* Re: [PATCH v2 3/3] selftests/lkdtm: Enable selftest for SLB multihit
From: Ganesh @ 2020-09-28 17:33 UTC (permalink / raw)
To: Kees Cook; +Cc: msuchanek, linuxppc-dev, mahesh, npiggin
In-Reply-To: <202009251257.52EAF495@keescook>
[-- Attachment #1: Type: text/plain, Size: 879 bytes --]
On 9/26/20 1:29 AM, Kees Cook wrote:
> On Fri, Sep 25, 2020 at 04:01:23PM +0530, Ganesh Goudar wrote:
>> Add PPC_SLB_MULTIHIT to lkdtm selftest framework.
>>
>> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
>> ---
>> tools/testing/selftests/lkdtm/tests.txt | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
>> index 9d266e79c6a2..7eb3cf91c89e 100644
>> --- a/tools/testing/selftests/lkdtm/tests.txt
>> +++ b/tools/testing/selftests/lkdtm/tests.txt
>> @@ -70,3 +70,4 @@ USERCOPY_KERNEL
>> USERCOPY_KERNEL_DS
>> STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
>> CFI_FORWARD_PROTO
>> +PPC_SLB_MULTIHIT Recovered
> Please squash this into the lkdtm patch -- I'd like test implementation
> and kselftest awareness to go in together.
Sure, Thanks.
>
[-- Attachment #2: Type: text/html, Size: 1532 bytes --]
^ permalink raw reply
* Re: [PATCH] rpadlpar_io:Add MODULE_DESCRIPTION entries to kernel modules
From: Tyrel Datwyler @ 2020-09-28 20:50 UTC (permalink / raw)
To: Oliver O'Halloran, Mamatha Inamdar
Cc: Bjorn Helgaas, linuxppc-dev, Linux Kernel Mailing List, linux-pci
In-Reply-To: <CAOSf1CEv3v940FR_we70qCBME0qFXPizPT8EFbf3XyK2-fPDrw@mail.gmail.com>
On 9/23/20 11:41 PM, Oliver O'Halloran wrote:
> On Thu, Sep 24, 2020 at 3:15 PM Mamatha Inamdar
> <mamatha4@linux.vnet.ibm.com> wrote:
>>
>> This patch adds a brief MODULE_DESCRIPTION to rpadlpar_io kernel modules
>> (descriptions taken from Kconfig file)
>>
>> Signed-off-by: Mamatha Inamdar <mamatha4@linux.vnet.ibm.com>
>> ---
>> drivers/pci/hotplug/rpadlpar_core.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/pci/hotplug/rpadlpar_core.c b/drivers/pci/hotplug/rpadlpar_core.c
>> index f979b70..bac65ed 100644
>> --- a/drivers/pci/hotplug/rpadlpar_core.c
>> +++ b/drivers/pci/hotplug/rpadlpar_core.c
>> @@ -478,3 +478,4 @@ static void __exit rpadlpar_io_exit(void)
>> module_init(rpadlpar_io_init);
>> module_exit(rpadlpar_io_exit);
>> MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("RPA Dynamic Logical Partitioning driver for I/O slots");
>
> RPA as a spec was superseded by PAPR in the early 2000s. Can we rename
> this already?
I seem to recall Michael and I discussed the naming briefly when I added the
maintainer entries for the drivers and that the PAPR acronym is almost as
meaningless to most as the original RPA. While, IBM no longer uses the term
pseries for Power hardware marketing it is the defacto platform identifier in
the Linux kernel tree for what we would call PAPR compliant. All in all I have
no problem with renaming, but maybe we should consider pseries_dlpar or even
simpler ibmdlpar.
>
> The only potential problem I can see is scripts doing: modprobe
> rpadlpar_io or similar
>
> However, we should be able to fix that with a module alias.
Agreed.
-Tyrel
>
> Oliver
>
^ permalink raw reply
* RE: [PATCH 1/5] Documentation: dt: binding: fsl: Add 'fsl,ippdexpcr1-alt-addr' property
From: Ran Wang @ 2020-09-29 0:34 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree@vger.kernel.org, Biwen Li, Shawn Guo,
linux-kernel@vger.kernel.org, Leo Li,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAL_Jsq+uzkr7CcvwQTe5vhpMPtdqL9v4EeqH5yZjMoT=JrDtDQ@mail.gmail.com>
Hi Rob,
On Monday, September 28, 2020 9:57 PM, Rob Herring wrote:
>
> On Wed, Sep 23, 2020 at 1:44 AM Ran Wang <ran.wang_1@nxp.com> wrote:
> >
> > Hi Rob,
> >
> > On Wednesday, September 23, 2020 10:33 AM, Rob Herring wrote:
> > >
> > > On Wed, Sep 16, 2020 at 04:18:27PM +0800, Ran Wang wrote:
> > > > From: Biwen Li <biwen.li@nxp.com>
> > > >
> > > > The 'fsl,ippdexpcr1-alt-addr' property is used to handle an errata
> > > > A-008646 on LS1021A
> > > >
> > > > Signed-off-by: Biwen Li <biwen.li@nxp.com>
> > > > Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
> > > > ---
> > > > Documentation/devicetree/bindings/soc/fsl/rcpm.txt | 19
> > > > +++++++++++++++++++
> > > > 1 file changed, 19 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> > > > b/Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> > > > index 5a33619..1be58a3 100644
> > > > --- a/Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> > > > +++ b/Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> > > > @@ -34,6 +34,11 @@ Chassis Version Example Chips
> > > > Optional properties:
> > > > - little-endian : RCPM register block is Little Endian. Without it RCPM
> > > > will be Big Endian (default case).
> > > > + - fsl,ippdexpcr1-alt-addr : The property is related to a hardware issue
> > > > + on SoC LS1021A and only needed on SoC LS1021A.
> > > > + Must include 2 entries:
> > > > + The first entry must be a link to the SCFG device node.
> > > > + The 2nd entry must be offset of register IPPDEXPCR1 in SCFG.
> > >
> > > You don't need a DT change for this. You can find SCFG node by its
> > > compatible string and then the offset should be known given this issue is
> only on 1 SoC.
> >
> > Did you mean that RCPM driver just to access IPPDEXPCR1 shadowed
> > register in SCFG directly without fetching it's offset info. from DT?
>
> Yes. There's only 1 possible value of the offset because there's only one SoC, so
> the driver can hardcode the offset. And I assume there's only one SCFG node,
> so you can find it by its compatible string (of_find_compatible_node).
Got it, let me update this in next version, thank you.
Regards,
Ran
> Rob
^ permalink raw reply
* Re: [PATCH] rpadlpar_io:Add MODULE_DESCRIPTION entries to kernel modules
From: Oliver O'Halloran @ 2020-09-29 0:55 UTC (permalink / raw)
To: Tyrel Datwyler
Cc: Linux Kernel Mailing List, Mamatha Inamdar, linux-pci,
Bjorn Helgaas, linuxppc-dev
In-Reply-To: <ff6a8c97-4a6a-c82b-bd35-e09fa44f8e20@linux.ibm.com>
On Tue, Sep 29, 2020 at 6:50 AM Tyrel Datwyler <tyreld@linux.ibm.com> wrote:
>
> On 9/23/20 11:41 PM, Oliver O'Halloran wrote:
> > On Thu, Sep 24, 2020 at 3:15 PM Mamatha Inamdar
> > <mamatha4@linux.vnet.ibm.com> wrote:
> >>
> >> This patch adds a brief MODULE_DESCRIPTION to rpadlpar_io kernel modules
> >> (descriptions taken from Kconfig file)
> >>
> >> Signed-off-by: Mamatha Inamdar <mamatha4@linux.vnet.ibm.com>
> >> ---
> >> drivers/pci/hotplug/rpadlpar_core.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/pci/hotplug/rpadlpar_core.c b/drivers/pci/hotplug/rpadlpar_core.c
> >> index f979b70..bac65ed 100644
> >> --- a/drivers/pci/hotplug/rpadlpar_core.c
> >> +++ b/drivers/pci/hotplug/rpadlpar_core.c
> >> @@ -478,3 +478,4 @@ static void __exit rpadlpar_io_exit(void)
> >> module_init(rpadlpar_io_init);
> >> module_exit(rpadlpar_io_exit);
> >> MODULE_LICENSE("GPL");
> >> +MODULE_DESCRIPTION("RPA Dynamic Logical Partitioning driver for I/O slots");
> >
> > RPA as a spec was superseded by PAPR in the early 2000s. Can we rename
> > this already?
>
> I seem to recall Michael and I discussed the naming briefly when I added the
> maintainer entries for the drivers and that the PAPR acronym is almost as
> meaningless to most as the original RPA. While, IBM no longer uses the term
> pseries for Power hardware marketing it is the defacto platform identifier in
> the Linux kernel tree for what we would call PAPR compliant. All in all I have
> no problem with renaming, but maybe we should consider pseries_dlpar or even
> simpler ibmdlpar.
I'm not too bothered by what we call it so long as it's consistent
with *something* else in the tree. Using pseries rather than ibm as a
prefix would probably be better since the legacy ibmphp driver is in
the same directory.
^ permalink raw reply
* Re: [PATCH v2 25/25] powerpc/signal32: Transform save_user_regs() and save_tm_user_regs() in 'unsafe' version
From: Christopher M. Riedl @ 2020-09-29 2:55 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <c7b37b385ccf9666066452e58f018a86573f83e8.1597770847.git.christophe.leroy@csgroup.eu>
On Tue Aug 18, 2020 at 12:19 PM CDT, Christophe Leroy wrote:
> Change those two functions to be used within a user access block.
>
> For that, change save_general_regs() to and unsafe_save_general_regs(),
> then replace all user accesses by unsafe_ versions.
>
> This series leads to a reduction from 2.55s to 1.73s of
> the system CPU time with the following microbench app
> on an mpc832x with KUAP (approx 32%)
>
> Without KUAP, the difference is in the noise.
>
> void sigusr1(int sig) { }
>
> int main(int argc, char **argv)
> {
> int i = 100000;
>
> signal(SIGUSR1, sigusr1);
> for (;i--;)
> raise(SIGUSR1);
> exit(0);
> }
>
> An additional 0.10s reduction is achieved by removing
> CONFIG_PPC_FPU, as the mpc832x has no FPU.
>
> A bit less spectacular on an 8xx as KUAP is less heavy, prior to
> the series (with KUAP) it ran in 8.10 ms. Once applies the removal
> of FPU regs handling, we get 7.05s. With the full series, we get 6.9s.
> If artificially re-activating FPU regs handling with the full series,
> we get 7.6s.
>
> So for the 8xx, the removal of the FPU regs copy is what makes the
> difference, but the rework of handle_signal also have a benefit.
>
> Same as above, without KUAP the difference is in the noise.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/kernel/signal_32.c | 224 ++++++++++++++++----------------
> 1 file changed, 111 insertions(+), 113 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_32.c
> b/arch/powerpc/kernel/signal_32.c
> index 86539a4e0514..f795fe0240a1 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -93,8 +93,8 @@ static inline int get_sigset_t(sigset_t *set,
> #define to_user_ptr(p) ptr_to_compat(p)
> #define from_user_ptr(p) compat_ptr(p)
>
> -static inline int save_general_regs(struct pt_regs *regs,
> - struct mcontext __user *frame)
> +static __always_inline int
> +save_general_regs_unsafe(struct pt_regs *regs, struct mcontext __user
> *frame)
> {
> elf_greg_t64 *gregs = (elf_greg_t64 *)regs;
> int val, i;
> @@ -108,10 +108,12 @@ static inline int save_general_regs(struct pt_regs
> *regs,
> else
> val = gregs[i];
>
> - if (__put_user(val, &frame->mc_gregs[i]))
> - return -EFAULT;
> + unsafe_put_user(val, &frame->mc_gregs[i], failed);
> }
> return 0;
> +
> +failed:
> + return 1;
> }
>
> static inline int restore_general_regs(struct pt_regs *regs,
> @@ -148,11 +150,15 @@ static inline int get_sigset_t(sigset_t *set,
> const sigset_t __user *uset)
> #define to_user_ptr(p) ((unsigned long)(p))
> #define from_user_ptr(p) ((void __user *)(p))
>
> -static inline int save_general_regs(struct pt_regs *regs,
> - struct mcontext __user *frame)
> +static __always_inline int
> +save_general_regs_unsafe(struct pt_regs *regs, struct mcontext __user
> *frame)
> {
> WARN_ON(!FULL_REGS(regs));
> - return __copy_to_user(&frame->mc_gregs, regs, GP_REGS_SIZE);
> + unsafe_copy_to_user(&frame->mc_gregs, regs, GP_REGS_SIZE, failed);
> + return 0;
> +
> +failed:
> + return 1;
> }
>
> static inline int restore_general_regs(struct pt_regs *regs,
> @@ -170,6 +176,11 @@ static inline int restore_general_regs(struct
> pt_regs *regs,
> }
> #endif
>
> +#define unsafe_save_general_regs(regs, frame, label) do { \
> + if (save_general_regs_unsafe(regs, frame)) \
Minor nitpick (sorry); this naming seems a bit strange to me, in x86 it
is "__unsafe_" as a prefix instead of "_unsafe" as a suffix. That sounds
a bit better to me, what do you think? Unless there is some convention I
am not aware of here apart from "unsafe_" using a goto label for errors.
> + goto label; \
> +} while (0)
> +
> /*
> * When we have signals to deliver, we set up on the
> * user stack, going down from the original stack pointer:
> @@ -249,21 +260,19 @@ static void prepare_save_user_regs(int
> ctx_has_vsx_region)
> #endif
> }
>
> -static int save_user_regs(struct pt_regs *regs, struct mcontext __user
> *frame,
> - struct mcontext __user *tm_frame, int ctx_has_vsx_region)
> +static int save_user_regs_unsafe(struct pt_regs *regs, struct mcontext
> __user *frame,
> + struct mcontext __user *tm_frame, int ctx_has_vsx_region)
> {
> unsigned long msr = regs->msr;
>
> /* save general registers */
> - if (save_general_regs(regs, frame))
> - return 1;
> + unsafe_save_general_regs(regs, frame, failed);
>
> #ifdef CONFIG_ALTIVEC
> /* save altivec registers */
> if (current->thread.used_vr) {
> - if (__copy_to_user(&frame->mc_vregs, ¤t->thread.vr_state,
> - ELF_NVRREG * sizeof(vector128)))
> - return 1;
> + unsafe_copy_to_user(&frame->mc_vregs, ¤t->thread.vr_state,
> + ELF_NVRREG * sizeof(vector128), failed);
> /* set MSR_VEC in the saved MSR value to indicate that
> frame->mc_vregs contains valid data */
> msr |= MSR_VEC;
> @@ -276,11 +285,10 @@ static int save_user_regs(struct pt_regs *regs,
> struct mcontext __user *frame,
> * most significant bits of that same vector. --BenH
> * Note that the current VRSAVE value is in the SPR at this point.
> */
> - if (__put_user(current->thread.vrsave, (u32 __user
> *)&frame->mc_vregs[32]))
> - return 1;
> + unsafe_put_user(current->thread.vrsave, (u32 __user
> *)&frame->mc_vregs[32],
> + failed);
> #endif /* CONFIG_ALTIVEC */
> - if (copy_fpr_to_user(&frame->mc_fregs, current))
> - return 1;
> + unsafe_copy_fpr_to_user(&frame->mc_fregs, current, failed);
>
> /*
> * Clear the MSR VSX bit to indicate there is no valid state attached
> @@ -295,17 +303,15 @@ static int save_user_regs(struct pt_regs *regs,
> struct mcontext __user *frame,
> * contains valid data
> */
> if (current->thread.used_vsr && ctx_has_vsx_region) {
> - if (copy_vsx_to_user(&frame->mc_vsregs, current))
> - return 1;
> + unsafe_copy_vsx_to_user(&frame->mc_vsregs, current, failed);
> msr |= MSR_VSX;
> }
> #endif /* CONFIG_VSX */
> #ifdef CONFIG_SPE
> /* save spe registers */
> if (current->thread.used_spe) {
> - if (__copy_to_user(&frame->mc_vregs, current->thread.evr,
> - ELF_NEVRREG * sizeof(u32)))
> - return 1;
> + unsafe_copy_to_user(&frame->mc_vregs, current->thread.evr,
> + ELF_NEVRREG * sizeof(u32)), failed);
> /* set MSR_SPE in the saved MSR value to indicate that
> frame->mc_vregs contains valid data */
> msr |= MSR_SPE;
> @@ -313,21 +319,29 @@ static int save_user_regs(struct pt_regs *regs,
> struct mcontext __user *frame,
> /* else assert((regs->msr & MSR_SPE) == 0) */
>
> /* We always copy to/from spefscr */
> - if (__put_user(current->thread.spefscr, (u32 __user *)&frame->mc_vregs
> + ELF_NEVRREG))
> - return 1;
> + unsafe_put_user(current->thread.spefscr,
> + (u32 __user *)&frame->mc_vregs + ELF_NEVRREG, failed);
> #endif /* CONFIG_SPE */
>
> - if (__put_user(msr, &frame->mc_gregs[PT_MSR]))
> - return 1;
> + unsafe_put_user(msr, &frame->mc_gregs[PT_MSR], failed);
> +
> /* We need to write 0 the MSR top 32 bits in the tm frame so that we
> * can check it on the restore to see if TM is active
> */
> - if (tm_frame && __put_user(0, &tm_frame->mc_gregs[PT_MSR]))
> - return 1;
> + if (tm_frame)
> + unsafe_put_user(0, &tm_frame->mc_gregs[PT_MSR], failed);
>
> return 0;
> +
> +failed:
> + return 1;
> }
>
> +#define unsafe_save_user_regs(regs, frame, tm_frame, has_vsx, label) do
> { \
> + if (save_user_regs_unsafe(regs, frame, tm_frame, has_vsx)) \
> + goto label; \
> +} while (0)
> +
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> /*
> * Save the current user registers on the user stack.
> @@ -336,7 +350,7 @@ static int save_user_regs(struct pt_regs *regs,
> struct mcontext __user *frame,
> * We also save the transactional registers to a second ucontext in the
> * frame.
> *
> - * See save_user_regs() and signal_64.c:setup_tm_sigcontexts().
> + * See save_user_regs_unsafe() and signal_64.c:setup_tm_sigcontexts().
> */
> static void prepare_save_tm_user_regs(void)
> {
> @@ -352,13 +366,12 @@ static void prepare_save_tm_user_regs(void)
> #endif
> }
>
> -static int save_tm_user_regs(struct pt_regs *regs, struct mcontext
> __user *frame,
> - struct mcontext __user *tm_frame, unsigned long msr)
> +static int save_tm_user_regs_unsafe(struct pt_regs *regs, struct
> mcontext __user *frame,
> + struct mcontext __user *tm_frame, unsigned long msr)
> {
> /* Save both sets of general registers */
> - if (save_general_regs(¤t->thread.ckpt_regs, frame)
> - || save_general_regs(regs, tm_frame))
> - return 1;
> + unsafe_save_general_regs(¤t->thread.ckpt_regs, frame, failed);
> + unsafe_save_general_regs(regs, tm_frame, failed);
>
> /* Stash the top half of the 64bit MSR into the 32bit MSR word
> * of the transactional mcontext. This way we have a backward-compatible
> @@ -366,26 +379,21 @@ static int save_tm_user_regs(struct pt_regs *regs,
> struct mcontext __user *frame
> * also look at what type of transaction (T or S) was active at the
> * time of the signal.
> */
> - if (__put_user((msr >> 32), &tm_frame->mc_gregs[PT_MSR]))
> - return 1;
> + unsafe_put_user((msr >> 32), &tm_frame->mc_gregs[PT_MSR], failed);
>
> #ifdef CONFIG_ALTIVEC
> /* save altivec registers */
> if (current->thread.used_vr) {
> - if (__copy_to_user(&frame->mc_vregs, ¤t->thread.ckvr_state,
> - ELF_NVRREG * sizeof(vector128)))
> - return 1;
> - if (msr & MSR_VEC) {
> - if (__copy_to_user(&tm_frame->mc_vregs,
> - ¤t->thread.vr_state,
> - ELF_NVRREG * sizeof(vector128)))
> - return 1;
> - } else {
> - if (__copy_to_user(&tm_frame->mc_vregs,
> - ¤t->thread.ckvr_state,
> - ELF_NVRREG * sizeof(vector128)))
> - return 1;
> - }
> + unsafe_copy_to_user(&frame->mc_vregs, ¤t->thread.ckvr_state,
> + ELF_NVRREG * sizeof(vector128), failed);
> + if (msr & MSR_VEC)
> + unsafe_copy_to_user(&tm_frame->mc_vregs,
> + ¤t->thread.vr_state,
> + ELF_NVRREG * sizeof(vector128), failed);
> + else
> + unsafe_copy_to_user(&tm_frame->mc_vregs,
> + ¤t->thread.ckvr_state,
> + ELF_NVRREG * sizeof(vector128), failed);
>
> /* set MSR_VEC in the saved MSR value to indicate that
> * frame->mc_vregs contains valid data
> @@ -398,29 +406,21 @@ static int save_tm_user_regs(struct pt_regs *regs,
> struct mcontext __user *frame
> * significant bits of a vector, we "cheat" and stuff VRSAVE in the
> * most significant bits of that same vector. --BenH
> */
> - if (__put_user(current->thread.ckvrsave,
> - (u32 __user *)&frame->mc_vregs[32]))
> - return 1;
> - if (msr & MSR_VEC) {
> - if (__put_user(current->thread.vrsave,
> - (u32 __user *)&tm_frame->mc_vregs[32]))
> - return 1;
> - } else {
> - if (__put_user(current->thread.ckvrsave,
> - (u32 __user *)&tm_frame->mc_vregs[32]))
> - return 1;
> - }
> + unsafe_put_user(current->thread.ckvrsave,
> + (u32 __user *)&frame->mc_vregs[32], failed);
> + if (msr & MSR_VEC)
> + unsafe_put_user(current->thread.vrsave,
> + (u32 __user *)&tm_frame->mc_vregs[32], failed);
> + else
> + unsafe_put_user(current->thread.ckvrsave,
> + (u32 __user *)&tm_frame->mc_vregs[32], failed);
> #endif /* CONFIG_ALTIVEC */
>
> - if (copy_ckfpr_to_user(&frame->mc_fregs, current))
> - return 1;
> - if (msr & MSR_FP) {
> - if (copy_fpr_to_user(&tm_frame->mc_fregs, current))
> - return 1;
> - } else {
> - if (copy_ckfpr_to_user(&tm_frame->mc_fregs, current))
> - return 1;
> - }
> + unsafe_copy_ckfpr_to_user(&frame->mc_fregs, current, failed);
> + if (msr & MSR_FP)
> + unsafe_copy_fpr_to_user(&tm_frame->mc_fregs, current, failed);
> + else
> + unsafe_copy_ckfpr_to_user(&tm_frame->mc_fregs, current, failed);
>
> #ifdef CONFIG_VSX
> /*
> @@ -430,53 +430,54 @@ static int save_tm_user_regs(struct pt_regs *regs,
> struct mcontext __user *frame
> * contains valid data
> */
> if (current->thread.used_vsr) {
> - if (copy_ckvsx_to_user(&frame->mc_vsregs, current))
> - return 1;
> - if (msr & MSR_VSX) {
> - if (copy_vsx_to_user(&tm_frame->mc_vsregs,
> - current))
> - return 1;
> - } else {
> - if (copy_ckvsx_to_user(&tm_frame->mc_vsregs, current))
> - return 1;
> - }
> + unsafe_copy_ckvsx_to_user(&frame->mc_vsregs, current, failed);
> + if (msr & MSR_VSX)
> + unsafe_copy_vsx_to_user(&tm_frame->mc_vsregs, current, failed);
> + else
> + unsafe_copy_ckvsx_to_user(&tm_frame->mc_vsregs, current, failed);
>
> msr |= MSR_VSX;
> }
> #endif /* CONFIG_VSX */
> #ifdef CONFIG_SPE
> /* SPE regs are not checkpointed with TM, so this section is
> - * simply the same as in save_user_regs().
> + * simply the same as in save_user_regs_unsafe().
> */
> if (current->thread.used_spe) {
> - if (__copy_to_user(&frame->mc_vregs, current->thread.evr,
> - ELF_NEVRREG * sizeof(u32)))
> - return 1;
> + unsafe_copy_to_user(&frame->mc_vregs, current->thread.evr,
> + ELF_NEVRREG * sizeof(u32), failed);
> /* set MSR_SPE in the saved MSR value to indicate that
> * frame->mc_vregs contains valid data */
> msr |= MSR_SPE;
> }
>
> /* We always copy to/from spefscr */
> - if (__put_user(current->thread.spefscr, (u32 __user *)&frame->mc_vregs
> + ELF_NEVRREG))
> - return 1;
> + unsafe_put_user(current->thread.spefscr,
> + (u32 __user *)&frame->mc_vregs + ELF_NEVRREG, failed);
> #endif /* CONFIG_SPE */
>
> - if (__put_user(msr, &frame->mc_gregs[PT_MSR]))
> - return 1;
> + unsafe_put_user(msr, &frame->mc_gregs[PT_MSR], failed);
>
> return 0;
> +
> +failed:
> + return 1;
> }
> #else
> static void prepare_save_tm_user_regs(void) { }
>
> -static int save_tm_user_regs(struct pt_regs *regs, struct mcontext
> __user *frame,
> - struct mcontext __user *tm_frame, unsigned long msr)
> +static int save_tm_user_regs_unsafe(struct pt_regs *regs, struct
> mcontext __user *frame,
> + struct mcontext __user *tm_frame, unsigned long msr)
> {
> return 0;
> }
> #endif
>
> +#define unsafe_save_tm_user_regs(regs, frame, tm_frame, msr, label) do
> { \
> + if (save_tm_user_regs_unsafe(regs, frame, tm_frame, msr)) \
> + goto label; \
> +} while (0)
> +
> /*
> * Restore the current user register values from the user stack,
> * (except for MSR).
> @@ -769,6 +770,11 @@ int handle_rt_signal32(struct ksignal *ksig,
> sigset_t *oldset,
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> tm_mctx = &frame->uc_transact.uc_mcontext;
> #endif
> + if (MSR_TM_ACTIVE(msr))
> + prepare_save_tm_user_regs();
> + else
> + prepare_save_user_regs(1);
> +
> if (!user_write_access_begin(frame, sizeof(*frame)))
> goto badframe;
>
> @@ -788,8 +794,10 @@ int handle_rt_signal32(struct ksignal *ksig,
> sigset_t *oldset,
> unsafe_put_user((unsigned long)tm_mctx,
> &frame->uc_transact.uc_regs, failed);
> #endif
> + unsafe_save_tm_user_regs(regs, mctx, tm_mctx, msr, failed);
> } else {
> unsafe_put_user(0, &frame->uc.uc_link, failed);
> + unsafe_save_user_regs(regs, mctx, tm_mctx, 1, failed);
> }
>
> /* Save user registers on the stack */
> @@ -812,15 +820,6 @@ int handle_rt_signal32(struct ksignal *ksig,
> sigset_t *oldset,
> if (tramp == (unsigned long)mctx->mc_pad)
> flush_icache_range(tramp, tramp + 2 * sizeof(unsigned long));
>
> - if (MSR_TM_ACTIVE(msr)) {
> - prepare_save_tm_user_regs();
> - if (save_tm_user_regs(regs, mctx, tm_mctx, msr))
> - goto badframe;
> - } else {
> - prepare_save_user_regs(1);
> - if (save_user_regs(regs, mctx, tm_mctx, 1))
> - goto badframe;
> - }
> regs->link = tramp;
>
> #ifdef CONFIG_PPC_FPU_REGS
> @@ -875,6 +874,11 @@ int handle_signal32(struct ksignal *ksig, sigset_t
> *oldset,
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> tm_mctx = &frame->mctx_transact;
> #endif
> + if (MSR_TM_ACTIVE(msr))
> + prepare_save_tm_user_regs();
> + else
> + prepare_save_user_regs(1);
> +
> if (!user_write_access_begin(frame, sizeof(*frame)))
> goto badframe;
> sc = (struct sigcontext __user *) &frame->sctx;
> @@ -892,6 +896,11 @@ int handle_signal32(struct ksignal *ksig, sigset_t
> *oldset,
> unsafe_put_user(to_user_ptr(mctx), &sc->regs, failed);
> unsafe_put_user(ksig->sig, &sc->signal, failed);
>
> + if (MSR_TM_ACTIVE(msr))
> + unsafe_save_tm_user_regs(regs, mctx, tm_mctx, msr, failed);
> + else
> + unsafe_save_user_regs(regs, mctx, tm_mctx, 1, failed);
> +
> if (vdso32_sigtramp && tsk->mm->context.vdso_base) {
> tramp = tsk->mm->context.vdso_base + vdso32_sigtramp;
> } else {
> @@ -905,16 +914,6 @@ int handle_signal32(struct ksignal *ksig, sigset_t
> *oldset,
> if (tramp == (unsigned long)mctx->mc_pad)
> flush_icache_range(tramp, tramp + 2 * sizeof(unsigned long));
>
> - if (MSR_TM_ACTIVE(msr)) {
> - prepare_save_tm_user_regs();
> - if (save_tm_user_regs(regs, mctx, tm_mctx, msr))
> - goto badframe;
> - } else {
> - prepare_save_user_regs(1);
> - if (save_user_regs(regs, mctx, tm_mctx, 1))
> - goto badframe;
> - }
> -
> regs->link = tramp;
>
> #ifdef CONFIG_PPC_FPU_REGS
> @@ -1066,10 +1065,9 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext
> __user *, old_ctx,
> mctx = (struct mcontext __user *)
> ((unsigned long) &old_ctx->uc_mcontext & ~0xfUL);
> prepare_save_user_regs(ctx_has_vsx_region);
> - if (save_user_regs(regs, mctx, NULL, ctx_has_vsx_region))
> - return -EFAULT;
> if (!user_write_access_begin(old_ctx, ctx_size))
> return -EFAULT;
> + unsafe_save_user_regs(regs, mctx, NULL, ctx_has_vsx_region, failed);
> unsafe_put_sigset_t(&old_ctx->uc_sigmask, ¤t->blocked, failed);
> unsafe_put_user(to_user_ptr(mctx), &old_ctx->uc_regs, failed);
> user_write_access_end();
> --
> 2.25.0
^ permalink raw reply
* Re: [PATCH v2 23/25] powerpc/signal: Create 'unsafe' versions of copy_[ck][fpr/vsx]_to_user()
From: Christopher M. Riedl @ 2020-09-29 2:04 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <29f6c4b8e7a5bbc61e6a8801b78bbf493f9f819e.1597770847.git.christophe.leroy@csgroup.eu>
On Tue Aug 18, 2020 at 12:19 PM CDT, Christophe Leroy wrote:
> For the non VSX version, that's trivial. Just use unsafe_copy_to_user()
> instead of __copy_to_user().
>
> For the VSX version, remove the intermediate step through a buffer and
> use unsafe_put_user() directly. This generates a far smaller code which
> is acceptable to inline, see below:
>
> Standard VSX version:
>
> 0000000000000000 <.copy_fpr_to_user>:
> 0: 7c 08 02 a6 mflr r0
> 4: fb e1 ff f8 std r31,-8(r1)
> 8: 39 00 00 20 li r8,32
> c: 39 24 0b 80 addi r9,r4,2944
> 10: 7d 09 03 a6 mtctr r8
> 14: f8 01 00 10 std r0,16(r1)
> 18: f8 21 fe 71 stdu r1,-400(r1)
> 1c: 39 41 00 68 addi r10,r1,104
> 20: e9 09 00 00 ld r8,0(r9)
> 24: 39 4a 00 08 addi r10,r10,8
> 28: 39 29 00 10 addi r9,r9,16
> 2c: f9 0a 00 00 std r8,0(r10)
> 30: 42 00 ff f0 bdnz 20 <.copy_fpr_to_user+0x20>
> 34: e9 24 0d 80 ld r9,3456(r4)
> 38: 3d 42 00 00 addis r10,r2,0
> 3a: R_PPC64_TOC16_HA .toc
> 3c: eb ea 00 00 ld r31,0(r10)
> 3e: R_PPC64_TOC16_LO_DS .toc
> 40: f9 21 01 70 std r9,368(r1)
> 44: e9 3f 00 00 ld r9,0(r31)
> 48: 81 29 00 20 lwz r9,32(r9)
> 4c: 2f 89 00 00 cmpwi cr7,r9,0
> 50: 40 9c 00 18 bge cr7,68 <.copy_fpr_to_user+0x68>
> 54: 4c 00 01 2c isync
> 58: 3d 20 40 00 lis r9,16384
> 5c: 79 29 07 c6 rldicr r9,r9,32,31
> 60: 7d 3d 03 a6 mtspr 29,r9
> 64: 4c 00 01 2c isync
> 68: 38 a0 01 08 li r5,264
> 6c: 38 81 00 70 addi r4,r1,112
> 70: 48 00 00 01 bl 70 <.copy_fpr_to_user+0x70>
> 70: R_PPC64_REL24 .__copy_tofrom_user
> 74: 60 00 00 00 nop
> 78: e9 3f 00 00 ld r9,0(r31)
> 7c: 81 29 00 20 lwz r9,32(r9)
> 80: 2f 89 00 00 cmpwi cr7,r9,0
> 84: 40 9c 00 18 bge cr7,9c <.copy_fpr_to_user+0x9c>
> 88: 4c 00 01 2c isync
> 8c: 39 20 ff ff li r9,-1
> 90: 79 29 00 44 rldicr r9,r9,0,1
> 94: 7d 3d 03 a6 mtspr 29,r9
> 98: 4c 00 01 2c isync
> 9c: 38 21 01 90 addi r1,r1,400
> a0: e8 01 00 10 ld r0,16(r1)
> a4: eb e1 ff f8 ld r31,-8(r1)
> a8: 7c 08 03 a6 mtlr r0
> ac: 4e 80 00 20 blr
>
> 'unsafe' simulated VSX version (The ... are only nops) using
> unsafe_copy_fpr_to_user() macro:
>
> unsigned long copy_fpr_to_user(void __user *to,
> struct task_struct *task)
> {
> unsafe_copy_fpr_to_user(to, task, failed);
> return 0;
> failed:
> return 1;
> }
>
> 0000000000000000 <.copy_fpr_to_user>:
> 0: 39 00 00 20 li r8,32
> 4: 39 44 0b 80 addi r10,r4,2944
> 8: 7d 09 03 a6 mtctr r8
> c: 7c 69 1b 78 mr r9,r3
> ...
> 20: e9 0a 00 00 ld r8,0(r10)
> 24: f9 09 00 00 std r8,0(r9)
> 28: 39 4a 00 10 addi r10,r10,16
> 2c: 39 29 00 08 addi r9,r9,8
> 30: 42 00 ff f0 bdnz 20 <.copy_fpr_to_user+0x20>
> 34: e9 24 0d 80 ld r9,3456(r4)
> 38: f9 23 01 00 std r9,256(r3)
> 3c: 38 60 00 00 li r3,0
> 40: 4e 80 00 20 blr
> ...
> 50: 38 60 00 01 li r3,1
> 54: 4e 80 00 20 blr
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/kernel/signal.h | 53 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
>
> diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
> index f610cfafa478..2559a681536e 100644
> --- a/arch/powerpc/kernel/signal.h
> +++ b/arch/powerpc/kernel/signal.h
> @@ -32,7 +32,54 @@ unsigned long copy_fpr_to_user(void __user *to,
> struct task_struct *task);
> unsigned long copy_ckfpr_to_user(void __user *to, struct task_struct
> *task);
> unsigned long copy_fpr_from_user(struct task_struct *task, void __user
> *from);
> unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user
> *from);
> +
> +#define unsafe_copy_fpr_to_user(to, task, label) do { \
> + struct task_struct *__t = task; \
> + u64 __user *buf = (u64 __user *)to; \
> + int i; \
> + \
> + for (i = 0; i < ELF_NFPREG - 1 ; i++) \
> + unsafe_put_user(__t->thread.TS_FPR(i), &buf[i], label); \
> + unsafe_put_user(__t->thread.fp_state.fpscr, &buf[i], label); \
> +} while (0)
> +
I've been working on the PPC64 side of this "unsafe" rework using this
series as a basis. One question here - I don't really understand what
the benefit of re-implementing this logic in macros (similarly for the
other copy_* functions below) is?
I am considering a "__unsafe_copy_*" implementation in signal.c for
each (just the original implementation w/ using the "unsafe_" variants
of the uaccess stuff) which gets called by the "safe" functions w/ the
appropriate "user_*_access_begin/user_*_access_end". Something like
(pseudo-ish code):
/* signal.c */
unsigned long __unsafe_copy_fpr_to_user(...)
{
...
unsafe_copy_to_user(..., bad);
return 0;
bad:
return 1; /* -EFAULT? */
}
unsigned long copy_fpr_to_user(...)
{
unsigned long err;
if (!user_write_access_begin(...))
return 1; /* -EFAULT? */
err = __unsafe_copy_fpr_to_user(...);
user_write_access_end();
return err;
}
/* signal.h */
unsigned long __unsafe_copy_fpr_to_user(...);
#define unsafe_copy_fpr_to_user(..., label) \
unsafe_op_wrap(__unsafe_copy_fpr_to_user(...), label)
This way there is a single implementation for each copy routine "body".
The "unsafe" wrappers then just exist as simple macros similar to what
x86 does: "unsafe_" macro wraps "__unsafe" functions for the goto label.
Granted this does pollute "signal.h" w/ the "__unsafe_copy_*"
prototypes...
> +#define unsafe_copy_vsx_to_user(to, task, label) do { \
> + struct task_struct *__t = task; \
> + u64 __user *buf = (u64 __user *)to; \
> + int i; \
> + \
> + for (i = 0; i < ELF_NVSRHALFREG ; i++) \
> + unsafe_put_user(__t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET], \
> + &buf[i], label);\
> +} while (0)
> +
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +#define unsafe_copy_ckfpr_to_user(to, task, label) do { \
> + struct task_struct *__t = task; \
> + u64 __user *buf = (u64 __user *)to; \
> + int i; \
> + \
> + for (i = 0; i < ELF_NFPREG - 1 ; i++) \
> + unsafe_put_user(__t->thread.TS_CKFPR(i), &buf[i], label);\
> + unsafe_put_user(__t->thread.ckfp_state.fpscr, &buf[i], label); \
> +} while (0)
> +
> +#define unsafe_copy_ckvsx_to_user(to, task, label) do { \
> + struct task_struct *__t = task; \
> + u64 __user *buf = (u64 __user *)to; \
> + int i; \
> + \
> + for (i = 0; i < ELF_NVSRHALFREG ; i++) \
> + unsafe_put_user(__t->thread.ckfp_state.fpr[i][TS_VSRLOWOFFSET], \
> + &buf[i], label);\
> +} while (0)
> +#endif
> #elif defined(CONFIG_PPC_FPU_REGS)
> +
> +#define unsafe_copy_fpr_to_user(to, task, label) \
> + unsafe_copy_to_user(to, (task)->thread.fp_state.fpr, \
> + ELF_NFPREG * sizeof(double), label)
> +
> static inline unsigned long
> copy_fpr_to_user(void __user *to, struct task_struct *task)
> {
> @@ -48,6 +95,10 @@ copy_fpr_from_user(struct task_struct *task, void
> __user *from)
> }
>
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +#define unsafe_copy_ckfpr_to_user(to, task, label) \
> + unsafe_copy_to_user(to, (task)->thread.ckfp_state.fpr, \
> + ELF_NFPREG * sizeof(double), label)
> +
> inline unsigned long copy_ckfpr_to_user(void __user *to, struct
> task_struct *task)
> {
> return __copy_to_user(to, task->thread.ckfp_state.fpr,
> @@ -62,6 +113,8 @@ copy_ckfpr_from_user(struct task_struct *task, void
> __user *from)
> }
> #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
> #else
> +#define unsafe_copy_fpr_to_user(to, task, label) do { } while (0)
> +
> static inline unsigned long
> copy_fpr_to_user(void __user *to, struct task_struct *task)
> {
> --
> 2.25.0
^ permalink raw reply
* Re: [PATCH v2 14/14] powerpc/pseries/iommu: Rename "direct window" to "dma window"
From: Alexey Kardashevskiy @ 2020-09-29 3:55 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Joel Stanley, Christophe Leroy,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev
In-Reply-To: <20200911170738.82818-15-leobras.c@gmail.com>
On 12/09/2020 03:07, Leonardo Bras wrote:
> Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
>
> A previous change introduced the usage of DDW as a bigger indirect DMA
> mapping when the DDW available size does not map the whole partition.
>
> As most of the code that manipulates direct mappings was reused for
> indirect mappings, it's necessary to rename all names and debug/info
> messages to reflect that it can be used for both kinds of mapping.
>
> Also, defines DEFAULT_DMA_WIN as "ibm,dma-window" to document that
> it's the name of the default DMA window.
"ibm,dma-window" is so old so it does not need a macro (which btw would
be DMA_WIN_PROPNAME to match the other names) :)
> Those changes are not supposed to change how the code works in any
> way, just adjust naming.
I simply have this in my .vimrc for the cases like this one:
===
This should cause no behavioural change.
===
>
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> ---
> arch/powerpc/platforms/pseries/iommu.c | 102 +++++++++++++------------
> 1 file changed, 53 insertions(+), 49 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index c4de23080d1b..56638b7f07fc 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -349,7 +349,7 @@ struct dynamic_dma_window_prop {
> __be32 window_shift; /* ilog2(tce_window_size) */
> };
>
> -struct direct_window {
> +struct dma_win {
> struct device_node *device;
> const struct dynamic_dma_window_prop *prop;
> struct list_head list;
> @@ -369,13 +369,14 @@ struct ddw_create_response {
> u32 addr_lo;
> };
>
> -static LIST_HEAD(direct_window_list);
> +static LIST_HEAD(dma_win_list);
> /* prevents races between memory on/offline and window creation */
> -static DEFINE_SPINLOCK(direct_window_list_lock);
> +static DEFINE_SPINLOCK(dma_win_list_lock);
> /* protects initializing window twice for same device */
> -static DEFINE_MUTEX(direct_window_init_mutex);
> +static DEFINE_MUTEX(dma_win_init_mutex);
> #define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
> #define DMA64_PROPNAME "linux,dma64-ddr-window-info"
> +#define DEFAULT_DMA_WIN "ibm,dma-window"
>
> static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
> unsigned long num_pfn, const void *arg)
> @@ -706,15 +707,18 @@ static void pci_dma_bus_setup_pSeriesLP(struct pci_bus *bus)
> pr_debug("pci_dma_bus_setup_pSeriesLP: setting up bus %pOF\n",
> dn);
>
> - /* Find nearest ibm,dma-window, walking up the device tree */
> + /*
> + * Find nearest ibm,dma-window (default DMA window), walking up the
> + * device tree
> + */
> for (pdn = dn; pdn != NULL; pdn = pdn->parent) {
> - dma_window = of_get_property(pdn, "ibm,dma-window", NULL);
> + dma_window = of_get_property(pdn, DEFAULT_DMA_WIN, NULL);
> if (dma_window != NULL)
> break;
> }
>
> if (dma_window == NULL) {
> - pr_debug(" no ibm,dma-window property !\n");
> + pr_debug(" no %s property !\n", DEFAULT_DMA_WIN);
> return;
> }
>
> @@ -810,11 +814,11 @@ static void remove_dma_window(struct device_node *np, u32 *ddw_avail,
>
> ret = rtas_call(ddw_avail[DDW_REMOVE_PE_DMA_WIN], 1, 1, NULL, liobn);
> if (ret)
> - pr_warn("%pOF: failed to remove direct window: rtas returned "
> + pr_warn("%pOF: failed to remove dma window: rtas returned "
> "%d to ibm,remove-pe-dma-window(%x) %llx\n",
> np, ret, ddw_avail[DDW_REMOVE_PE_DMA_WIN], liobn);
> else
> - pr_debug("%pOF: successfully removed direct window: rtas returned "
> + pr_debug("%pOF: successfully removed dma window: rtas returned "
s/dma/DMA/ in all user visible strings.
> "%d to ibm,remove-pe-dma-window(%x) %llx\n",
> np, ret, ddw_avail[DDW_REMOVE_PE_DMA_WIN], liobn);
> }
> @@ -842,7 +846,7 @@ static int remove_ddw(struct device_node *np, bool remove_prop, const char *win_
>
> ret = of_remove_property(np, win);
> if (ret)
> - pr_warn("%pOF: failed to remove direct window property: %d\n",
> + pr_warn("%pOF: failed to remove dma window property: %d\n",
> np, ret);
> return 0;
> }
> @@ -886,34 +890,34 @@ static phys_addr_t ddw_memory_hotplug_max(void)
>
> static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr, bool *direct_mapping)
> {
> - struct direct_window *window;
> - const struct dynamic_dma_window_prop *direct64;
> + struct dma_win *window;
> + const struct dynamic_dma_window_prop *dma64;
> unsigned long window_size;
> bool found = false;
>
> - spin_lock(&direct_window_list_lock);
> + spin_lock(&dma_win_list_lock);
> /* check if we already created a window and dupe that config if so */
> - list_for_each_entry(window, &direct_window_list, list) {
> + list_for_each_entry(window, &dma_win_list, list) {
> if (window->device == pdn) {
> - direct64 = window->prop;
> - *dma_addr = be64_to_cpu(direct64->dma_base);
> + dma64 = window->prop;
> + *dma_addr = be64_to_cpu(dma64->dma_base);
>
> - window_size = (1UL << be32_to_cpu(direct64->window_shift));
> + window_size = (1UL << be32_to_cpu(dma64->window_shift));
> *direct_mapping = (window_size >= ddw_memory_hotplug_max());
>
> found = true;
> break;
> }
> }
> - spin_unlock(&direct_window_list_lock);
> + spin_unlock(&dma_win_list_lock);
>
> return found;
> }
>
> -static struct direct_window *ddw_list_new_entry(struct device_node *pdn,
> - const struct dynamic_dma_window_prop *dma64)
> +static struct dma_win *ddw_list_new_entry(struct device_node *pdn,
> + const struct dynamic_dma_window_prop *dma64)
> {
> - struct direct_window *window;
> + struct dma_win *window;
>
> window = kzalloc(sizeof(*window), GFP_KERNEL);
> if (!window)
> @@ -929,7 +933,7 @@ static void find_existing_ddw_windows_named(const char *name)
> {
> int len;
> struct device_node *pdn;
> - struct direct_window *window;
> + struct dma_win *window;
> const struct dynamic_dma_window_prop *dma64;
>
> for_each_node_with_property(pdn, name) {
> @@ -943,9 +947,9 @@ static void find_existing_ddw_windows_named(const char *name)
> if (!window)
> break;
>
> - spin_lock(&direct_window_list_lock);
> - list_add(&window->list, &direct_window_list);
> - spin_unlock(&direct_window_list_lock);
> + spin_lock(&dma_win_list_lock);
> + list_add(&window->list, &dma_win_list);
> + spin_unlock(&dma_win_list_lock);
> }
> }
>
> @@ -1179,7 +1183,7 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> u64 max_addr, win_addr;
> struct device_node *dn;
> u32 ddw_avail[DDW_APPLICABLE_SIZE];
> - struct direct_window *window;
> + struct dma_win *window;
> const char *win_name;
> struct property *win64 = NULL;
> struct failed_ddw_pdn *fpdn;
> @@ -1187,10 +1191,10 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> struct pci_dn *pci = PCI_DN(pdn);
> struct iommu_table *tbl = pci->table_group->tables[0];
>
> - mutex_lock(&direct_window_init_mutex);
> + mutex_lock(&dma_win_init_mutex);
>
> if (find_existing_ddw(pdn, &dev->dev.archdata.dma_offset, &direct_mapping)) {
> - mutex_unlock(&direct_window_init_mutex);
> + mutex_unlock(&dma_win_init_mutex);
> return direct_mapping;
> }
>
> @@ -1241,7 +1245,7 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> struct property *default_win;
> int reset_win_ext;
>
> - default_win = of_find_property(pdn, "ibm,dma-window", NULL);
> + default_win = of_find_property(pdn, DEFAULT_DMA_WIN, NULL);
> if (!default_win)
> goto out_failed;
>
> @@ -1272,8 +1276,8 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> } else if ((query.page_size & 1) && PAGE_SHIFT >= 12) {
> page_shift = 12; /* 4kB */
> } else {
> - dev_dbg(&dev->dev, "no supported direct page size in mask %x",
> - query.page_size);
> + dev_dbg(&dev->dev, "no supported page size in mask %x",
> + query.page_size);
> goto out_failed;
> }
>
> @@ -1331,7 +1335,7 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> ret = walk_system_ram_range(0, memblock_end_of_DRAM() >> PAGE_SHIFT,
> win64->value, tce_setrange_multi_pSeriesLP_walk);
> if (ret) {
> - dev_info(&dev->dev, "failed to map direct window for %pOF: %d\n",
> + dev_info(&dev->dev, "failed to map DMA window for %pOF: %d\n",
> dn, ret);
> goto out_list_del;
> }
> @@ -1354,9 +1358,9 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> set_iommu_table_base(&dev->dev, tbl);
> }
>
> - spin_lock(&direct_window_list_lock);
> - list_add(&window->list, &direct_window_list);
> - spin_unlock(&direct_window_list_lock);
> + spin_lock(&dma_win_list_lock);
> + list_add(&window->list, &dma_win_list);
> + spin_unlock(&dma_win_list_lock);
>
> dev->dev.archdata.dma_offset = win_addr;
> goto out_unlock;
> @@ -1387,7 +1391,7 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> list_add(&fpdn->list, &failed_ddw_pdn_list);
>
> out_unlock:
> - mutex_unlock(&direct_window_init_mutex);
> + mutex_unlock(&dma_win_init_mutex);
> return win64 && direct_mapping;
> }
>
> @@ -1411,7 +1415,7 @@ static void pci_dma_dev_setup_pSeriesLP(struct pci_dev *dev)
>
> for (pdn = dn; pdn && PCI_DN(pdn) && !PCI_DN(pdn)->table_group;
> pdn = pdn->parent) {
> - dma_window = of_get_property(pdn, "ibm,dma-window", NULL);
> + dma_window = of_get_property(pdn, DEFAULT_DMA_WIN, NULL);
> if (dma_window)
> break;
> }
> @@ -1461,7 +1465,7 @@ static bool iommu_bypass_supported_pSeriesLP(struct pci_dev *pdev, u64 dma_mask)
> */
> for (pdn = dn; pdn && PCI_DN(pdn) && !PCI_DN(pdn)->table_group;
> pdn = pdn->parent) {
> - dma_window = of_get_property(pdn, "ibm,dma-window", NULL);
> + dma_window = of_get_property(pdn, DEFAULT_DMA_WIN, NULL);
> if (dma_window)
> break;
> }
> @@ -1475,29 +1479,29 @@ static bool iommu_bypass_supported_pSeriesLP(struct pci_dev *pdev, u64 dma_mask)
> static int iommu_mem_notifier(struct notifier_block *nb, unsigned long action,
> void *data)
> {
> - struct direct_window *window;
> + struct dma_win *window;
> struct memory_notify *arg = data;
> int ret = 0;
>
> switch (action) {
> case MEM_GOING_ONLINE:
> - spin_lock(&direct_window_list_lock);
> - list_for_each_entry(window, &direct_window_list, list) {
> + spin_lock(&dma_win_list_lock);
> + list_for_each_entry(window, &dma_win_list, list) {
> ret |= tce_setrange_multi_pSeriesLP(arg->start_pfn,
> arg->nr_pages, window->prop);
> /* XXX log error */
> }
> - spin_unlock(&direct_window_list_lock);
> + spin_unlock(&dma_win_list_lock);
> break;
> case MEM_CANCEL_ONLINE:
> case MEM_OFFLINE:
> - spin_lock(&direct_window_list_lock);
> - list_for_each_entry(window, &direct_window_list, list) {
> + spin_lock(&dma_win_list_lock);
> + list_for_each_entry(window, &dma_win_list, list) {
> ret |= tce_clearrange_multi_pSeriesLP(arg->start_pfn,
> arg->nr_pages, window->prop);
> /* XXX log error */
> }
> - spin_unlock(&direct_window_list_lock);
> + spin_unlock(&dma_win_list_lock);
> break;
> default:
> break;
> @@ -1518,7 +1522,7 @@ static int iommu_reconfig_notifier(struct notifier_block *nb, unsigned long acti
> struct of_reconfig_data *rd = data;
> struct device_node *np = rd->dn;
> struct pci_dn *pci = PCI_DN(np);
> - struct direct_window *window;
> + struct dma_win *window;
>
> switch (action) {
> case OF_RECONFIG_DETACH_NODE:
> @@ -1537,15 +1541,15 @@ static int iommu_reconfig_notifier(struct notifier_block *nb, unsigned long acti
> iommu_pseries_free_group(pci->table_group,
> np->full_name);
>
> - spin_lock(&direct_window_list_lock);
> - list_for_each_entry(window, &direct_window_list, list) {
> + spin_lock(&dma_win_list_lock);
> + list_for_each_entry(window, &dma_win_list, list) {
> if (window->device == np) {
> list_del(&window->list);
> kfree(window);
> break;
> }
> }
> - spin_unlock(&direct_window_list_lock);
> + spin_unlock(&dma_win_list_lock);
> break;
> default:
> err = NOTIFY_DONE;
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v2 11/14] powerpc/pseries/iommu: Update remove_dma_window() to accept property name
From: Alexey Kardashevskiy @ 2020-09-29 3:56 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Joel Stanley, Christophe Leroy,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev
In-Reply-To: <20200911170738.82818-12-leobras.c@gmail.com>
On 12/09/2020 03:07, Leonardo Bras wrote:
> Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
>
> Update remove_dma_window() so it can be used to remove DDW with a given
> property name.
>
Out of context this seems useless. How about?
===
At the moment pseries stores information about created directly mapped
DDW window in DIRECT64_PROPNAME. We are going to implement indirect DDW
window which we need to preserve during kexec so we need another
property for that.
===
Feel free to correct my english :)
> This enables the creation of new property names for DDW, so we can
> have different usage for it, like indirect mapping.
>
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> ---
> arch/powerpc/platforms/pseries/iommu.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index abd36b257725..f6a65ecd1db5 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -818,31 +818,32 @@ static void remove_dma_window(struct device_node *np, u32 *ddw_avail,
> np, ret, ddw_avail[DDW_REMOVE_PE_DMA_WIN], liobn);
> }
>
> -static void remove_ddw(struct device_node *np, bool remove_prop)
> +static int remove_ddw(struct device_node *np, bool remove_prop, const char *win_name)
> {
> struct property *win;
> u32 ddw_avail[DDW_APPLICABLE_SIZE];
> int ret = 0;
>
> + win = of_find_property(np, win_name, NULL);
> + if (!win)
> + return -EINVAL;
> +
> ret = of_property_read_u32_array(np, "ibm,ddw-applicable",
> &ddw_avail[0], DDW_APPLICABLE_SIZE);
> if (ret)
> - return;
> -
> - win = of_find_property(np, DIRECT64_PROPNAME, NULL);
> - if (!win)
> - return;
> + return 0;
>
> if (win->length >= sizeof(struct dynamic_dma_window_prop))
> remove_dma_window(np, ddw_avail, win);
>
> if (!remove_prop)
> - return;
> + return 0;
>
> ret = of_remove_property(np, win);
> if (ret)
> pr_warn("%pOF: failed to remove direct window property: %d\n",
> np, ret);
> + return 0;
You do not test the return code anywhere until 13/14 so I'd say merge
this one into 13/14, the same comment applies to 12/14. If you do not
move chunks in 13/14, it is going to be fairly small patch.
> }
>
> static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr)
> @@ -894,7 +895,7 @@ static int find_existing_ddw_windows(void)
> for_each_node_with_property(pdn, DIRECT64_PROPNAME) {
> direct64 = of_get_property(pdn, DIRECT64_PROPNAME, &len);
> if (!direct64 || len < sizeof(*direct64)) {
> - remove_ddw(pdn, true);
> + remove_ddw(pdn, true, DIRECT64_PROPNAME);
> continue;
> }
>
> @@ -1325,7 +1326,7 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> win64 = NULL;
>
> out_win_del:
> - remove_ddw(pdn, true);
> + remove_ddw(pdn, true, DIRECT64_PROPNAME);
>
> out_failed:
> if (default_win_removed)
> @@ -1480,7 +1481,7 @@ static int iommu_reconfig_notifier(struct notifier_block *nb, unsigned long acti
> * we have to remove the property when releasing
> * the device node.
> */
> - remove_ddw(np, false);
> + remove_ddw(np, false, DIRECT64_PROPNAME);
> if (pci && pci->table_group)
> iommu_pseries_free_group(pci->table_group,
> np->full_name);
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v2 13/14] powerpc/pseries/iommu: Make use of DDW for indirect mapping
From: Alexey Kardashevskiy @ 2020-09-29 3:56 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Joel Stanley, Christophe Leroy,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev
In-Reply-To: <20200911170738.82818-14-leobras.c@gmail.com>
On 12/09/2020 03:07, Leonardo Bras wrote:
> Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
>
> So far it's assumed possible to map the guest RAM 1:1 to the bus, which
> works with a small number of devices. SRIOV changes it as the user can
> configure hundreds VFs and since phyp preallocates TCEs and does not
> allow IOMMU pages bigger than 64K, it has to limit the number of TCEs
> per a PE to limit waste of physical pages.
>
> As of today, if the assumed direct mapping is not possible, DDW creation
> is skipped and the default DMA window "ibm,dma-window" is used instead.
>
> The default DMA window uses 4k pages instead of 64k pages, and since
> the amount of pages (TCEs) may stay the same (on pHyp case), making
> use of DDW instead of the default DMA window for indirect mapping will
> expand in up to 16x the amount of memory that can be mapped on DMA.
>
> Indirect mapping will only be used if direct mapping is not a
> possibility.
>
> For indirect mapping, it's necessary to re-create the iommu_table with
> the new DMA window parameters, so iommu_alloc() can use it.
>
> Removing the default DMA window for using DDW with indirect mapping
> is only allowed if there is no current IOMMU memory allocated in
> the iommu_table. enable_ddw() is aborted otherwise.
>
> Even though there won't be both direct and indirect mappings at the
> same time, we can't reuse the DIRECT64_PROPNAME property name, or else
> an older kexec()ed kernel can assume direct mapping, and skip
> iommu_alloc(), causing undesirable behavior.
> So a new property name DMA64_PROPNAME "linux,dma64-ddr-window-info"
> was created to represent a DDW that does not allow direct mapping.
>
> Note: ddw_memory_hotplug_max() was moved up so it can be used in
> find_existing_ddw().
>
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> ---
> arch/powerpc/platforms/pseries/iommu.c | 160 ++++++++++++++++---------
> 1 file changed, 103 insertions(+), 57 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 9b7c03652e72..c4de23080d1b 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -375,6 +375,7 @@ static DEFINE_SPINLOCK(direct_window_list_lock);
> /* protects initializing window twice for same device */
> static DEFINE_MUTEX(direct_window_init_mutex);
> #define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
> +#define DMA64_PROPNAME "linux,dma64-ddr-window-info"
>
> static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
> unsigned long num_pfn, const void *arg)
> @@ -846,10 +847,48 @@ static int remove_ddw(struct device_node *np, bool remove_prop, const char *win_
> return 0;
> }
>
> -static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr)
> +static phys_addr_t ddw_memory_hotplug_max(void)
Please, forward declaration or a separate patch; this creates
unnecessary noise to the actual change.
> +{
> + phys_addr_t max_addr = memory_hotplug_max();
> + struct device_node *memory;
> +
> + /*
> + * The "ibm,pmemory" can appear anywhere in the address space.
> + * Assuming it is still backed by page structs, set the upper limit
> + * for the huge DMA window as MAX_PHYSMEM_BITS.
> + */
> + if (of_find_node_by_type(NULL, "ibm,pmemory"))
> + return (sizeof(phys_addr_t) * 8 <= MAX_PHYSMEM_BITS) ?
> + (phys_addr_t)-1 : (1ULL << MAX_PHYSMEM_BITS);
> +
> + for_each_node_by_type(memory, "memory") {
> + unsigned long start, size;
> + int n_mem_addr_cells, n_mem_size_cells, len;
> + const __be32 *memcell_buf;
> +
> + memcell_buf = of_get_property(memory, "reg", &len);
> + if (!memcell_buf || len <= 0)
> + continue;
> +
> + n_mem_addr_cells = of_n_addr_cells(memory);
> + n_mem_size_cells = of_n_size_cells(memory);
> +
> + start = of_read_number(memcell_buf, n_mem_addr_cells);
> + memcell_buf += n_mem_addr_cells;
> + size = of_read_number(memcell_buf, n_mem_size_cells);
> + memcell_buf += n_mem_size_cells;
> +
> + max_addr = max_t(phys_addr_t, max_addr, start + size);
> + }
> +
> + return max_addr;
> +}
> +
> +static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr, bool *direct_mapping)
> {
> struct direct_window *window;
> const struct dynamic_dma_window_prop *direct64;
> + unsigned long window_size;
> bool found = false;
>
> spin_lock(&direct_window_list_lock);
> @@ -858,6 +897,10 @@ static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr)
> if (window->device == pdn) {
> direct64 = window->prop;
> *dma_addr = be64_to_cpu(direct64->dma_base);
> +
> + window_size = (1UL << be32_to_cpu(direct64->window_shift));
> + *direct_mapping = (window_size >= ddw_memory_hotplug_max());
> +
> found = true;
> break;
> }
> @@ -912,6 +955,7 @@ static int find_existing_ddw_windows(void)
> return 0;
>
> find_existing_ddw_windows_named(DIRECT64_PROPNAME);
> + find_existing_ddw_windows_named(DMA64_PROPNAME);
>
> return 0;
> }
> @@ -1054,43 +1098,6 @@ struct failed_ddw_pdn {
>
> static LIST_HEAD(failed_ddw_pdn_list);
>
> -static phys_addr_t ddw_memory_hotplug_max(void)
> -{
> - phys_addr_t max_addr = memory_hotplug_max();
> - struct device_node *memory;
> -
> - /*
> - * The "ibm,pmemory" can appear anywhere in the address space.
> - * Assuming it is still backed by page structs, set the upper limit
> - * for the huge DMA window as MAX_PHYSMEM_BITS.
> - */
> - if (of_find_node_by_type(NULL, "ibm,pmemory"))
> - return (sizeof(phys_addr_t) * 8 <= MAX_PHYSMEM_BITS) ?
> - (phys_addr_t) -1 : (1ULL << MAX_PHYSMEM_BITS);
> -
> - for_each_node_by_type(memory, "memory") {
> - unsigned long start, size;
> - int n_mem_addr_cells, n_mem_size_cells, len;
> - const __be32 *memcell_buf;
> -
> - memcell_buf = of_get_property(memory, "reg", &len);
> - if (!memcell_buf || len <= 0)
> - continue;
> -
> - n_mem_addr_cells = of_n_addr_cells(memory);
> - n_mem_size_cells = of_n_size_cells(memory);
> -
> - start = of_read_number(memcell_buf, n_mem_addr_cells);
> - memcell_buf += n_mem_addr_cells;
> - size = of_read_number(memcell_buf, n_mem_size_cells);
> - memcell_buf += n_mem_size_cells;
> -
> - max_addr = max_t(phys_addr_t, max_addr, start + size);
> - }
> -
> - return max_addr;
> -}
> -
> /*
> * Platforms supporting the DDW option starting with LoPAR level 2.7 implement
> * ibm,ddw-extensions, which carries the rtas token for
> @@ -1173,14 +1180,19 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> struct device_node *dn;
> u32 ddw_avail[DDW_APPLICABLE_SIZE];
> struct direct_window *window;
> + const char *win_name;
> struct property *win64 = NULL;
> struct failed_ddw_pdn *fpdn;
> - bool default_win_removed = false;
> + bool default_win_removed = false, direct_mapping = false;
> + struct pci_dn *pci = PCI_DN(pdn);
> + struct iommu_table *tbl = pci->table_group->tables[0];
>
> mutex_lock(&direct_window_init_mutex);
>
> - if (find_existing_ddw(pdn, &dev->dev.archdata.dma_offset))
> - goto out_unlock;
> + if (find_existing_ddw(pdn, &dev->dev.archdata.dma_offset, &direct_mapping)) {
> + mutex_unlock(&direct_window_init_mutex);
> + return direct_mapping;
> + }
>
> /*
> * If we already went through this for a previous function of
> @@ -1266,15 +1278,25 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> }
>
> /* verify the window * number of ptes will map the partition */
> - /* check largest block * page size > max memory hotplug addr */
> max_addr = ddw_memory_hotplug_max();
> if (query.largest_available_block < (max_addr >> page_shift)) {
> - dev_dbg(&dev->dev, "can't map partition max 0x%llx with %llu "
> - "%llu-sized pages\n", max_addr, query.largest_available_block,
> - 1ULL << page_shift);
> + dev_dbg(&dev->dev, "can't map partition max 0x%llx with %llu %llu-sized pages\n",
> + max_addr, query.largest_available_block,
> + 1ULL << page_shift);
> +
> + len = order_base_2(query.largest_available_block << page_shift);
> + win_name = DMA64_PROPNAME;
> + } else {
> + direct_mapping = true;
> + len = order_base_2(max_addr);
> + win_name = DIRECT64_PROPNAME;
> + }
> +
> + /* DDW + IOMMU on single window may fail if there is any allocation */
> + if (default_win_removed && !direct_mapping && iommu_table_in_use(tbl)) {
> + dev_dbg(&dev->dev, "current IOMMU table in use, can't be replaced.\n");
> goto out_failed;
> }
> - len = order_base_2(max_addr);
>
> ret = create_ddw(dev, ddw_avail, &create, page_shift, len);
> if (ret != 0)
> @@ -1284,8 +1306,7 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> create.liobn, dn);
>
> win_addr = ((u64)create.addr_hi << 32) | create.addr_lo;
> - win64 = ddw_property_create(DIRECT64_PROPNAME, create.liobn, win_addr,
> - page_shift, len);
> + win64 = ddw_property_create(win_name, create.liobn, win_addr, page_shift, len);
> if (!win64) {
> dev_info(&dev->dev,
> "couldn't allocate property, property name, or value\n");
> @@ -1300,15 +1321,37 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> }
>
> window = ddw_list_new_entry(pdn, win64->value);
> - if (!window)
> + if (!window) {
> + dev_dbg(&dev->dev, "couldn't create new list entry\n");
> goto out_prop_del;
> + }
>
> - ret = walk_system_ram_range(0, memblock_end_of_DRAM() >> PAGE_SHIFT,
> - win64->value, tce_setrange_multi_pSeriesLP_walk);
> - if (ret) {
> - dev_info(&dev->dev, "failed to map direct window for %pOF: %d\n",
> - dn, ret);
> - goto out_list_del;
> + if (direct_mapping) {
> + /* DDW maps the whole partition, so enable direct DMA mapping */
> + ret = walk_system_ram_range(0, memblock_end_of_DRAM() >> PAGE_SHIFT,
> + win64->value, tce_setrange_multi_pSeriesLP_walk);
> + if (ret) {
> + dev_info(&dev->dev, "failed to map direct window for %pOF: %d\n",
> + dn, ret);
> + goto out_list_del;
> + }
> + } else {
> + /* New table for using DDW instead of the default DMA window */
> + tbl = iommu_pseries_alloc_table(pci->phb->node);
> + if (!tbl) {
> + dev_dbg(&dev->dev, "couldn't create new IOMMU table\n");
> + goto out_list_del;
> + }
> +
> + _iommu_table_setparms(tbl, pci->phb->bus->number, create.liobn, win_addr,
> + 1UL << len, page_shift, 0, &iommu_table_lpar_multi_ops);
> + iommu_init_table(tbl, pci->phb->node, 0, 0);
It is 0,0 only if win_addr>0 which is not the QEMU case.
> +
> + /* Free old table and replace by the newer */
> + iommu_tce_table_put(pci->table_group->tables[0]);
> + pci->table_group->tables[0] = tbl;
> +
> + set_iommu_table_base(&dev->dev, tbl);
> }
>
> spin_lock(&direct_window_list_lock);
> @@ -1345,7 +1388,7 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>
> out_unlock:
> mutex_unlock(&direct_window_init_mutex);
> - return win64;
> + return win64 && direct_mapping;
> }
>
> static void pci_dma_dev_setup_pSeriesLP(struct pci_dev *dev)
> @@ -1486,7 +1529,10 @@ static int iommu_reconfig_notifier(struct notifier_block *nb, unsigned long acti
> * we have to remove the property when releasing
> * the device node.
> */
> - remove_ddw(np, false, DIRECT64_PROPNAME);
> +
> + if (remove_ddw(np, false, DIRECT64_PROPNAME))
> + remove_ddw(np, false, DMA64_PROPNAME);
> +
> if (pci && pci->table_group)
> iommu_pseries_free_group(pci->table_group,
> np->full_name);
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v2 10/14] powerpc/pseries/iommu: Reorganize iommu_table_setparms*() with new helper
From: Alexey Kardashevskiy @ 2020-09-29 3:56 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Joel Stanley, Christophe Leroy,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev
In-Reply-To: <20200911170738.82818-11-leobras.c@gmail.com>
On 12/09/2020 03:07, Leonardo Bras wrote:
> Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
>
> Add a new helper _iommu_table_setparms(), and use it in
> iommu_table_setparms() and iommu_table_setparms_lpar() to avoid duplicated
> code.
>
> Also, setting tbl->it_ops was happening outsite iommu_table_setparms*(),
> so move it to the new helper. Since we need the iommu_table_ops to be
> declared before used, move iommu_table_lpar_multi_ops and
> iommu_table_pseries_ops to before their respective iommu_table_setparms*().
>
> The tce_exchange_pseries() also had to be moved up, since it's used in
> iommu_table_lpar_multi_ops.xchg_no_kill.
Use forward declarations (preferred) or make a separate patch for moving
chunks (I do not see much point).
>
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> ---
> arch/powerpc/platforms/pseries/iommu.c | 149 ++++++++++++-------------
> 1 file changed, 72 insertions(+), 77 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 510ccb0521af..abd36b257725 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -495,12 +495,62 @@ static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
> return rc;
> }
>
> +#ifdef CONFIG_IOMMU_API
> +static int tce_exchange_pseries(struct iommu_table *tbl, long index, unsigned
> + long *tce, enum dma_data_direction *direction,
> + bool realmode)
> +{
> + long rc;
> + unsigned long ioba = (unsigned long)index << tbl->it_page_shift;
> + unsigned long flags, oldtce = 0;
> + u64 proto_tce = iommu_direction_to_tce_perm(*direction);
> + unsigned long newtce = *tce | proto_tce;
> +
> + spin_lock_irqsave(&tbl->large_pool.lock, flags);
> +
> + rc = plpar_tce_get((u64)tbl->it_index, ioba, &oldtce);
> + if (!rc)
> + rc = plpar_tce_put((u64)tbl->it_index, ioba, newtce);
> +
> + if (!rc) {
> + *direction = iommu_tce_direction(oldtce);
> + *tce = oldtce & ~(TCE_PCI_READ | TCE_PCI_WRITE);
> + }
> +
> + spin_unlock_irqrestore(&tbl->large_pool.lock, flags);
> +
> + return rc;
> +}
> +#endif
> +
> static int tce_setrange_multi_pSeriesLP_walk(unsigned long start_pfn,
> unsigned long num_pfn, void *arg)
> {
> return tce_setrange_multi_pSeriesLP(start_pfn, num_pfn, arg);
> }
>
> +static inline void _iommu_table_setparms(struct iommu_table *tbl, unsigned long busno,
> + unsigned long liobn, unsigned long win_addr,
> + unsigned long window_size, unsigned long page_shift,
> + unsigned long base, struct iommu_table_ops *table_ops)
> +{
> + tbl->it_busno = busno;
> + tbl->it_index = liobn;
> + tbl->it_offset = win_addr >> page_shift;
> + tbl->it_size = window_size >> page_shift;
> + tbl->it_page_shift = page_shift;
> + tbl->it_base = base;
> + tbl->it_blocksize = 16;
> + tbl->it_type = TCE_PCI;
> + tbl->it_ops = table_ops;
> +}
> +
> +struct iommu_table_ops iommu_table_pseries_ops = {
> + .set = tce_build_pSeries,
> + .clear = tce_free_pSeries,
> + .get = tce_get_pseries
> +};
> +
> static void iommu_table_setparms(struct pci_controller *phb,
> struct device_node *dn,
> struct iommu_table *tbl)
> @@ -509,8 +559,13 @@ static void iommu_table_setparms(struct pci_controller *phb,
> const unsigned long *basep;
> const u32 *sizep;
>
> - node = phb->dn;
> + /* Test if we are going over 2GB of DMA space */
> + if (phb->dma_window_base_cur + phb->dma_window_size > 0x80000000ul) {
> + udbg_printf("PCI_DMA: Unexpected number of IOAs under this PHB.\n");
> + panic("PCI_DMA: Unexpected number of IOAs under this PHB.\n");
> + }
s/0x80000000ul/2*SZ_1G/
but more to the point - why this check? QEMU can create windows at 0 and
as big as the VM requested. And I am pretty sure I can construct QEMU
command line such as it won't have MMIO32 at all and a 4GB default DMA
window.
>
> + node = phb->dn;
> basep = of_get_property(node, "linux,tce-base", NULL);
> sizep = of_get_property(node, "linux,tce-size", NULL);
> if (basep == NULL || sizep == NULL) {
> @@ -519,33 +574,25 @@ static void iommu_table_setparms(struct pci_controller *phb,
> return;
> }
>
> - tbl->it_base = (unsigned long)__va(*basep);
> + _iommu_table_setparms(tbl, phb->bus->number, 0, phb->dma_window_base_cur,
> + phb->dma_window_size, IOMMU_PAGE_SHIFT_4K,
> + (unsigned long)__va(*basep), &iommu_table_pseries_ops);
>
> if (!is_kdump_kernel())
> memset((void *)tbl->it_base, 0, *sizep);
>
> - tbl->it_busno = phb->bus->number;
> - tbl->it_page_shift = IOMMU_PAGE_SHIFT_4K;
> -
> - /* Units of tce entries */
> - tbl->it_offset = phb->dma_window_base_cur >> tbl->it_page_shift;
> -
> - /* Test if we are going over 2GB of DMA space */
> - if (phb->dma_window_base_cur + phb->dma_window_size > 0x80000000ul) {
> - udbg_printf("PCI_DMA: Unexpected number of IOAs under this PHB.\n");
> - panic("PCI_DMA: Unexpected number of IOAs under this PHB.\n");
> - }
> -
> phb->dma_window_base_cur += phb->dma_window_size;
> -
> - /* Set the tce table size - measured in entries */
> - tbl->it_size = phb->dma_window_size >> tbl->it_page_shift;
> -
> - tbl->it_index = 0;
> - tbl->it_blocksize = 16;
> - tbl->it_type = TCE_PCI;
> }
>
> +struct iommu_table_ops iommu_table_lpar_multi_ops = {
> + .set = tce_buildmulti_pSeriesLP,
> +#ifdef CONFIG_IOMMU_API
> + .xchg_no_kill = tce_exchange_pseries,
> +#endif
> + .clear = tce_freemulti_pSeriesLP,
> + .get = tce_get_pSeriesLP
> +};
> +
> /*
> * iommu_table_setparms_lpar
> *
> @@ -557,28 +604,17 @@ static void iommu_table_setparms_lpar(struct pci_controller *phb,
> struct iommu_table_group *table_group,
> const __be32 *dma_window)
> {
> - unsigned long offset, size;
> + unsigned long offset, size, liobn;
>
> - of_parse_dma_window(dn, dma_window, &tbl->it_index, &offset, &size);
> + of_parse_dma_window(dn, dma_window, &liobn, &offset, &size);
>
> - tbl->it_busno = phb->bus->number;
> - tbl->it_page_shift = IOMMU_PAGE_SHIFT_4K;
> - tbl->it_base = 0;
> - tbl->it_blocksize = 16;
> - tbl->it_type = TCE_PCI;
> - tbl->it_offset = offset >> tbl->it_page_shift;
> - tbl->it_size = size >> tbl->it_page_shift;
> + _iommu_table_setparms(tbl, phb->bus->number, liobn, offset, size, IOMMU_PAGE_SHIFT_4K, 0,
> + &iommu_table_lpar_multi_ops);
>
> table_group->tce32_start = offset;
> table_group->tce32_size = size;
> }
>
> -struct iommu_table_ops iommu_table_pseries_ops = {
> - .set = tce_build_pSeries,
> - .clear = tce_free_pSeries,
> - .get = tce_get_pseries
> -};
> -
> static void pci_dma_bus_setup_pSeries(struct pci_bus *bus)
> {
> struct device_node *dn;
> @@ -647,7 +683,6 @@ static void pci_dma_bus_setup_pSeries(struct pci_bus *bus)
> tbl = pci->table_group->tables[0];
>
> iommu_table_setparms(pci->phb, dn, tbl);
> - tbl->it_ops = &iommu_table_pseries_ops;
> iommu_init_table(tbl, pci->phb->node, 0, 0);
>
> /* Divide the rest (1.75GB) among the children */
> @@ -658,43 +693,6 @@ static void pci_dma_bus_setup_pSeries(struct pci_bus *bus)
> pr_debug("ISA/IDE, window size is 0x%llx\n", pci->phb->dma_window_size);
> }
>
> -#ifdef CONFIG_IOMMU_API
> -static int tce_exchange_pseries(struct iommu_table *tbl, long index, unsigned
> - long *tce, enum dma_data_direction *direction,
> - bool realmode)
> -{
> - long rc;
> - unsigned long ioba = (unsigned long) index << tbl->it_page_shift;
> - unsigned long flags, oldtce = 0;
> - u64 proto_tce = iommu_direction_to_tce_perm(*direction);
> - unsigned long newtce = *tce | proto_tce;
> -
> - spin_lock_irqsave(&tbl->large_pool.lock, flags);
> -
> - rc = plpar_tce_get((u64)tbl->it_index, ioba, &oldtce);
> - if (!rc)
> - rc = plpar_tce_put((u64)tbl->it_index, ioba, newtce);
> -
> - if (!rc) {
> - *direction = iommu_tce_direction(oldtce);
> - *tce = oldtce & ~(TCE_PCI_READ | TCE_PCI_WRITE);
> - }
> -
> - spin_unlock_irqrestore(&tbl->large_pool.lock, flags);
> -
> - return rc;
> -}
> -#endif
> -
> -struct iommu_table_ops iommu_table_lpar_multi_ops = {
> - .set = tce_buildmulti_pSeriesLP,
> -#ifdef CONFIG_IOMMU_API
> - .xchg_no_kill = tce_exchange_pseries,
> -#endif
> - .clear = tce_freemulti_pSeriesLP,
> - .get = tce_get_pSeriesLP
> -};
> -
> static void pci_dma_bus_setup_pSeriesLP(struct pci_bus *bus)
> {
> struct iommu_table *tbl;
> @@ -729,7 +727,6 @@ static void pci_dma_bus_setup_pSeriesLP(struct pci_bus *bus)
> tbl = ppci->table_group->tables[0];
> iommu_table_setparms_lpar(ppci->phb, pdn, tbl,
> ppci->table_group, dma_window);
> - tbl->it_ops = &iommu_table_lpar_multi_ops;
> iommu_init_table(tbl, ppci->phb->node, 0, 0);
> iommu_register_group(ppci->table_group,
> pci_domain_nr(bus), 0);
> @@ -758,7 +755,6 @@ static void pci_dma_dev_setup_pSeries(struct pci_dev *dev)
> PCI_DN(dn)->table_group = iommu_pseries_alloc_group(phb->node);
> tbl = PCI_DN(dn)->table_group->tables[0];
> iommu_table_setparms(phb, dn, tbl);
> - tbl->it_ops = &iommu_table_pseries_ops;
> iommu_init_table(tbl, phb->node, 0, 0);
> set_iommu_table_base(&dev->dev, tbl);
> return;
> @@ -1385,7 +1381,6 @@ static void pci_dma_dev_setup_pSeriesLP(struct pci_dev *dev)
> tbl = pci->table_group->tables[0];
> iommu_table_setparms_lpar(pci->phb, pdn, tbl,
> pci->table_group, dma_window);
> - tbl->it_ops = &iommu_table_lpar_multi_ops;
> iommu_init_table(tbl, pci->phb->node, 0, 0);
> iommu_register_group(pci->table_group,
> pci_domain_nr(pci->phb->bus), 0);
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v2 09/14] powerpc/pseries/iommu: Add ddw_property_create() and refactor enable_ddw()
From: Alexey Kardashevskiy @ 2020-09-29 3:56 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Joel Stanley, Christophe Leroy,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev
In-Reply-To: <20200911170738.82818-10-leobras.c@gmail.com>
On 12/09/2020 03:07, Leonardo Bras wrote:
> Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
>
> Code used to create a ddw property that was previously scattered in
> enable_ddw() is now gathered in ddw_property_create(), which deals with
> allocation and filling the property, letting it ready for
> of_property_add(), which now occurs in sequence.
>
> This created an opportunity to reorganize the second part of enable_ddw():
>
> Without this patch enable_ddw() does, in order:
> kzalloc() property & members, create_ddw(), fill ddwprop inside property,
> ddw_list_new_entry(), do tce_setrange_multi_pSeriesLP_walk in all memory,
> of_add_property(), and list_add().
>
> With this patch enable_ddw() does, in order:
> create_ddw(), ddw_property_create(), of_add_property(),
> ddw_list_new_entry(), do tce_setrange_multi_pSeriesLP_walk in all memory,
> and list_add().
>
> This change requires of_remove_property() in case anything fails after
> of_add_property(), but we get to do tce_setrange_multi_pSeriesLP_walk
> in all memory, which looks the most expensive operation, only if
> everything else succeeds.
>
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> ---
> arch/powerpc/platforms/pseries/iommu.c | 106 +++++++++++++++----------
> 1 file changed, 63 insertions(+), 43 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index bba8584a8f9d..510ccb0521af 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -177,7 +177,7 @@ static int tce_build_pSeriesLP(unsigned long liobn, long tcenum, long tceshift,
> if (unlikely(rc == H_NOT_ENOUGH_RESOURCES)) {
> ret = (int)rc;
> tce_free_pSeriesLP(liobn, tcenum_start, tceshift,
> - (npages_start - (npages + 1)));
> + (npages_start - (npages + 1)));
Unrelated spaces change, the existing code does not seem incorrect (tabs
first, then spaces).
> break;
> }
>
> @@ -215,7 +215,7 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> if ((npages == 1) || !firmware_has_feature(FW_FEATURE_PUT_TCE_IND)) {
> return tce_build_pSeriesLP(tbl->it_index, tcenum,
> tceshift, npages, uaddr,
> - direction, attrs);
> + direction, attrs);
And here.
> }
>
> local_irq_save(flags); /* to protect tcep and the page behind it */
> @@ -269,7 +269,7 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> if (unlikely(rc == H_NOT_ENOUGH_RESOURCES)) {
> ret = (int)rc;
> tce_freemulti_pSeriesLP(tbl, tcenum_start,
> - (npages_start - (npages + limit)));
> + (npages_start - (npages + limit)));
And here.
> return ret;
> }
>
> @@ -1121,6 +1121,35 @@ static void reset_dma_window(struct pci_dev *dev, struct device_node *par_dn)
> ret);
> }
>
> +static struct property *ddw_property_create(const char *propname, u32 liobn, u64 dma_addr,
> + u32 page_shift, u32 window_shift)
> +{
> + struct dynamic_dma_window_prop *ddwprop;
> + struct property *win64;
> +
> + win64 = kzalloc(sizeof(*win64), GFP_KERNEL);
> + if (!win64)
> + return NULL;
> +
> + win64->name = kstrdup(propname, GFP_KERNEL);
> + ddwprop = kzalloc(sizeof(*ddwprop), GFP_KERNEL);
> + win64->value = ddwprop;
> + win64->length = sizeof(*ddwprop);
> + if (!win64->name || !win64->value) {
> + kfree(win64);
> + kfree(win64->name);
> + kfree(win64->value);
> + return NULL;
> + }
> +
> + ddwprop->liobn = cpu_to_be32(liobn);
> + ddwprop->dma_base = cpu_to_be64(dma_addr);
> + ddwprop->tce_shift = cpu_to_be32(page_shift);
> + ddwprop->window_shift = cpu_to_be32(window_shift);
> +
> + return win64;
> +}
> +
> /*
> * If the PE supports dynamic dma windows, and there is space for a table
> * that can map all pages in a linear offset, then setup such a table,
> @@ -1138,12 +1167,11 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> struct ddw_query_response query;
> struct ddw_create_response create;
> int page_shift;
> - u64 max_addr;
> + u64 max_addr, win_addr;
> struct device_node *dn;
> u32 ddw_avail[DDW_APPLICABLE_SIZE];
> struct direct_window *window;
> - struct property *win64;
> - struct dynamic_dma_window_prop *ddwprop;
> + struct property *win64 = NULL;
> struct failed_ddw_pdn *fpdn;
> bool default_win_removed = false;
>
> @@ -1245,72 +1273,64 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> goto out_failed;
> }
> len = order_base_2(max_addr);
> - win64 = kzalloc(sizeof(struct property), GFP_KERNEL);
> - if (!win64) {
> - dev_info(&dev->dev,
> - "couldn't allocate property for 64bit dma window\n");
> - goto out_failed;
> - }
> - win64->name = kstrdup(DIRECT64_PROPNAME, GFP_KERNEL);
> - win64->value = ddwprop = kmalloc(sizeof(*ddwprop), GFP_KERNEL);
> - win64->length = sizeof(*ddwprop);
> - if (!win64->name || !win64->value) {
> - dev_info(&dev->dev,
> - "couldn't allocate property name and value\n");
> - goto out_free_prop;
> - }
>
> ret = create_ddw(dev, ddw_avail, &create, page_shift, len);
> if (ret != 0)
> - goto out_free_prop;
> -
> - ddwprop->liobn = cpu_to_be32(create.liobn);
> - ddwprop->dma_base = cpu_to_be64(((u64)create.addr_hi << 32) |
> - create.addr_lo);
> - ddwprop->tce_shift = cpu_to_be32(page_shift);
> - ddwprop->window_shift = cpu_to_be32(len);
> + goto out_failed;
>
> dev_dbg(&dev->dev, "created tce table LIOBN 0x%x for %pOF\n",
> - create.liobn, dn);
> + create.liobn, dn);
Unrelated. If you think the spaces/tabs thing needs to be fixed, make it
a separate patch and do all these changes there at once.
>
> - window = ddw_list_new_entry(pdn, ddwprop);
> + win_addr = ((u64)create.addr_hi << 32) | create.addr_lo;
> + win64 = ddw_property_create(DIRECT64_PROPNAME, create.liobn, win_addr,
> + page_shift, len);
> + if (!win64) {
> + dev_info(&dev->dev,
> + "couldn't allocate property, property name, or value\n");
> + goto out_win_del;
> + }
> +
> + ret = of_add_property(pdn, win64);
> + if (ret) {
> + dev_err(&dev->dev, "unable to add dma window property for %pOF: %d",
> + pdn, ret);
> + goto out_prop_free;
> + }
> +
> + window = ddw_list_new_entry(pdn, win64->value);
> if (!window)
> - goto out_clear_window;
> + goto out_prop_del;
>
> ret = walk_system_ram_range(0, memblock_end_of_DRAM() >> PAGE_SHIFT,
> win64->value, tce_setrange_multi_pSeriesLP_walk);
> if (ret) {
> dev_info(&dev->dev, "failed to map direct window for %pOF: %d\n",
> dn, ret);
> - goto out_free_window;
> - }
> -
> - ret = of_add_property(pdn, win64);
> - if (ret) {
> - dev_err(&dev->dev, "unable to add dma window property for %pOF: %d",
> - pdn, ret);
> - goto out_free_window;
> + goto out_list_del;
> }
>
> spin_lock(&direct_window_list_lock);
> list_add(&window->list, &direct_window_list);
> spin_unlock(&direct_window_list_lock);
>
> - dev->dev.archdata.dma_offset = be64_to_cpu(ddwprop->dma_base);
> + dev->dev.archdata.dma_offset = win_addr;
> goto out_unlock;
>
> -out_free_window:
> +out_list_del:
> kfree(window);
>
> -out_clear_window:
> - remove_ddw(pdn, true);
> +out_prop_del:
> + of_remove_property(pdn, win64);
>
> -out_free_prop:
> +out_prop_free:
Really? :) s/out_prop_del/out_del_prop/ may be? The less unrelated
changes the better.
> kfree(win64->name);
> kfree(win64->value);
> kfree(win64);
> win64 = NULL;
>
> +out_win_del:
> + remove_ddw(pdn, true);
> +
> out_failed:
> if (default_win_removed)
> reset_dma_window(dev, pdn);
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v2 01/14] powerpc/pseries/iommu: Replace hard-coded page shift
From: Alexey Kardashevskiy @ 2020-09-29 3:56 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Joel Stanley, Christophe Leroy,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev
In-Reply-To: <20200911170738.82818-2-leobras.c@gmail.com>
On 12/09/2020 03:07, Leonardo Bras wrote:
> Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
These normally go right before "signed-off-by".
>
> Some functions assume IOMMU page size can only be 4K (pageshift == 12).
> Update them to accept any page size passed, so we can use 64K pages.
>
> In the process, some defines like TCE_SHIFT were made obsolete, and then
> removed.
>
> IODA3 Revision 3.0_prd1 (OpenPowerFoundation), Figures 3.4 and 3.5 show
> a RPN of 52-bit, and considers a 12-bit pageshift, so there should be
> no need of using TCE_RPN_MASK, which masks out any bit after 40 in rpn.
> It's usage removed from tce_build_pSeries(), tce_build_pSeriesLP(), and
> tce_buildmulti_pSeriesLP().
>
> Most places had a tbl struct, so using tbl->it_page_shift was simple.
> tce_free_pSeriesLP() was a special case, since callers not always have a
> tbl struct, so adding a tceshift parameter seems the right thing to do.
>
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/include/asm/tce.h | 8 ------
> arch/powerpc/platforms/pseries/iommu.c | 39 +++++++++++++++-----------
> 2 files changed, 23 insertions(+), 24 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/tce.h b/arch/powerpc/include/asm/tce.h
> index db5fc2f2262d..0c34d2756d92 100644
> --- a/arch/powerpc/include/asm/tce.h
> +++ b/arch/powerpc/include/asm/tce.h
> @@ -19,15 +19,7 @@
> #define TCE_VB 0
> #define TCE_PCI 1
>
> -/* TCE page size is 4096 bytes (1 << 12) */
> -
> -#define TCE_SHIFT 12
> -#define TCE_PAGE_SIZE (1 << TCE_SHIFT)
> -
> #define TCE_ENTRY_SIZE 8 /* each TCE is 64 bits */
> -
> -#define TCE_RPN_MASK 0xfffffffffful /* 40-bit RPN (4K pages) */
> -#define TCE_RPN_SHIFT 12
> #define TCE_VALID 0x800 /* TCE valid */
> #define TCE_ALLIO 0x400 /* TCE valid for all lpars */
> #define TCE_PCI_WRITE 0x2 /* write from PCI allowed */
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index e4198700ed1a..9db3927607a4 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -107,6 +107,8 @@ static int tce_build_pSeries(struct iommu_table *tbl, long index,
> u64 proto_tce;
> __be64 *tcep;
> u64 rpn;
> + const unsigned long tceshift = tbl->it_page_shift;
> + const unsigned long pagesize = IOMMU_PAGE_SIZE(tbl);
>
> proto_tce = TCE_PCI_READ; // Read allowed
>
> @@ -117,10 +119,10 @@ static int tce_build_pSeries(struct iommu_table *tbl, long index,
>
> while (npages--) {
> /* can't move this out since we might cross MEMBLOCK boundary */
> - rpn = __pa(uaddr) >> TCE_SHIFT;
> - *tcep = cpu_to_be64(proto_tce | (rpn & TCE_RPN_MASK) << TCE_RPN_SHIFT);
> + rpn = __pa(uaddr) >> tceshift;
> + *tcep = cpu_to_be64(proto_tce | rpn << tceshift);
>
> - uaddr += TCE_PAGE_SIZE;
> + uaddr += pagesize;
> tcep++;
> }
> return 0;
> @@ -146,7 +148,7 @@ static unsigned long tce_get_pseries(struct iommu_table *tbl, long index)
> return be64_to_cpu(*tcep);
> }
>
> -static void tce_free_pSeriesLP(unsigned long liobn, long, long);
> +static void tce_free_pSeriesLP(unsigned long liobn, long, long, long);
> static void tce_freemulti_pSeriesLP(struct iommu_table*, long, long);
>
> static int tce_build_pSeriesLP(unsigned long liobn, long tcenum, long tceshift,
> @@ -166,12 +168,12 @@ static int tce_build_pSeriesLP(unsigned long liobn, long tcenum, long tceshift,
> proto_tce |= TCE_PCI_WRITE;
>
> while (npages--) {
> - tce = proto_tce | (rpn & TCE_RPN_MASK) << tceshift;
> + tce = proto_tce | rpn << tceshift;
> rc = plpar_tce_put((u64)liobn, (u64)tcenum << tceshift, tce);
>
> if (unlikely(rc == H_NOT_ENOUGH_RESOURCES)) {
> ret = (int)rc;
> - tce_free_pSeriesLP(liobn, tcenum_start,
> + tce_free_pSeriesLP(liobn, tcenum_start, tceshift,
> (npages_start - (npages + 1)));
> break;
> }
> @@ -205,10 +207,11 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> long tcenum_start = tcenum, npages_start = npages;
> int ret = 0;
> unsigned long flags;
> + const unsigned long tceshift = tbl->it_page_shift;
>
> if ((npages == 1) || !firmware_has_feature(FW_FEATURE_PUT_TCE_IND)) {
> return tce_build_pSeriesLP(tbl->it_index, tcenum,
> - tbl->it_page_shift, npages, uaddr,
> + tceshift, npages, uaddr,
> direction, attrs);
> }
>
> @@ -225,13 +228,13 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> if (!tcep) {
> local_irq_restore(flags);
> return tce_build_pSeriesLP(tbl->it_index, tcenum,
> - tbl->it_page_shift,
> + tceshift,
> npages, uaddr, direction, attrs);
> }
> __this_cpu_write(tce_page, tcep);
> }
>
> - rpn = __pa(uaddr) >> TCE_SHIFT;
> + rpn = __pa(uaddr) >> tceshift;
> proto_tce = TCE_PCI_READ;
> if (direction != DMA_TO_DEVICE)
> proto_tce |= TCE_PCI_WRITE;
> @@ -245,12 +248,12 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> limit = min_t(long, npages, 4096/TCE_ENTRY_SIZE);
>
> for (l = 0; l < limit; l++) {
> - tcep[l] = cpu_to_be64(proto_tce | (rpn & TCE_RPN_MASK) << TCE_RPN_SHIFT);
> + tcep[l] = cpu_to_be64(proto_tce | rpn << tceshift);
> rpn++;
> }
>
> rc = plpar_tce_put_indirect((u64)tbl->it_index,
> - (u64)tcenum << 12,
> + (u64)tcenum << tceshift,
> (u64)__pa(tcep),
> limit);
>
> @@ -277,12 +280,13 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> return ret;
> }
>
> -static void tce_free_pSeriesLP(unsigned long liobn, long tcenum, long npages)
> +static void tce_free_pSeriesLP(unsigned long liobn, long tcenum, long tceshift,
> + long npages)
> {
> u64 rc;
>
> while (npages--) {
> - rc = plpar_tce_put((u64)liobn, (u64)tcenum << 12, 0);
> + rc = plpar_tce_put((u64)liobn, (u64)tcenum << tceshift, 0);
>
> if (rc && printk_ratelimit()) {
> printk("tce_free_pSeriesLP: plpar_tce_put failed. rc=%lld\n", rc);
> @@ -301,9 +305,11 @@ static void tce_freemulti_pSeriesLP(struct iommu_table *tbl, long tcenum, long n
> u64 rc;
>
> if (!firmware_has_feature(FW_FEATURE_STUFF_TCE))
> - return tce_free_pSeriesLP(tbl->it_index, tcenum, npages);
> + return tce_free_pSeriesLP(tbl->it_index, tcenum,
> + tbl->it_page_shift, npages);
>
> - rc = plpar_tce_stuff((u64)tbl->it_index, (u64)tcenum << 12, 0, npages);
> + rc = plpar_tce_stuff((u64)tbl->it_index,
> + (u64)tcenum << tbl->it_page_shift, 0, npages);
>
> if (rc && printk_ratelimit()) {
> printk("tce_freemulti_pSeriesLP: plpar_tce_stuff failed\n");
> @@ -319,7 +325,8 @@ static unsigned long tce_get_pSeriesLP(struct iommu_table *tbl, long tcenum)
> u64 rc;
> unsigned long tce_ret;
>
> - rc = plpar_tce_get((u64)tbl->it_index, (u64)tcenum << 12, &tce_ret);
> + rc = plpar_tce_get((u64)tbl->it_index,
> + (u64)tcenum << tbl->it_page_shift, &tce_ret);
>
> if (rc && printk_ratelimit()) {
> printk("tce_get_pSeriesLP: plpar_tce_get failed. rc=%lld\n", rc);
>
--
--
Alexey
^ permalink raw reply
* Re: [PATCH v2 05/14] powerpc/kernel/iommu: Add new iommu_table_in_use() helper
From: Alexey Kardashevskiy @ 2020-09-29 3:57 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Joel Stanley, Christophe Leroy,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev
In-Reply-To: <20200911170738.82818-6-leobras.c@gmail.com>
On 12/09/2020 03:07, Leonardo Bras wrote:
> Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
>
> Having a function to check if the iommu table has any allocation helps
> deciding if a tbl can be reset for using a new DMA window.
>
> It should be enough to replace all instances of !bitmap_empty(tbl...).
>
> iommu_table_in_use() skips reserved memory, so we don't need to worry about
> releasing it before testing. This causes iommu_table_release_pages() to
> become unnecessary, given it is only used to remove reserved memory for
> testing.
>
> Also, only allow storing reserved memory values in tbl if they are valid
> in the table, so there is no need to check it in the new helper.
>
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> ---
> arch/powerpc/include/asm/iommu.h | 1 +
> arch/powerpc/kernel/iommu.c | 61 +++++++++++++++-----------------
> 2 files changed, 30 insertions(+), 32 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
> index 5032f1593299..2913e5c8b1f8 100644
> --- a/arch/powerpc/include/asm/iommu.h
> +++ b/arch/powerpc/include/asm/iommu.h
> @@ -154,6 +154,7 @@ extern int iommu_tce_table_put(struct iommu_table *tbl);
> */
> extern struct iommu_table *iommu_init_table(struct iommu_table *tbl,
> int nid, unsigned long res_start, unsigned long res_end);
> +bool iommu_table_in_use(struct iommu_table *tbl);
>
> #define IOMMU_TABLE_GROUP_MAX_TABLES 2
>
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index ffb2637dc82b..c838da3d8f32 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -655,34 +655,21 @@ static void iommu_table_reserve_pages(struct iommu_table *tbl,
> if (tbl->it_offset == 0)
> set_bit(0, tbl->it_map);
>
> + /* Check if res_start..res_end is a valid range in the table */
> + if (res_start >= res_end || res_start < tbl->it_offset ||
> + res_end > (tbl->it_offset + tbl->it_size)) {
> + tbl->it_reserved_start = tbl->it_offset;
> + tbl->it_reserved_end = tbl->it_offset;
This silently ignores overlapped range of the reserved area and the
window which does not seem right.
> + return;
> + }
> +
> tbl->it_reserved_start = res_start;
> tbl->it_reserved_end = res_end;
>
> - /* Check if res_start..res_end isn't empty and overlaps the table */
> - if (res_start && res_end &&
> - (tbl->it_offset + tbl->it_size < res_start ||
> - res_end < tbl->it_offset))
> - return;
> -
> for (i = tbl->it_reserved_start; i < tbl->it_reserved_end; ++i)
> set_bit(i - tbl->it_offset, tbl->it_map);
> }
>
> -static void iommu_table_release_pages(struct iommu_table *tbl)
> -{
> - int i;
> -
> - /*
> - * In case we have reserved the first bit, we should not emit
> - * the warning below.
> - */
> - if (tbl->it_offset == 0)
> - clear_bit(0, tbl->it_map);
> -
> - for (i = tbl->it_reserved_start; i < tbl->it_reserved_end; ++i)
> - clear_bit(i - tbl->it_offset, tbl->it_map);
> -}
> -
> /*
> * Build a iommu_table structure. This contains a bit map which
> * is used to manage allocation of the tce space.
> @@ -743,6 +730,23 @@ struct iommu_table *iommu_init_table(struct iommu_table *tbl, int nid,
> return tbl;
> }
>
> +bool iommu_table_in_use(struct iommu_table *tbl)
> +{
> + unsigned long start = 0, end;
> +
> + /* ignore reserved bit0 */
> + if (tbl->it_offset == 0)
> + start = 1;
> + end = tbl->it_reserved_start - tbl->it_offset;
> + if (find_next_bit(tbl->it_map, end, start) != end)
> + return true;
> +
> + start = tbl->it_reserved_end - tbl->it_offset;
> + end = tbl->it_size;
> + return find_next_bit(tbl->it_map, end, start) != end;
> +
Unnecessary empty line.
> +}
> +
> static void iommu_table_free(struct kref *kref)
> {
> unsigned long bitmap_sz;
> @@ -759,10 +763,8 @@ static void iommu_table_free(struct kref *kref)
> return;
> }
>
> - iommu_table_release_pages(tbl);
> -
> /* verify that table contains no entries */
> - if (!bitmap_empty(tbl->it_map, tbl->it_size))
> + if (iommu_table_in_use(tbl))
> pr_warn("%s: Unexpected TCEs\n", __func__);
>
> /* calculate bitmap size in bytes */
> @@ -1068,18 +1070,13 @@ int iommu_take_ownership(struct iommu_table *tbl)
> for (i = 0; i < tbl->nr_pools; i++)
> spin_lock(&tbl->pools[i].lock);
>
> - iommu_table_release_pages(tbl);
> -
> - if (!bitmap_empty(tbl->it_map, tbl->it_size)) {
> + if (iommu_table_in_use(tbl)) {
> pr_err("iommu_tce: it_map is not empty");
> ret = -EBUSY;
> - /* Undo iommu_table_release_pages, i.e. restore bit#0, etc */
> - iommu_table_reserve_pages(tbl, tbl->it_reserved_start,
> - tbl->it_reserved_end);
> - } else {
> - memset(tbl->it_map, 0xff, sz);
> }
>
> + memset(tbl->it_map, 0xff, sz);
> +
> for (i = 0; i < tbl->nr_pools; i++)
> spin_unlock(&tbl->pools[i].lock);
> spin_unlock_irqrestore(&tbl->large_pool.lock, flags);
>
--
--
Alexey
^ permalink raw reply
* Re: [PATCH v2 02/14] powerpc/pseries/iommu: Makes sure IOMMU_PAGE_SIZE <= PAGE_SIZE
From: Alexey Kardashevskiy @ 2020-09-29 3:57 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Joel Stanley, Christophe Leroy,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev
In-Reply-To: <20200911170738.82818-3-leobras.c@gmail.com>
On 12/09/2020 03:07, Leonardo Bras wrote:
> Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
>
> Having System pagesize < IOMMU pagesize may cause a page owned by another
> process/VM to be written by a buggy driver / device.
>
> As it's intended to use DDW for indirect mapping, it's possible to get
> a configuration where (PAGE_SIZE = 4k) < (IOMMU_PAGE_SIZE() = 64k).
Have you tried 4K VM kernel under phyp with 64K iommu pages? phyp may
assume it is still correct as it knows that it allocates memory in 256MB
contiguous chunks and for DMA purposes phyp may assume that there just a
VM kernel which owns all the VM physical memory and can align size in
iommu_range_alloc() to the IOMMU page size. Not sure.
>
> To avoid this, make sure create_ddw() can only use pagesize <= PAGE_SIZE.
>
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> ---
> arch/powerpc/platforms/pseries/iommu.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 9db3927607a4..4eff3a6a441e 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -1206,17 +1206,20 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> goto out_failed;
> }
> }
> - if (query.page_size & 4) {
> - page_shift = 24; /* 16MB */
> - } else if (query.page_size & 2) {
> +
> + /* Choose the biggest pagesize available <= PAGE_SHIFT */
> + if ((query.page_size & 4)) {
This just doubles braces.
Also I believe for the indirect case you should not even try 16MB.
> + page_shift = 24; /* 16MB - Hugepages */
> + } else if ((query.page_size & 2) && PAGE_SHIFT >= 16) {
PAGE_SHIFT == 16
> page_shift = 16; /* 64kB */
> - } else if (query.page_size & 1) {
> + } else if ((query.page_size & 1) && PAGE_SHIFT >= 12) {
Kind of useless check, if it is not 64K, it is 4K.
I understand you are trying to be safe here but this give a new reader
an idea of more flexibility that there is :) BUILD_BUG_ON() would do
here imho.
> page_shift = 12; /* 4kB */
> } else {
> dev_dbg(&dev->dev, "no supported direct page size in mask %x",
> query.page_size);
> goto out_failed;
> }
> +
Unrelated.
> /* verify the window * number of ptes will map the partition */
> /* check largest block * page size > max memory hotplug addr */
> max_addr = ddw_memory_hotplug_max();
>
--
--
Alexey
^ 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