* [PATCH -next] trap: Cleanup trap_init()
From: Kefeng Wang @ 2021-08-12 12:36 UTC (permalink / raw)
To: linux-snps-arc, linux-kernel, linux-arm-kernel, uclinux-h8-devel,
linux-hexagon, openrisc, linux-parisc, linuxppc-dev, linux-riscv,
linux-um, linux-mm
Cc: Jonas Bonn, Kefeng Wang, Andrew Morton, Yoshinori Sato,
Helge Deller, Paul Walmsley, Russell King, Ley Foon Tan,
Stefan Kristiansson, James E . J . Bottomley, Richard Weinberger,
Paul Mackerras, Vineet Gupta, Stafford Horne, Jeff Dike,
Anton Ivanov
There are some empty trap_init() in different ARCHs, introduce
a new weak trap_init() function to cleanup them.
Cc: Vineet Gupta <vgupta@kernel.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Ley Foon Tan <ley.foon.tan@intel.com>
Cc: Jonas Bonn <jonas@southpole.se>
Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
Cc: Stafford Horne <shorne@gmail.com>
Cc: James E.J. Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Helge Deller <deller@gmx.de>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Paul Walmsley <palmerdabbelt@google.com>
Cc: Jeff Dike <jdike@addtoit.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
arch/arc/kernel/traps.c | 5 -----
arch/arm/kernel/traps.c | 5 -----
arch/h8300/kernel/traps.c | 4 ----
arch/hexagon/kernel/traps.c | 4 ----
arch/nds32/kernel/traps.c | 5 -----
arch/nios2/kernel/traps.c | 5 -----
arch/openrisc/kernel/traps.c | 5 -----
arch/parisc/kernel/traps.c | 4 ----
arch/powerpc/kernel/traps.c | 5 -----
arch/riscv/kernel/traps.c | 5 -----
arch/um/kernel/trap.c | 4 ----
init/main.c | 2 ++
12 files changed, 2 insertions(+), 51 deletions(-)
diff --git a/arch/arc/kernel/traps.c b/arch/arc/kernel/traps.c
index 57235e5c0cea..6b83e3f2b41c 100644
--- a/arch/arc/kernel/traps.c
+++ b/arch/arc/kernel/traps.c
@@ -20,11 +20,6 @@
#include <asm/unaligned.h>
#include <asm/kprobes.h>
-void __init trap_init(void)
-{
- return;
-}
-
void die(const char *str, struct pt_regs *regs, unsigned long address)
{
show_kernel_fault_diag(str, regs, address);
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 64308e3a5d0c..e9b4f2b49bd8 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -781,11 +781,6 @@ void abort(void)
panic("Oops failed to kill thread");
}
-void __init trap_init(void)
-{
- return;
-}
-
#ifdef CONFIG_KUSER_HELPERS
static void __init kuser_init(void *vectors)
{
diff --git a/arch/h8300/kernel/traps.c b/arch/h8300/kernel/traps.c
index 5d8b969cd8f3..bdbe988d8dbc 100644
--- a/arch/h8300/kernel/traps.c
+++ b/arch/h8300/kernel/traps.c
@@ -39,10 +39,6 @@ void __init base_trap_init(void)
{
}
-void __init trap_init(void)
-{
-}
-
asmlinkage void set_esp0(unsigned long ssp)
{
current->thread.esp0 = ssp;
diff --git a/arch/hexagon/kernel/traps.c b/arch/hexagon/kernel/traps.c
index 904134b37232..edfc35dafeb1 100644
--- a/arch/hexagon/kernel/traps.c
+++ b/arch/hexagon/kernel/traps.c
@@ -28,10 +28,6 @@
#define TRAP_SYSCALL 1
#define TRAP_DEBUG 0xdb
-void __init trap_init(void)
-{
-}
-
#ifdef CONFIG_GENERIC_BUG
/* Maybe should resemble arch/sh/kernel/traps.c ?? */
int is_valid_bugaddr(unsigned long addr)
diff --git a/arch/nds32/kernel/traps.c b/arch/nds32/kernel/traps.c
index ee0d9ae192a5..f06421c645af 100644
--- a/arch/nds32/kernel/traps.c
+++ b/arch/nds32/kernel/traps.c
@@ -183,11 +183,6 @@ void __pgd_error(const char *file, int line, unsigned long val)
}
extern char *exception_vector, *exception_vector_end;
-void __init trap_init(void)
-{
- return;
-}
-
void __init early_trap_init(void)
{
unsigned long ivb = 0;
diff --git a/arch/nios2/kernel/traps.c b/arch/nios2/kernel/traps.c
index b172da4eb1a9..596986a74a26 100644
--- a/arch/nios2/kernel/traps.c
+++ b/arch/nios2/kernel/traps.c
@@ -105,11 +105,6 @@ void show_stack(struct task_struct *task, unsigned long *stack,
printk("%s\n", loglvl);
}
-void __init trap_init(void)
-{
- /* Nothing to do here */
-}
-
/* Breakpoint handler */
asmlinkage void breakpoint_c(struct pt_regs *fp)
{
diff --git a/arch/openrisc/kernel/traps.c b/arch/openrisc/kernel/traps.c
index 4d61333c2623..aa1e709405ac 100644
--- a/arch/openrisc/kernel/traps.c
+++ b/arch/openrisc/kernel/traps.c
@@ -231,11 +231,6 @@ void unhandled_exception(struct pt_regs *regs, int ea, int vector)
die("Oops", regs, 9);
}
-void __init trap_init(void)
-{
- /* Nothing needs to be done */
-}
-
asmlinkage void do_trap(struct pt_regs *regs, unsigned long address)
{
force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)regs->pc);
diff --git a/arch/parisc/kernel/traps.c b/arch/parisc/kernel/traps.c
index 8d8441d4562a..747c328fb886 100644
--- a/arch/parisc/kernel/traps.c
+++ b/arch/parisc/kernel/traps.c
@@ -859,7 +859,3 @@ void __init early_trap_init(void)
initialize_ivt(&fault_vector_20);
}
-
-void __init trap_init(void)
-{
-}
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index e103b89234cd..91efb5c6f2f3 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -2209,11 +2209,6 @@ DEFINE_INTERRUPT_HANDLER(kernel_bad_stack)
die("Bad kernel stack pointer", regs, SIGABRT);
}
-void __init trap_init(void)
-{
-}
-
-
#ifdef CONFIG_PPC_EMULATED_STATS
#define WARN_EMULATED_SETUP(type) .type = { .name = #type }
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 0a98fd0ddfe9..0daaa3e4630d 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -199,11 +199,6 @@ int is_valid_bugaddr(unsigned long pc)
}
#endif /* CONFIG_GENERIC_BUG */
-/* stvec & scratch is already set from head.S */
-void __init trap_init(void)
-{
-}
-
#ifdef CONFIG_VMAP_STACK
static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)],
overflow_stack)__aligned(16);
diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
index ad12f78bda7e..3198c4767387 100644
--- a/arch/um/kernel/trap.c
+++ b/arch/um/kernel/trap.c
@@ -311,7 +311,3 @@ void winch(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs)
{
do_IRQ(WINCH_IRQ, regs);
}
-
-void trap_init(void)
-{
-}
diff --git a/init/main.c b/init/main.c
index 4b4897b791fd..863e5087263d 100644
--- a/init/main.c
+++ b/init/main.c
@@ -775,6 +775,8 @@ void __init __weak poking_init(void) { }
void __init __weak pgtable_cache_init(void) { }
+void __init __weak trap_init(void) { }
+
bool initcall_debug;
core_param(initcall_debug, initcall_debug, bool, 0644);
--
2.26.2
^ permalink raw reply related
* Re: [PATCH v6 1/2] tty: hvc: pass DMA capable memory to put_chars()
From: kernel test robot @ 2021-08-12 12:26 UTC (permalink / raw)
To: Xianting Tian, gregkh, jirislaby, amit, arnd, osandov
Cc: kbuild-all, Xianting Tian, linux-kernel, virtualization,
clang-built-linux, linuxppc-dev
In-Reply-To: <20210812094532.145497-2-xianting.tian@linux.alibaba.com>
[-- Attachment #1: Type: text/plain, Size: 3818 bytes --]
Hi Xianting,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on char-misc/char-misc-testing soc/for-next v5.14-rc5 next-20210812]
[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/Xianting-Tian/make-hvc-pass-dma-capable-memory-to-its-backend/20210812-174847
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: hexagon-randconfig-r041-20210812 (attached as .config)
compiler: clang version 12.0.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/9f2925b5429149ceb0ea6eeaa8c81d422c3124fc
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Xianting-Tian/make-hvc-pass-dma-capable-memory-to-its-backend/20210812-174847
git checkout 9f2925b5429149ceb0ea6eeaa8c81d422c3124fc
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=hexagon
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/tty/hvc/hvc_console.c:190:26: warning: variable 'hp' is uninitialized when used here [-Wuninitialized]
spin_unlock_irqrestore(&hp->c_lock, flags);
^~
drivers/tty/hvc/hvc_console.c:149:23: note: initialize the variable 'hp' to silence this warning
struct hvc_struct *hp;
^
= NULL
1 warning generated.
vim +/hp +190 drivers/tty/hvc/hvc_console.c
136
137 /*
138 * Console APIs, NOT TTY. These APIs are available immediately when
139 * hvc_console_setup() finds adapters.
140 */
141
142 static void hvc_console_print(struct console *co, const char *b,
143 unsigned count)
144 {
145 char *c;
146 unsigned i = 0, n = 0;
147 int r, donecr = 0, index = co->index;
148 unsigned long flags;
149 struct hvc_struct *hp;
150
151 /* Console access attempt outside of acceptable console range. */
152 if (index >= MAX_NR_HVC_CONSOLES)
153 return;
154
155 /* This console adapter was removed so it is not usable. */
156 if (vtermnos[index] == -1 || !cons_outbuf[index])
157 return;
158
159 c = cons_outbuf[index];
160
161 spin_lock_irqsave(&hp->c_lock, flags);
162 while (count > 0 || i > 0) {
163 if (count > 0 && i < sizeof(c)) {
164 if (b[n] == '\n' && !donecr) {
165 c[i++] = '\r';
166 donecr = 1;
167 } else {
168 c[i++] = b[n++];
169 donecr = 0;
170 --count;
171 }
172 } else {
173 r = cons_ops[index]->put_chars(vtermnos[index], c, i);
174 if (r <= 0) {
175 /* throw away characters on error
176 * but spin in case of -EAGAIN */
177 if (r != -EAGAIN) {
178 i = 0;
179 } else {
180 hvc_console_flush(cons_ops[index],
181 vtermnos[index]);
182 }
183 } else if (r > 0) {
184 i -= r;
185 if (i > 0)
186 memmove(c, c+r, i);
187 }
188 }
189 }
> 190 spin_unlock_irqrestore(&hp->c_lock, flags);
191 hvc_console_flush(cons_ops[index], vtermnos[index]);
192 }
193
---
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: 28662 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/book3s64/radix: make tlb_single_page_flush_ceiling a debugfs entry
From: Michael Ellerman @ 2021-08-12 12:14 UTC (permalink / raw)
To: Aneesh Kumar K.V, Christophe Leroy, linuxppc-dev
In-Reply-To: <bb3855e1-f15d-1df6-5699-9addc69ce1db@linux.ibm.com>
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> On 8/12/21 12:58 PM, Michael Ellerman wrote:
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>> Le 10/08/2021 à 06:53, Aneesh Kumar K.V a écrit :
>>>> Similar to x86/s390 add a debugfs file to tune tlb_single_page_flush_ceiling.
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>> ---
>>>> arch/powerpc/mm/book3s64/radix_tlb.c | 48 ++++++++++++++++++++++++++++
>>>> 1 file changed, 48 insertions(+)
>>>>
>>>> diff --git a/arch/powerpc/mm/book3s64/radix_tlb.c b/arch/powerpc/mm/book3s64/radix_tlb.c
>>>> index aefc100d79a7..5cca0fe130e7 100644
>>>> --- a/arch/powerpc/mm/book3s64/radix_tlb.c
>>>> +++ b/arch/powerpc/mm/book3s64/radix_tlb.c
>>>> @@ -17,6 +17,7 @@
>> ...
>>>> +
>>>> +static int __init create_tlb_single_page_flush_ceiling(void)
>>>> +{
>>>> + debugfs_create_file("tlb_single_page_flush_ceiling", S_IRUSR | S_IWUSR,
>>>> + powerpc_debugfs_root, NULL, &fops_tlbflush);
>>>
>>> Could you just use debugfs_create_u32() instead of re-implementing simple read and write ?
>>
>> Yeah AFAICS that should work fine.
>>
>> It could probably even be a u16?
>
> I was looking at switching all that to u64. Should i fallback to u16,
> considering a tlb_signle_page_flush_ceiling value larger that 2**16
> doesn't make sense?
Hmm, if we make it u16 and someone writes a value >= 2^16 it just
truncates the value to 0, which is a bit unfortunate.
So maybe just make it u32, that way if someone writes a stupidly large
value it stays large.
cheers
^ permalink raw reply
* Re: [PATCH 07/11] treewide: Replace the use of mem_encrypt_active() with prot_guest_has()
From: Kirill A. Shutemov @ 2021-08-12 10:07 UTC (permalink / raw)
To: Tom Lendacky
Cc: Kuppuswamy, Sathyanarayanan, linux-efi, Brijesh Singh, kvm,
Peter Zijlstra, Dave Hansen, dri-devel, platform-driver-x86,
Will Deacon, linux-s390, Andi Kleen, Baoquan He, Joerg Roedel,
x86, amd-gfx, David Airlie, Ingo Molnar,
linux-graphics-maintainer, Dave Young, Tianyu Lan,
Thomas Zimmermann, Maarten Lankhorst, Maxime Ripard,
Borislav Petkov, Andy Lutomirski, Thomas Gleixner, kexec,
linux-kernel, iommu, Daniel Vetter, linux-fsdevel, linuxppc-dev
In-Reply-To: <0a819549-e481-c004-7da8-82ba427b13ce@amd.com>
On Wed, Aug 11, 2021 at 10:52:55AM -0500, Tom Lendacky wrote:
> On 8/11/21 7:19 AM, Kirill A. Shutemov wrote:
> > On Tue, Aug 10, 2021 at 02:48:54PM -0500, Tom Lendacky wrote:
> >> On 8/10/21 1:45 PM, Kuppuswamy, Sathyanarayanan wrote:
> >>>
> >>>
> >>> On 7/27/21 3:26 PM, Tom Lendacky wrote:
> >>>> diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
> >>>> index de01903c3735..cafed6456d45 100644
> >>>> --- a/arch/x86/kernel/head64.c
> >>>> +++ b/arch/x86/kernel/head64.c
> >>>> @@ -19,7 +19,7 @@
> >>>> #include <linux/start_kernel.h>
> >>>> #include <linux/io.h>
> >>>> #include <linux/memblock.h>
> >>>> -#include <linux/mem_encrypt.h>
> >>>> +#include <linux/protected_guest.h>
> >>>> #include <linux/pgtable.h>
> >>>> #include <asm/processor.h>
> >>>> @@ -285,7 +285,7 @@ unsigned long __head __startup_64(unsigned long
> >>>> physaddr,
> >>>> * there is no need to zero it after changing the memory encryption
> >>>> * attribute.
> >>>> */
> >>>> - if (mem_encrypt_active()) {
> >>>> + if (prot_guest_has(PATTR_MEM_ENCRYPT)) {
> >>>> vaddr = (unsigned long)__start_bss_decrypted;
> >>>> vaddr_end = (unsigned long)__end_bss_decrypted;
> >>>
> >>>
> >>> Since this change is specific to AMD, can you replace PATTR_MEM_ENCRYPT with
> >>> prot_guest_has(PATTR_SME) || prot_guest_has(PATTR_SEV). It is not used in
> >>> TDX.
> >>
> >> This is a direct replacement for now.
> >
> > With current implementation of prot_guest_has() for TDX it breaks boot for
> > me.
> >
> > Looking at code agains, now I *think* the reason is accessing a global
> > variable from __startup_64() inside TDX version of prot_guest_has().
> >
> > __startup_64() is special. If you access any global variable you need to
> > use fixup_pointer(). See comment before __startup_64().
> >
> > I'm not sure how you get away with accessing sme_me_mask directly from
> > there. Any clues? Maybe just a luck and complier generates code just right
> > for your case, I donno.
>
> Hmm... yeah, could be that the compiler is using rip-relative addressing
> for it because it lives in the .data section?
I guess. It has to be fixed. It may break with complier upgrade or any
random change around the code.
BTW, does it work with clang for you?
> For the static variables in mem_encrypt_identity.c I did an assembler rip
> relative LEA, but probably could have passed physaddr to sme_enable() and
> used a fixup_pointer() style function, instead.
Sounds like a plan.
> > A separate point is that TDX version of prot_guest_has() relies on
> > cpu_feature_enabled() which is not ready at this point.
>
> Does TDX have to do anything special to make memory able to be shared with
> the hypervisor?
Yes. But there's nothing that required any changes in early boot. It
handled in ioremap/set_memory.
> You might have to use something that is available earlier
> than cpu_feature_enabled() in that case (should you eventually support
> kvmclock).
Maybe.
> > I think __bss_decrypted fixup has to be done if sme_me_mask is non-zero.
> > Or just do it uncoditionally because it's NOP for sme_me_mask == 0.
>
> For SNP, we'll have to additionally call the HV to update the RMP to make
> the memory shared. But that could also be done unconditionally since the
> early_snp_set_memory_shared() routine will check for SNP before doing
> anything.
--
Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH] powerpc/interrupt: Do not call single_step_exception() from other exceptions
From: Nicholas Piggin @ 2021-08-12 10:01 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, fthain,
Michael Ellerman, Paul Mackerras, userm57
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <aed174f5cbc06f2cf95233c071d8aac948e46043.1628611921.git.christophe.leroy@csgroup.eu>
Excerpts from Christophe Leroy's message of August 11, 2021 2:13 am:
> single_step_exception() is called by emulate_single_step() which
> is called from (at least) alignment exception() handler and
> program_check_exception() handler.
>
> Redefine it as a regular __single_step_exception() which is called
> by both single_step_exception() handler and emulate_single_step()
> function.
>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> Fixes: 3a96570ffceb ("powerpc: convert interrupt handlers to use wrappers")
> Cc: stable@vger.kernel.org
> Cc: Stan Johnson <userm57@yahoo.com>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Finn Thain <fthain@linux-m68k.org>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/kernel/traps.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index dfbce527c98e..d56254f05e17 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -1104,7 +1104,7 @@ DEFINE_INTERRUPT_HANDLER(RunModeException)
> _exception(SIGTRAP, regs, TRAP_UNK, 0);
> }
>
> -DEFINE_INTERRUPT_HANDLER(single_step_exception)
> +static void __single_step_exception(struct pt_regs *regs)
> {
> clear_single_step(regs);
> clear_br_trace(regs);
> @@ -1121,6 +1121,11 @@ DEFINE_INTERRUPT_HANDLER(single_step_exception)
> _exception(SIGTRAP, regs, TRAP_TRACE, regs->nip);
> }
>
> +DEFINE_INTERRUPT_HANDLER(single_step_exception)
> +{
> + __single_step_exception(regs);
> +}
> +
> /*
> * After we have successfully emulated an instruction, we have to
> * check if the instruction was being single-stepped, and if so,
> @@ -1130,7 +1135,7 @@ DEFINE_INTERRUPT_HANDLER(single_step_exception)
> static void emulate_single_step(struct pt_regs *regs)
> {
> if (single_stepping(regs))
> - single_step_exception(regs);
> + __single_step_exception(regs);
> }
>
> static inline int __parse_fpscr(unsigned long fpscr)
> --
> 2.25.0
>
>
^ permalink raw reply
* Re: [PATCH] powerpc/interrupt: Fix OOPS by not calling do_IRQ() from timer_interrupt()
From: Nicholas Piggin @ 2021-08-12 10:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, fthain,
Michael Ellerman, Paul Mackerras, userm57
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <c17d234f4927d39a1d7100864a8e1145323d33a0.1628611927.git.christophe.leroy@csgroup.eu>
Excerpts from Christophe Leroy's message of August 11, 2021 2:13 am:
> An interrupt handler shall not be called from another interrupt
> handler otherwise this leads to problems like the following:
>
> Kernel attempted to write user page (afd4fa84) - exploit attempt? (uid: 1000)
> ------------[ cut here ]------------
> Bug: Write fault blocked by KUAP!
> WARNING: CPU: 0 PID: 1617 at arch/powerpc/mm/fault.c:230 do_page_fault+0x484/0x720
> Modules linked in:
> CPU: 0 PID: 1617 Comm: sshd Tainted: G W 5.13.0-pmac-00010-g8393422eb77 #7
> NIP: c001b77c LR: c001b77c CTR: 00000000
> REGS: cb9e5bc0 TRAP: 0700 Tainted: G W (5.13.0-pmac-00010-g8393422eb77)
> MSR: 00021032 <ME,IR,DR,RI> CR: 24942424 XER: 00000000
>
> GPR00: c001b77c cb9e5c80 c1582c00 00000021 3ffffbff 085b0000 00000027 c8eb644c
> GPR08: 00000023 00000000 00000000 00000000 24942424 0063f8c8 00000000 000186a0
> GPR16: afd52dd4 afd52dd0 afd52dcc afd52dc8 0065a990 c07640c4 cb9e5e98 cb9e5e90
> GPR24: 00000040 afd4fa96 00000040 02000000 c1fda6c0 afd4fa84 00000300 cb9e5cc0
> NIP [c001b77c] do_page_fault+0x484/0x720
> LR [c001b77c] do_page_fault+0x484/0x720
> Call Trace:
> [cb9e5c80] [c001b77c] do_page_fault+0x484/0x720 (unreliable)
> [cb9e5cb0] [c000424c] DataAccess_virt+0xd4/0xe4
> --- interrupt: 300 at __copy_tofrom_user+0x110/0x20c
> NIP: c001f9b4 LR: c03250a0 CTR: 00000004
> REGS: cb9e5cc0 TRAP: 0300 Tainted: G W (5.13.0-pmac-00010-g8393422eb77)
> MSR: 00009032 <EE,ME,IR,DR,RI> CR: 48028468 XER: 20000000
> DAR: afd4fa84 DSISR: 0a000000
> GPR00: 20726f6f cb9e5d80 c1582c00 00000004 cb9e5e3a 00000016 afd4fa80 00000000
> GPR08: 3835202d 72777872 2d78722d 00000004 28028464 0063f8c8 00000000 000186a0
> GPR16: afd52dd4 afd52dd0 afd52dcc afd52dc8 0065a990 c07640c4 cb9e5e98 cb9e5e90
> GPR24: 00000040 afd4fa96 00000040 cb9e5e0c 00000daa a0000000 cb9e5e98 afd4fa56
> NIP [c001f9b4] __copy_tofrom_user+0x110/0x20c
> LR [c03250a0] _copy_to_iter+0x144/0x990
> --- interrupt: 300
> [cb9e5d80] [c03e89c0] n_tty_read+0xa4/0x598 (unreliable)
> [cb9e5df0] [c03e2a0c] tty_read+0xdc/0x2b4
> [cb9e5e80] [c0156bf8] vfs_read+0x274/0x340
> [cb9e5f00] [c01571ac] ksys_read+0x70/0x118
> [cb9e5f30] [c0016048] ret_from_syscall+0x0/0x28
> --- interrupt: c00 at 0xa7855c88
> NIP: a7855c88 LR: a7855c5c CTR: 00000000
> REGS: cb9e5f40 TRAP: 0c00 Tainted: G W (5.13.0-pmac-00010-g8393422eb77)
> MSR: 0000d032 <EE,PR,ME,IR,DR,RI> CR: 2402446c XER: 00000000
>
> GPR00: 00000003 afd4ec70 a72137d0 0000000b afd4ecac 00004000 0065a990 00000800
> GPR08: 00000000 a7947930 00000000 00000004 c15831b0 0063f8c8 00000000 000186a0
> GPR16: afd52dd4 afd52dd0 afd52dcc afd52dc8 0065a990 0065a9e0 00000001 0065fac0
> GPR24: 00000000 00000089 00664050 00000000 00668e30 a720c8dc a7943ff4 0065f9b0
> NIP [a7855c88] 0xa7855c88
> LR [a7855c5c] 0xa7855c5c
> --- interrupt: c00
> Instruction dump:
> 3884aa88 38630178 48076861 807f0080 48042e45 2f830000 419e0148 3c80c079
> 3c60c076 38841be4 386301c0 4801f705 <0fe00000> 3860000b 4bfffe30 3c80c06b
> ---[ end trace fd69b91a8046c2e5 ]---
>
> Here the problem is that by re-enterring an exception handler,
> kuap_save_and_lock() is called a second time with this time KUAP
> access locked, leading to regs->kuap being overwritten hence
> KUAP not being unlocked at exception exit as expected.
>
> Do not call do_IRQ() from timer_interrupt() directly. Instead,
> redefine do_IRQ() as a standard function named __do_IRQ(), and
> call it from both do_IRQ() and time_interrupt() handlers.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> Reported-by: Stan Johnson <userm57@yahoo.com>
> Fixes: 3a96570ffceb ("powerpc: convert interrupt handlers to use wrappers")
> Cc: stable@vger.kernel.org
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Finn Thain <fthain@linux-m68k.org>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/include/asm/interrupt.h | 3 +++
> arch/powerpc/include/asm/irq.h | 2 +-
> arch/powerpc/kernel/irq.c | 7 ++++++-
> arch/powerpc/kernel/time.c | 2 +-
> 4 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
> index fc4702bdd119..1e984a35a39f 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -590,6 +590,9 @@ DECLARE_INTERRUPT_HANDLER_NMI(hmi_exception_realmode);
>
> DECLARE_INTERRUPT_HANDLER_ASYNC(TAUException);
>
> +/* irq.c */
> +DECLARE_INTERRUPT_HANDLER_ASYNC(do_IRQ);
> +
> void __noreturn unrecoverable_exception(struct pt_regs *regs);
>
> void replay_system_reset(void);
> diff --git a/arch/powerpc/include/asm/irq.h b/arch/powerpc/include/asm/irq.h
> index 4982f3711fc3..2b3278534bc1 100644
> --- a/arch/powerpc/include/asm/irq.h
> +++ b/arch/powerpc/include/asm/irq.h
> @@ -52,7 +52,7 @@ extern void *mcheckirq_ctx[NR_CPUS];
> extern void *hardirq_ctx[NR_CPUS];
> extern void *softirq_ctx[NR_CPUS];
>
> -extern void do_IRQ(struct pt_regs *regs);
> +void __do_IRQ(struct pt_regs *regs);
> extern void __init init_IRQ(void);
> extern void __do_irq(struct pt_regs *regs);
>
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index 91e63eac4e8f..551b653228c4 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -750,7 +750,7 @@ void __do_irq(struct pt_regs *regs)
> trace_irq_exit(regs);
> }
>
> -DEFINE_INTERRUPT_HANDLER_ASYNC(do_IRQ)
> +void __do_IRQ(struct pt_regs *regs)
> {
> struct pt_regs *old_regs = set_irq_regs(regs);
> void *cursp, *irqsp, *sirqsp;
> @@ -774,6 +774,11 @@ DEFINE_INTERRUPT_HANDLER_ASYNC(do_IRQ)
> set_irq_regs(old_regs);
> }
>
> +DEFINE_INTERRUPT_HANDLER_ASYNC(do_IRQ)
> +{
> + __do_IRQ(regs);
> +}
> +
> static void *__init alloc_vm_stack(void)
> {
> return __vmalloc_node(THREAD_SIZE, THREAD_ALIGN, THREADINFO_GFP,
> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index e45ce427bffb..c487ba5a6e11 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -586,7 +586,7 @@ DEFINE_INTERRUPT_HANDLER_ASYNC(timer_interrupt)
>
> #if defined(CONFIG_PPC32) && defined(CONFIG_PPC_PMAC)
> if (atomic_read(&ppc_n_lost_interrupts) != 0)
> - do_IRQ(regs);
> + __do_IRQ(regs);
> #endif
>
> old_regs = set_irq_regs(regs);
> --
> 2.25.0
>
>
^ permalink raw reply
* [PATCH v6 2/2] virtio-console: remove unnecessary kmemdup()
From: Xianting Tian @ 2021-08-12 9:45 UTC (permalink / raw)
To: gregkh, jirislaby, amit, arnd, osandov
Cc: Xianting Tian, linuxppc-dev, linux-kernel, virtualization
In-Reply-To: <20210812094532.145497-1-xianting.tian@linux.alibaba.com>
hvc framework will never pass stack memory to the put_chars() function,
So the calling of kmemdup() is unnecessary, we can remove it.
This revert commit c4baad5029 ("virtio-console: avoid DMA from stack")
Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com>
---
drivers/char/virtio_console.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 7eaf303a7..4ed3ffb1d 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1117,8 +1117,6 @@ static int put_chars(u32 vtermno, const char *buf, int count)
{
struct port *port;
struct scatterlist sg[1];
- void *data;
- int ret;
if (unlikely(early_put_chars))
return early_put_chars(vtermno, buf, count);
@@ -1127,14 +1125,8 @@ static int put_chars(u32 vtermno, const char *buf, int count)
if (!port)
return -EPIPE;
- data = kmemdup(buf, count, GFP_ATOMIC);
- if (!data)
- return -ENOMEM;
-
- sg_init_one(sg, data, count);
- ret = __send_to_port(port, sg, 1, count, data, false);
- kfree(data);
- return ret;
+ sg_init_one(sg, buf, count);
+ return __send_to_port(port, sg, 1, count, (void *)buf, false);
}
/*
--
2.17.1
^ permalink raw reply related
* [PATCH v6 1/2] tty: hvc: pass DMA capable memory to put_chars()
From: Xianting Tian @ 2021-08-12 9:45 UTC (permalink / raw)
To: gregkh, jirislaby, amit, arnd, osandov
Cc: Xianting Tian, linuxppc-dev, linux-kernel, virtualization
In-Reply-To: <20210812094532.145497-1-xianting.tian@linux.alibaba.com>
As well known, hvc backend can register its opertions to hvc backend.
the opertions contain put_chars(), get_chars() and so on.
Some hvc backend may do dma in its opertions. eg, put_chars() of
virtio-console. But in the code of hvc framework, it may pass DMA
incapable memory to put_chars() under a specific configuration, which
is explained in commit c4baad5029(virtio-console: avoid DMA from stack):
1, c[] is on stack,
hvc_console_print():
char c[N_OUTBUF] __ALIGNED__;
cons_ops[index]->put_chars(vtermnos[index], c, i);
2, ch is on stack,
static void hvc_poll_put_char(,,char ch)
{
struct tty_struct *tty = driver->ttys[0];
struct hvc_struct *hp = tty->driver_data;
int n;
do {
n = hp->ops->put_chars(hp->vtermno, &ch, 1);
} while (n <= 0);
}
Commit c4baad5029 is just the fix to avoid DMA from stack memory, which
is passed to virtio-console by hvc framework in above code. But I think
the fix is aggressive, it directly uses kmemdup() to alloc new buffer
from kmalloc area and do memcpy no matter the memory is in kmalloc area
or not. But most importantly, it should better be fixed in the hvc
framework, by changing it to never pass stack memory to the put_chars()
function in the first place. Otherwise, we still face the same issue if
a new hvc backend using dma added in the furture.
We make 'char c[N_OUTBUF]' part of 'struct hvc_struct', so hp->c is no
longer the stack memory. we can use it in above two cases.
Other fix is use L1_CACHE_BYTES as the alignment, use 'sizeof(long)' as
dma alignment is wrong. And use struct_size() to calculate size of
hvc_struct.
Introduce another array(cons_outbuf[]) for the hp->c pointers next to
the cons_ops[] and vtermnos[] arrays.
With the patch, we can remove the fix c4baad5029.
Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com>
Tested-by: Xianting Tian <xianting.tian@linux.alibaba.com>
---
drivers/tty/hvc/hvc_console.c | 40 +++++++++++++++++++++--------------
drivers/tty/hvc/hvc_console.h | 16 ++++++++++++--
2 files changed, 38 insertions(+), 18 deletions(-)
diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 5bb8c4e44..c56564eb7 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -41,16 +41,6 @@
*/
#define HVC_CLOSE_WAIT (HZ/100) /* 1/10 of a second */
-/*
- * These sizes are most efficient for vio, because they are the
- * native transfer size. We could make them selectable in the
- * future to better deal with backends that want other buffer sizes.
- */
-#define N_OUTBUF 16
-#define N_INBUF 16
-
-#define __ALIGNED__ __attribute__((__aligned__(sizeof(long))))
-
static struct tty_driver *hvc_driver;
static struct task_struct *hvc_task;
@@ -142,6 +132,7 @@ static int hvc_flush(struct hvc_struct *hp)
static const struct hv_ops *cons_ops[MAX_NR_HVC_CONSOLES];
static uint32_t vtermnos[MAX_NR_HVC_CONSOLES] =
{[0 ... MAX_NR_HVC_CONSOLES - 1] = -1};
+static char *cons_outbuf[MAX_NR_HVC_CONSOLES];
/*
* Console APIs, NOT TTY. These APIs are available immediately when
@@ -151,18 +142,23 @@ static uint32_t vtermnos[MAX_NR_HVC_CONSOLES] =
static void hvc_console_print(struct console *co, const char *b,
unsigned count)
{
- char c[N_OUTBUF] __ALIGNED__;
+ char *c;
unsigned i = 0, n = 0;
int r, donecr = 0, index = co->index;
+ unsigned long flags;
+ struct hvc_struct *hp;
/* Console access attempt outside of acceptable console range. */
if (index >= MAX_NR_HVC_CONSOLES)
return;
/* This console adapter was removed so it is not usable. */
- if (vtermnos[index] == -1)
+ if (vtermnos[index] == -1 || !cons_outbuf[index])
return;
+ c = cons_outbuf[index];
+
+ spin_lock_irqsave(&hp->c_lock, flags);
while (count > 0 || i > 0) {
if (count > 0 && i < sizeof(c)) {
if (b[n] == '\n' && !donecr) {
@@ -191,6 +187,7 @@ static void hvc_console_print(struct console *co, const char *b,
}
}
}
+ spin_unlock_irqrestore(&hp->c_lock, flags);
hvc_console_flush(cons_ops[index], vtermnos[index]);
}
@@ -878,9 +875,19 @@ static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch)
struct tty_struct *tty = driver->ttys[0];
struct hvc_struct *hp = tty->driver_data;
int n;
+ unsigned long flags;
+ char *c;
+
+ if (!hp || !cons_outbuf[hp->index])
+ return;
+
+ c = cons_outbuf[hp->index];
do {
- n = hp->ops->put_chars(hp->vtermno, &ch, 1);
+ spin_lock_irqsave(&hp->c_lock, flags);
+ c[0] = ch;
+ n = hp->ops->put_chars(hp->vtermno, c, 1);
+ spin_unlock_irqrestore(&hp->c_lock, flags);
} while (n <= 0);
}
#endif
@@ -922,8 +929,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
return ERR_PTR(err);
}
- hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size,
- GFP_KERNEL);
+ hp = kzalloc(struct_size(hp, outbuf, outbuf_size), GFP_KERNEL);
if (!hp)
return ERR_PTR(-ENOMEM);
@@ -931,13 +937,13 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
hp->data = data;
hp->ops = ops;
hp->outbuf_size = outbuf_size;
- hp->outbuf = &((char *)hp)[ALIGN(sizeof(*hp), sizeof(long))];
tty_port_init(&hp->port);
hp->port.ops = &hvc_port_ops;
INIT_WORK(&hp->tty_resize, hvc_set_winsz);
spin_lock_init(&hp->lock);
+ spin_lock_init(&hp->c_lock);
mutex_lock(&hvc_structs_mutex);
/*
@@ -964,6 +970,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
if (i < MAX_NR_HVC_CONSOLES) {
cons_ops[i] = ops;
vtermnos[i] = vtermno;
+ cons_outbuf[i] = hp->c;
}
list_add_tail(&(hp->next), &hvc_structs);
@@ -988,6 +995,7 @@ int hvc_remove(struct hvc_struct *hp)
if (hp->index < MAX_NR_HVC_CONSOLES) {
vtermnos[hp->index] = -1;
cons_ops[hp->index] = NULL;
+ cons_outbuf[hp->index] = NULL;
}
/* Don't whack hp->irq because tty_hangup() will need to free the irq. */
diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
index 18d005814..52374e2da 100644
--- a/drivers/tty/hvc/hvc_console.h
+++ b/drivers/tty/hvc/hvc_console.h
@@ -32,13 +32,21 @@
*/
#define HVC_ALLOC_TTY_ADAPTERS 8
+/*
+ * These sizes are most efficient for vio, because they are the
+ * native transfer size. We could make them selectable in the
+ * future to better deal with backends that want other buffer sizes.
+ */
+#define N_OUTBUF 16
+#define N_INBUF 16
+
+#define __ALIGNED__ __attribute__((__aligned__(L1_CACHE_BYTES)))
+
struct hvc_struct {
struct tty_port port;
spinlock_t lock;
int index;
int do_wakeup;
- char *outbuf;
- int outbuf_size;
int n_outbuf;
uint32_t vtermno;
const struct hv_ops *ops;
@@ -48,6 +56,10 @@ struct hvc_struct {
struct work_struct tty_resize;
struct list_head next;
unsigned long flags;
+ spinlock_t c_lock;
+ char c[N_OUTBUF] __ALIGNED__;
+ int outbuf_size;
+ char outbuf[0] __ALIGNED__;
};
/* implemented by a low level driver */
--
2.17.1
^ permalink raw reply related
* [PATCH v6 0/2] make hvc pass dma capable memory to its backend
From: Xianting Tian @ 2021-08-12 9:45 UTC (permalink / raw)
To: gregkh, jirislaby, amit, arnd, osandov
Cc: Xianting Tian, linuxppc-dev, linux-kernel, virtualization
Dear all,
This patch series make hvc framework pass DMA capable memory to
put_chars() of hvc backend(eg, virtio-console), and revert commit
c4baad5029 ("virtio-console: avoid DMA from stack”)
V1
virtio-console: avoid DMA from vmalloc area
https://lkml.org/lkml/2021/7/27/494
For v1 patch, Arnd Bergmann suggests to fix the issue in the first
place:
Make hvc pass DMA capable memory to put_chars()
The fix suggestion is included in v2.
V2
[PATCH 1/2] tty: hvc: pass DMA capable memory to put_chars()
https://lkml.org/lkml/2021/8/1/8
[PATCH 2/2] virtio-console: remove unnecessary kmemdup()
https://lkml.org/lkml/2021/8/1/9
For v2 patch, Arnd Bergmann suggests to make new buf part of the
hvc_struct structure, and fix the compile issue.
The fix suggestion is included in v3.
V3
[PATCH v3 1/2] tty: hvc: pass DMA capable memory to put_chars()
https://lkml.org/lkml/2021/8/3/1347
[PATCH v3 2/2] virtio-console: remove unnecessary kmemdup()
https://lkml.org/lkml/2021/8/3/1348
For v3 patch, Jiri Slaby suggests to make 'char c[N_OUTBUF]' part of
hvc_struct, and make 'hp->outbuf' aligned and use struct_size() to
calculate the size of hvc_struct. The fix suggestion is included in
v4.
V4
[PATCH v4 0/2] make hvc pass dma capable memory to its backend
https://lkml.org/lkml/2021/8/5/1350
[PATCH v4 1/2] tty: hvc: pass DMA capable memory to put_chars()
https://lkml.org/lkml/2021/8/5/1351
[PATCH v4 2/2] virtio-console: remove unnecessary kmemdup()
https://lkml.org/lkml/2021/8/5/1352
For v4 patch, Arnd Bergmann suggests to introduce another
array(cons_outbuf[]) for the buffer pointers next to the cons_ops[]
and vtermnos[] arrays. This fix included in this v5 patch.
V5
Arnd Bergmann suggests to use "L1_CACHE_BYTES" as dma alignment,
use 'sizeof(long)' as dma alignment is wrong. fix it in v6.
drivers/char/virtio_console.c | 12 ++----------
drivers/tty/hvc/hvc_console.c | 40 +++++++++++++++++++++--------------
drivers/tty/hvc/hvc_console.h | 16 ++++++++++++--
3 file changed
^ permalink raw reply
* Re: [PATCH v4 1/2] tty: hvc: pass DMA capable memory to put_chars()
From: Xianting TIan @ 2021-08-12 9:14 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jiri Slaby, Amit Shah, gregkh, Linux Kernel Mailing List,
open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE, Guo Ren,
linuxppc-dev, Omar Sandoval
In-Reply-To: <CAK8P3a2ykLvJkhX+wDAOHdyLHjPFAfhOxi5BNM9kTKv_8F7VQg@mail.gmail.com>
在 2021/8/12 下午4:54, Arnd Bergmann 写道:
> On Thu, Aug 12, 2021 at 10:08 AM Xianting TIan
> <xianting.tian@linux.alibaba.com> wrote:
>> 在 2021/8/6 下午10:51, Arnd Bergmann 写道:
>>> On Fri, Aug 6, 2021 at 5:01 AM Xianting Tian
>>>> +#define __ALIGNED__ __attribute__((__aligned__(sizeof(long))))
>>> I think you need a higher alignment for DMA buffers, instead of sizeof(long),
>>> I would suggest ARCH_DMA_MINALIGN.
>> As some ARCH(eg, x86, riscv) doesn't define ARCH_DMA_MINALIG, so i think
>> it 's better remain the code unchanged,
>>
>> I will send v5 patch soon.
> I think you could just use "L1_CACHE_BYTES" as the alignment in this case.
> This will make the structure slightly larger for architectures that do not have
> alignment constraints on DMA buffers, but using a smaller alignment is
> clearly wrong. Another option would be to use ARCH_KMALLOC_MINALIGN.
yes, I unstand you, the align size must L1_CACHE_BYTES at least.
>
> Note that there is a patch to add ARCH_DMA_MINALIGN to riscv already,
yes, I summited this patch, it is discussing, seems they don't want to
apply it.
> as some implementations do not have coherent DMA. I had failed to
> realized though that on x86 you do not get an ARCH_DMA_MINALIGN
> definition.
I didn't find the definition in arch/x86/include/asm/cache.h and other
place, x86 is dma coherent, it may doesn't need it.
>
> Arnd
^ permalink raw reply
* Re: [PATCH v2 1/2] KVM: Refactor kvm_arch_vcpu_fault() to return a struct page pointer
From: David Hildenbrand @ 2021-08-12 9:04 UTC (permalink / raw)
To: Hou Wenlong, kvm
Cc: x86, Wanpeng Li, linux-mips, H. Peter Anvin, Claudio Imbrenda,
Will Deacon, kvmarm, linux-s390, Janosch Frank, Marc Zyngier,
Joerg Roedel, Huacai Chen, Christian Borntraeger,
Aleksandar Markovic, Ingo Molnar, Catalin Marinas, Vasily Gorbik,
Suzuki K Poulose, Heiko Carstens, kvm-ppc, Borislav Petkov,
Thomas Gleixner, Alexandru Elisei, linux-arm-kernel, Jim Mattson,
Thomas Bogendoerfer, Sean Christopherson, Cornelia Huck,
linux-kernel, James Morse, Paolo Bonzini, Vitaly Kuznetsov,
linuxppc-dev
In-Reply-To: <1c510b24fc1d7cbae8aa4b69c0799ebd32e65b82.1628739116.git.houwenlong93@linux.alibaba.com>
On 12.08.21 06:02, Hou Wenlong wrote:
> From: Sean Christopherson <seanjc@google.com>
>
> Refactor kvm_arch_vcpu_fault() to return 'struct page *' instead of
> 'vm_fault_t' to simplify architecture specific implementations that do
> more than return SIGBUS. Currently this only applies to s390, but a
> future patch will move x86's pio_data handling into x86 where it belongs.
>
> No functional changed intended.
>
> Cc: Hou Wenlong <houwenlong93@linux.alibaba.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Hou Wenlong <houwenlong93@linux.alibaba.com>
> ---
> arch/arm64/kvm/arm.c | 4 ++--
> arch/mips/kvm/mips.c | 4 ++--
> arch/powerpc/kvm/powerpc.c | 4 ++--
> arch/s390/kvm/kvm-s390.c | 12 ++++--------
> arch/x86/kvm/x86.c | 4 ++--
> include/linux/kvm_host.h | 2 +-
> virt/kvm/kvm_main.c | 5 ++++-
> 7 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index e9a2b8f27792..83f4ffe3e4f2 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -161,9 +161,9 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> return ret;
> }
>
> -vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
> +struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
> {
> - return VM_FAULT_SIGBUS;
> + return NULL;
> }
>
>
> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> index af9dd029a4e1..ae79874e6fd2 100644
> --- a/arch/mips/kvm/mips.c
> +++ b/arch/mips/kvm/mips.c
> @@ -1053,9 +1053,9 @@ int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
> return -ENOIOCTLCMD;
> }
>
> -vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
> +struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
> {
> - return VM_FAULT_SIGBUS;
> + return NULL;
> }
>
> int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index be33b5321a76..b9c21f9ab784 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -2090,9 +2090,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> return r;
> }
>
> -vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
> +struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
> {
> - return VM_FAULT_SIGBUS;
> + return NULL;
> }
>
> static int kvm_vm_ioctl_get_pvinfo(struct kvm_ppc_pvinfo *pvinfo)
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 02574d7b3612..e1b69833e228 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4979,17 +4979,13 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> return r;
> }
>
> -vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
> +struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
> {
> #ifdef CONFIG_KVM_S390_UCONTROL
> - if ((vmf->pgoff == KVM_S390_SIE_PAGE_OFFSET)
> - && (kvm_is_ucontrol(vcpu->kvm))) {
> - vmf->page = virt_to_page(vcpu->arch.sie_block);
> - get_page(vmf->page);
> - return 0;
> - }
> + if (vmf->pgoff == KVM_S390_SIE_PAGE_OFFSET && kvm_is_ucontrol(vcpu->kvm))
> + return virt_to_page(vcpu->arch.sie_block);
> #endif
> - return VM_FAULT_SIGBUS;
> + return NULL;
> }
>
> /* Section: memory related */
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3cedc7cc132a..1e3bbe5cd33a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5347,9 +5347,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> return r;
> }
>
> -vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
> +struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
> {
> - return VM_FAULT_SIGBUS;
> + return NULL;
> }
>
> static int kvm_vm_ioctl_set_tss_addr(struct kvm *kvm, unsigned long addr)
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 492d183dd7d0..a949de534722 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -995,7 +995,7 @@ long kvm_arch_dev_ioctl(struct file *filp,
> unsigned int ioctl, unsigned long arg);
> long kvm_arch_vcpu_ioctl(struct file *filp,
> unsigned int ioctl, unsigned long arg);
> -vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf);
> +struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf);
>
> int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext);
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 30d322519253..f7d21418971b 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3448,7 +3448,10 @@ static vm_fault_t kvm_vcpu_fault(struct vm_fault *vmf)
> &vcpu->dirty_ring,
> vmf->pgoff - KVM_DIRTY_LOG_PAGE_OFFSET);
> else
> - return kvm_arch_vcpu_fault(vcpu, vmf);
> + page = kvm_arch_vcpu_fault(vcpu, vmf);
> + if (!page)
> + return VM_FAULT_SIGBUS;
> +
> get_page(page);
> vmf->page = page;
> return 0;
>
Reviewed-by: David Hildenbrand <david@redhat.com>
But at the same time I wonder if we should just get rid of
CONFIG_KVM_S390_UCONTROL and consequently kvm_arch_vcpu_fault().
In practice CONFIG_KVM_S390_UCONTROL, is never enabled in any reasonable
kernel build and consequently it's never tested; further, exposing the
sie_block to user space allows user space to generate random SIE
validity intercepts.
CONFIG_KVM_S390_UCONTROL feels like something that should just be
maintained out of tree by someone who really needs to hack deep into hw
virtualization for testing purposes etc.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v4 1/2] tty: hvc: pass DMA capable memory to put_chars()
From: Arnd Bergmann @ 2021-08-12 8:54 UTC (permalink / raw)
To: Xianting TIan
Cc: Arnd Bergmann, Jiri Slaby, Amit Shah, gregkh,
Linux Kernel Mailing List,
open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE, Guo Ren,
linuxppc-dev, Omar Sandoval
In-Reply-To: <f18d017b-d6f7-cf87-8859-8d6b50c7c289@linux.alibaba.com>
On Thu, Aug 12, 2021 at 10:08 AM Xianting TIan
<xianting.tian@linux.alibaba.com> wrote:
> 在 2021/8/6 下午10:51, Arnd Bergmann 写道:
> > On Fri, Aug 6, 2021 at 5:01 AM Xianting Tian
> >> +#define __ALIGNED__ __attribute__((__aligned__(sizeof(long))))
> > I think you need a higher alignment for DMA buffers, instead of sizeof(long),
> > I would suggest ARCH_DMA_MINALIGN.
>
> As some ARCH(eg, x86, riscv) doesn't define ARCH_DMA_MINALIG, so i think
> it 's better remain the code unchanged,
>
> I will send v5 patch soon.
I think you could just use "L1_CACHE_BYTES" as the alignment in this case.
This will make the structure slightly larger for architectures that do not have
alignment constraints on DMA buffers, but using a smaller alignment is
clearly wrong. Another option would be to use ARCH_KMALLOC_MINALIGN.
Note that there is a patch to add ARCH_DMA_MINALIGN to riscv already,
as some implementations do not have coherent DMA. I had failed to
realized though that on x86 you do not get an ARCH_DMA_MINALIGN
definition.
Arnd
^ permalink raw reply
* [PATCH v5 2/2] virtio-console: remove unnecessary kmemdup()
From: Xianting Tian @ 2021-08-12 8:51 UTC (permalink / raw)
To: gregkh, jirislaby, amit, arnd, osandov
Cc: Xianting Tian, guoren, linuxppc-dev, linux-kernel, virtualization
In-Reply-To: <20210812085112.145265-1-xianting.tian@linux.alibaba.com>
hvc framework will never pass stack memory to the put_chars() function,
So the calling of kmemdup() is unnecessary, we can remove it.
This revert commit c4baad5029 ("virtio-console: avoid DMA from stack")
Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com>
---
drivers/char/virtio_console.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 7eaf303a7..4ed3ffb1d 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1117,8 +1117,6 @@ static int put_chars(u32 vtermno, const char *buf, int count)
{
struct port *port;
struct scatterlist sg[1];
- void *data;
- int ret;
if (unlikely(early_put_chars))
return early_put_chars(vtermno, buf, count);
@@ -1127,14 +1125,8 @@ static int put_chars(u32 vtermno, const char *buf, int count)
if (!port)
return -EPIPE;
- data = kmemdup(buf, count, GFP_ATOMIC);
- if (!data)
- return -ENOMEM;
-
- sg_init_one(sg, data, count);
- ret = __send_to_port(port, sg, 1, count, data, false);
- kfree(data);
- return ret;
+ sg_init_one(sg, buf, count);
+ return __send_to_port(port, sg, 1, count, (void *)buf, false);
}
/*
--
2.17.1
^ permalink raw reply related
* [PATCH v5 0/2] make hvc pass dma capable memory to its backend
From: Xianting Tian @ 2021-08-12 8:51 UTC (permalink / raw)
To: gregkh, jirislaby, amit, arnd, osandov
Cc: Xianting Tian, guoren, linuxppc-dev, linux-kernel, virtualization
Dear all,
This patch series make hvc framework pass DMA capable memory to
put_chars() of hvc backend(eg, virtio-console), and revert commit
c4baad5029 ("virtio-console: avoid DMA from stack”)
V1
virtio-console: avoid DMA from vmalloc area
https://lkml.org/lkml/2021/7/27/494
For v1 patch, Arnd Bergmann suggests to fix the issue in the first
place:
Make hvc pass DMA capable memory to put_chars()
The fix suggestion is included in v2.
V2
[PATCH 1/2] tty: hvc: pass DMA capable memory to put_chars()
https://lkml.org/lkml/2021/8/1/8
[PATCH 2/2] virtio-console: remove unnecessary kmemdup()
https://lkml.org/lkml/2021/8/1/9
For v2 patch, Arnd Bergmann suggests to make new buf part of the
hvc_struct structure, and fix the compile issue.
The fix suggestion is included in v3.
V3
[PATCH v3 1/2] tty: hvc: pass DMA capable memory to put_chars()
https://lkml.org/lkml/2021/8/3/1347
[PATCH v3 2/2] virtio-console: remove unnecessary kmemdup()
https://lkml.org/lkml/2021/8/3/1348
For v3 patch, Jiri Slaby suggests to make 'char c[N_OUTBUF]' part of
hvc_struct, and make 'hp->outbuf' aligned and use struct_size() to
calculate the size of hvc_struct. The fix suggestion is included in
v4.
V4
[PATCH v4 0/2] make hvc pass dma capable memory to its backend
https://lkml.org/lkml/2021/8/5/1350
[PATCH v4 1/2] tty: hvc: pass DMA capable memory to put_chars()
https://lkml.org/lkml/2021/8/5/1351
[PATCH v4 2/2] virtio-console: remove unnecessary kmemdup()
https://lkml.org/lkml/2021/8/5/1352
For v4 patch, Arnd Bergmann suggests to introduce another
array(cons_outbuf[]) for the buffer pointers next to the cons_ops[]
and vtermnos[] arrays. This fix included in this v5 patch.
drivers/char/virtio_console.c | 12 ++----------
drivers/tty/hvc/hvc_console.c | 40 +++++++++++++++++++++--------------
drivers/tty/hvc/hvc_console.h | 16 ++++++++++++--
3 file changed
^ permalink raw reply
* [PATCH v5 1/2] tty: hvc: pass DMA capable memory to put_chars()
From: Xianting Tian @ 2021-08-12 8:51 UTC (permalink / raw)
To: gregkh, jirislaby, amit, arnd, osandov
Cc: Xianting Tian, guoren, linuxppc-dev, linux-kernel, virtualization
In-Reply-To: <20210812085112.145265-1-xianting.tian@linux.alibaba.com>
As well known, hvc backend can register its opertions to hvc backend.
the opertions contain put_chars(), get_chars() and so on.
Some hvc backend may do dma in its opertions. eg, put_chars() of
virtio-console. But in the code of hvc framework, it may pass DMA
incapable memory to put_chars() under a specific configuration, which
is explained in commit c4baad5029(virtio-console: avoid DMA from stack):
1, c[] is on stack,
hvc_console_print():
char c[N_OUTBUF] __ALIGNED__;
cons_ops[index]->put_chars(vtermnos[index], c, i);
2, ch is on stack,
static void hvc_poll_put_char(,,char ch)
{
struct tty_struct *tty = driver->ttys[0];
struct hvc_struct *hp = tty->driver_data;
int n;
do {
n = hp->ops->put_chars(hp->vtermno, &ch, 1);
} while (n <= 0);
}
Commit c4baad5029 is just the fix to avoid DMA from stack memory, which
is passed to virtio-console by hvc framework in above code. But I think
the fix is aggressive, it directly uses kmemdup() to alloc new buffer
from kmalloc area and do memcpy no matter the memory is in kmalloc area
or not. But most importantly, it should better be fixed in the hvc
framework, by changing it to never pass stack memory to the put_chars()
function in the first place. Otherwise, we still face the same issue if
a new hvc backend using dma added in the furture.
We make 'char c[N_OUTBUF]' part of 'struct hvc_struct', so hp->c is no
longer the stack memory. we can use it in above two cases.
Other cleanup is use ARCH_DMA_MINALIGN for align, and make 'hp->outbuf'
aligned and use struct_size() for the size parameter of kzalloc().
With the patch, we can remove the fix c4baad5029.
Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com>
Tested-by: Xianting Tian <xianting.tian@linux.alibaba.com>
---
drivers/tty/hvc/hvc_console.c | 40 +++++++++++++++++++++--------------
drivers/tty/hvc/hvc_console.h | 16 ++++++++++++--
2 files changed, 38 insertions(+), 18 deletions(-)
diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 5bb8c4e44..c56564eb7 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -41,16 +41,6 @@
*/
#define HVC_CLOSE_WAIT (HZ/100) /* 1/10 of a second */
-/*
- * These sizes are most efficient for vio, because they are the
- * native transfer size. We could make them selectable in the
- * future to better deal with backends that want other buffer sizes.
- */
-#define N_OUTBUF 16
-#define N_INBUF 16
-
-#define __ALIGNED__ __attribute__((__aligned__(sizeof(long))))
-
static struct tty_driver *hvc_driver;
static struct task_struct *hvc_task;
@@ -142,6 +132,7 @@ static int hvc_flush(struct hvc_struct *hp)
static const struct hv_ops *cons_ops[MAX_NR_HVC_CONSOLES];
static uint32_t vtermnos[MAX_NR_HVC_CONSOLES] =
{[0 ... MAX_NR_HVC_CONSOLES - 1] = -1};
+static char *cons_outbuf[MAX_NR_HVC_CONSOLES];
/*
* Console APIs, NOT TTY. These APIs are available immediately when
@@ -151,18 +142,23 @@ static uint32_t vtermnos[MAX_NR_HVC_CONSOLES] =
static void hvc_console_print(struct console *co, const char *b,
unsigned count)
{
- char c[N_OUTBUF] __ALIGNED__;
+ char *c;
unsigned i = 0, n = 0;
int r, donecr = 0, index = co->index;
+ unsigned long flags;
+ struct hvc_struct *hp;
/* Console access attempt outside of acceptable console range. */
if (index >= MAX_NR_HVC_CONSOLES)
return;
/* This console adapter was removed so it is not usable. */
- if (vtermnos[index] == -1)
+ if (vtermnos[index] == -1 || !cons_outbuf[index])
return;
+ c = cons_outbuf[index];
+
+ spin_lock_irqsave(&hp->c_lock, flags);
while (count > 0 || i > 0) {
if (count > 0 && i < sizeof(c)) {
if (b[n] == '\n' && !donecr) {
@@ -191,6 +187,7 @@ static void hvc_console_print(struct console *co, const char *b,
}
}
}
+ spin_unlock_irqrestore(&hp->c_lock, flags);
hvc_console_flush(cons_ops[index], vtermnos[index]);
}
@@ -878,9 +875,19 @@ static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch)
struct tty_struct *tty = driver->ttys[0];
struct hvc_struct *hp = tty->driver_data;
int n;
+ unsigned long flags;
+ char *c;
+
+ if (!hp || !cons_outbuf[hp->index])
+ return;
+
+ c = cons_outbuf[hp->index];
do {
- n = hp->ops->put_chars(hp->vtermno, &ch, 1);
+ spin_lock_irqsave(&hp->c_lock, flags);
+ c[0] = ch;
+ n = hp->ops->put_chars(hp->vtermno, c, 1);
+ spin_unlock_irqrestore(&hp->c_lock, flags);
} while (n <= 0);
}
#endif
@@ -922,8 +929,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
return ERR_PTR(err);
}
- hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size,
- GFP_KERNEL);
+ hp = kzalloc(struct_size(hp, outbuf, outbuf_size), GFP_KERNEL);
if (!hp)
return ERR_PTR(-ENOMEM);
@@ -931,13 +937,13 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
hp->data = data;
hp->ops = ops;
hp->outbuf_size = outbuf_size;
- hp->outbuf = &((char *)hp)[ALIGN(sizeof(*hp), sizeof(long))];
tty_port_init(&hp->port);
hp->port.ops = &hvc_port_ops;
INIT_WORK(&hp->tty_resize, hvc_set_winsz);
spin_lock_init(&hp->lock);
+ spin_lock_init(&hp->c_lock);
mutex_lock(&hvc_structs_mutex);
/*
@@ -964,6 +970,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
if (i < MAX_NR_HVC_CONSOLES) {
cons_ops[i] = ops;
vtermnos[i] = vtermno;
+ cons_outbuf[i] = hp->c;
}
list_add_tail(&(hp->next), &hvc_structs);
@@ -988,6 +995,7 @@ int hvc_remove(struct hvc_struct *hp)
if (hp->index < MAX_NR_HVC_CONSOLES) {
vtermnos[hp->index] = -1;
cons_ops[hp->index] = NULL;
+ cons_outbuf[hp->index] = NULL;
}
/* Don't whack hp->irq because tty_hangup() will need to free the irq. */
diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
index 18d005814..52374e2da 100644
--- a/drivers/tty/hvc/hvc_console.h
+++ b/drivers/tty/hvc/hvc_console.h
@@ -32,13 +32,21 @@
*/
#define HVC_ALLOC_TTY_ADAPTERS 8
+/*
+ * These sizes are most efficient for vio, because they are the
+ * native transfer size. We could make them selectable in the
+ * future to better deal with backends that want other buffer sizes.
+ */
+#define N_OUTBUF 16
+#define N_INBUF 16
+
+#define __ALIGNED__ __attribute__((__aligned__(sizeof(long))))
+
struct hvc_struct {
struct tty_port port;
spinlock_t lock;
int index;
int do_wakeup;
- char *outbuf;
- int outbuf_size;
int n_outbuf;
uint32_t vtermno;
const struct hv_ops *ops;
@@ -48,6 +56,10 @@ struct hvc_struct {
struct work_struct tty_resize;
struct list_head next;
unsigned long flags;
+ spinlock_t c_lock;
+ char c[N_OUTBUF] __ALIGNED__;
+ int outbuf_size;
+ char outbuf[0] __ALIGNED__;
};
/* implemented by a low level driver */
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v3 4/8] PCI: replace pci_dev::driver usage that gets the driver name
From: Uwe Kleine-König @ 2021-08-12 8:14 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alexander Duyck, oss-drivers, Paul Mackerras, Herbert Xu,
Ido Schimmel, Rafa?? Mi??ecki, Jesse Brandeburg, Bjorn Helgaas,
linux-pci, Jakub Kicinski, Yisen Zhuang, Vadym Kochan,
Michael Buesch, Jiri Pirko, Salil Mehta, Greg Kroah-Hartman,
linux-wireless, linux-kernel, Taras Chornyi, Zhou Wang,
linux-crypto, kernel, netdev, Simon Horman, Oliver O'Halloran,
linuxppc-dev, David S. Miller
In-Reply-To: <YRTIqGm5Dr8du7a7@infradead.org>
[-- Attachment #1: Type: text/plain, Size: 2565 bytes --]
On Thu, Aug 12, 2021 at 08:07:20AM +0100, Christoph Hellwig wrote:
> On Wed, Aug 11, 2021 at 10:06:33AM +0200, Uwe Kleine-K??nig wrote:
> > static inline const char *eeh_driver_name(struct pci_dev *pdev)
> > {
> > - return (pdev && pdev->driver) ? pdev->driver->name : "<null>";
> > + const char *drvstr = pdev ? dev_driver_string(&pdev->dev) : "";
> > +
> > + if (*drvstr == '\0')
> > + return "<null>";
> > +
> > + return drvstr;
>
> This looks rather obsfucated due to the fact that dev_driver_string
> never returns '\0', and due to the strange mix of a tenary operation
> and the if on a related condition.
dev_driver_string() might return "" (via dev_bus_name()). If that happens
*drvstr == '\0' becomes true.
Would the following be better?:
const char *drvstr;
if (pdev)
return "<null>";
drvstr = dev_driver_string(&pdev->dev);
if (!strcmp(drvstr, ""))
return "<null>";
return drvstr;
When I thought about this hunk I considered it ugly to have "<null>" in
it twice.
> > }
> >
> > #endif /* CONFIG_EEH */
> > diff --git a/drivers/bcma/host_pci.c b/drivers/bcma/host_pci.c
> > index 69c10a7b7c61..dc2ffa686964 100644
> > --- a/drivers/bcma/host_pci.c
> > +++ b/drivers/bcma/host_pci.c
> > @@ -175,9 +175,10 @@ static int bcma_host_pci_probe(struct pci_dev *dev,
> > if (err)
> > goto err_kfree_bus;
> >
> > - name = dev_name(&dev->dev);
> > - if (dev->driver && dev->driver->name)
> > - name = dev->driver->name;
> > + name = dev_driver_string(&dev->dev);
> > + if (*name == '\0')
> > + name = dev_name(&dev->dev);
>
> Where does this '\0' check come from?
The original code is equivalent to
if (dev->driver && dev->driver->name)
name = dev->driver->name;
else:
name = dev_name(...);
As dev_driver_string() implements something like:
if (dev->driver && dev->driver->name)
return dev->driver->name;
else
return "";
the change looks fine to me. (One could wonder if it's sensible to fall
back to the device name if the driver has no nice name, but this isn't
new with my change.)
> > + name = dev_driver_string(&dev->dev);
> > + if (*name == '\0')
> > + name = dev_name(&dev->dev);
> > +
>
> More of this weirdness.
I admit it's not pretty. Would it help to use !strcmp(name, "")
instead of *name == '\0'? Any other constructive suggestion?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/book3s64/radix: make tlb_single_page_flush_ceiling a debugfs entry
From: Aneesh Kumar K.V @ 2021-08-12 8:13 UTC (permalink / raw)
To: Michael Ellerman, Christophe Leroy, linuxppc-dev
In-Reply-To: <87o8a3m89i.fsf@mpe.ellerman.id.au>
On 8/12/21 12:58 PM, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 10/08/2021 à 06:53, Aneesh Kumar K.V a écrit :
>>> Similar to x86/s390 add a debugfs file to tune tlb_single_page_flush_ceiling.
>>>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> ---
>>> arch/powerpc/mm/book3s64/radix_tlb.c | 48 ++++++++++++++++++++++++++++
>>> 1 file changed, 48 insertions(+)
>>>
>>> diff --git a/arch/powerpc/mm/book3s64/radix_tlb.c b/arch/powerpc/mm/book3s64/radix_tlb.c
>>> index aefc100d79a7..5cca0fe130e7 100644
>>> --- a/arch/powerpc/mm/book3s64/radix_tlb.c
>>> +++ b/arch/powerpc/mm/book3s64/radix_tlb.c
>>> @@ -17,6 +17,7 @@
> ...
>>> +
>>> +static int __init create_tlb_single_page_flush_ceiling(void)
>>> +{
>>> + debugfs_create_file("tlb_single_page_flush_ceiling", S_IRUSR | S_IWUSR,
>>> + powerpc_debugfs_root, NULL, &fops_tlbflush);
>>
>> Could you just use debugfs_create_u32() instead of re-implementing simple read and write ?
>
> Yeah AFAICS that should work fine.
>
> It could probably even be a u16?
>
I was looking at switching all that to u64. Should i fallback to u16,
considering a tlb_signle_page_flush_ceiling value larger that 2**16
doesn't make sense?
-aneesh
^ permalink raw reply
* Re: [PATCH v4 1/2] tty: hvc: pass DMA capable memory to put_chars()
From: Xianting TIan @ 2021-08-12 8:07 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jiri Slaby, Amit Shah, gregkh, Linux Kernel Mailing List,
open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE, Guo Ren,
linuxppc-dev, Omar Sandoval
In-Reply-To: <CAK8P3a2=BmVv0tvUKaca+LYxuAussAJtAJW9O3fRN2CbV2-9aw@mail.gmail.com>
在 2021/8/6 下午10:51, Arnd Bergmann 写道:
> On Fri, Aug 6, 2021 at 5:01 AM Xianting Tian
> <xianting.tian@linux.alibaba.com> wrote:
>> @@ -163,6 +155,13 @@ static void hvc_console_print(struct console *co, const char *b,
>> if (vtermnos[index] == -1)
>> return;
>>
>> + list_for_each_entry(hp, &hvc_structs, next)
>> + if (hp->vtermno == vtermnos[index])
>> + break;
>> +
>> + c = hp->c;
>> +
>> + spin_lock_irqsave(&hp->c_lock, flags);
> The loop looks like it might race against changes to the list. It seems strange
> that the print function has to actually search for the structure here.
>
> It may be better to have yet another array for the buffer pointers next to
> the cons_ops[] and vtermnos[] arrays.
>
>> +/*
>> + * These sizes are most efficient for vio, because they are the
>> + * native transfer size. We could make them selectable in the
>> + * future to better deal with backends that want other buffer sizes.
>> + */
>> +#define N_OUTBUF 16
>> +#define N_INBUF 16
>> +
>> +#define __ALIGNED__ __attribute__((__aligned__(sizeof(long))))
> I think you need a higher alignment for DMA buffers, instead of sizeof(long),
> I would suggest ARCH_DMA_MINALIGN.
As some ARCH(eg, x86, riscv) doesn't define ARCH_DMA_MINALIG, so i think
it 's better remain the code unchanged,
I will send v5 patch soon.
>
> Arnd
^ permalink raw reply
* Re: [PATCH] powerpc/book3s64/radix: make tlb_single_page_flush_ceiling a debugfs entry
From: Michael Ellerman @ 2021-08-12 7:28 UTC (permalink / raw)
To: Christophe Leroy, Aneesh Kumar K.V, linuxppc-dev
In-Reply-To: <22c7c194-d75d-bf90-c8e5-83c995c7130c@csgroup.eu>
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 10/08/2021 à 06:53, Aneesh Kumar K.V a écrit :
>> Similar to x86/s390 add a debugfs file to tune tlb_single_page_flush_ceiling.
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>> arch/powerpc/mm/book3s64/radix_tlb.c | 48 ++++++++++++++++++++++++++++
>> 1 file changed, 48 insertions(+)
>>
>> diff --git a/arch/powerpc/mm/book3s64/radix_tlb.c b/arch/powerpc/mm/book3s64/radix_tlb.c
>> index aefc100d79a7..5cca0fe130e7 100644
>> --- a/arch/powerpc/mm/book3s64/radix_tlb.c
>> +++ b/arch/powerpc/mm/book3s64/radix_tlb.c
>> @@ -17,6 +17,7 @@
...
>> +
>> +static int __init create_tlb_single_page_flush_ceiling(void)
>> +{
>> + debugfs_create_file("tlb_single_page_flush_ceiling", S_IRUSR | S_IWUSR,
>> + powerpc_debugfs_root, NULL, &fops_tlbflush);
>
> Could you just use debugfs_create_u32() instead of re-implementing simple read and write ?
Yeah AFAICS that should work fine.
It could probably even be a u16?
cheers
^ permalink raw reply
* Re: [PATCH v3 4/8] PCI: replace pci_dev::driver usage that gets the driver name
From: Christoph Hellwig @ 2021-08-12 7:07 UTC (permalink / raw)
To: Uwe Kleine-K??nig
Cc: linux-pci, Alexander Duyck, oss-drivers, Paul Mackerras,
Herbert Xu, Rafa?? Mi??ecki, Jesse Brandeburg, Bjorn Helgaas,
Ido Schimmel, Jakub Kicinski, Yisen Zhuang, Vadym Kochan,
Michael Buesch, Jiri Pirko, Salil Mehta, Greg Kroah-Hartman,
linux-wireless, linux-kernel, Taras Chornyi, Zhou Wang,
linux-crypto, kernel, netdev, Simon Horman, Oliver O'Halloran,
linuxppc-dev, David S. Miller
In-Reply-To: <20210811080637.2596434-5-u.kleine-koenig@pengutronix.de>
On Wed, Aug 11, 2021 at 10:06:33AM +0200, Uwe Kleine-K??nig wrote:
> static inline const char *eeh_driver_name(struct pci_dev *pdev)
> {
> - return (pdev && pdev->driver) ? pdev->driver->name : "<null>";
> + const char *drvstr = pdev ? dev_driver_string(&pdev->dev) : "";
> +
> + if (*drvstr == '\0')
> + return "<null>";
> +
> + return drvstr;
This looks rather obsfucated due to the fact that dev_driver_string
never returns '\0', and due to the strange mix of a tenary operation
and the if on a related condition.
> }
>
> #endif /* CONFIG_EEH */
> diff --git a/drivers/bcma/host_pci.c b/drivers/bcma/host_pci.c
> index 69c10a7b7c61..dc2ffa686964 100644
> --- a/drivers/bcma/host_pci.c
> +++ b/drivers/bcma/host_pci.c
> @@ -175,9 +175,10 @@ static int bcma_host_pci_probe(struct pci_dev *dev,
> if (err)
> goto err_kfree_bus;
>
> - name = dev_name(&dev->dev);
> - if (dev->driver && dev->driver->name)
> - name = dev->driver->name;
> + name = dev_driver_string(&dev->dev);
> + if (*name == '\0')
> + name = dev_name(&dev->dev);
Where does this '\0' check come from?
> +
> + name = dev_driver_string(&dev->dev);
> + if (*name == '\0')
> + name = dev_name(&dev->dev);
> +
More of this weirdness.
^ permalink raw reply
* Re: clang/ld.lld build fails with `can't create dynamic relocation R_PPC64_ADDR64 against local symbol in readonly segment`
From: Michael Ellerman @ 2021-08-12 5:46 UTC (permalink / raw)
To: Paul Menzel, Benjamin Herrenschmidt, Paul Mackerras
Cc: Derek Parker, Dmitrii Okunev, linuxppc-dev
In-Reply-To: <77a69755-5291-285c-45be-c1e42423fddc@molgen.mpg.de>
Paul Menzel <pmenzel@molgen.mpg.de> writes:
> Am 29.07.21 um 10:23 schrieb Paul Menzel:
>
>> I just wanted to make you aware that building Linux for ppc64le with
>> clang/lld.ld fails with [1]:
>>
>> ld.lld: error: can't create dynamic relocation R_PPC64_ADDR64
>> against symbol: empty_zero_page in readonly segment; recompile object
>> files with -fPIC or pass '-Wl,-z,notext' to allow text relocations in
>> the output
>> >>> defined in arch/powerpc/kernel/head_64.o
>> >>> referenced by
>> arch/powerpc/kernel/head_64.o:(___ksymtab+empty_zero_page+0x0)
>>
>> The patch below from one of the comments [2] fixes it.
>>
>> --- i/arch/powerpc/Makefile
>> +++ w/arch/powerpc/Makefile
>> @@ -122,7 +122,7 @@ cflags-$(CONFIG_STACKPROTECTOR) +=
>> -mstack-protector-guard-reg=r2
>> endif
>>
>> LDFLAGS_vmlinux-y := -Bstatic
>> -LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) := -pie
>> +LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) := -pie -z notext
>> LDFLAGS_vmlinux := $(LDFLAGS_vmlinux-y)
>> LDFLAGS_vmlinux += $(call ld-option,--orphan-handling=warn)
>
> Any comments, if this is the right fix? Current Linux master branch
> still fails to build with `LLVM=1` on Ubuntu 21.04 without this change.
Sorry but I have no idea if it's the right fix. What I need is the
author (or someone else) to send a patch with a change log explaining
the change, what it does, why it's right for llvm, and why it's right
for binutils.
cheers
^ permalink raw reply
* Re: [PATCH v2 7/9] usb: phy: fsl-usb: add IRQ check
From: Felipe Balbi @ 2021-08-12 5:38 UTC (permalink / raw)
To: Sergey Shtylyov; +Cc: Greg Kroah-Hartman, linux-usb, linuxppc-dev, Ran Wang
In-Reply-To: <b0a86089-8b8b-122e-fd6d-73e8c2304964@omp.ru>
Sergey Shtylyov <s.shtylyov@omp.ru> writes:
> The driver neglects to check the result of platform_get_irq()'s call and
> blithely passes the negative error codes to request_irq() (which takes
> *unsigned* IRQ #), causing it to fail with -EINVAL, overriding an original
> error code. Stop calling request_irq() with the invalid IRQ #s.
>
> Fixes: 0807c500a1a6 ("USB: add Freescale USB OTG Transceiver driver")
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
Acked-by: Felipe Balbi <balbi@kernel.org>
--
balbi
^ permalink raw reply
* [PATCH v2 1/2] KVM: Refactor kvm_arch_vcpu_fault() to return a struct page pointer
From: Hou Wenlong @ 2021-08-12 4:02 UTC (permalink / raw)
To: kvm
Cc: x86, Wanpeng Li, David Hildenbrand, linux-mips, H. Peter Anvin,
Claudio Imbrenda, Will Deacon, kvmarm, linux-s390, Janosch Frank,
Marc Zyngier, Joerg Roedel, Huacai Chen, Christian Borntraeger,
Aleksandar Markovic, Ingo Molnar, Catalin Marinas, Vasily Gorbik,
Suzuki K Poulose, Heiko Carstens, kvm-ppc, Borislav Petkov,
Thomas Gleixner, Alexandru Elisei, linux-arm-kernel, Jim Mattson,
Thomas Bogendoerfer, Sean Christopherson, Cornelia Huck,
linux-kernel, James Morse, Paolo Bonzini, Vitaly Kuznetsov,
linuxppc-dev
In-Reply-To: <YRQcZqCWwVH8bCGc@google.com>
From: Sean Christopherson <seanjc@google.com>
Refactor kvm_arch_vcpu_fault() to return 'struct page *' instead of
'vm_fault_t' to simplify architecture specific implementations that do
more than return SIGBUS. Currently this only applies to s390, but a
future patch will move x86's pio_data handling into x86 where it belongs.
No functional changed intended.
Cc: Hou Wenlong <houwenlong93@linux.alibaba.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Hou Wenlong <houwenlong93@linux.alibaba.com>
---
arch/arm64/kvm/arm.c | 4 ++--
arch/mips/kvm/mips.c | 4 ++--
arch/powerpc/kvm/powerpc.c | 4 ++--
arch/s390/kvm/kvm-s390.c | 12 ++++--------
arch/x86/kvm/x86.c | 4 ++--
include/linux/kvm_host.h | 2 +-
virt/kvm/kvm_main.c | 5 ++++-
7 files changed, 17 insertions(+), 18 deletions(-)
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index e9a2b8f27792..83f4ffe3e4f2 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -161,9 +161,9 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
return ret;
}
-vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
+struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
{
- return VM_FAULT_SIGBUS;
+ return NULL;
}
diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
index af9dd029a4e1..ae79874e6fd2 100644
--- a/arch/mips/kvm/mips.c
+++ b/arch/mips/kvm/mips.c
@@ -1053,9 +1053,9 @@ int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
return -ENOIOCTLCMD;
}
-vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
+struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
{
- return VM_FAULT_SIGBUS;
+ return NULL;
}
int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index be33b5321a76..b9c21f9ab784 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -2090,9 +2090,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
return r;
}
-vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
+struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
{
- return VM_FAULT_SIGBUS;
+ return NULL;
}
static int kvm_vm_ioctl_get_pvinfo(struct kvm_ppc_pvinfo *pvinfo)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 02574d7b3612..e1b69833e228 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -4979,17 +4979,13 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
return r;
}
-vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
+struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
{
#ifdef CONFIG_KVM_S390_UCONTROL
- if ((vmf->pgoff == KVM_S390_SIE_PAGE_OFFSET)
- && (kvm_is_ucontrol(vcpu->kvm))) {
- vmf->page = virt_to_page(vcpu->arch.sie_block);
- get_page(vmf->page);
- return 0;
- }
+ if (vmf->pgoff == KVM_S390_SIE_PAGE_OFFSET && kvm_is_ucontrol(vcpu->kvm))
+ return virt_to_page(vcpu->arch.sie_block);
#endif
- return VM_FAULT_SIGBUS;
+ return NULL;
}
/* Section: memory related */
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3cedc7cc132a..1e3bbe5cd33a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5347,9 +5347,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
return r;
}
-vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
+struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
{
- return VM_FAULT_SIGBUS;
+ return NULL;
}
static int kvm_vm_ioctl_set_tss_addr(struct kvm *kvm, unsigned long addr)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 492d183dd7d0..a949de534722 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -995,7 +995,7 @@ long kvm_arch_dev_ioctl(struct file *filp,
unsigned int ioctl, unsigned long arg);
long kvm_arch_vcpu_ioctl(struct file *filp,
unsigned int ioctl, unsigned long arg);
-vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf);
+struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf);
int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 30d322519253..f7d21418971b 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3448,7 +3448,10 @@ static vm_fault_t kvm_vcpu_fault(struct vm_fault *vmf)
&vcpu->dirty_ring,
vmf->pgoff - KVM_DIRTY_LOG_PAGE_OFFSET);
else
- return kvm_arch_vcpu_fault(vcpu, vmf);
+ page = kvm_arch_vcpu_fault(vcpu, vmf);
+ if (!page)
+ return VM_FAULT_SIGBUS;
+
get_page(page);
vmf->page = page;
return 0;
--
2.31.1
^ permalink raw reply related
* Re: [PATCH v7 5/6] powerpc/pseries: Add support for FORM2 associativity
From: Aneesh Kumar K.V @ 2021-08-12 3:36 UTC (permalink / raw)
To: David Gibson; +Cc: Nathan Lynch, Daniel Henrique Barboza, linuxppc-dev
In-Reply-To: <YRR8Z0EhlXgEKtY8@yekko>
On 8/12/21 7:11 AM, David Gibson wrote:
> On Wed, Aug 11, 2021 at 09:39:32AM +0530, Aneesh Kumar K.V wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>>
>>> On Mon, Aug 09, 2021 at 10:54:33AM +0530, Aneesh Kumar K.V wrote:
>>>> PAPR interface currently supports two different ways of communicating resource
>>>> grouping details to the OS. These are referred to as Form 0 and Form 1
>>>> associativity grouping. Form 0 is the older format and is now considered
>>>> deprecated. This patch adds another resource grouping named FORM2.
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>
>>> LGTM, with the exception of some minor nits noted below.
>> ...
>>
>>> +
>>>> + for (i = 0; i < max_numa_index; i++)
>>>> + /* +1 skip the max_numa_index in the property */
>>>> + numa_id_index_table[i] = of_read_number(&numa_lookup_index[i + 1], 1);
>>>> +
>>>> +
>>>> + if (numa_dist_table_length != max_numa_index * max_numa_index) {
>>>> +
>>>
>>> Stray extra whitespace line here.
>>>
>>>> + WARN(1, "Wrong NUMA distance information\n");
>>>> + /* consider everybody else just remote. */
>>>> + for (i = 0; i < max_numa_index; i++) {
>>>> + for (j = 0; j < max_numa_index; j++) {
>>>> + int nodeA = numa_id_index_table[i];
>>>> + int nodeB = numa_id_index_table[j];
>>>> +
>>>> + if (nodeA == nodeB)
>>>> + numa_distance_table[nodeA][nodeB] = LOCAL_DISTANCE;
>>>> + else
>>>> + numa_distance_table[nodeA][nodeB] = REMOTE_DISTANCE;
>>>> + }
>>>> + }
>>>
>>> I don't think it's necessarily a problem, but something to consider is
>>> that this fallback will initialize distance for *all* node IDs,
>>> whereas the normal path will only initialize it for nodes that are in
>>> the index table. Since some later error checks key off whether
>>> certain fields in the distance table are initialized, is that the
>>> outcome you want?
>>>
>>
>> With the device tree details not correct, one of the possible way to
>> make progress is to consider everybody remote. With new node hotplug
>> support we used to check whether the distance table entry is
>> initialized. With the updated spec, we expect all possible numa node
>> distance to be available during boot.
>
> Sure. But my main point here is that the fallback behaviour in this
> clause is different from the fallback behaviour if the table is there
> and parseable, but incomplete - which is also not expected.
>
With FORM2 fallback with bad device tree details is to consider
everybody REMOTE. With Form1, we leave the distance table not populated
as it was with the current kernel versions.
-aneesh
^ permalink raw reply
* Re: [PATCH v7 5/6] powerpc/pseries: Add support for FORM2 associativity
From: David Gibson @ 2021-08-12 1:41 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: Nathan Lynch, Daniel Henrique Barboza, linuxppc-dev
In-Reply-To: <87a6loaagz.fsf@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 2691 bytes --]
On Wed, Aug 11, 2021 at 09:39:32AM +0530, Aneesh Kumar K.V wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
>
> > On Mon, Aug 09, 2021 at 10:54:33AM +0530, Aneesh Kumar K.V wrote:
> >> PAPR interface currently supports two different ways of communicating resource
> >> grouping details to the OS. These are referred to as Form 0 and Form 1
> >> associativity grouping. Form 0 is the older format and is now considered
> >> deprecated. This patch adds another resource grouping named FORM2.
> >>
> >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> >
> > LGTM, with the exception of some minor nits noted below.
> ...
>
> > +
> >> + for (i = 0; i < max_numa_index; i++)
> >> + /* +1 skip the max_numa_index in the property */
> >> + numa_id_index_table[i] = of_read_number(&numa_lookup_index[i + 1], 1);
> >> +
> >> +
> >> + if (numa_dist_table_length != max_numa_index * max_numa_index) {
> >> +
> >
> > Stray extra whitespace line here.
> >
> >> + WARN(1, "Wrong NUMA distance information\n");
> >> + /* consider everybody else just remote. */
> >> + for (i = 0; i < max_numa_index; i++) {
> >> + for (j = 0; j < max_numa_index; j++) {
> >> + int nodeA = numa_id_index_table[i];
> >> + int nodeB = numa_id_index_table[j];
> >> +
> >> + if (nodeA == nodeB)
> >> + numa_distance_table[nodeA][nodeB] = LOCAL_DISTANCE;
> >> + else
> >> + numa_distance_table[nodeA][nodeB] = REMOTE_DISTANCE;
> >> + }
> >> + }
> >
> > I don't think it's necessarily a problem, but something to consider is
> > that this fallback will initialize distance for *all* node IDs,
> > whereas the normal path will only initialize it for nodes that are in
> > the index table. Since some later error checks key off whether
> > certain fields in the distance table are initialized, is that the
> > outcome you want?
> >
>
> With the device tree details not correct, one of the possible way to
> make progress is to consider everybody remote. With new node hotplug
> support we used to check whether the distance table entry is
> initialized. With the updated spec, we expect all possible numa node
> distance to be available during boot.
Sure. But my main point here is that the fallback behaviour in this
clause is different from the fallback behaviour if the table is there
and parseable, but incomplete - which is also not expected.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ 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