* [PATCH v5,4/4] drivers: uio: new driver for fsl_85xx_cache_sram
From: Wang Wenhu @ 2020-04-17 7:16 UTC (permalink / raw)
To: gregkh, linux-kernel, robh, oss, christophe.leroy, linuxppc-dev
Cc: kernel, Wang Wenhu
In-Reply-To: <20200417071616.44598-1-wenhu.wang@vivo.com>
A driver for freescale 85xx platforms to access the Cache-Sram form
user level. This is extremely helpful for some user-space applications
that require high performance memory accesses.
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
---
Changes since v1:
* Addressed comments from Greg K-H
* Moved kfree(info->name) into uio_info_free_internal()
Changes since v2:
* Addressed comments from Greg, Scott and Christophe
* Use "uiomem->internal_addr" as if condition for sram memory free,
and memset the uiomem entry
* of_match_table modified to be apart from HW info which belong to
the HW level driver fsl_85xx_cache_sram to match
* Use roundup_pow_of_two for align calc
* Remove useless clear block of uiomem entries.
* Use UIO_INFO_VER micro for info->version, and define it as
"devicetree,pseudo", meaning this is pseudo device and probed from
device tree configuration
Changes since v3:
* Addressed comments from Christophe(use devm_xxx memory alloc interfaces)
Changes since v4:
* Use module_param_string for of_match_table, no binding to devicetree
---
drivers/uio/Kconfig | 9 ++
drivers/uio/Makefile | 1 +
drivers/uio/uio_fsl_85xx_cache_sram.c | 154 ++++++++++++++++++++++++++
3 files changed, 164 insertions(+)
create mode 100644 drivers/uio/uio_fsl_85xx_cache_sram.c
diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 202ee81cfc2b..9c3b47461b71 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -105,6 +105,15 @@ config UIO_NETX
To compile this driver as a module, choose M here; the module
will be called uio_netx.
+config UIO_FSL_85XX_CACHE_SRAM
+ tristate "Freescale 85xx Cache-Sram driver"
+ depends on FSL_SOC_BOOKE && PPC32
+ select FSL_85XX_CACHE_SRAM
+ help
+ Generic driver for accessing the Cache-Sram form user level. This
+ is extremely helpful for some user-space applications that require
+ high performance memory accesses.
+
config UIO_FSL_ELBC_GPCM
tristate "eLBC/GPCM driver"
depends on FSL_LBC
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index c285dd2a4539..be2056cffc21 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -10,4 +10,5 @@ obj-$(CONFIG_UIO_NETX) += uio_netx.o
obj-$(CONFIG_UIO_PRUSS) += uio_pruss.o
obj-$(CONFIG_UIO_MF624) += uio_mf624.o
obj-$(CONFIG_UIO_FSL_ELBC_GPCM) += uio_fsl_elbc_gpcm.o
+obj-$(CONFIG_UIO_FSL_85XX_CACHE_SRAM) += uio_fsl_85xx_cache_sram.o
obj-$(CONFIG_UIO_HV_GENERIC) += uio_hv_generic.o
diff --git a/drivers/uio/uio_fsl_85xx_cache_sram.c b/drivers/uio/uio_fsl_85xx_cache_sram.c
new file mode 100644
index 000000000000..4db3648629b3
--- /dev/null
+++ b/drivers/uio/uio_fsl_85xx_cache_sram.c
@@ -0,0 +1,154 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Vivo Communication Technology Co. Ltd.
+ * Copyright (C) 2020 Wang Wenhu <wenhu.wang@vivo.com>
+ * All rights reserved.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/uio_driver.h>
+#include <linux/stringify.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <asm/fsl_85xx_cache_sram.h>
+
+#define DRIVER_NAME "uio_fsl_85xx_cache_sram"
+#define UIO_INFO_VER "devicetree,pseudo"
+#define UIO_NAME "uio_cache_sram"
+
+static void uio_info_free_internal(struct uio_info *info)
+{
+ int i;
+
+ for (i = 0; i < MAX_UIO_MAPS; i++) {
+ struct uio_mem *uiomem = &info->mem[i];
+
+ if (uiomem->internal_addr) {
+ mpc85xx_cache_sram_free(uiomem->internal_addr);
+ memset(uiomem, 0, sizeof(*uiomem));
+ }
+ }
+}
+
+static int uio_fsl_85xx_cache_sram_probe(struct platform_device *pdev)
+{
+ struct device_node *parent = pdev->dev.of_node;
+ struct device_node *node = NULL;
+ struct uio_info *info;
+ struct uio_mem *uiomem;
+ const char *dt_name;
+ u32 mem_size;
+ int ret;
+
+ /* alloc uio_info for one device */
+ info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+
+ /* get optional uio name */
+ if (of_property_read_string(parent, "uio_name", &dt_name))
+ dt_name = UIO_NAME;
+
+ info->name = devm_kstrdup(&pdev->dev, dt_name, GFP_KERNEL);
+ if (!info->name)
+ return -ENOMEM;
+
+ uiomem = info->mem;
+ for_each_child_of_node(parent, node) {
+ void *virt;
+ phys_addr_t phys;
+
+ ret = of_property_read_u32(node, "cache-mem-size", &mem_size);
+ if (ret) {
+ ret = -EINVAL;
+ goto err_out;
+ }
+
+ if (mem_size == 0) {
+ dev_err(&pdev->dev, "error cache-mem-size should not be 0\n");
+ ret = -EINVAL;
+ goto err_out;
+ }
+
+ virt = mpc85xx_cache_sram_alloc(mem_size, &phys,
+ roundup_pow_of_two(mem_size));
+ if (!virt) {
+ /* mpc85xx_cache_sram_alloc to define the real cause */
+ ret = -ENOMEM;
+ goto err_out;
+ }
+
+ uiomem->memtype = UIO_MEM_PHYS;
+ uiomem->addr = phys;
+ uiomem->size = mem_size;
+ uiomem->name = kstrdup(node->name, GFP_KERNEL);
+ uiomem->internal_addr = virt;
+ uiomem++;
+
+ if (uiomem >= &info->mem[MAX_UIO_MAPS]) {
+ dev_warn(&pdev->dev, "more than %d uio-maps for device.\n",
+ MAX_UIO_MAPS);
+ break;
+ }
+ }
+
+ if (uiomem == info->mem) {
+ dev_err(&pdev->dev, "error no valid uio-map configuration found\n");
+ return -EINVAL;
+ }
+
+ info->version = UIO_INFO_VER;
+
+ /* register uio device */
+ if (uio_register_device(&pdev->dev, info)) {
+ dev_err(&pdev->dev, "error uio,cache-sram registration failed\n");
+ ret = -ENODEV;
+ goto err_out;
+ }
+
+ platform_set_drvdata(pdev, info);
+
+ return 0;
+err_out:
+ uio_info_free_internal(info);
+ return ret;
+}
+
+static int uio_fsl_85xx_cache_sram_remove(struct platform_device *pdev)
+{
+ struct uio_info *info = platform_get_drvdata(pdev);
+
+ uio_unregister_device(info);
+
+ uio_info_free_internal(info);
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static struct of_device_id uio_fsl_85xx_cache_sram_of_match[] = {
+ { /* This is filled with module_parm */ },
+ { /* Sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, uio_fsl_85xx_cache_sram_of_match);
+
+module_param_string(of_id, uio_fsl_85xx_cache_sram_of_match[0].compatible,
+ sizeof(uio_fsl_85xx_cache_sram_of_match[0].compatible), 0);
+MODULE_PARM_DESC(of_id, "platform device id to be handled by cache-sram-uio");
+#endif
+
+static struct platform_driver uio_fsl_85xx_cache_sram = {
+ .probe = uio_fsl_85xx_cache_sram_probe,
+ .remove = uio_fsl_85xx_cache_sram_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ .of_match_table = of_match_ptr(uio_fsl_85xx_cache_sram_of_match),
+ },
+};
+
+module_platform_driver(uio_fsl_85xx_cache_sram);
+
+MODULE_AUTHOR("Wang Wenhu <wenhu.wang@vivo.com>");
+MODULE_DESCRIPTION("Freescale MPC85xx Cache-Sram UIO Platform Driver");
+MODULE_ALIAS("platform:" DRIVER_NAME);
+MODULE_LICENSE("GPL v2");
--
2.17.1
^ permalink raw reply related
* Re:Re: [PATCH v4,4/4] drivers: uio: new driver for fsl_85xx_cache_sram
From: 王文虎 @ 2020-04-17 7:04 UTC (permalink / raw)
To: Scott Wood; +Cc: Rob Herring, gregkh, linux-kernel, kernel, linuxppc-dev
In-Reply-To: <64bb1f056abd8bfab2befef5d1e6baec2056077f.camel@buserror.net>
>> > > On Thu, 2020-04-16 at 08:35 -0700, Wang Wenhu wrote:
>> > > > +#define UIO_INFO_VER "devicetree,pseudo"
>> > >
>> > > What does this mean? Changing a number into a non-obvious string (Why
>> > > "pseudo"? Why does the UIO user care that the config came from the
>> > > device
>> > > tree?) just to avoid setting off Greg's version number autoresponse
>> > > isn't
>> > > really helping anything.
As I mentioned before, this is not currently meaningful for us, and maybe the better
way is to set it optionally for uio, but it belongs to uio core, which is a framework for
different kind of drivers or devices, but not only for us. So I guess this is not a thing
troubles and arguing about this is really helpless.
>> > >
>> > > > +static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
>> > > > + { .compatible = "uio,mpc85xx-cache-sram", },
>> >
>> > Form is <vendor>,<device> and "uio" is not a vendor (and never will be).
>> >
>>
>> Should have been something like "fsl,mpc85xx-cache-sram-uio", and if it is
>> to be defined with module parameters, this would be user defined.
>> Anyway, <vendor>,<device> should always be used.
>>
>> > > > + {},
>> > > > +};
>> > > > +
>> > > > +static struct platform_driver uio_fsl_85xx_cache_sram = {
>> > > > + .probe = uio_fsl_85xx_cache_sram_probe,
>> > > > + .remove = uio_fsl_85xx_cache_sram_remove,
>> > > > + .driver = {
>> > > > + .name = DRIVER_NAME,
>> > > > + .owner = THIS_MODULE,
>> > > > + .of_match_table = uio_mpc85xx_l2ctlr_of_match,
>> > > > + },
>> > > > +};
>> > >
>> > > Greg's comment notwithstanding, I really don't think this belongs in the
>> > > device tree (and if I do get overruled on that point, it at least needs
>> > > a
>> > > binding document). Let me try to come up with a patch for dynamic
>> > > allocation.
>> >
>> > Agreed. "UIO" bindings have long been rejected.
>> >
>>
>> Sounds it is. And does the modification below fit well?
>> ---
>> -static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
>> - { .compatible = "uio,mpc85xx-cache-sram", },
>> - {},
>> +#ifdef CONFIG_OF
>> +static struct of_device_id uio_fsl_85xx_cache_sram_of_match[] = {
>> + { /* This is filled with module_parm */ },
>> + { /* Sentinel */ },
>> };
>> +MODULE_DEVICE_TABLE(of, uio_fsl_85xx_cache_sram_of_match);
>> +module_param_string(of_id, uio_fsl_85xx_cache_sram_of_match[0].compatible,
>> + sizeof(uio_fsl_85xx_cache_sram_of_match[0].compa
>> tible), 0);
>> +MODULE_PARM_DESC(of_id, "platform device id to be handled by cache-sram-
>> uio");
>> +#endif
>
>No. The point is that you wouldn't be configuring this with the device tree
>at all.
>
It was to fit for the unbinding requriement. And I mentioned what if I want to create
more than one device and each owns multiple uiomaps?
Devicetree is definitely better choice to solve the problem and make the driver more
convenient for users to use. Pseudo device is a device and a device. Or else device
tree should be hardware-only-devicetree.
The point is why we left the better choice and write a plenty of redundant codes
to parse the module parameters?
Further more, I don't think there is enough reason for the lower driver mpc85xx cache-sram
to restrict other from using it in a way of module param or dynamic allocation with ioctl or so.
UIO is there and all these parts cooperate well to make the cache-sram more useful and better
resolve user requirement.
I will update the patch with the diff applied in v5.
Thanks,
Wenhu
^ permalink raw reply
* Re: POWER9 crash due to STRICT_KERNEL_RWX (WAS: Re: Linux-next POWER9 NULL pointer NIP...)
From: Naveen N. Rao @ 2020-04-17 7:01 UTC (permalink / raw)
To: Qian Cai, Michael Ellerman, Russell Currey
Cc: Steven Rostedt, linuxppc-dev, LKML, Nicholas Piggin
In-Reply-To: <69F0448F-CA5B-497D-B8AF-2848175B9477@lca.pw>
Hi Qian,
Qian Cai wrote:
> OK, reverted the commit,
>
> c55d7b5e6426 (“powerpc: Remove STRICT_KERNEL_RWX incompatibility with RELOCATABLE”)
>
> or set STRICT_KERNEL_RWX=n fixed the crash below and also mentioned in this thread,
>
> https://lore.kernel.org/lkml/15AC5B0E-A221-4B8C-9039-FA96B8EF7C88@lca.pw/
Do you see any errors logged in dmesg when you see the crash?
STRICT_KERNEL_RWX changes how patch_instruction() works, so it would be
interesting to see if there are any ftrace-related errors thrown before
the crash.
- Naveen
^ permalink raw reply
* Re: [PATCH V2] vhost: do not enable VHOST_MENU by default
From: Michael S. Tsirkin @ 2020-04-17 6:33 UTC (permalink / raw)
To: Jason Wang
Cc: linux-s390, tsbogend, gor, kvm, linux-kernel, heiko.carstens,
linux-mips, virtualization, borntraeger, geert, netdev, paulus,
linuxppc-dev
In-Reply-To: <b7e2deb7-cb64-b625-aeb4-760c7b28c0c8@redhat.com>
On Fri, Apr 17, 2020 at 11:12:14AM +0800, Jason Wang wrote:
>
> On 2020/4/17 上午6:55, Michael S. Tsirkin wrote:
> > On Wed, Apr 15, 2020 at 10:43:56AM +0800, Jason Wang wrote:
> > > We try to keep the defconfig untouched after decoupling CONFIG_VHOST
> > > out of CONFIG_VIRTUALIZATION in commit 20c384f1ea1a
> > > ("vhost: refine vhost and vringh kconfig") by enabling VHOST_MENU by
> > > default. Then the defconfigs can keep enabling CONFIG_VHOST_NET
> > > without the caring of CONFIG_VHOST.
> > >
> > > But this will leave a "CONFIG_VHOST_MENU=y" in all defconfigs and even
> > > for the ones that doesn't want vhost. So it actually shifts the
> > > burdens to the maintainers of all other to add "CONFIG_VHOST_MENU is
> > > not set". So this patch tries to enable CONFIG_VHOST explicitly in
> > > defconfigs that enables CONFIG_VHOST_NET and CONFIG_VHOST_VSOCK.
> > >
> > > Acked-by: Christian Borntraeger<borntraeger@de.ibm.com> (s390)
> > > Acked-by: Michael Ellerman<mpe@ellerman.id.au> (powerpc)
> > > Cc: Thomas Bogendoerfer<tsbogend@alpha.franken.de>
> > > Cc: Benjamin Herrenschmidt<benh@kernel.crashing.org>
> > > Cc: Paul Mackerras<paulus@samba.org>
> > > Cc: Michael Ellerman<mpe@ellerman.id.au>
> > > Cc: Heiko Carstens<heiko.carstens@de.ibm.com>
> > > Cc: Vasily Gorbik<gor@linux.ibm.com>
> > > Cc: Christian Borntraeger<borntraeger@de.ibm.com>
> > > Reported-by: Geert Uytterhoeven<geert@linux-m68k.org>
> > > Signed-off-by: Jason Wang<jasowang@redhat.com>
> > I rebased this on top of OABI fix since that
> > seems more orgent to fix.
> > Pushed to my vhost branch pls take a look and
> > if possible test.
> > Thanks!
>
>
> I test this patch by generating the defconfigs that wants vhost_net or
> vhost_vsock. All looks fine.
>
> But having CONFIG_VHOST_DPN=y may end up with the similar situation that
> this patch want to address.
> Maybe we can let CONFIG_VHOST depends on !ARM || AEABI then add another
> menuconfig for VHOST_RING and do something similar?
>
> Thanks
Sorry I don't understand. After this patch CONFIG_VHOST_DPN is just
an internal variable for the OABI fix. I kept it separate
so it's easy to revert for 5.8. Yes we could squash it into
VHOST directly but I don't see how that changes logic at all.
--
MST
^ permalink raw reply
* Re: [PATCH kernel v2 0/7] powerpc/powenv/ioda: Allow huge DMA window at 4GB
From: Alexey Kardashevskiy @ 2020-04-17 5:47 UTC (permalink / raw)
To: Russell Currey, Oliver O'Halloran
Cc: KVM list, Fabiano Rosas, Alistair Popple, kvm-ppc, linuxppc-dev,
David Gibson
In-Reply-To: <b0b361092d2d7e38f753edee6dcd9222b4e388ce.camel@russell.cc>
On 17/04/2020 11:26, Russell Currey wrote:
> On Thu, 2020-04-16 at 12:53 +1000, Oliver O'Halloran wrote:
>> On Thu, Apr 16, 2020 at 12:34 PM Oliver O'Halloran <oohall@gmail.com>
>> wrote:
>>> On Thu, Apr 16, 2020 at 11:27 AM Alexey Kardashevskiy <
>>> aik@ozlabs.ru> wrote:
>>>> Anyone? Is it totally useless or wrong approach? Thanks,
>>>
>>> I wouldn't say it's either, but I still hate it.
>>>
>>> The 4GB mode being per-PHB makes it difficult to use unless we
>>> force
>>> that mode on 100% of the time which I'd prefer not to do. Ideally
>>> devices that actually support 64bit addressing (which is most of
>>> them)
>>> should be able to use no-translate mode when possible since a) It's
>>> faster, and b) It frees up room in the TCE cache devices that
>>> actually
>>> need them. I know you've done some testing with 100G NICs and found
>>> the overhead was fine, but IMO that's a bad test since it's pretty
>>> much the best-case scenario since all the devices on the PHB are in
>>> the same PE. The PHB's TCE cache only hits when the TCE matches the
>>> DMA bus address and the PE number for the device so in a multi-PE
>>> environment there's a lot of potential for TCE cache trashing. If
>>> there was one or two PEs under that PHB it's probably not going to
>>> matter, but if you have an NVMe rack with 20 drives it starts to
>>> look
>>> a bit ugly.
>>>
>>> That all said, it might be worth doing this anyway since we
>>> probably
>>> want the software infrastructure in place to take advantage of it.
>>> Maybe expand the command line parameters to allow it to be enabled
>>> on
>>> a per-PHB basis rather than globally.
>>
>> Since we're on the topic
>>
>> I've been thinking the real issue we have is that we're trying to
>> pick
>> an "optimal" IOMMU config at a point where we don't have enough
>> information to work out what's actually optimal. The IOMMU config is
>> done on a per-PE basis, but since PEs may contain devices with
>> different DMA masks (looking at you wierd AMD audio function) we're
>> always going to have to pick something conservative as the default
>> config for TVE#0 (64k, no bypass mapping) since the driver will tell
>> us what the device actually supports long after the IOMMU
>> configuation
>> is done. What we really want is to be able to have separate IOMMU
>> contexts for each device, or at the very least a separate context for
>> the crippled devices.
>>
>> We could allow a per-device IOMMU context by extending the Master /
>> Slave PE thing to cover DMA in addition to MMIO. Right now we only
>> use
>> slave PEs when a device's MMIO BARs extend over multiple m64
>> segments.
>> When that happens an MMIO error causes the PHB to freezes the PE
>> corresponding to one of those segments, but not any of the others. To
>> present a single "PE" to the EEH core we check the freeze status of
>> each of the slave PEs when the EEH core does a PE status check and if
>> any of them are frozen, we freeze the rest of them too. When a driver
>> sets a limited DMA mask we could move that device to a seperate slave
>> PE so that it has it's own IOMMU context taylored to its DMA
>> addressing limits.
>>
>> Thoughts?
>
> For what it's worth this sounds like a good idea to me, it just sounds
> tricky to implement. You're adding another layer of complexity on top
> of EEH (well, making things look simple to the EEH core and doing your
> own freezing on top of it) in addition to the DMA handling.
>
> If it works then great, just has a high potential to become a new bug
> haven.
imho putting every PCI function to a separate PE is the right thing to
do here but I've been told it is not that simple, and I believe that.
Reusing slave PEs seems unreliable - the configuration will depend on
whether a PE occupied enough segments to give an unique PE to a PCI
function and my little brain explodes.
So this is not happening soon.
For the time being, this patchset is good for:
1. weird hardware which has limited DMA mask (this is why the patchset
was written in the first place)
2. debug DMA by routing it via IOMMU (even when 4GB hack is not enabled).
--
Alexey
^ permalink raw reply
* Re: [PATCH V3 3/5] selftests/powerpc: Add NX-GZIP engine compress testcase
From: Michael Ellerman @ 2020-04-17 5:05 UTC (permalink / raw)
To: Raphael Moreira Zinsly, linuxppc-dev, linux-crypto, dja
Cc: abali, haren, herbert, rzinsly
In-Reply-To: <20200413155916.16900-4-rzinsly@linux.ibm.com>
Hi Raphael,
Some comments below ...
Raphael Moreira Zinsly <rzinsly@linux.ibm.com> writes:
> Add a compression testcase for the powerpc NX-GZIP engine.
>
> Signed-off-by: Bulent Abali <abali@us.ibm.com>
> Signed-off-by: Raphael Moreira Zinsly <rzinsly@linux.ibm.com>
> ---
> .../selftests/powerpc/nx-gzip/Makefile | 21 +
> .../selftests/powerpc/nx-gzip/gzfht_test.c | 432 ++++++++++++++++++
> .../selftests/powerpc/nx-gzip/gzip_vas.c | 316 +++++++++++++
> 3 files changed, 769 insertions(+)
> create mode 100644 tools/testing/selftests/powerpc/nx-gzip/Makefile
> create mode 100644 tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
> create mode 100644 tools/testing/selftests/powerpc/nx-gzip/gzip_vas.c
You haven't added this to tools/testing/selftests/powerpc/Makefile,
which means it's not actually built as part of the selftests build.
Which makes it not really a selftest.
We need:
diff --git a/tools/testing/selftests/powerpc/Makefile b/tools/testing/selftests/powerpc/Makefile
index 644770c3b754..0830e63818c1 100644
--- a/tools/testing/selftests/powerpc/Makefile
+++ b/tools/testing/selftests/powerpc/Makefile
@@ -19,6 +19,7 @@ SUB_DIRS = alignment \
copyloops \
dscr \
mm \
+ nx-gzip \
pmu \
signal \
primitives \
> diff --git a/tools/testing/selftests/powerpc/nx-gzip/Makefile b/tools/testing/selftests/powerpc/nx-gzip/Makefile
> new file mode 100644
> index 000000000000..ab903f63bbbd
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/nx-gzip/Makefile
> @@ -0,0 +1,21 @@
> +CC = gcc
> +CFLAGS = -O3
> +INC = ./inc
> +SRC = gzfht_test.c
> +OBJ = $(SRC:.c=.o)
> +TESTS = gzfht_test
> +EXTRA_SOURCES = gzip_vas.c
> +
> +all: $(TESTS)
> +
> +$(OBJ): %.o: %.c
> + $(CC) $(CFLAGS) -I$(INC) -c $<
> +
> +$(TESTS): $(OBJ)
> + $(CC) $(CFLAGS) -I$(INC) -o $@ $@.o $(EXTRA_SOURCES)
> +
> +run_tests: $(TESTS)
> + ./gzfht_test gzip_vas.c
> +
> +clean:
> + rm -f $(TESTS) *.o *~ *.gz
We have an existing system for Makefiles for selftests.
Also the test programs need to be able to just run standalone with no
arguments in order for the selftest machinery to run them correctly.
The patch below should integrate the tests properly and add a wrapper
script to run the tests.
However I'm still getting some warnings from the build:
gunz_test.c: In function ‘decompress_file’:
gunz_test.c:914:12: error: ‘total_out’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
914 | total_out = total_out + tpbc;
| ~~~~~~~~~~^~~~~~~~~~~~~~~~~~
gunz_test.c:287:11: error: ‘last_comp_ratio’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
287 | uint64_t last_comp_ratio; /* 1000 max */
| ^~~~~~~~~~~~~~~
gunz_test.c:519:8: error: ‘outf’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
519 | n = fwrite(fifo_out, 1, write_sz, outf);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
gunz_test.c:68:20: error: ‘inpf’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
68 | #define GETINPC(X) fgetc(X)
| ^~~~~
gunz_test.c:274:8: note: ‘inpf’ was declared here
274 | FILE *inpf;
| ^~~~
cc1: all warnings being treated as errors
make[2]: *** [../../lib.mk:142: /home/michael/build/adhoc/kselftest/powerpc/nx-gzip/gunz_test] Error 1
Please send a fix for those. If you just need to initialise them to 0 or
NULL at the beginning of the function that's fine by me.
cheers
diff --git a/tools/testing/selftests/powerpc/nx-gzip/Makefile b/tools/testing/selftests/powerpc/nx-gzip/Makefile
index 82abc19a49a0..387ad3857c9d 100644
--- a/tools/testing/selftests/powerpc/nx-gzip/Makefile
+++ b/tools/testing/selftests/powerpc/nx-gzip/Makefile
@@ -1,22 +1,8 @@
-CC = gcc
-CFLAGS = -O3
-INC = ./inc
-SRC = gzfht_test.c gunz_test.c
-OBJ = $(SRC:.c=.o)
-TESTS = gzfht_test gunz_test
-EXTRA_SOURCES = gzip_vas.c
+CFLAGS += -O3 -m64 -I./inc
-all: $(TESTS)
+TEST_GEN_FILES := gzfht_test gunz_test
+TEST_PROGS := nx-gzip-test.sh
-$(OBJ): %.o: %.c
- $(CC) $(CFLAGS) -I$(INC) -c $<
+include ../../lib.mk
-$(TESTS): $(OBJ)
- $(CC) $(CFLAGS) -I$(INC) -o $@ $@.o $(EXTRA_SOURCES)
-
-run_tests: $(TESTS)
- ./gzfht_test gzip_vas.c
- ./gunz_test gzip_vas.c.nx.gz
-
-clean:
- rm -f $(TESTS) *.o *~ *.gz *.gunzip
+$(TEST_GEN_FILES): gzip_vas.c
diff --git a/tools/testing/selftests/powerpc/nx-gzip/gunz_test.c b/tools/testing/selftests/powerpc/nx-gzip/gunz_test.c
index 94cb79616225..8196cf56df7a 100644
--- a/tools/testing/selftests/powerpc/nx-gzip/gunz_test.c
+++ b/tools/testing/selftests/powerpc/nx-gzip/gunz_test.c
@@ -36,6 +36,9 @@
* vas: virtual accelerator switch; the user mode interface
*/
+#define _ISOC11_SOURCE // For aligned_alloc()
+#define _DEFAULT_SOURCE // For endian.h
+
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
@@ -242,7 +245,6 @@ static int nx_touch_pages_dde(struct nx_dde_t *ddep, long buf_sz, long page_sz,
static int nx_submit_job(struct nx_dde_t *src, struct nx_dde_t *dst,
struct nx_gzip_crb_cpb_t *cmdp, void *handle)
{
- int cc;
uint64_t csbaddr;
memset((void *)&cmdp->crb.csb, 0, sizeof(cmdp->crb.csb));
diff --git a/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c b/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
index e60f743e2c6b..a3d90b2b1591 100644
--- a/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
+++ b/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
@@ -41,6 +41,8 @@
* vas: virtual accelerator switch; the user mode interface
*/
+#define _ISOC11_SOURCE // For aligned_alloc()
+#define _DEFAULT_SOURCE // For endian.h
#include <stdio.h>
#include <stdlib.h>
@@ -75,7 +77,6 @@ static int compress_fht_sample(char *src, uint32_t srclen, char *dst,
uint32_t dstlen, int with_count,
struct nx_gzip_crb_cpb_t *cmdp, void *handle)
{
- int cc;
uint32_t fc;
assert(!!cmdp);
diff --git a/tools/testing/selftests/powerpc/nx-gzip/nx-gzip-test.sh b/tools/testing/selftests/powerpc/nx-gzip/nx-gzip-test.sh
new file mode 100755
index 000000000000..896428ea7800
--- /dev/null
+++ b/tools/testing/selftests/powerpc/nx-gzip/nx-gzip-test.sh
@@ -0,0 +1,45 @@
+#!/bin/bash
+
+if [[ ! -w /dev/crypto/nx-gzip ]]; then
+ echo "Can't access /dev/crypto/nx-gzip, skipping"
+ echo "skip: $0"
+ exit 4
+fi
+
+set -e
+
+function cleanup
+{
+ rm -f nx-tempfile*
+}
+
+trap cleanup EXIT
+
+function test_sizes
+{
+ local n=$1
+ local fname="nx-tempfile.$n"
+
+ for size in 4K 64K 1M 64M
+ do
+ echo "Testing $size ($n) ..."
+ dd if=/dev/urandom of=$fname bs=$size count=1
+ ./gzfht_test $fname
+ ./gunz_test ${fname}.nx.gz
+ done
+}
+
+echo "Doing basic test of different sizes ..."
+test_sizes 0
+
+echo "Running tests in parallel ..."
+for i in {1..16}
+do
+ test_sizes $i &
+done
+
+wait
+
+echo "OK"
+
+exit 0
^ permalink raw reply related
* Re: [PATCH V3 1/5] selftests/powerpc: Add header files for GZIP engine test
From: Michael Ellerman @ 2020-04-17 5:05 UTC (permalink / raw)
To: Raphael Moreira Zinsly, linuxppc-dev, linux-crypto, dja
Cc: abali, haren, herbert, rzinsly
In-Reply-To: <20200413155916.16900-2-rzinsly@linux.ibm.com>
Hi Raphael,
Some comments below ...
Raphael Moreira Zinsly <rzinsly@linux.ibm.com> writes:
> Add files to access the powerpc NX-GZIP engine in user space.
>
> Signed-off-by: Bulent Abali <abali@us.ibm.com>
> Signed-off-by: Raphael Moreira Zinsly <rzinsly@linux.ibm.com>
> ---
> .../selftests/powerpc/nx-gzip/inc/crb.h | 159 ++++++++++++++++++
> .../selftests/powerpc/nx-gzip/inc/nx-gzip.h | 27 +++
> .../powerpc/nx-gzip/inc/nx-helpers.h | 54 ++++++
> .../selftests/powerpc/nx-gzip/inc/nx.h | 38 +++++
> 4 files changed, 278 insertions(+)
> create mode 100644 tools/testing/selftests/powerpc/nx-gzip/inc/crb.h
> create mode 100644 tools/testing/selftests/powerpc/nx-gzip/inc/nx-gzip.h
> create mode 100644 tools/testing/selftests/powerpc/nx-gzip/inc/nx-helpers.h
> create mode 100644 tools/testing/selftests/powerpc/nx-gzip/inc/nx.h
It's standard to call the directory "include", can you rename it please?
> diff --git a/tools/testing/selftests/powerpc/nx-gzip/inc/crb.h b/tools/testing/selftests/powerpc/nx-gzip/inc/crb.h
> new file mode 100644
> index 000000000000..9056e3dc1831
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/nx-gzip/inc/crb.h
> @@ -0,0 +1,159 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef __CRB_H
> +#define __CRB_H
> +#include <linux/types.h>
> +#include "nx.h"
> +
> +typedef unsigned char u8;
> +typedef unsigned int u32;
> +typedef unsigned long long u64;
The vast bulk of the code uses the stdint.h types, so it would be
preferable to either use those throughout or use the kernel types
throughout, eg. __u8, __u32, __u64, rather than defining your own here.
...
> diff --git a/tools/testing/selftests/powerpc/nx-gzip/inc/nx-gzip.h b/tools/testing/selftests/powerpc/nx-gzip/inc/nx-gzip.h
> new file mode 100644
> index 000000000000..75482c45574d
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/nx-gzip/inc/nx-gzip.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * Copyright 2020 IBM Corp.
> + *
> + */
> +
> +#ifndef _UAPI_MISC_VAS_H
> +#define _UAPI_MISC_VAS_H
That's the wrong include guard.
> +
> +#include <asm/ioctl.h>
> +
> +#define VAS_FLAGS_PIN_WINDOW 0x1
> +#define VAS_FLAGS_HIGH_PRI 0x2
> +
> +#define VAS_FTW_SETUP _IOW('v', 1, struct vas_gzip_setup_attr)
> +#define VAS_842_TX_WIN_OPEN _IOW('v', 2, struct vas_gzip_setup_attr)
> +#define VAS_GZIP_TX_WIN_OPEN _IOW('v', 0x20, struct vas_gzip_setup_attr)
> +
> +struct vas_gzip_setup_attr {
> + int32_t version;
> + int16_t vas_id;
> + int16_t reserved1;
> + int64_t flags;
> + int64_t reserved2[6];
> +};
This doesn't match the kernel header.
In fact you should just be able to symlink the uapi header.
> +#endif /* _UAPI_MISC_VAS_H */
> diff --git a/tools/testing/selftests/powerpc/nx-gzip/inc/nx-helpers.h b/tools/testing/selftests/powerpc/nx-gzip/inc/nx-helpers.h
> new file mode 100644
> index 000000000000..e0d68914c941
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/nx-gzip/inc/nx-helpers.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#include <sys/time.h>
> +#include <asm/byteorder.h>
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include "crb.h"
> +
> +#define cpu_to_be32 __cpu_to_be32
> +#define cpu_to_be64 __cpu_to_be64
> +#define be32_to_cpu __be32_to_cpu
> +#define be64_to_cpu __be64_to_cpu
> +
> +/*
> + * Several helpers/macros below were copied from the tree
> + * (kernel.h, nx-842.h, nx-ftw.h, asm-compat.h etc)
> + */
> +
> +/* from kernel.h */
> +#define IS_ALIGNED(x, a) (((x) & ((typeof(x))(a) - 1)) == 0)
> +#define __round_mask(x, y) ((__typeof__(x))((y)-1))
> +#define round_up(x, y) ((((x)-1) | __round_mask(x, y))+1)
> +#define round_down(x, y) ((x) & ~__round_mask(x, y))
Unused?
> +#define min_t(t, x, y) ((x) < (y) ? (x) : (y))
Unused?
> +/*
> + * Get/Set bit fields. (from nx-842.h)
> + */
> +#define GET_FIELD(m, v) (((v) & (m)) >> MASK_LSH(m))
> +#define MASK_LSH(m) (__builtin_ffsl(m) - 1)
> +#define SET_FIELD(m, v, val) \
> + (((v) & ~(m)) | ((((typeof(v))(val)) << MASK_LSH(m)) & (m)))
Unused?
> +
> +/* From asm-compat.h */
> +#define __stringify_in_c(...) #__VA_ARGS__
> +#define stringify_in_c(...) __stringify_in_c(__VA_ARGS__) " "
> +
> +#define pr_debug
> +#define pr_debug_ratelimited printf
> +#define pr_err printf
> +#define pr_err_ratelimited printf
> +
> +#define WARN_ON_ONCE(x) do {if (x) \
> + printf("WARNING: %s:%d\n", __func__, __LINE__)\
> + } while (0)
Unused?
> +extern void dump_buffer(char *msg, char *buf, int len);
> +extern void *alloc_aligned_mem(int len, int align, char *msg);
> +extern void get_payload(char *buf, int len);
> +extern void time_add(struct timeval *in, int seconds, struct timeval *out);
>
> +extern bool time_after(struct timeval *a, struct timeval *b);
> +extern long time_delta(struct timeval *a, struct timeval *b);
> +extern void dump_dde(struct data_descriptor_entry *dde, char *msg);
> +extern void copy_paste_crb_data(struct coprocessor_request_block *crb);
None of those externs appear to exist or be used.
> diff --git a/tools/testing/selftests/powerpc/nx-gzip/inc/nx.h b/tools/testing/selftests/powerpc/nx-gzip/inc/nx.h
> new file mode 100644
> index 000000000000..1ae8348b59d6
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/nx-gzip/inc/nx.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * Copyright 2020 IBM Corp.
> + *
> + */
> +#ifndef _NX_H
> +#define _NX_H
> +
> +#include <stdbool.h>
> +
> +#define NX_FUNC_COMP_842 1
> +#define NX_FUNC_COMP_GZIP 2
> +
> +#ifndef __aligned
> +#define __aligned(x) __attribute__((aligned(x)))
> +#endif
> +
> +struct nx842_func_args {
> + bool use_crc;
> + bool decompress; /* true decompress; false compress */
> + bool move_data;
> + int timeout; /* seconds */
> +};
> +
> +struct nxbuf_t {
> + int len;
> + char *buf;
> +};
> +
> +/* @function should be EFT (aka 842), GZIP etc */
> +extern void *nx_function_begin(int function, int pri);
> +
> +extern int nx_function(void *handle, struct nxbuf_t *in, struct nxbuf_t *out,
> + void *arg);
> +
> +extern int nx_function_end(void *handle);
You don't need extern on function declarations in headers.
> +
> +#endif /* _NX_H */
> --
> 2.21.0
cheers
^ permalink raw reply
* Re: [PATCH v4,4/4] drivers: uio: new driver for fsl_85xx_cache_sram
From: Scott Wood @ 2020-04-17 4:58 UTC (permalink / raw)
To: 王文虎, Rob Herring
Cc: gregkh, linuxppc-dev, linux-kernel, kernel
In-Reply-To: <ANcAoADRCKKtO5p9r33Ll4og.3.1587090694317.Hmail.wenhu.wang@vivo.com>
On Fri, 2020-04-17 at 10:31 +0800, 王文虎 wrote:
> > > On Thu, 2020-04-16 at 08:35 -0700, Wang Wenhu wrote:
> > > > +#define UIO_INFO_VER "devicetree,pseudo"
> > >
> > > What does this mean? Changing a number into a non-obvious string (Why
> > > "pseudo"? Why does the UIO user care that the config came from the
> > > device
> > > tree?) just to avoid setting off Greg's version number autoresponse
> > > isn't
> > > really helping anything.
> > >
> > > > +static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
> > > > + { .compatible = "uio,mpc85xx-cache-sram", },
> >
> > Form is <vendor>,<device> and "uio" is not a vendor (and never will be).
> >
>
> Should have been something like "fsl,mpc85xx-cache-sram-uio", and if it is
> to be defined with module parameters, this would be user defined.
> Anyway, <vendor>,<device> should always be used.
>
> > > > + {},
> > > > +};
> > > > +
> > > > +static struct platform_driver uio_fsl_85xx_cache_sram = {
> > > > + .probe = uio_fsl_85xx_cache_sram_probe,
> > > > + .remove = uio_fsl_85xx_cache_sram_remove,
> > > > + .driver = {
> > > > + .name = DRIVER_NAME,
> > > > + .owner = THIS_MODULE,
> > > > + .of_match_table = uio_mpc85xx_l2ctlr_of_match,
> > > > + },
> > > > +};
> > >
> > > Greg's comment notwithstanding, I really don't think this belongs in the
> > > device tree (and if I do get overruled on that point, it at least needs
> > > a
> > > binding document). Let me try to come up with a patch for dynamic
> > > allocation.
> >
> > Agreed. "UIO" bindings have long been rejected.
> >
>
> Sounds it is. And does the modification below fit well?
> ---
> -static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
> - { .compatible = "uio,mpc85xx-cache-sram", },
> - {},
> +#ifdef CONFIG_OF
> +static struct of_device_id uio_fsl_85xx_cache_sram_of_match[] = {
> + { /* This is filled with module_parm */ },
> + { /* Sentinel */ },
> };
> +MODULE_DEVICE_TABLE(of, uio_fsl_85xx_cache_sram_of_match);
> +module_param_string(of_id, uio_fsl_85xx_cache_sram_of_match[0].compatible,
> + sizeof(uio_fsl_85xx_cache_sram_of_match[0].compa
> tible), 0);
> +MODULE_PARM_DESC(of_id, "platform device id to be handled by cache-sram-
> uio");
> +#endif
No. The point is that you wouldn't be configuring this with the device tree
at all.
-Scott
^ permalink raw reply
* Re: [PATCH v5 3/9] powerpc/vas: Add VAS user space API
From: Michael Ellerman @ 2020-04-17 4:55 UTC (permalink / raw)
To: Haren Myneni
Cc: mikey, herbert, npiggin, linux-crypto, sukadev, linuxppc-dev, dja
In-Reply-To: <1585778365.10664.479.camel@hbabu-laptop>
Haren Myneni <haren@linux.ibm.com> writes:
> On power9, user space can send GZIP compression requests directly to NX
> once kernel establishes NX channel / window with VAS. This patch provides
> user space API which allows user space to establish channel using open
> VAS_TX_WIN_OPEN ioctl, mmap and close operations.
>
> Each window corresponds to file descriptor and application can open
> multiple windows. After the window is opened, VAS_TX_WIN_OPEN icoctl to
> open a window on specific VAS instance, mmap() system call to map
> the hardware address of engine's request queue into the application's
> virtual address space.
>
> Then the application can then submit one or more requests to the the
> engine by using the copy/paste instructions and pasting the CRBs to
> the virtual address (aka paste_address) returned by mmap().
>
> Only NX GZIP coprocessor type is supported right now and allow GZIP
> engine access via /dev/crypto/nx-gzip device node.
>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
> arch/powerpc/include/asm/vas.h | 11 ++
> arch/powerpc/platforms/powernv/Makefile | 2 +-
> arch/powerpc/platforms/powernv/vas-api.c | 257 ++++++++++++++++++++++++++++
> arch/powerpc/platforms/powernv/vas-window.c | 6 +-
> arch/powerpc/platforms/powernv/vas.h | 2 +
> 5 files changed, 274 insertions(+), 4 deletions(-)
> create mode 100644 arch/powerpc/platforms/powernv/vas-api.c
>
> diff --git a/arch/powerpc/include/asm/vas.h b/arch/powerpc/include/asm/vas.h
> index f93e6b0..e064953 100644
> --- a/arch/powerpc/include/asm/vas.h
> +++ b/arch/powerpc/include/asm/vas.h
> @@ -163,4 +163,15 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop,
> */
> int vas_paste_crb(struct vas_window *win, int offset, bool re);
>
> +/*
> + * Register / unregister coprocessor type to VAS API which will be exported
> + * to user space. Applications can use this API to open / close window
> + * which can be used to send / receive requests directly to cooprcessor.
> + *
> + * Only NX GZIP coprocessor type is supported now, but this API can be
> + * used for others in future.
> + */
> +int vas_register_coproc_api(struct module *mod);
> +void vas_unregister_coproc_api(void);
> +
> #endif /* __ASM_POWERPC_VAS_H */
> diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
> index 395789f..fe3f0fb 100644
> --- a/arch/powerpc/platforms/powernv/Makefile
> +++ b/arch/powerpc/platforms/powernv/Makefile
> @@ -17,7 +17,7 @@ obj-$(CONFIG_MEMORY_FAILURE) += opal-memory-errors.o
> obj-$(CONFIG_OPAL_PRD) += opal-prd.o
> obj-$(CONFIG_PERF_EVENTS) += opal-imc.o
> obj-$(CONFIG_PPC_MEMTRACE) += memtrace.o
> -obj-$(CONFIG_PPC_VAS) += vas.o vas-window.o vas-debug.o vas-fault.o
> +obj-$(CONFIG_PPC_VAS) += vas.o vas-window.o vas-debug.o vas-fault.o vas-api.o
> obj-$(CONFIG_OCXL_BASE) += ocxl.o
> obj-$(CONFIG_SCOM_DEBUGFS) += opal-xscom.o
> obj-$(CONFIG_PPC_SECURE_BOOT) += opal-secvar.o
> diff --git a/arch/powerpc/platforms/powernv/vas-api.c b/arch/powerpc/platforms/powernv/vas-api.c
> new file mode 100644
> index 0000000..7d049af
> --- /dev/null
> +++ b/arch/powerpc/platforms/powernv/vas-api.c
> @@ -0,0 +1,257 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * VAS user space API for its accelerators (Only NX-GZIP is supported now)
> + * Copyright (C) 2019 Haren Myneni, IBM Corp
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/cdev.h>
> +#include <linux/fs.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <asm/vas.h>
> +#include <uapi/asm/vas-api.h>
> +#include "vas.h"
> +
> +/*
> + * The driver creates the device node that can be used as follows:
> + * For NX-GZIP
> + *
> + * fd = open("/dev/crypto/nx-gzip", O_RDWR);
> + * rc = ioctl(fd, VAS_TX_WIN_OPEN, &attr);
> + * paste_addr = mmap(NULL, PAGE_SIZE, prot, MAP_SHARED, fd, 0ULL).
> + * vas_copy(&crb, 0, 1);
> + * vas_paste(paste_addr, 0, 1);
> + * close(fd) or exit process to close window.
> + *
> + * where "vas_copy" and "vas_paste" are defined in copy-paste.h.
> + * copy/paste returns to the user space directly. So refer NX hardware
> + * documententation for exact copy/paste usage and completion / error
> + * conditions.
> + */
> +
> +static char *coproc_dev_name = "nx-gzip";
> +
> +/*
> + * Wrapper object for the nx-gzip device - there is just one instance of
> + * this node for the whole system.
> + */
> +static struct coproc_dev {
> + struct cdev cdev;
> + struct device *device;
> + char *name;
> + dev_t devt;
> + struct class *class;
> +} coproc_device;
> +
> +static char *coproc_devnode(struct device *dev, umode_t *mode)
> +{
> + return kasprintf(GFP_KERNEL, "crypto/%s", dev_name(dev));
> +}
> +
> +static int coproc_open(struct inode *inode, struct file *fp)
> +{
> + /*
> + * vas_window is allocated and assigned to fp->private_data
> + * in ioctl. Nothing to do here for NX GZIP.
> + */
> + return 0;
> +}
> +
> +static int coproc_ioc_tx_win_open(struct file *fp, unsigned long arg)
> +{
> + void __user *uptr = (void __user *)arg;
> + struct vas_tx_win_attr txattr = {};
> + struct vas_tx_win_open_attr uattr;
> + struct vas_window *txwin;
> + int rc, vasid;
> +
> + /*
> + * One window for file descriptor
> + */
> + if (fp->private_data)
> + return -EEXIST;
> +
> + rc = copy_from_user(&uattr, uptr, sizeof(uattr));
> + if (rc) {
> + pr_err("%s(): copy_from_user() returns %d\n", __func__, rc);
> + return -EFAULT;
> + }
> +
> + if (uattr.version != 1) {
> + pr_err("Invalid version\n");
> + return -EINVAL;
> + }
> +
> + vasid = uattr.vas_id;
> +
> + vas_init_tx_win_attr(&txattr, VAS_COP_TYPE_GZIP);
We shouldn't be hard coding GZIP in here.
I don't mind if you have a single coproc_dev for now, but we should at
least be passing the type etc. in from the driver.
Something like the patch below should work. Please test.
cheers
diff --git a/arch/powerpc/include/asm/vas.h b/arch/powerpc/include/asm/vas.h
index 994db6f41a19..5fa65a5af095 100644
--- a/arch/powerpc/include/asm/vas.h
+++ b/arch/powerpc/include/asm/vas.h
@@ -170,7 +170,8 @@ int vas_paste_crb(struct vas_window *win, int offset, bool re);
* Only NX GZIP coprocessor type is supported now, but this API can be
* used for others in future.
*/
-int vas_register_coproc_api(struct module *mod);
+int vas_register_coproc_api(struct module *mod, enum vas_cop_type cop_type,
+ const char *name);
void vas_unregister_coproc_api(void);
#endif /* __ASM_POWERPC_VAS_H */
diff --git a/arch/powerpc/platforms/powernv/vas-api.c b/arch/powerpc/platforms/powernv/vas-api.c
index 7d049afde63a..3537a531bec1 100644
--- a/arch/powerpc/platforms/powernv/vas-api.c
+++ b/arch/powerpc/platforms/powernv/vas-api.c
@@ -31,7 +31,6 @@
* conditions.
*/
-static char *coproc_dev_name = "nx-gzip";
/*
* Wrapper object for the nx-gzip device - there is just one instance of
@@ -43,8 +42,14 @@ static struct coproc_dev {
char *name;
dev_t devt;
struct class *class;
+ enum vas_cop_type cop_type;
} coproc_device;
+struct coproc_instance {
+ struct coproc_dev *coproc;
+ struct vas_window *txwin;
+};
+
static char *coproc_devnode(struct device *dev, umode_t *mode)
{
return kasprintf(GFP_KERNEL, "crypto/%s", dev_name(dev));
@@ -52,10 +57,15 @@ static char *coproc_devnode(struct device *dev, umode_t *mode)
static int coproc_open(struct inode *inode, struct file *fp)
{
- /*
- * vas_window is allocated and assigned to fp->private_data
- * in ioctl. Nothing to do here for NX GZIP.
- */
+ struct coproc_instance *cp_inst;
+
+ cp_inst = kzalloc(sizeof(*cp_inst), GFP_KERNEL);
+ if (!cp_inst)
+ return -ENOMEM;
+
+ cp_inst->coproc = container_of(inode->i_cdev, struct coproc_dev, cdev);
+ fp->private_data = cp_inst;
+
return 0;
}
@@ -64,13 +74,16 @@ static int coproc_ioc_tx_win_open(struct file *fp, unsigned long arg)
void __user *uptr = (void __user *)arg;
struct vas_tx_win_attr txattr = {};
struct vas_tx_win_open_attr uattr;
+ struct coproc_instance *cp_inst;
struct vas_window *txwin;
int rc, vasid;
+ cp_inst = fp->private_data;
+
/*
* One window for file descriptor
*/
- if (fp->private_data)
+ if (cp_inst->txwin)
return -EEXIST;
rc = copy_from_user(&uattr, uptr, sizeof(uattr));
@@ -86,7 +99,7 @@ static int coproc_ioc_tx_win_open(struct file *fp, unsigned long arg)
vasid = uattr.vas_id;
- vas_init_tx_win_attr(&txattr, VAS_COP_TYPE_GZIP);
+ vas_init_tx_win_attr(&txattr, cp_inst->coproc->cop_type);
txattr.lpid = mfspr(SPRN_LPID);
txattr.pidr = mfspr(SPRN_PID);
@@ -97,27 +110,30 @@ static int coproc_ioc_tx_win_open(struct file *fp, unsigned long arg)
pr_devel("Pid %d: Opening txwin, PIDR %ld\n", txattr.pidr,
mfspr(SPRN_PID));
- txwin = vas_tx_win_open(vasid, VAS_COP_TYPE_GZIP, &txattr);
+ txwin = vas_tx_win_open(vasid, cp_inst->coproc->cop_type, &txattr);
if (IS_ERR(txwin)) {
pr_err("%s() vas_tx_win_open() failed, %ld\n", __func__,
PTR_ERR(txwin));
return PTR_ERR(txwin);
}
- fp->private_data = txwin;
+ cp_inst->txwin = txwin;
return 0;
}
static int coproc_release(struct inode *inode, struct file *fp)
{
- struct vas_window *txwin = fp->private_data;
+ struct coproc_instance *cp_inst = fp->private_data;
- if (txwin) {
- vas_win_close(txwin);
- fp->private_data = NULL;
+ if (cp_inst->txwin) {
+ vas_win_close(cp_inst->txwin);
+ cp_inst->txwin = NULL;
}
+ kfree(cp_inst);
+ fp->private_data = NULL;
+
/*
* We don't know here if user has other receive windows
* open, so we can't really call clear_thread_tidr().
@@ -131,12 +147,15 @@ static int coproc_release(struct inode *inode, struct file *fp)
static int coproc_mmap(struct file *fp, struct vm_area_struct *vma)
{
- struct vas_window *txwin = fp->private_data;
+ struct coproc_instance *cp_inst = fp->private_data;
+ struct vas_window *txwin;
unsigned long pfn;
u64 paste_addr;
pgprot_t prot;
int rc;
+ txwin = cp_inst->txwin;
+
if ((vma->vm_end - vma->vm_start) > PAGE_SIZE) {
pr_debug("%s(): size 0x%zx, PAGE_SIZE 0x%zx\n", __func__,
(vma->vm_end - vma->vm_start), PAGE_SIZE);
@@ -188,28 +207,30 @@ static struct file_operations coproc_fops = {
* Supporting only nx-gzip coprocessor type now, but this API code
* extended to other coprocessor types later.
*/
-int vas_register_coproc_api(struct module *mod)
+int vas_register_coproc_api(struct module *mod, enum vas_cop_type cop_type,
+ const char *name)
{
int rc = -EINVAL;
dev_t devno;
- rc = alloc_chrdev_region(&coproc_device.devt, 1, 1, coproc_dev_name);
+ rc = alloc_chrdev_region(&coproc_device.devt, 1, 1, name);
if (rc) {
pr_err("Unable to allocate coproc major number: %i\n", rc);
return rc;
}
- pr_devel("%s device allocated, dev [%i,%i]\n", coproc_dev_name,
- MAJOR(coproc_device.devt), MINOR(coproc_device.devt));
+ pr_devel("%s device allocated, dev [%i,%i]\n", name,
+ MAJOR(coproc_device.devt), MINOR(coproc_device.devt));
- coproc_device.class = class_create(mod, coproc_dev_name);
+ coproc_device.class = class_create(mod, name);
if (IS_ERR(coproc_device.class)) {
rc = PTR_ERR(coproc_device.class);
- pr_err("Unable to create %s class %d\n", coproc_dev_name, rc);
+ pr_err("Unable to create %s class %d\n", name, rc);
goto err_class;
}
coproc_device.class->devnode = coproc_devnode;
+ coproc_device.cop_type = cop_type;
coproc_fops.owner = mod;
cdev_init(&coproc_device.cdev, &coproc_fops);
@@ -221,7 +242,7 @@ int vas_register_coproc_api(struct module *mod)
}
coproc_device.device = device_create(coproc_device.class, NULL,
- devno, NULL, coproc_dev_name, MINOR(devno));
+ devno, NULL, name, MINOR(devno));
if (IS_ERR(coproc_device.device)) {
rc = PTR_ERR(coproc_device.device);
pr_err("Unable to create coproc-%d %d\n", MINOR(devno), rc);
diff --git a/drivers/crypto/nx/nx-common-powernv.c b/drivers/crypto/nx/nx-common-powernv.c
index 38333e475097..1979fa6055de 100644
--- a/drivers/crypto/nx/nx-common-powernv.c
+++ b/drivers/crypto/nx/nx-common-powernv.c
@@ -1088,7 +1088,7 @@ static __init int nx_compress_powernv_init(void)
* that user space can use GZIP engine.
* 842 compression is supported only in kernel.
*/
- ret = vas_register_coproc_api(THIS_MODULE);
+ ret = vas_register_coproc_api(THIS_MODULE, VAS_COP_TYPE_GZIP, "nx-gzip");
/*
* GZIP is not supported in kernel right now.
^ permalink raw reply related
* [PATCH] powerpc/pseries: Make vio and ibmebus initcalls pseries specific
From: Oliver O'Halloran @ 2020-04-17 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Oliver O'Halloran
The vio and ibmebus buses are used for pseries specific paravirtualised
devices and currently they're initialised by the generic initcall types.
This is mostly fine, but it can result in some nuisance errors in dmesg
when booting on PowerNV on some OSes, e.g.
[ 2.984439] synth uevent: /devices/vio: failed to send uevent
[ 2.984442] vio vio: uevent: failed to send synthetic uevent
[ 17.968551] synth uevent: /devices/vio: failed to send uevent
[ 17.968554] vio vio: uevent: failed to send synthetic uevent
We don't see anything similar for the ibmebus because that depends on
!CONFIG_LITTLE_ENDIAN.
This patch squashes those by switching to using machine_*_initcall() so the bus
type is only registered when the kernel is running on a pseries machine.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/platforms/pseries/ibmebus.c | 2 +-
arch/powerpc/platforms/pseries/vio.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/ibmebus.c b/arch/powerpc/platforms/pseries/ibmebus.c
index b91eb09..4cd9e65 100644
--- a/arch/powerpc/platforms/pseries/ibmebus.c
+++ b/arch/powerpc/platforms/pseries/ibmebus.c
@@ -464,4 +464,4 @@ static int __init ibmebus_bus_init(void)
return 0;
}
-postcore_initcall(ibmebus_bus_init);
+machine_postcore_initcall(pseries, ibmebus_bus_init);
diff --git a/arch/powerpc/platforms/pseries/vio.c b/arch/powerpc/platforms/pseries/vio.c
index 37f1f25..33be37e 100644
--- a/arch/powerpc/platforms/pseries/vio.c
+++ b/arch/powerpc/platforms/pseries/vio.c
@@ -1513,7 +1513,7 @@ static int __init vio_bus_init(void)
return 0;
}
-postcore_initcall(vio_bus_init);
+machine_postcore_initcall(pseries, vio_bus_init);
static int __init vio_device_init(void)
{
@@ -1522,7 +1522,7 @@ static int __init vio_device_init(void)
return 0;
}
-device_initcall(vio_device_init);
+machine_device_initcall(pseries, vio_device_init);
static ssize_t name_show(struct device *dev,
struct device_attribute *attr, char *buf)
@@ -1703,4 +1703,4 @@ static int __init vio_init(void)
dma_debug_add_bus(&vio_bus_type);
return 0;
}
-fs_initcall(vio_init);
+machine_fs_initcall(pseries, vio_init);
--
2.9.5
^ permalink raw reply related
* Re: [PATCH V2] vhost: do not enable VHOST_MENU by default
From: Jason Wang @ 2020-04-17 3:12 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-s390, tsbogend, gor, kvm, linux-kernel, heiko.carstens,
linux-mips, virtualization, borntraeger, geert, netdev, paulus,
linuxppc-dev
In-Reply-To: <20200416185426-mutt-send-email-mst@kernel.org>
On 2020/4/17 上午6:55, Michael S. Tsirkin wrote:
> On Wed, Apr 15, 2020 at 10:43:56AM +0800, Jason Wang wrote:
>> We try to keep the defconfig untouched after decoupling CONFIG_VHOST
>> out of CONFIG_VIRTUALIZATION in commit 20c384f1ea1a
>> ("vhost: refine vhost and vringh kconfig") by enabling VHOST_MENU by
>> default. Then the defconfigs can keep enabling CONFIG_VHOST_NET
>> without the caring of CONFIG_VHOST.
>>
>> But this will leave a "CONFIG_VHOST_MENU=y" in all defconfigs and even
>> for the ones that doesn't want vhost. So it actually shifts the
>> burdens to the maintainers of all other to add "CONFIG_VHOST_MENU is
>> not set". So this patch tries to enable CONFIG_VHOST explicitly in
>> defconfigs that enables CONFIG_VHOST_NET and CONFIG_VHOST_VSOCK.
>>
>> Acked-by: Christian Borntraeger<borntraeger@de.ibm.com> (s390)
>> Acked-by: Michael Ellerman<mpe@ellerman.id.au> (powerpc)
>> Cc: Thomas Bogendoerfer<tsbogend@alpha.franken.de>
>> Cc: Benjamin Herrenschmidt<benh@kernel.crashing.org>
>> Cc: Paul Mackerras<paulus@samba.org>
>> Cc: Michael Ellerman<mpe@ellerman.id.au>
>> Cc: Heiko Carstens<heiko.carstens@de.ibm.com>
>> Cc: Vasily Gorbik<gor@linux.ibm.com>
>> Cc: Christian Borntraeger<borntraeger@de.ibm.com>
>> Reported-by: Geert Uytterhoeven<geert@linux-m68k.org>
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
> I rebased this on top of OABI fix since that
> seems more orgent to fix.
> Pushed to my vhost branch pls take a look and
> if possible test.
> Thanks!
I test this patch by generating the defconfigs that wants vhost_net or
vhost_vsock. All looks fine.
But having CONFIG_VHOST_DPN=y may end up with the similar situation that
this patch want to address.
Maybe we can let CONFIG_VHOST depends on !ARM || AEABI then add another
menuconfig for VHOST_RING and do something similar?
Thanks
^ permalink raw reply
* Re: POWER9 crash due to STRICT_KERNEL_RWX (WAS: Re: Linux-next POWER9 NULL pointer NIP...)
From: Qian Cai @ 2020-04-17 3:16 UTC (permalink / raw)
To: Russell Currey; +Cc: linuxppc-dev, Nicholas Piggin, Steven Rostedt, LKML
In-Reply-To: <fec2e755ea20e15dc5b6fee6c856562aa42872bd.camel@russell.cc>
> On Apr 16, 2020, at 10:46 PM, Russell Currey <ruscur@russell.cc> wrote:
>
> On Thu, 2020-04-16 at 22:40 -0400, Qian Cai wrote:
>>> On Apr 16, 2020, at 10:27 PM, Russell Currey <ruscur@russell.cc>
>>> wrote:
>>>
>>> Reverting the patch with the given config will have the same effect
>>> as
>>> STRICT_KERNEL_RWX=n. Not discounting that it could be a bug on the
>>> powerpc side (i.e. relocatable kernels with strict RWX on haven't
>>> been
>>> exhaustively tested yet), but we should definitely figure out
>>> what's
>>> going on with this bad access first.
>>
>> BTW, this bad access only happened once. The overwhelming rest of
>> crashes are with NULL pointer NIP like below. How can you explain
>> that STRICT_KERNEL_RWX=n would also make those NULL NIP disappear if
>> STRICT_KERNEL_RWX is just a messenger?
>
> What happens if you test with STRICT_KERNEL_RWX=y and RELOCATABLE=n,
> reverting my patch? This would give us an idea of whether it's
> something broken recently or if there's something else going on.
I don’t know what did you mean by reverting your patch because that combination
can be tested as-is. Anyway, it could take a long time to reproduce, so I’ll keep it
running for up to 12-hour to confirm it could not really crash.
>
>>
>> [ 215.281666][T16896] LTP: starting chown04_16
>> [ 215.424203][T18297] BUG: Unable to handle kernel instruction fetch
>> (NULL pointer?)
>> [ 215.424289][T18297] Faulting instruction address: 0x00000000
>> [ 215.424313][T18297] Oops: Kernel access of bad area, sig: 11 [#1]
>> [ 215.424341][T18297] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=256
>> DEBUG_PAGEALLOC NUMA PowerNV
>> [ 215.424383][T18297] Modules linked in: loop kvm_hv kvm ip_tables
>> x_tables xfs sd_mod bnx2x mdio tg3 ahci libahci libphy libata
>> firmware_class dm_mirror dm_region_hash dm_log dm_mod
>> [ 215.424459][T18297] CPU: 85 PID: 18297 Comm: chown04_16 Tainted:
>> G W 5.6.0-next-20200405+ #3
>> [ 215.424489][T18297] NIP: 0000000000000000 LR: c00800000fbc0408
>> CTR: 0000000000000000
>> [ 215.424530][T18297] REGS: c000200b8606f990 TRAP: 0400 Tainted:
>> G W (5.6.0-next-20200405+)
>> [ 215.424570][T18297] MSR: 9000000040009033
>> <SF,HV,EE,ME,IR,DR,RI,LE> CR: 84000248 XER: 20040000
>> [ 215.424619][T18297] CFAR: c00800000fbc64f4 IRQMASK: 0
>> [ 215.424619][T18297] GPR00: c0000000006c2238 c000200b8606fc20
>> c00000000165ce00 0000000000000000
>> [ 215.424619][T18297] GPR04: c000201a58106400 c000200b8606fcc0
>> 000000005f037e7d ffffffff00013bfb
>> [ 215.424619][T18297] GPR08: c000201a58106400 0000000000000000
>> 0000000000000000 c000000001652ee0
>> [ 215.424619][T18297] GPR12: 0000000000000000 c000201fff69a600
>> 0000000000000000 0000000000000000
>> [ 215.424619][T18297] GPR16: 0000000000000000 0000000000000000
>> 0000000000000000 0000000000000000
>> [ 215.424619][T18297] GPR20: 0000000000000000 0000000000000000
>> 0000000000000000 0000000000000007
>> [ 215.424619][T18297] GPR24: 0000000000000000 0000000000000000
>> c00800000fbc8688 c000200b8606fcc0
>> [ 215.424619][T18297] GPR28: 0000000000000000 000000007fffffff
>> c00800000fbc0400 c00020068b8c0e70
>> [ 215.424914][T18297] NIP [0000000000000000] 0x0
>> [ 215.424953][T18297] LR [c00800000fbc0408] find_free_cb+0x8/0x30
>> [loop]
>> find_free_cb at drivers/block/loop.c:2129
>> [ 215.424997][T18297] Call Trace:
>> [ 215.425036][T18297] [c000200b8606fc20] [c0000000006c2290]
>> idr_for_each+0xf0/0x170 (unreliable)
>> [ 215.425073][T18297] [c000200b8606fca0] [c00800000fbc2744]
>> loop_lookup.part.2+0x4c/0xb0 [loop]
>> loop_lookup at drivers/block/loop.c:2144
>> [ 215.425105][T18297] [c000200b8606fce0] [c00800000fbc3558]
>> loop_control_ioctl+0x120/0x1d0 [loop]
>> [ 215.425149][T18297] [c000200b8606fd40] [c0000000004eb688]
>> ksys_ioctl+0xd8/0x130
>> [ 215.425190][T18297] [c000200b8606fd90] [c0000000004eb708]
>> sys_ioctl+0x28/0x40
>> [ 215.425233][T18297] [c000200b8606fdb0] [c00000000003cc30]
>> system_call_exception+0x110/0x1e0
>> [ 215.425274][T18297] [c000200b8606fe20] [c00000000000c9f0]
>> system_call_common+0xf0/0x278
>> [ 215.425314][T18297] Instruction dump:
>> [ 215.425338][T18297] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
>> XXXXXXXX XXXXXXXX XXXXXXXX
>> [ 215.425374][T18297] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
>> XXXXXXXX XXXXXXXX XXXXXXXX
>> [ 215.425422][T18297] ---[ end trace ebed248fad431966 ]---
>> [ 215.642114][T18297]
>> [ 216.642220][T18297] Kernel panic - not syncing: Fatal exception
^ permalink raw reply
* Re: POWER9 crash due to STRICT_KERNEL_RWX (WAS: Re: Linux-next POWER9 NULL pointer NIP...)
From: Russell Currey @ 2020-04-17 2:46 UTC (permalink / raw)
To: Qian Cai; +Cc: linuxppc-dev, Nicholas Piggin, Steven Rostedt, LKML
In-Reply-To: <43EE54C0-6F20-4ADD-9948-21F24D90C5E1@lca.pw>
On Thu, 2020-04-16 at 22:40 -0400, Qian Cai wrote:
> > On Apr 16, 2020, at 10:27 PM, Russell Currey <ruscur@russell.cc>
> > wrote:
> >
> > Reverting the patch with the given config will have the same effect
> > as
> > STRICT_KERNEL_RWX=n. Not discounting that it could be a bug on the
> > powerpc side (i.e. relocatable kernels with strict RWX on haven't
> > been
> > exhaustively tested yet), but we should definitely figure out
> > what's
> > going on with this bad access first.
>
> BTW, this bad access only happened once. The overwhelming rest of
> crashes are with NULL pointer NIP like below. How can you explain
> that STRICT_KERNEL_RWX=n would also make those NULL NIP disappear if
> STRICT_KERNEL_RWX is just a messenger?
What happens if you test with STRICT_KERNEL_RWX=y and RELOCATABLE=n,
reverting my patch? This would give us an idea of whether it's
something broken recently or if there's something else going on.
>
> [ 215.281666][T16896] LTP: starting chown04_16
> [ 215.424203][T18297] BUG: Unable to handle kernel instruction fetch
> (NULL pointer?)
> [ 215.424289][T18297] Faulting instruction address: 0x00000000
> [ 215.424313][T18297] Oops: Kernel access of bad area, sig: 11 [#1]
> [ 215.424341][T18297] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=256
> DEBUG_PAGEALLOC NUMA PowerNV
> [ 215.424383][T18297] Modules linked in: loop kvm_hv kvm ip_tables
> x_tables xfs sd_mod bnx2x mdio tg3 ahci libahci libphy libata
> firmware_class dm_mirror dm_region_hash dm_log dm_mod
> [ 215.424459][T18297] CPU: 85 PID: 18297 Comm: chown04_16 Tainted:
> G W 5.6.0-next-20200405+ #3
> [ 215.424489][T18297] NIP: 0000000000000000 LR: c00800000fbc0408
> CTR: 0000000000000000
> [ 215.424530][T18297] REGS: c000200b8606f990 TRAP: 0400 Tainted:
> G W (5.6.0-next-20200405+)
> [ 215.424570][T18297] MSR: 9000000040009033
> <SF,HV,EE,ME,IR,DR,RI,LE> CR: 84000248 XER: 20040000
> [ 215.424619][T18297] CFAR: c00800000fbc64f4 IRQMASK: 0
> [ 215.424619][T18297] GPR00: c0000000006c2238 c000200b8606fc20
> c00000000165ce00 0000000000000000
> [ 215.424619][T18297] GPR04: c000201a58106400 c000200b8606fcc0
> 000000005f037e7d ffffffff00013bfb
> [ 215.424619][T18297] GPR08: c000201a58106400 0000000000000000
> 0000000000000000 c000000001652ee0
> [ 215.424619][T18297] GPR12: 0000000000000000 c000201fff69a600
> 0000000000000000 0000000000000000
> [ 215.424619][T18297] GPR16: 0000000000000000 0000000000000000
> 0000000000000000 0000000000000000
> [ 215.424619][T18297] GPR20: 0000000000000000 0000000000000000
> 0000000000000000 0000000000000007
> [ 215.424619][T18297] GPR24: 0000000000000000 0000000000000000
> c00800000fbc8688 c000200b8606fcc0
> [ 215.424619][T18297] GPR28: 0000000000000000 000000007fffffff
> c00800000fbc0400 c00020068b8c0e70
> [ 215.424914][T18297] NIP [0000000000000000] 0x0
> [ 215.424953][T18297] LR [c00800000fbc0408] find_free_cb+0x8/0x30
> [loop]
> find_free_cb at drivers/block/loop.c:2129
> [ 215.424997][T18297] Call Trace:
> [ 215.425036][T18297] [c000200b8606fc20] [c0000000006c2290]
> idr_for_each+0xf0/0x170 (unreliable)
> [ 215.425073][T18297] [c000200b8606fca0] [c00800000fbc2744]
> loop_lookup.part.2+0x4c/0xb0 [loop]
> loop_lookup at drivers/block/loop.c:2144
> [ 215.425105][T18297] [c000200b8606fce0] [c00800000fbc3558]
> loop_control_ioctl+0x120/0x1d0 [loop]
> [ 215.425149][T18297] [c000200b8606fd40] [c0000000004eb688]
> ksys_ioctl+0xd8/0x130
> [ 215.425190][T18297] [c000200b8606fd90] [c0000000004eb708]
> sys_ioctl+0x28/0x40
> [ 215.425233][T18297] [c000200b8606fdb0] [c00000000003cc30]
> system_call_exception+0x110/0x1e0
> [ 215.425274][T18297] [c000200b8606fe20] [c00000000000c9f0]
> system_call_common+0xf0/0x278
> [ 215.425314][T18297] Instruction dump:
> [ 215.425338][T18297] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
> XXXXXXXX XXXXXXXX XXXXXXXX
> [ 215.425374][T18297] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
> XXXXXXXX XXXXXXXX XXXXXXXX
> [ 215.425422][T18297] ---[ end trace ebed248fad431966 ]---
> [ 215.642114][T18297]
> [ 216.642220][T18297] Kernel panic - not syncing: Fatal exception
^ permalink raw reply
* Re: POWER9 crash due to STRICT_KERNEL_RWX (WAS: Re: Linux-next POWER9 NULL pointer NIP...)
From: Qian Cai @ 2020-04-17 2:40 UTC (permalink / raw)
To: Russell Currey; +Cc: linuxppc-dev, Nicholas Piggin, Steven Rostedt, LKML
> On Apr 16, 2020, at 10:27 PM, Russell Currey <ruscur@russell.cc> wrote:
>
> Reverting the patch with the given config will have the same effect as
> STRICT_KERNEL_RWX=n. Not discounting that it could be a bug on the
> powerpc side (i.e. relocatable kernels with strict RWX on haven't been
> exhaustively tested yet), but we should definitely figure out what's
> going on with this bad access first.
BTW, this bad access only happened once. The overwhelming rest of crashes are with NULL pointer NIP like below. How can you explain that STRICT_KERNEL_RWX=n would also make those NULL NIP disappear if STRICT_KERNEL_RWX is just a messenger?
[ 215.281666][T16896] LTP: starting chown04_16
[ 215.424203][T18297] BUG: Unable to handle kernel instruction fetch (NULL pointer?)
[ 215.424289][T18297] Faulting instruction address: 0x00000000
[ 215.424313][T18297] Oops: Kernel access of bad area, sig: 11 [#1]
[ 215.424341][T18297] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=256 DEBUG_PAGEALLOC NUMA PowerNV
[ 215.424383][T18297] Modules linked in: loop kvm_hv kvm ip_tables x_tables xfs sd_mod bnx2x mdio tg3 ahci libahci libphy libata firmware_class dm_mirror dm_region_hash dm_log dm_mod
[ 215.424459][T18297] CPU: 85 PID: 18297 Comm: chown04_16 Tainted: G W 5.6.0-next-20200405+ #3
[ 215.424489][T18297] NIP: 0000000000000000 LR: c00800000fbc0408 CTR: 0000000000000000
[ 215.424530][T18297] REGS: c000200b8606f990 TRAP: 0400 Tainted: G W (5.6.0-next-20200405+)
[ 215.424570][T18297] MSR: 9000000040009033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 84000248 XER: 20040000
[ 215.424619][T18297] CFAR: c00800000fbc64f4 IRQMASK: 0
[ 215.424619][T18297] GPR00: c0000000006c2238 c000200b8606fc20 c00000000165ce00 0000000000000000
[ 215.424619][T18297] GPR04: c000201a58106400 c000200b8606fcc0 000000005f037e7d ffffffff00013bfb
[ 215.424619][T18297] GPR08: c000201a58106400 0000000000000000 0000000000000000 c000000001652ee0
[ 215.424619][T18297] GPR12: 0000000000000000 c000201fff69a600 0000000000000000 0000000000000000
[ 215.424619][T18297] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[ 215.424619][T18297] GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000007
[ 215.424619][T18297] GPR24: 0000000000000000 0000000000000000 c00800000fbc8688 c000200b8606fcc0
[ 215.424619][T18297] GPR28: 0000000000000000 000000007fffffff c00800000fbc0400 c00020068b8c0e70
[ 215.424914][T18297] NIP [0000000000000000] 0x0
[ 215.424953][T18297] LR [c00800000fbc0408] find_free_cb+0x8/0x30 [loop]
find_free_cb at drivers/block/loop.c:2129
[ 215.424997][T18297] Call Trace:
[ 215.425036][T18297] [c000200b8606fc20] [c0000000006c2290] idr_for_each+0xf0/0x170 (unreliable)
[ 215.425073][T18297] [c000200b8606fca0] [c00800000fbc2744] loop_lookup.part.2+0x4c/0xb0 [loop]
loop_lookup at drivers/block/loop.c:2144
[ 215.425105][T18297] [c000200b8606fce0] [c00800000fbc3558] loop_control_ioctl+0x120/0x1d0 [loop]
[ 215.425149][T18297] [c000200b8606fd40] [c0000000004eb688] ksys_ioctl+0xd8/0x130
[ 215.425190][T18297] [c000200b8606fd90] [c0000000004eb708] sys_ioctl+0x28/0x40
[ 215.425233][T18297] [c000200b8606fdb0] [c00000000003cc30] system_call_exception+0x110/0x1e0
[ 215.425274][T18297] [c000200b8606fe20] [c00000000000c9f0] system_call_common+0xf0/0x278
[ 215.425314][T18297] Instruction dump:
[ 215.425338][T18297] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
[ 215.425374][T18297] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
[ 215.425422][T18297] ---[ end trace ebed248fad431966 ]---
[ 215.642114][T18297]
[ 216.642220][T18297] Kernel panic - not syncing: Fatal exception
^ permalink raw reply
* Re: [PATCH v4,4/4] drivers: uio: new driver for fsl_85xx_cache_sram
From: 王文虎 @ 2020-04-17 2:31 UTC (permalink / raw)
To: Rob Herring; +Cc: gregkh, linux-kernel, Scott Wood, kernel, linuxppc-dev
In-Reply-To: <20200416213535.GA2511@bogus>
>> On Thu, 2020-04-16 at 08:35 -0700, Wang Wenhu wrote:
>> > +#define UIO_INFO_VER "devicetree,pseudo"
>>
>> What does this mean? Changing a number into a non-obvious string (Why
>> "pseudo"? Why does the UIO user care that the config came from the device
>> tree?) just to avoid setting off Greg's version number autoresponse isn't
>> really helping anything.
>>
>> > +static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
>> > + { .compatible = "uio,mpc85xx-cache-sram", },
>
>Form is <vendor>,<device> and "uio" is not a vendor (and never will be).
>
Should have been something like "fsl,mpc85xx-cache-sram-uio", and if it is
to be defined with module parameters, this would be user defined.
Anyway, <vendor>,<device> should always be used.
>> > + {},
>> > +};
>> > +
>> > +static struct platform_driver uio_fsl_85xx_cache_sram = {
>> > + .probe = uio_fsl_85xx_cache_sram_probe,
>> > + .remove = uio_fsl_85xx_cache_sram_remove,
>> > + .driver = {
>> > + .name = DRIVER_NAME,
>> > + .owner = THIS_MODULE,
>> > + .of_match_table = uio_mpc85xx_l2ctlr_of_match,
>> > + },
>> > +};
>>
>> Greg's comment notwithstanding, I really don't think this belongs in the
>> device tree (and if I do get overruled on that point, it at least needs a
>> binding document). Let me try to come up with a patch for dynamic allocation.
>
>Agreed. "UIO" bindings have long been rejected.
>
Sounds it is. And does the modification below fit well?
---
-static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
- { .compatible = "uio,mpc85xx-cache-sram", },
- {},
+#ifdef CONFIG_OF
+static struct of_device_id uio_fsl_85xx_cache_sram_of_match[] = {
+ { /* This is filled with module_parm */ },
+ { /* Sentinel */ },
};
+MODULE_DEVICE_TABLE(of, uio_fsl_85xx_cache_sram_of_match);
+module_param_string(of_id, uio_fsl_85xx_cache_sram_of_match[0].compatible,
+ sizeof(uio_fsl_85xx_cache_sram_of_match[0].compatible), 0);
+MODULE_PARM_DESC(of_id, "platform device id to be handled by cache-sram-uio");
+#endif
static struct platform_driver uio_fsl_85xx_cache_sram = {
.probe = uio_fsl_85xx_cache_sram_probe,
.remove = uio_fsl_85xx_cache_sram_remove,
.driver = {
.name = DRIVER_NAME,
- .owner = THIS_MODULE,
- .of_match_table = uio_mpc85xx_l2ctlr_of_match,
+ .of_match_table = of_match_ptr(uio_fsl_85xx_cache_sram_of_match),
},
};
Regards,
Wenhu
^ permalink raw reply
* Re: POWER9 crash due to STRICT_KERNEL_RWX (WAS: Re: Linux-next POWER9 NULL pointer NIP...)
From: Russell Currey @ 2020-04-17 2:27 UTC (permalink / raw)
To: Steven Rostedt, Qian Cai; +Cc: linuxppc-dev, LKML, Nicholas Piggin
In-Reply-To: <20200416221753.4e47080a@oasis.local.home>
On Thu, 2020-04-16 at 22:17 -0400, Steven Rostedt wrote:
> On Thu, 16 Apr 2020 21:19:10 -0400
> Qian Cai <cai@lca.pw> wrote:
>
> > OK, reverted the commit,
> >
> > c55d7b5e6426 (“powerpc: Remove STRICT_KERNEL_RWX incompatibility
> > with RELOCATABLE”)
> >
> > or set STRICT_KERNEL_RWX=n fixed the crash below and also mentioned
> > in this thread,
>
> This may be a symptom and not a cure.
Reverting the patch with the given config will have the same effect as
STRICT_KERNEL_RWX=n. Not discounting that it could be a bug on the
powerpc side (i.e. relocatable kernels with strict RWX on haven't been
exhaustively tested yet), but we should definitely figure out what's
going on with this bad access first.
>
> > https://lore.kernel.org/lkml/15AC5B0E-A221-4B8C-9039-FA96B8EF7C88@lca.pw/
> >
> > [ 148.110969][T13115] LTP: starting chown04_16
> > [ 148.255048][T13380] kernel tried to execute exec-protected page
> > (c0000000016804ac) - exploit attempt? (uid: 0)
> > [ 148.255099][T13380] BUG: Unable to handle kernel instruction
> > fetch
> > [ 148.255122][T13380] Faulting instruction address:
> > 0xc0000000016804ac
> > [ 148.255136][T13380] Oops: Kernel access of bad area, sig: 11
> > [#1]
> > [ 148.255157][T13380] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=256
> > DEBUG_PAGEALLOC NUMA PowerNV
> > [ 148.255171][T13380] Modules linked in: loop kvm_hv kvm xfs
> > sd_mod bnx2x mdio ahci tg3 libahci libphy libata firmware_class
> > dm_mirror dm_region_hash dm_log dm_mod
> > [ 148.255213][T13380] CPU: 45 PID: 13380 Comm: chown04_16 Tainted:
> > G W 5.6.0+ #7
> > [ 148.255236][T13380] NIP: c0000000016804ac LR: c00800000fa60408
> > CTR: c0000000016804ac
> > [ 148.255250][T13380] REGS: c0000010a6fafa00 TRAP: 0400 Tainted:
> > G W (5.6.0+)
> > [ 148.255281][T13380] MSR: 9000000010009033
> > <SF,HV,EE,ME,IR,DR,RI,LE> CR: 84000248 XER: 20040000
> > [ 148.255310][T13380] CFAR: c00800000fa66534 IRQMASK: 0
> > [ 148.255310][T13380] GPR00: c000000000973268 c0000010a6fafc90
> > c000000001648200 0000000000000000
> > [ 148.255310][T13380] GPR04: c000000d8a22dc00 c0000010a6fafd30
> > 00000000b5e98331 ffffffff00012c9f
> > [ 148.255310][T13380] GPR08: c000000d8a22dc00 0000000000000000
> > 0000000000000000 c00000000163c520
> > [ 148.255310][T13380] GPR12: c0000000016804ac c000001ffffdad80
> > 0000000000000000 0000000000000000
> > [ 148.255310][T13380] GPR16: 0000000000000000 0000000000000000
> > 0000000000000000 0000000000000000
> > [ 148.255310][T13380] GPR20: 0000000000000000 0000000000000000
> > 0000000000000000 0000000000000000
> > [ 148.255310][T13380] GPR24: 00007fff8f5e2e48 0000000000000000
> > c00800000fa6a488 c0000010a6fafd30
> > [ 148.255310][T13380] GPR28: 0000000000000000 000000007fffffff
> > c00800000fa60400 c000000efd0c6780
> > [ 148.255494][T13380] NIP [c0000000016804ac]
> > sysctl_net_busy_read+0x0/0x4
>
> The instruction pointer is on sysctl_net_busy_read? Isn't that data
> and
> not code?
>
> In net/socket.c:
>
> #ifdef CONFIG_NET_RX_BUSY_POLL
> unsigned int sysctl_net_busy_read __read_mostly;
> unsigned int sysctl_net_busy_poll __read_mostly;
> #endif
>
> -- Steve
>
>
> > [ 148.255516][T13380] LR [c00800000fa60408] find_free_cb+0x8/0x30
> > [loop]
> > [ 148.255528][T13380] Call Trace:
> > [ 148.255538][T13380] [c0000010a6fafc90] [c0000000009732c0]
> > idr_for_each+0xf0/0x170 (unreliable)
> > [ 148.255572][T13380] [c0000010a6fafd10] [c00800000fa626c4]
> > loop_lookup.part.1+0x4c/0xb0 [loop]
> > [ 148.255597][T13380] [c0000010a6fafd50] [c00800000fa634d8]
> > loop_control_ioctl+0x120/0x1d0 [loop]
> > [ 148.255623][T13380] [c0000010a6fafdb0] [c0000000004ddc08]
> > ksys_ioctl+0xd8/0x130
> > [ 148.255636][T13380] [c0000010a6fafe00] [c0000000004ddc88]
> > sys_ioctl+0x28/0x40
> > [ 148.255669][T13380] [c0000010a6fafe20] [c00000000000b378]
> > system_call+0x5c/0x68
> > [ 148.255699][T13380] Instruction dump:
> > [ 148.255718][T13380] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
> > XXXXXXXX XXXXXXXX XXXXXXXX
> > [ 148.255744][T13380] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
> > XXXXXXXX XXXXXXXX XXXXXXXX
> > [ 148.255772][T13380] ---[ end trace a5894a74208c22ec ]---
> > [ 148.576663][T13380]
> > [ 149.576765][T13380] Kernel panic - not syncing: Fatal exception
> >
^ permalink raw reply
* Re: POWER9 crash due to STRICT_KERNEL_RWX (WAS: Re: Linux-next POWER9 NULL pointer NIP...)
From: Steven Rostedt @ 2020-04-17 2:17 UTC (permalink / raw)
To: Qian Cai; +Cc: linuxppc-dev, LKML, Nicholas Piggin
In-Reply-To: <69F0448F-CA5B-497D-B8AF-2848175B9477@lca.pw>
On Thu, 16 Apr 2020 21:19:10 -0400
Qian Cai <cai@lca.pw> wrote:
> OK, reverted the commit,
>
> c55d7b5e6426 (“powerpc: Remove STRICT_KERNEL_RWX incompatibility with RELOCATABLE”)
>
> or set STRICT_KERNEL_RWX=n fixed the crash below and also mentioned in this thread,
This may be a symptom and not a cure.
>
> https://lore.kernel.org/lkml/15AC5B0E-A221-4B8C-9039-FA96B8EF7C88@lca.pw/
>
> [ 148.110969][T13115] LTP: starting chown04_16
> [ 148.255048][T13380] kernel tried to execute exec-protected page (c0000000016804ac) - exploit attempt? (uid: 0)
> [ 148.255099][T13380] BUG: Unable to handle kernel instruction fetch
> [ 148.255122][T13380] Faulting instruction address: 0xc0000000016804ac
> [ 148.255136][T13380] Oops: Kernel access of bad area, sig: 11 [#1]
> [ 148.255157][T13380] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=256 DEBUG_PAGEALLOC NUMA PowerNV
> [ 148.255171][T13380] Modules linked in: loop kvm_hv kvm xfs sd_mod bnx2x mdio ahci tg3 libahci libphy libata firmware_class dm_mirror dm_region_hash dm_log dm_mod
> [ 148.255213][T13380] CPU: 45 PID: 13380 Comm: chown04_16 Tainted: G W 5.6.0+ #7
> [ 148.255236][T13380] NIP: c0000000016804ac LR: c00800000fa60408 CTR: c0000000016804ac
> [ 148.255250][T13380] REGS: c0000010a6fafa00 TRAP: 0400 Tainted: G W (5.6.0+)
> [ 148.255281][T13380] MSR: 9000000010009033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 84000248 XER: 20040000
> [ 148.255310][T13380] CFAR: c00800000fa66534 IRQMASK: 0
> [ 148.255310][T13380] GPR00: c000000000973268 c0000010a6fafc90 c000000001648200 0000000000000000
> [ 148.255310][T13380] GPR04: c000000d8a22dc00 c0000010a6fafd30 00000000b5e98331 ffffffff00012c9f
> [ 148.255310][T13380] GPR08: c000000d8a22dc00 0000000000000000 0000000000000000 c00000000163c520
> [ 148.255310][T13380] GPR12: c0000000016804ac c000001ffffdad80 0000000000000000 0000000000000000
> [ 148.255310][T13380] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [ 148.255310][T13380] GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [ 148.255310][T13380] GPR24: 00007fff8f5e2e48 0000000000000000 c00800000fa6a488 c0000010a6fafd30
> [ 148.255310][T13380] GPR28: 0000000000000000 000000007fffffff c00800000fa60400 c000000efd0c6780
> [ 148.255494][T13380] NIP [c0000000016804ac] sysctl_net_busy_read+0x0/0x4
The instruction pointer is on sysctl_net_busy_read? Isn't that data and
not code?
In net/socket.c:
#ifdef CONFIG_NET_RX_BUSY_POLL
unsigned int sysctl_net_busy_read __read_mostly;
unsigned int sysctl_net_busy_poll __read_mostly;
#endif
-- Steve
> [ 148.255516][T13380] LR [c00800000fa60408] find_free_cb+0x8/0x30 [loop]
> [ 148.255528][T13380] Call Trace:
> [ 148.255538][T13380] [c0000010a6fafc90] [c0000000009732c0] idr_for_each+0xf0/0x170 (unreliable)
> [ 148.255572][T13380] [c0000010a6fafd10] [c00800000fa626c4] loop_lookup.part.1+0x4c/0xb0 [loop]
> [ 148.255597][T13380] [c0000010a6fafd50] [c00800000fa634d8] loop_control_ioctl+0x120/0x1d0 [loop]
> [ 148.255623][T13380] [c0000010a6fafdb0] [c0000000004ddc08] ksys_ioctl+0xd8/0x130
> [ 148.255636][T13380] [c0000010a6fafe00] [c0000000004ddc88] sys_ioctl+0x28/0x40
> [ 148.255669][T13380] [c0000010a6fafe20] [c00000000000b378] system_call+0x5c/0x68
> [ 148.255699][T13380] Instruction dump:
> [ 148.255718][T13380] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
> [ 148.255744][T13380] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
> [ 148.255772][T13380] ---[ end trace a5894a74208c22ec ]---
> [ 148.576663][T13380]
> [ 149.576765][T13380] Kernel panic - not syncing: Fatal exception
>
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Segher Boessenkool @ 2020-04-17 1:48 UTC (permalink / raw)
To: Rich Felker
Cc: libc-alpha, musl, Nicholas Piggin, Florian Weimer, libc-dev,
linuxppc-dev
In-Reply-To: <20200417003442.GD11469@brightrain.aerifal.cx>
On Thu, Apr 16, 2020 at 08:34:42PM -0400, Rich Felker wrote:
> On Thu, Apr 16, 2020 at 06:02:35PM -0500, Segher Boessenkool wrote:
> > On Thu, Apr 16, 2020 at 08:12:19PM +0200, Florian Weimer wrote:
> > > > I think my choice would be just making the inline syscall be a single
> > > > call insn to an asm source file that out-of-lines the loading of TOC
> > > > pointer and call through it or branch based on hwcap so that it's not
> > > > repeated all over the place.
> > >
> > > I don't know how problematic control flow out of an inline asm is on
> > > POWER. But this is basically the -moutline-atomics approach.
> >
> > Control flow out of inline asm (other than with "asm goto") is not
> > allowed at all, just like on any other target (and will not work in
> > practice, either -- just like on any other target). But the suggestion
> > was to use actual assembler code, not inline asm?
>
> Calling it control flow out of inline asm is something of a misnomer.
> The enclosing state is not discarded or altered; the asm statement
> exits normally, reaching the next instruction in the enclosing
> block/function as soon as the call from the asm statement returns,
> with all register/clobber constraints satisfied.
Ah. That should always Just Work, then -- our ABIs guarantee you can.
> Control flow out of inline asm would be more like longjmp, and it can
> be valid -- for instance, you can implement coroutines this way
> (assuming you switch stack correctly) or do longjmp this way (jumping
> to the location saved by setjmp). But it's not what'd be happening
> here.
Yeah, you cannot do that in C, not without making assumptions about what
machine code the compiler generates. GCC explicitly disallows it, too:
'asm' statements may not perform jumps into other 'asm' statements,
only to the listed GOTOLABELS. GCC's optimizers do not know about
other jumps; therefore they cannot take account of them when
deciding how to optimize.
Segher
^ permalink raw reply
* Re: [PATCH] powerpc/uaccess: Use flexible addressing with __put_user()/__get_user()
From: Segher Boessenkool @ 2020-04-17 1:39 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linux-kernel, npiggin, Paul Mackerras, linuxppc-dev
In-Reply-To: <1f5a7975-3b32-3a14-e03e-7c875df57aa3@c-s.fr>
Hi!
On Thu, Apr 16, 2020 at 07:50:00AM +0200, Christophe Leroy wrote:
> Le 16/04/2020 à 00:06, Segher Boessenkool a écrit :
> >On Wed, Apr 15, 2020 at 09:20:26AM +0000, Christophe Leroy wrote:
> >>At the time being, __put_user()/__get_user() and friends only use
> >>register indirect with immediate index addressing, with the index
> >>set to 0. Ex:
> >>
> >> lwz reg1, 0(reg2)
> >
> >This is called a "D-form" instruction, or sometimes "offset addressing".
> >Don't talk about an "index", it confuses things, because the *other*
> >kind is called "indexed" already, also in the ISA docs! (X-form, aka
> >indexed addressing, [reg+reg], where D-form does [reg+imm], and both
> >forms can do [reg]).
>
> In the "Programming Environments Manual for 32-Bit Implementations of
> the PowerPC™ Architecture", they list the following addressing modes:
>
> Load and store operations have three categories of effective address
> generation that depend on the
> operands specified:
> • Register indirect with immediate index mode
> • Register indirect with index mode
> • Register indirect mode
Huh. I must have pushed all that confusing terminology to the back of
my head :-)
> >%Un on an "m" operand doesn't do much: you need to make it "m<>" if you
> >want pre-modify ("update") insns to be generated. (You then will want
> >to make sure that operand is used in a way GCC can understand; since it
> >is used only once here, that works fine).
>
> Ah ? Indeed I got the idea from include/asm/io.h where there is:
>
> #define DEF_MMIO_IN_D(name, size, insn) \
> static inline u##size name(const volatile u##size __iomem *addr) \
> { \
> u##size ret; \
> __asm__ __volatile__("sync;"#insn"%U1%X1 %0,%1;twi 0,%0,0;isync"\
> : "=r" (ret) : "m" (*addr) : "memory"); \
> return ret; \
> }
>
> It should be "m<>" there as well ?
Yes, that will work here.
Long ago, "m" in inline assembler code did the same as "m<>" now does
(and "m" internal in GCC still does). To get a memory without pre-modify
addressing, you used "es".
Since people kept getting that wrong (it *is* surprising), it was changed
to the current scheme. But the kernel uses weren't updated (and no one
seems to have missed it).
> >> #else /* __powerpc64__ */
> >> #define __put_user_asm2(x, addr, err) \
> >> __asm__ __volatile__( \
> >>- "1: stw %1,0(%2)\n" \
> >>- "2: stw %1+1,4(%2)\n" \
> >>+ "1: stw%U2%X2 %1,%2\n" \
> >>+ "2: stw%U2%X2 %L1,%L2\n" \
> >> "3:\n" \
> >> ".section .fixup,\"ax\"\n" \
> >> "4: li %0,%3\n" \
> >>@@ -140,7 +140,7 @@ extern long __put_user_bad(void);
> >> EX_TABLE(1b, 4b) \
> >> EX_TABLE(2b, 4b) \
> >> : "=r" (err) \
> >>- : "r" (x), "b" (addr), "i" (-EFAULT), "0" (err))
> >>+ : "r" (x), "m" (*addr), "i" (-EFAULT), "0" (err))
> >
> >Here, it doesn't work. You don't want two consecutive update insns in
> >any case. Easiest is to just not use "m<>", and then, don't use %Un
> >(which won't do anything, but it is confusing).
>
> Can't we leave the Un on the second stw ?
That cannot work. You can use it on only the *first* though :-)
And this doesn't work on LE I think? (But the asm doesn't anyway?)
Or, you can decide this is all way too tricky, and not worth it.
Segher
^ permalink raw reply
* Re: [PATCH kernel v2 0/7] powerpc/powenv/ioda: Allow huge DMA window at 4GB
From: Russell Currey @ 2020-04-17 1:26 UTC (permalink / raw)
To: Oliver O'Halloran, Alexey Kardashevskiy
Cc: KVM list, Fabiano Rosas, Alistair Popple, kvm-ppc, linuxppc-dev,
David Gibson
In-Reply-To: <CAOSf1CHgUsJ7jGokg6QD6cEDr4-o5hnyyyjRZ=YijsRY3T1sYA@mail.gmail.com>
On Thu, 2020-04-16 at 12:53 +1000, Oliver O'Halloran wrote:
> On Thu, Apr 16, 2020 at 12:34 PM Oliver O'Halloran <oohall@gmail.com>
> wrote:
> > On Thu, Apr 16, 2020 at 11:27 AM Alexey Kardashevskiy <
> > aik@ozlabs.ru> wrote:
> > > Anyone? Is it totally useless or wrong approach? Thanks,
> >
> > I wouldn't say it's either, but I still hate it.
> >
> > The 4GB mode being per-PHB makes it difficult to use unless we
> > force
> > that mode on 100% of the time which I'd prefer not to do. Ideally
> > devices that actually support 64bit addressing (which is most of
> > them)
> > should be able to use no-translate mode when possible since a) It's
> > faster, and b) It frees up room in the TCE cache devices that
> > actually
> > need them. I know you've done some testing with 100G NICs and found
> > the overhead was fine, but IMO that's a bad test since it's pretty
> > much the best-case scenario since all the devices on the PHB are in
> > the same PE. The PHB's TCE cache only hits when the TCE matches the
> > DMA bus address and the PE number for the device so in a multi-PE
> > environment there's a lot of potential for TCE cache trashing. If
> > there was one or two PEs under that PHB it's probably not going to
> > matter, but if you have an NVMe rack with 20 drives it starts to
> > look
> > a bit ugly.
> >
> > That all said, it might be worth doing this anyway since we
> > probably
> > want the software infrastructure in place to take advantage of it.
> > Maybe expand the command line parameters to allow it to be enabled
> > on
> > a per-PHB basis rather than globally.
>
> Since we're on the topic
>
> I've been thinking the real issue we have is that we're trying to
> pick
> an "optimal" IOMMU config at a point where we don't have enough
> information to work out what's actually optimal. The IOMMU config is
> done on a per-PE basis, but since PEs may contain devices with
> different DMA masks (looking at you wierd AMD audio function) we're
> always going to have to pick something conservative as the default
> config for TVE#0 (64k, no bypass mapping) since the driver will tell
> us what the device actually supports long after the IOMMU
> configuation
> is done. What we really want is to be able to have separate IOMMU
> contexts for each device, or at the very least a separate context for
> the crippled devices.
>
> We could allow a per-device IOMMU context by extending the Master /
> Slave PE thing to cover DMA in addition to MMIO. Right now we only
> use
> slave PEs when a device's MMIO BARs extend over multiple m64
> segments.
> When that happens an MMIO error causes the PHB to freezes the PE
> corresponding to one of those segments, but not any of the others. To
> present a single "PE" to the EEH core we check the freeze status of
> each of the slave PEs when the EEH core does a PE status check and if
> any of them are frozen, we freeze the rest of them too. When a driver
> sets a limited DMA mask we could move that device to a seperate slave
> PE so that it has it's own IOMMU context taylored to its DMA
> addressing limits.
>
> Thoughts?
For what it's worth this sounds like a good idea to me, it just sounds
tricky to implement. You're adding another layer of complexity on top
of EEH (well, making things look simple to the EEH core and doing your
own freezing on top of it) in addition to the DMA handling.
If it works then great, just has a high potential to become a new bug
haven.
>
> Oliver
^ permalink raw reply
* POWER9 crash due to STRICT_KERNEL_RWX (WAS: Re: Linux-next POWER9 NULL pointer NIP...)
From: Qian Cai @ 2020-04-17 1:19 UTC (permalink / raw)
To: Michael Ellerman, Russell Currey
Cc: Steven Rostedt, linuxppc-dev, LKML, Nicholas Piggin
In-Reply-To: <161662E3-5D9C-4C15-919C-CFEFE4CC35CB@lca.pw>
OK, reverted the commit,
c55d7b5e6426 (“powerpc: Remove STRICT_KERNEL_RWX incompatibility with RELOCATABLE”)
or set STRICT_KERNEL_RWX=n fixed the crash below and also mentioned in this thread,
https://lore.kernel.org/lkml/15AC5B0E-A221-4B8C-9039-FA96B8EF7C88@lca.pw/
[ 148.110969][T13115] LTP: starting chown04_16
[ 148.255048][T13380] kernel tried to execute exec-protected page (c0000000016804ac) - exploit attempt? (uid: 0)
[ 148.255099][T13380] BUG: Unable to handle kernel instruction fetch
[ 148.255122][T13380] Faulting instruction address: 0xc0000000016804ac
[ 148.255136][T13380] Oops: Kernel access of bad area, sig: 11 [#1]
[ 148.255157][T13380] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=256 DEBUG_PAGEALLOC NUMA PowerNV
[ 148.255171][T13380] Modules linked in: loop kvm_hv kvm xfs sd_mod bnx2x mdio ahci tg3 libahci libphy libata firmware_class dm_mirror dm_region_hash dm_log dm_mod
[ 148.255213][T13380] CPU: 45 PID: 13380 Comm: chown04_16 Tainted: G W 5.6.0+ #7
[ 148.255236][T13380] NIP: c0000000016804ac LR: c00800000fa60408 CTR: c0000000016804ac
[ 148.255250][T13380] REGS: c0000010a6fafa00 TRAP: 0400 Tainted: G W (5.6.0+)
[ 148.255281][T13380] MSR: 9000000010009033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 84000248 XER: 20040000
[ 148.255310][T13380] CFAR: c00800000fa66534 IRQMASK: 0
[ 148.255310][T13380] GPR00: c000000000973268 c0000010a6fafc90 c000000001648200 0000000000000000
[ 148.255310][T13380] GPR04: c000000d8a22dc00 c0000010a6fafd30 00000000b5e98331 ffffffff00012c9f
[ 148.255310][T13380] GPR08: c000000d8a22dc00 0000000000000000 0000000000000000 c00000000163c520
[ 148.255310][T13380] GPR12: c0000000016804ac c000001ffffdad80 0000000000000000 0000000000000000
[ 148.255310][T13380] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[ 148.255310][T13380] GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[ 148.255310][T13380] GPR24: 00007fff8f5e2e48 0000000000000000 c00800000fa6a488 c0000010a6fafd30
[ 148.255310][T13380] GPR28: 0000000000000000 000000007fffffff c00800000fa60400 c000000efd0c6780
[ 148.255494][T13380] NIP [c0000000016804ac] sysctl_net_busy_read+0x0/0x4
[ 148.255516][T13380] LR [c00800000fa60408] find_free_cb+0x8/0x30 [loop]
[ 148.255528][T13380] Call Trace:
[ 148.255538][T13380] [c0000010a6fafc90] [c0000000009732c0] idr_for_each+0xf0/0x170 (unreliable)
[ 148.255572][T13380] [c0000010a6fafd10] [c00800000fa626c4] loop_lookup.part.1+0x4c/0xb0 [loop]
[ 148.255597][T13380] [c0000010a6fafd50] [c00800000fa634d8] loop_control_ioctl+0x120/0x1d0 [loop]
[ 148.255623][T13380] [c0000010a6fafdb0] [c0000000004ddc08] ksys_ioctl+0xd8/0x130
[ 148.255636][T13380] [c0000010a6fafe00] [c0000000004ddc88] sys_ioctl+0x28/0x40
[ 148.255669][T13380] [c0000010a6fafe20] [c00000000000b378] system_call+0x5c/0x68
[ 148.255699][T13380] Instruction dump:
[ 148.255718][T13380] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
[ 148.255744][T13380] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
[ 148.255772][T13380] ---[ end trace a5894a74208c22ec ]---
[ 148.576663][T13380]
[ 149.576765][T13380] Kernel panic - not syncing: Fatal exception
^ permalink raw reply
* Re: [PATCH] target/ppc: Fix mtmsr(d) L=1 variant that loses interrupts
From: David Gibson @ 2020-04-17 0:40 UTC (permalink / raw)
To: Nicholas Piggin
Cc: qemu-devel, qemu-stable, Cédric Le Goater, qemu-ppc,
Nathan Chancellor, linuxppc-dev
In-Reply-To: <20200414111131.465560-1-npiggin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 6769 bytes --]
On Tue, Apr 14, 2020 at 09:11:31PM +1000, Nicholas Piggin wrote:
65;5803;1c> If mtmsr L=1 sets MSR[EE] while there is a maskable exception pending,
> it does not cause an interrupt. This causes the test case to hang:
>
> https://lists.gnu.org/archive/html/qemu-ppc/2019-10/msg00826.html
>
> More recently, Linux reduced the occurance of operations (e.g., rfi)
> which stop translation and allow pending interrupts to be processed.
> This started causing hangs in Linux boot in long-running kernel tests,
> running with '-d int' shows the decrementer stops firing despite DEC
> wrapping and MSR[EE]=1.
>
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-April/208301.html
>
> The cause is the broken mtmsr L=1 behaviour, which is contrary to the
> architecture. From Power ISA v3.0B, p.977, Move To Machine State Register,
> Programming Note states:
>
> If MSR[EE]=0 and an External, Decrementer, or Performance Monitor
> exception is pending, executing an mtmsrd instruction that sets
> MSR[EE] to 1 will cause the interrupt to occur before the next
> instruction is executed, if no higher priority exception exists
>
> Fix this by handling L=1 exactly the same way as L=0, modulo the MSR
> bits altered.
>
> The confusion arises from L=0 being "context synchronizing" whereas L=1
> is "execution synchronizing", which is a weaker semantic. However this
> is not a relaxation of the requirement that these exceptions cause
> interrupts when MSR[EE]=1 (e.g., when mtmsr executes to completion as
> TCG is doing here), rather it specifies how a pipelined processor can
> have multiple instructions in flight where one may influence how another
> behaves.
>
> Cc: qemu-stable@nongnu.org
> Reported-by: Anton Blanchard <anton@ozlabs.org>
> Reported-by: Nathan Chancellor <natechancellor@gmail.com>
> Tested-by: Nathan Chancellor <natechancellor@gmail.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Thanks very much to Nathan for reporting and testing it, I added his
> Tested-by tag despite a more polished patch, as the the basics are
> still the same (and still fixes his test case here).
>
> This bug possibly goes back to early v2.04 / mtmsrd L=1 support around
> 2007, and the code has been changed several times since then so may
> require some backporting.
>
> 32-bit / mtmsr untested at the moment, I don't have an environment
> handy.
>
> target/ppc/translate.c | 46 +++++++++++++++++++++++++-----------------
> 1 file changed, 27 insertions(+), 19 deletions(-)
Applied to ppc-for-5.0.
>
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index b207fb5386..9959259dba 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -4361,30 +4361,34 @@ static void gen_mtmsrd(DisasContext *ctx)
> CHK_SV;
>
> #if !defined(CONFIG_USER_ONLY)
> + if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
> + gen_io_start();
> + }
> if (ctx->opcode & 0x00010000) {
> - /* Special form that does not need any synchronisation */
> + /* L=1 form only updates EE and RI */
> TCGv t0 = tcg_temp_new();
> + TCGv t1 = tcg_temp_new();
> tcg_gen_andi_tl(t0, cpu_gpr[rS(ctx->opcode)],
> (1 << MSR_RI) | (1 << MSR_EE));
> - tcg_gen_andi_tl(cpu_msr, cpu_msr,
> + tcg_gen_andi_tl(t1, cpu_msr,
> ~(target_ulong)((1 << MSR_RI) | (1 << MSR_EE)));
> - tcg_gen_or_tl(cpu_msr, cpu_msr, t0);
> + tcg_gen_or_tl(t1, t1, t0);
> +
> + gen_helper_store_msr(cpu_env, t1);
> tcg_temp_free(t0);
> + tcg_temp_free(t1);
> +
> } else {
> /*
> * XXX: we need to update nip before the store if we enter
> * power saving mode, we will exit the loop directly from
> * ppc_store_msr
> */
> - if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
> - gen_io_start();
> - }
> gen_update_nip(ctx, ctx->base.pc_next);
> gen_helper_store_msr(cpu_env, cpu_gpr[rS(ctx->opcode)]);
> - /* Must stop the translation as machine state (may have) changed */
> - /* Note that mtmsr is not always defined as context-synchronizing */
> - gen_stop_exception(ctx);
> }
> + /* Must stop the translation as machine state (may have) changed */
> + gen_stop_exception(ctx);
> #endif /* !defined(CONFIG_USER_ONLY) */
> }
> #endif /* defined(TARGET_PPC64) */
> @@ -4394,15 +4398,23 @@ static void gen_mtmsr(DisasContext *ctx)
> CHK_SV;
>
> #if !defined(CONFIG_USER_ONLY)
> - if (ctx->opcode & 0x00010000) {
> - /* Special form that does not need any synchronisation */
> + if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
> + gen_io_start();
> + }
> + if (ctx->opcode & 0x00010000) {
> + /* L=1 form only updates EE and RI */
> TCGv t0 = tcg_temp_new();
> + TCGv t1 = tcg_temp_new();
> tcg_gen_andi_tl(t0, cpu_gpr[rS(ctx->opcode)],
> (1 << MSR_RI) | (1 << MSR_EE));
> - tcg_gen_andi_tl(cpu_msr, cpu_msr,
> + tcg_gen_andi_tl(t1, cpu_msr,
> ~(target_ulong)((1 << MSR_RI) | (1 << MSR_EE)));
> - tcg_gen_or_tl(cpu_msr, cpu_msr, t0);
> + tcg_gen_or_tl(t1, t1, t0);
> +
> + gen_helper_store_msr(cpu_env, t1);
> tcg_temp_free(t0);
> + tcg_temp_free(t1);
> +
> } else {
> TCGv msr = tcg_temp_new();
>
> @@ -4411,9 +4423,6 @@ static void gen_mtmsr(DisasContext *ctx)
> * power saving mode, we will exit the loop directly from
> * ppc_store_msr
> */
> - if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
> - gen_io_start();
> - }
> gen_update_nip(ctx, ctx->base.pc_next);
> #if defined(TARGET_PPC64)
> tcg_gen_deposit_tl(msr, cpu_msr, cpu_gpr[rS(ctx->opcode)], 0, 32);
> @@ -4422,10 +4431,9 @@ static void gen_mtmsr(DisasContext *ctx)
> #endif
> gen_helper_store_msr(cpu_env, msr);
> tcg_temp_free(msr);
> - /* Must stop the translation as machine state (may have) changed */
> - /* Note that mtmsr is not always defined as context-synchronizing */
> - gen_stop_exception(ctx);
> }
> + /* Must stop the translation as machine state (may have) changed */
> + gen_stop_exception(ctx);
> #endif
> }
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] KVM: PPC: Book3S HV: Handle non-present PTEs in page fault functions
From: David Gibson @ 2020-04-17 0:47 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, groug, kvm-ppc, linux-kernel, linuxppc-dev
In-Reply-To: <a4e1bf29-af52-232e-d0d2-06206fa05fbe@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 4724 bytes --]
On Thu, Apr 16, 2020 at 10:07:49AM +0200, Cédric Le Goater wrote:
> On 4/16/20 7:03 AM, Paul Mackerras wrote:
> > Since cd758a9b57ee "KVM: PPC: Book3S HV: Use __gfn_to_pfn_memslot in HPT
> > page fault handler", it's been possible in fairly rare circumstances to
> > load a non-present PTE in kvmppc_book3s_hv_page_fault() when running a
> > guest on a POWER8 host.
> >
> > Because that case wasn't checked for, we could misinterpret the non-present
> > PTE as being a cache-inhibited PTE. That could mismatch with the
> > corresponding hash PTE, which would cause the function to fail with -EFAULT
> > a little further down. That would propagate up to the KVM_RUN ioctl()
> > generally causing the KVM userspace (usually qemu) to fall over.
> >
> > This addresses the problem by catching that case and returning to the guest
> > instead, letting it fault again, and retrying the whole page fault from
> > the beginning.
> >
> > For completeness, this fixes the radix page fault handler in the same
> > way. For radix this didn't cause any obvious misbehaviour, because we
> > ended up putting the non-present PTE into the guest's partition-scoped
> > page tables, leading immediately to another hypervisor data/instruction
> > storage interrupt, which would go through the page fault path again
> > and fix things up.
> >
> > Fixes: cd758a9b57ee "KVM: PPC: Book3S HV: Use __gfn_to_pfn_memslot in HPT page fault handler"
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1820402
> > Reported-by: David Gibson <david@gibson.dropbear.id.au>
> > Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
>
> I didn't see the reported issue with the current 5.7-rc1. Anyhow I gave
> this patch a try on a P8 host and a P9 host with a radix guest and a hash
> guest (using rhel6). Passthrough is fine also.
>
> Tested-by: Cédric Le Goater <clg@kaod.org>
>
> The code looks correct,
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
I ran my test case overnight with this patch for over 1000 iterations,
without any apparent problems so
Tested-by: David Gibson <david@gibson.dropbear.id.au>
>
> Thanks,
>
> C.
>
>
> > ---
> > This is a reworked version of the patch David Gibson sent recently,
> > with the fix applied to the radix case as well. The commit message
> > is mostly stolen from David's patch.
> >
> > arch/powerpc/kvm/book3s_64_mmu_hv.c | 9 +++++----
> > arch/powerpc/kvm/book3s_64_mmu_radix.c | 9 +++++----
> > 2 files changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > index 3aecec8..20b7dce 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > @@ -604,18 +604,19 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
> > */
> > local_irq_disable();
> > ptep = __find_linux_pte(vcpu->arch.pgdir, hva, NULL, &shift);
> > + pte = __pte(0);
> > + if (ptep)
> > + pte = *ptep;
> > + local_irq_enable();
> > /*
> > * If the PTE disappeared temporarily due to a THP
> > * collapse, just return and let the guest try again.
> > */
> > - if (!ptep) {
> > - local_irq_enable();
> > + if (!pte_present(pte)) {
> > if (page)
> > put_page(page);
> > return RESUME_GUEST;
> > }
> > - pte = *ptep;
> > - local_irq_enable();
> > hpa = pte_pfn(pte) << PAGE_SHIFT;
> > pte_size = PAGE_SIZE;
> > if (shift)
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > index 134fbc1..7bf94ba 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > @@ -815,18 +815,19 @@ int kvmppc_book3s_instantiate_page(struct kvm_vcpu *vcpu,
> > */
> > local_irq_disable();
> > ptep = __find_linux_pte(vcpu->arch.pgdir, hva, NULL, &shift);
> > + pte = __pte(0);
> > + if (ptep)
> > + pte = *ptep;
> > + local_irq_enable();
> > /*
> > * If the PTE disappeared temporarily due to a THP
> > * collapse, just return and let the guest try again.
> > */
> > - if (!ptep) {
> > - local_irq_enable();
> > + if (!pte_present(pte)) {
> > if (page)
> > put_page(page);
> > return RESUME_GUEST;
> > }
> > - pte = *ptep;
> > - local_irq_enable();
> >
> > /* If we're logging dirty pages, always map single pages */
> > large_enable = !(memslot->flags & KVM_MEM_LOG_DIRTY_PAGES);
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 2/3] powerpc/lib: Initialize a temporary mm for code patching
From: Michael Ellerman @ 2020-04-17 0:57 UTC (permalink / raw)
To: Christophe Leroy, Christopher M Riedl, linuxppc-dev
In-Reply-To: <8ff6d279-fdd9-4e4d-b4e0-f5c5cba480a4@c-s.fr>
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Le 31/03/2020 à 05:19, Christopher M Riedl a écrit :
>>> On March 24, 2020 11:10 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>> Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
>>>> When code patching a STRICT_KERNEL_RWX kernel the page containing the
>>>> address to be patched is temporarily mapped with permissive memory
>>>> protections. Currently, a per-cpu vmalloc patch area is used for this
>>>> purpose. While the patch area is per-cpu, the temporary page mapping is
>>>> inserted into the kernel page tables for the duration of the patching.
>>>> The mapping is exposed to CPUs other than the patching CPU - this is
>>>> undesirable from a hardening perspective.
>>>>
>>>> Use the `poking_init` init hook to prepare a temporary mm and patching
>>>> address. Initialize the temporary mm by copying the init mm. Choose a
>>>> randomized patching address inside the temporary mm userspace address
>>>> portion. The next patch uses the temporary mm and patching address for
>>>> code patching.
>>>>
>>>> Based on x86 implementation:
>>>>
>>>> commit 4fc19708b165
>>>> ("x86/alternatives: Initialize temporary mm for patching")
>>>>
>>>> Signed-off-by: Christopher M. Riedl <cmr@informatik.wtf>
>>>> ---
>>>> arch/powerpc/lib/code-patching.c | 26 ++++++++++++++++++++++++++
>>>> 1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
>>>> index 3345f039a876..18b88ecfc5a8 100644
>>>> --- a/arch/powerpc/lib/code-patching.c
>>>> +++ b/arch/powerpc/lib/code-patching.c
>>>> @@ -11,6 +11,8 @@
>>>> #include <linux/cpuhotplug.h>
>>>> #include <linux/slab.h>
>>>> #include <linux/uaccess.h>
>>>> +#include <linux/sched/task.h>
>>>> +#include <linux/random.h>
>>>>
>>>> #include <asm/pgtable.h>
>>>> #include <asm/tlbflush.h>
>>>> @@ -39,6 +41,30 @@ int raw_patch_instruction(unsigned int *addr, unsigned int instr)
>>>> }
>>>>
>>>> #ifdef CONFIG_STRICT_KERNEL_RWX
>>>> +
>>>> +__ro_after_init struct mm_struct *patching_mm;
>>>> +__ro_after_init unsigned long patching_addr;
>>>
>>> Can we make those those static ?
>>>
>>
>> Yes, makes sense to me.
>>
>>>> +
>>>> +void __init poking_init(void)
>>>> +{
>>>> + spinlock_t *ptl; /* for protecting pte table */
>>>> + pte_t *ptep;
>>>> +
>>>> + patching_mm = copy_init_mm();
>>>> + BUG_ON(!patching_mm);
>>>
>>> Does it needs to be a BUG_ON() ? Can't we fail gracefully with just a
>>> WARN_ON ?
>>>
>>
>> I'm not sure what failing gracefully means here? The main reason this could
>> fail is if there is not enough memory to allocate the patching_mm. The
>> previous implementation had this justification for BUG_ON():
>
> But the system can continue running just fine after this failure.
> Only the things that make use of code patching will fail (ftrace, kgdb, ...)
That's probably true of ftrace, but we can't fail patching for jump
labels (static keys).
See:
void arch_jump_label_transform(struct jump_entry *entry,
enum jump_label_type type)
{
u32 *addr = (u32 *)(unsigned long)entry->code;
if (type == JUMP_LABEL_JMP)
patch_branch(addr, entry->target, 0);
else
patch_instruction(addr, PPC_INST_NOP);
}
cheers
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Rich Felker @ 2020-04-17 0:34 UTC (permalink / raw)
To: Segher Boessenkool
Cc: libc-alpha, musl, Nicholas Piggin, Florian Weimer, libc-dev,
linuxppc-dev
In-Reply-To: <20200416230235.GG26902@gate.crashing.org>
On Thu, Apr 16, 2020 at 06:02:35PM -0500, Segher Boessenkool wrote:
> On Thu, Apr 16, 2020 at 08:12:19PM +0200, Florian Weimer wrote:
> > > I think my choice would be just making the inline syscall be a single
> > > call insn to an asm source file that out-of-lines the loading of TOC
> > > pointer and call through it or branch based on hwcap so that it's not
> > > repeated all over the place.
> >
> > I don't know how problematic control flow out of an inline asm is on
> > POWER. But this is basically the -moutline-atomics approach.
>
> Control flow out of inline asm (other than with "asm goto") is not
> allowed at all, just like on any other target (and will not work in
> practice, either -- just like on any other target). But the suggestion
> was to use actual assembler code, not inline asm?
Calling it control flow out of inline asm is something of a misnomer.
The enclosing state is not discarded or altered; the asm statement
exits normally, reaching the next instruction in the enclosing
block/function as soon as the call from the asm statement returns,
with all register/clobber constraints satisfied.
Control flow out of inline asm would be more like longjmp, and it can
be valid -- for instance, you can implement coroutines this way
(assuming you switch stack correctly) or do longjmp this way (jumping
to the location saved by setjmp). But it's not what'd be happening
here.
Rich
^ 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