* Re: [PATCH] powerpc/mce: check if event info is valid
From: Ganesh @ 2021-09-17 8:54 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: mahesh, npiggin
In-Reply-To: <20210806132307.367688-1-ganeshgr@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 851 bytes --]
On 8/6/21 6:53 PM, Ganesh Goudar wrote:
> Check if the event info is valid before printing the
> event information. When a fwnmi enabled nested kvm guest
> hits a machine check exception L0 and L2 would generate
> machine check event info, But L1 would not generate any
> machine check event info as it won't go through 0x200
> vector and prints some unwanted message.
>
> To fix this, 'in_use' variable in machine check event info is
> no more in use, rename it to 'valid' and check if the event
> information is valid before logging the event information.
>
> without this patch L1 would print following message for
> exceptions encountered in L2, as event structure will be
> empty in L1.
>
> "Machine Check Exception, Unknown event version 0".
>
> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
> ---
Hi mpe, Any comments on this patch.
[-- Attachment #2: Type: text/html, Size: 1255 bytes --]
^ permalink raw reply
* Re: [PATCH] swiotlb: set IO TLB segment size via cmdline
From: Jan Beulich @ 2021-09-17 9:52 UTC (permalink / raw)
To: Roman Skakun
Cc: linux-doc, Peter Zijlstra, Viresh Kumar,
Linux Kernel Mailing List, Paul Mackerras, Will Deacon,
Boris Ostrovsky, Marek Szyprowski, Stefano Stabellini,
Jonathan Corbet, Christoph Hellwig, xen-devel, Paul E. McKenney,
Konrad Rzeszutek Wilk, Muchun Song, Thomas Gleixner,
Juergen Gross, Thomas Bogendoerfer, Andrii Anisov, linuxppc-dev,
Randy Dunlap, linux-mips, iommu, Roman Skakun, Andrew Morton,
Maciej W. Rozycki, Robin Murphy, Mike Rapoport, Lu Baolu
In-Reply-To: <CADu_u-OZzgVj+z=iD6kUQOZxUufF5QSMR6-MmpN_hLZ9PyQJhQ@mail.gmail.com>
On 17.09.2021 11:36, Roman Skakun wrote:
> I use Xen PV display. In my case, PV display backend(Dom0) allocates
> contiguous buffer via DMA-API to
> to implement zero-copy between Dom0 and DomU.
Why does the buffer need to be allocated by Dom0? If it was allocated
by DomU, it could use grants to give the backend direct (zero-copy)
access.
Jan
^ permalink raw reply
* Re: [PATCH] swiotlb: set IO TLB segment size via cmdline
From: Robin Murphy @ 2021-09-17 9:44 UTC (permalink / raw)
To: Roman Skakun, Christoph Hellwig
Cc: linux-doc, Peter Zijlstra, Viresh Kumar,
Linux Kernel Mailing List, Paul Mackerras, Jan Beulich,
Will Deacon, Boris Ostrovsky, Marek Szyprowski,
Stefano Stabellini, Jonathan Corbet, xen-devel, Paul E. McKenney,
Konrad Rzeszutek Wilk, Muchun Song, Thomas Gleixner,
Juergen Gross, Thomas Bogendoerfer, Andrii Anisov, Randy Dunlap,
linux-mips, iommu, Roman Skakun, Andrew Morton, Maciej W. Rozycki,
linuxppc-dev, Mike Rapoport, Lu Baolu
In-Reply-To: <CADu_u-OZzgVj+z=iD6kUQOZxUufF5QSMR6-MmpN_hLZ9PyQJhQ@mail.gmail.com>
On 2021-09-17 10:36, Roman Skakun wrote:
> Hi, Christoph
>
> I use Xen PV display. In my case, PV display backend(Dom0) allocates
> contiguous buffer via DMA-API to
> to implement zero-copy between Dom0 and DomU.
Well, something's gone badly wrong there - if you have to shadow the
entire thing in a bounce buffer to import it then it's hardly zero-copy,
is it? If you want to do buffer sharing the buffer really needs to be
allocated appropriately to begin with, such that all relevant devices
can access it directly. That might be something which needs fixing in Xen.
Robin.
> When I start Weston under DomU, I got the next log in Dom0:
> ```
> [ 112.554471] CPU: 0 PID: 367 Comm: weston Tainted: G O
> 5.10.0-yocto-standard+ #312
> [ 112.575149] Call trace:
> [ 112.577666] dump_backtrace+0x0/0x1b0
> [ 112.581373] show_stack+0x18/0x70
> [ 112.584746] dump_stack+0xd0/0x12c
> [ 112.588200] swiotlb_tbl_map_single+0x234/0x360
> [ 112.592781] xen_swiotlb_map_page+0xe4/0x4c0
> [ 112.597095] xen_swiotlb_map_sg+0x84/0x12c
> [ 112.601249] dma_map_sg_attrs+0x54/0x60
> [ 112.605138] vsp1_du_map_sg+0x30/0x60
> [ 112.608851] rcar_du_vsp_map_fb+0x134/0x170
> [ 112.613082] rcar_du_vsp_plane_prepare_fb+0x44/0x64
> [ 112.618007] drm_atomic_helper_prepare_planes+0xac/0x160
> [ 112.623362] drm_atomic_helper_commit+0x88/0x390
> [ 112.628029] drm_atomic_nonblocking_commit+0x4c/0x60
> [ 112.633043] drm_mode_atomic_ioctl+0x9a8/0xb0c
> [ 112.637532] drm_ioctl_kernel+0xc4/0x11c
> [ 112.641506] drm_ioctl+0x21c/0x460
> [ 112.644967] __arm64_sys_ioctl+0xa8/0xf0
> [ 112.648939] el0_svc_common.constprop.0+0x78/0x1a0
> [ 112.653775] do_el0_svc+0x24/0x90
> [ 112.657148] el0_svc+0x14/0x20
> [ 112.660254] el0_sync_handler+0x1a4/0x1b0
> [ 112.664315] el0_sync+0x174/0x180
> [ 112.668145] rcar-fcp fea2f000.fcp: swiotlb buffer is full (sz:
> 3686400 bytes), total 65536 (slots), used 112 (slots)
> ```
> The problem is happened here:
> https://elixir.bootlin.com/linux/v5.14.4/source/drivers/gpu/drm/rcar-du/rcar_du_vsp.c#L202
>
> Sgt was created in dma_get_sgtable() by dma_common_get_sgtable() and
> includes a single page chunk
> as shown here:
> https://elixir.bootlin.com/linux/v5.14.5/source/kernel/dma/ops_helpers.c#L18
>
> After creating a new sgt, we tried to map this sgt through vsp1_du_map_sg().
> Internally, vsp1_du_map_sg() using ops->map_sg (e.g
> xen_swiotlb_map_sg) to perform
> mapping.
>
> I realized that required segment is too big to be fitted to default
> swiotlb segment and condition
> https://elixir.bootlin.com/linux/latest/source/kernel/dma/swiotlb.c#L474
> is always false.
>
> I know that I use a large buffer, but why can't I map this buffer in one chunk?
>
> Thanks!
>
> ср, 15 сент. 2021 г. в 16:53, Christoph Hellwig <hch@lst.de>:
>>
>> On Wed, Sep 15, 2021 at 03:49:52PM +0200, Jan Beulich wrote:
>>> But the question remains: Why does the framebuffer need to be mapped
>>> in a single giant chunk?
>>
>> More importantly: if you use dynamic dma mappings for your framebuffer
>> you're doing something wrong.
>
>
>
^ permalink raw reply
* Re: [PATCH] swiotlb: set IO TLB segment size via cmdline
From: Roman Skakun @ 2021-09-17 9:36 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Roman Skakun, linux-doc, Peter Zijlstra, Viresh Kumar,
Linux Kernel Mailing List, Paul Mackerras, Jan Beulich,
Will Deacon, Boris Ostrovsky, Marek Szyprowski,
Stefano Stabellini, Jonathan Corbet, xen-devel, Paul E. McKenney,
Konrad Rzeszutek Wilk, Muchun Song, Thomas Gleixner,
Juergen Gross, Thomas Bogendoerfer, Andrii Anisov, linuxppc-dev,
Randy Dunlap, linux-mips, iommu, Roman Skakun, Andrew Morton,
Lu Baolu, Robin Murphy, Mike Rapoport, Maciej W. Rozycki
In-Reply-To: <20210915135321.GA15216@lst.de>
Hi, Christoph
I use Xen PV display. In my case, PV display backend(Dom0) allocates
contiguous buffer via DMA-API to
to implement zero-copy between Dom0 and DomU.
When I start Weston under DomU, I got the next log in Dom0:
```
[ 112.554471] CPU: 0 PID: 367 Comm: weston Tainted: G O
5.10.0-yocto-standard+ #312
[ 112.575149] Call trace:
[ 112.577666] dump_backtrace+0x0/0x1b0
[ 112.581373] show_stack+0x18/0x70
[ 112.584746] dump_stack+0xd0/0x12c
[ 112.588200] swiotlb_tbl_map_single+0x234/0x360
[ 112.592781] xen_swiotlb_map_page+0xe4/0x4c0
[ 112.597095] xen_swiotlb_map_sg+0x84/0x12c
[ 112.601249] dma_map_sg_attrs+0x54/0x60
[ 112.605138] vsp1_du_map_sg+0x30/0x60
[ 112.608851] rcar_du_vsp_map_fb+0x134/0x170
[ 112.613082] rcar_du_vsp_plane_prepare_fb+0x44/0x64
[ 112.618007] drm_atomic_helper_prepare_planes+0xac/0x160
[ 112.623362] drm_atomic_helper_commit+0x88/0x390
[ 112.628029] drm_atomic_nonblocking_commit+0x4c/0x60
[ 112.633043] drm_mode_atomic_ioctl+0x9a8/0xb0c
[ 112.637532] drm_ioctl_kernel+0xc4/0x11c
[ 112.641506] drm_ioctl+0x21c/0x460
[ 112.644967] __arm64_sys_ioctl+0xa8/0xf0
[ 112.648939] el0_svc_common.constprop.0+0x78/0x1a0
[ 112.653775] do_el0_svc+0x24/0x90
[ 112.657148] el0_svc+0x14/0x20
[ 112.660254] el0_sync_handler+0x1a4/0x1b0
[ 112.664315] el0_sync+0x174/0x180
[ 112.668145] rcar-fcp fea2f000.fcp: swiotlb buffer is full (sz:
3686400 bytes), total 65536 (slots), used 112 (slots)
```
The problem is happened here:
https://elixir.bootlin.com/linux/v5.14.4/source/drivers/gpu/drm/rcar-du/rcar_du_vsp.c#L202
Sgt was created in dma_get_sgtable() by dma_common_get_sgtable() and
includes a single page chunk
as shown here:
https://elixir.bootlin.com/linux/v5.14.5/source/kernel/dma/ops_helpers.c#L18
After creating a new sgt, we tried to map this sgt through vsp1_du_map_sg().
Internally, vsp1_du_map_sg() using ops->map_sg (e.g
xen_swiotlb_map_sg) to perform
mapping.
I realized that required segment is too big to be fitted to default
swiotlb segment and condition
https://elixir.bootlin.com/linux/latest/source/kernel/dma/swiotlb.c#L474
is always false.
I know that I use a large buffer, but why can't I map this buffer in one chunk?
Thanks!
ср, 15 сент. 2021 г. в 16:53, Christoph Hellwig <hch@lst.de>:
>
> On Wed, Sep 15, 2021 at 03:49:52PM +0200, Jan Beulich wrote:
> > But the question remains: Why does the framebuffer need to be mapped
> > in a single giant chunk?
>
> More importantly: if you use dynamic dma mappings for your framebuffer
> you're doing something wrong.
--
Best Regards, Roman.
^ permalink raw reply
* [PATCH] powerpc/8xx: Simplify TLB handling
From: Christophe Leroy @ 2021-09-17 9:16 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In the old days, TLB handling for 8xx was using tlbie and tlbia
instructions directly as much as possible.
But commit f048aace29e0 ("powerpc/mm: Add SMP support to no-hash
TLB handling") broke that by introducing out-of-line unnecessary
complex functions for booke/smp which don't have tlbie/tlbia
instructions and require more complex handling.
Restore direct use of tlbie and tlbia for 8xx which is never SMP.
With this patch we now get
c00ecc68 <ptep_clear_flush>:
c00ecc68: 39 00 00 00 li r8,0
c00ecc6c: 81 46 00 00 lwz r10,0(r6)
c00ecc70: 91 06 00 00 stw r8,0(r6)
c00ecc74: 7c 00 2a 64 tlbie r5,r0
c00ecc78: 7c 00 04 ac hwsync
c00ecc7c: 91 43 00 00 stw r10,0(r3)
c00ecc80: 4e 80 00 20 blr
Before it was
c0012880 <local_flush_tlb_page>:
c0012880: 2c 03 00 00 cmpwi r3,0
c0012884: 41 82 00 54 beq c00128d8 <local_flush_tlb_page+0x58>
c0012888: 81 22 00 00 lwz r9,0(r2)
c001288c: 81 43 00 20 lwz r10,32(r3)
c0012890: 39 29 00 01 addi r9,r9,1
c0012894: 91 22 00 00 stw r9,0(r2)
c0012898: 2c 0a 00 00 cmpwi r10,0
c001289c: 41 82 00 10 beq c00128ac <local_flush_tlb_page+0x2c>
c00128a0: 81 2a 01 dc lwz r9,476(r10)
c00128a4: 2c 09 ff ff cmpwi r9,-1
c00128a8: 41 82 00 0c beq c00128b4 <local_flush_tlb_page+0x34>
c00128ac: 7c 00 22 64 tlbie r4,r0
c00128b0: 7c 00 04 ac hwsync
c00128b4: 81 22 00 00 lwz r9,0(r2)
c00128b8: 39 29 ff ff addi r9,r9,-1
c00128bc: 2c 09 00 00 cmpwi r9,0
c00128c0: 91 22 00 00 stw r9,0(r2)
c00128c4: 4c a2 00 20 bclr+ 4,eq
c00128c8: 81 22 00 70 lwz r9,112(r2)
c00128cc: 71 29 00 04 andi. r9,r9,4
c00128d0: 4d 82 00 20 beqlr
c00128d4: 48 65 76 74 b c0669f48 <preempt_schedule>
c00128d8: 81 22 00 00 lwz r9,0(r2)
c00128dc: 39 29 00 01 addi r9,r9,1
c00128e0: 91 22 00 00 stw r9,0(r2)
c00128e4: 4b ff ff c8 b c00128ac <local_flush_tlb_page+0x2c>
...
c00ecdc8 <ptep_clear_flush>:
c00ecdc8: 94 21 ff f0 stwu r1,-16(r1)
c00ecdcc: 39 20 00 00 li r9,0
c00ecdd0: 93 c1 00 08 stw r30,8(r1)
c00ecdd4: 83 c6 00 00 lwz r30,0(r6)
c00ecdd8: 91 26 00 00 stw r9,0(r6)
c00ecddc: 93 e1 00 0c stw r31,12(r1)
c00ecde0: 7c 08 02 a6 mflr r0
c00ecde4: 7c 7f 1b 78 mr r31,r3
c00ecde8: 7c 83 23 78 mr r3,r4
c00ecdec: 7c a4 2b 78 mr r4,r5
c00ecdf0: 90 01 00 14 stw r0,20(r1)
c00ecdf4: 4b f2 5a 8d bl c0012880 <local_flush_tlb_page>
c00ecdf8: 93 df 00 00 stw r30,0(r31)
c00ecdfc: 7f e3 fb 78 mr r3,r31
c00ece00: 80 01 00 14 lwz r0,20(r1)
c00ece04: 83 c1 00 08 lwz r30,8(r1)
c00ece08: 83 e1 00 0c lwz r31,12(r1)
c00ece0c: 7c 08 03 a6 mtlr r0
c00ece10: 38 21 00 10 addi r1,r1,16
c00ece14: 4e 80 00 20 blr
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/nohash/tlbflush.h | 20 ++++++++++++++++++++
arch/powerpc/mm/nohash/tlb.c | 2 ++
2 files changed, 22 insertions(+)
diff --git a/arch/powerpc/include/asm/nohash/tlbflush.h b/arch/powerpc/include/asm/nohash/tlbflush.h
index 1edb7243e515..a7ec171d79a6 100644
--- a/arch/powerpc/include/asm/nohash/tlbflush.h
+++ b/arch/powerpc/include/asm/nohash/tlbflush.h
@@ -32,11 +32,31 @@ extern void flush_tlb_range(struct vm_area_struct *vma, unsigned long start,
unsigned long end);
extern void flush_tlb_kernel_range(unsigned long start, unsigned long end);
+#ifdef CONFIG_PPC_8xx
+#include <asm/trace.h>
+
+static inline void local_flush_tlb_mm(struct mm_struct *mm)
+{
+ unsigned int pid = READ_ONCE(mm->context.id);
+
+ if (pid != MMU_NO_CONTEXT) {
+ asm volatile ("sync; tlbia; isync" : : : "memory");
+ trace_tlbia(pid);
+ }
+}
+
+static inline void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long vmaddr)
+{
+ asm volatile ("tlbie %0; sync" : : "r" (vmaddr) : "memory");
+ trace_tlbie(0, 0, vmaddr, vma ? vma->vm_mm->context.id : 0, 0, 0, 0);
+}
+#else
extern void local_flush_tlb_mm(struct mm_struct *mm);
extern void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long vmaddr);
extern void __local_flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr,
int tsize, int ind);
+#endif
#ifdef CONFIG_SMP
extern void flush_tlb_mm(struct mm_struct *mm);
diff --git a/arch/powerpc/mm/nohash/tlb.c b/arch/powerpc/mm/nohash/tlb.c
index 5872f69141d5..6d8cb68f6efb 100644
--- a/arch/powerpc/mm/nohash/tlb.c
+++ b/arch/powerpc/mm/nohash/tlb.c
@@ -185,6 +185,7 @@ EXPORT_PER_CPU_SYMBOL(next_tlbcam_idx);
* processor
*/
+#ifndef CONFIG_PPC_8xx
/*
* These are the base non-SMP variants of page and mm flushing
*/
@@ -218,6 +219,7 @@ void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long vmaddr)
mmu_get_tsize(mmu_virtual_psize), 0);
}
EXPORT_SYMBOL(local_flush_tlb_page);
+#endif
/*
* And here are the SMP non-local implementations
--
2.31.1
^ permalink raw reply related
* Re: [PATCH v2] powerpc/mce: Fix access error in mce handler
From: Ganesh @ 2021-09-17 8:40 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev, mpe; +Cc: mahesh, npiggin
In-Reply-To: <87lf3v903y.fsf@linkitivity.dja.id.au>
[-- Attachment #1: Type: text/plain, Size: 2655 bytes --]
On 9/17/21 12:09 PM, Daniel Axtens wrote:
> Hi Ganesh,
>
>> We queue an irq work for deferred processing of mce event
>> in realmode mce handler, where translation is disabled.
>> Queuing of the work may result in accessing memory outside
>> RMO region, such access needs the translation to be enabled
>> for an LPAR running with hash mmu else the kernel crashes.
>>
>> After enabling translation in mce_handle_error() we used to
>> leave it enabled to avoid crashing here, but now with the
>> commit 74c3354bc1d89 ("powerpc/pseries/mce: restore msr before
>> returning from handler") we are restoring the MSR to disable
>> translation.
>>
>> Hence to fix this enable the translation before queuing the work.
> [snip]
>
>> Fixes: 74c3354bc1d89 ("powerpc/pseries/mce: restore msr before returning from handler")
> That patch changes arch/powerpc/powerpc/platforms/pseries/ras.c just
> below this comment:
>
> /*
> * Enable translation as we will be accessing per-cpu variables
> * in save_mce_event() which may fall outside RMO region, also
> * leave it enabled because subsequently we will be queuing work
> * to workqueues where again per-cpu variables accessed, besides
> * fwnmi_release_errinfo() crashes when called in realmode on
> * pseries.
> * Note: All the realmode handling like flushing SLB entries for
> * SLB multihit is done by now.
> */
>
> That suggests per-cpu variables need protection. In your patch, you
> enable translations just around irq_work_queue:
The comment is bit old, most of it doesn't make any sense now, yes per-cpu
variables cannot be accessed in realmode, but with commit 923b3cf00b3f
("powerpc/mce: Remove per cpu variables from MCE handlers") we moved all of
them to paca.
>> + /* Queue irq work to process this event later. Before
>> + * queuing the work enable translation for non radix LPAR,
>> + * as irq_work_queue may try to access memory outside RMO
>> + * region.
>> + */
>> + if (!radix_enabled() && firmware_has_feature(FW_FEATURE_LPAR)) {
>> + msr = mfmsr();
>> + mtmsr(msr | MSR_IR | MSR_DR);
>> + irq_work_queue(&mce_event_process_work);
>> + mtmsr(msr);
>> + } else {
>> + irq_work_queue(&mce_event_process_work);
>> + }
> However, just before that in the function, there are a few things that
> access per-cpu variables via the local_paca, e.g.:
>
> memcpy(&local_paca->mce_info->mce_event_queue[index],
> &evt, sizeof(evt));
>
> Do we need to widen the window where translations are enabled in order
> to protect accesses to local_paca?
paca will be within Real Mode Area, so it can be accessed with translate off.
[-- Attachment #2: Type: text/html, Size: 3620 bytes --]
^ permalink raw reply
* Re: [PATCH v1 1/2] powerpc/64s: system call rfscv workaround for TM bugs
From: Daniel Axtens @ 2021-09-17 8:02 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Eirik Fuller, Nicholas Piggin, kvm-ppc
In-Reply-To: <20210908101718.118522-1-npiggin@gmail.com>
Nicholas Piggin <npiggin@gmail.com> writes:
> The rfscv instruction does not work correctly with the fake-suspend mode
> in POWER9, which can end up with the hypervisor restoring an incorrect
> checkpoint.
If I understand correctly from commit 4bb3c7a0208f ("KVM: PPC: Book3S
HV: Work around transactional memory bugs in POWER9"), this is because
rfscv does not cause a soft-patch interrupt in the way that rfid etc do.
So we need to avoid calling rfscv if we are in fake-suspend state -
instead we must call something that does indeed get soft-patched - like
rfid.
> Work around this by setting the _TIF_RESTOREALL flag if a system call
> returns to a transaction active state, causing rfid to be used instead
> of rfscv to return, which will do the right thing. The contents of the
> registers are irrelevant because they will be overwritten in this case
> anyway.
I can follow that this will indeed cause syscall_exit_prepare to return
non-zero and therefore we should take the
syscall_vectored_*_restore_regs path which does an RFID_TO_USER rather
than a RFSCV_TO_USER. My only question/concern is:
.Lsyscall_vectored_\name\()_exit:
addi r4,r1,STACK_FRAME_OVERHEAD
li r5,1 /* scv */
bl syscall_exit_prepare <-------- we get r3 != 0 here
std r1,PACA_EXIT_SAVE_R1(r13) /* save r1 for restart */
.Lsyscall_vectored_\name\()_rst_start:
lbz r11,PACAIRQHAPPENED(r13)
andi. r11,r11,(~PACA_IRQ_HARD_DIS)@l
bne- syscall_vectored_\name\()_restart <-- can we end up taking
this branch?
Are there any circumstances that would take us down the _restart path,
and if so, will we still go through the correct RFID_TO_USER branch
rather than the RFSCV_TO_USER branch?
Apart from that this looks good to me, although with the heavy
disclaimer that I only learned about fake suspend for the first time
while reviewing the patch.
Kind regards,
Daniel
>
> Reported-by: Eirik Fuller <efuller@redhat.com>
> Fixes: 7fa95f9adaee7 ("powerpc/64s: system call support for scv/rfscv instructions")
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/kernel/interrupt.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index c77c80214ad3..917a2ac4def6 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -139,6 +139,19 @@ notrace long system_call_exception(long r3, long r4, long r5,
> */
> irq_soft_mask_regs_set_state(regs, IRQS_ENABLED);
>
> + /*
> + * If system call is called with TM active, set _TIF_RESTOREALL to
> + * prevent RFSCV being used to return to userspace, because POWER9
> + * TM implementation has problems with this instruction returning to
> + * transactional state. Final register values are not relevant because
> + * the transaction will be aborted upon return anyway. Or in the case
> + * of unsupported_scv SIGILL fault, the return state does not much
> + * matter because it's an edge case.
> + */
> + if (IS_ENABLED(CONFIG_PPC_TRANSACTIONAL_MEM) &&
> + unlikely(MSR_TM_TRANSACTIONAL(regs->msr)))
> + current_thread_info()->flags |= _TIF_RESTOREALL;
> +
> /*
> * If the system call was made with a transaction active, doom it and
> * return without performing the system call. Unless it was an
> --
> 2.23.0
^ permalink raw reply
* Re: [PATCH v5 6/6] sched/fair: Consider SMT in ASYM_PACKING load balance
From: Vincent Guittot @ 2021-09-17 7:41 UTC (permalink / raw)
To: Ricardo Neri
Cc: Juri Lelli, Aubrey Li, Srikar Dronamraju, Ravi V. Shankar,
Peter Zijlstra (Intel), Ricardo Neri, Ben Segall,
Srinivas Pandruvada, Joel Fernandes (Google), Ingo Molnar,
Rafael J . Wysocki, Steven Rostedt, Mel Gorman, Len Brown,
Nicholas Piggin, Aubrey Li, Dietmar Eggemann, Tim Chen,
Quentin Perret, Daniel Bristot de Oliveira, linux-kernel,
linuxppc-dev
In-Reply-To: <20210917010044.GA23727@ranerica-svr.sc.intel.com>
On Fri, 17 Sept 2021 at 03:01, Ricardo Neri
<ricardo.neri-calderon@linux.intel.com> wrote:
>
> On Wed, Sep 15, 2021 at 05:43:44PM +0200, Vincent Guittot wrote:
> > On Sat, 11 Sept 2021 at 03:19, Ricardo Neri
> > <ricardo.neri-calderon@linux.intel.com> wrote:
> > >
> > > When deciding to pull tasks in ASYM_PACKING, it is necessary not only to
> > > check for the idle state of the destination CPU, dst_cpu, but also of
> > > its SMT siblings.
> > >
> > > If dst_cpu is idle but its SMT siblings are busy, performance suffers
> > > if it pulls tasks from a medium priority CPU that does not have SMT
> > > siblings.
> > >
> > > Implement asym_smt_can_pull_tasks() to inspect the state of the SMT
> > > siblings of both dst_cpu and the CPUs in the candidate busiest group.
> > >
> > > Cc: Aubrey Li <aubrey.li@intel.com>
> > > Cc: Ben Segall <bsegall@google.com>
> > > Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
> > > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> > > Cc: Mel Gorman <mgorman@suse.de>
> > > Cc: Quentin Perret <qperret@google.com>
> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > Cc: Tim Chen <tim.c.chen@linux.intel.com>
> > > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > Reviewed-by: Len Brown <len.brown@intel.com>
> > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > > ---
> > > Changes since v4:
> > > * Use sg_lb_stats::sum_nr_running the idle state of a scheduling group.
> > > (Vincent, Peter)
> > > * Do not even idle CPUs in asym_smt_can_pull_tasks(). (Vincent)
> > > * Updated function documentation and corrected a typo.
> > >
> > > Changes since v3:
> > > * Removed the arch_asym_check_smt_siblings() hook. Discussions with the
> > > powerpc folks showed that this patch should not impact them. Also, more
> > > recent powerpc processor no longer use asym_packing. (PeterZ)
> > > * Removed unnecessary local variable in asym_can_pull_tasks(). (Dietmar)
> > > * Removed unnecessary check for local CPUs when the local group has zero
> > > utilization. (Joel)
> > > * Renamed asym_can_pull_tasks() as asym_smt_can_pull_tasks() to reflect
> > > the fact that it deals with SMT cases.
> > > * Made asym_smt_can_pull_tasks() return false for !CONFIG_SCHED_SMT so
> > > that callers can deal with non-SMT cases.
> > >
> > > Changes since v2:
> > > * Reworded the commit message to reflect updates in code.
> > > * Corrected misrepresentation of dst_cpu as the CPU doing the load
> > > balancing. (PeterZ)
> > > * Removed call to arch_asym_check_smt_siblings() as it is now called in
> > > sched_asym().
> > >
> > > Changes since v1:
> > > * Don't bailout in update_sd_pick_busiest() if dst_cpu cannot pull
> > > tasks. Instead, reclassify the candidate busiest group, as it
> > > may still be selected. (PeterZ)
> > > * Avoid an expensive and unnecessary call to cpumask_weight() when
> > > determining if a sched_group is comprised of SMT siblings.
> > > (PeterZ).
> > > ---
> > > kernel/sched/fair.c | 94 +++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 94 insertions(+)
> > >
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index 26db017c14a3..8d763dd0174b 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -8597,10 +8597,98 @@ group_type group_classify(unsigned int imbalance_pct,
> > > return group_has_spare;
> > > }
> > >
> > > +/**
> > > + * asym_smt_can_pull_tasks - Check whether the load balancing CPU can pull tasks
> > > + * @dst_cpu: Destination CPU of the load balancing
> > > + * @sds: Load-balancing data with statistics of the local group
> > > + * @sgs: Load-balancing statistics of the candidate busiest group
> > > + * @sg: The candidate busiest group
> > > + *
> > > + * Check the state of the SMT siblings of both @sds::local and @sg and decide
> > > + * if @dst_cpu can pull tasks.
> > > + *
> > > + * If @dst_cpu does not have SMT siblings, it can pull tasks if two or more of
> > > + * the SMT siblings of @sg are busy. If only one CPU in @sg is busy, pull tasks
> > > + * only if @dst_cpu has higher priority.
> > > + *
> > > + * If both @dst_cpu and @sg have SMT siblings, and @sg has exactly one more
> > > + * busy CPU than @sds::local, let @dst_cpu pull tasks if it has higher priority.
> > > + * Bigger imbalances in the number of busy CPUs will be dealt with in
> > > + * update_sd_pick_busiest().
> > > + *
> > > + * If @sg does not have SMT siblings, only pull tasks if all of the SMT siblings
> > > + * of @dst_cpu are idle and @sg has lower priority.
> > > + */
> > > +static bool asym_smt_can_pull_tasks(int dst_cpu, struct sd_lb_stats *sds,
> > > + struct sg_lb_stats *sgs,
> > > + struct sched_group *sg)
> > > +{
> > > +#ifdef CONFIG_SCHED_SMT
> > > + bool local_is_smt, sg_is_smt;
> > > + int sg_busy_cpus;
> > > +
> > > + local_is_smt = sds->local->flags & SD_SHARE_CPUCAPACITY;
> > > + sg_is_smt = sg->flags & SD_SHARE_CPUCAPACITY;
> > > +
> > > + sg_busy_cpus = sgs->group_weight - sgs->idle_cpus;
> > > +
> > > + if (!local_is_smt) {
> > > + /*
> > > + * If we are here, @dst_cpu is idle and does not have SMT
> > > + * siblings. Pull tasks if candidate group has two or more
> > > + * busy CPUs.
> > > + */
> > > + if (sg_is_smt && sg_busy_cpus >= 2)
> >
> > Do you really need to test sg_is_smt ? if sg_busy_cpus >= 2 then
> > sd_is_smt must be true ?
>
> Thank you very much for your feedback Vincent!
>
> Yes, it is true that sg_busy_cpus >=2 is only true if @sg is SMT. I will
> remove this check.
>
> >
> > Also, This is the default behavior where we want to even the number of
> > busy cpu. Shouldn't you return false and fall back to the default
> > behavior ?
>
> This is also true.
>
> >
> > That being said, the default behavior tries to even the number of idle
> > cpus which is easier to compute and is equal to even the number of
> > busy cpus in "normal" system with the same number of cpus in groups
> > but this is not the case here. It could be good to change the default
> > behavior to even the number of busy cpus and that you use the default
> > behavior here. Additional condition will be used to select the busiest
> > group like more busy cpu or more number of running tasks
>
> That is a very good observation. Checking the number of idle CPUs
> assumes that both groups have the same number of CPUs. I'll look into
> modifying the default behavior.
>
> >
> > > + return true;
> > > +
> > > + /*
> > > + * @dst_cpu does not have SMT siblings. @sg may have SMT
> > > + * siblings and only one is busy. In such case, @dst_cpu
> > > + * can help if it has higher priority and is idle (i.e.,
> > > + * it has no running tasks).
> >
> > The previous comment above assume that "@dst_cpu is idle" but now you
> > need to check that sds->local_stat.sum_nr_running == 0
>
> But we already know that, right? We are here because in
> update_sg_lb_stats() we determine that dst CPU is idle (env->idle !=
> CPU_NOT_IDLE).
That's my point:
Why do you add the condition !sds->local_stat.sum_nr_running below ? I
assume that it's to check that the cpu is idle, isn't it ?
> > > + */
> > > + return !sds->local_stat.sum_nr_running &&
> > > + sched_asym_prefer(dst_cpu, sg->asym_prefer_cpu);
> > > + }
>
> Thanks and BR,
> Ricardo
^ permalink raw reply
* [PATCH v5 3/5] signal: Add unsafe_copy_siginfo_to_user()
From: Christophe Leroy @ 2021-09-17 6:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
ebiederm, hch
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1718f38859d5366f82d5bef531f255cedf537b5d.1631861883.git.christophe.leroy@csgroup.eu>
In the same spirit as commit fb05121fd6a2 ("signal: Add
unsafe_get_compat_sigset()"), implement an 'unsafe' version of
copy_siginfo_to_user() in order to use it within user access blocks.
For that, also add an 'unsafe' version of clear_user().
This commit adds the generic fallback for unsafe_clear_user().
Architectures wanting to use unsafe_copy_siginfo_to_user() within a
user_access_begin() section have to make sure they have their
own unsafe_clear_user().
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v3: Added precision about unsafe_clear_user() in commit message.
---
include/linux/signal.h | 15 +++++++++++++++
include/linux/uaccess.h | 1 +
kernel/signal.c | 5 -----
3 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/include/linux/signal.h b/include/linux/signal.h
index 3f96a6374e4f..70ea7e741427 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -35,6 +35,21 @@ static inline void copy_siginfo_to_external(siginfo_t *to,
int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from);
int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from);
+static __always_inline char __user *si_expansion(const siginfo_t __user *info)
+{
+ return ((char __user *)info) + sizeof(struct kernel_siginfo);
+}
+
+#define unsafe_copy_siginfo_to_user(to, from, label) do { \
+ siginfo_t __user *__ucs_to = to; \
+ const kernel_siginfo_t *__ucs_from = from; \
+ char __user *__ucs_expansion = si_expansion(__ucs_to); \
+ \
+ unsafe_copy_to_user(__ucs_to, __ucs_from, \
+ sizeof(struct kernel_siginfo), label); \
+ unsafe_clear_user(__ucs_expansion, SI_EXPANSION_SIZE, label); \
+} while (0)
+
enum siginfo_layout {
SIL_KILL,
SIL_TIMER,
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index c05e903cef02..37073caac474 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -398,6 +398,7 @@ long strnlen_user_nofault(const void __user *unsafe_addr, long count);
#define unsafe_put_user(x,p,e) unsafe_op_wrap(__put_user(x,p),e)
#define unsafe_copy_to_user(d,s,l,e) unsafe_op_wrap(__copy_to_user(d,s,l),e)
#define unsafe_copy_from_user(d,s,l,e) unsafe_op_wrap(__copy_from_user(d,s,l),e)
+#define unsafe_clear_user(d, l, e) unsafe_op_wrap(__clear_user(d, l), e)
static inline unsigned long user_access_save(void) { return 0UL; }
static inline void user_access_restore(unsigned long flags) { }
#endif
diff --git a/kernel/signal.c b/kernel/signal.c
index 952741f6d0f9..23f168730b7e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3324,11 +3324,6 @@ enum siginfo_layout siginfo_layout(unsigned sig, int si_code)
return layout;
}
-static inline char __user *si_expansion(const siginfo_t __user *info)
-{
- return ((char __user *)info) + sizeof(struct kernel_siginfo);
-}
-
int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from)
{
char __user *expansion = si_expansion(to);
--
2.31.1
^ permalink raw reply related
* [PATCH v5 5/5] powerpc/signal: Use unsafe_copy_siginfo_to_user()
From: Christophe Leroy @ 2021-09-17 6:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
ebiederm, hch
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1718f38859d5366f82d5bef531f255cedf537b5d.1631861883.git.christophe.leroy@csgroup.eu>
Use unsafe_copy_siginfo_to_user() in order to do the copy
within the user access block.
On an mpc 8321 (book3s/32) the improvment is about 5% on a process
sending a signal to itself.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v5: Added missing __user flag when calling unsafe_copy_siginfo_to_user()
v4: Use another approach for compat: drop the unsafe_copy_siginfo_to_user32(), instead directly call copy_siginfo_to_external32() before user_access_begin()
v3: Don't leave compat aside, use the new unsafe_copy_siginfo_to_user32()
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/signal_32.c | 17 ++++++++---------
arch/powerpc/kernel/signal_64.c | 5 +----
2 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index ff101e2b3bab..0baf3c10b6c0 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -710,12 +710,6 @@ static long restore_tm_user_regs(struct pt_regs *regs, struct mcontext __user *s
}
#endif
-#ifdef CONFIG_PPC64
-
-#define copy_siginfo_to_user copy_siginfo_to_user32
-
-#endif /* CONFIG_PPC64 */
-
/*
* Set up a signal frame for a "real-time" signal handler
* (one which gets siginfo).
@@ -731,6 +725,7 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
struct pt_regs *regs = tsk->thread.regs;
/* Save the thread's msr before get_tm_stackpointer() changes it */
unsigned long msr = regs->msr;
+ compat_siginfo_t uinfo;
/* Set up Signal Frame */
frame = get_sigframe(ksig, tsk, sizeof(*frame), 1);
@@ -744,6 +739,9 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
else
prepare_save_user_regs(1);
+ if (IS_ENABLED(CONFIG_COMPAT))
+ copy_siginfo_to_external32(&uinfo, &ksig->info);
+
if (!user_access_begin(newsp, __SIGNAL_FRAMESIZE + 16 + sizeof(*frame)))
goto badframe;
@@ -779,15 +777,16 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
asm("dcbst %y0; sync; icbi %y0; sync" :: "Z" (mctx->mc_pad[0]));
}
unsafe_put_sigset_t(&frame->uc.uc_sigmask, oldset, failed);
+ if (IS_ENABLED(CONFIG_COMPAT))
+ unsafe_copy_to_user(&frame->info, &uinfo, sizeof(frame->info), failed);
+ else
+ unsafe_copy_siginfo_to_user((void __user *)&frame->info, &ksig->info, failed);
/* create a stack frame for the caller of the handler */
unsafe_put_user(regs->gpr[1], newsp, failed);
user_access_end();
- if (copy_siginfo_to_user(&frame->info, &ksig->info))
- goto badframe;
-
regs->link = tramp;
#ifdef CONFIG_PPC_FPU_REGS
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index d80ff83cacb9..56c0c74aa28c 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -901,15 +901,12 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
}
unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
+ unsafe_copy_siginfo_to_user(&frame->info, &ksig->info, badframe_block);
/* Allocate a dummy caller frame for the signal handler. */
unsafe_put_user(regs->gpr[1], newsp, badframe_block);
user_write_access_end();
- /* Save the siginfo outside of the unsafe block. */
- if (copy_siginfo_to_user(&frame->info, &ksig->info))
- goto badframe;
-
/* Make sure signal handler doesn't get spurious FP exceptions */
tsk->thread.fp_state.fpscr = 0;
--
2.31.1
^ permalink raw reply related
* [PATCH v5 2/5] powerpc/signal: Include the new stack frame inside the user access block
From: Christophe Leroy @ 2021-09-17 6:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
ebiederm, hch
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1718f38859d5366f82d5bef531f255cedf537b5d.1631861883.git.christophe.leroy@csgroup.eu>
Include the new stack frame inside the user access block and set it up
using unsafe_put_user().
On an mpc 8321 (book3s/32) the improvment is about 4% on a process
sending a signal to itself.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/signal_32.c | 29 +++++++++++++----------------
arch/powerpc/kernel/signal_64.c | 14 +++++++-------
2 files changed, 20 insertions(+), 23 deletions(-)
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 0608581967f0..ff101e2b3bab 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -726,7 +726,7 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
struct rt_sigframe __user *frame;
struct mcontext __user *mctx;
struct mcontext __user *tm_mctx = NULL;
- unsigned long newsp = 0;
+ unsigned long __user *newsp;
unsigned long tramp;
struct pt_regs *regs = tsk->thread.regs;
/* Save the thread's msr before get_tm_stackpointer() changes it */
@@ -734,6 +734,7 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
/* Set up Signal Frame */
frame = get_sigframe(ksig, tsk, sizeof(*frame), 1);
+ newsp = (unsigned long __user *)((unsigned long)frame - (__SIGNAL_FRAMESIZE + 16));
mctx = &frame->uc.uc_mcontext;
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
tm_mctx = &frame->uc_transact.uc_mcontext;
@@ -743,7 +744,7 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
else
prepare_save_user_regs(1);
- if (!user_access_begin(frame, sizeof(*frame)))
+ if (!user_access_begin(newsp, __SIGNAL_FRAMESIZE + 16 + sizeof(*frame)))
goto badframe;
/* Put the siginfo & fill in most of the ucontext */
@@ -779,6 +780,9 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
}
unsafe_put_sigset_t(&frame->uc.uc_sigmask, oldset, failed);
+ /* create a stack frame for the caller of the handler */
+ unsafe_put_user(regs->gpr[1], newsp, failed);
+
user_access_end();
if (copy_siginfo_to_user(&frame->info, &ksig->info))
@@ -790,13 +794,8 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
tsk->thread.fp_state.fpscr = 0; /* turn off all fp exceptions */
#endif
- /* create a stack frame for the caller of the handler */
- newsp = ((unsigned long)frame) - (__SIGNAL_FRAMESIZE + 16);
- if (put_user(regs->gpr[1], (u32 __user *)newsp))
- goto badframe;
-
/* Fill registers for signal handler */
- regs->gpr[1] = newsp;
+ regs->gpr[1] = (unsigned long)newsp;
regs->gpr[3] = ksig->sig;
regs->gpr[4] = (unsigned long)&frame->info;
regs->gpr[5] = (unsigned long)&frame->uc;
@@ -826,7 +825,7 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
struct sigframe __user *frame;
struct mcontext __user *mctx;
struct mcontext __user *tm_mctx = NULL;
- unsigned long newsp = 0;
+ unsigned long __user *newsp;
unsigned long tramp;
struct pt_regs *regs = tsk->thread.regs;
/* Save the thread's msr before get_tm_stackpointer() changes it */
@@ -834,6 +833,7 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
/* Set up Signal Frame */
frame = get_sigframe(ksig, tsk, sizeof(*frame), 1);
+ newsp = (unsigned long __user *)((unsigned long)frame - __SIGNAL_FRAMESIZE);
mctx = &frame->mctx;
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
tm_mctx = &frame->mctx_transact;
@@ -843,7 +843,7 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
else
prepare_save_user_regs(1);
- if (!user_access_begin(frame, sizeof(*frame)))
+ if (!user_access_begin(newsp, __SIGNAL_FRAMESIZE + sizeof(*frame)))
goto badframe;
sc = (struct sigcontext __user *) &frame->sctx;
@@ -873,6 +873,8 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
unsafe_put_user(PPC_RAW_SC(), &mctx->mc_pad[1], failed);
asm("dcbst %y0; sync; icbi %y0; sync" :: "Z" (mctx->mc_pad[0]));
}
+ /* create a stack frame for the caller of the handler */
+ unsafe_put_user(regs->gpr[1], newsp, failed);
user_access_end();
regs->link = tramp;
@@ -881,12 +883,7 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
tsk->thread.fp_state.fpscr = 0; /* turn off all fp exceptions */
#endif
- /* create a stack frame for the caller of the handler */
- newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE;
- if (put_user(regs->gpr[1], (u32 __user *)newsp))
- goto badframe;
-
- regs->gpr[1] = newsp;
+ regs->gpr[1] = (unsigned long)newsp;
regs->gpr[3] = ksig->sig;
regs->gpr[4] = (unsigned long) sc;
regs_set_return_ip(regs, (unsigned long) ksig->ka.sa.sa_handler);
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 7b1cd50bc4fb..d80ff83cacb9 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -847,13 +847,14 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
struct task_struct *tsk)
{
struct rt_sigframe __user *frame;
- unsigned long newsp = 0;
+ unsigned long __user *newsp;
long err = 0;
struct pt_regs *regs = tsk->thread.regs;
/* Save the thread's msr before get_tm_stackpointer() changes it */
unsigned long msr = regs->msr;
frame = get_sigframe(ksig, tsk, sizeof(*frame), 0);
+ newsp = (unsigned long __user *)((unsigned long)frame - __SIGNAL_FRAMESIZE);
/*
* This only applies when calling unsafe_setup_sigcontext() and must be
@@ -862,7 +863,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
if (!MSR_TM_ACTIVE(msr))
prepare_setup_sigcontext(tsk);
- if (!user_write_access_begin(frame, sizeof(*frame)))
+ if (!user_write_access_begin(newsp, __SIGNAL_FRAMESIZE + sizeof(*frame)))
goto badframe;
unsafe_put_user(&frame->info, &frame->pinfo, badframe_block);
@@ -900,6 +901,9 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
}
unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
+ /* Allocate a dummy caller frame for the signal handler. */
+ unsafe_put_user(regs->gpr[1], newsp, badframe_block);
+
user_write_access_end();
/* Save the siginfo outside of the unsafe block. */
@@ -919,10 +923,6 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
regs_set_return_ip(regs, (unsigned long) &frame->tramp[0]);
}
- /* Allocate a dummy caller frame for the signal handler. */
- newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE;
- err |= put_user(regs->gpr[1], (unsigned long __user *)newsp);
-
/* Set up "regs" so we "return" to the signal handler. */
if (is_elf2_task()) {
regs->ctr = (unsigned long) ksig->ka.sa.sa_handler;
@@ -947,7 +947,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
/* enter the signal handler in native-endian mode */
regs_set_return_msr(regs, (regs->msr & ~MSR_LE) | (MSR_KERNEL & MSR_LE));
- regs->gpr[1] = newsp;
+ regs->gpr[1] = (unsigned long)newsp;
regs->gpr[3] = ksig->sig;
regs->result = 0;
if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
--
2.31.1
^ permalink raw reply related
* [PATCH v5 4/5] powerpc/uaccess: Add unsafe_clear_user()
From: Christophe Leroy @ 2021-09-17 6:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
ebiederm, hch
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1718f38859d5366f82d5bef531f255cedf537b5d.1631861883.git.christophe.leroy@csgroup.eu>
Implement unsafe_clear_user() for powerpc.
It's a copy/paste of unsafe_copy_to_user() with value 0 as source.
It may be improved in a later patch by using 'dcbz' instruction
to zeroize full cache lines at once.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/uaccess.h | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 22c79ab40006..962b675485ff 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -467,6 +467,26 @@ do { \
unsafe_put_user(*(u8*)(_src + _i), (u8 __user *)(_dst + _i), e); \
} while (0)
+#define unsafe_clear_user(d, l, e) \
+do { \
+ u8 __user *_dst = (u8 __user *)(d); \
+ size_t _len = (l); \
+ int _i; \
+ \
+ for (_i = 0; _i < (_len & ~(sizeof(u64) - 1)); _i += sizeof(u64)) \
+ unsafe_put_user(0, (u64 __user *)(_dst + _i), e); \
+ if (_len & 4) { \
+ unsafe_put_user(0, (u32 __user *)(_dst + _i), e); \
+ _i += 4; \
+ } \
+ if (_len & 2) { \
+ unsafe_put_user(0, (u16 __user *)(_dst + _i), e); \
+ _i += 2; \
+ } \
+ if (_len & 1) \
+ unsafe_put_user(0, (u8 __user *)(_dst + _i), e); \
+} while (0)
+
#define HAVE_GET_KERNEL_NOFAULT
#define __get_kernel_nofault(dst, src, type, err_label) \
--
2.31.1
^ permalink raw reply related
* [PATCH v5 1/5] powerpc/signal64: Access function descriptor with user access block
From: Christophe Leroy @ 2021-09-17 6:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
ebiederm, hch
Cc: linuxppc-dev, linux-kernel
Access the function descriptor of the handler within a
user access block.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v3: Flatten the change to avoid nested gotos.
---
arch/powerpc/kernel/signal_64.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 1831bba0582e..7b1cd50bc4fb 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -936,8 +936,13 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
func_descr_t __user *funct_desc_ptr =
(func_descr_t __user *) ksig->ka.sa.sa_handler;
- err |= get_user(regs->ctr, &funct_desc_ptr->entry);
- err |= get_user(regs->gpr[2], &funct_desc_ptr->toc);
+ if (!user_read_access_begin(funct_desc_ptr, sizeof(func_descr_t)))
+ goto badfunc;
+
+ unsafe_get_user(regs->ctr, &funct_desc_ptr->entry, badfunc_block);
+ unsafe_get_user(regs->gpr[2], &funct_desc_ptr->toc, badfunc_block);
+
+ user_read_access_end();
}
/* enter the signal handler in native-endian mode */
@@ -962,5 +967,12 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
badframe:
signal_fault(current, regs, "handle_rt_signal64", frame);
+ return 1;
+
+badfunc_block:
+ user_read_access_end();
+badfunc:
+ signal_fault(current, regs, __func__, (void __user *)ksig->ka.sa.sa_handler);
+
return 1;
}
--
2.31.1
^ permalink raw reply related
* Re: [PATCH v2] powerpc/mce: Fix access error in mce handler
From: Daniel Axtens @ 2021-09-17 6:39 UTC (permalink / raw)
To: Ganesh Goudar, linuxppc-dev, mpe; +Cc: Ganesh Goudar, mahesh, npiggin
In-Reply-To: <20210909064330.312432-1-ganeshgr@linux.ibm.com>
Hi Ganesh,
> We queue an irq work for deferred processing of mce event
> in realmode mce handler, where translation is disabled.
> Queuing of the work may result in accessing memory outside
> RMO region, such access needs the translation to be enabled
> for an LPAR running with hash mmu else the kernel crashes.
>
> After enabling translation in mce_handle_error() we used to
> leave it enabled to avoid crashing here, but now with the
> commit 74c3354bc1d89 ("powerpc/pseries/mce: restore msr before
> returning from handler") we are restoring the MSR to disable
> translation.
>
> Hence to fix this enable the translation before queuing the work.
[snip]
> Fixes: 74c3354bc1d89 ("powerpc/pseries/mce: restore msr before returning from handler")
That patch changes arch/powerpc/powerpc/platforms/pseries/ras.c just
below this comment:
/*
* Enable translation as we will be accessing per-cpu variables
* in save_mce_event() which may fall outside RMO region, also
* leave it enabled because subsequently we will be queuing work
* to workqueues where again per-cpu variables accessed, besides
* fwnmi_release_errinfo() crashes when called in realmode on
* pseries.
* Note: All the realmode handling like flushing SLB entries for
* SLB multihit is done by now.
*/
That suggests per-cpu variables need protection. In your patch, you
enable translations just around irq_work_queue:
> + /* Queue irq work to process this event later. Before
> + * queuing the work enable translation for non radix LPAR,
> + * as irq_work_queue may try to access memory outside RMO
> + * region.
> + */
> + if (!radix_enabled() && firmware_has_feature(FW_FEATURE_LPAR)) {
> + msr = mfmsr();
> + mtmsr(msr | MSR_IR | MSR_DR);
> + irq_work_queue(&mce_event_process_work);
> + mtmsr(msr);
> + } else {
> + irq_work_queue(&mce_event_process_work);
> + }
However, just before that in the function, there are a few things that
access per-cpu variables via the local_paca, e.g.:
memcpy(&local_paca->mce_info->mce_event_queue[index],
&evt, sizeof(evt));
Do we need to widen the window where translations are enabled in order
to protect accesses to local_paca?
Kind regards,
Daniel
^ permalink raw reply
* Re: [PATCH] lib/zlib_inflate/inffast: Check config in C to avoid unused function warning
From: Christophe Leroy @ 2021-09-17 5:17 UTC (permalink / raw)
To: Paul Menzel, Nathan Chancellor, Nick Desaulniers
Cc: llvm, linux-kernel, Paul Mackerras, Zhen Lei, Andrew Morton,
linuxppc-dev
In-Reply-To: <20210916142210.26722-1-pmenzel@molgen.mpg.de>
Le 16/09/2021 à 16:22, Paul Menzel a écrit :
> Building Linux for ppc64le with Ubuntu clang version 12.0.0-3ubuntu1~21.04.1
> shows the warning below.
>
> arch/powerpc/boot/inffast.c:20:1: warning: unused function 'get_unaligned16' [-Wunused-function]
> get_unaligned16(const unsigned short *p)
> ^
> 1 warning generated.
>
> Fix it, by moving the check from the preprocessor to C, so the compiler
> sees the use.
>
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
> lib/zlib_inflate/inffast.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/lib/zlib_inflate/inffast.c b/lib/zlib_inflate/inffast.c
> index f19c4fbe1be7..444ad3c3ccd3 100644
> --- a/lib/zlib_inflate/inffast.c
> +++ b/lib/zlib_inflate/inffast.c
> @@ -254,11 +254,7 @@ void inflate_fast(z_streamp strm, unsigned start)
> sfrom = (unsigned short *)(from);
> loops = len >> 1;
> do
> -#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> - *sout++ = *sfrom++;
> -#else
> - *sout++ = get_unaligned16(sfrom++);
> -#endif
> + *sout++ = CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS ? *sfrom++ : get_unaligned16(sfrom++);
You can't do that, CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is not
something that have value 0 or 1, it is something which is either
defined or not. You have to use IS_ENABLED() macro, so it should become
something like:
if (IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
*sout++ = *sfrom++;
else
*sout++ = get_unaligned16(sfrom++);
> while (--loops);
> out = (unsigned char *)sout;
> from = (unsigned char *)sfrom;
>
^ permalink raw reply
* Re: [PATCH v5 6/6] sched/fair: Consider SMT in ASYM_PACKING load balance
From: Ricardo Neri @ 2021-09-17 1:00 UTC (permalink / raw)
To: Vincent Guittot
Cc: Juri Lelli, Aubrey Li, Srikar Dronamraju, Ravi V. Shankar,
Peter Zijlstra (Intel), Ricardo Neri, Ben Segall,
Srinivas Pandruvada, Joel Fernandes (Google), Ingo Molnar,
Rafael J . Wysocki, Steven Rostedt, Mel Gorman, Len Brown,
Nicholas Piggin, Aubrey Li, Dietmar Eggemann, Tim Chen,
Quentin Perret, Daniel Bristot de Oliveira, linux-kernel,
linuxppc-dev
In-Reply-To: <CAKfTPtBcDP3Yp54sd4+1kP=o=4e_1HEmOf=eMXydag_J38CEng@mail.gmail.com>
On Wed, Sep 15, 2021 at 05:43:44PM +0200, Vincent Guittot wrote:
> On Sat, 11 Sept 2021 at 03:19, Ricardo Neri
> <ricardo.neri-calderon@linux.intel.com> wrote:
> >
> > When deciding to pull tasks in ASYM_PACKING, it is necessary not only to
> > check for the idle state of the destination CPU, dst_cpu, but also of
> > its SMT siblings.
> >
> > If dst_cpu is idle but its SMT siblings are busy, performance suffers
> > if it pulls tasks from a medium priority CPU that does not have SMT
> > siblings.
> >
> > Implement asym_smt_can_pull_tasks() to inspect the state of the SMT
> > siblings of both dst_cpu and the CPUs in the candidate busiest group.
> >
> > Cc: Aubrey Li <aubrey.li@intel.com>
> > Cc: Ben Segall <bsegall@google.com>
> > Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
> > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> > Cc: Mel Gorman <mgorman@suse.de>
> > Cc: Quentin Perret <qperret@google.com>
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Tim Chen <tim.c.chen@linux.intel.com>
> > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > Reviewed-by: Len Brown <len.brown@intel.com>
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > ---
> > Changes since v4:
> > * Use sg_lb_stats::sum_nr_running the idle state of a scheduling group.
> > (Vincent, Peter)
> > * Do not even idle CPUs in asym_smt_can_pull_tasks(). (Vincent)
> > * Updated function documentation and corrected a typo.
> >
> > Changes since v3:
> > * Removed the arch_asym_check_smt_siblings() hook. Discussions with the
> > powerpc folks showed that this patch should not impact them. Also, more
> > recent powerpc processor no longer use asym_packing. (PeterZ)
> > * Removed unnecessary local variable in asym_can_pull_tasks(). (Dietmar)
> > * Removed unnecessary check for local CPUs when the local group has zero
> > utilization. (Joel)
> > * Renamed asym_can_pull_tasks() as asym_smt_can_pull_tasks() to reflect
> > the fact that it deals with SMT cases.
> > * Made asym_smt_can_pull_tasks() return false for !CONFIG_SCHED_SMT so
> > that callers can deal with non-SMT cases.
> >
> > Changes since v2:
> > * Reworded the commit message to reflect updates in code.
> > * Corrected misrepresentation of dst_cpu as the CPU doing the load
> > balancing. (PeterZ)
> > * Removed call to arch_asym_check_smt_siblings() as it is now called in
> > sched_asym().
> >
> > Changes since v1:
> > * Don't bailout in update_sd_pick_busiest() if dst_cpu cannot pull
> > tasks. Instead, reclassify the candidate busiest group, as it
> > may still be selected. (PeterZ)
> > * Avoid an expensive and unnecessary call to cpumask_weight() when
> > determining if a sched_group is comprised of SMT siblings.
> > (PeterZ).
> > ---
> > kernel/sched/fair.c | 94 +++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 94 insertions(+)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 26db017c14a3..8d763dd0174b 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -8597,10 +8597,98 @@ group_type group_classify(unsigned int imbalance_pct,
> > return group_has_spare;
> > }
> >
> > +/**
> > + * asym_smt_can_pull_tasks - Check whether the load balancing CPU can pull tasks
> > + * @dst_cpu: Destination CPU of the load balancing
> > + * @sds: Load-balancing data with statistics of the local group
> > + * @sgs: Load-balancing statistics of the candidate busiest group
> > + * @sg: The candidate busiest group
> > + *
> > + * Check the state of the SMT siblings of both @sds::local and @sg and decide
> > + * if @dst_cpu can pull tasks.
> > + *
> > + * If @dst_cpu does not have SMT siblings, it can pull tasks if two or more of
> > + * the SMT siblings of @sg are busy. If only one CPU in @sg is busy, pull tasks
> > + * only if @dst_cpu has higher priority.
> > + *
> > + * If both @dst_cpu and @sg have SMT siblings, and @sg has exactly one more
> > + * busy CPU than @sds::local, let @dst_cpu pull tasks if it has higher priority.
> > + * Bigger imbalances in the number of busy CPUs will be dealt with in
> > + * update_sd_pick_busiest().
> > + *
> > + * If @sg does not have SMT siblings, only pull tasks if all of the SMT siblings
> > + * of @dst_cpu are idle and @sg has lower priority.
> > + */
> > +static bool asym_smt_can_pull_tasks(int dst_cpu, struct sd_lb_stats *sds,
> > + struct sg_lb_stats *sgs,
> > + struct sched_group *sg)
> > +{
> > +#ifdef CONFIG_SCHED_SMT
> > + bool local_is_smt, sg_is_smt;
> > + int sg_busy_cpus;
> > +
> > + local_is_smt = sds->local->flags & SD_SHARE_CPUCAPACITY;
> > + sg_is_smt = sg->flags & SD_SHARE_CPUCAPACITY;
> > +
> > + sg_busy_cpus = sgs->group_weight - sgs->idle_cpus;
> > +
> > + if (!local_is_smt) {
> > + /*
> > + * If we are here, @dst_cpu is idle and does not have SMT
> > + * siblings. Pull tasks if candidate group has two or more
> > + * busy CPUs.
> > + */
> > + if (sg_is_smt && sg_busy_cpus >= 2)
>
> Do you really need to test sg_is_smt ? if sg_busy_cpus >= 2 then
> sd_is_smt must be true ?
Thank you very much for your feedback Vincent!
Yes, it is true that sg_busy_cpus >=2 is only true if @sg is SMT. I will
remove this check.
>
> Also, This is the default behavior where we want to even the number of
> busy cpu. Shouldn't you return false and fall back to the default
> behavior ?
This is also true.
>
> That being said, the default behavior tries to even the number of idle
> cpus which is easier to compute and is equal to even the number of
> busy cpus in "normal" system with the same number of cpus in groups
> but this is not the case here. It could be good to change the default
> behavior to even the number of busy cpus and that you use the default
> behavior here. Additional condition will be used to select the busiest
> group like more busy cpu or more number of running tasks
That is a very good observation. Checking the number of idle CPUs
assumes that both groups have the same number of CPUs. I'll look into
modifying the default behavior.
>
> > + return true;
> > +
> > + /*
> > + * @dst_cpu does not have SMT siblings. @sg may have SMT
> > + * siblings and only one is busy. In such case, @dst_cpu
> > + * can help if it has higher priority and is idle (i.e.,
> > + * it has no running tasks).
>
> The previous comment above assume that "@dst_cpu is idle" but now you
> need to check that sds->local_stat.sum_nr_running == 0
But we already know that, right? We are here because in
update_sg_lb_stats() we determine that dst CPU is idle (env->idle !=
CPU_NOT_IDLE).
Thanks and BR,
Ricardo
^ permalink raw reply
* Re: [PATCH] lib/zlib_inflate/inffast: Check config in C to avoid unused function warning
From: kernel test robot @ 2021-09-17 0:42 UTC (permalink / raw)
To: Paul Menzel, Nathan Chancellor, Nick Desaulniers
Cc: Paul Menzel, kbuild-all, Zhen Lei, Linux Memory Management List,
Paul Mackerras, Andrew Morton, linuxppc-dev
In-Reply-To: <20210916142210.26722-1-pmenzel@molgen.mpg.de>
[-- Attachment #1: Type: text/plain, Size: 15490 bytes --]
Hi Paul,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.15-rc1 next-20210916]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Paul-Menzel/lib-zlib_inflate-inffast-Check-config-in-C-to-avoid-unused-function-warning/20210916-222359
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git ff1ffd71d5f0612cf194f5705c671d6b64bf5f91
config: nds32-defconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/127eebd64d8e291cf75563499f6c886bd54a99d8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Paul-Menzel/lib-zlib_inflate-inffast-Check-config-in-C-to-avoid-unused-function-warning/20210916-222359
git checkout 127eebd64d8e291cf75563499f6c886bd54a99d8
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=nds32 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
lib/zlib_inflate/inffast.c: In function 'inflate_fast':
>> lib/zlib_inflate/inffast.c:257:39: error: 'CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS' undeclared (first use in this function)
257 | *sout++ = CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS ? *sfrom++ : get_unaligned16(sfrom++);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/zlib_inflate/inffast.c:257:39: note: each undeclared identifier is reported only once for each function it appears in
vim +/CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS +257 lib/zlib_inflate/inffast.c
29
30 /*
31 Decode literal, length, and distance codes and write out the resulting
32 literal and match bytes until either not enough input or output is
33 available, an end-of-block is encountered, or a data error is encountered.
34 When large enough input and output buffers are supplied to inflate(), for
35 example, a 16K input buffer and a 64K output buffer, more than 95% of the
36 inflate execution time is spent in this routine.
37
38 Entry assumptions:
39
40 state->mode == LEN
41 strm->avail_in >= 6
42 strm->avail_out >= 258
43 start >= strm->avail_out
44 state->bits < 8
45
46 On return, state->mode is one of:
47
48 LEN -- ran out of enough output space or enough available input
49 TYPE -- reached end of block code, inflate() to interpret next block
50 BAD -- error in block data
51
52 Notes:
53
54 - The maximum input bits used by a length/distance pair is 15 bits for the
55 length code, 5 bits for the length extra, 15 bits for the distance code,
56 and 13 bits for the distance extra. This totals 48 bits, or six bytes.
57 Therefore if strm->avail_in >= 6, then there is enough input to avoid
58 checking for available input while decoding.
59
60 - The maximum bytes that a single length/distance pair can output is 258
61 bytes, which is the maximum length that can be coded. inflate_fast()
62 requires strm->avail_out >= 258 for each loop to avoid checking for
63 output space.
64
65 - @start: inflate()'s starting value for strm->avail_out
66 */
67 void inflate_fast(z_streamp strm, unsigned start)
68 {
69 struct inflate_state *state;
70 const unsigned char *in; /* local strm->next_in */
71 const unsigned char *last; /* while in < last, enough input available */
72 unsigned char *out; /* local strm->next_out */
73 unsigned char *beg; /* inflate()'s initial strm->next_out */
74 unsigned char *end; /* while out < end, enough space available */
75 #ifdef INFLATE_STRICT
76 unsigned dmax; /* maximum distance from zlib header */
77 #endif
78 unsigned wsize; /* window size or zero if not using window */
79 unsigned whave; /* valid bytes in the window */
80 unsigned write; /* window write index */
81 unsigned char *window; /* allocated sliding window, if wsize != 0 */
82 unsigned long hold; /* local strm->hold */
83 unsigned bits; /* local strm->bits */
84 code const *lcode; /* local strm->lencode */
85 code const *dcode; /* local strm->distcode */
86 unsigned lmask; /* mask for first level of length codes */
87 unsigned dmask; /* mask for first level of distance codes */
88 code this; /* retrieved table entry */
89 unsigned op; /* code bits, operation, extra bits, or */
90 /* window position, window bytes to copy */
91 unsigned len; /* match length, unused bytes */
92 unsigned dist; /* match distance */
93 unsigned char *from; /* where to copy match from */
94
95 /* copy state to local variables */
96 state = (struct inflate_state *)strm->state;
97 in = strm->next_in;
98 last = in + (strm->avail_in - 5);
99 out = strm->next_out;
100 beg = out - (start - strm->avail_out);
101 end = out + (strm->avail_out - 257);
102 #ifdef INFLATE_STRICT
103 dmax = state->dmax;
104 #endif
105 wsize = state->wsize;
106 whave = state->whave;
107 write = state->write;
108 window = state->window;
109 hold = state->hold;
110 bits = state->bits;
111 lcode = state->lencode;
112 dcode = state->distcode;
113 lmask = (1U << state->lenbits) - 1;
114 dmask = (1U << state->distbits) - 1;
115
116 /* decode literals and length/distances until end-of-block or not enough
117 input data or output space */
118 do {
119 if (bits < 15) {
120 hold += (unsigned long)(*in++) << bits;
121 bits += 8;
122 hold += (unsigned long)(*in++) << bits;
123 bits += 8;
124 }
125 this = lcode[hold & lmask];
126 dolen:
127 op = (unsigned)(this.bits);
128 hold >>= op;
129 bits -= op;
130 op = (unsigned)(this.op);
131 if (op == 0) { /* literal */
132 *out++ = (unsigned char)(this.val);
133 }
134 else if (op & 16) { /* length base */
135 len = (unsigned)(this.val);
136 op &= 15; /* number of extra bits */
137 if (op) {
138 if (bits < op) {
139 hold += (unsigned long)(*in++) << bits;
140 bits += 8;
141 }
142 len += (unsigned)hold & ((1U << op) - 1);
143 hold >>= op;
144 bits -= op;
145 }
146 if (bits < 15) {
147 hold += (unsigned long)(*in++) << bits;
148 bits += 8;
149 hold += (unsigned long)(*in++) << bits;
150 bits += 8;
151 }
152 this = dcode[hold & dmask];
153 dodist:
154 op = (unsigned)(this.bits);
155 hold >>= op;
156 bits -= op;
157 op = (unsigned)(this.op);
158 if (op & 16) { /* distance base */
159 dist = (unsigned)(this.val);
160 op &= 15; /* number of extra bits */
161 if (bits < op) {
162 hold += (unsigned long)(*in++) << bits;
163 bits += 8;
164 if (bits < op) {
165 hold += (unsigned long)(*in++) << bits;
166 bits += 8;
167 }
168 }
169 dist += (unsigned)hold & ((1U << op) - 1);
170 #ifdef INFLATE_STRICT
171 if (dist > dmax) {
172 strm->msg = (char *)"invalid distance too far back";
173 state->mode = BAD;
174 break;
175 }
176 #endif
177 hold >>= op;
178 bits -= op;
179 op = (unsigned)(out - beg); /* max distance in output */
180 if (dist > op) { /* see if copy from window */
181 op = dist - op; /* distance back in window */
182 if (op > whave) {
183 strm->msg = (char *)"invalid distance too far back";
184 state->mode = BAD;
185 break;
186 }
187 from = window;
188 if (write == 0) { /* very common case */
189 from += wsize - op;
190 if (op < len) { /* some from window */
191 len -= op;
192 do {
193 *out++ = *from++;
194 } while (--op);
195 from = out - dist; /* rest from output */
196 }
197 }
198 else if (write < op) { /* wrap around window */
199 from += wsize + write - op;
200 op -= write;
201 if (op < len) { /* some from end of window */
202 len -= op;
203 do {
204 *out++ = *from++;
205 } while (--op);
206 from = window;
207 if (write < len) { /* some from start of window */
208 op = write;
209 len -= op;
210 do {
211 *out++ = *from++;
212 } while (--op);
213 from = out - dist; /* rest from output */
214 }
215 }
216 }
217 else { /* contiguous in window */
218 from += write - op;
219 if (op < len) { /* some from window */
220 len -= op;
221 do {
222 *out++ = *from++;
223 } while (--op);
224 from = out - dist; /* rest from output */
225 }
226 }
227 while (len > 2) {
228 *out++ = *from++;
229 *out++ = *from++;
230 *out++ = *from++;
231 len -= 3;
232 }
233 if (len) {
234 *out++ = *from++;
235 if (len > 1)
236 *out++ = *from++;
237 }
238 }
239 else {
240 unsigned short *sout;
241 unsigned long loops;
242
243 from = out - dist; /* copy direct from output */
244 /* minimum length is three */
245 /* Align out addr */
246 if (!((long)(out - 1) & 1)) {
247 *out++ = *from++;
248 len--;
249 }
250 sout = (unsigned short *)(out);
251 if (dist > 2) {
252 unsigned short *sfrom;
253
254 sfrom = (unsigned short *)(from);
255 loops = len >> 1;
256 do
> 257 *sout++ = CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS ? *sfrom++ : get_unaligned16(sfrom++);
258 while (--loops);
259 out = (unsigned char *)sout;
260 from = (unsigned char *)sfrom;
261 } else { /* dist == 1 or dist == 2 */
262 unsigned short pat16;
263
264 pat16 = *(sout-1);
265 if (dist == 1) {
266 union uu mm;
267 /* copy one char pattern to both bytes */
268 mm.us = pat16;
269 mm.b[0] = mm.b[1];
270 pat16 = mm.us;
271 }
272 loops = len >> 1;
273 do
274 *sout++ = pat16;
275 while (--loops);
276 out = (unsigned char *)sout;
277 }
278 if (len & 1)
279 *out++ = *from++;
280 }
281 }
282 else if ((op & 64) == 0) { /* 2nd level distance code */
283 this = dcode[this.val + (hold & ((1U << op) - 1))];
284 goto dodist;
285 }
286 else {
287 strm->msg = (char *)"invalid distance code";
288 state->mode = BAD;
289 break;
290 }
291 }
292 else if ((op & 64) == 0) { /* 2nd level length code */
293 this = lcode[this.val + (hold & ((1U << op) - 1))];
294 goto dolen;
295 }
296 else if (op & 32) { /* end-of-block */
297 state->mode = TYPE;
298 break;
299 }
300 else {
301 strm->msg = (char *)"invalid literal/length code";
302 state->mode = BAD;
303 break;
304 }
305 } while (in < last && out < end);
306
307 /* return unused bytes (on entry, bits < 8, so in won't go too far back) */
308 len = bits >> 3;
309 in -= len;
310 bits -= len << 3;
311 hold &= (1U << bits) - 1;
312
313 /* update state and return */
314 strm->next_in = in;
315 strm->next_out = out;
316 strm->avail_in = (unsigned)(in < last ? 5 + (last - in) : 5 - (in - last));
317 strm->avail_out = (unsigned)(out < end ?
318 257 + (end - out) : 257 - (out - end));
319 state->hold = hold;
320 state->bits = bits;
321 return;
322 }
323
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 11023 bytes --]
^ permalink raw reply
* Re: [PATCH] lib/zlib_inflate/inffast: Check config in C to avoid unused function warning
From: kernel test robot @ 2021-09-16 23:56 UTC (permalink / raw)
To: Paul Menzel, Nathan Chancellor, Nick Desaulniers
Cc: Paul Menzel, kbuild-all, llvm, Linux Memory Management List,
Zhen Lei, Paul Mackerras, Andrew Morton, linuxppc-dev
In-Reply-To: <20210916142210.26722-1-pmenzel@molgen.mpg.de>
[-- Attachment #1: Type: text/plain, Size: 15370 bytes --]
Hi Paul,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.15-rc1 next-20210916]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Paul-Menzel/lib-zlib_inflate-inffast-Check-config-in-C-to-avoid-unused-function-warning/20210916-222359
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git ff1ffd71d5f0612cf194f5705c671d6b64bf5f91
config: hexagon-randconfig-r045-20210916 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c8b3d7d6d6de37af68b2f379d0e37304f78e115f)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/127eebd64d8e291cf75563499f6c886bd54a99d8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Paul-Menzel/lib-zlib_inflate-inffast-Check-config-in-C-to-avoid-unused-function-warning/20210916-222359
git checkout 127eebd64d8e291cf75563499f6c886bd54a99d8
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash lib/zlib_inflate/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> lib/zlib_inflate/inffast.c:257:18: error: use of undeclared identifier 'CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS'
*sout++ = CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS ? *sfrom++ : get_unaligned16(sfrom++);
^
1 error generated.
vim +/CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS +257 lib/zlib_inflate/inffast.c
29
30 /*
31 Decode literal, length, and distance codes and write out the resulting
32 literal and match bytes until either not enough input or output is
33 available, an end-of-block is encountered, or a data error is encountered.
34 When large enough input and output buffers are supplied to inflate(), for
35 example, a 16K input buffer and a 64K output buffer, more than 95% of the
36 inflate execution time is spent in this routine.
37
38 Entry assumptions:
39
40 state->mode == LEN
41 strm->avail_in >= 6
42 strm->avail_out >= 258
43 start >= strm->avail_out
44 state->bits < 8
45
46 On return, state->mode is one of:
47
48 LEN -- ran out of enough output space or enough available input
49 TYPE -- reached end of block code, inflate() to interpret next block
50 BAD -- error in block data
51
52 Notes:
53
54 - The maximum input bits used by a length/distance pair is 15 bits for the
55 length code, 5 bits for the length extra, 15 bits for the distance code,
56 and 13 bits for the distance extra. This totals 48 bits, or six bytes.
57 Therefore if strm->avail_in >= 6, then there is enough input to avoid
58 checking for available input while decoding.
59
60 - The maximum bytes that a single length/distance pair can output is 258
61 bytes, which is the maximum length that can be coded. inflate_fast()
62 requires strm->avail_out >= 258 for each loop to avoid checking for
63 output space.
64
65 - @start: inflate()'s starting value for strm->avail_out
66 */
67 void inflate_fast(z_streamp strm, unsigned start)
68 {
69 struct inflate_state *state;
70 const unsigned char *in; /* local strm->next_in */
71 const unsigned char *last; /* while in < last, enough input available */
72 unsigned char *out; /* local strm->next_out */
73 unsigned char *beg; /* inflate()'s initial strm->next_out */
74 unsigned char *end; /* while out < end, enough space available */
75 #ifdef INFLATE_STRICT
76 unsigned dmax; /* maximum distance from zlib header */
77 #endif
78 unsigned wsize; /* window size or zero if not using window */
79 unsigned whave; /* valid bytes in the window */
80 unsigned write; /* window write index */
81 unsigned char *window; /* allocated sliding window, if wsize != 0 */
82 unsigned long hold; /* local strm->hold */
83 unsigned bits; /* local strm->bits */
84 code const *lcode; /* local strm->lencode */
85 code const *dcode; /* local strm->distcode */
86 unsigned lmask; /* mask for first level of length codes */
87 unsigned dmask; /* mask for first level of distance codes */
88 code this; /* retrieved table entry */
89 unsigned op; /* code bits, operation, extra bits, or */
90 /* window position, window bytes to copy */
91 unsigned len; /* match length, unused bytes */
92 unsigned dist; /* match distance */
93 unsigned char *from; /* where to copy match from */
94
95 /* copy state to local variables */
96 state = (struct inflate_state *)strm->state;
97 in = strm->next_in;
98 last = in + (strm->avail_in - 5);
99 out = strm->next_out;
100 beg = out - (start - strm->avail_out);
101 end = out + (strm->avail_out - 257);
102 #ifdef INFLATE_STRICT
103 dmax = state->dmax;
104 #endif
105 wsize = state->wsize;
106 whave = state->whave;
107 write = state->write;
108 window = state->window;
109 hold = state->hold;
110 bits = state->bits;
111 lcode = state->lencode;
112 dcode = state->distcode;
113 lmask = (1U << state->lenbits) - 1;
114 dmask = (1U << state->distbits) - 1;
115
116 /* decode literals and length/distances until end-of-block or not enough
117 input data or output space */
118 do {
119 if (bits < 15) {
120 hold += (unsigned long)(*in++) << bits;
121 bits += 8;
122 hold += (unsigned long)(*in++) << bits;
123 bits += 8;
124 }
125 this = lcode[hold & lmask];
126 dolen:
127 op = (unsigned)(this.bits);
128 hold >>= op;
129 bits -= op;
130 op = (unsigned)(this.op);
131 if (op == 0) { /* literal */
132 *out++ = (unsigned char)(this.val);
133 }
134 else if (op & 16) { /* length base */
135 len = (unsigned)(this.val);
136 op &= 15; /* number of extra bits */
137 if (op) {
138 if (bits < op) {
139 hold += (unsigned long)(*in++) << bits;
140 bits += 8;
141 }
142 len += (unsigned)hold & ((1U << op) - 1);
143 hold >>= op;
144 bits -= op;
145 }
146 if (bits < 15) {
147 hold += (unsigned long)(*in++) << bits;
148 bits += 8;
149 hold += (unsigned long)(*in++) << bits;
150 bits += 8;
151 }
152 this = dcode[hold & dmask];
153 dodist:
154 op = (unsigned)(this.bits);
155 hold >>= op;
156 bits -= op;
157 op = (unsigned)(this.op);
158 if (op & 16) { /* distance base */
159 dist = (unsigned)(this.val);
160 op &= 15; /* number of extra bits */
161 if (bits < op) {
162 hold += (unsigned long)(*in++) << bits;
163 bits += 8;
164 if (bits < op) {
165 hold += (unsigned long)(*in++) << bits;
166 bits += 8;
167 }
168 }
169 dist += (unsigned)hold & ((1U << op) - 1);
170 #ifdef INFLATE_STRICT
171 if (dist > dmax) {
172 strm->msg = (char *)"invalid distance too far back";
173 state->mode = BAD;
174 break;
175 }
176 #endif
177 hold >>= op;
178 bits -= op;
179 op = (unsigned)(out - beg); /* max distance in output */
180 if (dist > op) { /* see if copy from window */
181 op = dist - op; /* distance back in window */
182 if (op > whave) {
183 strm->msg = (char *)"invalid distance too far back";
184 state->mode = BAD;
185 break;
186 }
187 from = window;
188 if (write == 0) { /* very common case */
189 from += wsize - op;
190 if (op < len) { /* some from window */
191 len -= op;
192 do {
193 *out++ = *from++;
194 } while (--op);
195 from = out - dist; /* rest from output */
196 }
197 }
198 else if (write < op) { /* wrap around window */
199 from += wsize + write - op;
200 op -= write;
201 if (op < len) { /* some from end of window */
202 len -= op;
203 do {
204 *out++ = *from++;
205 } while (--op);
206 from = window;
207 if (write < len) { /* some from start of window */
208 op = write;
209 len -= op;
210 do {
211 *out++ = *from++;
212 } while (--op);
213 from = out - dist; /* rest from output */
214 }
215 }
216 }
217 else { /* contiguous in window */
218 from += write - op;
219 if (op < len) { /* some from window */
220 len -= op;
221 do {
222 *out++ = *from++;
223 } while (--op);
224 from = out - dist; /* rest from output */
225 }
226 }
227 while (len > 2) {
228 *out++ = *from++;
229 *out++ = *from++;
230 *out++ = *from++;
231 len -= 3;
232 }
233 if (len) {
234 *out++ = *from++;
235 if (len > 1)
236 *out++ = *from++;
237 }
238 }
239 else {
240 unsigned short *sout;
241 unsigned long loops;
242
243 from = out - dist; /* copy direct from output */
244 /* minimum length is three */
245 /* Align out addr */
246 if (!((long)(out - 1) & 1)) {
247 *out++ = *from++;
248 len--;
249 }
250 sout = (unsigned short *)(out);
251 if (dist > 2) {
252 unsigned short *sfrom;
253
254 sfrom = (unsigned short *)(from);
255 loops = len >> 1;
256 do
> 257 *sout++ = CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS ? *sfrom++ : get_unaligned16(sfrom++);
258 while (--loops);
259 out = (unsigned char *)sout;
260 from = (unsigned char *)sfrom;
261 } else { /* dist == 1 or dist == 2 */
262 unsigned short pat16;
263
264 pat16 = *(sout-1);
265 if (dist == 1) {
266 union uu mm;
267 /* copy one char pattern to both bytes */
268 mm.us = pat16;
269 mm.b[0] = mm.b[1];
270 pat16 = mm.us;
271 }
272 loops = len >> 1;
273 do
274 *sout++ = pat16;
275 while (--loops);
276 out = (unsigned char *)sout;
277 }
278 if (len & 1)
279 *out++ = *from++;
280 }
281 }
282 else if ((op & 64) == 0) { /* 2nd level distance code */
283 this = dcode[this.val + (hold & ((1U << op) - 1))];
284 goto dodist;
285 }
286 else {
287 strm->msg = (char *)"invalid distance code";
288 state->mode = BAD;
289 break;
290 }
291 }
292 else if ((op & 64) == 0) { /* 2nd level length code */
293 this = lcode[this.val + (hold & ((1U << op) - 1))];
294 goto dolen;
295 }
296 else if (op & 32) { /* end-of-block */
297 state->mode = TYPE;
298 break;
299 }
300 else {
301 strm->msg = (char *)"invalid literal/length code";
302 state->mode = BAD;
303 break;
304 }
305 } while (in < last && out < end);
306
307 /* return unused bytes (on entry, bits < 8, so in won't go too far back) */
308 len = bits >> 3;
309 in -= len;
310 bits -= len << 3;
311 hold &= (1U << bits) - 1;
312
313 /* update state and return */
314 strm->next_in = in;
315 strm->next_out = out;
316 strm->avail_in = (unsigned)(in < last ? 5 + (last - in) : 5 - (in - last));
317 strm->avail_out = (unsigned)(out < end ?
318 257 + (end - out) : 257 - (out - end));
319 state->hold = hold;
320 state->bits = bits;
321 return;
322 }
323
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28632 bytes --]
^ permalink raw reply
* [PATCH] powerpc/lib/sstep: Don't use __{get/put}_user() on kernel addresses
From: Christophe Leroy @ 2021-09-16 18:43 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: Stan Johnson, Finn Thain, linuxppc-dev, linux-kernel
In the old days, when we didn't have kernel userspace access
protection and had set_fs(), it was wise to use __get_user()
and friends to read kernel memory.
Nowadays, get_user() and put_user() are granting userspace access and
are exclusively for userspace access.
Convert single step emulation functions to user_access_begin() and
friends and use unsafe_get_user() and unsafe_put_user().
When addressing kernel addresses, there is no need to open userspace
access. And for book3s/32 it is particularly important to no try and
open userspace access on kernel address, because that would break the
content of kernel space segment registers. No guard has been put
against that risk in order to avoid degrading performance.
copy_from_kernel_nofault() and copy_to_kernel_nofault() should
be used but they are out-of-line functions which would degrade
performance. Those two functions are making use of
__get_kernel_nofault() and __put_kernel_nofault() macros.
Those two macros are just wrappers behind __get_user_size_goto() and
__put_user_size_goto().
unsafe_get_user() and unsafe_put_user() are also wrappers of
__get_user_size_goto() and __put_user_size_goto(). Use them to
access kernel space. That allows refactoring userspace and
kernelspace access.
Reported-by: Stan Johnson <userm57@yahoo.com>
Cc: Finn Thain <fthain@linux-m68k.org>
Depends-on: 4fe5cda9f89d ("powerpc/uaccess: Implement user_read_access_begin and user_write_access_begin")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/lib/sstep.c | 197 ++++++++++++++++++++++++++++-----------
1 file changed, 140 insertions(+), 57 deletions(-)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index d8d5f901cee1..86f49e3e7cf5 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -302,33 +302,51 @@ static nokprobe_inline void do_byte_reverse(void *ptr, int nb)
}
}
-static nokprobe_inline int read_mem_aligned(unsigned long *dest,
- unsigned long ea, int nb,
- struct pt_regs *regs)
+static __always_inline int
+__read_mem_aligned(unsigned long *dest, unsigned long ea, int nb, struct pt_regs *regs)
{
- int err = 0;
unsigned long x = 0;
switch (nb) {
case 1:
- err = __get_user(x, (unsigned char __user *) ea);
+ unsafe_get_user(x, (unsigned char __user *)ea, Efault);
break;
case 2:
- err = __get_user(x, (unsigned short __user *) ea);
+ unsafe_get_user(x, (unsigned short __user *)ea, Efault);
break;
case 4:
- err = __get_user(x, (unsigned int __user *) ea);
+ unsafe_get_user(x, (unsigned int __user *)ea, Efault);
break;
#ifdef __powerpc64__
case 8:
- err = __get_user(x, (unsigned long __user *) ea);
+ unsafe_get_user(x, (unsigned long __user *)ea, Efault);
break;
#endif
}
- if (!err)
- *dest = x;
- else
+ *dest = x;
+ return 0;
+
+Efault:
+ regs->dar = ea;
+ return -EFAULT;
+}
+
+static nokprobe_inline int
+read_mem_aligned(unsigned long *dest, unsigned long ea, int nb, struct pt_regs *regs)
+{
+ int err;
+
+ if (is_kernel_addr(ea))
+ return __read_mem_aligned(dest, ea, nb, regs);
+
+ if (user_read_access_begin((void __user *)ea, nb)) {
+ err = __read_mem_aligned(dest, ea, nb, regs);
+ user_read_access_end();
+ } else {
+ err = -EFAULT;
regs->dar = ea;
+ }
+
return err;
}
@@ -336,10 +354,8 @@ static nokprobe_inline int read_mem_aligned(unsigned long *dest,
* Copy from userspace to a buffer, using the largest possible
* aligned accesses, up to sizeof(long).
*/
-static nokprobe_inline int copy_mem_in(u8 *dest, unsigned long ea, int nb,
- struct pt_regs *regs)
+static __always_inline int __copy_mem_in(u8 *dest, unsigned long ea, int nb, struct pt_regs *regs)
{
- int err = 0;
int c;
for (; nb > 0; nb -= c) {
@@ -348,31 +364,46 @@ static nokprobe_inline int copy_mem_in(u8 *dest, unsigned long ea, int nb,
c = max_align(nb);
switch (c) {
case 1:
- err = __get_user(*dest, (unsigned char __user *) ea);
+ unsafe_get_user(*dest, (u8 __user *)ea, Efault);
break;
case 2:
- err = __get_user(*(u16 *)dest,
- (unsigned short __user *) ea);
+ unsafe_get_user(*(u16 *)dest, (u16 __user *)ea, Efault);
break;
case 4:
- err = __get_user(*(u32 *)dest,
- (unsigned int __user *) ea);
+ unsafe_get_user(*(u32 *)dest, (u32 __user *)ea, Efault);
break;
#ifdef __powerpc64__
case 8:
- err = __get_user(*(unsigned long *)dest,
- (unsigned long __user *) ea);
+ unsafe_get_user(*(u64 *)dest, (u64 __user *)ea, Efault);
break;
#endif
}
- if (err) {
- regs->dar = ea;
- return err;
- }
dest += c;
ea += c;
}
return 0;
+
+Efault:
+ regs->dar = ea;
+ return -EFAULT;
+}
+
+static nokprobe_inline int copy_mem_in(u8 *dest, unsigned long ea, int nb, struct pt_regs *regs)
+{
+ int err;
+
+ if (is_kernel_addr(ea))
+ return __copy_mem_in(dest, ea, nb, regs);
+
+ if (user_read_access_begin((void __user *)ea, nb)) {
+ err = __copy_mem_in(dest, ea, nb, regs);
+ user_read_access_end();
+ } else {
+ err = -EFAULT;
+ regs->dar = ea;
+ }
+
+ return err;
}
static nokprobe_inline int read_mem_unaligned(unsigned long *dest,
@@ -410,30 +441,48 @@ static int read_mem(unsigned long *dest, unsigned long ea, int nb,
}
NOKPROBE_SYMBOL(read_mem);
-static nokprobe_inline int write_mem_aligned(unsigned long val,
- unsigned long ea, int nb,
- struct pt_regs *regs)
+static __always_inline int
+__write_mem_aligned(unsigned long val, unsigned long ea, int nb, struct pt_regs *regs)
{
- int err = 0;
-
switch (nb) {
case 1:
- err = __put_user(val, (unsigned char __user *) ea);
+ unsafe_put_user(val, (unsigned char __user *)ea, Efault);
break;
case 2:
- err = __put_user(val, (unsigned short __user *) ea);
+ unsafe_put_user(val, (unsigned short __user *)ea, Efault);
break;
case 4:
- err = __put_user(val, (unsigned int __user *) ea);
+ unsafe_put_user(val, (unsigned int __user *)ea, Efault);
break;
#ifdef __powerpc64__
case 8:
- err = __put_user(val, (unsigned long __user *) ea);
+ unsafe_put_user(val, (unsigned long __user *)ea, Efault);
break;
#endif
}
- if (err)
+ return 0;
+
+Efault:
+ regs->dar = ea;
+ return -EFAULT;
+}
+
+static nokprobe_inline int
+write_mem_aligned(unsigned long val, unsigned long ea, int nb, struct pt_regs *regs)
+{
+ int err;
+
+ if (is_kernel_addr(ea))
+ return __write_mem_aligned(val, ea, nb, regs);
+
+ if (user_write_access_begin((void __user *)ea, nb)) {
+ err = __write_mem_aligned(val, ea, nb, regs);
+ user_write_access_end();
+ } else {
+ err = -EFAULT;
regs->dar = ea;
+ }
+
return err;
}
@@ -441,10 +490,8 @@ static nokprobe_inline int write_mem_aligned(unsigned long val,
* Copy from a buffer to userspace, using the largest possible
* aligned accesses, up to sizeof(long).
*/
-static nokprobe_inline int copy_mem_out(u8 *dest, unsigned long ea, int nb,
- struct pt_regs *regs)
+static nokprobe_inline int __copy_mem_out(u8 *dest, unsigned long ea, int nb, struct pt_regs *regs)
{
- int err = 0;
int c;
for (; nb > 0; nb -= c) {
@@ -453,31 +500,46 @@ static nokprobe_inline int copy_mem_out(u8 *dest, unsigned long ea, int nb,
c = max_align(nb);
switch (c) {
case 1:
- err = __put_user(*dest, (unsigned char __user *) ea);
+ unsafe_put_user(*dest, (u8 __user *)ea, Efault);
break;
case 2:
- err = __put_user(*(u16 *)dest,
- (unsigned short __user *) ea);
+ unsafe_put_user(*(u16 *)dest, (u16 __user *)ea, Efault);
break;
case 4:
- err = __put_user(*(u32 *)dest,
- (unsigned int __user *) ea);
+ unsafe_put_user(*(u32 *)dest, (u32 __user *)ea, Efault);
break;
#ifdef __powerpc64__
case 8:
- err = __put_user(*(unsigned long *)dest,
- (unsigned long __user *) ea);
+ unsafe_put_user(*(u64 *)dest, (u64 __user *)ea, Efault);
break;
#endif
}
- if (err) {
- regs->dar = ea;
- return err;
- }
dest += c;
ea += c;
}
return 0;
+
+Efault:
+ regs->dar = ea;
+ return -EFAULT;
+}
+
+static nokprobe_inline int copy_mem_out(u8 *dest, unsigned long ea, int nb, struct pt_regs *regs)
+{
+ int err;
+
+ if (is_kernel_addr(ea))
+ return __copy_mem_out(dest, ea, nb, regs);
+
+ if (user_write_access_begin((void __user *)ea, nb)) {
+ err = __copy_mem_out(dest, ea, nb, regs);
+ user_write_access_end();
+ } else {
+ err = -EFAULT;
+ regs->dar = ea;
+ }
+
+ return err;
}
static nokprobe_inline int write_mem_unaligned(unsigned long val,
@@ -986,10 +1048,24 @@ static nokprobe_inline int do_vsx_store(struct instruction_op *op,
}
#endif /* CONFIG_VSX */
+static int __emulate_dcbz(unsigned long ea)
+{
+ unsigned long i;
+ unsigned long size = l1_dcache_bytes();
+
+ for (i = 0; i < size; i += sizeof(long))
+ unsafe_put_user(0, (unsigned long __user *)(ea + i), Efault);
+
+ return 0;
+
+Efault:
+ return -EFAULT;
+}
+
int emulate_dcbz(unsigned long ea, struct pt_regs *regs)
{
int err;
- unsigned long i, size;
+ unsigned long size;
#ifdef __powerpc64__
size = ppc64_caches.l1d.block_size;
@@ -1001,14 +1077,21 @@ int emulate_dcbz(unsigned long ea, struct pt_regs *regs)
ea &= ~(size - 1);
if (!address_ok(regs, ea, size))
return -EFAULT;
- for (i = 0; i < size; i += sizeof(long)) {
- err = __put_user(0, (unsigned long __user *) (ea + i));
- if (err) {
- regs->dar = ea;
- return err;
- }
+
+ if (is_kernel_addr(ea)) {
+ err = __emulate_dcbz(ea);
+ } else if (user_write_access_begin((void __user *)ea, size)) {
+ err = __emulate_dcbz(ea);
+ user_write_access_end();
+ } else {
+ err = -EFAULT;
}
- return 0;
+
+ if (err)
+ regs->dar = ea;
+
+
+ return err;
}
NOKPROBE_SYMBOL(emulate_dcbz);
--
2.31.1
^ permalink raw reply related
* Re: [PATCH v3 0/8] Implement generic cc_platform_has() helper function
From: Kuppuswamy, Sathyanarayanan @ 2021-09-16 18:38 UTC (permalink / raw)
To: Borislav Petkov
Cc: linux-efi, Brijesh Singh, kvm, David Airlie, Dave Hansen,
dri-devel, platform-driver-x86, Paul Mackerras, Will Deacon,
Ard Biesheuvel, linux-s390, Andi Kleen, Baoquan He, Joerg Roedel,
x86, amd-gfx, Christoph Hellwig, Christian Borntraeger,
Ingo Molnar, linux-graphics-maintainer, Dave Young, Tom Lendacky,
Tianyu Lan, Thomas Zimmermann, Vasily Gorbik, Heiko Carstens,
Maarten Lankhorst, Maxime Ripard, Andy Lutomirski,
Thomas Gleixner, Peter Zijlstra, kexec, linux-kernel, iommu,
Daniel Vetter, linux-fsdevel, linuxppc-dev
In-Reply-To: <YUNckGH0+KXdEmqu@zn.tnic>
On 9/16/21 8:02 AM, Borislav Petkov wrote:
> On Wed, Sep 15, 2021 at 10:26:06AM -0700, Kuppuswamy, Sathyanarayanan wrote:
>> I have a Intel variant patch (please check following patch). But it includes
>> TDX changes as well. Shall I move TDX changes to different patch and just
>> create a separate patch for adding intel_cc_platform_has()?
>
> Yes, please, so that I can expedite that stuff separately and so that it
> can go in early in order for future work to be based ontop.
Sent it part of TDX patch series. Please check and cherry pick it.
https://lore.kernel.org/lkml/20210916183550.15349-2-sathyanarayanan.kuppuswamy@linux.intel.com/
>
> Thx.
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply
* [PATCH 5.14 232/432] hvsi: dont panic on tty_register_driver failure
From: Greg Kroah-Hartman @ 2021-09-16 15:59 UTC (permalink / raw)
To: linux-kernel
Cc: Sasha Levin, Greg Kroah-Hartman, Jiri Slaby, linuxppc-dev, stable
In-Reply-To: <20210916155810.813340753@linuxfoundation.org>
From: Jiri Slaby <jslaby@suse.cz>
[ Upstream commit 7ccbdcc4d08a6d7041e4849219bbb12ffa45db4c ]
The alloc_tty_driver failure is handled gracefully in hvsi_init. But
tty_register_driver is not. panic is called if that one fails.
So handle the failure of tty_register_driver gracefully too. This will
keep at least the console functional as it was enabled earlier by
console_initcall in hvsi_console_init. Instead of shooting down the
whole system.
This means, we disable interrupts and restore hvsi_wait back to
poll_for_state().
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20210723074317.32690-3-jslaby@suse.cz
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/tty/hvc/hvsi.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/hvc/hvsi.c b/drivers/tty/hvc/hvsi.c
index bfc15279d5bc..f0bc8e780051 100644
--- a/drivers/tty/hvc/hvsi.c
+++ b/drivers/tty/hvc/hvsi.c
@@ -1038,7 +1038,7 @@ static const struct tty_operations hvsi_ops = {
static int __init hvsi_init(void)
{
- int i;
+ int i, ret;
hvsi_driver = alloc_tty_driver(hvsi_count);
if (!hvsi_driver)
@@ -1069,12 +1069,25 @@ static int __init hvsi_init(void)
}
hvsi_wait = wait_for_state; /* irqs active now */
- if (tty_register_driver(hvsi_driver))
- panic("Couldn't register hvsi console driver\n");
+ ret = tty_register_driver(hvsi_driver);
+ if (ret) {
+ pr_err("Couldn't register hvsi console driver\n");
+ goto err_free_irq;
+ }
printk(KERN_DEBUG "HVSI: registered %i devices\n", hvsi_count);
return 0;
+err_free_irq:
+ hvsi_wait = poll_for_state;
+ for (i = 0; i < hvsi_count; i++) {
+ struct hvsi_struct *hp = &hvsi_ports[i];
+
+ free_irq(hp->virq, hp);
+ }
+ tty_driver_kref_put(hvsi_driver);
+
+ return ret;
}
device_initcall(hvsi_init);
--
2.30.2
^ permalink raw reply related
* [PATCH 5.13 205/380] hvsi: dont panic on tty_register_driver failure
From: Greg Kroah-Hartman @ 2021-09-16 15:59 UTC (permalink / raw)
To: linux-kernel
Cc: Sasha Levin, Greg Kroah-Hartman, Jiri Slaby, linuxppc-dev, stable
In-Reply-To: <20210916155803.966362085@linuxfoundation.org>
From: Jiri Slaby <jslaby@suse.cz>
[ Upstream commit 7ccbdcc4d08a6d7041e4849219bbb12ffa45db4c ]
The alloc_tty_driver failure is handled gracefully in hvsi_init. But
tty_register_driver is not. panic is called if that one fails.
So handle the failure of tty_register_driver gracefully too. This will
keep at least the console functional as it was enabled earlier by
console_initcall in hvsi_console_init. Instead of shooting down the
whole system.
This means, we disable interrupts and restore hvsi_wait back to
poll_for_state().
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20210723074317.32690-3-jslaby@suse.cz
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/tty/hvc/hvsi.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/hvc/hvsi.c b/drivers/tty/hvc/hvsi.c
index e8c58f9bd263..d6afaae1729a 100644
--- a/drivers/tty/hvc/hvsi.c
+++ b/drivers/tty/hvc/hvsi.c
@@ -1038,7 +1038,7 @@ static const struct tty_operations hvsi_ops = {
static int __init hvsi_init(void)
{
- int i;
+ int i, ret;
hvsi_driver = alloc_tty_driver(hvsi_count);
if (!hvsi_driver)
@@ -1069,12 +1069,25 @@ static int __init hvsi_init(void)
}
hvsi_wait = wait_for_state; /* irqs active now */
- if (tty_register_driver(hvsi_driver))
- panic("Couldn't register hvsi console driver\n");
+ ret = tty_register_driver(hvsi_driver);
+ if (ret) {
+ pr_err("Couldn't register hvsi console driver\n");
+ goto err_free_irq;
+ }
printk(KERN_DEBUG "HVSI: registered %i devices\n", hvsi_count);
return 0;
+err_free_irq:
+ hvsi_wait = poll_for_state;
+ for (i = 0; i < hvsi_count; i++) {
+ struct hvsi_struct *hp = &hvsi_ports[i];
+
+ free_irq(hp->virq, hp);
+ }
+ tty_driver_kref_put(hvsi_driver);
+
+ return ret;
}
device_initcall(hvsi_init);
--
2.30.2
^ permalink raw reply related
* [PATCH 5.10 163/306] hvsi: dont panic on tty_register_driver failure
From: Greg Kroah-Hartman @ 2021-09-16 15:58 UTC (permalink / raw)
To: linux-kernel
Cc: Sasha Levin, Greg Kroah-Hartman, Jiri Slaby, linuxppc-dev, stable
In-Reply-To: <20210916155753.903069397@linuxfoundation.org>
From: Jiri Slaby <jslaby@suse.cz>
[ Upstream commit 7ccbdcc4d08a6d7041e4849219bbb12ffa45db4c ]
The alloc_tty_driver failure is handled gracefully in hvsi_init. But
tty_register_driver is not. panic is called if that one fails.
So handle the failure of tty_register_driver gracefully too. This will
keep at least the console functional as it was enabled earlier by
console_initcall in hvsi_console_init. Instead of shooting down the
whole system.
This means, we disable interrupts and restore hvsi_wait back to
poll_for_state().
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20210723074317.32690-3-jslaby@suse.cz
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/tty/hvc/hvsi.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/hvc/hvsi.c b/drivers/tty/hvc/hvsi.c
index e8c58f9bd263..d6afaae1729a 100644
--- a/drivers/tty/hvc/hvsi.c
+++ b/drivers/tty/hvc/hvsi.c
@@ -1038,7 +1038,7 @@ static const struct tty_operations hvsi_ops = {
static int __init hvsi_init(void)
{
- int i;
+ int i, ret;
hvsi_driver = alloc_tty_driver(hvsi_count);
if (!hvsi_driver)
@@ -1069,12 +1069,25 @@ static int __init hvsi_init(void)
}
hvsi_wait = wait_for_state; /* irqs active now */
- if (tty_register_driver(hvsi_driver))
- panic("Couldn't register hvsi console driver\n");
+ ret = tty_register_driver(hvsi_driver);
+ if (ret) {
+ pr_err("Couldn't register hvsi console driver\n");
+ goto err_free_irq;
+ }
printk(KERN_DEBUG "HVSI: registered %i devices\n", hvsi_count);
return 0;
+err_free_irq:
+ hvsi_wait = poll_for_state;
+ for (i = 0; i < hvsi_count; i++) {
+ struct hvsi_struct *hp = &hvsi_ports[i];
+
+ free_irq(hp->virq, hp);
+ }
+ tty_driver_kref_put(hvsi_driver);
+
+ return ret;
}
device_initcall(hvsi_init);
--
2.30.2
^ permalink raw reply related
* Re: [PATCH v3 0/8] Implement generic cc_platform_has() helper function
From: Borislav Petkov @ 2021-09-16 15:02 UTC (permalink / raw)
To: Kuppuswamy, Sathyanarayanan
Cc: linux-efi, Brijesh Singh, kvm, David Airlie, Dave Hansen,
dri-devel, platform-driver-x86, Paul Mackerras, Will Deacon,
Ard Biesheuvel, linux-s390, Andi Kleen, Baoquan He, Joerg Roedel,
x86, amd-gfx, Christoph Hellwig, Christian Borntraeger,
Ingo Molnar, linux-graphics-maintainer, Dave Young, Tom Lendacky,
Tianyu Lan, Thomas Zimmermann, Vasily Gorbik, Heiko Carstens,
Maarten Lankhorst, Maxime Ripard, Andy Lutomirski,
Thomas Gleixner, Peter Zijlstra, kexec, linux-kernel, iommu,
Daniel Vetter, linux-fsdevel, linuxppc-dev
In-Reply-To: <d48e6a17-d2b4-67da-56d1-fc9a61dfe2b8@linux.intel.com>
On Wed, Sep 15, 2021 at 10:26:06AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> I have a Intel variant patch (please check following patch). But it includes
> TDX changes as well. Shall I move TDX changes to different patch and just
> create a separate patch for adding intel_cc_platform_has()?
Yes, please, so that I can expedite that stuff separately and so that it
can go in early in order for future work to be based ontop.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH] lib/zlib_inflate/inffast: Check config in C to avoid unused function warning
From: Nathan Chancellor @ 2021-09-16 14:58 UTC (permalink / raw)
To: Paul Menzel, Nick Desaulniers
Cc: llvm, Zhen Lei, linux-kernel, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <20210916142210.26722-1-pmenzel@molgen.mpg.de>
Hi Paul,
On 9/16/2021 7:22 AM, Paul Menzel wrote:
> Building Linux for ppc64le with Ubuntu clang version 12.0.0-3ubuntu1~21.04.1
> shows the warning below.
>
> arch/powerpc/boot/inffast.c:20:1: warning: unused function 'get_unaligned16' [-Wunused-function]
> get_unaligned16(const unsigned short *p)
> ^
> 1 warning generated.
>
> Fix it, by moving the check from the preprocessor to C, so the compiler
> sees the use.
>
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
> lib/zlib_inflate/inffast.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/lib/zlib_inflate/inffast.c b/lib/zlib_inflate/inffast.c
> index f19c4fbe1be7..444ad3c3ccd3 100644
> --- a/lib/zlib_inflate/inffast.c
> +++ b/lib/zlib_inflate/inffast.c
> @@ -254,11 +254,7 @@ void inflate_fast(z_streamp strm, unsigned start)
> sfrom = (unsigned short *)(from);
> loops = len >> 1;
> do
> -#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> - *sout++ = *sfrom++;
> -#else
> - *sout++ = get_unaligned16(sfrom++);
> -#endif
> + *sout++ = CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS ? *sfrom++ : get_unaligned16(sfrom++);
> while (--loops);
> out = (unsigned char *)sout;
> from = (unsigned char *)sfrom;
>
Thanks for the patch. This should probably be
IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) ? ...
which matches the rest of the kernel tree, as certain CONFIG_... values
are not guaranteed to always be defined.
Cheers,
Nathan
^ 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