[PATCH] powerpc/dcr: Use cmplwi instead of 3-argument cmpli
In dcr-low.S we use cmpli with three arguments, instead of four arguments as defined in the ISA: cmpli cr0,r3,1024 This appears to be a PPC440-ism, looking at the "PPC440x5 CPU Core User’s Manual" it shows cmpli having no L field, but implied to be 0 due to the core being 32-bit. It mentions that the ISA defines four arguments and recommends using cmplwi. dcr-low.S is only built 32-bit, because it is only built when DCR_NATIVE=y, which is only selected by 40x and 44x. Looking at the generated code (with gcc/gas) we see cmplwi as expected. Although gas is happy with the 3-argument version when building for 32-bit, the LLVM assembler is not and errors out with: arch/powerpc/sysdev/dcr-low.S:27:10: error: invalid operand for instruction cmpli 0,%r3,1024; ... ^ Switching to the four argument version avoids any confusion when reading the ISA, fixes the issue with the LLVM assembler, and also means the code could be built 64-bit in future (though that's very unlikely). Reported-by: Nick Desaulniers Signed-off-by: Michael Ellerman --- arch/powerpc/sysdev/dcr-low.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/powerpc/sysdev/dcr-low.S b/arch/powerpc/sysdev/dcr-low.S index efeeb1b885a1..329b9c4ae542 100644 --- a/arch/powerpc/sysdev/dcr-low.S +++ b/arch/powerpc/sysdev/dcr-low.S @@ -11,7 +11,7 @@ #include #define DCR_ACCESS_PROLOG(table) \ - cmpli cr0,r3,1024; \ + cmplwi cr0,r3,1024; \ rlwinm r3,r3,4,18,27; \ lis r5,table@h; \ ori r5,r5,table@l; \ -- 2.25.1
Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver
On Wed, Oct 13, 2021 at 06:33:56AM -0500, Bjorn Helgaas wrote: > On Wed, Oct 13, 2021 at 12:26:42PM +0300, Andy Shevchenko wrote: > > On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas wrote: > > > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote: ... > > It's a bit unusual. Other to_*_dev() are not NULL-aware IIRC. > > It is a little unusual. I only found three of 77 that are NULL-aware: > > to_moxtet_driver() > to_siox_driver() > to_spi_driver() > > It seems worthwhile to me because it makes the patch and the resulting > code significantly cleaner. I'm not objecting the change, just a remark. ... > > > + for (id = drv ? drv->id_table : NULL; id && id->vendor; id++) > > > + if (id->vendor == vendor && id->device == device) > > > > > + break; > > > > return true; > > > > > return id && id->vendor; > > > > return false; > > Good cleanup for a follow-up patch, but doesn't seem directly related > to the objective here. True. Maybe you can bake one while not forgotten? ... > > > + return drv && drv->resume ? > > > + drv->resume(pci_dev) : > > > pci_pm_reenable_device(pci_dev); > > > > One line? > > I don't think I touched that line. Then why they are both in + section? ... > > > + struct pci_driver *drv = to_pci_driver(dev->dev.driver); > > > const struct pci_error_handlers *err_handler = > > > - dev->dev.driver ? > > > to_pci_driver(dev->dev.driver)->err_handler : NULL; > > > + drv ? drv->err_handler : NULL; > > > > Isn't dev->driver == to_pci_driver(dev->dev.driver)? > > Yes, I think so, but not sure what you're getting at here, can you > elaborate? Getting pointer from another pointer seems waste of resources, why we can't simply struct pci_driver *drv = dev->driver; ? ... > > Stray change? Or is it in a separate patch in your tree? > > Could be skipped. The string now fits on one line so I combined it to > make it more greppable. This is inconsistency in your changes, in one case you are objecting of doing something close to the changed lines, in the other you are doing unrelated change. -- With Best Regards, Andy Shevchenko
Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver
On Wed, Oct 13, 2021 at 12:26:42PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas wrote:
> > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:
>
> > I split some of the bigger patches apart so they only touched one
> > driver or subsystem at a time. I also updated to_pci_driver() so it
> > returns NULL when given NULL, which makes some of the validations
> > quite a bit simpler, especially in the PM code in pci-driver.c.
>
> It's a bit unusual. Other to_*_dev() are not NULL-aware IIRC.
It is a little unusual. I only found three of 77 that are NULL-aware:
to_moxtet_driver()
to_siox_driver()
to_spi_driver()
It seems worthwhile to me because it makes the patch and the resulting
code significantly cleaner. Here's one example without the NULL
check:
@@ -493,12 +493,15 @@ static void pci_device_remove(struct device *dev)
static void pci_device_shutdown(struct device *dev)
{
struct pci_dev *pci_dev = to_pci_dev(dev);
- struct pci_driver *drv = pci_dev->driver;
pm_runtime_resume(dev);
- if (drv && drv->shutdown)
- drv->shutdown(pci_dev);
+ if (pci_dev->dev.driver) {
+ struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);
+
+ if (drv->shutdown)
+ drv->shutdown(pci_dev);
+ }
static void pci_device_shutdown(struct device *dev)
{
struct pci_dev *pci_dev = to_pci_dev(dev);
pm_runtime_resume(dev);
if (pci_dev->dev.driver) {
struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);
if (drv->shutdown)
drv->shutdown(pci_dev);
}
and here's the same thing with the NULL check:
@@ -493,7 +493,7 @@ static void pci_device_remove(struct device *dev)
static void pci_device_shutdown(struct device *dev)
{
struct pci_dev *pci_dev = to_pci_dev(dev);
- struct pci_driver *drv = pci_dev->driver;
+ struct pci_driver *drv = to_pci_driver(dev->driver);
static void pci_device_shutdown(struct device *dev)
{
struct pci_dev *pci_dev = to_pci_dev(dev);
struct pci_driver *drv = to_pci_driver(dev->driver);
pm_runtime_resume(dev);
if (drv && drv->shutdown)
drv->shutdown(pci_dev);
> > static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned
> > short device)
> > {
> > + struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
> > const struct pci_device_id *id;
> >
> > if (pdev->vendor == vendor && pdev->device == device)
> > return true;
>
> > + for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
> > + if (id->vendor == vendor && id->device == device)
>
> > + break;
>
> return true;
>
> > return id && id->vendor;
>
> return false;
Good cleanup for a follow-up patch, but doesn't seem directly related
to the objective here. The current patch is:
@@ -80,7 +80,7 @@ static struct resource video_rom_resource = {
*/
static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned
short device)
{
- struct pci_driver *drv = pdev->driver;
+ struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
const struct pci_device_id *id;
if (pdev->vendor == vendor && pdev->device == device)
> > device_lock(_dev->dev);
> > - if (vf_dev->dev.driver) {
> > + if (to_pci_driver(vf_dev->dev.driver)) {
>
> Hmm...
Yeah, it could be either of:
if (to_pci_driver(vf_dev->dev.driver))
if (vf_dev->dev.driver)
I went back and forth on that and went with to_pci_driver() on the
theory that we were testing the pci_driver * before and the patch is
more of a mechanical change and easier to review if we test the
pci_driver * after.
> > + if (!pci_dev->state_saved && pci_dev->current_state !=
> > PCI_D0
>
> > + && pci_dev->current_state != PCI_UNKNOWN) {
>
> Can we keep && on the previous line?
I think this is in pci_legacy_suspend(), and I didn't touch that line.
It shows up in the interdiff because without the NULL check in
to_pci_driver(), we had to indent this code another level. With the
NULL check, we don't need that extra indentation.
> > + pci_WARN_ONCE(pci_dev, pci_dev->current_state !=
> > prev,
> > + "PCI PM: Device state not saved by
> > %pS\n",
> > + drv->suspend);
> > }
>
> ...
>
> > + return drv && drv->resume ?
> > + drv->resume(pci_dev) :
> > pci_pm_reenable_device(pci_dev);
>
> One line?
I don't think I touched that line.
> > + struct pci_driver *drv = to_pci_driver(dev->dev.driver);
> > const struct pci_error_handlers *err_handler =
> > - dev->dev.driver ?
> > to_pci_driver(dev->dev.driver)->err_handler : NULL;
> > +
Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver
Hello,
On Wed, Oct 13, 2021 at 05:54:28AM -0500, Bjorn Helgaas wrote:
> On Wed, Oct 13, 2021 at 10:51:31AM +0200, Uwe Kleine-König wrote:
> > On Tue, Oct 12, 2021 at 06:32:12PM -0500, Bjorn Helgaas wrote:
> > > diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
> > > index d997c9c3ebb5..7eb3706cf42d 100644
> > > --- a/drivers/misc/cxl/guest.c
> > > +++ b/drivers/misc/cxl/guest.c
> > > @@ -20,38 +20,38 @@ static void pci_error_handlers(struct cxl_afu *afu,
> > > pci_channel_state_t state)
> > > {
> > > struct pci_dev *afu_dev;
> > > + struct pci_driver *afu_drv;
> > > + struct pci_error_handlers *err_handler;
> >
> > These two could be moved into the for loop (where afu_drv was with my
> > patch already). This is also possible in a few other drivers.
>
> That's true, they could. I tried to follow the prevailing style in
> the file. At least in cxl, I didn't see any other cases of
> declarations being in the minimal scope like that.
I don't care much, do whatever you consider nice. I'm happy you liked
the cleanup and that you took it.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver
On Wed, Oct 13, 2021 at 10:51:31AM +0200, Uwe Kleine-König wrote:
> On Tue, Oct 12, 2021 at 06:32:12PM -0500, Bjorn Helgaas wrote:
> > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > >
> > > this is v6 of the quest to drop the "driver" member from struct pci_dev
> > > which tracks the same data (apart from a constant offset) as dev.driver.
> >
> > I like this a lot and applied it to pci/driver for v5.16, thanks!
> >
> > I split some of the bigger patches apart so they only touched one
> > driver or subsystem at a time. I also updated to_pci_driver() so it
> > returns NULL when given NULL, which makes some of the validations
> > quite a bit simpler, especially in the PM code in pci-driver.c.
>
> OK.
>
> > Full interdiff from this v6 series:
> >
> > diff --git a/arch/x86/kernel/probe_roms.c b/arch/x86/kernel/probe_roms.c
> > index deaaef6efe34..36e84d904260 100644
> > --- a/arch/x86/kernel/probe_roms.c
> > +++ b/arch/x86/kernel/probe_roms.c
> > @@ -80,17 +80,15 @@ static struct resource video_rom_resource = {
> > */
> > static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned
> > short device)
> > {
> > + struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
> > const struct pci_device_id *id;
> >
> > if (pdev->vendor == vendor && pdev->device == device)
> > return true;
> >
> > - if (pdev->dev.driver) {
> > - struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
> > - for (id = drv->id_table; id && id->vendor; id++)
> > - if (id->vendor == vendor && id->device == device)
> > - break;
> > - }
> > + for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
> > + if (id->vendor == vendor && id->device == device)
> > + break;
> >
> > return id && id->vendor;
> > }
> > diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
> > index d997c9c3ebb5..7eb3706cf42d 100644
> > --- a/drivers/misc/cxl/guest.c
> > +++ b/drivers/misc/cxl/guest.c
> > @@ -20,38 +20,38 @@ static void pci_error_handlers(struct cxl_afu *afu,
> > pci_channel_state_t state)
> > {
> > struct pci_dev *afu_dev;
> > + struct pci_driver *afu_drv;
> > + struct pci_error_handlers *err_handler;
>
> These two could be moved into the for loop (where afu_drv was with my
> patch already). This is also possible in a few other drivers.
That's true, they could. I tried to follow the prevailing style in
the file. At least in cxl, I didn't see any other cases of
declarations being in the minimal scope like that.
Bjorn
[PATCH] powerpc/64s: Default to 64K pages for 64 bit book3s
For 64-bit book3s the default should be 64K as that's what modern CPUs are designed for. The following defconfigs already set CONFIG_PPC_64K_PAGES: cell_defconfig pasemi_defconfig powernv_defconfig ppc64_defconfig pseries_defconfig skiroot_defconfig The have the option removed from the defconfig, as it is now the default. The defconfigs that now need to set CONFIG_PPC_4K_PAGES to maintain their existing behaviour are: g5_defconfig maple_defconfig microwatt_defconfig ps3_defconfig Link: https://github.com/linuxppc/issues/issues/109 Signed-off-by: Joel Stanley --- arch/powerpc/Kconfig | 1 + arch/powerpc/configs/cell_defconfig | 1 - arch/powerpc/configs/g5_defconfig| 1 + arch/powerpc/configs/maple_defconfig | 1 + arch/powerpc/configs/microwatt_defconfig | 2 +- arch/powerpc/configs/pasemi_defconfig| 1 - arch/powerpc/configs/powernv_defconfig | 1 - arch/powerpc/configs/ppc64_defconfig | 1 - arch/powerpc/configs/ps3_defconfig | 1 + arch/powerpc/configs/pseries_defconfig | 1 - arch/powerpc/configs/skiroot_defconfig | 1 - 11 files changed, 5 insertions(+), 7 deletions(-) diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 8a584414ef67..e2c220fa91c0 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -708,6 +708,7 @@ config ARCH_MEMORY_PROBE choice prompt "Page size" + default PPC_64K_PAGES if PPC_BOOK3S_64 default PPC_4K_PAGES help Select the kernel logical page size. Increasing the page size diff --git a/arch/powerpc/configs/cell_defconfig b/arch/powerpc/configs/cell_defconfig index cc2c0d51f493..7fd9e596ea33 100644 --- a/arch/powerpc/configs/cell_defconfig +++ b/arch/powerpc/configs/cell_defconfig @@ -36,7 +36,6 @@ CONFIG_GEN_RTC=y CONFIG_BINFMT_MISC=m CONFIG_IRQ_ALL_CPUS=y CONFIG_NUMA=y -CONFIG_PPC_64K_PAGES=y CONFIG_SCHED_SMT=y CONFIG_PCIEPORTBUS=y CONFIG_NET=y diff --git a/arch/powerpc/configs/g5_defconfig b/arch/powerpc/configs/g5_defconfig index 63d611cc160f..9d6212a8b195 100644 --- a/arch/powerpc/configs/g5_defconfig +++ b/arch/powerpc/configs/g5_defconfig @@ -26,6 +26,7 @@ CONFIG_CPU_FREQ_PMAC64=y CONFIG_GEN_RTC=y CONFIG_KEXEC=y CONFIG_IRQ_ALL_CPUS=y +CONFIG_PPC_4K_PAGES=y CONFIG_PCI_MSI=y CONFIG_NET=y CONFIG_PACKET=y diff --git a/arch/powerpc/configs/maple_defconfig b/arch/powerpc/configs/maple_defconfig index 9424c1e67e1c..c821a97f4a89 100644 --- a/arch/powerpc/configs/maple_defconfig +++ b/arch/powerpc/configs/maple_defconfig @@ -25,6 +25,7 @@ CONFIG_UDBG_RTAS_CONSOLE=y CONFIG_GEN_RTC=y CONFIG_KEXEC=y CONFIG_IRQ_ALL_CPUS=y +CONFIG_PPC_4K_PAGES=y CONFIG_PCI_MSI=y CONFIG_NET=y CONFIG_PACKET=y diff --git a/arch/powerpc/configs/microwatt_defconfig b/arch/powerpc/configs/microwatt_defconfig index 9465209b8c5b..556ec5eec684 100644 --- a/arch/powerpc/configs/microwatt_defconfig +++ b/arch/powerpc/configs/microwatt_defconfig @@ -1,7 +1,6 @@ # CONFIG_SWAP is not set # CONFIG_CROSS_MEMORY_ATTACH is not set CONFIG_HIGH_RES_TIMERS=y -CONFIG_PREEMPT_VOLUNTARY=y CONFIG_TICK_CPU_ACCOUNTING=y CONFIG_LOG_BUF_SHIFT=16 CONFIG_PRINTK_SAFE_LOG_BUF_SHIFT=12 @@ -26,6 +25,7 @@ CONFIG_PPC_MICROWATT=y # CONFIG_PPC_OF_BOOT_TRAMPOLINE is not set CONFIG_CPU_FREQ=y CONFIG_HZ_100=y +CONFIG_PPC_4K_PAGES=y # CONFIG_PPC_MEM_KEYS is not set # CONFIG_SECCOMP is not set # CONFIG_MQ_IOSCHED_KYBER is not set diff --git a/arch/powerpc/configs/pasemi_defconfig b/arch/powerpc/configs/pasemi_defconfig index 78606b7e42df..e00a703581c3 100644 --- a/arch/powerpc/configs/pasemi_defconfig +++ b/arch/powerpc/configs/pasemi_defconfig @@ -22,7 +22,6 @@ CONFIG_CPU_FREQ_GOV_POWERSAVE=y CONFIG_CPU_FREQ_GOV_USERSPACE=y CONFIG_CPU_FREQ_GOV_ONDEMAND=y CONFIG_HZ_1000=y -CONFIG_PPC_64K_PAGES=y # CONFIG_SECCOMP is not set CONFIG_PCI_MSI=y CONFIG_PCCARD=y diff --git a/arch/powerpc/configs/powernv_defconfig b/arch/powerpc/configs/powernv_defconfig index 8bfeea6c7de7..49f49c263935 100644 --- a/arch/powerpc/configs/powernv_defconfig +++ b/arch/powerpc/configs/powernv_defconfig @@ -62,7 +62,6 @@ CONFIG_MEMORY_FAILURE=y CONFIG_HWPOISON_INJECT=m CONFIG_TRANSPARENT_HUGEPAGE=y CONFIG_DEFERRED_STRUCT_PAGE_INIT=y -CONFIG_PPC_64K_PAGES=y CONFIG_SCHED_SMT=y CONFIG_PM=y CONFIG_HOTPLUG_PCI=y diff --git a/arch/powerpc/configs/ppc64_defconfig b/arch/powerpc/configs/ppc64_defconfig index 0ad2291337a7..203d0b7f0bb8 100644 --- a/arch/powerpc/configs/ppc64_defconfig +++ b/arch/powerpc/configs/ppc64_defconfig @@ -52,7 +52,6 @@ CONFIG_KEXEC_FILE=y CONFIG_CRASH_DUMP=y CONFIG_FA_DUMP=y CONFIG_IRQ_ALL_CPUS=y -CONFIG_PPC_64K_PAGES=y CONFIG_SCHED_SMT=y CONFIG_HOTPLUG_PCI=y CONFIG_HOTPLUG_PCI_RPA=m diff --git a/arch/powerpc/configs/ps3_defconfig b/arch/powerpc/configs/ps3_defconfig index f300dcb937cc..7c95fab4b920 100644 --- a/arch/powerpc/configs/ps3_defconfig +++ b/arch/powerpc/configs/ps3_defconfig @@ -30,6 +30,7 @@ CONFIG_PS3_LPM=m # CONFIG_CORE_DUMP_DEFAULT_ELF_HEADERS is not
Re: linux-next: build warnings in Linus' tree
On Wed, Oct 13, 2021 at 5:28 PM Anatolij Gustschin wrote: > > On Wed, 13 Oct 2021 17:17:25 -0500 > Rob Herring robh...@kernel.org wrote: > ... > >In general, you shouldn't need to be changing the drivers. Can you > >tell me which warnings need driver changes? > > ethernet and mdio drivers share registers, so they use same unit-address: > > arch/powerpc/boot/dts/tqm5200.dts:127.17-133.5: Warning > (unique_unit_address): /soc5200@f000/ethernet@3000: duplicate > unit-address (also used in node /soc5200@f000/mdio@3000) > > arch/powerpc/boot/dts/mpc5200b.dtsi:218.23-223.5: Warning > (unique_unit_address): /soc5200@f000/ethernet@3000: duplicate > unit-address (also used in node /soc5200@f000/mdio@3000) > also defined at arch/powerpc/boot/dts/digsy_mtc.dts:60.17-62.5 Those are W=1 warnings and off by default. You shouldn't fix them if it breaks compatibility with the driver. Rob
[PATCH] powerpc/5200: dts: fix psc node warning
Fix build warning:
Warning (spi_bus_bridge): /soc5200@f000/psc@2000: node name for SPI buses
should be 'spi'
Signed-off-by: Anatolij Gustschin
---
arch/powerpc/boot/dts/motionpro.dts | 4
arch/powerpc/boot/dts/o2d.dtsi | 6 +-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/boot/dts/motionpro.dts
b/arch/powerpc/boot/dts/motionpro.dts
index 09f0eaa4ad49..cc217ddd50a0 100644
--- a/arch/powerpc/boot/dts/motionpro.dts
+++ b/arch/powerpc/boot/dts/motionpro.dts
@@ -19,6 +19,8 @@
label = "motionpro-readyled";
};
+/delete-node/
+
/ {
model = "promess,motionpro";
compatible = "promess,motionpro";
@@ -35,6 +37,8 @@
// PSC2 in spi master mode
psc@2200 { // PSC2
compatible =
"fsl,mpc5200b-psc-spi","fsl,mpc5200-psc-spi";
+ reg = <0x2200 0x100>;
+ interrupts = <2 2 0>;
cell-index = <1>;
};
diff --git a/arch/powerpc/boot/dts/o2d.dtsi b/arch/powerpc/boot/dts/o2d.dtsi
index 898fe08a9fd0..34a836a37084 100644
--- a/arch/powerpc/boot/dts/o2d.dtsi
+++ b/arch/powerpc/boot/dts/o2d.dtsi
@@ -15,6 +15,8 @@
};
{ gpio-controller; };
+/delete-node/
+
/ {
model = "ifm,o2d";
compatible = "ifm,o2d";
@@ -29,8 +31,10 @@
status = "disabled";
};
- psc@2000 { // PSC1
+ spi@2000 { // PSC1
compatible =
"fsl,mpc5200b-psc-spi","fsl,mpc5200-psc-spi";
+ reg = <0x2000 0x100>;
+ interrupts = <2 1 0>;
#address-cells = <1>;
#size-cells = <0>;
cell-index = <0>;
--
2.17.1
Re: linux-next: build warnings in Linus' tree
On Wed, 13 Oct 2021 17:17:25 -0500 Rob Herring robh...@kernel.org wrote: ... >In general, you shouldn't need to be changing the drivers. Can you >tell me which warnings need driver changes? ethernet and mdio drivers share registers, so they use same unit-address: arch/powerpc/boot/dts/tqm5200.dts:127.17-133.5: Warning (unique_unit_address): /soc5200@f000/ethernet@3000: duplicate unit-address (also used in node /soc5200@f000/mdio@3000) arch/powerpc/boot/dts/mpc5200b.dtsi:218.23-223.5: Warning (unique_unit_address): /soc5200@f000/ethernet@3000: duplicate unit-address (also used in node /soc5200@f000/mdio@3000) also defined at arch/powerpc/boot/dts/digsy_mtc.dts:60.17-62.5 Anatolij
Re: linux-next: build warnings in Linus' tree
On Wed, Oct 13, 2021 at 5:12 PM Anatolij Gustschin wrote: > > Hi Arnd, Rob, > > On Tue, 12 Oct 2021 16:39:56 +0200 > Arnd Bergmann a...@arndb.de wrote: > ... > >Grant Likely was the original maintainer for MPC52xx until 2011, > >Anatolij Gustschin is still listed as maintainer since then but hasn't > >been active in it for a while either. Anatolij can probably best judge > >which of these boards are still in going to be used with future kernels, > >but I suspect once you start removing bits from 52xx, the newer > >but less common 512x platform can go away as well. > > many of these boards are still used, i.e. o2d*, digsy_mtc, tqm5200. > I've sent first series to fix some warnings. Other dts fixes > require driver changes, so it will take some time to fix them. In general, you shouldn't need to be changing the drivers. Can you tell me which warnings need driver changes? Rob
Re: linux-next: build warnings in Linus' tree
Hi Arnd, Rob, On Tue, 12 Oct 2021 16:39:56 +0200 Arnd Bergmann a...@arndb.de wrote: ... >Grant Likely was the original maintainer for MPC52xx until 2011, >Anatolij Gustschin is still listed as maintainer since then but hasn't >been active in it for a while either. Anatolij can probably best judge >which of these boards are still in going to be used with future kernels, >but I suspect once you start removing bits from 52xx, the newer >but less common 512x platform can go away as well. many of these boards are still used, i.e. o2d*, digsy_mtc, tqm5200. I've sent first series to fix some warnings. Other dts fixes require driver changes, so it will take some time to fix them. Anatolij
[PATCH 1/4] powerpc/5200: dts: add missing pci ranges
Add ranges property to fix build warnings:
Warning (pci_bridge): /pci@fd00: missing ranges for PCI bridge (or not a
bridge)
Signed-off-by: Anatolij Gustschin
---
arch/powerpc/boot/dts/mpc5200b.dtsi | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/boot/dts/mpc5200b.dtsi
b/arch/powerpc/boot/dts/mpc5200b.dtsi
index 648fe31795f4..8c645d2bc455 100644
--- a/arch/powerpc/boot/dts/mpc5200b.dtsi
+++ b/arch/powerpc/boot/dts/mpc5200b.dtsi
@@ -276,7 +276,9 @@
clock-frequency = <0>; // From boot loader
interrupts = <2 8 0 2 9 0 2 10 0>;
bus-range = <0 0>;
- // ranges = need to add
+ ranges = <0x4200 0 0x8000 0x8000 0 0x1000>,
+<0x0200 0 0x9000 0x9000 0 0x1000>,
+<0x0100 0 0x 0xa000 0 0x0100>;
};
localbus: localbus {
--
2.17.1
[PATCH 4/4] powerpc/5200: dts: fix localbus node warnings
Fix build warnings like:
localbus:ranges: 'oneOf' conditional failed, one must be fixed
...
Warning (unit_address_vs_reg): /localbus: node has a reg or ranges property,
but no unit name
Warning (simple_bus_reg): /localbus/flash@0,0: simple-bus unit address format
error, expected "0"
Signed-off-by: Anatolij Gustschin
---
arch/powerpc/boot/dts/a3m071.dts| 12 +-
arch/powerpc/boot/dts/a4m072.dts| 20 -
arch/powerpc/boot/dts/charon.dts| 14 ++--
arch/powerpc/boot/dts/cm5200.dts| 7 --
arch/powerpc/boot/dts/digsy_mtc.dts | 16 --
arch/powerpc/boot/dts/lite5200.dts | 4 ++--
arch/powerpc/boot/dts/lite5200b.dts | 6 +++--
arch/powerpc/boot/dts/media5200.dts | 20 +
arch/powerpc/boot/dts/motionpro.dts | 32 +++
arch/powerpc/boot/dts/mpc5200b.dtsi | 2 +-
arch/powerpc/boot/dts/mucmc52.dts | 34 +++--
arch/powerpc/boot/dts/o2d.dts | 10 +
arch/powerpc/boot/dts/o2d.dtsi | 12 +-
arch/powerpc/boot/dts/o2d300.dts| 10 +
arch/powerpc/boot/dts/o2dnt2.dts| 10 +
arch/powerpc/boot/dts/o2i.dts | 4 ++--
arch/powerpc/boot/dts/o2mnt.dts | 4 ++--
arch/powerpc/boot/dts/o3dnt.dts | 10 +
arch/powerpc/boot/dts/pcm030.dts| 2 +-
arch/powerpc/boot/dts/pcm032.dts| 26 --
arch/powerpc/boot/dts/tqm5200.dts | 4 ++--
arch/powerpc/boot/dts/uc101.dts | 14 +++-
22 files changed, 151 insertions(+), 122 deletions(-)
diff --git a/arch/powerpc/boot/dts/a3m071.dts b/arch/powerpc/boot/dts/a3m071.dts
index 034cfd8aa95b..14e59aaa0ba7 100644
--- a/arch/powerpc/boot/dts/a3m071.dts
+++ b/arch/powerpc/boot/dts/a3m071.dts
@@ -87,15 +87,15 @@
};
};
- localbus {
+ localbus@8000 {
compatible = "fsl,mpc5200b-lpb","simple-bus";
#address-cells = <2>;
#size-cells = <1>;
- ranges = <0 0 0xfc00 0x0200
- 3 0 0xe900 0x0008
- 5 0 0xe800 0x0001>;
+ ranges = <0 0 0xfc00 0x0200>,
+<3 0 0xe900 0x0008>,
+<5 0 0xe800 0x0001>;
- flash@0,0 {
+ flash@0 {
#address-cells = <1>;
#size-cells = <1>;
reg = <0 0x0 0x0200>;
@@ -124,7 +124,7 @@
};
};
- fpga@3,0 {
+ fpga@3 {
compatible = "anonymous,a3m071-fpga";
reg = <3 0x0 0x0008
5 0x0 0x0001>;
diff --git a/arch/powerpc/boot/dts/a4m072.dts b/arch/powerpc/boot/dts/a4m072.dts
index d4270a2ec6c7..dede711957da 100644
--- a/arch/powerpc/boot/dts/a4m072.dts
+++ b/arch/powerpc/boot/dts/a4m072.dts
@@ -98,25 +98,25 @@
};
};
- localbus {
+ localbus@8000 {
compatible = "fsl,mpc5200b-lpb","simple-bus";
#address-cells = <2>;
#size-cells = <1>;
- ranges = <0 0 0xfe00 0x0200
- 1 0 0x6200 0x0040
- 2 0 0x6400 0x0020
- 3 0 0x6600 0x0100
- 6 0 0x6800 0x0100
- 7 0 0x6a00 0x0004>;
-
- flash@0,0 {
+ ranges = <0 0 0xfe00 0x0200>,
+<1 0 0x6200 0x0040>,
+<2 0 0x6400 0x0020>,
+<3 0 0x6600 0x0100>,
+<6 0 0x6800 0x0100>,
+<7 0 0x6a00 0x0004>;
+
+ flash@0 {
compatible = "cfi-flash";
reg = <0 0 0x0200>;
bank-width = <2>;
#size-cells = <1>;
#address-cells = <1>;
};
- sram0@1,0 {
+ sram0@1 {
compatible = "mtd-ram";
reg = <1 0x0 0x0040>;
bank-width = <2>;
diff --git a/arch/powerpc/boot/dts/charon.dts b/arch/powerpc/boot/dts/charon.dts
index ea6e76ae2545..2e3518909321 100644
--- a/arch/powerpc/boot/dts/charon.dts
+++ b/arch/powerpc/boot/dts/charon.dts
@@ -177,15 +177,15 @@
};
};
- localbus {
+ localbus@8000 {
compatible = "fsl,mpc5200-lpb","simple-bus";
#address-cells = <2>;
#size-cells = <1>;
- ranges = < 0 0 0xfc00 0x0200
- 1 0 0xe000 0x0400 // CS1 range, SM501
- 3 0
[PATCH 3/4] powerpc/5200: dts: fix memory node unit name
Fixes build warnings:
Warning (unit_address_vs_reg): /memory: node has a reg or ranges property, but
no unit name
Signed-off-by: Anatolij Gustschin
---
arch/powerpc/boot/dts/charon.dts| 2 +-
arch/powerpc/boot/dts/digsy_mtc.dts | 2 +-
arch/powerpc/boot/dts/lite5200.dts | 2 +-
arch/powerpc/boot/dts/lite5200b.dts | 2 +-
arch/powerpc/boot/dts/media5200.dts | 2 +-
arch/powerpc/boot/dts/mpc5200b.dtsi | 2 +-
arch/powerpc/boot/dts/o2d.dts | 2 +-
arch/powerpc/boot/dts/o2d.dtsi | 2 +-
arch/powerpc/boot/dts/o2dnt2.dts| 2 +-
arch/powerpc/boot/dts/o3dnt.dts | 2 +-
arch/powerpc/boot/dts/pcm032.dts| 2 +-
arch/powerpc/boot/dts/tqm5200.dts | 2 +-
12 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/boot/dts/charon.dts b/arch/powerpc/boot/dts/charon.dts
index 6f9fe88a5f36..ea6e76ae2545 100644
--- a/arch/powerpc/boot/dts/charon.dts
+++ b/arch/powerpc/boot/dts/charon.dts
@@ -35,7 +35,7 @@
};
};
- memory {
+ memory@0 {
device_type = "memory";
reg = <0x 0x0800>; // 128MB
};
diff --git a/arch/powerpc/boot/dts/digsy_mtc.dts
b/arch/powerpc/boot/dts/digsy_mtc.dts
index 010156649bfe..57024a4c1e7d 100644
--- a/arch/powerpc/boot/dts/digsy_mtc.dts
+++ b/arch/powerpc/boot/dts/digsy_mtc.dts
@@ -16,7 +16,7 @@
model = "intercontrol,digsy-mtc";
compatible = "intercontrol,digsy-mtc";
- memory {
+ memory@0 {
reg = <0x 0x0200>; // 32MB
};
diff --git a/arch/powerpc/boot/dts/lite5200.dts
b/arch/powerpc/boot/dts/lite5200.dts
index 84a12e7915e9..b9d8487813b4 100644
--- a/arch/powerpc/boot/dts/lite5200.dts
+++ b/arch/powerpc/boot/dts/lite5200.dts
@@ -32,7 +32,7 @@
};
};
- memory {
+ memory@0 {
device_type = "memory";
reg = <0x 0x0400>; // 64MB
};
diff --git a/arch/powerpc/boot/dts/lite5200b.dts
b/arch/powerpc/boot/dts/lite5200b.dts
index c361c59a9681..7e2d91c7cb66 100644
--- a/arch/powerpc/boot/dts/lite5200b.dts
+++ b/arch/powerpc/boot/dts/lite5200b.dts
@@ -31,7 +31,7 @@
led4 { gpios = <_simple 2 1>; };
};
- memory {
+ memory@0 {
reg = <0x 0x1000>; // 256MB
};
diff --git a/arch/powerpc/boot/dts/media5200.dts
b/arch/powerpc/boot/dts/media5200.dts
index f7d7538eb69d..96524ede16cd 100644
--- a/arch/powerpc/boot/dts/media5200.dts
+++ b/arch/powerpc/boot/dts/media5200.dts
@@ -32,7 +32,7 @@
};
};
- memory {
+ memory@0 {
reg = <0x 0x0800>; // 128MB RAM
};
diff --git a/arch/powerpc/boot/dts/mpc5200b.dtsi
b/arch/powerpc/boot/dts/mpc5200b.dtsi
index 8c645d2bc455..ffa82c7e1055 100644
--- a/arch/powerpc/boot/dts/mpc5200b.dtsi
+++ b/arch/powerpc/boot/dts/mpc5200b.dtsi
@@ -33,7 +33,7 @@
};
};
- memory: memory {
+ memory: memory@0 {
device_type = "memory";
reg = <0x 0x0400>; // 64MB
};
diff --git a/arch/powerpc/boot/dts/o2d.dts b/arch/powerpc/boot/dts/o2d.dts
index 24a46f65e529..e0a8d3034417 100644
--- a/arch/powerpc/boot/dts/o2d.dts
+++ b/arch/powerpc/boot/dts/o2d.dts
@@ -12,7 +12,7 @@
model = "ifm,o2d";
compatible = "ifm,o2d";
- memory {
+ memory@0 {
reg = <0x 0x0800>; // 128MB
};
diff --git a/arch/powerpc/boot/dts/o2d.dtsi b/arch/powerpc/boot/dts/o2d.dtsi
index 6661955a2be4..b55a9e5bd828 100644
--- a/arch/powerpc/boot/dts/o2d.dtsi
+++ b/arch/powerpc/boot/dts/o2d.dtsi
@@ -19,7 +19,7 @@
model = "ifm,o2d";
compatible = "ifm,o2d";
- memory {
+ memory@0 {
reg = <0x 0x0400>; // 64MB
};
diff --git a/arch/powerpc/boot/dts/o2dnt2.dts b/arch/powerpc/boot/dts/o2dnt2.dts
index eeba7f5507d5..c2eedbd1f5fc 100644
--- a/arch/powerpc/boot/dts/o2dnt2.dts
+++ b/arch/powerpc/boot/dts/o2dnt2.dts
@@ -12,7 +12,7 @@
model = "ifm,o2dnt2";
compatible = "ifm,o2d";
- memory {
+ memory@0 {
reg = <0x 0x0800>; // 128MB
};
diff --git a/arch/powerpc/boot/dts/o3dnt.dts b/arch/powerpc/boot/dts/o3dnt.dts
index fd00396b0593..e4c1bdd41271 100644
--- a/arch/powerpc/boot/dts/o3dnt.dts
+++ b/arch/powerpc/boot/dts/o3dnt.dts
@@ -12,7 +12,7 @@
model = "ifm,o3dnt";
compatible = "ifm,o2d";
- memory {
+ memory@0 {
reg = <0x 0x0400>; // 64MB
};
diff --git a/arch/powerpc/boot/dts/pcm032.dts b/arch/powerpc/boot/dts/pcm032.dts
index b6fdf861c7d6..d00f13b62510 100644
--- a/arch/powerpc/boot/dts/pcm032.dts
+++ b/arch/powerpc/boot/dts/pcm032.dts
@@ -20,7 +20,7 @@
model = "phytec,pcm032";
compatible = "phytec,pcm032";
- memory {
+ memory@0 {
[PATCH 0/4] Update mpc5200 dts files to fix warnings
This series fixes localbus, memory and pci node build warnings. It was tested with current linux-next on digsy_mtc and tqm5200 boards. Anatolij Gustschin (4): powerpc/5200: dts: add missing pci ranges powerpc/5200: dts: fix pci ranges warnings powerpc/5200: dts: fix memory node unit name powerpc/5200: dts: fix localbus node warnings arch/powerpc/boot/dts/a3m071.dts| 12 - arch/powerpc/boot/dts/a4m072.dts| 26 ++-- arch/powerpc/boot/dts/charon.dts| 22 - arch/powerpc/boot/dts/cm5200.dts| 7 -- arch/powerpc/boot/dts/digsy_mtc.dts | 24 +- arch/powerpc/boot/dts/lite5200.dts | 12 - arch/powerpc/boot/dts/lite5200b.dts | 14 ++- arch/powerpc/boot/dts/media5200.dts | 28 +++-- arch/powerpc/boot/dts/motionpro.dts | 32 +--- arch/powerpc/boot/dts/mpc5200b.dtsi | 8 +++--- arch/powerpc/boot/dts/mucmc52.dts | 38 +++-- arch/powerpc/boot/dts/o2d.dts | 12 + arch/powerpc/boot/dts/o2d.dtsi | 14 ++- arch/powerpc/boot/dts/o2d300.dts| 10 +--- arch/powerpc/boot/dts/o2dnt2.dts| 12 + arch/powerpc/boot/dts/o2i.dts | 4 +-- arch/powerpc/boot/dts/o2mnt.dts | 4 +-- arch/powerpc/boot/dts/o3dnt.dts | 12 + arch/powerpc/boot/dts/pcm030.dts| 8 +++--- arch/powerpc/boot/dts/pcm032.dts| 34 ++ arch/powerpc/boot/dts/tqm5200.dts | 12 - arch/powerpc/boot/dts/uc101.dts | 14 ++- 22 files changed, 195 insertions(+), 164 deletions(-) -- 2.17.1
[PATCH 2/4] powerpc/5200: dts: fix pci ranges warnings
Fix ranges property warnings:
pci@fd00:ranges: 'oneOf' conditional failed, one must be fixed:
Signed-off-by: Anatolij Gustschin
---
arch/powerpc/boot/dts/a4m072.dts| 6 +++---
arch/powerpc/boot/dts/charon.dts| 6 +++---
arch/powerpc/boot/dts/digsy_mtc.dts | 6 +++---
arch/powerpc/boot/dts/lite5200.dts | 6 +++---
arch/powerpc/boot/dts/lite5200b.dts | 6 +++---
arch/powerpc/boot/dts/media5200.dts | 6 +++---
arch/powerpc/boot/dts/mucmc52.dts | 6 +++---
arch/powerpc/boot/dts/pcm030.dts| 6 +++---
arch/powerpc/boot/dts/pcm032.dts| 6 +++---
arch/powerpc/boot/dts/tqm5200.dts | 6 +++---
10 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/arch/powerpc/boot/dts/a4m072.dts b/arch/powerpc/boot/dts/a4m072.dts
index a9cef5726422..d4270a2ec6c7 100644
--- a/arch/powerpc/boot/dts/a4m072.dts
+++ b/arch/powerpc/boot/dts/a4m072.dts
@@ -140,8 +140,8 @@
clock-frequency = <0>; /* From boot loader */
interrupts = <2 8 0 2 9 0 2 10 0>;
bus-range = <0 0>;
- ranges = <0x4200 0 0x8000 0x8000 0 0x1000
- 0x0200 0 0x9000 0x9000 0 0x1000
- 0x0100 0 0x 0xa000 0 0x0100>;
+ ranges = <0x4200 0 0x8000 0x8000 0 0x1000>,
+<0x0200 0 0x9000 0x9000 0 0x1000>,
+<0x0100 0 0x 0xa000 0 0x0100>;
};
};
diff --git a/arch/powerpc/boot/dts/charon.dts b/arch/powerpc/boot/dts/charon.dts
index 408b486b13df..6f9fe88a5f36 100644
--- a/arch/powerpc/boot/dts/charon.dts
+++ b/arch/powerpc/boot/dts/charon.dts
@@ -225,8 +225,8 @@
clock-frequency = <0>; // From boot loader
interrupts = <2 8 0 2 9 0 2 10 0>;
bus-range = <0 0>;
- ranges = <0x4200 0 0x8000 0x8000 0 0x1000
- 0x0200 0 0x9000 0x9000 0 0x1000
- 0x0100 0 0x 0xa000 0 0x0100>;
+ ranges = <0x4200 0 0x8000 0x8000 0 0x1000>,
+<0x0200 0 0x9000 0x9000 0 0x1000>,
+<0x0100 0 0x 0xa000 0 0x0100>;
};
};
diff --git a/arch/powerpc/boot/dts/digsy_mtc.dts
b/arch/powerpc/boot/dts/digsy_mtc.dts
index 0e5e9d3acf79..010156649bfe 100644
--- a/arch/powerpc/boot/dts/digsy_mtc.dts
+++ b/arch/powerpc/boot/dts/digsy_mtc.dts
@@ -98,9 +98,9 @@
clock-frequency = <0>; // From boot loader
interrupts = <2 8 0 2 9 0 2 10 0>;
bus-range = <0 0>;
- ranges = <0x4200 0 0x8000 0x8000 0 0x1000
- 0x0200 0 0x9000 0x9000 0 0x1000
- 0x0100 0 0x 0xa000 0 0x0100>;
+ ranges = <0x4200 0 0x8000 0x8000 0 0x1000>,
+<0x0200 0 0x9000 0x9000 0 0x1000>,
+<0x0100 0 0x 0xa000 0 0x0100>;
};
localbus {
diff --git a/arch/powerpc/boot/dts/lite5200.dts
b/arch/powerpc/boot/dts/lite5200.dts
index cb2782dd6132..84a12e7915e9 100644
--- a/arch/powerpc/boot/dts/lite5200.dts
+++ b/arch/powerpc/boot/dts/lite5200.dts
@@ -283,9 +283,9 @@
clock-frequency = <0>; // From boot loader
interrupts = <2 8 0 2 9 0 2 10 0>;
bus-range = <0 0>;
- ranges = <0x4200 0 0x8000 0x8000 0 0x2000
- 0x0200 0 0xa000 0xa000 0 0x1000
- 0x0100 0 0x 0xb000 0 0x0100>;
+ ranges = <0x4200 0 0x8000 0x8000 0 0x2000>,
+<0x0200 0 0xa000 0xa000 0 0x1000>,
+<0x0100 0 0x 0xb000 0 0x0100>;
};
localbus {
diff --git a/arch/powerpc/boot/dts/lite5200b.dts
b/arch/powerpc/boot/dts/lite5200b.dts
index 2b86c81f9048..c361c59a9681 100644
--- a/arch/powerpc/boot/dts/lite5200b.dts
+++ b/arch/powerpc/boot/dts/lite5200b.dts
@@ -116,9 +116,9 @@
clock-frequency = <0>; // From boot loader
interrupts = <2 8 0 2 9 0 2 10 0>;
bus-range = <0 0>;
- ranges = <0x4200 0 0x8000 0x8000 0 0x2000
- 0x0200 0 0xa000 0xa000 0 0x1000
- 0x0100 0 0x 0xb000 0 0x0100>;
+ ranges = <0x4200 0 0x8000 0x8000 0 0x2000>,
+<0x0200 0 0xa000 0xa000 0 0x1000>,
+<0x0100 0 0x 0xb000 0 0x0100>;
};
localbus {
diff --git a/arch/powerpc/boot/dts/media5200.dts
[PATCH v2] powerpc/s64: Clarify that radix lacks DEBUG_PAGEALLOC
The page_alloc.c code will call into __kernel_map_pages when
DEBUG_PAGEALLOC is configured and enabled.
As the implementation assumes hash, this should crash spectacularly if
not for a bit of luck in __kernel_map_pages. In this function
linear_map_hash_count is always zero, the for loop exits without doing
any damage.
There are no other platforms that determine if they support
debug_pagealloc at runtime. Instead of adding code to mm/page_alloc.c to
do that, this change turns the map/unmap into a noop when in radix
mode and prints a warning once.
Signed-off-by: Joel Stanley
---
v2: Put __kernel_map_pages in pgtable.h
arch/powerpc/include/asm/book3s/64/hash.h| 2 ++
arch/powerpc/include/asm/book3s/64/pgtable.h | 11 +++
arch/powerpc/include/asm/book3s/64/radix.h | 3 +++
arch/powerpc/mm/book3s64/hash_utils.c| 2 +-
arch/powerpc/mm/book3s64/radix_pgtable.c | 7 +++
5 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/book3s/64/hash.h
b/arch/powerpc/include/asm/book3s/64/hash.h
index d959b0195ad9..674fe0e890dc 100644
--- a/arch/powerpc/include/asm/book3s/64/hash.h
+++ b/arch/powerpc/include/asm/book3s/64/hash.h
@@ -255,6 +255,8 @@ int hash__create_section_mapping(unsigned long start,
unsigned long end,
int nid, pgprot_t prot);
int hash__remove_section_mapping(unsigned long start, unsigned long end);
+void hash__kernel_map_pages(struct page *page, int numpages, int enable);
+
#endif /* !__ASSEMBLY__ */
#endif /* __KERNEL__ */
#endif /* _ASM_POWERPC_BOOK3S_64_HASH_H */
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h
b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 5d34a8646f08..265661ded238 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -1101,6 +1101,17 @@ static inline void vmemmap_remove_mapping(unsigned long
start,
}
#endif
+#ifdef CONFIG_DEBUG_PAGEALLOC
+static inline void __kernel_map_pages(struct page *page, int numpages, int
enable)
+{
+ if (radix_enabled()) {
+ radix__kernel_map_pages(page, numpages, enable);
+ return;
+ }
+ hash__kernel_map_pages(page, numpages, enable);
+}
+#endif
+
static inline pte_t pmd_pte(pmd_t pmd)
{
return __pte_raw(pmd_raw(pmd));
diff --git a/arch/powerpc/include/asm/book3s/64/radix.h
b/arch/powerpc/include/asm/book3s/64/radix.h
index 59cab558e2f0..d090d9612348 100644
--- a/arch/powerpc/include/asm/book3s/64/radix.h
+++ b/arch/powerpc/include/asm/book3s/64/radix.h
@@ -316,5 +316,8 @@ int radix__create_section_mapping(unsigned long start,
unsigned long end,
int nid, pgprot_t prot);
int radix__remove_section_mapping(unsigned long start, unsigned long end);
#endif /* CONFIG_MEMORY_HOTPLUG */
+
+void radix__kernel_map_pages(struct page *page, int numpages, int enable);
+
#endif /* __ASSEMBLY__ */
#endif
diff --git a/arch/powerpc/mm/book3s64/hash_utils.c
b/arch/powerpc/mm/book3s64/hash_utils.c
index c145776d3ae5..cfd45245d009 100644
--- a/arch/powerpc/mm/book3s64/hash_utils.c
+++ b/arch/powerpc/mm/book3s64/hash_utils.c
@@ -1988,7 +1988,7 @@ static void kernel_unmap_linear_page(unsigned long vaddr,
unsigned long lmi)
mmu_kernel_ssize, 0);
}
-void __kernel_map_pages(struct page *page, int numpages, int enable)
+void hash__kernel_map_pages(struct page *page, int numpages, int enable)
{
unsigned long flags, vaddr, lmi;
int i;
diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c
b/arch/powerpc/mm/book3s64/radix_pgtable.c
index ae20add7954a..83b33418ad28 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -920,6 +920,13 @@ void __meminit radix__vmemmap_remove_mapping(unsigned long
start, unsigned long
#endif
#endif
+#ifdef CONFIG_DEBUG_PAGEALLOC
+void radix__kernel_map_pages(struct page *page, int numpages, int enable)
+{
+pr_warn_once("DEBUG_PAGEALLOC not supported in radix mode\n");
+}
+#endif
+
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
unsigned long radix__pmd_hugepage_update(struct mm_struct *mm, unsigned long
addr,
--
2.33.0
Re: [PATCH v3 02/52] powerpc/64s: guard optional TIDR SPR with CPU ftr test
Michael Ellerman writes: > Fabiano Rosas writes: >> Nicholas Piggin writes: >> >>> The TIDR SPR only exists on POWER9. Avoid accessing it when the >>> feature bit for it is not set. >> >> Not related to this patch, but how does this work with compat mode? A P9 >> compat mode guest would get an invalid instruction when trying to access >> this SPR? > > Good question. > > I assume you're talking about P9 compat mode on P10. > > In general compat mode only applies to userspace, because it's > implemented by setting the PCR which only (mostly?) applies to PR=1. > > I don't think there's any special casing in the ISA for the TIDR, so I > think it just falls into the unimplemented SPR case for mt/fspr. > > That's documented in Book III section 5.4.4, in particular on page 1171 > it says: > > Execution of this instruction specifying an SPR number > that is undefined for the implementation causes one of > the following. > • if spr[0]=0: > - if MSR[PR]=1: Hypervisor Emulation Assistance interrupt > - if MSR[PR]=0: Hypervisor Emulation Assistance interrupt for SPR > 0,4,5, and 6, and no operation (i.e., the instruction is treated > as a no-op) when LPCR[EVIRT]=0 and Hypervisor Emulation Assistance > interrupt when LPCR[EVIRT]=1 for all other SPRs I knew this must have been somewhere in there but had no idea how to find it. Thanks. > Linux doesn't set EVIRT, and I assume neither does phyp, so it behaves > like a nop. > > We actually use that behaviour in xmon to detect that an SPR is not > implemented, by noticing that the mfspr has no effect on the target > register, see dump_one_spr(). > > We should really write some docs on compat mode in the linuxppc wiki > and/or Documentation ;) Hmm I was not aware we had a wiki. I'll see if I can contribute something. I need to go learn all this stuff first, though =D. > > cheers
[PATCH] powerpc: Mark .opd section read-only
.opd section contains function descriptors used to locate
functions in the kernel. If someone is able to modify a
function descriptor he will be able to run arbitrary
kernel function instead of another.
To avoid that, move .opd section inside read-only memory.
Signed-off-by: Christophe Leroy
---
arch/powerpc/kernel/vmlinux.lds.S | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/kernel/vmlinux.lds.S
b/arch/powerpc/kernel/vmlinux.lds.S
index 40bdefe9caa7..18e42c74abdd 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -143,6 +143,12 @@ SECTIONS
SOFT_MASK_TABLE(8)
RESTART_TABLE(8)
+ .opd : AT(ADDR(.opd) - LOAD_OFFSET) {
+ __start_opd = .;
+ KEEP(*(.opd))
+ __end_opd = .;
+ }
+
. = ALIGN(8);
__stf_entry_barrier_fixup : AT(ADDR(__stf_entry_barrier_fixup) -
LOAD_OFFSET) {
__start___stf_entry_barrier_fixup = .;
@@ -339,12 +345,6 @@ SECTIONS
*(.branch_lt)
}
- .opd : AT(ADDR(.opd) - LOAD_OFFSET) {
- __start_opd = .;
- KEEP(*(.opd))
- __end_opd = .;
- }
-
. = ALIGN(256);
.got : AT(ADDR(.got) - LOAD_OFFSET) {
__toc_start = .;
--
2.31.1
[RFC PATCH v3 10/13] ASoC: fsl: asrc_dma: protect for_each_dpcm_be() loops
Follow the locking model used within soc-pcm.c
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/fsl/fsl_asrc_dma.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c
index cd9b36ec0ecb..b67097503836 100644
--- a/sound/soc/fsl/fsl_asrc_dma.c
+++ b/sound/soc/fsl/fsl_asrc_dma.c
@@ -151,6 +151,7 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component
*component,
int ret, width;
/* Fetch the Back-End dma_data from DPCM */
+ snd_soc_dpcm_fe_lock_irq(rtd, stream);
for_each_dpcm_be(rtd, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
struct snd_pcm_substream *substream_be;
@@ -164,6 +165,7 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component
*component,
dev_be = dai->dev;
break;
}
+ snd_soc_dpcm_fe_unlock_irq(rtd, stream);
if (!dma_params_be) {
dev_err(dev, "failed to get the substream of Back-End\n");
--
2.25.1
Re: [RESEND PATCH v2 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()
On Wed, 13 Oct 2021 16:34:53 +0800 王贇 wrote: > On 2021/10/13 下午4:25, Miroslav Benes wrote: > >>> Side note... the comment will eventually conflict with peterz's > >>> https://lore.kernel.org/all/20210929152429.125997...@infradead.org/. > >> > >> Steven, would you like to share your opinion on this patch? > >> > >> If klp_synchronize_transition() will be removed anyway, the comments > >> will be meaningless and we can just drop it :-P > > > > The comment will still be needed in some form. We will handle it depending > > on what gets merged first. peterz's patches are not ready yet. > > Ok, then I'll move it before trylock() inside klp_ftrace_handler() anyway. +1 -- Steve
Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()
Le 13/10/2021 à 09:48, Christophe Leroy a écrit :
Le 13/10/2021 à 09:39, Christophe Leroy a écrit :
Le 13/10/2021 à 09:23, Kees Cook a écrit :
On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
Behind a location, lkdtm_EXEC_RODATA() executes a real function,
not a copy of do_nothing().
So do it directly instead of using execute_location().
And fix displayed addresses by dereferencing the function descriptors.
Signed-off-by: Christophe Leroy
---
drivers/misc/lkdtm/perms.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
index 442d60ed25ef..da16564e1ecd 100644
--- a/drivers/misc/lkdtm/perms.c
+++ b/drivers/misc/lkdtm/perms.c
@@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
void lkdtm_EXEC_RODATA(void)
{
- execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
+ pr_info("attempting ok execution at %px\n",
+ dereference_symbol_descriptor(do_nothing));
+ do_nothing();
+
+ pr_info("attempting bad execution at %px\n",
+ dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
+ lkdtm_rodata_do_nothing();
+ pr_err("FAIL: func returned\n");
}
(In re-reading this more carefully, I see now why kallsyms.h is used
earlier: _function_ vs _symbol_ descriptor.)
In the next patch:
static noinline void execute_location(void *dst, bool write)
{
...
func = setup_function_descriptor(, dst);
if (IS_ERR(func))
return;
pr_info("attempting bad execution at %px\n", dst);
func();
pr_err("FAIL: func returned\n");
}
What are the conditions for which dereference_symbol_descriptor works
but dereference _function_descriptor doesn't?
When LKDTM is built as a module I guess ?
To be more precise, dereference_symbol_descriptor() calls either
dereference_kernel_function_descriptor() or
dereference_module_function_descriptor()
Both functions call dereference_function_descriptor() after checking
that we want to dereference something that is in the OPD section.
If we call dereference_function_descriptor() directly instead of
dereference_symbol_descriptor() we skip the range verification and may
dereference something that is not a function descriptor.
Should we do that ?
Indeed we are using it only for well known functions so using
dereference_function_descriptor() is good enough. I'll use that in v2.
Re: [PATCH v1 10/10] lkdtm: Fix execute_[user]_location()
Le 13/10/2021 à 09:16, Kees Cook a écrit :
On Mon, Oct 11, 2021 at 05:25:37PM +0200, Christophe Leroy wrote:
execute_location() and execute_user_location() intent
to copy do_nothing() text and execute it at a new location.
However, at the time being it doesn't copy do_nothing() function
but do_nothing() function descriptor which still points to the
original text. So at the end it still executes do_nothing() at
its original location allthough using a copied function descriptor.
So, fix that by really copying do_nothing() text and build a new
function descriptor by copying do_nothing() function descriptor and
updating the target address with the new location.
Also fix the displayed addresses by dereferencing do_nothing()
function descriptor.
Signed-off-by: Christophe Leroy
---
drivers/misc/lkdtm/perms.c | 45 +++---
1 file changed, 37 insertions(+), 8 deletions(-)
diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
index da16564e1ecd..1d03cd44c21d 100644
--- a/drivers/misc/lkdtm/perms.c
+++ b/drivers/misc/lkdtm/perms.c
@@ -44,19 +44,42 @@ static noinline void do_overwritten(void)
return;
}
+static void *setup_function_descriptor(funct_descr_t *fdesc, void *dst)
+{
+ int err;
+
+ if (!__is_defined(HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR))
+ return dst;
+
+ err = copy_from_kernel_nofault(fdesc, do_nothing, sizeof(*fdesc));
+ if (err < 0)
+ return ERR_PTR(err);
+
+ fdesc->addr = (unsigned long)dst;
+ barrier();
+
+ return fdesc;
+}
+
static noinline void execute_location(void *dst, bool write)
{
- void (*func)(void) = dst;
+ void (*func)(void);
+ funct_descr_t fdesc;
+ void *do_nothing_text = dereference_symbol_descriptor(do_nothing);
- pr_info("attempting ok execution at %px\n", do_nothing);
+ pr_info("attempting ok execution at %px\n", do_nothing_text);
do_nothing();
if (write == CODE_WRITE) {
- memcpy(dst, do_nothing, EXEC_SIZE);
+ memcpy(dst, do_nothing_text, EXEC_SIZE);
flush_icache_range((unsigned long)dst,
(unsigned long)dst + EXEC_SIZE);
}
- pr_info("attempting bad execution at %px\n", func);
+ func = setup_function_descriptor(, dst);
+ if (IS_ERR(func))
+ return;
I think this error case should complain with some details. :) Maybe:
pr_err("FAIL: could not build function descriptor for %px\n", dst);
Ok, I was going to add that in v2, but after one more thought I realise
that there's no need to use copy_from_kernel_nofault() here, we are
copying a static object into our stack, so a memcpy() will be good enough.
+
+ pr_info("attempting bad execution at %px\n", dst);
And then leave this pr_info() as it was, before the
setup_function_descriptor() call.
func();
pr_err("FAIL: func returned\n");
}
@@ -66,16 +89,22 @@ static void execute_user_location(void *dst)
int copied;
/* Intentionally crossing kernel/user memory boundary. */
- void (*func)(void) = dst;
+ void (*func)(void);
+ funct_descr_t fdesc;
+ void *do_nothing_text = dereference_symbol_descriptor(do_nothing);
- pr_info("attempting ok execution at %px\n", do_nothing);
+ pr_info("attempting ok execution at %px\n", do_nothing_text);
do_nothing();
- copied = access_process_vm(current, (unsigned long)dst, do_nothing,
+ copied = access_process_vm(current, (unsigned long)dst, do_nothing_text,
EXEC_SIZE, FOLL_WRITE);
if (copied < EXEC_SIZE)
return;
- pr_info("attempting bad execution at %px\n", func);
+ func = setup_function_descriptor(, dst);
+ if (IS_ERR(func))
+ return;
+
+ pr_info("attempting bad execution at %px\n", dst);
Same here.
func();
pr_err("FAIL: func returned\n");
}
--
2.31.1
Re: [PATCH v1 06/10] asm-generic: Refactor dereference_[kernel]_function_descriptor()
On Wed, Oct 13, 2021 at 1:20 PM Christophe Leroy wrote: > Le 13/10/2021 à 09:02, Kees Cook a écrit : > > On Mon, Oct 11, 2021 at 05:25:33PM +0200, Christophe Leroy wrote: > >> dereference_function_descriptor() and > >> dereference_kernel_function_descriptor() are identical on the > >> three architectures implementing them. > >> > >> Make it common. > >> > >> Signed-off-by: Christophe Leroy > >> --- > >> arch/ia64/include/asm/sections.h| 19 --- > >> arch/parisc/include/asm/sections.h | 9 - > >> arch/parisc/kernel/process.c| 21 - > >> arch/powerpc/include/asm/sections.h | 23 --- > >> include/asm-generic/sections.h | 18 ++ > >> 5 files changed, 18 insertions(+), 72 deletions(-) > > > > A diffstat to love. :) > > > > Reviewed-by: Kees Cook Reviewed-by: Arnd Bergmann > Unless somebody minds, I will make them out of line as > suggested by Helge in he's comment to patch 4. > > Allthough there is no spectacular size reduction, the functions > are not worth being inlined as they are not used in critical pathes. Sounds good to me. Arnd
Re: [PATCH v1 06/10] asm-generic: Refactor dereference_[kernel]_function_descriptor()
Le 13/10/2021 à 09:02, Kees Cook a écrit : On Mon, Oct 11, 2021 at 05:25:33PM +0200, Christophe Leroy wrote: dereference_function_descriptor() and dereference_kernel_function_descriptor() are identical on the three architectures implementing them. Make it common. Signed-off-by: Christophe Leroy --- arch/ia64/include/asm/sections.h| 19 --- arch/parisc/include/asm/sections.h | 9 - arch/parisc/kernel/process.c| 21 - arch/powerpc/include/asm/sections.h | 23 --- include/asm-generic/sections.h | 18 ++ 5 files changed, 18 insertions(+), 72 deletions(-) A diffstat to love. :) Reviewed-by: Kees Cook Unless somebody minds, I will make them out of line as suggested by Helge in he's comment to patch 4. Allthough there is no spectacular size reduction, the functions are not worth being inlined as they are not used in critical pathes.
Re: [PATCH v5 0/4] Add perf interface to expose nvdimm
Hi Dan, Any comments on this patch-set? Thanks, Kajol Jain On 9/28/21 6:11 PM, Kajol Jain wrote: > Patchset adds performance stats reporting support for nvdimm. > Added interface includes support for pmu register/unregister > functions. A structure is added called nvdimm_pmu to be used for > adding arch/platform specific data such as cpumask, nvdimm device > pointer and pmu event functions like event_init/add/read/del. > User could use the standard perf tool to access perf events > exposed via pmu. > > Interface also defines supported event list, config fields for the > event attributes and their corresponding bit values which are exported > via sysfs. Patch 3 exposes IBM pseries platform nmem* device > performance stats using this interface. > > Result from power9 pseries lpar with 2 nvdimm device: > > Ex: List all event by perf list > > command:# perf list nmem > > nmem0/cache_rh_cnt/[Kernel PMU event] > nmem0/cache_wh_cnt/[Kernel PMU event] > nmem0/cri_res_util/[Kernel PMU event] > nmem0/ctl_res_cnt/ [Kernel PMU event] > nmem0/ctl_res_tm/ [Kernel PMU event] > nmem0/fast_w_cnt/ [Kernel PMU event] > nmem0/host_l_cnt/ [Kernel PMU event] > nmem0/host_l_dur/ [Kernel PMU event] > nmem0/host_s_cnt/ [Kernel PMU event] > nmem0/host_s_dur/ [Kernel PMU event] > nmem0/med_r_cnt/ [Kernel PMU event] > nmem0/med_r_dur/ [Kernel PMU event] > nmem0/med_w_cnt/ [Kernel PMU event] > nmem0/med_w_dur/ [Kernel PMU event] > nmem0/mem_life/[Kernel PMU event] > nmem0/poweron_secs/[Kernel PMU event] > ... > nmem1/mem_life/[Kernel PMU event] > nmem1/poweron_secs/[Kernel PMU event] > > Patch1: > Introduces the nvdimm_pmu structure > Patch2: > Adds common interface to add arch/platform specific data > includes nvdimm device pointer, pmu data along with > pmu event functions. It also defines supported event list > and adds attribute groups for format, events and cpumask. > It also adds code for cpu hotplug support. > Patch3: > Add code in arch/powerpc/platform/pseries/papr_scm.c to expose > nmem* pmu. It fills in the nvdimm_pmu structure with pmu name, > capabilities, cpumask and event functions and then registers > the pmu by adding callbacks to register_nvdimm_pmu. > Patch4: > Sysfs documentation patch > > Changelog > --- > v4 -> v5: > - Remove multiple variables defined in nvdimm_pmu structure include > name and pmu functions(event_int/add/del/read) as they are just > used to copy them again in pmu variable. Now we are directly doing > this step in arch specific code as suggested by Dan Williams. > > - Remove attribute group field from nvdimm pmu structure and > defined these attribute groups in common interface which > includes format, event list along with cpumask as suggested by > Dan Williams. > Since we added static defination for attrbute groups needed in > common interface, removes corresponding code from papr. > > - Add nvdimm pmu event list with event codes in the common interface. > > - Remove Acked-by/Reviewed-by/Tested-by tags as code is refactored > to handle review comments from Dan. > > - Make nvdimm_pmu_free_hotplug_memory function static as reported > by kernel test robot, also add corresponding Reported-by tag. > > - Link to the patchset v4: https://lkml.org/lkml/2021/9/3/45 > > v3 -> v4 > - Rebase code on top of current papr_scm code without any logical > changes. > > - Added Acked-by tag from Peter Zijlstra and Reviewed by tag > from Madhavan Srinivasan. > > - Link to the patchset v3: https://lkml.org/lkml/2021/6/17/605 > > v2 -> v3 > - Added Tested-by tag. > > - Fix nvdimm mailing list in the ABI Documentation. > > - Link to the patchset v2: https://lkml.org/lkml/2021/6/14/25 > > v1 -> v2 > - Fix hotplug code by adding pmu migration call > incase current designated cpu got offline. As > pointed by Peter Zijlstra. > > - Removed the retun -1 part from cpu hotplug offline > function. > > - Link to the patchset v1: https://lkml.org/lkml/2021/6/8/500 > > Kajol Jain (4): > drivers/nvdimm: Add nvdimm pmu structure > drivers/nvdimm: Add perf interface to expose nvdimm performance stats > powerpc/papr_scm: Add perf interface support > docs: ABI: sysfs-bus-nvdimm: Document sysfs event format entries for > nvdimm pmu > >
Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver
On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas wrote:
> On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:
> I split some of the bigger patches apart so they only touched one
> driver or subsystem at a time. I also updated to_pci_driver() so it
> returns NULL when given NULL, which makes some of the validations
> quite a bit simpler, especially in the PM code in pci-driver.c.
It's a bit unusual. Other to_*_dev() are not NULL-aware IIRC.
Below are some comments as well.
...
> static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned
> short device)
> {
> + struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
> const struct pci_device_id *id;
>
> if (pdev->vendor == vendor && pdev->device == device)
> return true;
> + for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
> + if (id->vendor == vendor && id->device == device)
> + break;
return true;
> return id && id->vendor;
return false;
> }
...
> + afu_result = err_handler->error_detected(afu_dev,
> +state);
One line?
...
> device_lock(_dev->dev);
> - if (vf_dev->dev.driver) {
> + if (to_pci_driver(vf_dev->dev.driver)) {
Hmm...
...
> + if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0
> + && pci_dev->current_state != PCI_UNKNOWN) {
Can we keep && on the previous line?
> + pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev,
> + "PCI PM: Device state not saved by
> %pS\n",
> + drv->suspend);
> }
...
> + return drv && drv->resume ?
> + drv->resume(pci_dev) :
> pci_pm_reenable_device(pci_dev);
One line?
...
> + struct pci_driver *drv = to_pci_driver(dev->dev.driver);
> const struct pci_error_handlers *err_handler =
> - dev->dev.driver ?
> to_pci_driver(dev->dev.driver)->err_handler : NULL;
> + drv ? drv->err_handler : NULL;
Isn't dev->driver == to_pci_driver(dev->dev.driver)?
...
> + struct pci_driver *drv = to_pci_driver(dev->dev.driver);
> const struct pci_error_handlers *err_handler =
> - dev->dev.driver ?
> to_pci_driver(dev->dev.driver)->err_handler : NULL;
> + drv ? drv->err_handler : NULL;
Ditto.
...
> device_lock(>dev);
> + pdrv = to_pci_driver(dev->dev.driver);
> if (!pci_dev_set_io_state(dev, state) ||
> - !dev->dev.driver ||
> - !(pdrv = to_pci_driver(dev->dev.driver))->err_handler ||
> + !pdrv ||
> + !pdrv->err_handler ||
One line now?
> !pdrv->err_handler->error_detected) {
Or this and the previous line?
...
> + pdrv = to_pci_driver(dev->dev.driver);
> + if (!pdrv ||
> + !pdrv->err_handler ||
> !pdrv->err_handler->mmio_enabled)
> goto out;
Ditto.
...
> + pdrv = to_pci_driver(dev->dev.driver);
> + if (!pdrv ||
> + !pdrv->err_handler ||
> !pdrv->err_handler->slot_reset)
> goto out;
Ditto.
...
> if (!pci_dev_set_io_state(dev, pci_channel_io_normal) ||
> - !dev->dev.driver ||
> - !(pdrv = to_pci_driver(dev->dev.driver))->err_handler ||
> + !pdrv ||
> + !pdrv->err_handler ||
> !pdrv->err_handler->resume)
> goto out;
Ditto.
> - result = PCI_ERS_RESULT_NONE;
>
> pcidev = pci_get_domain_bus_and_slot(domain, bus, devfn);
> if (!pcidev || !pcidev->dev.driver) {
> dev_err(>xdev->dev, "device or AER driver is NULL\n");
> pci_dev_put(pcidev);
> - return result;
> + return PCI_ERS_RESULT_NONE;
> }
> pdrv = to_pci_driver(pcidev->dev.driver);
What about splitting the conditional to two with clear error message
in each and use pci_err() in the second one?
...
> default:
> dev_err(>xdev->dev,
> - "bad request in aer recovery "
> - "operation!\n");
> + "bad request in AER recovery operation!\n");
Stray change? Or is it in a separate patch in your tree?
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver
On Tue, Oct 12, 2021 at 06:32:12PM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:
> > Hello,
> >
> > this is v6 of the quest to drop the "driver" member from struct pci_dev
> > which tracks the same data (apart from a constant offset) as dev.driver.
>
> I like this a lot and applied it to pci/driver for v5.16, thanks!
>
> I split some of the bigger patches apart so they only touched one
> driver or subsystem at a time. I also updated to_pci_driver() so it
> returns NULL when given NULL, which makes some of the validations
> quite a bit simpler, especially in the PM code in pci-driver.c.
OK.
> Full interdiff from this v6 series:
>
> diff --git a/arch/x86/kernel/probe_roms.c b/arch/x86/kernel/probe_roms.c
> index deaaef6efe34..36e84d904260 100644
> --- a/arch/x86/kernel/probe_roms.c
> +++ b/arch/x86/kernel/probe_roms.c
> @@ -80,17 +80,15 @@ static struct resource video_rom_resource = {
> */
> static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned
> short device)
> {
> + struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
> const struct pci_device_id *id;
>
> if (pdev->vendor == vendor && pdev->device == device)
> return true;
>
> - if (pdev->dev.driver) {
> - struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
> - for (id = drv->id_table; id && id->vendor; id++)
> - if (id->vendor == vendor && id->device == device)
> - break;
> - }
> + for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
> + if (id->vendor == vendor && id->device == device)
> + break;
>
> return id && id->vendor;
> }
> diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
> index d997c9c3ebb5..7eb3706cf42d 100644
> --- a/drivers/misc/cxl/guest.c
> +++ b/drivers/misc/cxl/guest.c
> @@ -20,38 +20,38 @@ static void pci_error_handlers(struct cxl_afu *afu,
> pci_channel_state_t state)
> {
> struct pci_dev *afu_dev;
> + struct pci_driver *afu_drv;
> + struct pci_error_handlers *err_handler;
These two could be moved into the for loop (where afu_drv was with my
patch already). This is also possible in a few other drivers.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH 2/2] kbuild: use more subdir- for visiting subdirectories while cleaning
On Wed, Oct 13, 2021 at 8:43 AM Masahiro Yamada wrote:
> Documentation/kbuild/makefiles.rst suggests to use "archclean" for
> cleaning arch/$(SRCARCH)/boot/.
>
> Since commit d92cc4d51643 ("kbuild: require all architectures to have
> arch/$(SRCARCH)/Kbuild"), we can use the "subdir- += boot" trick for
> all architectures. This can take advantage of the parallel option (-j)
> for "make clean".
>
> I also cleaned up the comments. The "archdep" target does not exist.
>
> Signed-off-by: Masahiro Yamada
> arch/m68k/Makefile | 4 +---
Acked-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH 2/2] kbuild: use more subdir- for visiting subdirectories while cleaning
Documentation/kbuild/makefiles.rst suggests to use "archclean" for
cleaning arch/$(SRCARCH)/boot/.
Since commit d92cc4d51643 ("kbuild: require all architectures to have
arch/$(SRCARCH)/Kbuild"), we can use the "subdir- += boot" trick for
all architectures. This can take advantage of the parallel option (-j)
for "make clean".
I also cleaned up the comments. The "archdep" target does not exist.
Signed-off-by: Masahiro Yamada
---
Documentation/kbuild/makefiles.rst | 17 ++---
arch/alpha/Kbuild | 3 +++
arch/alpha/Makefile| 3 ---
arch/arc/Kbuild| 3 +++
arch/arc/Makefile | 3 ---
arch/arm/Kbuild| 3 +++
arch/arm/Makefile | 4
arch/arm64/Kbuild | 3 +++
arch/arm64/Makefile| 7 ---
arch/arm64/kernel/Makefile | 3 +++
arch/csky/Kbuild | 3 +++
arch/csky/Makefile | 3 ---
arch/h8300/Kbuild | 3 +++
arch/h8300/Makefile| 3 ---
arch/ia64/Makefile | 2 --
arch/m68k/Makefile | 4 +---
arch/microblaze/Kbuild | 3 +++
arch/microblaze/Makefile | 3 ---
arch/mips/Kbuild | 3 +++
arch/mips/Makefile | 8 +---
arch/mips/boot/Makefile| 3 +++
arch/nds32/Kbuild | 3 +++
arch/nds32/Makefile| 3 ---
arch/nios2/Kbuild | 3 +++
arch/nios2/Makefile| 6 +-
arch/openrisc/Kbuild | 3 +++
arch/openrisc/Makefile | 7 +--
arch/parisc/Kbuild | 3 +++
arch/parisc/Makefile | 7 +--
arch/powerpc/Kbuild| 3 +++
arch/powerpc/Makefile | 7 +--
arch/riscv/Kbuild | 3 +++
arch/riscv/Makefile| 7 +--
arch/s390/Kbuild | 3 +++
arch/s390/Makefile | 8 +---
arch/sh/Kbuild | 3 +++
arch/sh/Makefile | 3 ---
arch/sparc/Kbuild | 3 +++
arch/sparc/Makefile| 3 ---
arch/x86/Kbuild| 3 +++
arch/x86/Makefile | 2 --
arch/xtensa/Makefile | 4 +---
42 files changed, 71 insertions(+), 103 deletions(-)
diff --git a/Documentation/kbuild/makefiles.rst
b/Documentation/kbuild/makefiles.rst
index db3af0b45baf..b008b90b92c9 100644
--- a/Documentation/kbuild/makefiles.rst
+++ b/Documentation/kbuild/makefiles.rst
@@ -1050,22 +1050,9 @@ is not sufficient this sometimes needs to be explicit.
The above assignment instructs kbuild to descend down in the
directory compressed/ when "make clean" is executed.
-To support the clean infrastructure in the Makefiles that build the
-final bootimage there is an optional target named archclean:
-
- Example::
-
- #arch/x86/Makefile
- archclean:
- $(Q)$(MAKE) $(clean)=arch/x86/boot
-
-When "make clean" is executed, make will descend down in arch/x86/boot,
-and clean as usual. The Makefile located in arch/x86/boot/ may use
-the subdir- trick to descend further down.
-
Note 1: arch/$(SRCARCH)/Makefile cannot use "subdir-", because that file is
-included in the top level makefile, and the kbuild infrastructure
-is not operational at that point.
+included in the top level makefile. Instead, arch/$(SRCARCH)/Kbuild can use
+"subdir-".
Note 2: All directories listed in core-y, libs-y, drivers-y and net-y will
be visited during "make clean".
diff --git a/arch/alpha/Kbuild b/arch/alpha/Kbuild
index c2302017403a..345d79df24bb 100644
--- a/arch/alpha/Kbuild
+++ b/arch/alpha/Kbuild
@@ -1,3 +1,6 @@
# SPDX-License-Identifier: GPL-2.0-only
obj-y += kernel/ mm/
obj-$(CONFIG_MATHEMU) += math-emu/
+
+# for cleaning
+subdir- += boot
diff --git a/arch/alpha/Makefile b/arch/alpha/Makefile
index 52529ee42dac..881cb913e23a 100644
--- a/arch/alpha/Makefile
+++ b/arch/alpha/Makefile
@@ -55,9 +55,6 @@ $(boot)/vmlinux.gz: vmlinux
bootimage bootpfile bootpzfile: vmlinux
$(Q)$(MAKE) $(build)=$(boot) $(boot)/$@
-archclean:
- $(Q)$(MAKE) $(clean)=$(boot)
-
archheaders:
$(Q)$(MAKE) $(build)=arch/alpha/kernel/syscalls all
diff --git a/arch/arc/Kbuild b/arch/arc/Kbuild
index 699d8cae9b1f..b94102fff68b 100644
--- a/arch/arc/Kbuild
+++ b/arch/arc/Kbuild
@@ -1,3 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
obj-y += kernel/
obj-y += mm/
+
+# for cleaning
+subdir- += boot
diff --git a/arch/arc/Makefile b/arch/arc/Makefile
index 08995f6c6441..efc54f3e35e0 100644
--- a/arch/arc/Makefile
+++ b/arch/arc/Makefile
@@ -112,6 +112,3 @@ uImage: $(uimage-default-y)
@$(kecho) ' Image $(boot)/uImage is ready'
CLEAN_FILES += $(boot)/uImage
-
-archclean:
- $(Q)$(MAKE) $(clean)=$(boot)
diff --git a/arch/arm/Kbuild
Re: [PATCH 2/2] kbuild: use more subdir- for visiting subdirectories while cleaning
On Wed, Oct 13, 2021 at 03:36:22PM +0900, Masahiro Yamada wrote:
> Documentation/kbuild/makefiles.rst suggests to use "archclean" for
> cleaning arch/$(SRCARCH)/boot/.
>
> Since commit d92cc4d51643 ("kbuild: require all architectures to have
> arch/$(SRCARCH)/Kbuild"), we can use the "subdir- += boot" trick for
> all architectures. This can take advantage of the parallel option (-j)
> for "make clean".
>
> I also cleaned up the comments. The "archdep" target does not exist.
>
> Signed-off-by: Masahiro Yamada
I like the clean-up!
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH v2] KVM: PPC: Defer vtime accounting 'til after IRQ handling
On 13/10/2021 01:18, Michael Ellerman wrote:
Laurent Vivier writes:
Commit 112665286d08 moved guest_exit() in the interrupt protected
area to avoid wrong context warning (or worse), but the tick counter
cannot be updated and the guest time is accounted to the system time.
To fix the problem port to POWER the x86 fix
160457140187 ("Defer vtime accounting 'til after IRQ handling"):
"Defer the call to account guest time until after servicing any IRQ(s)
that happened in the guest or immediately after VM-Exit. Tick-based
accounting of vCPU time relies on PF_VCPU being set when the tick IRQ
handler runs, and IRQs are blocked throughout the main sequence of
vcpu_enter_guest(), including the call into vendor code to actually
enter and exit the guest."
Fixes: 112665286d08 ("KVM: PPC: Book3S HV: Context tracking exit guest context
before enabling irqs")
Cc: npig...@gmail.com
Cc: # 5.12
Signed-off-by: Laurent Vivier
---
Notes:
v2: remove reference to commit 61bd0f66ff92
cc stable 5.12
add the same comment in the code as for x86
arch/powerpc/kvm/book3s_hv.c | 24
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 2acb1c96cfaf..a694d1a8f6ce 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
...
@@ -4506,13 +4514,21 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64
time_limit,
srcu_read_unlock(>srcu, srcu_idx);
+ context_tracking_guest_exit();
+
set_irq_happened(trap);
kvmppc_set_host_core(pcpu);
- guest_exit_irqoff();
-
local_irq_enable();
+ /*
+* Wait until after servicing IRQs to account guest time so that any
+* ticks that occurred while running the guest are properly accounted
+* to the guest. Waiting until IRQs are enabled degrades the accuracy
+* of accounting via context tracking, but the loss of accuracy is
+* acceptable for all known use cases.
+*/
+ vtime_account_guest_exit();
This pops a warning for me, running guest(s) on Power8:
[ 270.745303][T16661] [ cut here ]
[ 270.745374][T16661] WARNING: CPU: 72 PID: 16661 at
arch/powerpc/kernel/time.c:311 vtime_account_kernel+0xe0/0xf0
Thank you, I missed that...
My patch is wrong, I have to add vtime_account_guest_exit() before the
local_irq_enable().
arch/powerpc/kernel/time.c
305 static unsigned long vtime_delta(struct cpu_accounting_data *acct,
306 unsigned long *stime_scaled,
307 unsigned long *steal_time)
308 {
309 unsigned long now, stime;
310
311 WARN_ON_ONCE(!irqs_disabled());
...
But I don't understand how ticks can be accounted now if irqs are still
disabled.
Not sure it is as simple as expected...
Thanks,
Laurent
[PATCH v3 2/2] ftrace: do CPU checking after preemption disabled
With CONFIG_DEBUG_PREEMPT we observed reports like: BUG: using smp_processor_id() in preemptible caller is perf_ftrace_function_call+0x6f/0x2e0 CPU: 1 PID: 680 Comm: a.out Not tainted Call Trace: dump_stack_lvl+0x8d/0xcf check_preemption_disabled+0x104/0x110 ? optimize_nops.isra.7+0x230/0x230 ? text_poke_bp_batch+0x9f/0x310 perf_ftrace_function_call+0x6f/0x2e0 ... __text_poke+0x5/0x620 text_poke_bp_batch+0x9f/0x310 This telling us the CPU could be changed after task is preempted, and the checking on CPU before preemption will be invalid. Since now ftrace_test_recursion_trylock() will help to disable the preemption, this patch just do the checking after trylock() to address the issue. CC: Steven Rostedt Reported-by: Abaci Signed-off-by: Michael Wang --- kernel/trace/trace_event_perf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c index 6aed10e..fba8cb7 100644 --- a/kernel/trace/trace_event_perf.c +++ b/kernel/trace/trace_event_perf.c @@ -441,13 +441,13 @@ void perf_trace_buf_update(void *record, u16 type) if (!rcu_is_watching()) return; - if ((unsigned long)ops->private != smp_processor_id()) - return; - bit = ftrace_test_recursion_trylock(ip, parent_ip); if (bit < 0) return; + if ((unsigned long)ops->private != smp_processor_id()) + goto out; + event = container_of(ops, struct perf_event, ftrace_ops); /* -- 1.8.3.1
[PATCH v3 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()
As the documentation explained, ftrace_test_recursion_trylock()
and ftrace_test_recursion_unlock() were supposed to disable and
enable preemption properly, however currently this work is done
outside of the function, which could be missing by mistake.
This path will make sure the preemption was disabled when trylock()
succeed, and the unlock() will enable the preemption if previously
enabled.
CC: Steven Rostedt
CC: Miroslav Benes
Reported-by: Abaci
Suggested-by: Peter Zijlstra
Signed-off-by: Michael Wang
---
arch/csky/kernel/probes/ftrace.c | 2 --
arch/parisc/kernel/ftrace.c | 2 --
arch/powerpc/kernel/kprobes-ftrace.c | 2 --
arch/riscv/kernel/probes/ftrace.c| 2 --
arch/x86/kernel/kprobes/ftrace.c | 2 --
include/linux/trace_recursion.h | 17 -
kernel/livepatch/patch.c | 13 +++--
kernel/trace/trace_functions.c | 5 -
8 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/arch/csky/kernel/probes/ftrace.c b/arch/csky/kernel/probes/ftrace.c
index b388228..834cffc 100644
--- a/arch/csky/kernel/probes/ftrace.c
+++ b/arch/csky/kernel/probes/ftrace.c
@@ -17,7 +17,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long
parent_ip,
return;
regs = ftrace_get_regs(fregs);
- preempt_disable_notrace();
p = get_kprobe((kprobe_opcode_t *)ip);
if (!p) {
p = get_kprobe((kprobe_opcode_t *)(ip - MCOUNT_INSN_SIZE));
@@ -57,7 +56,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long
parent_ip,
__this_cpu_write(current_kprobe, NULL);
}
out:
- preempt_enable_notrace();
ftrace_test_recursion_unlock(bit);
}
NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c
index 0a1e75a..3543496 100644
--- a/arch/parisc/kernel/ftrace.c
+++ b/arch/parisc/kernel/ftrace.c
@@ -216,7 +216,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long
parent_ip,
return;
regs = ftrace_get_regs(fregs);
- preempt_disable_notrace();
p = get_kprobe((kprobe_opcode_t *)ip);
if (unlikely(!p) || kprobe_disabled(p))
goto out;
@@ -245,7 +244,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long
parent_ip,
}
__this_cpu_write(current_kprobe, NULL);
out:
- preempt_enable_notrace();
ftrace_test_recursion_unlock(bit);
}
NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/arch/powerpc/kernel/kprobes-ftrace.c
b/arch/powerpc/kernel/kprobes-ftrace.c
index 7154d58..072ebe7 100644
--- a/arch/powerpc/kernel/kprobes-ftrace.c
+++ b/arch/powerpc/kernel/kprobes-ftrace.c
@@ -26,7 +26,6 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long
parent_nip,
return;
regs = ftrace_get_regs(fregs);
- preempt_disable_notrace();
p = get_kprobe((kprobe_opcode_t *)nip);
if (unlikely(!p) || kprobe_disabled(p))
goto out;
@@ -61,7 +60,6 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long
parent_nip,
__this_cpu_write(current_kprobe, NULL);
}
out:
- preempt_enable_notrace();
ftrace_test_recursion_unlock(bit);
}
NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/arch/riscv/kernel/probes/ftrace.c
b/arch/riscv/kernel/probes/ftrace.c
index aab85a8..7142ec4 100644
--- a/arch/riscv/kernel/probes/ftrace.c
+++ b/arch/riscv/kernel/probes/ftrace.c
@@ -15,7 +15,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long
parent_ip,
if (bit < 0)
return;
- preempt_disable_notrace();
p = get_kprobe((kprobe_opcode_t *)ip);
if (unlikely(!p) || kprobe_disabled(p))
goto out;
@@ -52,7 +51,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long
parent_ip,
__this_cpu_write(current_kprobe, NULL);
}
out:
- preempt_enable_notrace();
ftrace_test_recursion_unlock(bit);
}
NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index 596de2f..dd2ec14 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -25,7 +25,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long
parent_ip,
if (bit < 0)
return;
- preempt_disable_notrace();
p = get_kprobe((kprobe_opcode_t *)ip);
if (unlikely(!p) || kprobe_disabled(p))
goto out;
@@ -59,7 +58,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long
parent_ip,
__this_cpu_write(current_kprobe, NULL);
}
out:
- preempt_enable_notrace();
ftrace_test_recursion_unlock(bit);
}
NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/include/linux/trace_recursion.h b/include/linux/trace_recursion.h
index a9f9c57..58e474c 100644
---
[PATCH v3 0/2] fix & prevent the missing preemption disabling
The testing show that perf_ftrace_function_call() are using smp_processor_id() with preemption enabled, all the checking on CPU could be wrong after preemption. As Peter point out, the section between ftrace_test_recursion_trylock/unlock() pair require the preemption to be disabled as 'Documentation/trace/ftrace-uses.rst' explained, but currently the work is done outside of the helpers. Patch 1/2 will make sure preemption disabled after trylock() succeed, patch 2/2 will do smp_processor_id() checking after trylock to address the issue. v1: https://lore.kernel.org/all/8c7de46d-9869-aa5e-2bb9-5dbc2eda3...@linux.alibaba.com/ v2: https://lore.kernel.org/all/b1d7fe43-ce84-0ed7-32f7-ea1d12d0b...@linux.alibaba.com/ Michael Wang (2): ftrace: disable preemption between ftrace_test_recursion_trylock/unlock() ftrace: do CPU checking after preemption disabled arch/csky/kernel/probes/ftrace.c | 2 -- arch/parisc/kernel/ftrace.c | 2 -- arch/powerpc/kernel/kprobes-ftrace.c | 2 -- arch/riscv/kernel/probes/ftrace.c| 2 -- arch/x86/kernel/kprobes/ftrace.c | 2 -- include/linux/trace_recursion.h | 17 - kernel/livepatch/patch.c | 13 +++-- kernel/trace/trace_event_perf.c | 6 +++--- kernel/trace/trace_functions.c | 5 - 9 files changed, 26 insertions(+), 25 deletions(-) -- 1.8.3.1
Re: [RESEND PATCH v2 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()
On 2021/10/13 下午4:25, Miroslav Benes wrote: >>> Side note... the comment will eventually conflict with peterz's >>> https://lore.kernel.org/all/20210929152429.125997...@infradead.org/. >> >> Steven, would you like to share your opinion on this patch? >> >> If klp_synchronize_transition() will be removed anyway, the comments >> will be meaningless and we can just drop it :-P > > The comment will still be needed in some form. We will handle it depending > on what gets merged first. peterz's patches are not ready yet. Ok, then I'll move it before trylock() inside klp_ftrace_handler() anyway. Regards, Michael Wang > > Miroslav >
Re: [RESEND PATCH v2 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()
> > Side note... the comment will eventually conflict with peterz's > > https://lore.kernel.org/all/20210929152429.125997...@infradead.org/. > > Steven, would you like to share your opinion on this patch? > > If klp_synchronize_transition() will be removed anyway, the comments > will be meaningless and we can just drop it :-P The comment will still be needed in some form. We will handle it depending on what gets merged first. peterz's patches are not ready yet. Miroslav
Re: [PATCH] powerpc: don't select KFENCE on platform PPC_FSL_BOOK3E
Le 13/10/2021 à 02:48, Michael Ellerman a écrit : Christophe Leroy writes: Le 12/10/2021 à 08:24, Michael Ellerman a écrit : Liu Shixin writes: kindly ping. I was under the impression you were trying to debug why it wasn't working with Christophe. The investigation was a bit dormant to be honest since Liu confirmed that neither KFENCE not DEBUG_PAGEALLOC works. No worries. Sorry it fell to you to do the investigation. No problem. I now looked at the effort to make it work, and it is not trivial. At the time being, all linear space is mapped with pinned TLBs and everything is setup for space 0, with space 1 being used temporarily when doing heavy changes to space 0. We can't use standard pages for linear space on space 0 because we need memory mapped at all time for exceptions (on booke exception run with MMU on in space 0). In order to use standard pages, we'd need to reorganise the kernel to have it run mostly in space 1 (for data at least) where we would map almost everything with standard pages, and keep pinned TLB to map linear space on space 0 for TLB miss exceptions. Then we'd do more or less like book3s/32 and switch back into space 1 into other exceptions prolog. That could be good to do it as we could maybe have more code in common with non booke 32 bits, but it is not a trivial job. So I suggest that for now, we just make KFENCE and DEBUG_PAGEALLOC unselectable for booke/32 (e500 and 44x). Yep seems reasonable. We also have a problem with STRICT_KERNEL_RWX as it is based on the same principles until someone implements it by blocks like book3s/32 and 8xx. So it should also be unselectable on e500 and 44x for now. Christophe
Re: [RESEND PATCH v2 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()
On 2021/10/13 下午3:55, Miroslav Benes wrote: >> diff --git a/include/linux/trace_recursion.h >> b/include/linux/trace_recursion.h >> index a9f9c57..101e1fb 100644 >> --- a/include/linux/trace_recursion.h >> +++ b/include/linux/trace_recursion.h >> @@ -208,13 +208,29 @@ static __always_inline void trace_clear_recursion(int >> bit) >> * Use this for ftrace callbacks. This will detect if the function >> * tracing recursed in the same context (normal vs interrupt), >> * >> + * The ftrace_test_recursion_trylock() will disable preemption, >> + * which is required for the variant of synchronize_rcu() that is >> + * used to allow patching functions where RCU is not watching. >> + * See klp_synchronize_transition() for more details. >> + * > > I think that you misunderstood. Steven proposed to put the comment before > ftrace_test_recursion_trylock() call site in klp_ftrace_handler(). Oh, I see... thanks for pointing out :-) > >> * Returns: -1 if a recursion happened. [snip] >> } > > Side note... the comment will eventually conflict with peterz's > https://lore.kernel.org/all/20210929152429.125997...@infradead.org/. Steven, would you like to share your opinion on this patch? If klp_synchronize_transition() will be removed anyway, the comments will be meaningless and we can just drop it :-P Regards, Michael Wang > > Miroslav >
Re: [PATCH v2 00/11] Add Apple M1 support to PASemi i2c driver
On 09 October 2021 at 03:57 pm, Christian Zigotzky wrote: > On 09 October 2021 at 12:10 pm, Wolfram Sang wrote: >>> I still don't have access to any old PASemi hardware but the changes from >>> v1 are pretty small and I expect them to still work. Would still be nice >>> if someone with access to such hardware could give this a quick test. >> Looks good to me. I will wait a few more days so that people can report >> their tests. But it will be in the next merge window. >> > Series v2: > > Tested-by: Christian Zigotzky [1] > > - Christian > > [1] https://forum.hyperion-entertainment.com/viewtopic.php?p=54213#p54213 Series v2: Tested-by: Damien Stewart (Hypex) [1] - Christian [1] https://forum.hyperion-entertainment.com/viewtopic.php?p=54217#p54217
Re: [RESEND PATCH v2 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()
> diff --git a/include/linux/trace_recursion.h b/include/linux/trace_recursion.h
> index a9f9c57..101e1fb 100644
> --- a/include/linux/trace_recursion.h
> +++ b/include/linux/trace_recursion.h
> @@ -208,13 +208,29 @@ static __always_inline void trace_clear_recursion(int
> bit)
> * Use this for ftrace callbacks. This will detect if the function
> * tracing recursed in the same context (normal vs interrupt),
> *
> + * The ftrace_test_recursion_trylock() will disable preemption,
> + * which is required for the variant of synchronize_rcu() that is
> + * used to allow patching functions where RCU is not watching.
> + * See klp_synchronize_transition() for more details.
> + *
I think that you misunderstood. Steven proposed to put the comment before
ftrace_test_recursion_trylock() call site in klp_ftrace_handler().
> * Returns: -1 if a recursion happened.
> * >= 0 if no recursion
> */
> static __always_inline int ftrace_test_recursion_trylock(unsigned long ip,
>unsigned long
> parent_ip)
> {
> - return trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START,
> TRACE_FTRACE_MAX);
> + int bit;
> +
> + bit = trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START,
> TRACE_FTRACE_MAX);
> + /*
> + * The zero bit indicate we are nested
> + * in another trylock(), which means the
> + * preemption already disabled.
> + */
> + if (bit > 0)
> + preempt_disable_notrace();
> +
> + return bit;
> }
[...]
> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> index e8029ae..6e66ccd 100644
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -52,11 +52,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
Here
> bit = ftrace_test_recursion_trylock(ip, parent_ip);
> if (WARN_ON_ONCE(bit < 0))
> return;
> - /*
> - * A variant of synchronize_rcu() is used to allow patching functions
> - * where RCU is not watching, see klp_synchronize_transition().
> - */
> - preempt_disable_notrace();
>
> func = list_first_or_null_rcu(>func_stack, struct klp_func,
> stack_node);
> @@ -120,7 +115,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
> klp_arch_set_pc(fregs, (unsigned long)func->new_func);
>
> unlock:
> - preempt_enable_notrace();
> ftrace_test_recursion_unlock(bit);
> }
Side note... the comment will eventually conflict with peterz's
https://lore.kernel.org/all/20210929152429.125997...@infradead.org/.
Miroslav
Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()
Le 13/10/2021 à 09:39, Christophe Leroy a écrit :
Le 13/10/2021 à 09:23, Kees Cook a écrit :
On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
Behind a location, lkdtm_EXEC_RODATA() executes a real function,
not a copy of do_nothing().
So do it directly instead of using execute_location().
And fix displayed addresses by dereferencing the function descriptors.
Signed-off-by: Christophe Leroy
---
drivers/misc/lkdtm/perms.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
index 442d60ed25ef..da16564e1ecd 100644
--- a/drivers/misc/lkdtm/perms.c
+++ b/drivers/misc/lkdtm/perms.c
@@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
void lkdtm_EXEC_RODATA(void)
{
- execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
+ pr_info("attempting ok execution at %px\n",
+ dereference_symbol_descriptor(do_nothing));
+ do_nothing();
+
+ pr_info("attempting bad execution at %px\n",
+ dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
+ lkdtm_rodata_do_nothing();
+ pr_err("FAIL: func returned\n");
}
(In re-reading this more carefully, I see now why kallsyms.h is used
earlier: _function_ vs _symbol_ descriptor.)
In the next patch:
static noinline void execute_location(void *dst, bool write)
{
...
func = setup_function_descriptor(, dst);
if (IS_ERR(func))
return;
pr_info("attempting bad execution at %px\n", dst);
func();
pr_err("FAIL: func returned\n");
}
What are the conditions for which dereference_symbol_descriptor works
but dereference _function_descriptor doesn't?
When LKDTM is built as a module I guess ?
To be more precise, dereference_symbol_descriptor() calls either
dereference_kernel_function_descriptor() or
dereference_module_function_descriptor()
Both functions call dereference_function_descriptor() after checking
that we want to dereference something that is in the OPD section.
If we call dereference_function_descriptor() directly instead of
dereference_symbol_descriptor() we skip the range verification and may
dereference something that is not a function descriptor.
Should we do that ?
Christophe
Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()
Le 13/10/2021 à 09:23, Kees Cook a écrit :
On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
Behind a location, lkdtm_EXEC_RODATA() executes a real function,
not a copy of do_nothing().
So do it directly instead of using execute_location().
And fix displayed addresses by dereferencing the function descriptors.
Signed-off-by: Christophe Leroy
---
drivers/misc/lkdtm/perms.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
index 442d60ed25ef..da16564e1ecd 100644
--- a/drivers/misc/lkdtm/perms.c
+++ b/drivers/misc/lkdtm/perms.c
@@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
void lkdtm_EXEC_RODATA(void)
{
- execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
+ pr_info("attempting ok execution at %px\n",
+ dereference_symbol_descriptor(do_nothing));
+ do_nothing();
+
+ pr_info("attempting bad execution at %px\n",
+ dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
+ lkdtm_rodata_do_nothing();
+ pr_err("FAIL: func returned\n");
}
(In re-reading this more carefully, I see now why kallsyms.h is used
earlier: _function_ vs _symbol_ descriptor.)
In the next patch:
static noinline void execute_location(void *dst, bool write)
{
...
func = setup_function_descriptor(, dst);
if (IS_ERR(func))
return;
pr_info("attempting bad execution at %px\n", dst);
func();
pr_err("FAIL: func returned\n");
}
What are the conditions for which dereference_symbol_descriptor works
but dereference _function_descriptor doesn't?
When LKDTM is built as a module I guess ?
Christophe
Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()
Le 13/10/2021 à 09:09, Kees Cook a écrit :
On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
Behind a location, lkdtm_EXEC_RODATA() executes a real function,
not a copy of do_nothing().
So do it directly instead of using execute_location().
I don't understand this. Why does the next patch not fix this?
Well, probably it would, but it looked incorrect in my mind.
lkdtm_rodata_do_nothing() is a function which has its own function
descriptor, it is not a copy of do_nothing().
If we use execute_location() as modified by next patch, then we will
execute it using the function descriptor of do_nothing(). Allthough it
most likely works (at least on powerpc as it uses the same TOC) it looks
odd to me to do so.
Am I missing something ?
Christophe
-Kees
And fix displayed addresses by dereferencing the function descriptors.
Signed-off-by: Christophe Leroy
---
drivers/misc/lkdtm/perms.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
index 442d60ed25ef..da16564e1ecd 100644
--- a/drivers/misc/lkdtm/perms.c
+++ b/drivers/misc/lkdtm/perms.c
@@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
void lkdtm_EXEC_RODATA(void)
{
- execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
+ pr_info("attempting ok execution at %px\n",
+ dereference_symbol_descriptor(do_nothing));
+ do_nothing();
+
+ pr_info("attempting bad execution at %px\n",
+ dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
+ lkdtm_rodata_do_nothing();
+ pr_err("FAIL: func returned\n");
}
void lkdtm_EXEC_USERSPACE(void)
--
2.31.1
Re: [PATCH v1 08/10] lkdtm: Really write into kernel text in WRITE_KERN
Le 13/10/2021 à 09:05, Kees Cook a écrit :
On Mon, Oct 11, 2021 at 05:25:35PM +0200, Christophe Leroy wrote:
WRITE_KERN is supposed to overwrite some kernel text, namely
do_overwritten() function.
But at the time being it overwrites do_overwritten() function
descriptor, not function text.
Fix it by dereferencing the function descriptor to obtain
function text pointer.
And make do_overwritten() noinline so that it is really
do_overwritten() which is called by lkdtm_WRITE_KERN().
Signed-off-by: Christophe Leroy
---
drivers/misc/lkdtm/perms.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
index 60b3b2fe929d..442d60ed25ef 100644
--- a/drivers/misc/lkdtm/perms.c
+++ b/drivers/misc/lkdtm/perms.c
@@ -5,6 +5,7 @@
* even non-readable regions.
*/
#include "lkdtm.h"
+#include
Why not #include instead here?
dereference_symbol_descriptor() is defined in linux/kallsyms.h
#include
#include
#include
@@ -37,7 +38,7 @@ static noinline void do_nothing(void)
}
/* Must immediately follow do_nothing for size calculuations to work out. */
-static void do_overwritten(void)
+static noinline void do_overwritten(void)
{
pr_info("do_overwritten wasn't overwritten!\n");
return;
@@ -113,8 +114,9 @@ void lkdtm_WRITE_KERN(void)
size_t size;
volatile unsigned char *ptr;
- size = (unsigned long)do_overwritten - (unsigned long)do_nothing;
- ptr = (unsigned char *)do_overwritten;
+ size = (unsigned long)dereference_symbol_descriptor(do_overwritten) -
+ (unsigned long)dereference_symbol_descriptor(do_nothing);
+ ptr = dereference_symbol_descriptor(do_overwritten);
But otherwise, yup, I expect there will be a bunch of things like this
to clean up in LKDTM. :| Sorry about that!
Acked-by: Kees Cook
pr_info("attempting bad %zu byte write at %px\n", size, ptr);
memcpy((void *)ptr, (unsigned char *)do_nothing, size);
--
2.31.1
Re: [PATCH v1 05/10] asm-generic: Define 'funct_descr_t' to commonly describe function descriptors
On Wed, Oct 13, 2021 at 09:23:56AM +0200, Christophe Leroy wrote: > > > Le 13/10/2021 à 09:01, Kees Cook a écrit : > > On Mon, Oct 11, 2021 at 05:25:32PM +0200, Christophe Leroy wrote: > > > We have three architectures using function descriptors, each with its > > > own name. > > > > > > Add a common typedef that can be used in generic code. > > > > > > Also add a stub typedef for architecture without function descriptors, > > > > nit: funct_descr_t reads weird to me. why not func_desc_t ? Either way: > > func_desc_t already exists in powerpc. I have a patch to remove it as it is > redundant with struct ppc64_opd_entry, but I didnt' want to include it in > this series. > > But after all I can add it in this series, I'll add it in v2. Ah-ha! That works for me. :) Thanks! -Kees -- Kees Cook
Re: [PATCH v1 05/10] asm-generic: Define 'funct_descr_t' to commonly describe function descriptors
Le 13/10/2021 à 09:01, Kees Cook a écrit :
On Mon, Oct 11, 2021 at 05:25:32PM +0200, Christophe Leroy wrote:
We have three architectures using function descriptors, each with its
own name.
Add a common typedef that can be used in generic code.
Also add a stub typedef for architecture without function descriptors,
nit: funct_descr_t reads weird to me. why not func_desc_t ? Either way:
func_desc_t already exists in powerpc. I have a patch to remove it as it
is redundant with struct ppc64_opd_entry, but I didnt' want to include
it in this series.
But after all I can add it in this series, I'll add it in v2.
Christophe
Reviewed-by: Kees Cook
to avoid a forest of #ifdefs.
Signed-off-by: Christophe Leroy
---
arch/ia64/include/asm/sections.h| 1 +
arch/parisc/include/asm/sections.h | 1 +
arch/powerpc/include/asm/sections.h | 1 +
include/asm-generic/sections.h | 3 +++
4 files changed, 6 insertions(+)
diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
index 80f5868afb06..929b5c535620 100644
--- a/arch/ia64/include/asm/sections.h
+++ b/arch/ia64/include/asm/sections.h
@@ -8,6 +8,7 @@
*/
#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
+typedef struct fdesc funct_descr_t;
#include
#include
diff --git a/arch/parisc/include/asm/sections.h
b/arch/parisc/include/asm/sections.h
index 2e781ee19b66..329e80f7af0a 100644
--- a/arch/parisc/include/asm/sections.h
+++ b/arch/parisc/include/asm/sections.h
@@ -4,6 +4,7 @@
#ifdef CONFIG_64BIT
#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
+typedef Elf64_Fdesc funct_descr_t;
#endif
/* nothing to see, move along */
diff --git a/arch/powerpc/include/asm/sections.h
b/arch/powerpc/include/asm/sections.h
index b7f1ba04e756..d0d5287fa568 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h
@@ -10,6 +10,7 @@
#ifdef PPC64_ELF_ABI_v1
#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
+typedef struct ppc64_opd_entry funct_descr_t;
#endif
#include
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index 1db5cfd69817..436412d94054 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -63,6 +63,9 @@ extern __visible const void __nosave_begin, __nosave_end;
#else
#define dereference_function_descriptor(p) ((void *)(p))
#define dereference_kernel_function_descriptor(p) ((void *)(p))
+typedef struct {
+ unsigned long addr;
+} funct_descr_t;
#endif
/* random extra sections (if any). Override
--
2.31.1
Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()
On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
> Behind a location, lkdtm_EXEC_RODATA() executes a real function,
> not a copy of do_nothing().
>
> So do it directly instead of using execute_location().
>
> And fix displayed addresses by dereferencing the function descriptors.
>
> Signed-off-by: Christophe Leroy
> ---
> drivers/misc/lkdtm/perms.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 442d60ed25ef..da16564e1ecd 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
>
> void lkdtm_EXEC_RODATA(void)
> {
> - execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
> + pr_info("attempting ok execution at %px\n",
> + dereference_symbol_descriptor(do_nothing));
> + do_nothing();
> +
> + pr_info("attempting bad execution at %px\n",
> + dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
> + lkdtm_rodata_do_nothing();
> + pr_err("FAIL: func returned\n");
> }
(In re-reading this more carefully, I see now why kallsyms.h is used
earlier: _function_ vs _symbol_ descriptor.)
In the next patch:
static noinline void execute_location(void *dst, bool write)
{
...
func = setup_function_descriptor(, dst);
if (IS_ERR(func))
return;
pr_info("attempting bad execution at %px\n", dst);
func();
pr_err("FAIL: func returned\n");
}
What are the conditions for which dereference_symbol_descriptor works
but dereference _function_descriptor doesn't?
--
Kees Cook
Re: [PATCH v1 10/10] lkdtm: Fix execute_[user]_location()
On Mon, Oct 11, 2021 at 05:25:37PM +0200, Christophe Leroy wrote:
> execute_location() and execute_user_location() intent
> to copy do_nothing() text and execute it at a new location.
> However, at the time being it doesn't copy do_nothing() function
> but do_nothing() function descriptor which still points to the
> original text. So at the end it still executes do_nothing() at
> its original location allthough using a copied function descriptor.
>
> So, fix that by really copying do_nothing() text and build a new
> function descriptor by copying do_nothing() function descriptor and
> updating the target address with the new location.
>
> Also fix the displayed addresses by dereferencing do_nothing()
> function descriptor.
>
> Signed-off-by: Christophe Leroy
> ---
> drivers/misc/lkdtm/perms.c | 45 +++---
> 1 file changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index da16564e1ecd..1d03cd44c21d 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -44,19 +44,42 @@ static noinline void do_overwritten(void)
> return;
> }
>
> +static void *setup_function_descriptor(funct_descr_t *fdesc, void *dst)
> +{
> + int err;
> +
> + if (!__is_defined(HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR))
> + return dst;
> +
> + err = copy_from_kernel_nofault(fdesc, do_nothing, sizeof(*fdesc));
> + if (err < 0)
> + return ERR_PTR(err);
> +
> + fdesc->addr = (unsigned long)dst;
> + barrier();
> +
> + return fdesc;
> +}
> +
> static noinline void execute_location(void *dst, bool write)
> {
> - void (*func)(void) = dst;
> + void (*func)(void);
> + funct_descr_t fdesc;
> + void *do_nothing_text = dereference_symbol_descriptor(do_nothing);
>
> - pr_info("attempting ok execution at %px\n", do_nothing);
> + pr_info("attempting ok execution at %px\n", do_nothing_text);
> do_nothing();
>
> if (write == CODE_WRITE) {
> - memcpy(dst, do_nothing, EXEC_SIZE);
> + memcpy(dst, do_nothing_text, EXEC_SIZE);
> flush_icache_range((unsigned long)dst,
> (unsigned long)dst + EXEC_SIZE);
> }
> - pr_info("attempting bad execution at %px\n", func);
> + func = setup_function_descriptor(, dst);
> + if (IS_ERR(func))
> + return;
I think this error case should complain with some details. :) Maybe:
pr_err("FAIL: could not build function descriptor for %px\n", dst);
> +
> + pr_info("attempting bad execution at %px\n", dst);
And then leave this pr_info() as it was, before the
setup_function_descriptor() call.
> func();
> pr_err("FAIL: func returned\n");
> }
> @@ -66,16 +89,22 @@ static void execute_user_location(void *dst)
> int copied;
>
> /* Intentionally crossing kernel/user memory boundary. */
> - void (*func)(void) = dst;
> + void (*func)(void);
> + funct_descr_t fdesc;
> + void *do_nothing_text = dereference_symbol_descriptor(do_nothing);
>
> - pr_info("attempting ok execution at %px\n", do_nothing);
> + pr_info("attempting ok execution at %px\n", do_nothing_text);
> do_nothing();
>
> - copied = access_process_vm(current, (unsigned long)dst, do_nothing,
> + copied = access_process_vm(current, (unsigned long)dst, do_nothing_text,
> EXEC_SIZE, FOLL_WRITE);
> if (copied < EXEC_SIZE)
> return;
> - pr_info("attempting bad execution at %px\n", func);
> + func = setup_function_descriptor(, dst);
> + if (IS_ERR(func))
> + return;
> +
> + pr_info("attempting bad execution at %px\n", dst);
Same here.
> func();
> pr_err("FAIL: func returned\n");
> }
> --
> 2.31.1
>
--
Kees Cook
Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()
On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
> Behind a location, lkdtm_EXEC_RODATA() executes a real function,
> not a copy of do_nothing().
>
> So do it directly instead of using execute_location().
I don't understand this. Why does the next patch not fix this?
-Kees
>
> And fix displayed addresses by dereferencing the function descriptors.
>
> Signed-off-by: Christophe Leroy
> ---
> drivers/misc/lkdtm/perms.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 442d60ed25ef..da16564e1ecd 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
>
> void lkdtm_EXEC_RODATA(void)
> {
> - execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
> + pr_info("attempting ok execution at %px\n",
> + dereference_symbol_descriptor(do_nothing));
> + do_nothing();
> +
> + pr_info("attempting bad execution at %px\n",
> + dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
> + lkdtm_rodata_do_nothing();
> + pr_err("FAIL: func returned\n");
> }
>
> void lkdtm_EXEC_USERSPACE(void)
> --
> 2.31.1
>
--
Kees Cook
Re: [PATCH v1 08/10] lkdtm: Really write into kernel text in WRITE_KERN
On Mon, Oct 11, 2021 at 05:25:35PM +0200, Christophe Leroy wrote:
> WRITE_KERN is supposed to overwrite some kernel text, namely
> do_overwritten() function.
>
> But at the time being it overwrites do_overwritten() function
> descriptor, not function text.
>
> Fix it by dereferencing the function descriptor to obtain
> function text pointer.
>
> And make do_overwritten() noinline so that it is really
> do_overwritten() which is called by lkdtm_WRITE_KERN().
>
> Signed-off-by: Christophe Leroy
> ---
> drivers/misc/lkdtm/perms.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 60b3b2fe929d..442d60ed25ef 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -5,6 +5,7 @@
> * even non-readable regions.
> */
> #include "lkdtm.h"
> +#include
Why not #include instead here?
> #include
> #include
> #include
> @@ -37,7 +38,7 @@ static noinline void do_nothing(void)
> }
>
> /* Must immediately follow do_nothing for size calculuations to work out. */
> -static void do_overwritten(void)
> +static noinline void do_overwritten(void)
> {
> pr_info("do_overwritten wasn't overwritten!\n");
> return;
> @@ -113,8 +114,9 @@ void lkdtm_WRITE_KERN(void)
> size_t size;
> volatile unsigned char *ptr;
>
> - size = (unsigned long)do_overwritten - (unsigned long)do_nothing;
> - ptr = (unsigned char *)do_overwritten;
> + size = (unsigned long)dereference_symbol_descriptor(do_overwritten) -
> +(unsigned long)dereference_symbol_descriptor(do_nothing);
> + ptr = dereference_symbol_descriptor(do_overwritten);
But otherwise, yup, I expect there will be a bunch of things like this
to clean up in LKDTM. :| Sorry about that!
Acked-by: Kees Cook
>
> pr_info("attempting bad %zu byte write at %px\n", size, ptr);
> memcpy((void *)ptr, (unsigned char *)do_nothing, size);
> --
> 2.31.1
>
--
Kees Cook
Re: [PATCH v1 07/10] lkdtm: Force do_nothing() out of line
On Mon, Oct 11, 2021 at 05:25:34PM +0200, Christophe Leroy wrote: > LKDTM tests display that the run do_nothing() at a given > address, but in reality do_nothing() is inlined into the > caller. > > Force it out of line so that it really runs text at the > displayed address. > > Signed-off-by: Christophe Leroy Acked-by: Kees Cook -- Kees Cook
Re: [PATCH v1 06/10] asm-generic: Refactor dereference_[kernel]_function_descriptor()
On Mon, Oct 11, 2021 at 05:25:33PM +0200, Christophe Leroy wrote:
> dereference_function_descriptor() and
> dereference_kernel_function_descriptor() are identical on the
> three architectures implementing them.
>
> Make it common.
>
> Signed-off-by: Christophe Leroy
> ---
> arch/ia64/include/asm/sections.h| 19 ---
> arch/parisc/include/asm/sections.h | 9 -
> arch/parisc/kernel/process.c| 21 -
> arch/powerpc/include/asm/sections.h | 23 ---
> include/asm-generic/sections.h | 18 ++
> 5 files changed, 18 insertions(+), 72 deletions(-)
A diffstat to love. :)
Reviewed-by: Kees Cook
>
> diff --git a/arch/ia64/include/asm/sections.h
> b/arch/ia64/include/asm/sections.h
> index 929b5c535620..d9addaea8339 100644
> --- a/arch/ia64/include/asm/sections.h
> +++ b/arch/ia64/include/asm/sections.h
> @@ -30,23 +30,4 @@ extern char __start_gate_brl_fsys_bubble_down_patchlist[],
> __end_gate_brl_fsys_b
> extern char __start_unwind[], __end_unwind[];
> extern char __start_ivt_text[], __end_ivt_text[];
>
> -#undef dereference_function_descriptor
> -static inline void *dereference_function_descriptor(void *ptr)
> -{
> - struct fdesc *desc = ptr;
> - void *p;
> -
> - if (!get_kernel_nofault(p, (void *)>addr))
> - ptr = p;
> - return ptr;
> -}
> -
> -#undef dereference_kernel_function_descriptor
> -static inline void *dereference_kernel_function_descriptor(void *ptr)
> -{
> - if (ptr < (void *)__start_opd || ptr >= (void *)__end_opd)
> - return ptr;
> - return dereference_function_descriptor(ptr);
> -}
> -
> #endif /* _ASM_IA64_SECTIONS_H */
> diff --git a/arch/parisc/include/asm/sections.h
> b/arch/parisc/include/asm/sections.h
> index 329e80f7af0a..b041fb32a300 100644
> --- a/arch/parisc/include/asm/sections.h
> +++ b/arch/parisc/include/asm/sections.h
> @@ -12,13 +12,4 @@ typedef Elf64_Fdesc funct_descr_t;
>
> extern char __alt_instructions[], __alt_instructions_end[];
>
> -#ifdef CONFIG_64BIT
> -
> -#undef dereference_function_descriptor
> -void *dereference_function_descriptor(void *);
> -
> -#undef dereference_kernel_function_descriptor
> -void *dereference_kernel_function_descriptor(void *);
> -#endif
> -
> #endif
> diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c
> index 38ec4ae81239..7382576b52a8 100644
> --- a/arch/parisc/kernel/process.c
> +++ b/arch/parisc/kernel/process.c
> @@ -266,27 +266,6 @@ get_wchan(struct task_struct *p)
> return 0;
> }
>
> -#ifdef CONFIG_64BIT
> -void *dereference_function_descriptor(void *ptr)
> -{
> - Elf64_Fdesc *desc = ptr;
> - void *p;
> -
> - if (!get_kernel_nofault(p, (void *)>addr))
> - ptr = p;
> - return ptr;
> -}
> -
> -void *dereference_kernel_function_descriptor(void *ptr)
> -{
> - if (ptr < (void *)__start_opd ||
> - ptr >= (void *)__end_opd)
> - return ptr;
> -
> - return dereference_function_descriptor(ptr);
> -}
> -#endif
> -
> static inline unsigned long brk_rnd(void)
> {
> return (get_random_int() & BRK_RND_MASK) << PAGE_SHIFT;
> diff --git a/arch/powerpc/include/asm/sections.h
> b/arch/powerpc/include/asm/sections.h
> index d0d5287fa568..8f8e95f234e2 100644
> --- a/arch/powerpc/include/asm/sections.h
> +++ b/arch/powerpc/include/asm/sections.h
> @@ -72,29 +72,6 @@ static inline int overlaps_kernel_text(unsigned long
> start, unsigned long end)
> (unsigned long)_stext < end;
> }
>
> -#ifdef PPC64_ELF_ABI_v1
> -
> -#undef dereference_function_descriptor
> -static inline void *dereference_function_descriptor(void *ptr)
> -{
> - struct ppc64_opd_entry *desc = ptr;
> - void *p;
> -
> - if (!get_kernel_nofault(p, (void *)>addr))
> - ptr = p;
> - return ptr;
> -}
> -
> -#undef dereference_kernel_function_descriptor
> -static inline void *dereference_kernel_function_descriptor(void *ptr)
> -{
> - if (ptr < (void *)__start_opd || ptr >= (void *)__end_opd)
> - return ptr;
> -
> - return dereference_function_descriptor(ptr);
> -}
> -#endif /* PPC64_ELF_ABI_v1 */
> -
> #endif
>
> #endif /* __KERNEL__ */
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index 436412d94054..5baaf9d7c671 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -60,6 +60,24 @@ extern __visible const void __nosave_begin, __nosave_end;
>
> /* Function descriptor handling (if any). Override in asm/sections.h */
> #ifdef HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR
> +static inline void *dereference_function_descriptor(void *ptr)
> +{
> + funct_descr_t *desc = ptr;
> + void *p;
> +
> + if (!get_kernel_nofault(p, (void *)>addr))
> + ptr = p;
> + return ptr;
> +}
> +
> +static inline void *dereference_kernel_function_descriptor(void *ptr)
> +{
> + if (ptr < (void
Re: [PATCH v1 05/10] asm-generic: Define 'funct_descr_t' to commonly describe function descriptors
On Mon, Oct 11, 2021 at 05:25:32PM +0200, Christophe Leroy wrote:
> We have three architectures using function descriptors, each with its
> own name.
>
> Add a common typedef that can be used in generic code.
>
> Also add a stub typedef for architecture without function descriptors,
nit: funct_descr_t reads weird to me. why not func_desc_t ? Either way:
Reviewed-by: Kees Cook
> to avoid a forest of #ifdefs.
>
> Signed-off-by: Christophe Leroy
> ---
> arch/ia64/include/asm/sections.h| 1 +
> arch/parisc/include/asm/sections.h | 1 +
> arch/powerpc/include/asm/sections.h | 1 +
> include/asm-generic/sections.h | 3 +++
> 4 files changed, 6 insertions(+)
>
> diff --git a/arch/ia64/include/asm/sections.h
> b/arch/ia64/include/asm/sections.h
> index 80f5868afb06..929b5c535620 100644
> --- a/arch/ia64/include/asm/sections.h
> +++ b/arch/ia64/include/asm/sections.h
> @@ -8,6 +8,7 @@
> */
>
> #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> +typedef struct fdesc funct_descr_t;
>
> #include
> #include
> diff --git a/arch/parisc/include/asm/sections.h
> b/arch/parisc/include/asm/sections.h
> index 2e781ee19b66..329e80f7af0a 100644
> --- a/arch/parisc/include/asm/sections.h
> +++ b/arch/parisc/include/asm/sections.h
> @@ -4,6 +4,7 @@
>
> #ifdef CONFIG_64BIT
> #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> +typedef Elf64_Fdesc funct_descr_t;
> #endif
>
> /* nothing to see, move along */
> diff --git a/arch/powerpc/include/asm/sections.h
> b/arch/powerpc/include/asm/sections.h
> index b7f1ba04e756..d0d5287fa568 100644
> --- a/arch/powerpc/include/asm/sections.h
> +++ b/arch/powerpc/include/asm/sections.h
> @@ -10,6 +10,7 @@
>
> #ifdef PPC64_ELF_ABI_v1
> #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> +typedef struct ppc64_opd_entry funct_descr_t;
> #endif
>
> #include
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index 1db5cfd69817..436412d94054 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -63,6 +63,9 @@ extern __visible const void __nosave_begin, __nosave_end;
> #else
> #define dereference_function_descriptor(p) ((void *)(p))
> #define dereference_kernel_function_descriptor(p) ((void *)(p))
> +typedef struct {
> + unsigned long addr;
> +} funct_descr_t;
> #endif
>
> /* random extra sections (if any). Override
> --
> 2.31.1
>
--
Kees Cook
Re: [PATCH v1 04/10] asm-generic: Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR to define associated stubs
On Mon, Oct 11, 2021 at 05:25:31PM +0200, Christophe Leroy wrote: > Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR instead of > 'dereference_function_descriptor' > to know whether arch has function descriptors. > > Signed-off-by: Christophe Leroy I'd mention the intentionally-empty #if/#else in the commit log, as I, like Helge, mentally tripped over it in the review. :) Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v1 03/10] ia64: Rename 'ip' to 'addr' in 'struct fdesc'
On Mon, Oct 11, 2021 at 05:25:30PM +0200, Christophe Leroy wrote: > There are three architectures with function descriptors, try to > have common names for the address they contain in order to > refactor some functions into generic functions later. > > powerpc has 'funcaddr' > ia64 has 'ip' > parisc has 'addr' > > Vote for 'addr' and update 'struct fdesc' accordingly. > > Signed-off-by: Christophe Leroy Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v1 02/10] powerpc: Rename 'funcaddr' to 'addr' in 'struct ppc64_opd_entry'
On Mon, Oct 11, 2021 at 05:25:29PM +0200, Christophe Leroy wrote: > There are three architectures with function descriptors, try to > have common names for the address they contain in order to > refactor some functions into generic functions later. > > powerpc has 'funcaddr' > ia64 has 'ip' > parisc has 'addr' > > Vote for 'addr' and update 'struct ppc64_opd_entry' accordingly. > > Signed-off-by: Christophe Leroy Reasonable. :) Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v1 01/10] powerpc: Move 'struct ppc64_opd_entry' back into asm/elf.h
On Mon, Oct 11, 2021 at 05:25:28PM +0200, Christophe Leroy wrote:
> 'struct ppc64_opd_entry' doesn't belong to uapi/asm/elf.h
>
> It was initially in module_64.c and commit 2d291e902791 ("Fix compile
> failure with non modular builds") moved it into asm/elf.h
>
> But it was by mistake added outside of __KERNEL__ section,
> therefore commit c3617f72036c ("UAPI: (Scripted) Disintegrate
> arch/powerpc/include/asm") moved it to uapi/asm/elf.h
>
> Move it back into asm/elf.h, this brings it back in line with
> IA64 and PARISC architectures.
>
> Fixes: 2d291e902791 ("Fix compile failure with non modular builds")
> Signed-off-by: Christophe Leroy
I'd agree with Arnd: this is a reasonable cleanup and nothing should be
using it.
Reviewed-by: Kees Cook
--
Kees Cook
Re: [RFC PATCH] powerpc: dts: Remove MPC5xxx platforms
Hi Rob, On Tue, 12 Oct 2021 10:34:56 -0500 Rob Herring wrote: > > The mpc5xxx platforms have had dts warnings for some time which no one > seems to care to fix, so let's just remove the dts files. > > According to Arnd: > "Specifically, MPC5200B has a 15 year lifetime, which ends in > 11 months from now. The original bplan/Genesi Efika 5K2 was > quite popular at the time it came out, and there are probably > still some of those hanging around, but they came with Open > Firmware rather than relying on the dts files that ship with the > kernel. > > Grant Likely was the original maintainer for MPC52xx until 2011, > Anatolij Gustschin is still listed as maintainer since then but hasn't > been active in it for a while either. Anatolij can probably best judge > which of these boards are still in going to be used with future kernels, > but I suspect once you start removing bits from 52xx, the newer > but less common 512x platform can go away as well." > > Cc: Anatolij Gustschin > Cc: Arnd Bergmann > Cc: Stephen Rothwell > Cc: Michael Ellerman > Cc: Benjamin Herrenschmidt > Cc: Paul Mackerras > Cc: linuxppc-dev@lists.ozlabs.org > Signed-off-by: Rob Herring > --- > Sending this out as a feeler to see if anyone cares. If anyone does, > please fix the warnings. Thanks. However .. :-) FATAL ERROR: Couldn't open "mpc5200b.dtsi": No such file or directory make[2]: *** [/home/sfr/next/next/scripts/Makefile.lib:358: arch/powerpc/boot/dts/digsy_mtc.dtb] Error 1 $ grep -wrl mpc5200b.dtsi arch/powerpc/boot/dts/digsy_mtc.dts missed one :-) -- Cheers, Stephen Rothwell pgpBkybkuzazH.pgp Description: OpenPGP digital signature
