* Re: [PATCH] of: error: 'const struct kimage' has no member named 'arch'
From: Lakshmi Ramasubramanian @ 2021-02-19 0:57 UTC (permalink / raw)
To: Mimi Zohar, bauerman, robh, takahiro.akashi, gregkh, will, joe,
catalin.marinas, mpe, sfr
Cc: sashal, devicetree, linux-kernel, james.morse, linux-integrity,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <c6490f6a126a2f10e3e3445b51ea552a26f896a9.camel@linux.ibm.com>
On 2/18/21 4:07 PM, Mimi Zohar wrote:
Hi Mimi,
> On Thu, 2021-02-18 at 14:33 -0800, Lakshmi Ramasubramanian wrote:
>> of_kexec_alloc_and_setup_fdt() defined in drivers/of/kexec.c builds
>> a new device tree object that includes architecture specific data
>> for kexec system call. This should be defined only if the architecture
>> being built defines kexec architecture structure "struct kimage_arch".
>>
>> Define a new boolean config OF_KEXEC that is enabled if
>> CONFIG_KEXEC_FILE and CONFIG_OF_FLATTREE are enabled, and
>> the architecture is arm64 or powerpc64. Build drivers/of/kexec.c
>> if CONFIG_OF_KEXEC is enabled.
>>
>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
>> Fixes: 33488dc4d61f ("of: Add a common kexec FDT setup function")
>> Reported-by: kernel test robot <lkp@intel.com>
>> ---
>> drivers/of/Kconfig | 6 ++++++
>> drivers/of/Makefile | 7 +------
>> 2 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>> index 18450437d5d5..f2e8fa54862a 100644
>> --- a/drivers/of/Kconfig
>> +++ b/drivers/of/Kconfig
>> @@ -100,4 +100,10 @@ config OF_DMA_DEFAULT_COHERENT
>> # arches should select this if DMA is coherent by default for OF devices
>> bool
>>
>> +config OF_KEXEC
>> + bool
>> + depends on KEXEC_FILE
>> + depends on OF_FLATTREE
>> + default y if ARM64 || PPC64
>> +
>> endif # OF
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index c13b982084a3..287579dd1695 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -13,11 +13,6 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
>> obj-$(CONFIG_OF_RESOLVE) += resolver.o
>> obj-$(CONFIG_OF_OVERLAY) += overlay.o
>> obj-$(CONFIG_OF_NUMA) += of_numa.o
>> -
>> -ifdef CONFIG_KEXEC_FILE
>> -ifdef CONFIG_OF_FLATTREE
>> -obj-y += kexec.o
>> -endif
>> -endif
>> +obj-$(CONFIG_OF_KEXEC) += kexec.o
>>
>> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
>
> Is it possible to reuse CONFIG_HAVE_IMA_KEXEC here?
>
For ppc64 CONFIG_HAVE_IMA_KEXEC is selected when CONFIG_KEXEC_FILE is
enabled. So I don't see a problem in reusing CONFIG_HAVE_IMA_KEXEC for ppc.
But for arm64, CONFIG_HAVE_IMA_KEXEC is enabled in the final patch in
the patch set (the one for carrying forward IMA log across kexec for
arm64). arm64 calls of_kexec_alloc_and_setup_fdt() prior to enabling
CONFIG_HAVE_IMA_KEXEC and hence breaks the build for arm64.
thanks,
-lakshmi
^ permalink raw reply
* Re: [PATCH] of: error: 'const struct kimage' has no member named 'arch'
From: Mimi Zohar @ 2021-02-19 0:07 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, bauerman, robh, takahiro.akashi, gregkh,
will, joe, catalin.marinas, mpe, sfr
Cc: sashal, devicetree, linux-kernel, james.morse, linux-integrity,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <20210218223305.2044-1-nramas@linux.microsoft.com>
On Thu, 2021-02-18 at 14:33 -0800, Lakshmi Ramasubramanian wrote:
> of_kexec_alloc_and_setup_fdt() defined in drivers/of/kexec.c builds
> a new device tree object that includes architecture specific data
> for kexec system call. This should be defined only if the architecture
> being built defines kexec architecture structure "struct kimage_arch".
>
> Define a new boolean config OF_KEXEC that is enabled if
> CONFIG_KEXEC_FILE and CONFIG_OF_FLATTREE are enabled, and
> the architecture is arm64 or powerpc64. Build drivers/of/kexec.c
> if CONFIG_OF_KEXEC is enabled.
>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> Fixes: 33488dc4d61f ("of: Add a common kexec FDT setup function")
> Reported-by: kernel test robot <lkp@intel.com>
> ---
> drivers/of/Kconfig | 6 ++++++
> drivers/of/Makefile | 7 +------
> 2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 18450437d5d5..f2e8fa54862a 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -100,4 +100,10 @@ config OF_DMA_DEFAULT_COHERENT
> # arches should select this if DMA is coherent by default for OF devices
> bool
>
> +config OF_KEXEC
> + bool
> + depends on KEXEC_FILE
> + depends on OF_FLATTREE
> + default y if ARM64 || PPC64
> +
> endif # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index c13b982084a3..287579dd1695 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -13,11 +13,6 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
> obj-$(CONFIG_OF_RESOLVE) += resolver.o
> obj-$(CONFIG_OF_OVERLAY) += overlay.o
> obj-$(CONFIG_OF_NUMA) += of_numa.o
> -
> -ifdef CONFIG_KEXEC_FILE
> -ifdef CONFIG_OF_FLATTREE
> -obj-y += kexec.o
> -endif
> -endif
> +obj-$(CONFIG_OF_KEXEC) += kexec.o
>
> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
Is it possible to reuse CONFIG_HAVE_IMA_KEXEC here?
Mimi
^ permalink raw reply
* Re: linux-next: manual merge of the devicetree tree with the powerpc tree
From: Michael Ellerman @ 2021-02-18 23:28 UTC (permalink / raw)
To: Rob Herring, Stephen Rothwell
Cc: Lakshmi Ramasubramanian, Linux Next Mailing List, PowerPC,
Linux Kernel Mailing List, Hari Bathini
In-Reply-To: <CAL_JsqJ9Ske4hkWn3uo8-nef29MQ1DkNdtE=gxbqj8CKrtQorg@mail.gmail.com>
Rob Herring <robherring2@gmail.com> writes:
> On Thu, Feb 18, 2021 at 5:34 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>> On Thu, 18 Feb 2021 21:44:37 +1100 Michael Ellerman <mpe@ellerman.id.au> wrote:
>> >
>> > I think it just needs this?
>> >
>> > diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
>> > index 87e34611f93d..0492ca6003f3 100644
>> > --- a/arch/powerpc/kexec/elf_64.c
>> > +++ b/arch/powerpc/kexec/elf_64.c
>> > @@ -104,7 +104,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>> >
>> > fdt = of_kexec_alloc_and_setup_fdt(image, initrd_load_addr,
>> > initrd_len, cmdline,
>> > - fdt_totalsize(initial_boot_params));
>> > + kexec_fdt_totalsize_ppc64(image));
>> > if (!fdt) {
>> > pr_err("Error setting up the new device tree.\n");
>> > ret = -EINVAL;
>> >
>>
>> I thought about that, but the last argument to
>> of_kexec_alloc_and_setup_fdt() is extra_fdt_size and the allocation
>> done is for this:
>>
>> fdt_size = fdt_totalsize(initial_boot_params) +
>> (cmdline ? strlen(cmdline) : 0) +
>> FDT_EXTRA_SPACE +
>> extra_fdt_size;
>>
>> and kexec_fdt_totalsize_ppc64() also includes
>> fdt_totalsize(initial_boot_params) so I was not sure. Maybe
>> kexec_fdt_totalsize_ppc64() needs modification as well?
>
> You're both right. Michael's fix is sufficient for the merge. The only
> risk with a larger size is failing to allocate it, but we're talking
> only 10s of KB. Historically until the commit causing the conflict,
> PPC was just used 2x fdt_totalsize(initial_boot_params). You could
> drop 'fdt_size = fdt_totalsize(initial_boot_params) + (2 *
> COMMAND_LINE_SIZE);' from kexec_fdt_totalsize_ppc64() as well, but
> then the function name is misleading.
>
> Lakshmi can send a follow-up patch to fine tune the size and rename
> kexec_fdt_totalsize_ppc64.
Sounds good.
cheers
^ permalink raw reply
* Re: [PATCH 2/6] powerpc/pseries: Add key to flags in pSeries_lpar_hpte_updateboltedpp()
From: Michael Ellerman @ 2021-02-18 23:25 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev; +Cc: aneesh.kumar
In-Reply-To: <87tuqca7vi.fsf@linkitivity.dja.id.au>
Daniel Axtens <dja@axtens.net> writes:
> Michael Ellerman <mpe@ellerman.id.au> writes:
>
>> The flags argument to plpar_pte_protect() (aka. H_PROTECT), includes
>> the key in bits 9-13, but currently we always set those bits to zero.
>>
>> In the past that hasn't been a problem because we always used key 0
>> for the kernel, and updateboltedpp() is only used for kernel mappings.
>>
>> However since commit d94b827e89dc ("powerpc/book3s64/kuap: Use Key 3
>> for kernel mapping with hash translation") we are now inadvertently
>> changing the key (to zero) when we call plpar_pte_protect().
>>
>> That hasn't broken anything because updateboltedpp() is only used for
>> STRICT_KERNEL_RWX, which is currently disabled on 64s due to other
>> bugs.
>>
>> But we want to fix that, so first we need to pass the key correctly to
>> plpar_pte_protect(). In the `newpp` value the low 3 bits of the key
>> are already in the correct spot, but the high 2 bits of the key need
>> to be shifted down.
>>
>> Fixes: d94b827e89dc ("powerpc/book3s64/kuap: Use Key 3 for kernel mapping with hash translation")
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>> arch/powerpc/platforms/pseries/lpar.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
>> index 764170fdb0f7..8bbbddff7226 100644
>> --- a/arch/powerpc/platforms/pseries/lpar.c
>> +++ b/arch/powerpc/platforms/pseries/lpar.c
>> @@ -976,11 +976,13 @@ static void pSeries_lpar_hpte_updateboltedpp(unsigned long newpp,
>> slot = pSeries_lpar_hpte_find(vpn, psize, ssize);
>> BUG_ON(slot == -1);
>>
>> - flags = newpp & 7;
>> + flags = newpp & (HPTE_R_PP | HPTE_R_N);
>> if (mmu_has_feature(MMU_FTR_KERNEL_RO))
>> /* Move pp0 into bit 8 (IBM 55) */
>> flags |= (newpp & HPTE_R_PP0) >> 55;
>>
>> + flags |= ((newpp & HPTE_R_KEY_HI) >> 48) | (newpp & HPTE_R_KEY_LO);
>> +
>
> I'm really confused about how these bits are getting packed into the
> flags parameter. It seems to match how they are unpacked in
> kvmppc_h_pr_protect, but I cannot figure out why they are packed in that
> order, and the LoPAR doesn't seem especially illuminating on this topic
> - although I may have missed the relevant section.
Yeah I agree it's not very clearly specified.
The hcall we're using here is H_PROTECT, which is specified in section
14.5.4.1.6 of LoPAPR v1.1.
It takes a `flags` parameter, and the description for flags says:
* flags: AVPN, pp0, pp1, pp2, key0-key4, n, and for the CMO
option: CMO Option flags as defined in Table 189‚
If you then go to the start of the parent section, 14.5.4.1, on page
405, it says:
Register Linkage (For hcall() tokens 0x04 - 0x18)
* On Call
* R3 function call token
* R4 flags (see Table 178‚ “Page Frame Table Access flags field definition‚” on page 401)
Then you have to go to section 14.5.3, and on page 394 there is a list
of hcalls and their tokens (table 176), and there you can see that
H_PROTECT == 0x18.
Finally you can look at table 178, on page 401, where it specifies the
layout of the bits for the key:
Bit Function
------------------
50-54 | key0-key4
Those are big-endian bit numbers, converting to normal bit numbers you
get bits 9-13, or 0x3e00.
If you look at the kernel source we have:
#define HPTE_R_KEY_HI ASM_CONST(0x3000000000000000)
#define HPTE_R_KEY_LO ASM_CONST(0x0000000000000e00)
So the LO bits are already in the right place, and the HI bits just need
to be shifted down by 48.
Hope that makes it clearer :)
cheers
^ permalink raw reply
* [PATCH] of: error: 'const struct kimage' has no member named 'arch'
From: Lakshmi Ramasubramanian @ 2021-02-18 22:33 UTC (permalink / raw)
To: zohar, bauerman, robh, takahiro.akashi, gregkh, will, joe,
catalin.marinas, mpe, sfr
Cc: sashal, devicetree, linux-kernel, james.morse, linux-integrity,
linuxppc-dev, linux-arm-kernel
of_kexec_alloc_and_setup_fdt() defined in drivers/of/kexec.c builds
a new device tree object that includes architecture specific data
for kexec system call. This should be defined only if the architecture
being built defines kexec architecture structure "struct kimage_arch".
Define a new boolean config OF_KEXEC that is enabled if
CONFIG_KEXEC_FILE and CONFIG_OF_FLATTREE are enabled, and
the architecture is arm64 or powerpc64. Build drivers/of/kexec.c
if CONFIG_OF_KEXEC is enabled.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Fixes: 33488dc4d61f ("of: Add a common kexec FDT setup function")
Reported-by: kernel test robot <lkp@intel.com>
---
drivers/of/Kconfig | 6 ++++++
drivers/of/Makefile | 7 +------
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 18450437d5d5..f2e8fa54862a 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -100,4 +100,10 @@ config OF_DMA_DEFAULT_COHERENT
# arches should select this if DMA is coherent by default for OF devices
bool
+config OF_KEXEC
+ bool
+ depends on KEXEC_FILE
+ depends on OF_FLATTREE
+ default y if ARM64 || PPC64
+
endif # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index c13b982084a3..287579dd1695 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -13,11 +13,6 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
obj-$(CONFIG_OF_RESOLVE) += resolver.o
obj-$(CONFIG_OF_OVERLAY) += overlay.o
obj-$(CONFIG_OF_NUMA) += of_numa.o
-
-ifdef CONFIG_KEXEC_FILE
-ifdef CONFIG_OF_FLATTREE
-obj-y += kexec.o
-endif
-endif
+obj-$(CONFIG_OF_KEXEC) += kexec.o
obj-$(CONFIG_OF_UNITTEST) += unittest-data/
--
2.30.0
^ permalink raw reply related
* Re: linux-next: manual merge of the devicetree tree with the powerpc tree
From: Stephen Rothwell @ 2021-02-18 20:31 UTC (permalink / raw)
To: Rob Herring
Cc: Linux Kernel Mailing List, Lakshmi Ramasubramanian,
Linux Next Mailing List, PowerPC, Hari Bathini
In-Reply-To: <CAL_JsqJ9Ske4hkWn3uo8-nef29MQ1DkNdtE=gxbqj8CKrtQorg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2287 bytes --]
Hi all,
On Thu, 18 Feb 2021 07:52:52 -0600 Rob Herring <robherring2@gmail.com> wrote:
>
> On Thu, Feb 18, 2021 at 5:34 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > On Thu, 18 Feb 2021 21:44:37 +1100 Michael Ellerman <mpe@ellerman.id.au> wrote:
> > >
> > > I think it just needs this?
> > >
> > > diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> > > index 87e34611f93d..0492ca6003f3 100644
> > > --- a/arch/powerpc/kexec/elf_64.c
> > > +++ b/arch/powerpc/kexec/elf_64.c
> > > @@ -104,7 +104,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> > >
> > > fdt = of_kexec_alloc_and_setup_fdt(image, initrd_load_addr,
> > > initrd_len, cmdline,
> > > - fdt_totalsize(initial_boot_params));
> > > + kexec_fdt_totalsize_ppc64(image));
> > > if (!fdt) {
> > > pr_err("Error setting up the new device tree.\n");
> > > ret = -EINVAL;
> > >
> >
> > I thought about that, but the last argument to
> > of_kexec_alloc_and_setup_fdt() is extra_fdt_size and the allocation
> > done is for this:
> >
> > fdt_size = fdt_totalsize(initial_boot_params) +
> > (cmdline ? strlen(cmdline) : 0) +
> > FDT_EXTRA_SPACE +
> > extra_fdt_size;
> >
> > and kexec_fdt_totalsize_ppc64() also includes
> > fdt_totalsize(initial_boot_params) so I was not sure. Maybe
> > kexec_fdt_totalsize_ppc64() needs modification as well?
>
> You're both right. Michael's fix is sufficient for the merge. The only
> risk with a larger size is failing to allocate it, but we're talking
> only 10s of KB. Historically until the commit causing the conflict,
> PPC was just used 2x fdt_totalsize(initial_boot_params). You could
> drop 'fdt_size = fdt_totalsize(initial_boot_params) + (2 *
> COMMAND_LINE_SIZE);' from kexec_fdt_totalsize_ppc64() as well, but
> then the function name is misleading.
>
> Lakshmi can send a follow-up patch to fine tune the size and rename
> kexec_fdt_totalsize_ppc64.
OK, I have mode Michael's suggested change to my resolution from today.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 484 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/4xx: Fix build errors from mfdcr()
From: Feng Tang @ 2021-02-18 13:04 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
In-Reply-To: <20210218123058.748882-1-mpe@ellerman.id.au>
On Thu, Feb 18, 2021 at 11:30:58PM +1100, Michael Ellerman wrote:
> lkp reported a build error in fsp2.o:
>
> CC arch/powerpc/platforms/44x/fsp2.o
> {standard input}:577: Error: unsupported relocation against base
>
> Which comes from:
>
> pr_err("GESR0: 0x%08x\n", mfdcr(base + PLB4OPB_GESR0));
>
> Where our mfdcr() macro is stringifying "base + PLB4OPB_GESR0", and
> passing that to the assembler, which obviously doesn't work.
>
> The mfdcr() macro already checks that the argument is constant using
> __builtin_constant_p(), and if not calls the out-of-line version of
> mfdcr(). But in this case GCC is smart enough to notice that "base +
> PLB4OPB_GESR0" will be constant, even though it's not something we can
> immediately stringify into a register number.
>
> Segher pointed out that passing the register number to the inline asm
> as a constant would be better, and in fact it fixes the build error,
> presumably because it gives GCC a chance to resolve the value.
>
> While we're at it, change mtdcr() similarly.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Acked-by: Feng Tang <feng.tang@intel.com>
Thanks!
> ---
> arch/powerpc/include/asm/dcr-native.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/dcr-native.h b/arch/powerpc/include/asm/dcr-native.h
> index 7141ccea8c94..a92059964579 100644
> --- a/arch/powerpc/include/asm/dcr-native.h
> +++ b/arch/powerpc/include/asm/dcr-native.h
> @@ -53,8 +53,8 @@ static inline void mtdcrx(unsigned int reg, unsigned int val)
> #define mfdcr(rn) \
> ({unsigned int rval; \
> if (__builtin_constant_p(rn) && rn < 1024) \
> - asm volatile("mfdcr %0," __stringify(rn) \
> - : "=r" (rval)); \
> + asm volatile("mfdcr %0, %1" : "=r" (rval) \
> + : "n" (rn)); \
> else if (likely(cpu_has_feature(CPU_FTR_INDEXED_DCR))) \
> rval = mfdcrx(rn); \
> else \
> @@ -64,8 +64,8 @@ static inline void mtdcrx(unsigned int reg, unsigned int val)
> #define mtdcr(rn, v) \
> do { \
> if (__builtin_constant_p(rn) && rn < 1024) \
> - asm volatile("mtdcr " __stringify(rn) ",%0" \
> - : : "r" (v)); \
> + asm volatile("mtdcr %0, %1" \
> + : : "n" (rn), "r" (v)); \
> else if (likely(cpu_has_feature(CPU_FTR_INDEXED_DCR))) \
> mtdcrx(rn, v); \
> else \
> --
> 2.25.1
^ permalink raw reply
* Re: linux-next: manual merge of the devicetree tree with the powerpc tree
From: Rob Herring @ 2021-02-18 13:52 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Linux Kernel Mailing List, Lakshmi Ramasubramanian,
Linux Next Mailing List, PowerPC, Hari Bathini
In-Reply-To: <20210218223427.77109d83@canb.auug.org.au>
On Thu, Feb 18, 2021 at 5:34 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Hi Michael,
>
> On Thu, 18 Feb 2021 21:44:37 +1100 Michael Ellerman <mpe@ellerman.id.au> wrote:
> >
> > I think it just needs this?
> >
> > diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> > index 87e34611f93d..0492ca6003f3 100644
> > --- a/arch/powerpc/kexec/elf_64.c
> > +++ b/arch/powerpc/kexec/elf_64.c
> > @@ -104,7 +104,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> >
> > fdt = of_kexec_alloc_and_setup_fdt(image, initrd_load_addr,
> > initrd_len, cmdline,
> > - fdt_totalsize(initial_boot_params));
> > + kexec_fdt_totalsize_ppc64(image));
> > if (!fdt) {
> > pr_err("Error setting up the new device tree.\n");
> > ret = -EINVAL;
> >
>
> I thought about that, but the last argument to
> of_kexec_alloc_and_setup_fdt() is extra_fdt_size and the allocation
> done is for this:
>
> fdt_size = fdt_totalsize(initial_boot_params) +
> (cmdline ? strlen(cmdline) : 0) +
> FDT_EXTRA_SPACE +
> extra_fdt_size;
>
> and kexec_fdt_totalsize_ppc64() also includes
> fdt_totalsize(initial_boot_params) so I was not sure. Maybe
> kexec_fdt_totalsize_ppc64() needs modification as well?
You're both right. Michael's fix is sufficient for the merge. The only
risk with a larger size is failing to allocate it, but we're talking
only 10s of KB. Historically until the commit causing the conflict,
PPC was just used 2x fdt_totalsize(initial_boot_params). You could
drop 'fdt_size = fdt_totalsize(initial_boot_params) + (2 *
COMMAND_LINE_SIZE);' from kexec_fdt_totalsize_ppc64() as well, but
then the function name is misleading.
Lakshmi can send a follow-up patch to fine tune the size and rename
kexec_fdt_totalsize_ppc64.
Rob
^ permalink raw reply
* Re: [PATCH kernel] powerpc/iommu: Annotate nested lock for lockdep
From: Frederic Barrat @ 2021-02-18 12:59 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev; +Cc: kvm-ppc
In-Reply-To: <20210216032000.21642-1-aik@ozlabs.ru>
On 16/02/2021 04:20, Alexey Kardashevskiy wrote:
> The IOMMU table is divided into pools for concurrent mappings and each
> pool has a separate spinlock. When taking the ownership of an IOMMU group
> to pass through a device to a VM, we lock these spinlocks which triggers
> a false negative warning in lockdep (below).
>
> This fixes it by annotating the large pool's spinlock as a nest lock.
>
> ===
> WARNING: possible recursive locking detected
> 5.11.0-le_syzkaller_a+fstn1 #100 Not tainted
> --------------------------------------------
> qemu-system-ppc/4129 is trying to acquire lock:
> c0000000119bddb0 (&(p->lock)/1){....}-{2:2}, at: iommu_take_ownership+0xac/0x1e0
>
> but task is already holding lock:
> c0000000119bdd30 (&(p->lock)/1){....}-{2:2}, at: iommu_take_ownership+0xac/0x1e0
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(&(p->lock)/1);
> lock(&(p->lock)/1);
> ===
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/kernel/iommu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index 557a09dd5b2f..2ee642a6731a 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -1089,7 +1089,7 @@ int iommu_take_ownership(struct iommu_table *tbl)
>
> spin_lock_irqsave(&tbl->large_pool.lock, flags);
> for (i = 0; i < tbl->nr_pools; i++)
> - spin_lock(&tbl->pools[i].lock);
> + spin_lock_nest_lock(&tbl->pools[i].lock, &tbl->large_pool.lock);
We have the same pattern and therefore should have the same problem in
iommu_release_ownership().
But as I understand, we're hacking our way around lockdep here, since
conceptually, those locks are independent. I was wondering why it seems
to fix it by worrying only about the large pool lock. That loop can take
many locks (up to 4 with current config). However, if the dma window is
less than 1GB, we would only have one, so it would make sense for
lockdep to stop complaining. Is it what happened? In which case, this
patch doesn't really fix it. Or I'm missing something :-)
Fred
> iommu_table_release_pages(tbl);
>
>
^ permalink raw reply
* [PATCH] powerpc/4xx: Fix build errors from mfdcr()
From: Michael Ellerman @ 2021-02-18 12:30 UTC (permalink / raw)
To: linuxppc-dev; +Cc: feng.tang
lkp reported a build error in fsp2.o:
CC arch/powerpc/platforms/44x/fsp2.o
{standard input}:577: Error: unsupported relocation against base
Which comes from:
pr_err("GESR0: 0x%08x\n", mfdcr(base + PLB4OPB_GESR0));
Where our mfdcr() macro is stringifying "base + PLB4OPB_GESR0", and
passing that to the assembler, which obviously doesn't work.
The mfdcr() macro already checks that the argument is constant using
__builtin_constant_p(), and if not calls the out-of-line version of
mfdcr(). But in this case GCC is smart enough to notice that "base +
PLB4OPB_GESR0" will be constant, even though it's not something we can
immediately stringify into a register number.
Segher pointed out that passing the register number to the inline asm
as a constant would be better, and in fact it fixes the build error,
presumably because it gives GCC a chance to resolve the value.
While we're at it, change mtdcr() similarly.
Reported-by: kernel test robot <lkp@intel.com>
Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/include/asm/dcr-native.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/dcr-native.h b/arch/powerpc/include/asm/dcr-native.h
index 7141ccea8c94..a92059964579 100644
--- a/arch/powerpc/include/asm/dcr-native.h
+++ b/arch/powerpc/include/asm/dcr-native.h
@@ -53,8 +53,8 @@ static inline void mtdcrx(unsigned int reg, unsigned int val)
#define mfdcr(rn) \
({unsigned int rval; \
if (__builtin_constant_p(rn) && rn < 1024) \
- asm volatile("mfdcr %0," __stringify(rn) \
- : "=r" (rval)); \
+ asm volatile("mfdcr %0, %1" : "=r" (rval) \
+ : "n" (rn)); \
else if (likely(cpu_has_feature(CPU_FTR_INDEXED_DCR))) \
rval = mfdcrx(rn); \
else \
@@ -64,8 +64,8 @@ static inline void mtdcrx(unsigned int reg, unsigned int val)
#define mtdcr(rn, v) \
do { \
if (__builtin_constant_p(rn) && rn < 1024) \
- asm volatile("mtdcr " __stringify(rn) ",%0" \
- : : "r" (v)); \
+ asm volatile("mtdcr %0, %1" \
+ : : "n" (rn), "r" (v)); \
else if (likely(cpu_has_feature(CPU_FTR_INDEXED_DCR))) \
mtdcrx(rn, v); \
else \
--
2.25.1
^ permalink raw reply related
* Re: linux-next: manual merge of the devicetree tree with the powerpc tree
From: Stephen Rothwell @ 2021-02-18 11:34 UTC (permalink / raw)
To: Michael Ellerman
Cc: Rob Herring, Linux Kernel Mailing List, Lakshmi Ramasubramanian,
Linux Next Mailing List, Rob Herring, PowerPC, Hari Bathini
In-Reply-To: <874ki9vene.fsf@mpe.ellerman.id.au>
[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]
Hi Michael,
On Thu, 18 Feb 2021 21:44:37 +1100 Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> I think it just needs this?
>
> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> index 87e34611f93d..0492ca6003f3 100644
> --- a/arch/powerpc/kexec/elf_64.c
> +++ b/arch/powerpc/kexec/elf_64.c
> @@ -104,7 +104,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>
> fdt = of_kexec_alloc_and_setup_fdt(image, initrd_load_addr,
> initrd_len, cmdline,
> - fdt_totalsize(initial_boot_params));
> + kexec_fdt_totalsize_ppc64(image));
> if (!fdt) {
> pr_err("Error setting up the new device tree.\n");
> ret = -EINVAL;
>
I thought about that, but the last argument to
of_kexec_alloc_and_setup_fdt() is extra_fdt_size and the allocation
done is for this:
fdt_size = fdt_totalsize(initial_boot_params) +
(cmdline ? strlen(cmdline) : 0) +
FDT_EXTRA_SPACE +
extra_fdt_size;
and kexec_fdt_totalsize_ppc64() also includes
fdt_totalsize(initial_boot_params) so I was not sure. Maybe
kexec_fdt_totalsize_ppc64() needs modification as well?
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v5 05/10] powerpc/signal64: Remove TM ifdefery in middle of if/else block
From: Michael Ellerman @ 2021-02-18 10:47 UTC (permalink / raw)
To: Christopher M. Riedl, Daniel Axtens, linuxppc-dev
In-Reply-To: <C9C1XKNVRQC4.LHKIIQAIC3L7@oc8246131445.ibm.com>
"Christopher M. Riedl" <cmr@codefail.de> writes:
> On Thu Feb 11, 2021 at 11:21 PM CST, Daniel Axtens wrote:
...
>>
>> My only concern here was whether it was valid to access
>> if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
>> if CONFIG_PPC_TRANSACTIONAL_MEM was not defined, but I didn't think of
>> any obvious reason why it wouldn't be...
>
> Hmm, we don't really need it for the non-TM case and it is another extra
> uaccess. I will take your suggestion to remove the need for the other
> ifdefs but might keep this one. Keeping it also makes it very clear this
> call is only here for TM and possible to remove in a potentially TM-less
> future :)
Yep I agree.
cheers
^ permalink raw reply
* Re: linux-next: manual merge of the devicetree tree with the powerpc tree
From: Michael Ellerman @ 2021-02-18 10:44 UTC (permalink / raw)
To: Stephen Rothwell, Rob Herring, PowerPC
Cc: Lakshmi Ramasubramanian, Linux Next Mailing List,
Linux Kernel Mailing List, Hari Bathini, Rob Herring
In-Reply-To: <20210218144815.5673ae6f@canb.auug.org.au>
Stephen Rothwell <sfr@canb.auug.org.au> writes:
> Hi all,
>
> Today's linux-next merge of the devicetree tree got a conflict in:
>
> arch/powerpc/kexec/elf_64.c
>
> between commit:
>
> 2377c92e37fe ("powerpc/kexec_file: fix FDT size estimation for kdump kernel")
>
> from the powerpc tree and commit:
>
> 130b2d59cec0 ("powerpc: Use common of_kexec_alloc_and_setup_fdt()")
>
> from the devicetree tree.
>
> I can't easily see how to resolve these, so for now I have just used
> the latter' changes to this file.
I think it just needs this?
diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index 87e34611f93d..0492ca6003f3 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -104,7 +104,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
fdt = of_kexec_alloc_and_setup_fdt(image, initrd_load_addr,
initrd_len, cmdline,
- fdt_totalsize(initial_boot_params));
+ kexec_fdt_totalsize_ppc64(image));
if (!fdt) {
pr_err("Error setting up the new device tree.\n");
ret = -EINVAL;
cheers
^ permalink raw reply related
* Re: [PATCH v2 2/7] ASoC: fsl_rpmsg: Add CPU DAI driver for audio base on rpmsg
From: Shengjiu Wang @ 2021-02-18 7:57 UTC (permalink / raw)
To: Mark Brown
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
alsa-devel, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Takashi Iwai, Liam Girdwood, linux-kernel, Nicolin Chen,
Rob Herring, linuxppc-dev
In-Reply-To: <20210210153808.GB4748@sirena.org.uk>
On Wed, Feb 10, 2021 at 11:39 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Feb 10, 2021 at 02:35:29PM +0800, Shengjiu Wang wrote:
> > On Wed, Feb 10, 2021 at 6:30 AM Mark Brown <broonie@kernel.org> wrote:
>
> > > Like I say I'd actually recommend moving this control to DAPM.
>
> > I may understand your point, you suggest to use the .set_bias_level
> > interface. But in my case I need to enable the clock in earlier stage
> > and keep the clock on when system go to suspend.
>
> The device can be kept alive over system suspend if that's needed, or
> possibly it sounds like runtime PM is a better fit? There's callbacks
> in the core to keep the device runtime PM enabled while it's open which
> is probably about the time range you're looking for.
Before enabling the clock, I need to reparent the clock according to
the sample rate, Maybe the hw_params is the right place to do
these things.
Can I add a flag:
"rpmsg->mclk_streams & BIT(substream->stream)"
for avoiding multiple calls of hw_params function before enabling
clock?
^ permalink raw reply
* Re: [PATCH v2 7/7] ASoC: dt-bindings: imx-rpmsg: Add binding doc for rpmsg machine driver
From: Shengjiu Wang @ 2021-02-18 7:23 UTC (permalink / raw)
To: Rob Herring
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
alsa-devel, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Takashi Iwai, Liam Girdwood, linux-kernel, Nicolin Chen,
Mark Brown, linuxppc-dev
In-Reply-To: <20210210221704.GA2894134@robh.at.kernel.org>
On Thu, Feb 11, 2021 at 6:18 AM Rob Herring <robh@kernel.org> wrote:
>
> On Sun, Feb 07, 2021 at 06:23:55PM +0800, Shengjiu Wang wrote:
> > Imx-rpmsg is a new added machine driver for supporting audio on Cortex-M
> > core. The Cortex-M core will control the audio interface, DMA and audio
> > codec, setup the pipeline, the audio driver on Cortex-A core side is just
> > to communitcate with M core, it is a virtual sound card and don't touch
> > the hardware.
>
> I don't understand why there are 2 nodes for this other than you happen
> to want to split this into 2 Linux drivers. It's 1 h/w thing.
This one is for the sound card machine driver. Another one is
for the sound card cpu dai driver. so there are 2 nodes.
>
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> > .../bindings/sound/imx-audio-rpmsg.yaml | 48 +++++++++++++++++++
> > 1 file changed, 48 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/sound/imx-audio-rpmsg.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/imx-audio-rpmsg.yaml b/Documentation/devicetree/bindings/sound/imx-audio-rpmsg.yaml
> > new file mode 100644
> > index 000000000000..b941aeb80678
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/imx-audio-rpmsg.yaml
> > @@ -0,0 +1,48 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/imx-audio-rpmsg.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP i.MX audio complex with rpmsg
> > +
> > +maintainers:
> > + - Shengjiu Wang <shengjiu.wang@nxp.com>
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - fsl,imx-audio-rpmsg
> > +
> > + model:
> > + $ref: /schemas/types.yaml#/definitions/string
> > + description: User specified audio sound card name
> > +
> > + audio-cpu:
> > + description: The phandle of an CPU DAI controller
> > +
> > + rpmsg-out:
> > + description: |
> > + This is a boolean property. If present, the transmitting function
> > + will be enabled,
> > +
> > + rpmsg-in:
> > + description: |
> > + This is a boolean property. If present, the receiving function
> > + will be enabled.
> > +
> > +required:
> > + - compatible
> > + - model
> > + - audio-cpu
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + sound-rpmsg {
> > + compatible = "fsl,imx-audio-rpmsg";
> > + model = "ak4497-audio";
> > + audio-cpu = <&rpmsg_audio>;
> > + rpmsg-out;
> > + };
> > --
> > 2.27.0
> >
^ permalink raw reply
* Re: [PATCH v2 3/7] ASoC: dt-bindings: fsl_rpmsg: Add binding doc for rpmsg cpu dai driver
From: Shengjiu Wang @ 2021-02-18 7:21 UTC (permalink / raw)
To: Rob Herring
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
alsa-devel, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Takashi Iwai, Liam Girdwood, linux-kernel, Nicolin Chen,
Mark Brown, linuxppc-dev
In-Reply-To: <20210210221252.GA2885308@robh.at.kernel.org>
On Thu, Feb 11, 2021 at 6:13 AM Rob Herring <robh@kernel.org> wrote:
>
> On Sun, Feb 07, 2021 at 06:23:51PM +0800, Shengjiu Wang wrote:
> > fsl_rpmsg cpu dai driver is driver for rpmsg audio, which is mainly used
> > for getting the user's configuration from device tree and configure the
> > clocks which is used by Cortex-M core. So in this document define the
> > needed property.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> > .../devicetree/bindings/sound/fsl,rpmsg.yaml | 80 +++++++++++++++++++
> > 1 file changed, 80 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/sound/fsl,rpmsg.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/fsl,rpmsg.yaml b/Documentation/devicetree/bindings/sound/fsl,rpmsg.yaml
> > new file mode 100644
> > index 000000000000..2d3ce10d42fc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/fsl,rpmsg.yaml
> > @@ -0,0 +1,80 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/fsl,rpmsg.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP Audio RPMSG CPU DAI Controller
> > +
> > +maintainers:
> > + - Shengjiu Wang <shengjiu.wang@nxp.com>
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - fsl,imx7ulp-rpmsg
> > + - fsl,imx8mn-rpmsg
> > + - fsl,imx8mm-rpmsg
> > + - fsl,imx8mp-rpmsg
>
> rpmsg is a protocol. What's the h/w block?
On Linux side this driver is a virtual driver, it is running
on Arm Cortex-A core. The h/w block is controlled by
another core (cortex-M core). so this driver actually
doesn't touch any hardware, it just does configuration
for rpmsg channel.
>
> > +
> > + clocks:
> > + items:
> > + - description: Peripheral clock for register access
> > + - description: Master clock
> > + - description: DMA clock for DMA register access
> > + - description: Parent clock for multiple of 8kHz sample rates
> > + - description: Parent clock for multiple of 11kHz sample rates
> > + minItems: 5
> > +
> > + clock-names:
> > + items:
> > + - const: ipg
> > + - const: mclk
> > + - const: dma
> > + - const: pll8k
> > + - const: pll11k
> > + minItems: 5
> > +
> > + power-domains:
> > + maxItems: 1
> > +
> > + fsl,audioindex:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: instance index for rpmsg image
> > +
> > + fsl,version:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: rpmsg image version index
>
> What are these 2 used for?
fsl,audioindex: As we may support multiple instance, for example
two sound card with one rpmsg channel, this is the instance index.
fsl,version: There are maybe different image running on M core, this
is the image version, different image has different function.
>
> > +
> > + fsl,buffer-size:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: pre allocate dma buffer size
> > +
> > + fsl,enable-lpa:
> > + $ref: /schemas/types.yaml#/definitions/flag
> > + description: enable low power audio path.
> > +
> > + fsl,codec-type:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: Sometimes the codec is registered by
> > + driver not the device tree, this items
> > + can be used to distinguish codecs
>
> 0-2^32 are valid values?
I should add range for it.
>
> > +
> > +required:
> > + - compatible
> > + - fsl,audioindex
> > + - fsl,version
> > + - fsl,buffer-size
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + rpmsg_audio: rpmsg_audio {
> > + compatible = "fsl,imx8mn-rpmsg";
> > + fsl,audioindex = <0> ;
> > + fsl,version = <2>;
> > + fsl,buffer-size = <0x6000000>;
> > + fsl,enable-lpa;
> > + status = "okay";
>
> Don't show status in examples.
ok, will remove it.
>
> > + };
> > --
> > 2.27.0
> >
^ permalink raw reply
* Re: [PATCH 4/4] ASoC: fsl: drop unneeded snd_soc_dai_set_drvdata
From: Nicolin Chen @ 2021-02-18 6:30 UTC (permalink / raw)
To: Julia Lawall
Cc: alsa-devel, linuxppc-dev, Timur Tabi, Xiubo Li, Shengjiu Wang,
Takashi Iwai, kernel-janitors, Liam Girdwood, Jaroslav Kysela,
Mark Brown, Fabio Estevam, linux-kernel
In-Reply-To: <20210213101907.1318496-5-Julia.Lawall@inria.fr>
On Sat, Feb 13, 2021 at 11:19:07AM +0100, Julia Lawall wrote:
> snd_soc_dai_set_drvdata is not needed when the set data comes from
> snd_soc_dai_get_drvdata or dev_get_drvdata. The problem was fixed
> usingthe following semantic patch: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression x,y,e;
> @@
> x = dev_get_drvdata(y->dev)
> ... when != x = e
> - snd_soc_dai_set_drvdata(y,x);
>
> @@
> expression x,y,e;
> @@
> x = snd_soc_dai_get_drvdata(y)
> ... when != x = e
> - snd_soc_dai_set_drvdata(y,x);
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr>
Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>
^ permalink raw reply
* Re: [PATCH] arm64: defconfig: enable modern virtio pci device
From: Jason Wang @ 2021-02-18 6:04 UTC (permalink / raw)
To: Arnd Bergmann, Anders Roxell
Cc: Chris Zankel, Thomas Bogendoerfer, Michael S. Tsirkin,
Arnd Bergmann, linuxppc-dev, Catalin Marinas, linux-xtensa,
Paul Walmsley, virtualization, linux-kernel@vger.kernel.org,
Russell King - ARM Linux, Max Filippov, SoC Team, Albert Ou,
Palmer Dabbelt, linux-riscv, open list:BROADCOM NVRAM DRIVER,
Will Deacon, Linux ARM
In-Reply-To: <CAK8P3a2ysNApoG2FDsLdNoWA7nPXvzLMzkjXWdCig9jaSWwuKw@mail.gmail.com>
On 2021/2/11 下午7:52, Arnd Bergmann wrote:
> On Wed, Feb 10, 2021 at 8:05 PM Anders Roxell <anders.roxell@linaro.org> wrote:
>> Since patch ("virtio-pci: introduce modern device module") got added it
>> is not possible to boot a defconfig kernel in qemu with a virtio pci
>> device. Add CONFIG_VIRTIO_PCI_MODERN=y fragment makes the kernel able
>> to boot.
>>
>> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
>> ---
>> arch/arm/configs/multi_v7_defconfig | 1 +
>> arch/arm64/configs/defconfig | 1 +
> Acked-by: Arnd Bergmann <arnd@arndb.de>
>
> Michael, can you pick this up in the vhost tree that introduces the regression?
>
> Arnd
>
Hi:
Based on the discussion previously, the plan is to select
VIRTIO_PCI_MODERN, and document the module that select it must depend on
PCI.
I will post a patch soon.
Thanks
^ permalink raw reply
* linux-next: manual merge of the devicetree tree with the powerpc tree
From: Stephen Rothwell @ 2021-02-18 3:48 UTC (permalink / raw)
To: Rob Herring, Michael Ellerman, PowerPC
Cc: Lakshmi Ramasubramanian, Linux Next Mailing List,
Linux Kernel Mailing List, Hari Bathini, Rob Herring
[-- Attachment #1: Type: text/plain, Size: 858 bytes --]
Hi all,
Today's linux-next merge of the devicetree tree got a conflict in:
arch/powerpc/kexec/elf_64.c
between commit:
2377c92e37fe ("powerpc/kexec_file: fix FDT size estimation for kdump kernel")
from the powerpc tree and commit:
130b2d59cec0 ("powerpc: Use common of_kexec_alloc_and_setup_fdt()")
from the devicetree tree.
I can't easily see how to resolve these, so for now I have just used
the latter' changes to this file.
I fixed it up and can carry the fix as necessary. This is now fixed as
far as linux-next is concerned, but any non trivial conflicts should be
mentioned to your upstream maintainer when your tree is submitted for
merging. You may also want to consider cooperating with the maintainer
of the conflicting tree to minimise any particularly complex conflicts.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v18 00/10] Carry forward IMA measurement log on kexec on ARM64
From: Rob Herring @ 2021-02-18 1:25 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: mark.rutland, tao.li, zohar, paulus, vincenzo.frascino,
frowand.list, sashal, masahiroy, jmorris, takahiro.akashi,
linux-arm-kernel, catalin.marinas, serge, devicetree,
pasha.tatashin, will, prsriva, hsinyi, allison, christophe.leroy,
mbrugger, balajib, dmitry.kasatkin, linux-kernel, james.morse,
gregkh, joe, linux-integrity, linuxppc-dev, bauerman
In-Reply-To: <20210213161049.6190-1-nramas@linux.microsoft.com>
On Sat, Feb 13, 2021 at 08:10:38AM -0800, Lakshmi Ramasubramanian wrote:
> On kexec file load Integrity Measurement Architecture (IMA) subsystem
> may verify the IMA signature of the kernel and initramfs, and measure
> it. The command line parameters passed to the kernel in the kexec call
> may also be measured by IMA. A remote attestation service can verify
> a TPM quote based on the TPM event log, the IMA measurement list, and
> the TPM PCR data. This can be achieved only if the IMA measurement log
> is carried over from the current kernel to the next kernel across
> the kexec call.
>
> powerpc already supports carrying forward the IMA measurement log on
> kexec. This patch set adds support for carrying forward the IMA
> measurement log on kexec on ARM64.
>
> This patch set moves the platform independent code defined for powerpc
> such that it can be reused for other platforms as well. A chosen node
> "linux,ima-kexec-buffer" is added to the DTB for ARM64 to hold
> the address and the size of the memory reserved to carry
> the IMA measurement log.
>
> This patch set has been tested for ARM64 platform using QEMU.
> I would like help from the community for testing this change on powerpc.
> Thanks.
>
> This patch set is based on
> commit f31e3386a4e9 ("ima: Free IMA measurement buffer after kexec syscall")
> in https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
> "ima-kexec-fixes" branch.
>
> Changelog:
>
> v18
> - Added a parameter to of_kexec_alloc_and_setup_fdt() for the caller
> to specify additional space needed for the FDT buffer
> - Renamed arm64 and powerpc ELF buffer address field in
> "struct kimage_arch" to elf_load_addr to match x86_64 name.
> - Removed of_ima_add_kexec_buffer() and instead directly set
> ima_buffer_addr and ima_buffer_size in ima_add_kexec_buffer()
> - Moved FDT_EXTRA_SPACE definition from include/linux/of.h to
> drivers/of/kexec.c
>
> v17
> - Renamed of_kexec_setup_new_fdt() to of_kexec_alloc_and_setup_fdt(),
> and moved memory allocation for the new FDT to this function.
>
> v16
> - Defined functions to allocate and free buffer for FDT for powerpc
> and arm64.
> - Moved ima_buffer_addr and ima_buffer_size fields from
> "struct kimage_arch" in powerpc to "struct kimage"
> v15
> - Included Rob's patches in the patch set, and rebased
> the changes to "next-integrity" branch.
> - Allocate memory for DTB, on arm64, using kmalloc() instead of
> vmalloc() to keep it consistent with powerpc implementation.
> - Call of_kexec_setup_new_fdt() from setup_new_fdt_ppc64() and
> remove setup_new_fdt() in the same patch to keep it bisect safe.
>
> v14
> - Select CONFIG_HAVE_IMA_KEXEC for CONFIG_KEXEC_FILE, for powerpc
> and arm64, if CONFIG_IMA is enabled.
> - Use IS_ENABLED() macro instead of "#ifdef" in remove_ima_buffer(),
> ima_get_kexec_buffer(), and ima_free_kexec_buffer().
> - Call of_kexec_setup_new_fdt() from setup_new_fdt_ppc64() and
> remove setup_new_fdt() in "arch/powerpc/kexec/file_load.c".
>
> v13
> - Moved the arch independent functions to drivers/of/kexec.c
> and then refactored the code.
> - Moved arch_ima_add_kexec_buffer() to
> security/integrity/ima/ima_kexec.c
>
> v12
> - Use fdt_appendprop_addrrange() in setup_ima_buffer()
> to setup the IMA measurement list property in
> the device tree.
> - Moved architecture independent functions from
> "arch/powerpc/kexec/ima.c" to "drivers/of/kexec."
> - Deleted "arch/powerpc/kexec/ima.c" and
> "arch/powerpc/include/asm/ima.h".
>
> v11
> - Rebased the changes on the kexec code refactoring done by
> Rob Herring in his "dt/kexec" branch
> - Removed "extern" keyword in function declarations
> - Removed unnecessary header files included in C files
> - Updated patch descriptions per Thiago's comments
>
> v10
> - Moved delete_fdt_mem_rsv(), remove_ima_buffer(),
> get_ima_kexec_buffer, and get_root_addr_size_cells()
> to drivers/of/kexec.c
> - Moved arch_ima_add_kexec_buffer() to
> security/integrity/ima/ima_kexec.c
> - Conditionally define IMA buffer fields in struct kimage_arch
>
> v9
> - Moved delete_fdt_mem_rsv() to drivers/of/kexec_fdt.c
> - Defined a new function get_ima_kexec_buffer() in
> drivers/of/ima_kexec.c to replace do_get_kexec_buffer()
> - Changed remove_ima_kexec_buffer() to the original function name
> remove_ima_buffer()
> - Moved remove_ima_buffer() to drivers/of/ima_kexec.c
> - Moved ima_get_kexec_buffer() and ima_free_kexec_buffer()
> to security/integrity/ima/ima_kexec.c
>
> v8:
> - Moved remove_ima_kexec_buffer(), do_get_kexec_buffer(), and
> delete_fdt_mem_rsv() to drivers/of/fdt.c
> - Moved ima_dump_measurement_list() and ima_add_kexec_buffer()
> back to security/integrity/ima/ima_kexec.c
>
> v7:
> - Renamed remove_ima_buffer() to remove_ima_kexec_buffer() and moved
> this function definition to kernel.
> - Moved delete_fdt_mem_rsv() definition to kernel
> - Moved ima_dump_measurement_list() and ima_add_kexec_buffer() to
> a new file namely ima_kexec_fdt.c in IMA
>
> v6:
> - Remove any existing FDT_PROP_IMA_KEXEC_BUFFER property in the device
> tree and also its corresponding memory reservation in the currently
> running kernel.
> - Moved the function remove_ima_buffer() defined for powerpc to IMA
> and renamed the function to ima_remove_kexec_buffer(). Also, moved
> delete_fdt_mem_rsv() from powerpc to IMA.
>
> v5:
> - Merged get_addr_size_cells() and do_get_kexec_buffer() into a single
> function when moving the arch independent code from powerpc to IMA
> - Reverted the change to use FDT functions in powerpc code and added
> back the original code in get_addr_size_cells() and
> do_get_kexec_buffer() for powerpc.
> - Added fdt_add_mem_rsv() for ARM64 to reserve the memory for
> the IMA log buffer during kexec.
> - Fixed the warning reported by kernel test bot for ARM64
> arch_ima_add_kexec_buffer() - moved this function to a new file
> namely arch/arm64/kernel/ima_kexec.c
>
> v4:
> - Submitting the patch series on behalf of the original author
> Prakhar Srivastava <prsriva@linux.microsoft.com>
> - Moved FDT_PROP_IMA_KEXEC_BUFFER ("linux,ima-kexec-buffer") to
> libfdt.h so that it can be shared by multiple platforms.
>
> v3:
> Breakup patches further into separate patches.
> - Refactoring non architecture specific code out of powerpc
> - Update powerpc related code to use fdt functions
> - Update IMA buffer read related code to use of functions
> - Add support to store the memory information of the IMA
> measurement logs to be carried forward.
> - Update the property strings to align with documented nodes
> https://github.com/devicetree-org/dt-schema/pull/46
>
> v2:
> Break patches into separate patches.
> - Powerpc related Refactoring
> - Updating the docuemntation for chosen node
> - Updating arm64 to support IMA buffer pass
>
> v1:
> Refactoring carrying over IMA measuremnet logs over Kexec. This patch
> moves the non-architecture specific code out of powerpc and adds to
> security/ima.(Suggested by Thiago)
> Add Documentation regarding the ima-kexec-buffer node in the chosen
> node documentation
>
> v0:
> Add a layer of abstraction to use the memory reserved by device tree
> for ima buffer pass.
> Add support for ima buffer pass using reserved memory for arm64 kexec.
> Update the arch sepcific code path in kexec file load to store the
> ima buffer in the reserved memory. The same reserved memory is read
> on kexec or cold boot.
>
> Lakshmi Ramasubramanian (7):
> arm64: Rename kexec elf_headers_mem to elf_load_addr
> powerpc: Move ima buffer fields to struct kimage
> powerpc: Enable passing IMA log to next kernel on kexec
> powerpc: Move arch independent ima kexec functions to
> drivers/of/kexec.c
> kexec: Use fdt_appendprop_addrrange() to add ima buffer to FDT
> powerpc: Delete unused function delete_fdt_mem_rsv()
> arm64: Enable passing IMA log to next kernel on kexec
>
> Rob Herring (4):
> powerpc: Rename kexec elfcorehdr_addr to elf_load_addr
> of: Add a common kexec FDT setup function
> arm64: Use common of_kexec_alloc_and_setup_fdt()
> powerpc: Use common of_kexec_alloc_and_setup_fdt()
I've applied the series. The merge conflict is not too bad, but will
need a follow on patch to change the powerpc FDT size calculation
(dropping the current FDT's size out).
Thanks for persisting!
Rob
^ permalink raw reply
* Re: [PATCH 1/4] add generic builtin command line
From: Andrew Morton @ 2021-02-17 21:16 UTC (permalink / raw)
To: Daniel Gimpelevich
Cc: Christophe Leroy, Maksym Kokhan, linux-kernel, Rob Herring,
Paul Mackerras, xe-linux-external, Daniel Walker, linuxppc-dev,
Daniel Walker
In-Reply-To: <1613417521.3853.5.camel@chimera>
On Mon, 15 Feb 2021 11:32:01 -0800 Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> wrote:
> On Thu, 2019-03-21 at 15:15 -0700, Andrew Morton wrote:
> > On Thu, 21 Mar 2019 08:13:08 -0700 Daniel Walker <danielwa@cisco.com> wrote:
> > > On Wed, Mar 20, 2019 at 08:14:33PM -0700, Andrew Morton wrote:
> > > > The patches (or some version of them) are already in linux-next,
> > > > which messes me up. I'll disable them for now.
> > >
> > > Those are from my tree, but I remove them when you picked up the series. The
> > > next linux-next should not have them.
> >
> > Yup, thanks, all looks good now.
>
> This patchset is currently neither in mainline nor in -next. May I ask
> what happened to it? Thanks.
Seems that I didn't bring them back after the confict with the powerpc
tree resolved itself.
Please resend everything for -rc1 and let's await the reviewer
feedback,
^ permalink raw reply
* Re: [PATCH v5 10/10] powerpc/signal64: Use __get_user() to copy sigset_t
From: Christopher M. Riedl @ 2021-02-17 19:53 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev
In-Reply-To: <87v9axc78z.fsf@dja-thinkpad.axtens.net>
On Fri Feb 12, 2021 at 3:21 PM CST, Daniel Axtens wrote:
> "Christopher M. Riedl" <cmr@codefail.de> writes:
>
> > Usually sigset_t is exactly 8B which is a "trivial" size and does not
> > warrant using __copy_from_user(). Use __get_user() directly in
> > anticipation of future work to remove the trivial size optimizations
> > from __copy_from_user(). Calling __get_user() also results in a small
> > boost to signal handling throughput here.
>
> Modulo the comments from Christophe, this looks good to me. It seems to
> do what it says on the tin.
>
> Reviewed-by: Daniel Axtens <dja@axtens.net>
>
> Do you know if this patch is responsible for the slightly increase in
> radix performance you observed in your cover letter? The rest of the
> series shouldn't really make things faster than the no-KUAP case...
No, this patch just results in a really small improvement overall.
I looked over this again and I think the reason for the speedup is that
my implementation of unsafe_copy_from_user() in this series calls
__copy_tofrom_user() directly avoiding a barrier_nospec(). This speeds
up all the unsafe_get_from_user() calls in this version.
Skipping the barrier_nospec() like that is incorrect, but Christophe
recently sent a patch which moves barrier_nospec() into the uaccess
allowance helpers. It looks like mpe has already accepted that patch:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2021-February/223959.html
That means that my implementation of unsafe_copy_from_user() is _now_
correct _and_ faster. We do not need to call barrier_nospec() since the
precondition for the 'unsafe' routine is that we called one of the
helpers to open up a uaccess window earlier.
>
> Kind regards,
> Daniel
>
> >
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> > arch/powerpc/kernel/signal_64.c | 14 ++++++++++++--
> > 1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> > index 817b64e1e409..42fdc4a7ff72 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -97,6 +97,14 @@ static void prepare_setup_sigcontext(struct task_struct *tsk, int ctx_has_vsx_re
> > #endif /* CONFIG_VSX */
> > }
> >
> > +static inline int get_user_sigset(sigset_t *dst, const sigset_t *src)
> > +{
> > + if (sizeof(sigset_t) <= 8)
> > + return __get_user(dst->sig[0], &src->sig[0]);
> > + else
> > + return __copy_from_user(dst, src, sizeof(sigset_t));
> > +}
> > +
> > /*
> > * Set up the sigcontext for the signal frame.
> > */
> > @@ -701,8 +709,9 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
> > * We kill the task with a SIGSEGV in this situation.
> > */
> >
> > - if (__copy_from_user(&set, &new_ctx->uc_sigmask, sizeof(set)))
> > + if (get_user_sigset(&set, &new_ctx->uc_sigmask))
> > do_exit(SIGSEGV);
> > +
> > set_current_blocked(&set);
> >
> > if (!user_read_access_begin(new_ctx, ctx_size))
> > @@ -740,8 +749,9 @@ SYSCALL_DEFINE0(rt_sigreturn)
> > if (!access_ok(uc, sizeof(*uc)))
> > goto badframe;
> >
> > - if (__copy_from_user(&set, &uc->uc_sigmask, sizeof(set)))
> > + if (get_user_sigset(&set, &uc->uc_sigmask))
> > goto badframe;
> > +
> > set_current_blocked(&set);
> >
> > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > --
> > 2.26.1
^ permalink raw reply
* Re: [PATCH v5 05/10] powerpc/signal64: Remove TM ifdefery in middle of if/else block
From: Christopher M. Riedl @ 2021-02-17 19:27 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev
In-Reply-To: <874kiheu93.fsf@dja-thinkpad.axtens.net>
On Thu Feb 11, 2021 at 11:21 PM CST, Daniel Axtens wrote:
> Hi Chris,
>
> > Rework the messy ifdef breaking up the if-else for TM similar to
> > commit f1cf4f93de2f ("powerpc/signal32: Remove ifdefery in middle of if/else").
>
> I'm not sure what 'the messy ifdef' and 'the if-else for TM' is (yet):
> perhaps you could start the commit message with a tiny bit of
> background.
Yup good point - I will reword this for the next spin.
>
> > Unlike that commit for ppc32, the ifdef can't be removed entirely since
> > uc_transact in sigframe depends on CONFIG_PPC_TRANSACTIONAL_MEM.
> >
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> > arch/powerpc/kernel/signal_64.c | 16 +++++++---------
> > 1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> > index b211a8ea4f6e..8e1d804ce552 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -710,9 +710,7 @@ SYSCALL_DEFINE0(rt_sigreturn)
> > struct pt_regs *regs = current_pt_regs();
> > struct ucontext __user *uc = (struct ucontext __user *)regs->gpr[1];
> > sigset_t set;
> > -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > unsigned long msr;
> > -#endif
> >
> > /* Always make any pending restarted system calls return -EINTR */
> > current->restart_block.fn = do_no_restart_syscall;
> > @@ -765,7 +763,10 @@ SYSCALL_DEFINE0(rt_sigreturn)
> >
> > if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
> > goto badframe;
> > +#endif
> > +
> > if (MSR_TM_ACTIVE(msr)) {
> > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > /* We recheckpoint on return. */
> > struct ucontext __user *uc_transact;
> >
> > @@ -778,9 +779,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
> > if (restore_tm_sigcontexts(current, &uc->uc_mcontext,
> > &uc_transact->uc_mcontext))
> > goto badframe;
> > - } else
> > #endif
> > - {
> > + } else {
> > /*
> > * Fall through, for non-TM restore
> > *
>
> I think you can get rid of all the ifdefs in _this function_ by
> providing a couple of stubs:
>
> diff --git a/arch/powerpc/kernel/process.c
> b/arch/powerpc/kernel/process.c
> index a66f435dabbf..19059a4b798f 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1120,6 +1120,7 @@ void restore_tm_state(struct pt_regs *regs)
> #else
> #define tm_recheckpoint_new_task(new)
> #define __switch_to_tm(prev, new)
> +void tm_reclaim_current(uint8_t cause) {}
> #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
>
> static inline void save_sprs(struct thread_struct *t)
> diff --git a/arch/powerpc/kernel/signal_64.c
> b/arch/powerpc/kernel/signal_64.c
> index 8e1d804ce552..ed58ca019ad9 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -594,6 +594,13 @@ static long restore_tm_sigcontexts(struct
> task_struct *tsk,
>
> return err;
> }
> +#else
> +static long restore_tm_sigcontexts(struct task_struct *tsk,
> + struct sigcontext __user *sc,
> + struct sigcontext __user *tm_sc)
> +{
> + return -EINVAL;
> +}
> #endif
>
> /*
> @@ -722,7 +729,6 @@ SYSCALL_DEFINE0(rt_sigreturn)
> goto badframe;
> set_current_blocked(&set);
>
> -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> /*
> * If there is a transactional state then throw it away.
> * The purpose of a sigreturn is to destroy all traces of the
> @@ -763,10 +769,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
>
> if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
> goto badframe;
> -#endif
>
> if (MSR_TM_ACTIVE(msr)) {
> -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> /* We recheckpoint on return. */
> struct ucontext __user *uc_transact;
>
> @@ -779,7 +783,6 @@ SYSCALL_DEFINE0(rt_sigreturn)
> if (restore_tm_sigcontexts(current, &uc->uc_mcontext,
> &uc_transact->uc_mcontext))
> goto badframe;
> -#endif
> } else {
> /*
> * Fall through, for non-TM restore
>
> My only concern here was whether it was valid to access
> if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
> if CONFIG_PPC_TRANSACTIONAL_MEM was not defined, but I didn't think of
> any obvious reason why it wouldn't be...
Hmm, we don't really need it for the non-TM case and it is another extra
uaccess. I will take your suggestion to remove the need for the other
ifdefs but might keep this one. Keeping it also makes it very clear this
call is only here for TM and possible to remove in a potentially TM-less
future :)
>
> > @@ -818,10 +818,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> > unsigned long newsp = 0;
> > long err = 0;
> > struct pt_regs *regs = tsk->thread.regs;
> > -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > /* Save the thread's msr before get_tm_stackpointer() changes it */
> > unsigned long msr = regs->msr;
> > -#endif
>
> I wondered if that comment still makes sense in the absence of the
> ifdef. It is preserved in commit f1cf4f93de2f ("powerpc/signal32: Remove
> ifdefery in middle of if/else"), and I can't figure out how you would
> improve it, so I guess it's probably good as it is.
>
> > frame = get_sigframe(ksig, tsk, sizeof(*frame), 0);
> > if (!access_ok(frame, sizeof(*frame)))
> > @@ -836,8 +834,9 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> > /* Create the ucontext. */
> > err |= __put_user(0, &frame->uc.uc_flags);
> > err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]);
> > -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > +
> > if (MSR_TM_ACTIVE(msr)) {
> > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > /* The ucontext_t passed to userland points to the second
> > * ucontext_t (for transactional state) with its uc_link ptr.
> > */
> > @@ -847,9 +846,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> > tsk, ksig->sig, NULL,
> > (unsigned long)ksig->ka.sa.sa_handler,
> > msr);
> > - } else
> > #endif
> > - {
> > + } else {
> > err |= __put_user(0, &frame->uc.uc_link);
> > prepare_setup_sigcontext(tsk, 1);
> > err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
>
> That seems reasonable to me.
Thanks for the feedback!
>
> Kind regards,
> Daniel
^ permalink raw reply
* Re: [PATCH kernel 2/2] powerpc/iommu: Do not immediately panic when failed IOMMU table allocation
From: Leonardo Bras @ 2021-02-17 19:32 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev; +Cc: kvm-ppc, David Gibson
In-Reply-To: <20210216033307.69863-3-aik@ozlabs.ru>
On Tue, 2021-02-16 at 14:33 +1100, Alexey Kardashevskiy wrote:
> Most platforms allocate IOMMU table structures (specifically it_map)
> at the boot time and when this fails - it is a valid reason for panic().
>
> However the powernv platform allocates it_map after a device is returned
> to the host OS after being passed through and this happens long after
> the host OS booted. It is quite possible to trigger the it_map allocation
> panic() and kill the host even though it is not necessary - the host OS
> can still use the DMA bypass mode (requires a tiny fraction of it_map's
> memory) and even if that fails, the host OS is runnnable as it was without
> the device for which allocating it_map causes the panic.
>
> Instead of immediately crashing in a powernv/ioda2 system, this prints
> an error and continues. All other platforms still call panic().
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Hello Alexey,
This looks like a good change, that passes panic() decision to platform
code. Everything looks pretty straightforward, but I have a question
regarding this:
> @@ -1930,16 +1931,16 @@ static long pnv_pci_ioda2_setup_default_config(struct pnv_ioda_pe *pe)
> res_start = pe->phb->ioda.m32_pci_base >> tbl->it_page_shift;
> res_end = min(window_size, SZ_4G) >> tbl->it_page_shift;
> }
> - iommu_init_table(tbl, pe->phb->hose->node, res_start, res_end);
> - rc = pnv_pci_ioda2_set_window(&pe->table_group, 0, tbl);
>
> + if (iommu_init_table(tbl, pe->phb->hose->node, res_start, res_end))
> + rc = pnv_pci_ioda2_set_window(&pe->table_group, 0, tbl);
> + else
> + rc = -ENOMEM;
> if (rc) {
> - pe_err(pe, "Failed to configure 32-bit TCE table, err %ld\n",
> - rc);
> + pe_err(pe, "Failed to configure 32-bit TCE table, err %ld\n", rc);
> iommu_tce_table_put(tbl);
> - return rc;
> + tbl = NULL; /* This clears iommu_table_base below */
> }
> -
> if (!pnv_iommu_bypass_disabled)
> pnv_pci_ioda2_set_bypass(pe, true);
>
>
If I could understand correctly, previously if iommu_init_table() did
not panic(), and pnv_pci_ioda2_set_window() returned something other
than 0, it would return rc in the if (rc) clause, but now it does not
happen anymore, going through if (!pnv_iommu_bypass_disabled) onwards.
Is that desired?
As far as I could see, returning rc there seems a good procedure after
iommu_init_table returning -ENOMEM.
Best regards,
Leonardo Bras
^ permalink raw reply
* Re: [PATCH v5 07/10] powerpc/signal64: Replace restore_sigcontext() w/ unsafe_restore_sigcontext()
From: Christopher M. Riedl @ 2021-02-17 19:32 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev
In-Reply-To: <87y2ftc7el.fsf@dja-thinkpad.axtens.net>
On Fri Feb 12, 2021 at 3:17 PM CST, Daniel Axtens wrote:
> Hi Chris,
>
> > Previously restore_sigcontext() performed a costly KUAP switch on every
> > uaccess operation. These repeated uaccess switches cause a significant
> > drop in signal handling performance.
> >
> > Rewrite restore_sigcontext() to assume that a userspace read access
> > window is open. Replace all uaccess functions with their 'unsafe'
> > versions which avoid the repeated uaccess switches.
> >
>
> Much of the same comments apply here as to the last patch:
> - the commit message might be improved by adding that you are also
> changing the calling functions to open the uaccess window before
> calling into the new unsafe functions
>
> - I have checked that the safe to unsafe conversions look right.
>
> - I think you're opening too wide a window in user_read_access_begin,
> it seems to me that it could be reduced to just the
> uc_mcontext. (Again, not that it makes a difference with the current
> HW.)
Ok, I'll fix these in the next version as well.
>
> Kind regards,
> Daniel
>
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> > arch/powerpc/kernel/signal_64.c | 68 ++++++++++++++++++++-------------
> > 1 file changed, 41 insertions(+), 27 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> > index 4248e4489ff1..d668f8af18fe 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -326,14 +326,14 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
> > /*
> > * Restore the sigcontext from the signal frame.
> > */
> > -
> > -static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> > - struct sigcontext __user *sc)
> > +#define unsafe_restore_sigcontext(tsk, set, sig, sc, e) \
> > + unsafe_op_wrap(__unsafe_restore_sigcontext(tsk, set, sig, sc), e)
> > +static long notrace __unsafe_restore_sigcontext(struct task_struct *tsk, sigset_t *set,
> > + int sig, struct sigcontext __user *sc)
> > {
> > #ifdef CONFIG_ALTIVEC
> > elf_vrreg_t __user *v_regs;
> > #endif
> > - unsigned long err = 0;
> > unsigned long save_r13 = 0;
> > unsigned long msr;
> > struct pt_regs *regs = tsk->thread.regs;
> > @@ -348,27 +348,28 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> > save_r13 = regs->gpr[13];
> >
> > /* copy the GPRs */
> > - err |= __copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr));
> > - err |= __get_user(regs->nip, &sc->gp_regs[PT_NIP]);
> > + unsafe_copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr),
> > + efault_out);
> > + unsafe_get_user(regs->nip, &sc->gp_regs[PT_NIP], efault_out);
> > /* get MSR separately, transfer the LE bit if doing signal return */
> > - err |= __get_user(msr, &sc->gp_regs[PT_MSR]);
> > + unsafe_get_user(msr, &sc->gp_regs[PT_MSR], efault_out);
> > if (sig)
> > regs->msr = (regs->msr & ~MSR_LE) | (msr & MSR_LE);
> > - err |= __get_user(regs->orig_gpr3, &sc->gp_regs[PT_ORIG_R3]);
> > - err |= __get_user(regs->ctr, &sc->gp_regs[PT_CTR]);
> > - err |= __get_user(regs->link, &sc->gp_regs[PT_LNK]);
> > - err |= __get_user(regs->xer, &sc->gp_regs[PT_XER]);
> > - err |= __get_user(regs->ccr, &sc->gp_regs[PT_CCR]);
> > + unsafe_get_user(regs->orig_gpr3, &sc->gp_regs[PT_ORIG_R3], efault_out);
> > + unsafe_get_user(regs->ctr, &sc->gp_regs[PT_CTR], efault_out);
> > + unsafe_get_user(regs->link, &sc->gp_regs[PT_LNK], efault_out);
> > + unsafe_get_user(regs->xer, &sc->gp_regs[PT_XER], efault_out);
> > + unsafe_get_user(regs->ccr, &sc->gp_regs[PT_CCR], efault_out);
> > /* Don't allow userspace to set SOFTE */
> > set_trap_norestart(regs);
> > - err |= __get_user(regs->dar, &sc->gp_regs[PT_DAR]);
> > - err |= __get_user(regs->dsisr, &sc->gp_regs[PT_DSISR]);
> > - err |= __get_user(regs->result, &sc->gp_regs[PT_RESULT]);
> > + unsafe_get_user(regs->dar, &sc->gp_regs[PT_DAR], efault_out);
> > + unsafe_get_user(regs->dsisr, &sc->gp_regs[PT_DSISR], efault_out);
> > + unsafe_get_user(regs->result, &sc->gp_regs[PT_RESULT], efault_out);
> >
> > if (!sig)
> > regs->gpr[13] = save_r13;
> > if (set != NULL)
> > - err |= __get_user(set->sig[0], &sc->oldmask);
> > + unsafe_get_user(set->sig[0], &sc->oldmask, efault_out);
> >
> > /*
> > * Force reload of FP/VEC.
> > @@ -378,29 +379,28 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> > regs->msr &= ~(MSR_FP | MSR_FE0 | MSR_FE1 | MSR_VEC | MSR_VSX);
> >
> > #ifdef CONFIG_ALTIVEC
> > - err |= __get_user(v_regs, &sc->v_regs);
> > - if (err)
> > - return err;
> > + unsafe_get_user(v_regs, &sc->v_regs, efault_out);
> > if (v_regs && !access_ok(v_regs, 34 * sizeof(vector128)))
> > return -EFAULT;
> > /* Copy 33 vec registers (vr0..31 and vscr) from the stack */
> > if (v_regs != NULL && (msr & MSR_VEC) != 0) {
> > - err |= __copy_from_user(&tsk->thread.vr_state, v_regs,
> > - 33 * sizeof(vector128));
> > + unsafe_copy_from_user(&tsk->thread.vr_state, v_regs,
> > + 33 * sizeof(vector128), efault_out);
> > tsk->thread.used_vr = true;
> > } else if (tsk->thread.used_vr) {
> > memset(&tsk->thread.vr_state, 0, 33 * sizeof(vector128));
> > }
> > /* Always get VRSAVE back */
> > if (v_regs != NULL)
> > - err |= __get_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
> > + unsafe_get_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33],
> > + efault_out);
> > else
> > tsk->thread.vrsave = 0;
> > if (cpu_has_feature(CPU_FTR_ALTIVEC))
> > mtspr(SPRN_VRSAVE, tsk->thread.vrsave);
> > #endif /* CONFIG_ALTIVEC */
> > /* restore floating point */
> > - err |= copy_fpr_from_user(tsk, &sc->fp_regs);
> > + unsafe_copy_fpr_from_user(tsk, &sc->fp_regs, efault_out);
> > #ifdef CONFIG_VSX
> > /*
> > * Get additional VSX data. Update v_regs to point after the
> > @@ -409,14 +409,17 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> > */
> > v_regs += ELF_NVRREG;
> > if ((msr & MSR_VSX) != 0) {
> > - err |= copy_vsx_from_user(tsk, v_regs);
> > + unsafe_copy_vsx_from_user(tsk, v_regs, efault_out);
> > tsk->thread.used_vsr = true;
> > } else {
> > for (i = 0; i < 32 ; i++)
> > tsk->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = 0;
> > }
> > #endif
> > - return err;
> > + return 0;
> > +
> > +efault_out:
> > + return -EFAULT;
> > }
> >
> > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > @@ -701,8 +704,14 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
> > if (__copy_from_user(&set, &new_ctx->uc_sigmask, sizeof(set)))
> > do_exit(SIGSEGV);
> > set_current_blocked(&set);
> > - if (restore_sigcontext(current, NULL, 0, &new_ctx->uc_mcontext))
> > +
> > + if (!user_read_access_begin(new_ctx, ctx_size))
> > + return -EFAULT;
> > + if (__unsafe_restore_sigcontext(current, NULL, 0, &new_ctx->uc_mcontext)) {
> > + user_read_access_end();
> > do_exit(SIGSEGV);
> > + }
> > + user_read_access_end();
> >
> > /* This returns like rt_sigreturn */
> > set_thread_flag(TIF_RESTOREALL);
> > @@ -807,8 +816,13 @@ SYSCALL_DEFINE0(rt_sigreturn)
> > * causing a TM bad thing.
> > */
> > current->thread.regs->msr &= ~MSR_TS_MASK;
> > - if (restore_sigcontext(current, NULL, 1, &uc->uc_mcontext))
> > + if (!user_read_access_begin(uc, sizeof(*uc)))
> > + return -EFAULT;
> > + if (__unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext)) {
> > + user_read_access_end();
> > goto badframe;
> > + }
> > + user_read_access_end();
> > }
> >
> > if (restore_altstack(&uc->uc_stack))
> > --
> > 2.26.1
^ 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