* Re: [PATCH v1 01/10] powerpc/pseries/iommu: Replace hard-coded page shift
From: Alexey Kardashevskiy @ 2020-09-03 4:26 UTC (permalink / raw)
To: Leonardo Bras, Oliver O'Halloran
Cc: Christophe Leroy, David Dai, Ram Pai, Linux Kernel Mailing List,
Murilo Fossa Vicentini, Paul Mackerras, Joel Stanley, Brian King,
linuxppc-dev, Thiago Jung Bauermann
In-Reply-To: <c381d7e60d0924e432b0f36dce9a44b89733a129.camel@gmail.com>
On 02/09/2020 07:38, Leonardo Bras wrote:
> On Mon, 2020-08-31 at 13:48 +1000, Alexey Kardashevskiy wrote:
>>>>> Well, I created this TCE_RPN_BITS = 52 because the previous mask was a
>>>>> hardcoded 40-bit mask (0xfffffffffful), for hard-coded 12-bit (4k)
>>>>> pagesize, and on PAPR+/LoPAR also defines TCE as having bits 0-51
>>>>> described as RPN, as described before.
>>>>>
>>>>> IODA3 Revision 3.0_prd1 (OpenPowerFoundation), Figure 3.4 and 3.5.
>>>>> shows system memory mapping into a TCE, and the TCE also has bits 0-51
>>>>> for the RPN (52 bits). "Table 3.6. TCE Definition" also shows it.
>>>>> In fact, by the looks of those figures, the RPN_MASK should always be a
>>>>> 52-bit mask, and RPN = (page >> tceshift) & RPN_MASK.
>>>>
>>>> I suspect the mask is there in the first place for extra protection
>>>> against too big addresses going to the TCE table (or/and for virtial vs
>>>> physical addresses). Using 52bit mask makes no sense for anything, you
>>>> could just drop the mask and let c compiler deal with 64bit "uint" as it
>>>> is basically a 4K page address anywhere in the 64bit space. Thanks,
>>>
>>> Assuming 4K pages you need 52 RPN bits to cover the whole 64bit
>>> physical address space. The IODA3 spec does explicitly say the upper
>>> bits are optional and the implementation only needs to support enough
>>> to cover up to the physical address limit, which is 56bits of P9 /
>>> PHB4. If you want to validate that the address will fit inside of
>>> MAX_PHYSMEM_BITS then fine, but I think that should be done as a
>>> WARN_ON or similar rather than just silently masking off the bits.
>>
>> We can do this and probably should anyway but I am also pretty sure we
>> can just ditch the mask and have the hypervisor return an error which
>> will show up in dmesg.
>
> Ok then, ditching the mask.
Well, you could run a little experiment and set some bits above that old
mask and see how phyp reacts :)
> Thanks!
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 1/2] dma-mapping: introduce dma_get_seg_boundary_nr_pages()
From: Michael Ellerman @ 2020-09-03 3:47 UTC (permalink / raw)
To: Nicolin Chen, hch
Cc: x86, linux-ia64, James.Bottomley, paulus, hpa, sparclinux, sfr,
deller, schnelle, borntraeger, mingo, mattst88, fenghua.yu, gor,
linux-s390, hca, ink, tglx, gerald.schaefer, rth, tony.luck,
linux-parisc, linux-kernel, linux-alpha, bp, linuxppc-dev, davem
In-Reply-To: <20200901221646.26491-2-nicoleotsuka@gmail.com>
Nicolin Chen <nicoleotsuka@gmail.com> writes:
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index 9704f3f76e63..cbc2e62db597 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -236,15 +236,10 @@ static unsigned long iommu_range_alloc(struct device *dev,
> }
> }
>
> - if (dev)
> - boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
> - 1 << tbl->it_page_shift);
> - else
> - boundary_size = ALIGN(1UL << 32, 1 << tbl->it_page_shift);
> - /* 4GB boundary for iseries_hv_alloc and iseries_hv_map */
> + boundary_size = dma_get_seg_boundary_nr_pages(dev, tbl->it_page_shift);
>
> n = iommu_area_alloc(tbl->it_map, limit, start, npages, tbl->it_offset,
> - boundary_size >> tbl->it_page_shift, align_mask);
> + boundary_size, align_mask);
This has changed the units of boundary_size, but it's unused elsewhere
in the function so that's OK.
If you need to do a v2 for any other reason, then I'd just drop
boundary_size and call dma_get_seg_boundary_nr_pages() directly in the
function call.
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
cheers
^ permalink raw reply
* Re: [PATCH] ASoC: fsl_sai: Set SAI Channel Mode to Output Mode
From: Nicolin Chen @ 2020-09-03 3:40 UTC (permalink / raw)
To: Shengjiu Wang
Cc: alsa-devel, timur, Xiubo.Lee, lgirdwood, linuxppc-dev, tiwai,
perex, broonie, festevam, linux-kernel
In-Reply-To: <1599102555-17178-1-git-send-email-shengjiu.wang@nxp.com>
On Thu, Sep 03, 2020 at 11:09:15AM +0800, Shengjiu Wang wrote:
> Transmit data pins will output zero when slots are masked or channels
> are disabled. In CHMOD TDM mode, transmit data pins are tri-stated when
> slots are masked or channels are disabled. When data pins are tri-stated,
> there is noise on some channels when FS clock value is high and data is
> read while fsclk is transitioning from high to low.
>
> Signed-off-by: Cosmin-Gabriel Samoila <cosmin.samoila@nxp.com>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>
Though one nit inline:
> ---
> sound/soc/fsl/fsl_sai.c | 12 ++++++++++--
> sound/soc/fsl/fsl_sai.h | 2 ++
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index 62c5fdb678fc..33b194a5c1dc 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -486,6 +486,12 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
>
> val_cr4 |= FSL_SAI_CR4_FRSZ(slots);
>
> + /* Output Mode - data pins transmit 0 when slots are masked
> + * or channels are disabled
> + */
Coding style for multi-line comments. Yet, probably can simplify?
/* Set to output mode to avoid tri-stated data pins */
^ permalink raw reply
* Re: [PATCH] ASoC: fsl_sai: Support multiple data channel enable bits
From: Nicolin Chen @ 2020-09-03 3:30 UTC (permalink / raw)
To: Shengjiu Wang
Cc: alsa-devel, timur, Xiubo.Lee, lgirdwood, linuxppc-dev, tiwai,
perex, broonie, festevam, linux-kernel
In-Reply-To: <1598958068-10552-1-git-send-email-shengjiu.wang@nxp.com>
On Tue, Sep 01, 2020 at 07:01:08PM +0800, Shengjiu Wang wrote:
> One data channel is one data line. From imx7ulp, the SAI IP is
> enhanced to support multiple data channels.
>
> If there is only two channels input and slots is 2, then enable one
> data channel is enough for data transfer. So enable the TCE/RCE and
> transmit/receive mask register according to the input channels and
> slots configuration.
>
> Move the data channel enablement from startup() to hw_params().
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>
^ permalink raw reply
* Re: [PATCH 0/2] dma-mapping: update default segment_boundary_mask
From: Nicolin Chen @ 2020-09-03 3:26 UTC (permalink / raw)
To: Niklas Schnelle
Cc: linux-ia64, James.Bottomley, paulus, hpa, sparclinux, hch, sfr,
deller, x86, borntraeger, mingo, mattst88, fenghua.yu, gor,
linux-s390, hca, ink, tglx, gerald.schaefer, rth, tony.luck,
linux-parisc, linux-kernel, linux-alpha, bp, linuxppc-dev, davem
In-Reply-To: <2c8db0aa-e8b5-e577-b971-1de10ecc6747@linux.ibm.com>
On Wed, Sep 02, 2020 at 10:13:12AM +0200, Niklas Schnelle wrote:
> On 9/2/20 12:16 AM, Nicolin Chen wrote:
> > These two patches are to update default segment_boundary_mask.
> >
> > PATCH-1 fixes overflow issues in callers of dma_get_seg_boundary.
> > Previous version was a series: https://lkml.org/lkml/2020/8/31/1026
> >
> > Then PATCH-2 sets default segment_boundary_mask to ULONG_MAX.
> >
> > Nicolin Chen (2):
> > dma-mapping: introduce dma_get_seg_boundary_nr_pages()
> > dma-mapping: set default segment_boundary_mask to ULONG_MAX
>
> I gave both of your patches a quick test ride on a couple of dev mainframes,
> both NVMe, ConnectX and virtio-pci devices all seems to work fine.
> I already commented on Christoph's mail that I like the helper approach,
> so as for s390 you can add my
>
> Acked-by: Niklas Schnelle <schnelle@linux.ibm.com>
Thanks for testing and the ack!
^ permalink raw reply
* [PATCH] ASoC: fsl_sai: Set SAI Channel Mode to Output Mode
From: Shengjiu Wang @ 2020-09-03 3:09 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, perex, tiwai,
alsa-devel, lgirdwood
Cc: linuxppc-dev, linux-kernel
Transmit data pins will output zero when slots are masked or channels
are disabled. In CHMOD TDM mode, transmit data pins are tri-stated when
slots are masked or channels are disabled. When data pins are tri-stated,
there is noise on some channels when FS clock value is high and data is
read while fsclk is transitioning from high to low.
Signed-off-by: Cosmin-Gabriel Samoila <cosmin.samoila@nxp.com>
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
sound/soc/fsl/fsl_sai.c | 12 ++++++++++--
sound/soc/fsl/fsl_sai.h | 2 ++
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index 62c5fdb678fc..33b194a5c1dc 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -486,6 +486,12 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
val_cr4 |= FSL_SAI_CR4_FRSZ(slots);
+ /* Output Mode - data pins transmit 0 when slots are masked
+ * or channels are disabled
+ */
+ if (tx)
+ val_cr4 |= FSL_SAI_CR4_CHMOD;
+
/*
* For SAI master mode, when Tx(Rx) sync with Rx(Tx) clock, Rx(Tx) will
* generate bclk and frame clock for Tx(Rx), we should set RCR4(TCR4),
@@ -494,7 +500,8 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
if (!sai->is_slave_mode && fsl_sai_dir_is_synced(sai, adir)) {
regmap_update_bits(sai->regmap, FSL_SAI_xCR4(!tx, ofs),
- FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK,
+ FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK |
+ FSL_SAI_CR4_CHMOD_MASK,
val_cr4);
regmap_update_bits(sai->regmap, FSL_SAI_xCR5(!tx, ofs),
FSL_SAI_CR5_WNW_MASK | FSL_SAI_CR5_W0W_MASK |
@@ -502,7 +509,8 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
}
regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx, ofs),
- FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK,
+ FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK |
+ FSL_SAI_CR4_CHMOD_MASK,
val_cr4);
regmap_update_bits(sai->regmap, FSL_SAI_xCR5(tx, ofs),
FSL_SAI_CR5_WNW_MASK | FSL_SAI_CR5_W0W_MASK |
diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
index 6aba7d28f5f3..19cd4e1bbff9 100644
--- a/sound/soc/fsl/fsl_sai.h
+++ b/sound/soc/fsl/fsl_sai.h
@@ -119,6 +119,8 @@
#define FSL_SAI_CR4_FRSZ_MASK (0x1f << 16)
#define FSL_SAI_CR4_SYWD(x) (((x) - 1) << 8)
#define FSL_SAI_CR4_SYWD_MASK (0x1f << 8)
+#define FSL_SAI_CR4_CHMOD BIT(5)
+#define FSL_SAI_CR4_CHMOD_MASK BIT(5)
#define FSL_SAI_CR4_MF BIT(4)
#define FSL_SAI_CR4_FSE BIT(3)
#define FSL_SAI_CR4_FSP BIT(1)
--
2.27.0
^ permalink raw reply related
* Re: [PATCH v2] scsi: ibmvfc: interface updates for future FPIN and MQ support
From: Martin K. Petersen @ 2020-09-03 2:11 UTC (permalink / raw)
To: Tyrel Datwyler
Cc: martin.petersen, linux-scsi, linux-kernel, james.bottomley,
brking, linuxppc-dev
In-Reply-To: <20200901002420.648532-1-tyreld@linux.ibm.com>
Tyrel,
> Fixup complier errors from neglected commit --amend
Bunch of formatting-related checkpatch warnings. Please fix.
Thanks!
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: [PATCH] powerpc/64s: handle ISA v3.1 local copy-paste context switches
From: Paul Mackerras @ 2020-09-03 1:19 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev, kvm-ppc
In-Reply-To: <20200825075535.224536-1-npiggin@gmail.com>
On Tue, Aug 25, 2020 at 05:55:35PM +1000, Nicholas Piggin wrote:
> The ISA v3.1 the copy-paste facility has a new memory move functionality
> which allows the copy buffer to be pasted to domestic memory (RAM) as
> opposed to foreign memory (accelerator).
>
> This means the POWER9 trick of avoiding the cp_abort on context switch if
> the process had not mapped foreign memory does not work on POWER10. Do the
> cp_abort unconditionally there.
>
> KVM must also cp_abort on guest exit to prevent copy buffer state leaking
> between contexts.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
For the KVM part:
Acked-by: Paul Mackerras <paulus@ozlabs.org>
^ permalink raw reply
* Re: [PATCH 10/10] powerpc: remove address space overrides using set_fs()
From: Linus Torvalds @ 2020-09-02 18:02 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, Kees Cook, the arch/x86 maintainers,
Linux Kernel Mailing List, Al Viro, linux-fsdevel, linuxppc-dev,
Christoph Hellwig
In-Reply-To: <d78cb4be-48a9-a7c5-d9d1-d04d2a02b4c6@csgroup.eu>
On Wed, Sep 2, 2020 at 8:17 AM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
> With this fix, I get
>
> root@vgoippro:~# time dd if=/dev/zero of=/dev/null count=1M
> 536870912 bytes (512.0MB) copied, 6.776327 seconds, 75.6MB/s
>
> That's still far from the 91.7MB/s I get with 5.9-rc2, but better than
> the 65.8MB/s I got yesterday with your series. Still some way to go thought.
I don't see why this change would make any difference.
And btw, why do the 32-bit and 64-bit checks even differ? It's not
like the extra (single) instruction should even matter. I think the
main reason is that the simpler 64-bit case could stay as a macro
(because it only uses "addr" and "size" once), but honestly, that
"simplification" doesn't help when you then need to have that #ifdef
for the 32-bit case and an inline function anyway.
So why isn't it just
static inline int __access_ok(unsigned long addr, unsigned long size)
{ return addr <= TASK_SIZE_MAX && size <= TASK_SIZE_MAX-addr; }
for both and be done with it?
The "size=0" check is only relevant for the "addr == TASK_SIZE_MAX"
case, and existed in the old code because it had that "-1" thing
becasue "seg.seg" was actually TASK_SIZE-1.
Now that we don't have any TASK_SIZE-1, zero isn't special any more.
However, I suspect a bigger reason for the actual performance
degradation would be the patch that makes things use "write_iter()"
for writing, even when a simpler "write()" exists.
For writing to /dev/null, the cost of setting up iterators and all the
pointless indirection is all kinds of stupid.
So I think "write()" should just go back to default to using
"->write()" rather than "->write_iter()" if the simpler case exists.
Linus
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/vdso64: link vdso64 with linker
From: Christophe Leroy @ 2020-09-02 18:02 UTC (permalink / raw)
To: Nick Desaulniers, Michael Ellerman
Cc: Christophe Leroy, Joe Lawrence, Kees Cook, Fangrui Song, LKML,
Nicholas Piggin, clang-built-linux, Paul Mackerras, linuxppc-dev
In-Reply-To: <CAKwvOd=ZeJU+vLUk2P7FpX35haj7AC50B9Yps4pyoGCpd7ueTw@mail.gmail.com>
Le 02/09/2020 à 19:41, Nick Desaulniers a écrit :
> On Wed, Sep 2, 2020 at 5:14 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>>
>> Nick Desaulniers <ndesaulniers@google.com> writes:
>>> Fixes: commit f2af201002a8 ("powerpc/build: vdso linker warning for orphan sections")
>>
>> I think I'll just revert that for v5.9 ?
>
> SGTM; you'll probably still want these changes with some modifications
> at some point; vdso32 did have at least one orphaned section, and will
> be important for hermetic builds. Seeing crashes in supported
> versions of the tools ties our hands at the moment.
>
Keeping the tool problem aside with binutils 2.26, do you have a way to
really link an elf32ppc object when building vdso32 for PPC64 ?
Christophe
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/vdso64: link vdso64 with linker
From: Nick Desaulniers @ 2020-09-02 17:41 UTC (permalink / raw)
To: Michael Ellerman
Cc: Christophe Leroy, Joe Lawrence, Kees Cook, Fangrui Song, LKML,
Nicholas Piggin, clang-built-linux, Paul Mackerras, linuxppc-dev
In-Reply-To: <87blio1ilu.fsf@mpe.ellerman.id.au>
On Wed, Sep 2, 2020 at 5:14 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Nick Desaulniers <ndesaulniers@google.com> writes:
> > Fixes: commit f2af201002a8 ("powerpc/build: vdso linker warning for orphan sections")
>
> I think I'll just revert that for v5.9 ?
SGTM; you'll probably still want these changes with some modifications
at some point; vdso32 did have at least one orphaned section, and will
be important for hermetic builds. Seeing crashes in supported
versions of the tools ties our hands at the moment.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
* Re: [PATCH 2/2] powerpc/vdso32: link vdso64 with linker
From: Segher Boessenkool @ 2020-09-02 16:57 UTC (permalink / raw)
To: Christophe Leroy
Cc: Christophe Leroy, Joe Lawrence, Kees Cook, Fangrui Song,
Nick Desaulniers, linux-kernel, Nicholas Piggin,
clang-built-linux, Paul Mackerras, linuxppc-dev
In-Reply-To: <0c895acf-b6d7-baaf-d613-236f8be8e1fe@csgroup.eu>
On Wed, Sep 02, 2020 at 05:43:03PM +0200, Christophe Leroy wrote:
> >Try with a newer ld? If it still happens with current versions, please
> >open a bug report? https://sourceware.org/bugzilla
>
> Yes it works with 2.30 and 2.34
Ah okay, I missed this part.
> But minimum for building kernel is supposed to be 2.23
Sure. Tthat could be upgraded to 2.24 -- you should use a binutils at
least as new as your GCC, and that requires 4.9 now -- but that
probably doesn't help you here).
Segher
^ permalink raw reply
* Re: ptrace_syscall_32 is failing
From: Andy Lutomirski @ 2020-09-02 16:49 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-s390, linuxppc-dev, Vasily Gorbik, Brian Gerst,
Heiko Carstens, X86 ML, LKML, Christian Borntraeger,
Paul Mackerras, Catalin Marinas, Andy Lutomirski, Will Deacon,
linux-arm-kernel
In-Reply-To: <87blioinub.fsf@nanos.tec.linutronix.de>
On Wed, Sep 2, 2020 at 1:29 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Tue, Sep 01 2020 at 17:09, Andy Lutomirski wrote:
> > On Tue, Sep 1, 2020 at 4:50 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> > I think that they almost work for x86, but not quite as
> >> > indicated by this bug. Even if we imagine we can somehow hack around
> >> > this bug, I imagine we're going to find other problems with this
> >> > model, e.g. the potential upcoming exit problem I noted in my review.
> >>
> >> What's the upcoming problem?
> >
> > If we ever want to get single-stepping fully correct across syscalls,
> > we might need to inject SIGTRAP on syscall return. This would be more
> > awkward if we can't run instrumentable code after the syscall part of
> > the syscall is done.
>
> We run a lot of instrumentable code after sys_foo() returns. Otherwise
> all the TIF work would not be possible at all.
>
> But you might tell me where exactly you want to inject the SIGTRAP in
> the syscall exit code flow.
It would be a bit complicated. Definitely after any signals from the
syscall are delivered. Right now, I think that we don't deliver a
SIGTRAP on the instruction boundary after SYSCALL while
single-stepping. (I think we used to, but only sometimes, and now we
are at least consistent.) This is because IRET will not trap if it
starts with TF clear and ends up setting it. (I asked Intel to
document this, and I think they finally did, although I haven't gotten
around to reading the new docs. Certainly the old docs as of a year
or two ago had no description whatsoever of how TF changes worked.)
Deciding exactly *when* a trap should occur would be nontrivial -- we
can't trap on sigreturn() from a SIGTRAP, for example.
So this isn't fully worked out.
>
> >> I don't think we want that in general. The current variant is perfectly
> >> fine for everything except the 32bit fast syscall nonsense. Also
> >> irqentry_entry/exit is not equivalent to the syscall_enter/exit
> >> counterparts.
> >
> > If there are any architectures in which actual work is needed to
> > figure out whether something is a syscall in the first place, they'll
> > want to do the usual kernel entry work before the syscall entry work.
>
> That's low level entry code which does not require RCU, lockdep, tracing
> or whatever muck we setup before actual work can be done.
>
> arch_asm_entry()
> ...
> arch_c_entry(cause) {
> switch(cause) {
> case EXCEPTION: arch_c_exception(...);
> case SYSCALL: arch_c_syscall(...);
> ...
> }
You're assuming that figuring out the cause doesn't need the kernel
entry code to run first. In the case of the 32-bit vDSO fast
syscalls, we arguably don't know whether an entry is a syscall until
we have done a user memory access. Logically, we're doing:
if (get_user() < 0) {
/* Not a syscall. This is actually a silly operation that sets AX =
-EFAULT and returns. Do not audit or invoke ptrace. */
} else {
/* This actually is a syscall. */
}
So we really do want to stick arch code between the
enter_from_user_mode() and the audit check. We *can't* audit because
we don't know the syscall args. Now maybe we could invent new
semantics for this in which a fault here is still somehow a syscall,
but I think that would be a real ABI change and would want very
careful thought. And it would be weird -- syscalls are supposed to
actually call the syscall handler, aren't they? (Arguably we should
go back in time and make this a SIGSEGV. We have the infrastructure
to do this cleanly, but when I wrote the code I just copied the ABI
from code that was before my time. Even so, it would be an exception,
not a syscall.)
>
> You really want to differentiate between exception and syscall
> entry/exit.
>
Why do we want to distinguish between exception and syscall
entry/exit? For the enter part, AFAICS the exception case boils down
to enter_from_user_mode() and the syscall case is:
enter_from_user_mode(regs);
instrumentation_begin();
local_irq_enable();
ti_work = READ_ONCE(current_thread_info()->flags);
if (ti_work & SYSCALL_ENTER_WORK)
syscall = syscall_trace_enter(regs, syscall, ti_work);
instrumentation_end();
Which would decompose quite nicely as a regular (non-syscall) entry
plus the syscall part later.
> The splitting of syscall_enter_from_user_mode() is only necessary for
> that 32bit fast syscall thing on x86 and there is no point to open code
> it with two calls for e.g. do_syscall_64().
>
> > Maybe your patch actually makes this possible -- I haven't digested
> > all the details yet.
> >
> > Who advised you to drop the arch parameter?
>
> Kees, IIRC, but I would have to search through the gazillions of mail
> threads to be sure.
>
> >> + syscall_enter_from_user_mode_prepare(regs);
> >
> > I'm getting lost in all these "enter" functions...
>
> It's not that hard.
>
> syscall_enter_from_user_mode_prepare()
> + syscall_enter_from_user_mode_work()
> = syscall_enter_from_user_mode()
>
> That's exactly what you suggested just with the difference that it is
> explicit for syscalls and not using irqentry_enter/exit().
>
> If we would do that then instead of having a single call for sane
> syscall pathes:
>
> arch_c_entry()
> nr = syscall_enter_from_user_mode();
>
> or for that 32bit fast syscall nonsense the split variant:
>
> arch_c_entry()
> syscall_enter_from_user_mode_prepare();
> do_fast_syscall_muck();
> nr = syscall_enter_from_user_mode_work();
>
> we'd have:
>
> arch_c_entry()
> irqentry_enter();
> local_irq_enble();
> nr = syscall_enter_from_user_mode_work();
> ...
>
> which enforces two calls for sane entries and more code in arch/....
This is why I still like my:
arch_c_entry()
irqentry_enter_from_user_mode();
generic_syscall();
exit...
}
^ permalink raw reply
* Re: [PATCH 2/2] powerpc/vdso32: link vdso64 with linker
From: Christophe Leroy @ 2020-09-02 15:43 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Christophe Leroy, Joe Lawrence, Kees Cook, Fangrui Song,
Nick Desaulniers, linux-kernel, Nicholas Piggin,
clang-built-linux, Paul Mackerras, linuxppc-dev
In-Reply-To: <20200902141431.GV28786@gate.crashing.org>
Hi,
Le 02/09/2020 à 16:14, Segher Boessenkool a écrit :
> Hi!
>
> On Wed, Sep 02, 2020 at 06:46:45AM +0000, Christophe Leroy wrote:
>> ld crashes:
>>
>> LD arch/powerpc/kernel/vdso32/vdso32.so.dbg
>> /bin/sh: line 1: 23780 Segmentation fault (core dumped)
>> ppc-linux-ld -EB -m elf32ppc -shared -soname linux-vdso32.so.1
>> --eh-frame-hdr --orphan-handling=warn -T
>> arch/powerpc/kernel/vdso32/vdso32.lds
>> arch/powerpc/kernel/vdso32/sigtramp.o
>> arch/powerpc/kernel/vdso32/gettimeofday.o
>> arch/powerpc/kernel/vdso32/datapage.o
>> arch/powerpc/kernel/vdso32/cacheflush.o
>> arch/powerpc/kernel/vdso32/note.o arch/powerpc/kernel/vdso32/getcpu.o -o
>> arch/powerpc/kernel/vdso32/vdso32.so.dbg
>> make[4]: *** [arch/powerpc/kernel/vdso32/vdso32.so.dbg] Error 139
>>
>>
>> [root@localhost linux-powerpc]# ppc-linux-ld --version
>> GNU ld (GNU Binutils) 2.26.20160125
>
> [ Don't build as root :-P ]
>
> Try with a newer ld? If it still happens with current versions, please
> open a bug report? https://sourceware.org/bugzilla
Yes it works with 2.30 and 2.34
But minimum for building kernel is supposed to be 2.23
Christophe
^ permalink raw reply
* Re: [PATCH 10/10] powerpc: remove address space overrides using set_fs()
From: Christophe Leroy @ 2020-09-02 15:17 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, Kees Cook, x86, linuxppc-dev, linux-kernel, Al Viro,
linux-fsdevel, Linus Torvalds
In-Reply-To: <20200902123646.GA31184@lst.de>
Le 02/09/2020 à 14:36, Christoph Hellwig a écrit :
> On Wed, Sep 02, 2020 at 08:15:12AM +0200, Christophe Leroy wrote:
>>> - return 0;
>>> - return (size == 0 || size - 1 <= seg.seg - addr);
>>> + if (addr >= TASK_SIZE_MAX)
>>> + return false;
>>> + if (size == 0)
>>> + return false;
>>
>> __access_ok() was returning true when size == 0 up to now. Any reason to
>> return false now ?
>
> No, this is accidental and broken. Can you re-run your benchmark with
> this fixed?
>
With this fix, I get
root@vgoippro:~# time dd if=/dev/zero of=/dev/null count=1M
1048576+0 records in
1048576+0 records out
536870912 bytes (512.0MB) copied, 6.776327 seconds, 75.6MB/s
real 0m 6.78s
user 0m 1.64s
sys 0m 5.13s
That's still far from the 91.7MB/s I get with 5.9-rc2, but better than
the 65.8MB/s I got yesterday with your series. Still some way to go thought.
Christophe
^ permalink raw reply
* RE: [PATCH 10/10] powerpc: remove address space overrides using set_fs()
From: David Laight @ 2020-09-02 15:02 UTC (permalink / raw)
To: 'Christophe Leroy', 'Christoph Hellwig'
Cc: linux-arch@vger.kernel.org, Kees Cook, x86@kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
Al Viro, linux-fsdevel@vger.kernel.org, Linus Torvalds
In-Reply-To: <6e88048a-8b30-400e-11c6-8d91ba77cbb0@csgroup.eu>
From: Christophe Leroy
> Sent: 02 September 2020 15:13
>
>
> Le 02/09/2020 à 15:51, David Laight a écrit :
> > From: Christophe Leroy
> >> Sent: 02 September 2020 14:25
> >> Le 02/09/2020 à 15:13, David Laight a écrit :
> >>> From: Christoph Hellwig
> >>>> Sent: 02 September 2020 13:37
> >>>>
> >>>> On Wed, Sep 02, 2020 at 08:15:12AM +0200, Christophe Leroy wrote:
> >>>>>> - return 0;
> >>>>>> - return (size == 0 || size - 1 <= seg.seg - addr);
> >>>>>> + if (addr >= TASK_SIZE_MAX)
> >>>>>> + return false;
> >>>>>> + if (size == 0)
> >>>>>> + return false;
> >>>>>
> >>>>> __access_ok() was returning true when size == 0 up to now. Any reason to
> >>>>> return false now ?
> >>>>
> >>>> No, this is accidental and broken. Can you re-run your benchmark with
> >>>> this fixed?
> >>>
> >>> Is TASK_SIZE_MASK defined such that you can do:
> >>>
> >>> return (addr | size) < TASK_SIZE_MAX) || !size;
> >>
> >> TASK_SIZE_MAX will usually be 0xc0000000
> >>
> >> With:
> >> addr = 0x80000000;
> >> size = 0x80000000;
> >>
> >> I expect it to fail ....
> >>
> >> With the formula you propose it will succeed, won't it ?
> >
> > Hmmm... Was i getting confused about some comments for 64bit
> > about there being such a big hole between valid user and kernel
> > addresses that it was enough to check that 'size < TASK_SIZE_MAX'.
> >
> > That would be true for 64bit x86 (and probably ppc (& arm??))
> > if TASK_SIZE_MAX were 0x4 << 60.
> > IIUC the highest user address is (much) less than 0x0 << 60
> > and the lowest kernel address (much) greater than 0xf << 60
> > on all these 64bit platforms.
> >
> > Actually if doing access_ok() inside get_user() you don't
> > need to check the size at all.
>
> You mean on 64 bit or on any platform ?
64bit and 32bit
> What about a word write to 0xbffffffe, won't it overwrite 0xc0000000 ?
>
> > You don't even need to in copy_to/from_user() provided
> > it always does a forwards copy.
>
> Do you mean due to the gap ?
> Is it garantied to be a gap ? Even on a 32 bits having TASK_SIZE set to
> 0xc0000000 and PAGE_OFFSET set to the same ?
I read somewhere (I won't find it again) that the last 4k page
(below 0xc0000000) must not be allocated on i386 because some
cpu (both intel and amd) do 'horrid things' if they try to
(IIRC) do instruction prefetches across the boundary.
So the accesses to 0xbffffffe will fault and the one to 0xc0000000
won't happen (in any useful way at least).
I'd suspect that not allocating the 3G-4k page would be a safe
bet on all architectures - even 68k.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* [Bug 206203] kmemleak reports various leaks in drivers/of/unittest.c
From: bugzilla-daemon @ 2020-09-02 14:45 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-206203-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=206203
Erhard F. (erhard_f@mailbox.org) changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #288187|0 |1
is obsolete| |
--- Comment #20 from Erhard F. (erhard_f@mailbox.org) ---
Created attachment 292289
--> https://bugzilla.kernel.org/attachment.cgi?id=292289&action=edit
kernel .config (kernel 5.9-rc3, Talos II)
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [Bug 206203] kmemleak reports various leaks in drivers/of/unittest.c
From: bugzilla-daemon @ 2020-09-02 14:39 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-206203-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=206203
Erhard F. (erhard_f@mailbox.org) changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #288189|0 |1
is obsolete| |
--- Comment #19 from Erhard F. (erhard_f@mailbox.org) ---
Created attachment 292287
--> https://bugzilla.kernel.org/attachment.cgi?id=292287&action=edit
kmemleak output (kernel 5.9-rc3, Talos II)
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [Bug 206203] kmemleak reports various leaks in drivers/of/unittest.c
From: bugzilla-daemon @ 2020-09-02 14:38 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-206203-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=206203
Erhard F. (erhard_f@mailbox.org) changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #288185|0 |1
is obsolete| |
--- Comment #18 from Erhard F. (erhard_f@mailbox.org) ---
Created attachment 292285
--> https://bugzilla.kernel.org/attachment.cgi?id=292285&action=edit
dmesg (kernel 5.9-rc3, Talos II)
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [PATCH 2/2] powerpc/vdso32: link vdso64 with linker
From: Segher Boessenkool @ 2020-09-02 14:14 UTC (permalink / raw)
To: Christophe Leroy
Cc: Christophe Leroy, Joe Lawrence, Kees Cook, Fangrui Song,
Nick Desaulniers, linux-kernel, Nicholas Piggin,
clang-built-linux, Paul Mackerras, linuxppc-dev
In-Reply-To: <b2066ccd-2b81-6032-08e3-41105b400f75@csgroup.eu>
Hi!
On Wed, Sep 02, 2020 at 06:46:45AM +0000, Christophe Leroy wrote:
> ld crashes:
>
> LD arch/powerpc/kernel/vdso32/vdso32.so.dbg
> /bin/sh: line 1: 23780 Segmentation fault (core dumped)
> ppc-linux-ld -EB -m elf32ppc -shared -soname linux-vdso32.so.1
> --eh-frame-hdr --orphan-handling=warn -T
> arch/powerpc/kernel/vdso32/vdso32.lds
> arch/powerpc/kernel/vdso32/sigtramp.o
> arch/powerpc/kernel/vdso32/gettimeofday.o
> arch/powerpc/kernel/vdso32/datapage.o
> arch/powerpc/kernel/vdso32/cacheflush.o
> arch/powerpc/kernel/vdso32/note.o arch/powerpc/kernel/vdso32/getcpu.o -o
> arch/powerpc/kernel/vdso32/vdso32.so.dbg
> make[4]: *** [arch/powerpc/kernel/vdso32/vdso32.so.dbg] Error 139
>
>
> [root@localhost linux-powerpc]# ppc-linux-ld --version
> GNU ld (GNU Binutils) 2.26.20160125
[ Don't build as root :-P ]
Try with a newer ld? If it still happens with current versions, please
open a bug report? https://sourceware.org/bugzilla
Segher
^ permalink raw reply
* Re: [PATCH 10/10] powerpc: remove address space overrides using set_fs()
From: Christophe Leroy @ 2020-09-02 14:12 UTC (permalink / raw)
To: David Laight, 'Christoph Hellwig'
Cc: linux-arch@vger.kernel.org, Kees Cook, x86@kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
Al Viro, linux-fsdevel@vger.kernel.org, Linus Torvalds
In-Reply-To: <0c298e0d972a48bd9ee178225e404b12@AcuMS.aculab.com>
Le 02/09/2020 à 15:51, David Laight a écrit :
> From: Christophe Leroy
>> Sent: 02 September 2020 14:25
>> Le 02/09/2020 à 15:13, David Laight a écrit :
>>> From: Christoph Hellwig
>>>> Sent: 02 September 2020 13:37
>>>>
>>>> On Wed, Sep 02, 2020 at 08:15:12AM +0200, Christophe Leroy wrote:
>>>>>> - return 0;
>>>>>> - return (size == 0 || size - 1 <= seg.seg - addr);
>>>>>> + if (addr >= TASK_SIZE_MAX)
>>>>>> + return false;
>>>>>> + if (size == 0)
>>>>>> + return false;
>>>>>
>>>>> __access_ok() was returning true when size == 0 up to now. Any reason to
>>>>> return false now ?
>>>>
>>>> No, this is accidental and broken. Can you re-run your benchmark with
>>>> this fixed?
>>>
>>> Is TASK_SIZE_MASK defined such that you can do:
>>>
>>> return (addr | size) < TASK_SIZE_MAX) || !size;
>>
>> TASK_SIZE_MAX will usually be 0xc0000000
>>
>> With:
>> addr = 0x80000000;
>> size = 0x80000000;
>>
>> I expect it to fail ....
>>
>> With the formula you propose it will succeed, won't it ?
>
> Hmmm... Was i getting confused about some comments for 64bit
> about there being such a big hole between valid user and kernel
> addresses that it was enough to check that 'size < TASK_SIZE_MAX'.
>
> That would be true for 64bit x86 (and probably ppc (& arm??))
> if TASK_SIZE_MAX were 0x4 << 60.
> IIUC the highest user address is (much) less than 0x0 << 60
> and the lowest kernel address (much) greater than 0xf << 60
> on all these 64bit platforms.
>
> Actually if doing access_ok() inside get_user() you don't
> need to check the size at all.
You mean on 64 bit or on any platform ?
What about a word write to 0xbffffffe, won't it overwrite 0xc0000000 ?
> You don't even need to in copy_to/from_user() provided
> it always does a forwards copy.
Do you mean due to the gap ?
Is it garantied to be a gap ? Even on a 32 bits having TASK_SIZE set to
0xc0000000 and PAGE_OFFSET set to the same ?
Christophe
^ permalink raw reply
* RE: [PATCH 10/10] powerpc: remove address space overrides using set_fs()
From: David Laight @ 2020-09-02 13:51 UTC (permalink / raw)
To: 'Christophe Leroy', 'Christoph Hellwig'
Cc: linux-arch@vger.kernel.org, Kees Cook, x86@kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
Al Viro, linux-fsdevel@vger.kernel.org, Linus Torvalds
In-Reply-To: <8de54fe0-4be9-5624-dd1d-d95d792e933d@csgroup.eu>
From: Christophe Leroy
> Sent: 02 September 2020 14:25
> Le 02/09/2020 à 15:13, David Laight a écrit :
> > From: Christoph Hellwig
> >> Sent: 02 September 2020 13:37
> >>
> >> On Wed, Sep 02, 2020 at 08:15:12AM +0200, Christophe Leroy wrote:
> >>>> - return 0;
> >>>> - return (size == 0 || size - 1 <= seg.seg - addr);
> >>>> + if (addr >= TASK_SIZE_MAX)
> >>>> + return false;
> >>>> + if (size == 0)
> >>>> + return false;
> >>>
> >>> __access_ok() was returning true when size == 0 up to now. Any reason to
> >>> return false now ?
> >>
> >> No, this is accidental and broken. Can you re-run your benchmark with
> >> this fixed?
> >
> > Is TASK_SIZE_MASK defined such that you can do:
> >
> > return (addr | size) < TASK_SIZE_MAX) || !size;
>
> TASK_SIZE_MAX will usually be 0xc0000000
>
> With:
> addr = 0x80000000;
> size = 0x80000000;
>
> I expect it to fail ....
>
> With the formula you propose it will succeed, won't it ?
Hmmm... Was i getting confused about some comments for 64bit
about there being such a big hole between valid user and kernel
addresses that it was enough to check that 'size < TASK_SIZE_MAX'.
That would be true for 64bit x86 (and probably ppc (& arm??))
if TASK_SIZE_MAX were 0x4 << 60.
IIUC the highest user address is (much) less than 0x0 << 60
and the lowest kernel address (much) greater than 0xf << 60
on all these 64bit platforms.
Actually if doing access_ok() inside get_user() you don't
need to check the size at all.
You don't even need to in copy_to/from_user() provided
it always does a forwards copy.
(Rather that copying the last word first for misaligned lengths.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH 10/10] powerpc: remove address space overrides using set_fs()
From: Christophe Leroy @ 2020-09-02 13:24 UTC (permalink / raw)
To: David Laight, 'Christoph Hellwig'
Cc: linux-arch@vger.kernel.org, Kees Cook, x86@kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
Al Viro, linux-fsdevel@vger.kernel.org, Linus Torvalds
In-Reply-To: <61b9a880a6424a34b841cf3dddb463ad@AcuMS.aculab.com>
Le 02/09/2020 à 15:13, David Laight a écrit :
> From: Christoph Hellwig
>> Sent: 02 September 2020 13:37
>>
>> On Wed, Sep 02, 2020 at 08:15:12AM +0200, Christophe Leroy wrote:
>>>> - return 0;
>>>> - return (size == 0 || size - 1 <= seg.seg - addr);
>>>> + if (addr >= TASK_SIZE_MAX)
>>>> + return false;
>>>> + if (size == 0)
>>>> + return false;
>>>
>>> __access_ok() was returning true when size == 0 up to now. Any reason to
>>> return false now ?
>>
>> No, this is accidental and broken. Can you re-run your benchmark with
>> this fixed?
>
> Is TASK_SIZE_MASK defined such that you can do:
>
> return (addr | size) < TASK_SIZE_MAX) || !size;
TASK_SIZE_MAX will usually be 0xc0000000
With:
addr = 0x80000000;
size = 0x80000000;
I expect it to fail ....
With the formula you propose it will succeed, won't it ?
Christophe
^ permalink raw reply
* Re: [PATCH v3 12/13] mm/debug_vm_pgtable/hugetlb: Disable hugetlb test on ppc64
From: Aneesh Kumar K.V @ 2020-09-02 13:20 UTC (permalink / raw)
To: Anshuman Khandual, linux-mm, akpm
Cc: linux-arch, linux-s390, Christophe Leroy, x86, Mike Rapoport,
Qian Cai, Gerald Schaefer, Vineet Gupta, linux-snps-arc,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <a76a180b-650c-c868-7a52-593afe97eab3@arm.com>
Anshuman Khandual <anshuman.khandual@arm.com> writes:
> On 09/01/2020 12:00 PM, Aneesh Kumar K.V wrote:
>> On 9/1/20 9:33 AM, Anshuman Khandual wrote:
>>>
>>>
>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>>> The seems to be missing quite a lot of details w.r.t allocating
>>>> the correct pgtable_t page (huge_pte_alloc()), holding the right
>>>> lock (huge_pte_lock()) etc. The vma used is also not a hugetlb VMA.
>>>>
>>>> ppc64 do have runtime checks within CONFIG_DEBUG_VM for most of these.
>>>> Hence disable the test on ppc64.
>>>
>>> Would really like this to get resolved in an uniform and better way
>>> instead, i.e a modified hugetlb_advanced_tests() which works on all
>>> platforms including ppc64.
>>>
>>> In absence of a modified version, I do realize the situation here,
>>> where DEBUG_VM_PGTABLE test either runs on ppc64 or just completely
>>> remove hugetlb_advanced_tests() from other platforms as well.
>>>
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>> ---
>>>> mm/debug_vm_pgtable.c | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>> index a188b6e4e37e..21329c7d672f 100644
>>>> --- a/mm/debug_vm_pgtable.c
>>>> +++ b/mm/debug_vm_pgtable.c
>>>> @@ -813,6 +813,7 @@ static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot)
>>>> #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
>>>> }
>>>> +#ifndef CONFIG_PPC_BOOK3S_64
>>>> static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>>> struct vm_area_struct *vma,
>>>> pte_t *ptep, unsigned long pfn,
>>>> @@ -855,6 +856,7 @@ static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>>> pte = huge_ptep_get(ptep);
>>>> WARN_ON(!(huge_pte_write(pte) && huge_pte_dirty(pte)));
>>>> }
>>>> +#endif
>>>
>>> In the worst case if we could not get a new hugetlb_advanced_tests() test
>>> that works on all platforms, this might be the last fallback option. In
>>> which case, it will require a proper comment section with a "FIXME: ",
>>> explaining the current situation (and that #ifdef is temporary in nature)
>>> and a hugetlb_advanced_tests() stub when CONFIG_PPC_BOOK3S_64 is enabled.
>>>
>>>> #else /* !CONFIG_HUGETLB_PAGE */
>>>> static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot) { }
>>>> static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>>> @@ -1065,7 +1067,9 @@ static int __init debug_vm_pgtable(void)
>>>> pud_populate_tests(mm, pudp, saved_pmdp);
>>>> spin_unlock(ptl);
>>>> +#ifndef CONFIG_PPC_BOOK3S_64
>>>> hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>>>> +#endif
>>>
>>
>> I actually wanted to add #ifdef BROKEN. That test is completely broken. Infact I would suggest to remove that test completely.
>>
>>
>>
>>> #ifdef will not be required here as there would be a stub definition
>>> for hugetlb_advanced_tests() when CONFIG_PPC_BOOK3S_64 is enabled.
>>>
>>>> spin_lock(&mm->page_table_lock);
>>>> p4d_clear_tests(mm, p4dp);
>>>>
>>>
>>> But again, we should really try and avoid taking this path.
>>>
>>
>> To be frank i am kind of frustrated with how this patch series is being looked at. We pushed a completely broken test to upstream and right now we have a code in upstream that crash when booted on ppc64. My attempt has been to make progress here and you definitely seems to be not in agreement to that.
>>
>
> I am afraid, this does not accurately represent the situation.
>
> - The second set patch series got merged in it's V5 after accommodating almost
> all reviews and objections during previous discussion cycles. For a complete
> development log, please refer https://patchwork.kernel.org/cover/11658627/.
>
> - The series has been repeatedly tested on arm64 and x86 platforms for multiple
> configurations but build tested on all other enabled platforms. I have always
> been dependent on voluntary help from folks on the list to get this tested on
> other enabled platforms as I dont have access to such systems. Always assumed
> that is the way to go for anything which runs on multiple platforms. So, am I
> expected to test on platforms that I dont have access to ? But I am ready to
> be corrected here, if the community protocol is not what I have always assumed
> it to be.
>
> - Each and every version of the series had appropriately copied all the enabled
> platform's mailing list. Also, I had explicitly asked for volunteers to test
> this out on platforms apart from x86 and arm64. We had positive response from
> all platforms i.e arc, s390, ppc32 but except for ppc64.
>
> https://patchwork.kernel.org/cover/11644771/
> https://patchwork.kernel.org/cover/11603713/
>
> - The development cycle provided sufficient time window for any detailed review
> and test. I have always been willing to address almost all the issues brought
> forward during these discussions. From past experience on this test, there is
> an inherent need to understand platform specific details while trying to come
> up with something generic enough that works on all platforms. It necessitates
> participation from relevant folks to enable this test on a given platform. We
> were able to enable this on arm64, x86, arc, s390, powerpc following a similar
> model.
>
> - I have to disagree here that the concerned test i.e hugetlb_advanced_tests()
> is completely broken. As mentioned before, the idea here has always been to
> emulate enough MM objects, so that a given page table helper could be tested.
> hugetlb_advanced_tests() seems to be insufficient on ppc64 platform causing it
> to crash, which is not the case on other platforms. But it is not perfect and
> can be improved upon. Given the constraints i.e limited emulation of objects,
> the test tries to do the right thing. Calling it broken is not an appropriate
> description.
>
None of the fixes done here are specific to ppc64. I am not sure why you
keep suggesting ppc64 specific issues. One should not do page table
updates without holding locks. A hugetlb pte updates expect a vma marked
hugetlb.
As explained in the patch, I see very little value in a bunch of tests
like this and the only reason I started to fix this up is because of
multiple crash reports on ppc64.
Considering the hugetlb tests require much larger change and as it is
currently written is broken, I wanted to remove that test and let you
come up with a proper test. But since you had it "working", I disabled
this only on ppc64.
But you keep suggesting that the hugetlb test need to be fixed as part
of the patch series review. I don't have enough motivation to fix that,
because I don't see much value in a bunch of tests like these. As shown
already these tests already reported success till now without even
following any page table update rules.
-aneesh
^ permalink raw reply
* RE: [PATCH 10/10] powerpc: remove address space overrides using set_fs()
From: David Laight @ 2020-09-02 13:13 UTC (permalink / raw)
To: 'Christoph Hellwig', Christophe Leroy
Cc: linux-arch@vger.kernel.org, Kees Cook, x86@kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
Al Viro, linux-fsdevel@vger.kernel.org, Linus Torvalds
In-Reply-To: <20200902123646.GA31184@lst.de>
From: Christoph Hellwig
> Sent: 02 September 2020 13:37
>
> On Wed, Sep 02, 2020 at 08:15:12AM +0200, Christophe Leroy wrote:
> >> - return 0;
> >> - return (size == 0 || size - 1 <= seg.seg - addr);
> >> + if (addr >= TASK_SIZE_MAX)
> >> + return false;
> >> + if (size == 0)
> >> + return false;
> >
> > __access_ok() was returning true when size == 0 up to now. Any reason to
> > return false now ?
>
> No, this is accidental and broken. Can you re-run your benchmark with
> this fixed?
Is TASK_SIZE_MASK defined such that you can do:
return (addr | size) < TASK_SIZE_MAX) || !size;
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ 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