* Re: [PATCH] kdb: use correct pointer when 'btc' calls 'btt'
From: Christophe LEROY @ 2018-09-27 11:30 UTC (permalink / raw)
To: Michael Ellerman, Daniel Thompson
Cc: Jason Wessel, linux-kernel, linuxppc-dev, kgdb-bugreport, stable,
me
In-Reply-To: <877ej7urh3.fsf@concordia.ellerman.id.au>
Le 27/09/2018 à 13:09, Michael Ellerman a écrit :
> Christophe LEROY <christophe.leroy@c-s.fr> writes:
>> Le 26/09/2018 à 13:11, Daniel Thompson a écrit :
>>> On 16/09/2018 20:06, Daniel Thompson wrote:
>>>> On Fri, Sep 14, 2018 at 12:35:44PM +0000, Christophe Leroy wrote:
>>>>> On a powerpc 8xx, 'btc' fails as follows:
>>>>> Entering kdb (current=0x(ptrval), pid 282) due to Keyboard Entry
> ...
>>>>>
>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>>> Cc: <stable@vger.kernel.org> # 4.15+
>>>>
>>>> Would a Fixes: be better here?
>>>> Fixes: ad67b74d2469d9b82 ("printk: hash addresses printed with %p")
>>>
>>> Christophe, When you add the Fixes: could you also add my
>>>
>>> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
>>
>> Ok, thanks for the review, but do I have to do anything really ?
>>
>> The Fixes: and now your Reviewed-by: appear automatically in patchwork
>> (https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=65715),
>> so I believe they'll be automatically included when Jason or someone
>> else takes the patch, no ?
>
> patchwork won't add the Fixes tag from the reply, it needs to be in the
> original mail.
>
> See:
> https://github.com/getpatchwork/patchwork/issues/151
>
Ok, so it accounts it and adds a '1' in the F column in the patches
list, but won't take it into account.
Then I'll send a v2 with revised commit text.
Christophe
^ permalink raw reply
* Re: [PATCH v4 1/2] powerpc/32: add stack protector support
From: Christophe LEROY @ 2018-09-27 11:51 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
linuxppc-dev, linux-kernel
In-Reply-To: <20180927074525.GQ23155@gate.crashing.org>
Le 27/09/2018 à 09:45, Segher Boessenkool a écrit :
> On Thu, Sep 27, 2018 at 08:20:00AM +0200, Christophe LEROY wrote:
>> Le 26/09/2018 à 21:16, Segher Boessenkool a écrit :
>>> On Wed, Sep 26, 2018 at 11:40:38AM +0000, Christophe Leroy wrote:
>>>> +static __always_inline void boot_init_stack_canary(void)
>>>> +{
>>>> + unsigned long canary;
>>>> +
>>>> + /* Try to get a semi random initial value. */
>>>> + get_random_bytes(&canary, sizeof(canary));
>>>> + canary ^= mftb();
>>>> + canary ^= LINUX_VERSION_CODE;
>>>> +
>>>> + current->stack_canary = canary;
>>>> +}
>>>
>>> I still think you should wait until there is entropy available. You
>>> haven't answered my questions about that (or I didn't see them): what
>>> does the kernel do in other similar cases?
>>>
>>> Looks great otherwise!
>>
>> What do you mean by 'other similar cases' ? All arches have similar
>> boot_init_stack_canary().
>
> Yes, those, and other things that want entropy early.
Functions add_latent_entropy() and add_device_randomness() are
there for that it seems, allthough it is not real entropy.
>
>> x86 uses rdtsc() which is equivalent to our
>> mftb(). Most arches xor it with LINUX_VERSION_CODE.
>>
>> The issue is that it is called very early in start_kernel(), however
>> they try to set some entropy anyway:
>>
>> boot_cpu_init();
>> page_address_init();
>> pr_notice("%s", linux_banner);
>> setup_arch(&command_line);
>> /*
>> * Set up the the initial canary and entropy after arch
>> * and after adding latent and command line entropy.
>> */
>> add_latent_entropy();
>> add_device_randomness(command_line, strlen(command_line));
>> boot_init_stack_canary();
>>
>> Apparently, it is too early for calling wait_for_random_bytes(), see below.
>
> Hrm. Too early to call wait_event_interruptible? From there it went
> into schedule(), which blew up. Well you say we have only one context
> at this point, so that is not too surprising then :-)
>
>> However this is the canary for initial startup only. Only idle() still
>> uses this canary once the system is running. A new canary is set for any
>> new forked task.
>
> Ah, that makes things a lot better! Do those new tasks get a canary
> from something with sufficient entropy though?
For the kernel threads that are started early, probably not. For the
ones started a bit later, and for user processes, I believe they have
better entropy. Anyway, all this is handled by the kernel core and is
out of control of individual arches, as its done in kernel/fork.c in
function dup_task_struct(). However this function is declared as
static __latent_entropy struct task_struct *copy_process(). This
__latent_entropy attibute must help in a way.
>
>> Maybe should the idle canary be updated later once there is more entropy
>
> That is tricky to do, but sure, if you can, that should help.
>
>> ? Today there is a new call to boot_init_stack_canary() in
>> cpu_startup_entry(), but it is enclosed inside #ifdef CONFIG_X86.
>
> It needs to know the details of how ssp works on each platform.
Well, that could be for another patch in the future. That's probably
feasible on x86 and PPC because they both use TLS guard, but not for
other arches where the guard is global at the moment. So we'll have to
do it carefully.
I agree with you that we may not for the time being get all the expected
security against hackers out of it due to that little entropy, but my
main concern at the time being is to deter developper's bugs clobbering
the stack, and for that any non trivial canary should make it, shouldn't
it ?
Christophe
^ permalink raw reply
* Re: [PATCH -next] PCI: hotplug: Use kmemdup rather than duplicating its implementation in pnv_php_add_devtree()
From: Michael Ellerman @ 2018-09-27 12:28 UTC (permalink / raw)
To: YueHaibing, Benjamin Herrenschmidt, Paul Mackerras, Bjorn Helgaas
Cc: YueHaibing, linuxppc-dev, linux-pci, kernel-janitors
In-Reply-To: <1538031141-138417-1-git-send-email-yuehaibing@huawei.com>
YueHaibing <yuehaibing@huawei.com> writes:
> Use kmemdup rather than duplicating its implementation
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> drivers/pci/hotplug/pnv_php.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
> index 5070620..ee54f5b 100644
> --- a/drivers/pci/hotplug/pnv_php.c
> +++ b/drivers/pci/hotplug/pnv_php.c
> @@ -275,14 +275,13 @@ static int pnv_php_add_devtree(struct pnv_php_slot *php_slot)
> goto free_fdt1;
> }
>
> - fdt = kzalloc(fdt_totalsize(fdt1), GFP_KERNEL);
> + fdt = kmemdup(fdt1, fdt_totalsize(fdt1), GFP_KERNEL);
> if (!fdt) {
> ret = -ENOMEM;
> goto free_fdt1;
> }
>
> /* Unflatten device tree blob */
> - memcpy(fdt, fdt1, fdt_totalsize(fdt1));
> dt = of_fdt_unflatten_tree(fdt, php_slot->dn, NULL);
> if (!dt) {
> ret = -EINVAL;
Looks correct to me.
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
cheers
^ permalink raw reply
* Re: [PATCH v3 1/9] powerpc: build .dtb files in dts directory
From: Rob Herring @ 2018-09-27 13:39 UTC (permalink / raw)
To: devicetree, linux-kernel@vger.kernel.org, Masahiro Yamada
Cc: Frank Rowand, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, linuxppc-dev
In-Reply-To: <20180910150403.19476-2-robh@kernel.org>
On Mon, Sep 10, 2018 at 10:04 AM Rob Herring <robh@kernel.org> wrote:
>
> Align powerpc with other architectures which build the dtb files in the
> same directory as the dts files. This is also in line with most other
> build targets which are located in the same directory as the source.
> This move will help enable the 'dtbs' target which builds all the dtbs
> regardless of kernel config.
>
> This transition could break some scripts if they expect dtb files in the
> old location.
>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v3:
> - Remove duplicate mpc5200 dtbs from image-y targets. The dtb target already
> comes from the cuImage.<platform> target.
Do the PPC folks have any comments on this?
Rob
>
> arch/powerpc/Makefile | 2 +-
> arch/powerpc/boot/Makefile | 55 ++++++++++++++++------------------
> arch/powerpc/boot/dts/Makefile | 1 +
> 3 files changed, 28 insertions(+), 30 deletions(-)
> create mode 100644 arch/powerpc/boot/dts/Makefile
>
> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> index 11a1acba164a..53ea887eb34e 100644
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -294,7 +294,7 @@ bootwrapper_install:
> $(Q)$(MAKE) $(build)=$(boot) $(patsubst %,$(boot)/%,$@)
>
> %.dtb: scripts
> - $(Q)$(MAKE) $(build)=$(boot) $(patsubst %,$(boot)/%,$@)
> + $(Q)$(MAKE) $(build)=$(boot)/dts $(patsubst %,$(boot)/dts/%,$@)
>
> # Used to create 'merged defconfigs'
> # To use it $(call) it with the first argument as the base defconfig
> diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
> index 0fb96c26136f..bca5c23767df 100644
> --- a/arch/powerpc/boot/Makefile
> +++ b/arch/powerpc/boot/Makefile
> @@ -304,9 +304,9 @@ image-$(CONFIG_PPC_ADDER875) += cuImage.adder875-uboot \
> dtbImage.adder875-redboot
>
> # Board ports in arch/powerpc/platform/52xx/Kconfig
> -image-$(CONFIG_PPC_LITE5200) += cuImage.lite5200 lite5200.dtb
> -image-$(CONFIG_PPC_LITE5200) += cuImage.lite5200b lite5200b.dtb
> -image-$(CONFIG_PPC_MEDIA5200) += cuImage.media5200 media5200.dtb
> +image-$(CONFIG_PPC_LITE5200) += cuImage.lite5200
> +image-$(CONFIG_PPC_LITE5200) += cuImage.lite5200b
> +image-$(CONFIG_PPC_MEDIA5200) += cuImage.media5200
>
> # Board ports in arch/powerpc/platform/82xx/Kconfig
> image-$(CONFIG_MPC8272_ADS) += cuImage.mpc8272ads
> @@ -381,11 +381,11 @@ $(addprefix $(obj)/, $(sort $(filter zImage.%, $(image-y)))): vmlinux $(wrapperb
> $(call if_changed,wrap,$(subst $(obj)/zImage.,,$@))
>
> # dtbImage% - a dtbImage is a zImage with an embedded device tree blob
> -$(obj)/dtbImage.initrd.%: vmlinux $(wrapperbits) $(obj)/%.dtb FORCE
> - $(call if_changed,wrap,$*,,$(obj)/$*.dtb,$(obj)/ramdisk.image.gz)
> +$(obj)/dtbImage.initrd.%: vmlinux $(wrapperbits) $(obj)/dts/%.dtb FORCE
> + $(call if_changed,wrap,$*,,$(obj)/dts/$*.dtb,$(obj)/ramdisk.image.gz)
>
> -$(obj)/dtbImage.%: vmlinux $(wrapperbits) $(obj)/%.dtb FORCE
> - $(call if_changed,wrap,$*,,$(obj)/$*.dtb)
> +$(obj)/dtbImage.%: vmlinux $(wrapperbits) $(obj)/dts/%.dtb FORCE
> + $(call if_changed,wrap,$*,,$(obj)/dts/$*.dtb)
>
> # This cannot be in the root of $(src) as the zImage rule always adds a $(obj)
> # prefix
> @@ -395,36 +395,33 @@ $(obj)/vmlinux.strip: vmlinux
> $(obj)/uImage: vmlinux $(wrapperbits) FORCE
> $(call if_changed,wrap,uboot)
>
> -$(obj)/uImage.initrd.%: vmlinux $(obj)/%.dtb $(wrapperbits) FORCE
> - $(call if_changed,wrap,uboot-$*,,$(obj)/$*.dtb,$(obj)/ramdisk.image.gz)
> +$(obj)/uImage.initrd.%: vmlinux $(obj)/dts/%.dtb $(wrapperbits) FORCE
> + $(call if_changed,wrap,uboot-$*,,$(obj)/dts/$*.dtb,$(obj)/ramdisk.image.gz)
>
> -$(obj)/uImage.%: vmlinux $(obj)/%.dtb $(wrapperbits) FORCE
> - $(call if_changed,wrap,uboot-$*,,$(obj)/$*.dtb)
> +$(obj)/uImage.%: vmlinux $(obj)/dts/%.dtb $(wrapperbits) FORCE
> + $(call if_changed,wrap,uboot-$*,,$(obj)/dts/$*.dtb)
>
> -$(obj)/cuImage.initrd.%: vmlinux $(obj)/%.dtb $(wrapperbits) FORCE
> - $(call if_changed,wrap,cuboot-$*,,$(obj)/$*.dtb,$(obj)/ramdisk.image.gz)
> +$(obj)/cuImage.initrd.%: vmlinux $(obj)/dts/%.dtb $(wrapperbits) FORCE
> + $(call if_changed,wrap,cuboot-$*,,$(obj)/dts/$*.dtb,$(obj)/ramdisk.image.gz)
>
> -$(obj)/cuImage.%: vmlinux $(obj)/%.dtb $(wrapperbits) FORCE
> - $(call if_changed,wrap,cuboot-$*,,$(obj)/$*.dtb)
> +$(obj)/cuImage.%: vmlinux $(obj)/dts/%.dtb $(wrapperbits) FORCE
> + $(call if_changed,wrap,cuboot-$*,,$(obj)/dts/$*.dtb)
>
> -$(obj)/simpleImage.initrd.%: vmlinux $(obj)/%.dtb $(wrapperbits) FORCE
> - $(call if_changed,wrap,simpleboot-$*,,$(obj)/$*.dtb,$(obj)/ramdisk.image.gz)
> +$(obj)/simpleImage.initrd.%: vmlinux $(obj)/dts/%.dtb $(wrapperbits) FORCE
> + $(call if_changed,wrap,simpleboot-$*,,$(obj)/dts/$*.dtb,$(obj)/ramdisk.image.gz)
>
> -$(obj)/simpleImage.%: vmlinux $(obj)/%.dtb $(wrapperbits) FORCE
> - $(call if_changed,wrap,simpleboot-$*,,$(obj)/$*.dtb)
> +$(obj)/simpleImage.%: vmlinux $(obj)/dts/%.dtb $(wrapperbits) FORCE
> + $(call if_changed,wrap,simpleboot-$*,,$(obj)/dts/$*.dtb)
>
> -$(obj)/treeImage.initrd.%: vmlinux $(obj)/%.dtb $(wrapperbits) FORCE
> - $(call if_changed,wrap,treeboot-$*,,$(obj)/$*.dtb,$(obj)/ramdisk.image.gz)
> +$(obj)/treeImage.initrd.%: vmlinux $(obj)/dts/%.dtb $(wrapperbits) FORCE
> + $(call if_changed,wrap,treeboot-$*,,$(obj)/dts/$*.dtb,$(obj)/ramdisk.image.gz)
>
> -$(obj)/treeImage.%: vmlinux $(obj)/%.dtb $(wrapperbits) FORCE
> - $(call if_changed,wrap,treeboot-$*,,$(obj)/$*.dtb)
> +$(obj)/treeImage.%: vmlinux $(obj)/dts/%.dtb $(wrapperbits) FORCE
> + $(call if_changed,wrap,treeboot-$*,,$(obj)/dts/$*.dtb)
>
> -# Rule to build device tree blobs
> -$(obj)/%.dtb: $(src)/dts/%.dts FORCE
> - $(call if_changed_dep,dtc)
> -
> -$(obj)/%.dtb: $(src)/dts/fsl/%.dts FORCE
> - $(call if_changed_dep,dtc)
> +# Needed for the above targets to work with dts/fsl/ files
> +$(obj)/dts/%.dtb: $(obj)/dts/fsl/%.dtb
> + @cp $< $@
>
> # If there isn't a platform selected then just strip the vmlinux.
> ifeq (,$(image-y))
> diff --git a/arch/powerpc/boot/dts/Makefile b/arch/powerpc/boot/dts/Makefile
> new file mode 100644
> index 000000000000..f66554cd5c45
> --- /dev/null
> +++ b/arch/powerpc/boot/dts/Makefile
> @@ -0,0 +1 @@
> +# SPDX-License-Identifier: GPL-2.0
> --
> 2.17.1
^ permalink raw reply
* Re: [PATCH 3/5] dma-direct: refine dma_direct_alloc zone selection
From: Christoph Hellwig @ 2018-09-27 13:49 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Christoph Hellwig, iommu, Marek Szyprowski, Robin Murphy,
Greg Kroah-Hartman, linux-kernel, linuxppc-dev
In-Reply-To: <1811156d5a1df1166c7ab7522525619b951f047d.camel@kernel.crashing.org>
On Thu, Sep 27, 2018 at 11:45:15AM +1000, Benjamin Herrenschmidt wrote:
> I'm not sure this is entirely right.
>
> Let's say the mask is 30 bits. You will return GFP_DMA32, which will
> fail if you allocate something above 1G (which is legit for
> ZONE_DMA32).
And then we will try GFP_DMA further down in the function:
if (IS_ENABLED(CONFIG_ZONE_DMA) &&
dev->coherent_dma_mask < DMA_BIT_MASK(32) &&
!(gfp & GFP_DMA)) {
gfp = (gfp & ~GFP_DMA32) | GFP_DMA;
goto again;
}
This is and old optimization from x86, because chances are high that
GFP_DMA32 will give you suitable memory for the infamous 31-bit
dma mask devices (at least at boot time) and thus we don't have
to deplete the tiny ZONE_DMA pool.
^ permalink raw reply
* Re: [PATCH 5/5] dma-direct: always allow dma mask <= physiscal memory size
From: Christoph Hellwig @ 2018-09-27 13:49 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Christoph Hellwig, iommu, Marek Szyprowski, Robin Murphy,
Greg Kroah-Hartman, linux-kernel, linuxppc-dev
In-Reply-To: <985079efc3296ac45de1e6344d7916c42fc7cbdd.camel@kernel.crashing.org>
On Thu, Sep 27, 2018 at 11:50:14AM +1000, Benjamin Herrenschmidt wrote:
> > - * to be able to satisfy them - either by not supporting more physical
> > - * memory, or by providing a ZONE_DMA32. If neither is the case, the
> > - * architecture needs to use an IOMMU instead of the direct mapping.
> > - */
> > - if (mask < phys_to_dma(dev, DMA_BIT_MASK(32)))
> > + u64 min_mask;
> > +
> > + if (IS_ENABLED(CONFIG_ZONE_DMA))
> > + min_mask = DMA_BIT_MASK(ARCH_ZONE_DMA_BITS);
> > + else
> > + min_mask = min_t(u64, DMA_BIT_MASK(32),
> > + (max_pfn - 1) << PAGE_SHIFT);
> > +
> > + if (mask >= phys_to_dma(dev, min_mask))
> > return 0;
>
> nitpick ... to be completely "correct", I would have written
>
> if (IS_ENABLED(CONFIG_ZONE_DMA))
> min_mask = DMA_BIT_MASK(ARCH_ZONE_DMA_BITS);
> else
> min_mask = DMA_BIT_MASK(32);
>
> min_mask = min_t(u64, min_mask, (max_pfn - 1) << PAGE_SHIFT);
>
> In "theory" it's also ok to have a mask < ZONE_DMA_BITS as long as it's
> big enough to fit all memory :-)
Yeah, we could do that.
^ permalink raw reply
* Re: [PATCH 2/5] dma-direct: add an explicit dma_direct_get_required_mask
From: Robin Murphy @ 2018-09-27 14:12 UTC (permalink / raw)
To: Christoph Hellwig, iommu
Cc: Marek Szyprowski, Benjamin Herrenschmidt, Greg Kroah-Hartman,
linux-kernel, linuxppc-dev
In-Reply-To: <20180920185247.20037-3-hch@lst.de>
On 20/09/18 19:52, Christoph Hellwig wrote:
> This is somewhat modelled after the powerpc version, and differs from
> the legacy fallback in use fls64 instead of pointlessly splitting up the
> address into low and high dwords and in that it takes (__)phys_to_dma
> into account.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> include/linux/dma-direct.h | 1 +
> kernel/dma/direct.c | 21 ++++++++++++++++++---
> 2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
> index 86a59ba5a7f3..b79496d8c75b 100644
> --- a/include/linux/dma-direct.h
> +++ b/include/linux/dma-direct.h
> @@ -55,6 +55,7 @@ static inline void dma_mark_clean(void *addr, size_t size)
> }
> #endif /* CONFIG_ARCH_HAS_DMA_MARK_CLEAN */
>
> +u64 dma_direct_get_required_mask(struct device *dev);
> void *dma_direct_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle,
> gfp_t gfp, unsigned long attrs);
> void dma_direct_free(struct device *dev, size_t size, void *cpu_addr,
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index c954f0a6dc62..81b73a5bba54 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -53,11 +53,25 @@ check_addr(struct device *dev, dma_addr_t dma_addr, size_t size,
> return true;
> }
>
> +static inline dma_addr_t phys_to_dma_direct(struct device *dev,
> + phys_addr_t phys)
> +{
> + if (force_dma_unencrypted())
> + return __phys_to_dma(dev, phys);
> + return phys_to_dma(dev, phys);
> +}
> +
> +u64 dma_direct_get_required_mask(struct device *dev)
> +{
> + u64 max_dma = phys_to_dma_direct(dev, (max_pfn - 1) << PAGE_SHIFT);
> +
> + return (1ULL << (fls64(max_dma) - 1)) * 2 - 1;
I think that may as well just use __fls64() - it seems reasonable to
assume max_dma > 0. Otherwise,
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> +}
> +
> static bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
> {
> - dma_addr_t addr = force_dma_unencrypted() ?
> - __phys_to_dma(dev, phys) : phys_to_dma(dev, phys);
> - return addr + size - 1 <= dev->coherent_dma_mask;
> + return phys_to_dma_direct(dev, phys) + size - 1 <=
> + dev->coherent_dma_mask;
> }
>
> void *dma_direct_alloc_pages(struct device *dev, size_t size,
> @@ -296,6 +310,7 @@ const struct dma_map_ops dma_direct_ops = {
> .unmap_page = dma_direct_unmap_page,
> .unmap_sg = dma_direct_unmap_sg,
> #endif
> + .get_required_mask = dma_direct_get_required_mask,
> .dma_supported = dma_direct_supported,
> .mapping_error = dma_direct_mapping_error,
> .cache_sync = arch_dma_cache_sync,
>
^ permalink raw reply
* Re: [PATCH 3/5] dma-direct: refine dma_direct_alloc zone selection
From: Robin Murphy @ 2018-09-27 14:30 UTC (permalink / raw)
To: Christoph Hellwig, iommu
Cc: Marek Szyprowski, Benjamin Herrenschmidt, Greg Kroah-Hartman,
linux-kernel, linuxppc-dev
In-Reply-To: <20180920185247.20037-4-hch@lst.de>
On 20/09/18 19:52, Christoph Hellwig wrote:
> We need to take the DMA offset and encryption bit into account when
> selecting a zone. User the opportunity to factor out the zone
> selection into a helper for reuse.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> kernel/dma/direct.c | 31 +++++++++++++++++++++----------
> 1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 81b73a5bba54..3c404e33d946 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -68,6 +68,22 @@ u64 dma_direct_get_required_mask(struct device *dev)
> return (1ULL << (fls64(max_dma) - 1)) * 2 - 1;
> }
>
> +static gfp_t __dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask,
> + u64 *phys_mask)
> +{
> + if (force_dma_unencrypted())
> + *phys_mask = __dma_to_phys(dev, dma_mask);
> + else
> + *phys_mask = dma_to_phys(dev, dma_mask);
Maybe make phys_to_dma_direct() take u64 instead of phys_addr_t so we
can reuse it here?
> + /* GFP_DMA32 and GFP_DMA are no ops without the corresponding zones: */
> + if (*phys_mask <= DMA_BIT_MASK(ARCH_ZONE_DMA_BITS))
> + return GFP_DMA;
If we don't have ZONE_DMA it would in theory be a tiny bit better to
fall back to GFP_DMA32 instead of returning 0 here, but I'm not sure
it's worth the bother.
> + if (*phys_mask <= DMA_BIT_MASK(32))
> + return GFP_DMA32;
> + return 0;
> +}
> +
> static bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
> {
> return phys_to_dma_direct(dev, phys) + size - 1 <=
> @@ -80,17 +96,13 @@ void *dma_direct_alloc_pages(struct device *dev, size_t size,
> unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> int page_order = get_order(size);
> struct page *page = NULL;
> + u64 phys_mask;
> void *ret;
>
> /* we always manually zero the memory once we are done: */
> gfp &= ~__GFP_ZERO;
> -
> - /* GFP_DMA32 and GFP_DMA are no ops without the corresponding zones: */
> - if (dev->coherent_dma_mask <= DMA_BIT_MASK(ARCH_ZONE_DMA_BITS))
> - gfp |= GFP_DMA;
> - if (dev->coherent_dma_mask <= DMA_BIT_MASK(32) && !(gfp & GFP_DMA))
> - gfp |= GFP_DMA32;
> -
> + gfp |= __dma_direct_optimal_gfp_mask(dev, dev->coherent_dma_mask,
> + &phys_mask);
> again:
> /* CMA can be used only in the context which permits sleeping */
> if (gfpflags_allow_blocking(gfp)) {
> @@ -109,15 +121,14 @@ void *dma_direct_alloc_pages(struct device *dev, size_t size,
> page = NULL;
>
> if (IS_ENABLED(CONFIG_ZONE_DMA32) &&
> - dev->coherent_dma_mask < DMA_BIT_MASK(64) &&
> + phys_mask < DMA_BIT_MASK(64) &&
> !(gfp & (GFP_DMA32 | GFP_DMA))) {
> gfp |= GFP_DMA32;
> goto again;
Ah right, in that situation we should probably end up here anyway, so
that's good enough - definitely not worth any more #ifdeffery above.
Nits aside,
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> }
>
> if (IS_ENABLED(CONFIG_ZONE_DMA) &&
> - dev->coherent_dma_mask < DMA_BIT_MASK(32) &&
> - !(gfp & GFP_DMA)) {
> + phys_mask < DMA_BIT_MASK(32) && !(gfp & GFP_DMA)) {
> gfp = (gfp & ~GFP_DMA32) | GFP_DMA;
> goto again;
> }
>
^ permalink raw reply
* Re: [PATCH 4/5] dma-direct: implement complete bus_dma_mask handling
From: Robin Murphy @ 2018-09-27 14:58 UTC (permalink / raw)
To: Christoph Hellwig, iommu
Cc: Marek Szyprowski, Benjamin Herrenschmidt, Greg Kroah-Hartman,
linux-kernel, linuxppc-dev
In-Reply-To: <20180920185247.20037-5-hch@lst.de>
On 20/09/18 19:52, Christoph Hellwig wrote:
> Instead of rejecting devices with a too small bus_dma_mask we can handle
> by taking the bus dma_mask into account for allocations and bounce
> buffering decisions.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> include/linux/dma-direct.h | 3 ++-
> kernel/dma/direct.c | 21 +++++++++++----------
> 2 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
> index b79496d8c75b..fbca184ff5a0 100644
> --- a/include/linux/dma-direct.h
> +++ b/include/linux/dma-direct.h
> @@ -27,7 +27,8 @@ static inline bool dma_capable(struct device *dev, dma_addr_t addr, size_t size)
> if (!dev->dma_mask)
> return false;
>
> - return addr + size - 1 <= *dev->dma_mask;
> + return addr + size - 1 <=
> + min_not_zero(*dev->dma_mask, dev->bus_dma_mask);
> }
> #endif /* !CONFIG_ARCH_HAS_PHYS_TO_DMA */
>
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 3c404e33d946..64466b7ef67b 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -43,10 +43,11 @@ check_addr(struct device *dev, dma_addr_t dma_addr, size_t size,
> return false;
> }
>
> - if (*dev->dma_mask >= DMA_BIT_MASK(32)) {
> + if (*dev->dma_mask >= DMA_BIT_MASK(32) || dev->bus_dma_mask) {
Hmm... say *dev->dma_mask is 31 bits and dev->bus_dma_mask is 40 bits
due to a global DT property, we'll now scream where we didn't before
even though the bus mask is almost certainly irrelevant - is that desirable?
> dev_err(dev,
> - "%s: overflow %pad+%zu of device mask %llx\n",
> - caller, &dma_addr, size, *dev->dma_mask);
> + "%s: overflow %pad+%zu of device mask %llx bus mask %llx\n",
> + caller, &dma_addr, size,
> + *dev->dma_mask, dev->bus_dma_mask);
> }
> return false;
> }
> @@ -65,12 +66,18 @@ u64 dma_direct_get_required_mask(struct device *dev)
> {
> u64 max_dma = phys_to_dma_direct(dev, (max_pfn - 1) << PAGE_SHIFT);
>
> + if (dev->bus_dma_mask && dev->bus_dma_mask < max_dma)
> + max_dma = dev->bus_dma_mask;
Again, I think we could just do another min_not_zero() here. A device
wired to address only one single page of RAM isn't a realistic prospect
(and we could just flip the -1 and the shift in the max_dma calculation
if we *really* wanted to support such things).
> +
> return (1ULL << (fls64(max_dma) - 1)) * 2 - 1;
> }
>
> static gfp_t __dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask,
> u64 *phys_mask)
> {
> + if (dev->bus_dma_mask && dev->bus_dma_mask < dma_mask)
> + dma_mask = dev->bus_dma_mask;
> +
Similarly, can't we assume dma_mask to be nonzero here too? It feels
like we really shouldn't have managed to get this far without one.
Robin.
> if (force_dma_unencrypted())
> *phys_mask = __dma_to_phys(dev, dma_mask);
> else
> @@ -87,7 +94,7 @@ static gfp_t __dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask,
> static bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
> {
> return phys_to_dma_direct(dev, phys) + size - 1 <=
> - dev->coherent_dma_mask;
> + min_not_zero(dev->coherent_dma_mask, dev->bus_dma_mask);
> }
>
> void *dma_direct_alloc_pages(struct device *dev, size_t size,
> @@ -291,12 +298,6 @@ int dma_direct_supported(struct device *dev, u64 mask)
> if (mask < phys_to_dma(dev, DMA_BIT_MASK(32)))
> return 0;
> #endif
> - /*
> - * Upstream PCI/PCIe bridges or SoC interconnects may not carry
> - * as many DMA address bits as the device itself supports.
> - */
> - if (dev->bus_dma_mask && mask > dev->bus_dma_mask)
> - return 0;
> return 1;
> }
>
>
^ permalink raw reply
* Re: [PATCH 5/5] dma-direct: always allow dma mask <= physiscal memory size
From: Robin Murphy @ 2018-09-27 15:07 UTC (permalink / raw)
To: Christoph Hellwig, Benjamin Herrenschmidt
Cc: iommu, Marek Szyprowski, Greg Kroah-Hartman, linux-kernel,
linuxppc-dev
In-Reply-To: <20180927134954.GB8281@lst.de>
[ oops, I should have looked at the replies first, now I see Ben already
had the same thing to say about #3... ]
On 27/09/18 14:49, Christoph Hellwig wrote:
> On Thu, Sep 27, 2018 at 11:50:14AM +1000, Benjamin Herrenschmidt wrote:
>>> - * to be able to satisfy them - either by not supporting more physical
>>> - * memory, or by providing a ZONE_DMA32. If neither is the case, the
>>> - * architecture needs to use an IOMMU instead of the direct mapping.
>>> - */
>>> - if (mask < phys_to_dma(dev, DMA_BIT_MASK(32)))
>>> + u64 min_mask;
>>> +
>>> + if (IS_ENABLED(CONFIG_ZONE_DMA))
>>> + min_mask = DMA_BIT_MASK(ARCH_ZONE_DMA_BITS);
>>> + else
>>> + min_mask = min_t(u64, DMA_BIT_MASK(32),
>>> + (max_pfn - 1) << PAGE_SHIFT);
>>> +
>>> + if (mask >= phys_to_dma(dev, min_mask))
>>> return 0;
>>
>> nitpick ... to be completely "correct", I would have written
>>
>> if (IS_ENABLED(CONFIG_ZONE_DMA))
>> min_mask = DMA_BIT_MASK(ARCH_ZONE_DMA_BITS);
>> else
>> min_mask = DMA_BIT_MASK(32);
>>
>> min_mask = min_t(u64, min_mask, (max_pfn - 1) << PAGE_SHIFT);
>>
>> In "theory" it's also ok to have a mask < ZONE_DMA_BITS as long as it's
>> big enough to fit all memory :-)
>
> Yeah, we could do that.
FWIW I like it even if just for looking slightly more readable. With
that fixup,
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
^ permalink raw reply
* Re: [PATCH 2/5] dma-direct: add an explicit dma_direct_get_required_mask
From: Christoph Hellwig @ 2018-09-27 15:28 UTC (permalink / raw)
To: Robin Murphy
Cc: Christoph Hellwig, iommu, Marek Szyprowski,
Benjamin Herrenschmidt, Greg Kroah-Hartman, linux-kernel,
linuxppc-dev
In-Reply-To: <02ce2ecc-281d-7b7f-6d30-8d246acd1eaa@arm.com>
On Thu, Sep 27, 2018 at 03:12:25PM +0100, Robin Murphy wrote:
>> +u64 dma_direct_get_required_mask(struct device *dev)
>> +{
>> + u64 max_dma = phys_to_dma_direct(dev, (max_pfn - 1) << PAGE_SHIFT);
>> +
>> + return (1ULL << (fls64(max_dma) - 1)) * 2 - 1;
>
> I think that may as well just use __fls64() - it seems reasonable to assume
> max_dma > 0. Otherwise,
Is there any good reason to micro-optimize given that this isn't
a fast path?
^ permalink raw reply
* Re: [PATCH 3/5] dma-direct: refine dma_direct_alloc zone selection
From: Christoph Hellwig @ 2018-09-27 15:30 UTC (permalink / raw)
To: Robin Murphy
Cc: Christoph Hellwig, iommu, Marek Szyprowski,
Benjamin Herrenschmidt, Greg Kroah-Hartman, linux-kernel,
linuxppc-dev
In-Reply-To: <fd524731-a0aa-b970-d320-fe209b9c68f6@arm.com>
On Thu, Sep 27, 2018 at 03:30:20PM +0100, Robin Murphy wrote:
>> +static gfp_t __dma_direct_optimal_gfp_mask(struct device *dev, u64
>> dma_mask,
>> + u64 *phys_mask)
>> +{
>> + if (force_dma_unencrypted())
>> + *phys_mask = __dma_to_phys(dev, dma_mask);
>> + else
>> + *phys_mask = dma_to_phys(dev, dma_mask);
>
> Maybe make phys_to_dma_direct() take u64 instead of phys_addr_t so we can
> reuse it here?
This is a dma_to_phys and not a phys_to_dma.
^ permalink raw reply
* Re: [PATCH 4/5] dma-direct: implement complete bus_dma_mask handling
From: Christoph Hellwig @ 2018-09-27 15:32 UTC (permalink / raw)
To: Robin Murphy
Cc: Christoph Hellwig, iommu, Marek Szyprowski,
Benjamin Herrenschmidt, Greg Kroah-Hartman, linux-kernel,
linuxppc-dev
In-Reply-To: <a7626d1b-4dd8-abc4-0ab0-21ab7e5d1a4c@arm.com>
On Thu, Sep 27, 2018 at 03:58:04PM +0100, Robin Murphy wrote:
>> }
>> #endif /* !CONFIG_ARCH_HAS_PHYS_TO_DMA */
>> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
>> index 3c404e33d946..64466b7ef67b 100644
>> --- a/kernel/dma/direct.c
>> +++ b/kernel/dma/direct.c
>> @@ -43,10 +43,11 @@ check_addr(struct device *dev, dma_addr_t dma_addr, size_t size,
>> return false;
>> }
>> - if (*dev->dma_mask >= DMA_BIT_MASK(32)) {
>> + if (*dev->dma_mask >= DMA_BIT_MASK(32) || dev->bus_dma_mask) {
>
> Hmm... say *dev->dma_mask is 31 bits and dev->bus_dma_mask is 40 bits due
> to a global DT property, we'll now scream where we didn't before even
> though the bus mask is almost certainly irrelevant - is that desirable?
This is just the reporting in the error case - we'll only hit this
IFF dma_capable already returned false. But if you don't want a message
here we can probably drop this hunk.
>> @@ -65,12 +66,18 @@ u64 dma_direct_get_required_mask(struct device *dev)
>> {
>> u64 max_dma = phys_to_dma_direct(dev, (max_pfn - 1) << PAGE_SHIFT);
>> + if (dev->bus_dma_mask && dev->bus_dma_mask < max_dma)
>> + max_dma = dev->bus_dma_mask;
>
> Again, I think we could just do another min_not_zero() here. A device wired
> to address only one single page of RAM isn't a realistic prospect (and we
> could just flip the -1 and the shift in the max_dma calculation if we
> *really* wanted to support such things).
This just seemed more readable to me than min_not_zero, but if others
prefer min_not_zero I can switch.
^ permalink raw reply
* Re: [PATCH 2/5] dma-direct: add an explicit dma_direct_get_required_mask
From: Robin Murphy @ 2018-09-27 15:35 UTC (permalink / raw)
To: Christoph Hellwig
Cc: iommu, Marek Szyprowski, Benjamin Herrenschmidt,
Greg Kroah-Hartman, linux-kernel, linuxppc-dev
In-Reply-To: <20180927152818.GC10566@lst.de>
On 27/09/18 16:28, Christoph Hellwig wrote:
> On Thu, Sep 27, 2018 at 03:12:25PM +0100, Robin Murphy wrote:
>>> +u64 dma_direct_get_required_mask(struct device *dev)
>>> +{
>>> + u64 max_dma = phys_to_dma_direct(dev, (max_pfn - 1) << PAGE_SHIFT);
>>> +
>>> + return (1ULL << (fls64(max_dma) - 1)) * 2 - 1;
>>
>> I think that may as well just use __fls64() - it seems reasonable to assume
>> max_dma > 0. Otherwise,
>
> Is there any good reason to micro-optimize given that this isn't
> a fast path?
Not at all, I wasn't even thinking in terms of optimisation other than
in terms of number of source characters and levels of parentheses.
But more importantly I was also being a big idiot because no matter how
much I have the fls()/__fls() thing in mind, __fls64() doesn't actually
exist. Nitpick rescinded!
Robin.
^ permalink raw reply
* Re: [PATCH 3/5] dma-direct: refine dma_direct_alloc zone selection
From: Robin Murphy @ 2018-09-27 15:38 UTC (permalink / raw)
To: Christoph Hellwig
Cc: iommu, Marek Szyprowski, Benjamin Herrenschmidt,
Greg Kroah-Hartman, linux-kernel, linuxppc-dev
In-Reply-To: <20180927153028.GD10566@lst.de>
On 27/09/18 16:30, Christoph Hellwig wrote:
> On Thu, Sep 27, 2018 at 03:30:20PM +0100, Robin Murphy wrote:
>>> +static gfp_t __dma_direct_optimal_gfp_mask(struct device *dev, u64
>>> dma_mask,
>>> + u64 *phys_mask)
>>> +{
>>> + if (force_dma_unencrypted())
>>> + *phys_mask = __dma_to_phys(dev, dma_mask);
>>> + else
>>> + *phys_mask = dma_to_phys(dev, dma_mask);
>>
>> Maybe make phys_to_dma_direct() take u64 instead of phys_addr_t so we can
>> reuse it here?
>
> This is a dma_to_phys and not a phys_to_dma.
Ugh, clearly it's time to stop reviewing patches for today... sorry :(
Robin.
^ permalink raw reply
* Re: [PATCH 3/5] dma-direct: refine dma_direct_alloc zone selection
From: Christoph Hellwig @ 2018-09-27 15:41 UTC (permalink / raw)
To: Robin Murphy
Cc: Christoph Hellwig, iommu, Marek Szyprowski,
Benjamin Herrenschmidt, Greg Kroah-Hartman, linux-kernel,
linuxppc-dev
In-Reply-To: <2b98d7b2-bf74-ccc9-881a-a91e2c9949c3@arm.com>
On Thu, Sep 27, 2018 at 04:38:31PM +0100, Robin Murphy wrote:
> On 27/09/18 16:30, Christoph Hellwig wrote:
>> On Thu, Sep 27, 2018 at 03:30:20PM +0100, Robin Murphy wrote:
>>>> +static gfp_t __dma_direct_optimal_gfp_mask(struct device *dev, u64
>>>> dma_mask,
>>>> + u64 *phys_mask)
>>>> +{
>>>> + if (force_dma_unencrypted())
>>>> + *phys_mask = __dma_to_phys(dev, dma_mask);
>>>> + else
>>>> + *phys_mask = dma_to_phys(dev, dma_mask);
>>>
>>> Maybe make phys_to_dma_direct() take u64 instead of phys_addr_t so we can
>>> reuse it here?
>>
>> This is a dma_to_phys and not a phys_to_dma.
>
> Ugh, clearly it's time to stop reviewing patches for today... sorry :(
I actually made the same mistake when writing it..
ALthough I'd really like to see some feedback from you on the arm64
swiotlb series once you had more cofee ;-)
^ permalink raw reply
* Re: [PATCH 4/5] dma-direct: implement complete bus_dma_mask handling
From: Robin Murphy @ 2018-09-27 16:14 UTC (permalink / raw)
To: Christoph Hellwig
Cc: iommu, Marek Szyprowski, Benjamin Herrenschmidt,
Greg Kroah-Hartman, linux-kernel, linuxppc-dev
In-Reply-To: <20180927153252.GE10566@lst.de>
On 27/09/18 16:32, Christoph Hellwig wrote:
> On Thu, Sep 27, 2018 at 03:58:04PM +0100, Robin Murphy wrote:
>>> }
>>> #endif /* !CONFIG_ARCH_HAS_PHYS_TO_DMA */
>>> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
>>> index 3c404e33d946..64466b7ef67b 100644
>>> --- a/kernel/dma/direct.c
>>> +++ b/kernel/dma/direct.c
>>> @@ -43,10 +43,11 @@ check_addr(struct device *dev, dma_addr_t dma_addr, size_t size,
>>> return false;
>>> }
>>> - if (*dev->dma_mask >= DMA_BIT_MASK(32)) {
>>> + if (*dev->dma_mask >= DMA_BIT_MASK(32) || dev->bus_dma_mask) {
>>
>> Hmm... say *dev->dma_mask is 31 bits and dev->bus_dma_mask is 40 bits due
>> to a global DT property, we'll now scream where we didn't before even
>> though the bus mask is almost certainly irrelevant - is that desirable?
>
> This is just the reporting in the error case - we'll only hit this
> IFF dma_capable already returned false. But if you don't want a message
> here we can probably drop this hunk.
It was only a question of consistency - whatever the reason was to avoid
warning for small device masks before, it's not obvious why it wouldn't
still apply in the presence of nonzero but larger bus mask. The fact
that the current check is there at all implied to me that we're
expecting less-capable device to hit this often and thus wanted to avoid
being noisy. If that's not the case then it's fine as-is AFAIC.
>>> @@ -65,12 +66,18 @@ u64 dma_direct_get_required_mask(struct device *dev)
>>> {
>>> u64 max_dma = phys_to_dma_direct(dev, (max_pfn - 1) << PAGE_SHIFT);
>>> + if (dev->bus_dma_mask && dev->bus_dma_mask < max_dma)
>>> + max_dma = dev->bus_dma_mask;
>>
>> Again, I think we could just do another min_not_zero() here. A device wired
>> to address only one single page of RAM isn't a realistic prospect (and we
>> could just flip the -1 and the shift in the max_dma calculation if we
>> *really* wanted to support such things).
>
> This just seemed more readable to me than min_not_zero, but if others
> prefer min_not_zero I can switch.
Nah, just checking whether there were any intentionally different
assumptions compared to the couple of other places in the patch where
min_not_zero() *is* used. If it's purely a style thing then no worries
(personally I'd have written it yet another way anyway).
Robin.
^ permalink raw reply
* Re: [PATCH 4/5] dma-direct: implement complete bus_dma_mask handling
From: Robin Murphy @ 2018-09-27 16:14 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-kernel, iommu, Greg Kroah-Hartman, linuxppc-dev,
Marek Szyprowski
In-Reply-To: <20180927153252.GE10566@lst.de>
On 27/09/18 16:32, Christoph Hellwig wrote:
> On Thu, Sep 27, 2018 at 03:58:04PM +0100, Robin Murphy wrote:
>>> }
>>> #endif /* !CONFIG_ARCH_HAS_PHYS_TO_DMA */
>>> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
>>> index 3c404e33d946..64466b7ef67b 100644
>>> --- a/kernel/dma/direct.c
>>> +++ b/kernel/dma/direct.c
>>> @@ -43,10 +43,11 @@ check_addr(struct device *dev, dma_addr_t dma_addr, size_t size,
>>> return false;
>>> }
>>> - if (*dev->dma_mask >= DMA_BIT_MASK(32)) {
>>> + if (*dev->dma_mask >= DMA_BIT_MASK(32) || dev->bus_dma_mask) {
>>
>> Hmm... say *dev->dma_mask is 31 bits and dev->bus_dma_mask is 40 bits due
>> to a global DT property, we'll now scream where we didn't before even
>> though the bus mask is almost certainly irrelevant - is that desirable?
>
> This is just the reporting in the error case - we'll only hit this
> IFF dma_capable already returned false. But if you don't want a message
> here we can probably drop this hunk.
It was only a question of consistency - whatever the reason was to avoid
warning for small device masks before, it's not obvious why it wouldn't
still apply in the presence of nonzero but larger bus mask. The fact
that the current check is there at all implied to me that we're
expecting less-capable device to hit this often and thus wanted to avoid
being noisy. If that's not the case then it's fine as-is AFAIC.
>>> @@ -65,12 +66,18 @@ u64 dma_direct_get_required_mask(struct device *dev)
>>> {
>>> u64 max_dma = phys_to_dma_direct(dev, (max_pfn - 1) << PAGE_SHIFT);
>>> + if (dev->bus_dma_mask && dev->bus_dma_mask < max_dma)
>>> + max_dma = dev->bus_dma_mask;
>>
>> Again, I think we could just do another min_not_zero() here. A device wired
>> to address only one single page of RAM isn't a realistic prospect (and we
>> could just flip the -1 and the shift in the max_dma calculation if we
>> *really* wanted to support such things).
>
> This just seemed more readable to me than min_not_zero, but if others
> prefer min_not_zero I can switch.
Nah, just checking whether there were any intentionally different
assumptions compared to the couple of other places in the patch where
min_not_zero() *is* used. If it's purely a style thing then no worries
(personally I'd have written it yet another way anyway).
Robin.
^ permalink raw reply
* Re: [PATCH 4/5] dma-direct: implement complete bus_dma_mask handling
From: Christoph Hellwig @ 2018-09-27 16:27 UTC (permalink / raw)
To: Robin Murphy
Cc: Christoph Hellwig, iommu, Marek Szyprowski,
Benjamin Herrenschmidt, Greg Kroah-Hartman, linux-kernel,
linuxppc-dev
In-Reply-To: <967e98b1-99ab-8f95-4c89-6156ce489b93@arm.com>
On Thu, Sep 27, 2018 at 05:14:56PM +0100, Robin Murphy wrote:
>> This just seemed more readable to me than min_not_zero, but if others
>> prefer min_not_zero I can switch.
>
> Nah, just checking whether there were any intentionally different
> assumptions compared to the couple of other places in the patch where
> min_not_zero() *is* used. If it's purely a style thing then no worries
> (personally I'd have written it yet another way anyway).
I'm curious: how would you have written it?
^ permalink raw reply
* Re: [PATCH 4/5] dma-direct: implement complete bus_dma_mask handling
From: Christoph Hellwig @ 2018-09-27 16:27 UTC (permalink / raw)
To: Robin Murphy
Cc: linux-kernel, iommu, Greg Kroah-Hartman, linuxppc-dev,
Christoph Hellwig, Marek Szyprowski
In-Reply-To: <967e98b1-99ab-8f95-4c89-6156ce489b93@arm.com>
On Thu, Sep 27, 2018 at 05:14:56PM +0100, Robin Murphy wrote:
>> This just seemed more readable to me than min_not_zero, but if others
>> prefer min_not_zero I can switch.
>
> Nah, just checking whether there were any intentionally different
> assumptions compared to the couple of other places in the patch where
> min_not_zero() *is* used. If it's purely a style thing then no worries
> (personally I'd have written it yet another way anyway).
I'm curious: how would you have written it?
^ permalink raw reply
* Re: [PATCH 4/5] dma-direct: implement complete bus_dma_mask handling
From: Robin Murphy @ 2018-09-27 16:41 UTC (permalink / raw)
To: Christoph Hellwig
Cc: iommu, Marek Szyprowski, Benjamin Herrenschmidt,
Greg Kroah-Hartman, linux-kernel, linuxppc-dev
In-Reply-To: <20180927162737.GA11974@lst.de>
On 27/09/18 17:27, Christoph Hellwig wrote:
> On Thu, Sep 27, 2018 at 05:14:56PM +0100, Robin Murphy wrote:
>>> This just seemed more readable to me than min_not_zero, but if others
>>> prefer min_not_zero I can switch.
>>
>> Nah, just checking whether there were any intentionally different
>> assumptions compared to the couple of other places in the patch where
>> min_not_zero() *is* used. If it's purely a style thing then no worries
>> (personally I'd have written it yet another way anyway).
>
> I'm curious: how would you have written it?
Come to think of it, I actually already have, in iommu-dma:
if (dev->bus_dma_mask)
mask &= dev->bus_dma_mask;
but of course it's not so pretty for those cases where you don't already
have the local variable ready to go.
Robin.
^ permalink raw reply
* Re: [PATCH 4/5] dma-direct: implement complete bus_dma_mask handling
From: Robin Murphy @ 2018-09-27 16:41 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-kernel, iommu, Greg Kroah-Hartman, linuxppc-dev,
Marek Szyprowski
In-Reply-To: <20180927162737.GA11974@lst.de>
On 27/09/18 17:27, Christoph Hellwig wrote:
> On Thu, Sep 27, 2018 at 05:14:56PM +0100, Robin Murphy wrote:
>>> This just seemed more readable to me than min_not_zero, but if others
>>> prefer min_not_zero I can switch.
>>
>> Nah, just checking whether there were any intentionally different
>> assumptions compared to the couple of other places in the patch where
>> min_not_zero() *is* used. If it's purely a style thing then no worries
>> (personally I'd have written it yet another way anyway).
>
> I'm curious: how would you have written it?
Come to think of it, I actually already have, in iommu-dma:
if (dev->bus_dma_mask)
mask &= dev->bus_dma_mask;
but of course it's not so pretty for those cases where you don't already
have the local variable ready to go.
Robin.
^ permalink raw reply
* [PATCH] powerpc/rtas: Fix a potential race between CPU-Offline & Migration
From: Gautham R. Shenoy @ 2018-09-27 16:51 UTC (permalink / raw)
To: Michael Ellerman, Nathan Fontenot, Tyrel Datwyler
Cc: linuxppc-dev, Gautham R. Shenoy
From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
Live Partition Migrations require all the present CPUs to execute the
H_JOIN call, and hence rtas_ibm_suspend_me() onlines any offline CPUs
before initiating the migration for this purpose.
The commit 85a88cabad57
("powerpc/pseries: Disable CPU hotplug across migrations")
disables any CPU-hotplug operations once all the offline CPUs are
brought online to prevent any further state change. Once the
CPU-Hotplug operation is disabled, the code assumes that all the CPUs
are online.
However, there is a minor window in rtas_ibm_suspend_me() between
onlining the offline CPUs and disabling CPU-Hotplug when a concurrent
CPU-offline operations initiated by the userspace can succeed thereby
nullifying the the aformentioned assumption. In this unlikely case
these offlined CPUs will not call H_JOIN, resulting in a system hang.
Fix this by verifying that all the present CPUs are actually online
after CPU-Hotplug has been disabled, failing which we return from
rtas_ibm_suspend_me() with -EBUSY.
Cc: Nathan Fontenot <nfont@linux.vnet.ibm.com>
Cc: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
arch/powerpc/kernel/rtas.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 2c7ed31..27f6fd3 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -982,6 +982,16 @@ int rtas_ibm_suspend_me(u64 handle)
}
cpu_hotplug_disable();
+
+ /* Check if we raced with a CPU-Offline Operation */
+ if (unlikely(!cpumask_equal(cpu_present_mask, cpu_online_mask))) {
+ pr_err("%s: Raced against a concurrent CPU-Offline\n",
+ __func__);
+ atomic_set(&data.error, -EBUSY);
+ cpu_hotplug_enable();
+ goto out;
+ }
+
stop_topology_update();
/* Call function on all CPUs. One of us will make the
--
1.9.4
^ permalink raw reply related
* [PATCH] powerpc/rtas: Fix a potential race between CPU-Offline & Migration
From: Gautham R. Shenoy @ 2018-09-27 16:51 UTC (permalink / raw)
To: Michael Ellerman, Nathan Fontenot, Tyrel Datwyler
Cc: Gautham R. Shenoy, linuxppc-dev
From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
Live Partition Migrations require all the present CPUs to execute the
H_JOIN call, and hence rtas_ibm_suspend_me() onlines any offline CPUs
before initiating the migration for this purpose.
The commit 85a88cabad57
("powerpc/pseries: Disable CPU hotplug across migrations")
disables any CPU-hotplug operations once all the offline CPUs are
brought online to prevent any further state change. Once the
CPU-Hotplug operation is disabled, the code assumes that all the CPUs
are online.
However, there is a minor window in rtas_ibm_suspend_me() between
onlining the offline CPUs and disabling CPU-Hotplug when a concurrent
CPU-offline operations initiated by the userspace can succeed thereby
nullifying the the aformentioned assumption. In this unlikely case
these offlined CPUs will not call H_JOIN, resulting in a system hang.
Fix this by verifying that all the present CPUs are actually online
after CPU-Hotplug has been disabled, failing which we return from
rtas_ibm_suspend_me() with -EBUSY.
Cc: Nathan Fontenot <nfont@linux.vnet.ibm.com>
Cc: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
arch/powerpc/kernel/rtas.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 2c7ed31..27f6fd3 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -982,6 +982,16 @@ int rtas_ibm_suspend_me(u64 handle)
}
cpu_hotplug_disable();
+
+ /* Check if we raced with a CPU-Offline Operation */
+ if (unlikely(!cpumask_equal(cpu_present_mask, cpu_online_mask))) {
+ pr_err("%s: Raced against a concurrent CPU-Offline\n",
+ __func__);
+ atomic_set(&data.error, -EBUSY);
+ cpu_hotplug_enable();
+ goto out;
+ }
+
stop_topology_update();
/* Call function on all CPUs. One of us will make the
--
1.9.4
^ permalink raw reply related
* Re: [PATCH 0/8] add generic builtin command line
From: Ard Biesheuvel @ 2018-09-27 17:05 UTC (permalink / raw)
To: Maksym Kokhan
Cc: Andrew Morton, Rob Herring, Frank Rowand, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin,
the arch/x86 maintainers, Russell King, Catalin Marinas,
Will Deacon, Ralf Baechle, Paul Burton, James Hogan,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Daniel Walker, Daniel Walker, Andrii Bordunov, Ruslan Bilovol,
Linux Kernel Mailing List, Devicetree List, linux-efi, linux-mips,
linuxppc-dev
In-Reply-To: <1538067309-5711-1-git-send-email-maksym.kokhan@globallogic.com>
On 27 September 2018 at 18:55, Maksym Kokhan
<maksym.kokhan@globallogic.com> wrote:
> There were series of patches [1] for 4.3.0-rc3, that allowed
> architectures to use a generic builtin command line. I have rebased
> these patches on kernel 4.19.0-rc4.
>
Could you please elaborate on the purpose of this series? Is it simply
to align between architectures? Does it solve an actual problem?
> Things, modified in comparison with original patches:
> * There was some bug for mips, in the case when CONFIG_CMDLINE_PREPEND
> and CONFIG_CMDLINE_APPEND are empty and CMDLINE_OVERRIDE is not set,
> command line from bootloader was ignored, so I fixed it, modifying
> patch "add generic builtin command line".
>
> * Implemented new patch to resolve conflict with new kernel, which
> modify EFI stub code. Unfortunately, I don't have capability to test
> this modification on real arm board with EFI.
>
> * Removed new realisation of mips builtin command line, which was
> created after 4.3.0-rc3.
>
> * Kernel 4.3.0-rc3 with original patches could not be compiled for
> powerpc due to prom_init.c checking by prom_init_check.sh. So I added
> strlcat (which is used by cmdline_add_builtin macro) to
> prom_init_check.sh whitelist.
>
> Patches have been tested in QEMU for x86, arm (little-endian), arm64
> (little-endian), mips (little-endian, 32-bit) and powerpc
> (big-endian, 64-bit), everything works perfectly. Also it was tested
> on linux-next (next-20180924 tag) for all listed above architectures.
>
> [1] : https://lore.kernel.org/patchwork/patch/604992/
>
> Daniel Walker (7):
> add generic builtin command line
> drivers: of: ifdef out cmdline section
> x86: convert to generic builtin command line
> arm: convert to generic builtin command line
> arm64: convert to generic builtin command line
> mips: convert to generic builtin command line
> powerpc: convert to generic builtin command line
>
> Maksym Kokhan (1):
> efi: modify EFI stub code for arm/arm64
>
> arch/arm/Kconfig | 38 +-----------------
> arch/arm/kernel/atags_parse.c | 14 ++-----
> arch/arm/kernel/devtree.c | 2 +
> arch/arm64/Kconfig | 17 +-------
> arch/arm64/kernel/setup.c | 3 ++
> arch/mips/Kconfig | 24 +----------
> arch/mips/Kconfig.debug | 47 ----------------------
> arch/mips/kernel/setup.c | 41 ++-----------------
> arch/powerpc/Kconfig | 23 +----------
> arch/powerpc/kernel/prom.c | 4 ++
> arch/powerpc/kernel/prom_init.c | 8 ++--
> arch/powerpc/kernel/prom_init_check.sh | 2 +-
> arch/x86/Kconfig | 44 +--------------------
> arch/x86/kernel/setup.c | 19 ++-------
> drivers/firmware/efi/libstub/arm-stub.c | 10 ++---
> drivers/of/fdt.c | 2 +-
> include/linux/cmdline.h | 70 +++++++++++++++++++++++++++++++++
> init/Kconfig | 68 ++++++++++++++++++++++++++++++++
> 18 files changed, 173 insertions(+), 263 deletions(-)
> create mode 100644 include/linux/cmdline.h
>
> --
> 2.7.4
>
^ 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