Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto
Christophe Leroy writes: > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > index 24725e50c7b4..34745f239208 100644 > --- a/scripts/mod/modpost.c > +++ b/scripts/mod/modpost.c > @@ -926,7 +926,7 @@ static void check_section(const char *modname, struct > elf_info *elf, > ".kprobes.text", ".cpuidle.text", ".noinstr.text" > #define OTHER_TEXT_SECTIONS ".ref.text", ".head.text", ".spinlock.text", \ > ".fixup", ".entry.text", ".exception.text", ".text.*", \ > - ".coldtext" > + ".coldtext .softirqentry.text" This wasn't working, I updated it to: ".coldtext", ".softirqentry.text" Which works. cheers
Re: [PATCH] ppc: add "-z notext" flag to disable diagnostic
On Sat, Aug 14, 2021 at 5:59 AM Segher Boessenkool
wrote:
>
> On Fri, Aug 13, 2021 at 01:05:08PM -0700, Fangrui Song wrote:
> > Text relocations are considered very awful by linker developers.
>
> By very few linker developers.
https://groups.google.com/g/generic-abi/c/Ckq19PfLxyk/m/uW29sgkoAgAJ
Ali Bahrami: "My opinion is that no one wants text relocations, but
not labeling them if they do exist doesn't seem right. I find the
presence of DF_TEXTREL very interesting when diagnosing various
issues."
https://gcc.gnu.org/legacy-ml/gcc/2016-04/msg00138.html
( "So why not simply create 'dynamic text relocations' then? Is that
possible with a pure linker change?" )
Cary Coutant: "Ugh. Besides being a bad idea from a performance point
of view, it's not even always possible to do. Depending on the
architecture, a direct reference from an executable to a variable in a
shared library may not have the necessary reach."
binutils-gdb commit "Add linker option: --warn-shared-textrel to
produce warnings when adding a DT_TEXTREL to a shared object."
Nick Clifton
https://www.openwall.com/lists/musl/2020/09/26/3
Szabolcs Nagy: "nice. and gcc passes -z text for static pie code so
that case should not end up with text rels."
Someone wrote "Overall, the overhead of processing text relocations
can cause serious performance degradation." in Solaris' Linker and
Libraries Guide.
Me :)
(I wrote lld/ELF 9.0.0~13.0.0 release notes and filed dozen of GNU
ld/gold bugs/feature requests)
> > binutils 2.35 added --enable-textrel-check={no,warn,error}
> > https://sourceware.org/bugzilla/show_bug.cgi?id=20824
>
> Yes, some people wanted the default to be configurable. So now we have
> a default default that is sane, so most people get to reap the benefits
> of having defaults at all, but we also allow other people to shoot
> themselves (and people who have to deal with them) in the foot.
> "Progress". Changing the defaults should be a one-time event, only done
> when the benefits strongly outweigh the costs. Defaults should never be
> configurable (by the user).
ld.lld has such a non-configurable model and thus caught the issue
(which the patch intends to address).
Future --enable-textrel-check={yes,error} configured GNU ld will be similar.
> > I can imagine that in the future some Linux distributions (especially those
> > focusing on security) will default their binutils to use
> > --enable-textrel-check={no,warn,error}.
>
> How would this be a benefit to security?
https://flameeyes.blog/2016/01/16/textrels-text-relocations-and-their-impact-on-hardening-techniques/
https://github.com/golang/go/issues/9210
Android: "libexample.so has text relocations. This is wasting memory
and prevents security hardening. Please fix."
FWIW I contributed a glibc patch allowing TEXTREL to co-exist with ifunc.
It requires temporary mapping the text segment W^X.
> > In -no-pie links, R_PPC64_ADDR64 relocations are link-time constants.
>
> Where "link" includes dynamic links as well. There are no constants.
"Link-time" usually refers to the processing of the static linker.
Dynamic links can use load-time or run-time.
> > There are no text relocations, therefore no need for -z notext.
>
> This is a choice by the compiler, nothing more. It saves some process
> startup time, and allows slightly more maps to be shared by processes
> that run the same images. But it is a tradeoff, so it might change; and
> of course it is not an ABI requirement.
>
>
> Segher
Text relocations are generally awful.
GNU ld and gold's traditional "add DF_TEXTREL on-demand" behavior made
such user errors easy to make.
I understand that kernels are special applications where we apply
relocations once and many user-space objection can be less of a
concern/ignored.
However, the Linux kernel is already in a position where many linker
options are controlled and thus should specify -z notext to make
the intention explicit, or fix the problems (I think x86-64 is good;
that said, powerpc
has a higher cost using PC-relative instructions so pay the oneshot relocation
time cost probably isn't a bad choice)
--
宋方睿
Re: [PATCH v2 03/12] x86/sev: Add an x86 version of prot_guest_has()
On Fri, Aug 13, 2021 at 11:59:22AM -0500, Tom Lendacky wrote:
> diff --git a/arch/x86/include/asm/protected_guest.h
> b/arch/x86/include/asm/protected_guest.h
> new file mode 100644
> index ..51e4eefd9542
> --- /dev/null
> +++ b/arch/x86/include/asm/protected_guest.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Protected Guest (and Host) Capability checks
> + *
> + * Copyright (C) 2021 Advanced Micro Devices, Inc.
> + *
> + * Author: Tom Lendacky
> + */
> +
> +#ifndef _X86_PROTECTED_GUEST_H
> +#define _X86_PROTECTED_GUEST_H
> +
> +#include
> +
> +#ifndef __ASSEMBLY__
> +
> +static inline bool prot_guest_has(unsigned int attr)
> +{
> +#ifdef CONFIG_AMD_MEM_ENCRYPT
> + if (sme_me_mask)
> + return amd_prot_guest_has(attr);
> +#endif
> +
> + return false;
> +}
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#endif /* _X86_PROTECTED_GUEST_H */
I think this can be simplified more, diff ontop below:
- no need for the ifdeffery as amd_prot_guest_has() has versions for
both when CONFIG_AMD_MEM_ENCRYPT is set or not.
- the sme_me_mask check is pushed there too.
- and since this is vendor-specific, I'm checking the vendor bit. Yeah,
yeah, cross-vendor but I don't really believe that.
---
diff --git a/arch/x86/include/asm/protected_guest.h
b/arch/x86/include/asm/protected_guest.h
index 51e4eefd9542..8541c76d5da4 100644
--- a/arch/x86/include/asm/protected_guest.h
+++ b/arch/x86/include/asm/protected_guest.h
@@ -12,18 +12,13 @@
#include
-#ifndef __ASSEMBLY__
-
static inline bool prot_guest_has(unsigned int attr)
{
-#ifdef CONFIG_AMD_MEM_ENCRYPT
- if (sme_me_mask)
+ if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
+ boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
return amd_prot_guest_has(attr);
-#endif
return false;
}
-#endif /* __ASSEMBLY__ */
-
#endif /* _X86_PROTECTED_GUEST_H */
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index edc67ddf065d..5a0442a6f072 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -392,6 +392,9 @@ bool noinstr sev_es_active(void)
bool amd_prot_guest_has(unsigned int attr)
{
+ if (!sme_me_mask)
+ return false;
+
switch (attr) {
case PATTR_MEM_ENCRYPT:
return sme_me_mask != 0;
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v2 02/12] mm: Introduce a function to check for virtualization protection features
On 8/14/21 1:32 PM, Borislav Petkov wrote: On Fri, Aug 13, 2021 at 11:59:21AM -0500, Tom Lendacky wrote: diff --git a/include/linux/protected_guest.h b/include/linux/protected_guest.h new file mode 100644 index ..43d4dde94793 --- /dev/null +++ b/include/linux/protected_guest.h @@ -0,0 +1,35 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Protected Guest (and Host) Capability checks + * + * Copyright (C) 2021 Advanced Micro Devices, Inc. + * + * Author: Tom Lendacky + */ + +#ifndef _PROTECTED_GUEST_H +#define _PROTECTED_GUEST_H + +#ifndef __ASSEMBLY__ ^ Do you really need that guard? It builds fine without it too. Or something coming later does need it...? No, I probably did it out of habit. I can remove it in the next version. Thanks, Tom
Re: [PATCH v2 02/12] mm: Introduce a function to check for virtualization protection features
On Fri, Aug 13, 2021 at 11:59:21AM -0500, Tom Lendacky wrote: > In prep for other protected virtualization technologies, introduce a > generic helper function, prot_guest_has(), that can be used to check > for specific protection attributes, like memory encryption. This is > intended to eliminate having to add multiple technology-specific checks > to the code (e.g. if (sev_active() || tdx_active())). > > Reviewed-by: Joerg Roedel > Co-developed-by: Andi Kleen > Signed-off-by: Andi Kleen > Co-developed-by: Kuppuswamy Sathyanarayanan > > Signed-off-by: Kuppuswamy Sathyanarayanan > > Signed-off-by: Tom Lendacky > --- > arch/Kconfig| 3 +++ > include/linux/protected_guest.h | 35 + > 2 files changed, 38 insertions(+) > create mode 100644 include/linux/protected_guest.h > > diff --git a/arch/Kconfig b/arch/Kconfig > index 98db63496bab..bd4f60c581f1 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -1231,6 +1231,9 @@ config RELR > config ARCH_HAS_MEM_ENCRYPT > bool > > +config ARCH_HAS_PROTECTED_GUEST > + bool > + > config HAVE_SPARSE_SYSCALL_NR > bool > help > diff --git a/include/linux/protected_guest.h b/include/linux/protected_guest.h > new file mode 100644 > index ..43d4dde94793 > --- /dev/null > +++ b/include/linux/protected_guest.h > @@ -0,0 +1,35 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Protected Guest (and Host) Capability checks > + * > + * Copyright (C) 2021 Advanced Micro Devices, Inc. > + * > + * Author: Tom Lendacky > + */ > + > +#ifndef _PROTECTED_GUEST_H > +#define _PROTECTED_GUEST_H > + > +#ifndef __ASSEMBLY__ ^ Do you really need that guard? It builds fine without it too. Or something coming later does need it...? -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH] crypto: DRBG - select SHA512
On Fri, Jul 16, 2021 at 04:14:12PM +0800, Herbert Xu wrote: > Stephan Mueller wrote: > > With the swtich to use HMAC(SHA-512) as the default DRBG type, the > > configuration must now also select SHA-512. > > > > Fixes: 9b7b94683a9b "crypto: DRBG - switch to HMAC SHA512 DRBG as default > > DRBG" > > Reported-by: Sachin Sant > > Signed-off-by: Stephan Mueller > > --- > > crypto/Kconfig | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Patch applied. Thanks. Is that patch going to Linus anytime soon? I still see it on latest rc5+: DRBG: could not allocate digest TFM handle: hmac(sha512) alg: drbg: Failed to reset rng alg: drbg: Test 0 failed for drbg_nopr_hmac_sha512 [ cut here ] alg: self-tests for drbg_nopr_hmac_sha512 (stdrng) failed (rc=-22) WARNING: CPU: 3 PID: 76 at crypto/testmgr.c:5652 alg_test.part.0+0x132/0x3c0 Modules linked in: CPU: 3 PID: 76 Comm: cryptomgr_test Not tainted 5.14.0-rc5+ #1 Hardware name: LENOVO 2320CTO/2320CTO, BIOS G2ET86WW (2.06 ) 11/13/2012 RIP: 0010:alg_test.part.0+0x132/0x3c0 Code: c0 74 2e 80 3d 7f 61 ad 02 00 0f 85 c0 64 5f 00 44 89 c1 4c 89 f2 4c 89 ee 44 89 44 24 04 48 c7 c7 f8 0a 11 82 e8 8c 57 5e 00 <0f> 0b 44 8b 44 24 04 48 8b 84 24 98 00 00 00 65 48 2b 04 25 28 00 RSP: :c978fe38 EFLAGS: 00010292 RAX: 0042 RBX: RCX: RDX: 0001 RSI: 810f520f RDI: 810f520f RBP: 0053 R08: 0001 R09: 0001 R10: 888219df9000 R11: 3fff R12: 0053 R13: 888100c0ee00 R14: 888100c0ee80 R15: 14c0 FS: () GS:888211f8() knlGS: CS: 0010 DS: ES: CR0: 80050033 CR2: CR3: 02412001 CR4: 001706e0 Call Trace: ? lock_is_held_type+0xd5/0x130 ? find_held_lock+0x2b/0x80 ? preempt_count_sub+0x9b/0xd0 ? crypto_acomp_scomp_free_ctx+0x30/0x30 cryptomgr_test+0x27/0x50 kthread+0x144/0x170 ? set_kthread_struct+0x40/0x40 ret_from_fork+0x22/0x30 irq event stamp: 411 hardirqs last enabled at (419): [] console_unlock+0x332/0x570 hardirqs last disabled at (426): [] console_unlock+0x3df/0x570 softirqs last enabled at (234): [] __do_softirq+0x329/0x496 softirqs last disabled at (151): [] irq_exit_rcu+0xdd/0x130 ---[ end trace edfdfd51982deb2d ]--- -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v2 01/12] x86/ioremap: Selectively build arch override encryption functions
On Fri, Aug 13, 2021 at 11:59:20AM -0500, Tom Lendacky wrote: > In prep for other uses of the prot_guest_has() function besides AMD's > memory encryption support, selectively build the AMD memory encryption > architecture override functions only when CONFIG_AMD_MEM_ENCRYPT=y. These > functions are: > - early_memremap_pgprot_adjust() > - arch_memremap_can_ram_remap() > > Additionally, routines that are only invoked by these architecture > override functions can also be conditionally built. These functions are: > - memremap_should_map_decrypted() > - memremap_is_efi_data() > - memremap_is_setup_data() > - early_memremap_is_setup_data() > > And finally, phys_mem_access_encrypted() is conditionally built as well, > but requires a static inline version of it when CONFIG_AMD_MEM_ENCRYPT is > not set. > > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: Borislav Petkov > Cc: Dave Hansen > Cc: Andy Lutomirski > Cc: Peter Zijlstra > Signed-off-by: Tom Lendacky > --- > arch/x86/include/asm/io.h | 8 > arch/x86/mm/ioremap.c | 2 +- > 2 files changed, 9 insertions(+), 1 deletion(-) LGTM. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH] ppc: add "-z notext" flag to disable diagnostic
On Fri, Aug 13, 2021 at 01:05:08PM -0700, Fangrui Song wrote:
> Text relocations are considered very awful by linker developers.
By very few linker developers.
> binutils 2.35 added --enable-textrel-check={no,warn,error}
> https://sourceware.org/bugzilla/show_bug.cgi?id=20824
Yes, some people wanted the default to be configurable. So now we have
a default default that is sane, so most people get to reap the benefits
of having defaults at all, but we also allow other people to shoot
themselves (and people who have to deal with them) in the foot.
"Progress". Changing the defaults should be a one-time event, only done
when the benefits strongly outweigh the costs. Defaults should never be
configurable (by the user).
> I can imagine that in the future some Linux distributions (especially those
> focusing on security) will default their binutils to use
> --enable-textrel-check={no,warn,error}.
How would this be a benefit to security?
> In -no-pie links, R_PPC64_ADDR64 relocations are link-time constants.
Where "link" includes dynamic links as well. There are no constants.
> There are no text relocations, therefore no need for -z notext.
This is a choice by the compiler, nothing more. It saves some process
startup time, and allows slightly more maps to be shared by processes
that run the same images. But it is a tradeoff, so it might change; and
of course it is not an ABI requirement.
Segher
Re: [PATCH v2 2/2] powerpc/perf: Return regs->nip as instruction pointer value when SIAR is 0
Christophe Leroy writes: > Le 13/08/2021 à 10:24, Kajol Jain a écrit : >> Incase of random sampling, there can be scenarios where SIAR is not >> latching sample address and results in 0 value. Since current code >> directly returning the siar value, we could see multiple instruction >> pointer values as 0 in perf report. Can you please give more detail on that? What scenarios? On what CPUs? >> Patch resolves this issue by adding a ternary condition to return >> regs->nip incase SIAR is 0. > > Your description seems rather similar to > https://github.com/linuxppc/linux/commit/2ca13a4cc56c920a6c9fc8ee45d02bccacd7f46c > > Does it mean that the problem occurs on more than the power10 DD1 ? > > In that case, can the solution be common instead of doing something for > power10 DD1 and something > for others ? Agreed. This change would seem to make that P10 DD1 logic superfluous. Also we already have a fallback to regs->nip in the else case of the if, so we should just use that rather than adding a ternary condition. eg. if (use_siar && siar_valid(regs) && siar) return siar + perf_ip_adjust(regs); else if (use_siar) return 0; // no valid instruction pointer else return regs->nip; I'm also not sure why we have that return 0 case, I can't think of why we'd ever want to do that rather than using nip. So maybe we should do another patch to drop that case. cheers
Re: [PATCH v2 1/2] powerpc/perf: Use stack siar instead of mfspr
Christophe Leroy writes:
> Le 13/08/2021 à 10:29, kajoljain a écrit :
>>
>> On 8/13/21 1:54 PM, Kajol Jain wrote:
>>> Minor optimization in the 'perf_instruction_pointer' function code by
>>> making use of stack siar instead of mfspr.
>>>
>>> Fixes: 75382aa72f06 ("powerpc/perf: Move code to select SIAR or pt_regs
>>> into perf_read_regs")
>>> Signed-off-by: Kajol Jain
>>
>> Please ignore this patch-set as I mentioned wrong version number. I will
>> resend
>> this patch-set again with correct version. Sorry for the confusion.
>
> I fear you are creating even more confusion by sending a v1 after sending a
> v2 ...
Yeah in future just reply to the v2 saying "oops I sent v2 instead of
v1" and leave it at that.
cheers
Re: [PATCH v2 2/2] powerpc/perf: Return regs->nip as instruction pointer value when SIAR is 0
Christophe Leroy writes:
> Le 13/08/2021 à 10:24, Kajol Jain a écrit :
>> Incase of random sampling, there can be scenarios where SIAR is not
>> latching sample address and results in 0 value. Since current code
>> directly returning the siar value, we could see multiple instruction
>> pointer values as 0 in perf report.
>> Patch resolves this issue by adding a ternary condition to return
>> regs->nip incase SIAR is 0.
>>
>> Fixes: 75382aa72f06 ("powerpc/perf: Move code to select SIAR or pt_regs
>> into perf_read_regs")
>> Signed-off-by: Kajol Jain
>> ---
>> arch/powerpc/perf/core-book3s.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/perf/core-book3s.c
>> b/arch/powerpc/perf/core-book3s.c
>> index 1b464aad29c4..aeecaaf6810f 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -2260,7 +2260,7 @@ unsigned long perf_instruction_pointer(struct pt_regs
>> *regs)
>> else
>> return regs->nip;
>> } else if (use_siar && siar_valid(regs))
>> -return siar + perf_ip_adjust(regs);
>> +return siar ? siar + perf_ip_adjust(regs) : regs->nip;
>
> Why bother about returning SIAR at all if regs->nip is ok ? Why not just
> return regs->nip all the time ?
Same answer as last time :)
https://lore.kernel.org/linuxppc-dev/87r1prxd9e@mpe.ellerman.id.au/
ie. SIAR can point into interrupts-off code, whereas regs->nip will
point to where we re-enabled interrupts.
cheers
Re: [PATCH] ppc: add "-z notext" flag to disable diagnostic
Bill Wendling writes:
> On Fri, Aug 13, 2021 at 7:13 AM Daniel Axtens wrote:
>> Bill Wendling writes:
...
>> > diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
>> > index 6505d66f1193..17a9fbf9b789 100644
>> > --- a/arch/powerpc/Makefile
>> > +++ b/arch/powerpc/Makefile
>> > @@ -122,6 +122,7 @@ endif
>> >
>> > LDFLAGS_vmlinux-y := -Bstatic
>> > LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) := -pie
>> > +LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) += -z notext
...
>
> Unrelated question: Should the "-pie" flag be added with "+= -pie"
> (note the plus sign)?
I noticed that too.
It's been like that since the original relocatable support was added in
2008, commit 549e8152de80 ("powerpc: Make the 64-bit kernel as a
position-independent executable"), which did:
-LDFLAGS_vmlinux:= -Bstatic
+LDFLAGS_vmlinux-yy := -Bstatic
+LDFLAGS_vmlinux-$(CONFIG_PPC64)$(CONFIG_RELOCATABLE) := -pie
+LDFLAGS_vmlinux:= $(LDFLAGS_vmlinux-yy)
There's no mention of those flags in the change log. But the way it's
written suggests the intention was to not pass -Bstatic for relocatable
builds, otherwise it could have been more simply:
+LDFLAGS_vmlinux-$(CONFIG_PPC64)$(CONFIG_RELOCATABLE) := -pie
+LDFLAGS_vmlinux:= -Bstatic $(LDFLAGS_vmlinux-yy)
So I think it was deliberate to not use +=, but whether that's actually
correct I can't say. Maybe in the past -Bstatic and -pie were
incompatible?
cheers
Re: [PATCH] ppc: add "-z notext" flag to disable diagnostic
On Fri, Aug 13, 2021 at 11:59:21AM -0700, Nick Desaulniers wrote: > Or we can dig through why there are relocations in read only sections, > fix those, then enable `-z text` for all linkers. My recommendation > would be get the thing building, then go digging time permitting. It is not always a bug. You can get much more efficient code if you have text relocations than if you don't. This "read-only" memory is perfectly writable until after relocation, a la relro. But you no doubt will find some non-optimalities (or even straight out bugs) if you build with -ztext sometimes :-) Segher
[powerpc:merge] BUILD SUCCESS 01dc10da827c1725c0f5491c78d700a4478aae08
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git merge branch HEAD: 01dc10da827c1725c0f5491c78d700a4478aae08 Automatic merge of 'fixes' into merge (2021-08-12 23:46) elapsed time: 2610m configs tested: 231 configs skipped: 4 The following configs have been built successfully. More configs may be tested in the coming days. gcc tested configs: arm defconfig arm64allyesconfig arm64 defconfig arm allyesconfig arm allmodconfig i386 randconfig-c001-20210814 i386 randconfig-c001-20210812 i386 randconfig-c001-20210813 powerpc mpc8540_ads_defconfig ia64defconfig arm imx_v6_v7_defconfig mips rs90_defconfig pariscgeneric-32bit_defconfig arm am200epdkit_defconfig openrisc or1klitex_defconfig m68km5407c3_defconfig powerpcklondike_defconfig mips loongson1c_defconfig arm ep93xx_defconfig arm iop32x_defconfig powerpc tqm8xx_defconfig arm lpc18xx_defconfig shedosk7760_defconfig x86_64allnoconfig sh shx3_defconfig mips maltasmvp_eva_defconfig mips bigsur_defconfig powerpc64 defconfig powerpccell_defconfig arm davinci_all_defconfig mipsworkpad_defconfig arm omap2plus_defconfig powerpc pq2fads_defconfig h8300alldefconfig i386 alldefconfig mips fuloong2e_defconfig mips mtx1_defconfig arm u8500_defconfig nios2 3c120_defconfig ia64generic_defconfig powerpc tqm8555_defconfig powerpc acadia_defconfig arm simpad_defconfig mips cavium_octeon_defconfig nds32 defconfig parisc alldefconfig arm tct_hammer_defconfig powerpc obs600_defconfig powerpc makalu_defconfig arm shannon_defconfig armclps711x_defconfig powerpc ppc64e_defconfig sh ul2_defconfig arm orion5x_defconfig xtensa allyesconfig arm imx_v4_v5_defconfig powerpc ep88xc_defconfig powerpc rainier_defconfig shshmin_defconfig h8300 defconfig powerpc mpc834x_itxgp_defconfig mipsmaltaup_defconfig mips ip22_defconfig sh se7721_defconfig m68k m5208evb_defconfig x86_64 alldefconfig powerpc pseries_defconfig mipsgpr_defconfig mips maltaaprp_defconfig powerpc ppc6xx_defconfig powerpc mpc837x_mds_defconfig ia64 alldefconfig shedosk7705_defconfig sh se7750_defconfig powerpcsocrates_defconfig riscv allnoconfig powerpc ksi8560_defconfig powerpc mpc837x_rdb_defconfig sh landisk_defconfig powerpc mpc832x_rdb_defconfig powerpc ebony_defconfig arm h5000_defconfig powerpc ppc40x_defconfig h8300allyesconfig h8300 h8s-sim_defconfig arm aspeed_g4_defconfig sh sh7785lcr_32bit_defconfig mips lemote2f_defconfig mips rm200_defconfig arm stm32_defconfig powerpc ppc64_defconfig xtensa audio_kc705_defconfig mips loongson3_defconfig mips ath79_defconfig arc haps_hs_smp_defconfig sh se7712_defconfig powerpc microwatt_defconfig sh r7780mp_defconfig arm cns3420vb_defconfig mips ath25_defconfig m68k
Re: [PATCH v3 4/8] PCI: replace pci_dev::driver usage that gets the driver name
On Thu, Aug 12, 2021 at 10:14:25AM +0200, Uwe Kleine-K??nig wrote:
> 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 "";
>
> drvstr = dev_driver_string(>dev);
>
> if (!strcmp(drvstr, ""))
> return "";
>
> return drvstr;
>
> When I thought about this hunk I considered it ugly to have "" in
> it twice.
Well, if you want to avoid that you can do:
if (pdev) {
const char *name = dev_driver_string(>dev);
if (strcmp(drvstr, ""))
return name;
}
return "";
Which would be a lot more readable.
Re: [PATCH v1 17/55] KVM: PPC: Book3S HV P9: Implement PMU save/restore in C
> On 13-Aug-2021, at 9:54 AM, Nicholas Piggin wrote:
>
> Excerpts from Athira Rajeev's message of August 9, 2021 1:03 pm:
>>
>>
>>> On 26-Jul-2021, at 9:19 AM, Nicholas Piggin wrote:
>
>
>>> +static void freeze_pmu(unsigned long mmcr0, unsigned long mmcra)
>>> +{
>>> + if (!(mmcr0 & MMCR0_FC))
>>> + goto do_freeze;
>>> + if (mmcra & MMCRA_SAMPLE_ENABLE)
>>> + goto do_freeze;
>>> + if (cpu_has_feature(CPU_FTR_ARCH_31)) {
>>> + if (!(mmcr0 & MMCR0_PMCCEXT))
>>> + goto do_freeze;
>>> + if (!(mmcra & MMCRA_BHRB_DISABLE))
>>> + goto do_freeze;
>>> + }
>>> + return;
>>> +
>>> +do_freeze:
>>> + mmcr0 = MMCR0_FC;
>>> + mmcra = 0;
>>> + if (cpu_has_feature(CPU_FTR_ARCH_31)) {
>>> + mmcr0 |= MMCR0_PMCCEXT;
>>> + mmcra = MMCRA_BHRB_DISABLE;
>>> + }
>>> +
>>> + mtspr(SPRN_MMCR0, mmcr0);
>>> + mtspr(SPRN_MMCRA, mmcra);
>>> + isync();
>>> +}
>>> +
>> Hi Nick,
>>
>> After feezing pmu, do we need to clear “pmcregs_in_use” as well?
>
> Not until we save the values out of the registers. pmcregs_in_use = 0
> means our hypervisor is free to clear our PMU register contents.
>
>> Also can’t we unconditionally do the MMCR0/MMCRA/ freeze settings in here ?
>> do we need the if conditions for FC/PMCCEXT/BHRB ?
>
> I think it's possible, but pretty minimal advantage. I would prefer to
> set them the way perf does for now.
Sure Nick,
Other changes looks good to me.
Reviewed-by: Athira Rajeev
Thanks
Athira
> If we can move this code into perf/
> it should become easier for you to tweak things.
>
> Thanks,
> Nick
