Re: Bogus struct page layout on 32-bit
On Sat, Apr 10, 2021 at 03:06:52PM +0100, Matthew Wilcox wrote: > How about moving the flags into the union? A bit messy, but we don't > have to play games with __packed__. Yes, that is probably the better solution, avoiding the games to try and get the union appropriately placed on 32-bit systems. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH v2] mm: Move mem_init_print_info() into mm_init()
On Wed, Mar 17, 2021 at 09:52:10AM +0800, Kefeng Wang wrote: > mem_init_print_info() is called in mem_init() on each architecture, > and pass NULL argument, so using void argument and move it into mm_init(). > > Acked-by: Dave Hansen > Signed-off-by: Kefeng Wang Acked-by: Russell King # for arm bits -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 02/10] ARM: disable CONFIG_IDE in footbridge_defconfig
On Mon, Mar 22, 2021 at 06:10:01PM +0100, Cye Borg wrote: > PWS 500au: > > snow / # lspci -vvx -s 7.1 > 00:07.1 IDE interface: Contaq Microsystems 82c693 (prog-if 80 [ISA > Compatibility mode-only controller, supports bus mastering]) > Control: I/O+ Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop- > ParErr+ Stepping- SERR- FastB2B- DisINTx- > Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium > >TAbort- SERR- Latency: 0 > Interrupt: pin A routed to IRQ 0 > Region 0: I/O ports at 01f0 [size=8] > Region 1: I/O ports at 03f4 > Region 4: I/O ports at 9080 [size=16] > Kernel driver in use: pata_cypress > Kernel modules: pata_cypress > 00: 80 10 93 c6 45 00 80 02 00 80 01 01 00 00 80 00 > 10: f1 01 00 00 f5 03 00 00 00 00 00 00 00 00 00 00 > 20: 81 90 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 30: 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00 00 > > snow / # lspci -vvx -s 7.2 > 00:07.2 IDE interface: Contaq Microsystems 82c693 (prog-if 00 [ISA > Compatibility mode-only controller]) > Control: I/O+ Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop- > ParErr+ Stepping- SERR- FastB2B- DisINTx- > Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium > >TAbort- SERR- Latency: 0 > Interrupt: pin B routed to IRQ 0 > Region 0: I/O ports at 0170 [size=8] > Region 1: I/O ports at 0374 > Region 4: Memory at 0c24 (32-bit, non-prefetchable) > [disabled] [size=64K] > Kernel modules: pata_cypress > 00: 80 10 93 c6 45 00 80 02 00 00 01 01 00 00 80 00 > 10: 71 01 00 00 75 03 00 00 00 00 00 00 00 00 00 00 > 20: 00 00 24 0c 00 00 00 00 00 00 00 00 00 00 00 00 > 30: 00 00 00 00 00 00 00 00 00 00 00 00 00 02 00 00 Thanks very much. Could I also ask for the output of: # lspci -vxxx -s 7.0 as well please - this will dump all 256 bytes for the ISA bridge, which contains a bunch of configuration registers. Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 02/10] ARM: disable CONFIG_IDE in footbridge_defconfig
On Mon, Mar 22, 2021 at 04:33:14PM +0100, Christoph Hellwig wrote: > On Mon, Mar 22, 2021 at 04:18:23PM +0100, Christoph Hellwig wrote: > > On Mon, Mar 22, 2021 at 03:15:03PM +, Russell King - ARM Linux admin > > wrote: > > > It gets worse than that though - due to a change to remove > > > pcibios_min_io from the generic code, moving it into the ARM > > > architecture code, this has caused a regression that prevents the > > > legacy resources being registered against the bus resource. So even > > > if they are there, they cause probe failures. I haven't found a > > > reasonable way to solve this yet, but until there is, there is no > > > way that the PATA driver can be used as the "legacy mode" support > > > is effectively done via the PCI code assigning virtual IO port > > > resources. > > > > > > I'm quite surprised that the CY82C693 even works on Alpha - I've > > > asked for a lspci for that last week but nothing has yet been > > > forthcoming from whoever responded to your patch for Alpha - so I > > > can't compare what I'm seeing with what's happening with Alpha. > > > > That sounds like something we could fix with a quirk for function 2 > > in the PCI resource assignment code. Can you show what vendor and > > device ID function 2 has so that I could try to come up with one? > > Something like this: That solves the problem for the IDE driver, which knows how to deal with legacy mode, but not the PATA driver, which doesn't. The PATA driver needs these resources. As I say, having these resources presents a problem on ARM. A previous commit (3c5d1699887b) changed the way the bus resources are setup which results in /proc/ioports containing: -000f : dma1 0020-003f : pic1 0060-006f : i8042 0070-0073 : rtc_cmos 0070-0073 : rtc0 0080-008f : dma low page 00a0-00bf : pic2 00c0-00df : dma2 0213-0213 : ISAPnP 02f8-02ff : serial8250.0 02f8-02ff : serial 03c0-03df : vga+ 03f8-03ff : serial8250.0 03f8-03ff : serial 0480-048f : dma high page 0a79-0a79 : isapnp write 1000- : PCI0 I/O 1000-107f : :00:08.0 1000-107f : 3c59x 1080-108f : :00:06.1 1090-109f : :00:07.0 1090-109f : pata_it821x 10a0-10a7 : :00:07.0 10a0-10a7 : pata_it821x 10a8-10af : :00:07.0 10a8-10af : pata_it821x 10b0-10b3 : :00:07.0 10b0-10b3 : pata_it821x 10b4-10b7 : :00:07.0 10b4-10b7 : pata_it821x The "PCI0 I/O" resource is the bus level resource, and the legacy resources can not be claimed against that. Without these resources, the PATA cypress driver doesn't work. As I said previously, the reason this regression was not picked up earlier is because I don't upgrade the kernel on this machine very often; the machine has had uptimes into thousands of days. I need to try reverting Rob's commit to find out if anything breaks on this platform - it's completely wrong from a technical point of view for any case where we have a PCI southbridge, since the southbridge provides ISA based resources. I'm not entirely sure what the point of it was, since we still have the PCIBIOS_MIN_IO macro which still uses pcibios_min_io. I'm looking at some of the other changes Rob made back at that time which also look wrong, such as 8ef6e6201b26 which has the effect of locating the 21285 IO resources to PCI address 0, over the top of the ISA southbridge resources. I've no idea what Rob was thinking when he removed the csrio allocation code in that commit, but looking at it to day, it's soo obviously wrong even to a casual glance. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 02/10] ARM: disable CONFIG_IDE in footbridge_defconfig
On Mon, Mar 22, 2021 at 05:09:13PM +0100, John Paul Adrian Glaubitz wrote: > On 3/22/21 4:15 PM, Russell King - ARM Linux admin wrote: > > I'm quite surprised that the CY82C693 even works on Alpha - I've > > asked for a lspci for that last week but nothing has yet been > > forthcoming from whoever responded to your patch for Alpha - so I > > can't compare what I'm seeing with what's happening with Alpha. > > Here is lspci on my DEC Alpha XP-1000: > > root@tsunami:~> lspci > :00:07.0 ISA bridge: Contaq Microsystems 82c693 > :00:07.1 IDE interface: Contaq Microsystems 82c693 > :00:07.2 IDE interface: Contaq Microsystems 82c693 > :00:07.3 USB controller: Contaq Microsystems 82c693 > :00:0d.0 VGA compatible controller: Texas Instruments TVP4020 [Permedia > 2] (rev 01) > 0001:01:03.0 Ethernet controller: Digital Equipment Corporation DECchip > 21142/43 (rev 41) > 0001:01:06.0 SCSI storage controller: QLogic Corp. ISP1020 Fast-wide SCSI > (rev 06) > 0001:01:08.0 PCI bridge: Digital Equipment Corporation DECchip 21152 (rev 03) > 0001:02:09.0 Ethernet controller: Intel Corporation 82541PI Gigabit Ethernet > Controller (rev 05) > root@tsunami:~> This is no good. What I asked last Thursday was: "Could you send me the output of lspci -vvx -s 7.1 and lspci -vvx -s 7.2 please?" so I can see the resources the kernel is using and a dump of the PCI config space to see what the hardware is using. Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 02/10] ARM: disable CONFIG_IDE in footbridge_defconfig
On Mon, Mar 22, 2021 at 04:18:23PM +0100, Christoph Hellwig wrote: > On Mon, Mar 22, 2021 at 03:15:03PM +0000, Russell King - ARM Linux admin > wrote: > > It gets worse than that though - due to a change to remove > > pcibios_min_io from the generic code, moving it into the ARM > > architecture code, this has caused a regression that prevents the > > legacy resources being registered against the bus resource. So even > > if they are there, they cause probe failures. I haven't found a > > reasonable way to solve this yet, but until there is, there is no > > way that the PATA driver can be used as the "legacy mode" support > > is effectively done via the PCI code assigning virtual IO port > > resources. > > > > I'm quite surprised that the CY82C693 even works on Alpha - I've > > asked for a lspci for that last week but nothing has yet been > > forthcoming from whoever responded to your patch for Alpha - so I > > can't compare what I'm seeing with what's happening with Alpha. > > That sounds like something we could fix with a quirk for function 2 > in the PCI resource assignment code. Can you show what vendor and > device ID function 2 has so that I could try to come up with one? I already have a quirk in arch/arm/kernel/bios32.c for this - but it is no longer sufficient due to changes in the PCI layer, where much of this is documented. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 02/10] ARM: disable CONFIG_IDE in footbridge_defconfig
On Mon, Mar 22, 2021 at 03:54:03PM +0100, Christoph Hellwig wrote:
> On Fri, Mar 19, 2021 at 05:53:12PM +0000, Russell King - ARM Linux admin
> wrote:
> > If I extend the arch/arm/kernel/bios32.c code to kill BARs 2/3 (which
> > actually are not present on the CY82C693) then the IDE driver works
> > for me, but the PATA driver does not:
> >
> > cy82c693 :00:06.1: IDE controller (0x1080:0xc693 rev 0x00)
> > cy82c693 :00:06.1: not 100% native mode: will probe irqs later
> > legacy IDE will be removed in 2021, please switch to libata
> > Report any missing HW support to linux-...@vger.kernel.org
> > ide0: BM-DMA at 0x1080-0x1087
> > ide1: BM-DMA at 0x1088-0x108f
> > Probing IDE interface ide0...
> > hda: PIONEER DVD-RW DVR-105, ATAPI CD/DVD-ROM drive
> > hda: host max PIO4 wanted PIO255(auto-tune) selected PIO4
> > ...
> >
> > (unbind Cypress_IDE and try binding pata_cypress)
> >
> > pata_cypress :00:06.1: no available native port
>
> This comes from ata_pci_sff_init_host when it tails to initialize
> a port. There are three cases why it can't initialize the port:
>
> 1) because it is marked as dummy, which is the case for the second
> port of the cypress controller, but you're not using that even
> with the old ide driver, and we'd still not get that message just
> because of that second port.
> 2) when ata_resources_present returns false because the BAR has
> a zero start or length
> 3) because pcim_iomap_regions() fails. This prints a warning to the
> log ("failed to request/iomap BARs for port %d (errno=%d)") that you
> should have seen
>
> So the problem here has to be number two. The legacy ide driver OTOH
> seems to lack a lot of these checks, although I'm not sure how it
> manages to actually work without those.
>
> Can you show how the BAR assignment for the device looks using lscpi
> or a tool of your choice?
There's a big problem here. I have to explicitly zero the resources
(getting rid of the legacy ones assigned by the PCI probe code)
because they are in fact _wrong_ for the CY82C693. The PCI code
assumes that PCI function 1 (primary port) and PCI function 2
(secondary port) are two independent dual-channel IDE ports, and
as the PROG-IF of the class code indicates that all ports are in
legacy mode, the PCI code assigns the legacy ioport resources to
_both_ PCI functions. Essentially, the CY82C693 is a bit of an odd-ball
because it splits the two IDE ports across two functions rather than a
single function.
It gets worse than that though - due to a change to remove
pcibios_min_io from the generic code, moving it into the ARM
architecture code, this has caused a regression that prevents the
legacy resources being registered against the bus resource. So even
if they are there, they cause probe failures. I haven't found a
reasonable way to solve this yet, but until there is, there is no
way that the PATA driver can be used as the "legacy mode" support
is effectively done via the PCI code assigning virtual IO port
resources.
I'm quite surprised that the CY82C693 even works on Alpha - I've
asked for a lspci for that last week but nothing has yet been
forthcoming from whoever responded to your patch for Alpha - so I
can't compare what I'm seeing with what's happening with Alpha.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 02/10] ARM: disable CONFIG_IDE in footbridge_defconfig
On Fri, Mar 19, 2021 at 05:07:53PM +, Russell King - ARM Linux admin wrote: > On Thu, Mar 18, 2021 at 05:56:58AM +0100, Christoph Hellwig wrote: > > footbridge_defconfig enables CONFIG_IDE but no actual host controller > > driver, so just drop it. > > I have been using the Cypress 82C693 IDE driver on Footbridge for a > CD ROM drive, and I know it doesn't work with the PATA driver - as > I need to disable BM DMA, otherwise the 82C693/DC21285 combination > deadlocks the PCI bus. The PATA driver doesn't support disabling > BM DMA without disabling it for all PATA ports, which is really > annoying for my IT821x card in the same machine. > > So, I'm rather stuck using the PATA driver for the HDDs and the > IDE driver for the CD ROM. > > That said, a commit a while back "cleaning up" the PCI layer appears > to have totally shafted the 82C693, as the kernel tries to request > IO resources at the legacy IDE addresses against the PCI bus resource > which only covers 0x1000-0x. Hence, the 82C693 IDE ports are non- > functional at the moment. > > I'm debating about trying to find a fix to the PCI breakage that was > introduced by "ARM: move PCI i/o resource setup into common code". > > I hadn't noticed it because I don't use the CD ROM drive very often, > and I don't upgrade the kernel that often either on the machine - > but it has been running 24x7 for almost two decades. Okay, a bit more on this... If I extend the arch/arm/kernel/bios32.c code to kill BARs 2/3 (which actually are not present on the CY82C693) then the IDE driver works for me, but the PATA driver does not: cy82c693 :00:06.1: IDE controller (0x1080:0xc693 rev 0x00) cy82c693 :00:06.1: not 100% native mode: will probe irqs later legacy IDE will be removed in 2021, please switch to libata Report any missing HW support to linux-...@vger.kernel.org ide0: BM-DMA at 0x1080-0x1087 ide1: BM-DMA at 0x1088-0x108f Probing IDE interface ide0... hda: PIONEER DVD-RW DVR-105, ATAPI CD/DVD-ROM drive hda: host max PIO4 wanted PIO255(auto-tune) selected PIO4 ... (unbind Cypress_IDE and try binding pata_cypress) pata_cypress :00:06.1: no available native port -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 02/10] ARM: disable CONFIG_IDE in footbridge_defconfig
On Thu, Mar 18, 2021 at 05:56:58AM +0100, Christoph Hellwig wrote: > footbridge_defconfig enables CONFIG_IDE but no actual host controller > driver, so just drop it. I have been using the Cypress 82C693 IDE driver on Footbridge for a CD ROM drive, and I know it doesn't work with the PATA driver - as I need to disable BM DMA, otherwise the 82C693/DC21285 combination deadlocks the PCI bus. The PATA driver doesn't support disabling BM DMA without disabling it for all PATA ports, which is really annoying for my IT821x card in the same machine. So, I'm rather stuck using the PATA driver for the HDDs and the IDE driver for the CD ROM. That said, a commit a while back "cleaning up" the PCI layer appears to have totally shafted the 82C693, as the kernel tries to request IO resources at the legacy IDE addresses against the PCI bus resource which only covers 0x1000-0x. Hence, the 82C693 IDE ports are non- functional at the moment. I'm debating about trying to find a fix to the PCI breakage that was introduced by "ARM: move PCI i/o resource setup into common code". I hadn't noticed it because I don't use the CD ROM drive very often, and I don't upgrade the kernel that often either on the machine - but it has been running 24x7 for almost two decades. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH v12 01/14] ARM: mm: add missing pud_page define to 2-level page tables
On Tue, Feb 02, 2021 at 07:47:04PM +0800, Ding Tianhong wrote:
> On 2021/2/2 19:13, Russell King - ARM Linux admin wrote:
> > On Tue, Feb 02, 2021 at 09:05:02PM +1000, Nicholas Piggin wrote:
> >> diff --git a/arch/arm/include/asm/pgtable.h
> >> b/arch/arm/include/asm/pgtable.h
> >> index c02f24400369..d63a5bb6bd0c 100644
> >> --- a/arch/arm/include/asm/pgtable.h
> >> +++ b/arch/arm/include/asm/pgtable.h
> >> @@ -166,6 +166,9 @@ extern struct page *empty_zero_page;
> >>
> >> extern pgd_t swapper_pg_dir[PTRS_PER_PGD];
> >>
> >> +#define pud_page(pud) pmd_page(__pmd(pud_val(pud)))
> >> +#define pud_write(pud)pmd_write(__pmd(pud_val(pud)))
> >
> > As there is no PUD, does it really make sense to return a valid
> > struct page (which will be the PTE page) for pud_page(), which is
> > several tables above?
> >
> --- a/arch/arm/include/asm/pgtable-2level.h
> +++ b/arch/arm/include/asm/pgtable-2level.h
>
> +static inline int pud_none(pud_t pud)
> +{
> + return 0;
> +}
>
> I think it could be fix like this.
We already have that.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH v12 01/14] ARM: mm: add missing pud_page define to 2-level page tables
On Tue, Feb 02, 2021 at 09:05:02PM +1000, Nicholas Piggin wrote: > diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h > index c02f24400369..d63a5bb6bd0c 100644 > --- a/arch/arm/include/asm/pgtable.h > +++ b/arch/arm/include/asm/pgtable.h > @@ -166,6 +166,9 @@ extern struct page *empty_zero_page; > > extern pgd_t swapper_pg_dir[PTRS_PER_PGD]; > > +#define pud_page(pud)pmd_page(__pmd(pud_val(pud))) > +#define pud_write(pud) pmd_write(__pmd(pud_val(pud))) As there is no PUD, does it really make sense to return a valid struct page (which will be the PTE page) for pud_page(), which is several tables above? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [RFC please help] membarrier: Rewrite sync_core_before_usermode()
On Wed, Dec 30, 2020 at 10:00:28AM +, Russell King - ARM Linux admin wrote: > On Wed, Dec 30, 2020 at 12:33:02PM +1000, Nicholas Piggin wrote: > > Excerpts from Russell King - ARM Linux admin's message of December 29, 2020 > > 8:44 pm: > > > On Tue, Dec 29, 2020 at 01:09:12PM +1000, Nicholas Piggin wrote: > > >> I think it should certainly be documented in terms of what guarantees > > >> it provides to application, _not_ the kinds of instructions it may or > > >> may not induce the core to execute. And if existing API can't be > > >> re-documented sanely, then deprecatd and new ones added that DTRT. > > >> Possibly under a new system call, if arch's like ARM want a range > > >> flush and we don't want to expand the multiplexing behaviour of > > >> membarrier even more (sigh). > > > > > > The 32-bit ARM sys_cacheflush() is there only to support self-modifying > > > code, and takes whatever actions are necessary to support that. > > > Exactly what actions it takes are cache implementation specific, and > > > should be of no concern to the caller, but the underlying thing is... > > > it's to support self-modifying code. > > > >Caveat > >cacheflush() should not be used in programs intended to be > > portable. > >On Linux, this call first appeared on the MIPS architecture, but > > nowa‐ > >days, Linux provides a cacheflush() system call on some other > > architec‐ > >tures, but with different arguments. > > > > What a disaster. Another badly designed interface, although it didn't > > originate in Linux it sounds like we weren't to be outdone so > > we messed it up even worse. > > > > flushing caches is neither necessary nor sufficient for code modification > > on many processors. Maybe some old MIPS specific private thing was fine, > > but certainly before it grew to other architectures, somebody should > > have thought for more than 2 minutes about it. Sigh. > > WARNING: You are bordering on being objectionable and offensive with > that comment. > > The ARM interface was designed by me back in the very early days of > Linux, probably while you were still in dypers, based on what was > known at the time. Back in the early 2000s, ideas such as relaxed > memory ordering were not known. All there was was one level of > harvard cache. Sorry, I got that slightly wrong. Its origins on ARM were from 12 Dec 1998: http://www.armlinux.org.uk/developer/patches/viewpatch.php?id=88/1 by Philip Blundell, and first appeared in the ARM pre-patch-2.1.131-19981214-1.gz. It was subsequently documented in the kernel sources by me in July 2001 in ARM patch-2.4.6-rmk2.gz. It has a slightly different signature: the third argument on ARM is a flags argument, whereas the MIPS code, it is some undocumented "cache" parameter. Whether the ARM version pre or post dates the MIPS code, I couldn't say. Whether it was ultimately taken from the MIPS implementation, again, I couldn't say. However, please stop insulting your fellow developers ability to think. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [RFC please help] membarrier: Rewrite sync_core_before_usermode()
On Wed, Dec 30, 2020 at 12:33:02PM +1000, Nicholas Piggin wrote: > Excerpts from Russell King - ARM Linux admin's message of December 29, 2020 > 8:44 pm: > > On Tue, Dec 29, 2020 at 01:09:12PM +1000, Nicholas Piggin wrote: > >> I think it should certainly be documented in terms of what guarantees > >> it provides to application, _not_ the kinds of instructions it may or > >> may not induce the core to execute. And if existing API can't be > >> re-documented sanely, then deprecatd and new ones added that DTRT. > >> Possibly under a new system call, if arch's like ARM want a range > >> flush and we don't want to expand the multiplexing behaviour of > >> membarrier even more (sigh). > > > > The 32-bit ARM sys_cacheflush() is there only to support self-modifying > > code, and takes whatever actions are necessary to support that. > > Exactly what actions it takes are cache implementation specific, and > > should be of no concern to the caller, but the underlying thing is... > > it's to support self-modifying code. > >Caveat >cacheflush() should not be used in programs intended to be portable. >On Linux, this call first appeared on the MIPS architecture, but nowa‐ >days, Linux provides a cacheflush() system call on some other architec‐ >tures, but with different arguments. > > What a disaster. Another badly designed interface, although it didn't > originate in Linux it sounds like we weren't to be outdone so > we messed it up even worse. > > flushing caches is neither necessary nor sufficient for code modification > on many processors. Maybe some old MIPS specific private thing was fine, > but certainly before it grew to other architectures, somebody should > have thought for more than 2 minutes about it. Sigh. WARNING: You are bordering on being objectionable and offensive with that comment. The ARM interface was designed by me back in the very early days of Linux, probably while you were still in dypers, based on what was known at the time. Back in the early 2000s, ideas such as relaxed memory ordering were not known. All there was was one level of harvard cache. So, juts shut up with your insults. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [RFC please help] membarrier: Rewrite sync_core_before_usermode()
On Tue, Dec 29, 2020 at 01:09:12PM +1000, Nicholas Piggin wrote: > I think it should certainly be documented in terms of what guarantees > it provides to application, _not_ the kinds of instructions it may or > may not induce the core to execute. And if existing API can't be > re-documented sanely, then deprecatd and new ones added that DTRT. > Possibly under a new system call, if arch's like ARM want a range > flush and we don't want to expand the multiplexing behaviour of > membarrier even more (sigh). The 32-bit ARM sys_cacheflush() is there only to support self-modifying code, and takes whatever actions are necessary to support that. Exactly what actions it takes are cache implementation specific, and should be of no concern to the caller, but the underlying thing is... it's to support self-modifying code. Sadly, because it's existed for 20+ years, and it has historically been sufficient for other purposes too, it has seen quite a bit of abuse despite its design purpose not changing - it's been used by graphics drivers for example. They quickly learnt the error of their ways with ARMv6+, since it does not do sufficient for their purposes given the cache architectures found there. Let's not go around redesigning this after twenty odd years, requiring a hell of a lot of pain to users. This interface is called by code generated by GCC, so to change it you're looking at patching GCC as well as the kernel, and you basically will make new programs incompatible with older kernels - very bad news for users. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [RFC please help] membarrier: Rewrite sync_core_before_usermode()
On Mon, Dec 28, 2020 at 11:44:33AM -0800, Andy Lutomirski wrote: > On Mon, Dec 28, 2020 at 11:09 AM Russell King - ARM Linux admin > wrote: > > > > On Mon, Dec 28, 2020 at 07:29:34PM +0100, Jann Horn wrote: > > > After chatting with rmk about this (but without claiming that any of > > > this is his opinion), based on the manpage, I think membarrier() > > > currently doesn't really claim to be synchronizing caches? It just > > > serializes cores. So arguably if userspace wants to use membarrier() > > > to synchronize code changes, userspace should first do the code > > > change, then flush icache as appropriate for the architecture, and > > > then do the membarrier() to ensure that the old code is unused? > > > > > > For 32-bit arm, rmk pointed out that that would be the cacheflush() > > > syscall. That might cause you to end up with two IPIs instead of one > > > in total, but we probably don't care _that_ much about extra IPIs on > > > 32-bit arm? > > > > > > For arm64, I believe userspace can flush icache across the entire > > > system with some instructions from userspace - "DC CVAU" followed by > > > "DSB ISH", or something like that, I think? (See e.g. > > > compat_arm_syscall(), the arm64 compat code that implements the 32-bit > > > arm cacheflush() syscall.) > > > > Note that the ARM cacheflush syscall calls flush_icache_user_range() > > over the range of addresses that userspace has passed - it's intention > > since day one is to support cases where userspace wants to change > > executable code. > > > > It will issue the appropriate write-backs to the data cache (DCCMVAU), > > the invalidates to the instruction cache (ICIMVAU), invalidate the > > branch target buffer (BPIALLIS or BPIALL as appropriate), and issue > > the appropriate barriers (DSB ISHST, ISB). > > > > Note that neither flush_icache_user_range() nor flush_icache_range() > > result in IPIs; cache operations are broadcast across all CPUs (which > > is one of the minimums we require for SMP systems.) > > > > Now, that all said, I think the question that has to be asked is... > > > > What is the basic purpose of membarrier? > > > > Is the purpose of it to provide memory barriers, or is it to provide > > memory coherence? > > > > If it's the former and not the latter, then cache flushes are out of > > scope, and expecting memory written to be visible to the instruction > > stream is totally out of scope of the membarrier interface, whether > > or not the writes happen on the same or a different CPU to the one > > executing the rewritten code. > > > > The documentation in the kernel does not seem to describe what it's > > supposed to be doing - the only thing I could find is this: > > Documentation/features/sched/membarrier-sync-core/arch-support.txt > > which describes it as "arch supports core serializing membarrier" > > whatever that means. > > > > Seems to be the standard and usual case of utterly poor to non-existent > > documentation within the kernel tree, or even a pointer to where any > > useful documentation can be found. > > > > Reading the membarrier(2) man page, I find nothing in there that talks > > about any kind of cache coherency for self-modifying code - it only > > seems to be about _barriers_ and nothing more, and barriers alone do > > precisely nothing to save you from non-coherent Harvard caches. > > > > So, either Andy has a misunderstanding, or the man page is wrong, or > > my rudimentary understanding of what membarrier is supposed to be > > doing is wrong... > > Look at the latest man page: > > https://man7.org/linux/man-pages/man2/membarrier.2.html > > for MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE. The result may not be > all that enlightening. MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE (since Linux 4.16) In addition to providing the memory ordering guarantees de■ scribed in MEMBARRIER_CMD_PRIVATE_EXPEDITED, upon return from system call the calling thread has a guarantee that all its run■ ning thread siblings have executed a core serializing instruc■ tion. This guarantee is provided only for threads in the same process as the calling thread. The "expedited" commands complete faster than the non-expedited ones, they never block, but have the downside of causing extra overhead. A process must register its intent to use the priv
Re: [RFC please help] membarrier: Rewrite sync_core_before_usermode()
On Mon, Dec 28, 2020 at 07:29:34PM +0100, Jann Horn wrote: > After chatting with rmk about this (but without claiming that any of > this is his opinion), based on the manpage, I think membarrier() > currently doesn't really claim to be synchronizing caches? It just > serializes cores. So arguably if userspace wants to use membarrier() > to synchronize code changes, userspace should first do the code > change, then flush icache as appropriate for the architecture, and > then do the membarrier() to ensure that the old code is unused? > > For 32-bit arm, rmk pointed out that that would be the cacheflush() > syscall. That might cause you to end up with two IPIs instead of one > in total, but we probably don't care _that_ much about extra IPIs on > 32-bit arm? > > For arm64, I believe userspace can flush icache across the entire > system with some instructions from userspace - "DC CVAU" followed by > "DSB ISH", or something like that, I think? (See e.g. > compat_arm_syscall(), the arm64 compat code that implements the 32-bit > arm cacheflush() syscall.) Note that the ARM cacheflush syscall calls flush_icache_user_range() over the range of addresses that userspace has passed - it's intention since day one is to support cases where userspace wants to change executable code. It will issue the appropriate write-backs to the data cache (DCCMVAU), the invalidates to the instruction cache (ICIMVAU), invalidate the branch target buffer (BPIALLIS or BPIALL as appropriate), and issue the appropriate barriers (DSB ISHST, ISB). Note that neither flush_icache_user_range() nor flush_icache_range() result in IPIs; cache operations are broadcast across all CPUs (which is one of the minimums we require for SMP systems.) Now, that all said, I think the question that has to be asked is... What is the basic purpose of membarrier? Is the purpose of it to provide memory barriers, or is it to provide memory coherence? If it's the former and not the latter, then cache flushes are out of scope, and expecting memory written to be visible to the instruction stream is totally out of scope of the membarrier interface, whether or not the writes happen on the same or a different CPU to the one executing the rewritten code. The documentation in the kernel does not seem to describe what it's supposed to be doing - the only thing I could find is this: Documentation/features/sched/membarrier-sync-core/arch-support.txt which describes it as "arch supports core serializing membarrier" whatever that means. Seems to be the standard and usual case of utterly poor to non-existent documentation within the kernel tree, or even a pointer to where any useful documentation can be found. Reading the membarrier(2) man page, I find nothing in there that talks about any kind of cache coherency for self-modifying code - it only seems to be about _barriers_ and nothing more, and barriers alone do precisely nothing to save you from non-coherent Harvard caches. So, either Andy has a misunderstanding, or the man page is wrong, or my rudimentary understanding of what membarrier is supposed to be doing is wrong... -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [RFC please help] membarrier: Rewrite sync_core_before_usermode()
On Mon, Dec 28, 2020 at 09:14:23AM -0800, Andy Lutomirski wrote: > On Mon, Dec 28, 2020 at 2:25 AM Russell King - ARM Linux admin > wrote: > > > > On Sun, Dec 27, 2020 at 01:36:13PM -0800, Andy Lutomirski wrote: > > > On Sun, Dec 27, 2020 at 12:18 PM Mathieu Desnoyers > > > wrote: > > > > > > > > - On Dec 27, 2020, at 1:28 PM, Andy Lutomirski l...@kernel.org > > > > wrote: > > > > > > > > > > > > > > > > > I admit that I'm rather surprised that the code worked at all on > > > > > arm64, > > > > > and I'm suspicious that it has never been very well tested. My > > > > > apologies > > > > > for not reviewing this more carefully in the first place. > > > > > > > > Please refer to > > > > Documentation/features/sched/membarrier-sync-core/arch-support.txt > > > > > > > > It clearly states that only arm, arm64, powerpc and x86 support the > > > > membarrier > > > > sync core feature as of now: > > > > > > Sigh, I missed arm (32). Russell or ARM folks, what's the right > > > incantation to make the CPU notice instruction changes initiated by > > > other cores on 32-bit ARM? > > > > You need to call flush_icache_range(), since the changes need to be > > flushed from the data cache to the point of unification (of the Harvard > > I and D), and the instruction cache needs to be invalidated so it can > > then see those updated instructions. This will also take care of the > > necessary barriers that the CPU requires for you. > > With what parameters? From looking at the header, this is for the > case in which the kernel writes some memory and then intends to > execute it. That's not what membarrier() does at all. membarrier() > works like this: You didn't specify that you weren't looking at kernel memory. If you're talking about userspace, then the interface you require is flush_icache_user_range(), which does the same as flush_icache_range() but takes userspace addresses. Note that this requires that the memory is currently mapped at those userspace addresses. If that doesn't fit your needs, there isn't an interface to do what you require, and it basically means creating something brand new on every architecture. What you are asking for is not "just a matter of a few instructions". I have stated the required steps to achieve what you require above; that is the minimum when you have non-snooping harvard caches, which the majority of 32-bit ARMs have. > User thread 1: > > write to RWX memory *or* write to an RW alias of an X region. > membarrier(...); > somehow tell thread 2 that we're ready (with a store release, perhaps, > or even just a relaxed store.) > > User thread 2: > > wait for the indication from thread 1. > barrier(); > jump to the code. > > membarrier() is, for better or for worse, not given a range of addresses. Then, I'm sorry, it can't work on 32-bit ARM. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [RFC please help] membarrier: Rewrite sync_core_before_usermode()
On Sun, Dec 27, 2020 at 01:36:13PM -0800, Andy Lutomirski wrote: > On Sun, Dec 27, 2020 at 12:18 PM Mathieu Desnoyers > wrote: > > > > - On Dec 27, 2020, at 1:28 PM, Andy Lutomirski l...@kernel.org wrote: > > > > > > > > > I admit that I'm rather surprised that the code worked at all on arm64, > > > and I'm suspicious that it has never been very well tested. My apologies > > > for not reviewing this more carefully in the first place. > > > > Please refer to > > Documentation/features/sched/membarrier-sync-core/arch-support.txt > > > > It clearly states that only arm, arm64, powerpc and x86 support the > > membarrier > > sync core feature as of now: > > Sigh, I missed arm (32). Russell or ARM folks, what's the right > incantation to make the CPU notice instruction changes initiated by > other cores on 32-bit ARM? You need to call flush_icache_range(), since the changes need to be flushed from the data cache to the point of unification (of the Harvard I and D), and the instruction cache needs to be invalidated so it can then see those updated instructions. This will also take care of the necessary barriers that the CPU requires for you. ... as documented in Documentation/core-api/cachetlb.rst and so should be available on every kernel supported CPU. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
On Tue, Jul 14, 2020 at 03:01:09PM +0200, Peter Zijlstra wrote:
> On Tue, Jul 14, 2020 at 03:19:24PM +0300, Ard Biesheuvel wrote:
> > So perhaps the answer is to have text_alloc() not with a 'where'
> > argument but with a 'why' argument. Or more simply, just have separate
> > alloc/free APIs for each case, with generic versions that can be
> > overridden by the architecture.
>
> Well, there only seem to be 2 cases here, either the pointer needs to
> fit in some immediate displacement, or not.
>
> On x86 we seem have the advantage of a fairly large immediate
> displacement as compared to many other architectures (due to our
> variable sized instructions). And thus have been fairly liberal with our
> usage of it (also our indirect jmps/calls suck, double so with
> RETCH-POLINE).
>
> Still, the indirect jump, as mentioned by Russel should work for
> arbitrarily placed code for us too.
>
>
> So I'm thinking that something like:
>
> enum ptr_type {
> immediate_displacement,
> absolute,
> };
>
> void *text_alloc(unsigned long size, enum ptr_type type)
> {
> unsigned long vstart = VMALLOC_START;
> unsigned long vend = VMALLOC_END;
>
> if (type == immediate_displacement) {
> vstart = MODULES_VADDR;
> vend = MODULES_END;
> }
>
> return __vmalloc_node_range(size, TEXT_ALIGN, vstart, vend,
> GFP_KERNEL, PAGE_KERNEL_EXEC, 0,
> NUMA_NO_NODE, _RET_IP_);
> }
>
> void text_free(void *ptr)
> {
> vfree(ptr);
> }
Beware, however, that on 32-bit ARM, if module PLTs are enabled,
we will try to place the module in the module region (which gives
best performance) but if that allocation fails, we will fall back
to placing it in the vmalloc region and using PLTs.
So, for a module allocation, we would need to make up to two calls
to text_alloc(), once with "immediate_displacement", and if that
fails and we have PLT support enabled, again with "absolute".
Hence, as other people have said, why module_alloc() would need to
stay separate.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
On Tue, Jul 14, 2020 at 01:17:22PM +0300, Ard Biesheuvel wrote: > On Tue, 14 Jul 2020 at 12:53, Jarkko Sakkinen > wrote: > > > > On Mon, Jul 13, 2020 at 10:49:48PM +0300, Ard Biesheuvel wrote: > > > This patch suggests that there are other reasons why conflating > > > allocation of module space and allocating text pages for other uses > > > is a bad idea, but switching all users to text_alloc() is a step in > > > the wrong direction. It would be better to stop using module_alloc() > > > in core code except in the module loader, and have a generic > > > text_alloc() that can be overridden by the arch if necessary. Note > > > that x86 and s390 are the only architectures that use module_alloc() > > > in ftrace code. > > > > This series essentially does this: introduces text_alloc() and > > text_memfree(), which have generic implementations in kernel/text.c. > > Those can be overriddent by arch specific implementations. > > > > What you think should be done differently than in my patch set? > > > > On arm64, module_alloc is only used by the module loader, and so > pulling it out and renaming it will cause unused code to be > incorporated into the kernel when building without module support, > which is the use case you claim to be addressing. > > Module_alloc has semantics that are intimately tied to the module > loader, but over the years, it ended up being (ab)used by other > subsystems, which don't require those semantics but just need n pages > of vmalloc space with executable permissions. > > So the correct approach is to make text_alloc() implement just that, > generically, and switch bpf etc to use it. Then, only on architectures > that need it, override it with an implementation that has the required > additional semantics. > > Refactoring 10+ architectures like this without any regard for how > text_alloc() deviates from module_alloc() just creates a lot of churn > that others will have to clean up after you. For 32-bit ARM, our bpf code uses "blx/bx" (or equivalent code sequences) rather than encoding a "bl" or "b", so BPF there doesn't care where the executable memory is mapped, and doesn't need any PLTs. Given that, should bpf always allocate from the vmalloc() region to preserve the module space for modules? I'm more concerned about ftrace though, but only because I don't have the understanding of that subsystem to really say whether there are any side effects from having the allocations potentially be out of range of a "bl" or "b" instruction. If ftrace jumps both to and from the allocated page using a "load address to register, branch to register" approach like BPF does, then ftrace should be safe - and again, raises the issue that maybe it should always come from vmalloc() space. So, I think we need to keep module_alloc() for allocating memory for modules, and provide a new text_alloc() for these other cases. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
On Tue, Jul 14, 2020 at 12:49:28PM +0300, Jarkko Sakkinen wrote: > On Mon, Jul 13, 2020 at 07:34:10PM +0100, Russell King - ARM Linux admin > wrote: > > On Mon, Jul 13, 2020 at 09:19:37PM +0300, Jarkko Sakkinen wrote: > > > Rename module_alloc() to text_alloc() and module_memfree() to > > > text_memfree(), and move them to kernel/text.c, which is unconditionally > > > compiled to the kernel proper. This allows kprobes, ftrace and bpf to > > > allocate space for executable code without requiring to compile the > > > modules > > > support (CONFIG_MODULES=y) in. > > > > I'm not sure this is a good idea for 32-bit ARM. The code you are > > moving for 32-bit ARM is quite specific to module use in that it also > > supports allocating from the vmalloc area, where the module code > > knows to add PLT entries. > > > > If the other proposed users of this text_alloc() do not have the logic > > to add PLT entries when branches between kernel code and this > > allocation are not reachable by, e.g. a 26-bit signed offset for 32-bit > > ARM code, then this code is not suitable for that use. > > My intention is to use this in kprobes code in the place of > module_alloc(). I'm not sure why moving this code out of the module > subsystem could possibly break anything. Unfortunately I forgot to add > covere letter to my series. Sending v2 with that to explain my use case > for this. Ah, so you're merely renaming module_alloc() to text_alloc() everywhere? It sounded from the initial patch like you were also converting other users to use module_alloc(). -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
On Tue, Jul 14, 2020 at 12:45:36PM +0300, Jarkko Sakkinen wrote: > Rename module_alloc() to text_alloc() and module_memfree() to > text_memfree(), and move them to kernel/text.c, which is unconditionally > compiled to the kernel proper. This allows kprobes, ftrace and bpf to > allocate space for executable code without requiring to compile the modules > support (CONFIG_MODULES=y) in. As I said in response to your first post of this, which seems to have gone unacknowledged, I don't think this is a good idea. So, NAK on the whole concept this without some discussion on the 32-bit ARM issues. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
On Mon, Jul 13, 2020 at 09:19:37PM +0300, Jarkko Sakkinen wrote: > Rename module_alloc() to text_alloc() and module_memfree() to > text_memfree(), and move them to kernel/text.c, which is unconditionally > compiled to the kernel proper. This allows kprobes, ftrace and bpf to > allocate space for executable code without requiring to compile the modules > support (CONFIG_MODULES=y) in. I'm not sure this is a good idea for 32-bit ARM. The code you are moving for 32-bit ARM is quite specific to module use in that it also supports allocating from the vmalloc area, where the module code knows to add PLT entries. If the other proposed users of this text_alloc() do not have the logic to add PLT entries when branches between kernel code and this allocation are not reachable by, e.g. a 26-bit signed offset for 32-bit ARM code, then this code is not suitable for that use. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH v2 5/5] uaccess: Rename user_access_begin/end() to user_full_access_begin/end()
On Tue, Apr 21, 2020 at 03:49:19AM +0100, Al Viro wrote: > The only source I'd been able to find speeks of >= 60 cycles > (and possibly much more) for non-pipelined coprocessor instructions; > the list of such does contain loads and stores to a bunch of registers. > However, the register in question (p15/c3) has only store mentioned there, > so loads might be cheap; no obvious reasons for those to be slow. > That's a question to arm folks, I'm afraid... rmk? I have no information on that; instruction timings are not defined at architecture level (architecture reference manual), nor do I find information in the CPU technical reference manual (which would be specific to the CPU). Instruction timings tend to be implementation dependent. I've always consulted Will Deacon when I've needed to know whether an instruction is expensive or not. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
Re: [PATCH] soc: fsl: dpio: avoid stack usage warning
On Wed, Apr 08, 2020 at 08:58:16PM +0200, Arnd Bergmann wrote:
> A 1024 byte variable on the stack will warn on any 32-bit architecture
> during compile-testing, and is generally a bad idea anyway:
>
> fsl/dpio/dpio-service.c: In function
> 'dpaa2_io_service_enqueue_multiple_desc_fq':
> fsl/dpio/dpio-service.c:495:1: error: the frame size of 1032 bytes is larger
> than 1024 bytes [-Werror=frame-larger-than=]
>
> There are currently no callers of this function, so I cannot tell whether
> dynamic memory allocation is allowed once callers are added. Change
> it to kcalloc for now, if anyone gets a warning about calling this in
> atomic context after they start using it, they can fix it later.
>
> Fixes: 9d98809711ae ("soc: fsl: dpio: Adding QMAN multiple enqueue interface")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/soc/fsl/dpio/dpio-service.c | 18 +-
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/soc/fsl/dpio/dpio-service.c
> b/drivers/soc/fsl/dpio/dpio-service.c
> index cd4f6410e8c2..ff0ef8cbdbff 100644
> --- a/drivers/soc/fsl/dpio/dpio-service.c
> +++ b/drivers/soc/fsl/dpio/dpio-service.c
> @@ -478,12 +478,17 @@ int dpaa2_io_service_enqueue_multiple_desc_fq(struct
> dpaa2_io *d,
> const struct dpaa2_fd *fd,
> int nb)
> {
> - int i;
> - struct qbman_eq_desc ed[32];
> + struct qbman_eq_desc *ed = kcalloc(sizeof(struct qbman_eq_desc), 32,
> GFP_KERNEL);
I think you need to rearrange this to be more compliant with the coding
style.
> + int i, ret;
> +
> + if (!ed)
> + return -ENOMEM;
>
> d = service_select(d);
> - if (!d)
> - return -ENODEV;
> + if (!d) {
> + ret = -ENODEV;
> + goto out;
> + }
>
> for (i = 0; i < nb; i++) {
> qbman_eq_desc_clear([i]);
> @@ -491,7 +496,10 @@ int dpaa2_io_service_enqueue_multiple_desc_fq(struct
> dpaa2_io *d,
> qbman_eq_desc_set_fq([i], fqid[i]);
> }
>
> - return qbman_swp_enqueue_multiple_desc(d->swp, [0], fd, nb);
> + ret = qbman_swp_enqueue_multiple_desc(d->swp, [0], fd, nb);
> +out:
> + kfree(ed);
> + return ret;
> }
> EXPORT_SYMBOL(dpaa2_io_service_enqueue_multiple_desc_fq);
>
> --
> 2.26.0
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
Re: decruft the vmalloc API
On Wed, Apr 08, 2020 at 01:58:58PM +0200, Christoph Hellwig wrote: > Hi all, > > Peter noticed that with some dumb luck you can toast the kernel address > space with exported vmalloc symbols. > > I used this as an opportunity to decruft the vmalloc.c API and make it > much more systematic. This also removes any chance to create vmalloc > mappings outside the designated areas or using executable permissions > from modules. Besides that it removes more than 300 lines of code. I haven't read all your patches yet. Have you tested it on 32-bit ARM, where the module area is located _below_ PAGE_OFFSET and outside of the vmalloc area? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
Re: [PATCH RESEND 1/4] uaccess: Add user_read_access_begin/end and user_write_access_begin/end
On Fri, Apr 03, 2020 at 12:26:10PM +0100, Catalin Marinas wrote: > On Fri, Apr 03, 2020 at 01:58:31AM +0100, Al Viro wrote: > > On Thu, Apr 02, 2020 at 11:35:57AM -0700, Kees Cook wrote: > > > Yup, I think it's a weakness of the ARM implementation and I'd like to > > > not extend it further. AFAIK we should never nest, but I would not be > > > surprised at all if we did. > > > > > > If we were looking at a design goal for all architectures, I'd like > > > to be doing what the public PaX patchset did for their memory access > > > switching, which is to alarm if calling into "enable" found the access > > > already enabled, etc. Such a condition would show an unexpected nesting > > > (like we've seen with similar constructs with set_fs() not getting reset > > > during an exception handler, etc etc). > > > > FWIW, maybe I'm misreading the ARM uaccess logics, but... it smells like > > KERNEL_DS is somewhat more dangerous there than on e.g. x86. > > > > Look: with CONFIG_CPU_DOMAINS, set_fs(KERNEL_DS) tells MMU to ignore > > per-page permission bits in DOMAIN_KERNEL (i.e. for kernel address > > ranges), allowing them even if they would normally be denied. We need > > that for actual uaccess loads/stores, since those use insns that pretend > > to be done in user mode and we want them to access the kernel pages. > > But that affects the normal loads/stores as well; unless I'm misreading > > that code, it will ignore (supervisor) r/o on a page. And that's not > > just for the code inside the uaccess blocks; *everything* done under > > KERNEL_DS is subject to that. > > That's correct. Luckily this only affects ARMv5 and earlier. From ARMv6 > onwards, CONFIG_CPU_USE_DOMAINS is no longer selected and the uaccess > instructions are just plain ldr/str. > > Russell should know the details on whether there was much choice. Since > the kernel was living in the linear map with full rwx permissions, the > KERNEL_DS overriding was probably not a concern and the ldrt/strt for > uaccess deemed more secure. We also have weird permission setting > pre-ARMv6 (or rather v6k) where RO user pages are writable from the > kernel with standard str instructions (breaking CoW). I don't recall > whether it was a choice made by the kernel or something the architecture > enforced. The vectors page has to be kernel writable (and user RO) to > store the TLS value in the absence of a TLS register but maybe we could > do this via the linear alias together with the appropriate cache > maintenance. > > From ARMv6, the domain overriding had the side-effect of ignoring the XN > bit and causing random instruction fetches from ioremap() areas. So we > had to remove the domain switching. We also gained a dedicated TLS > register. Indeed. On pre-ARMv6, we have the following choices for protection attributes: Page tables Control Reg Privileged User AP S,R permission permission 00 0,0 No access No access 00 1,0 Read-only No access 00 0,1 Read-only Read-only 00 1,1 Unpredictable Unpredictable 01 X,X Read/Write No access 10 X,X Read/Write Read-only 11 X,X Read/Write Read/Write We use S,R=1,0 under Linux because this allows us to read-protect kernel pages without making them visible to userspace. If we changed to S,R=0,1, then we could have our read-only permissions for both kernel and userspace, drop domain switching, and use the plain LDR/STR instructions, but we then lose the ability to write-protect module executable code and other parts of kernel space without making them visible to userspace. So, it essentially boils down to making a choice - which set of security features we think are the most important. > I think uaccess_enable() could indeed switch the kernel domain if > KERNEL_DS is set and move this out of set_fs(). It would reduce the > window the kernel domain permissions are overridden. Anyway, > uaccess_enable() appeared much later on arm when Russell introduced PAN > (SMAP) like support by switching the user domain. Yes, that would be a possibility. Another possibility would be to eliminate as much usage of KERNEL_DS as possible - I've just found one instance in sys_oabi-compat.c that can be eliminated (epoll_ctl) but there's several there that can't with the current code structure, and re-coding the contents of some fs/* functions to work around that is a very bad idea. If there's some scope for rejigging some of the fs/* code, it may be possible to elimate some other cases in there. I notice that the fs/* code seems like some of the last remaining users of KERNEL_DS, although I suspect that some aren't possible to eliminate. :( -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps
Re: [PATCH RESEND 1/4] uaccess: Add user_read_access_begin/end and user_write_access_begin/end
On Thu, Apr 02, 2020 at 11:35:57AM -0700, Kees Cook wrote:
> On Thu, Apr 02, 2020 at 06:50:32PM +0100, Al Viro wrote:
> > On Thu, Apr 02, 2020 at 07:03:28PM +0200, Christophe Leroy wrote:
> >
> > > user_access_begin() grants both read and write.
> > >
> > > This patch adds user_read_access_begin() and user_write_access_begin() but
> > > it doesn't remove user_access_begin()
> >
> > Ouch... So the most generic name is for the rarest case?
> >
> > > > What should we do about that? Do we prohibit such blocks outside
> > > > of arch?
> > > >
> > > > What should we do about arm and s390? There we want a cookie passed
> > > > from beginning of block to its end; should that be a return value?
> > >
> > > That was the way I implemented it in January, see
> > > https://patchwork.ozlabs.org/patch/1227926/
> > >
> > > There was some discussion around that and most noticeable was:
> > >
> > > H. Peter (hpa) said about it: "I have *deep* concern with carrying state
> > > in
> > > a "key" variable: it's a direct attack vector for a crowbar attack,
> > > especially since it is by definition live inside a user access region."
> >
> > > This patch minimises the change by just adding user_read_access_begin()
> > > and
> > > user_write_access_begin() keeping the same parameters as the existing
> > > user_access_begin().
> >
> > Umm... What about the arm situation? The same concerns would apply there,
> > wouldn't they? Currently we have
> > static __always_inline unsigned int uaccess_save_and_enable(void)
> > {
> > #ifdef CONFIG_CPU_SW_DOMAIN_PAN
> > unsigned int old_domain = get_domain();
> >
> > /* Set the current domain access to permit user accesses */
> > set_domain((old_domain & ~domain_mask(DOMAIN_USER)) |
> >domain_val(DOMAIN_USER, DOMAIN_CLIENT));
> >
> > return old_domain;
> > #else
> > return 0;
> > #endif
> > }
> > and
> > static __always_inline void uaccess_restore(unsigned int flags)
> > {
> > #ifdef CONFIG_CPU_SW_DOMAIN_PAN
> > /* Restore the user access mask */
> > set_domain(flags);
> > #endif
> > }
> >
> > How much do we need nesting on those, anyway? rmk?
It's that way because it's easy, logical, and actually *more* efficient
to do it that way, rather than read-modify-write the domain register
each time we want to change it.
> Yup, I think it's a weakness of the ARM implementation and I'd like to
> not extend it further. AFAIK we should never nest, but I would not be
> surprised at all if we did.
There is one known nesting, which is __clear_user() when used with
the (IMHO horrid and I don't care about) UACCESS_WITH_MEMCPY feature.
That's not intentional however.
When I introduced this on ARM, the placement I adopted was to locate
it _as close as sanely possible_ to the userspace access so we
minimised the kernel accesses, so we minimise the number of accesses
that could go stray because of the domain issue - we ideally only
want the access done by the accessor itself to be affected, which
we achieve for most accesses.
Thinking laterally, maybe we should get rid of the whole KERNEL_DS
stuff entirely, and come up with an alternative way of handling the
kernel-wants-to-access-kernelspace-via-user-accessors problem.
Such as, copying some data back to userspace memory?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
Re: [PATCH RESEND 1/4] uaccess: Add user_read_access_begin/end and user_write_access_begin/end
On Fri, Apr 03, 2020 at 01:58:31AM +0100, Al Viro wrote: > On Thu, Apr 02, 2020 at 11:35:57AM -0700, Kees Cook wrote: > > > Yup, I think it's a weakness of the ARM implementation and I'd like to > > not extend it further. AFAIK we should never nest, but I would not be > > surprised at all if we did. > > > > If we were looking at a design goal for all architectures, I'd like > > to be doing what the public PaX patchset did for their memory access > > switching, which is to alarm if calling into "enable" found the access > > already enabled, etc. Such a condition would show an unexpected nesting > > (like we've seen with similar constructs with set_fs() not getting reset > > during an exception handler, etc etc). > > FWIW, maybe I'm misreading the ARM uaccess logics, but... it smells like > KERNEL_DS is somewhat more dangerous there than on e.g. x86. > > Look: with CONFIG_CPU_DOMAINS, set_fs(KERNEL_DS) tells MMU to ignore > per-page permission bits in DOMAIN_KERNEL (i.e. for kernel address > ranges), allowing them even if they would normally be denied. We need > that for actual uaccess loads/stores, since those use insns that pretend > to be done in user mode and we want them to access the kernel pages. > But that affects the normal loads/stores as well; unless I'm misreading > that code, it will ignore (supervisor) r/o on a page. And that's not > just for the code inside the uaccess blocks; *everything* done under > KERNEL_DS is subject to that. > > Why do we do that (modify_domain(), that is) inside set_fs() and not > in uaccess_enable() et.al.? First, CONFIG_CPU_DOMAINS is used on older ARMs, not ARMv7. Second, the kernel image itself is not RO-protected on any ARM32 platform. If we get rid of CONFIG_CPU_DOMAINS, we will use the ARMv7 method of user access, which is to use normal load/stores for the user accessors and every access must check against the address limit, even the __-accessors. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
Re: [PATCH v2 00/13] mm: remove __ARCH_HAS_5LEVEL_HACK
On Sun, Feb 16, 2020 at 10:18:30AM +0200, Mike Rapoport wrote: > From: Mike Rapoport > > Hi, > > These patches convert several architectures to use page table folding and > remove __ARCH_HAS_5LEVEL_HACK along with include/asm-generic/5level-fixup.h. > > The changes are mostly about mechanical replacement of pgd accessors with p4d > ones and the addition of higher levels to page table traversals. > > All the patches were sent separately to the respective arch lists and > maintainers hence the "v2" prefix. You fail to explain why this change which adds 488 additional lines of code is desirable. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH V13] mm/debug: Add tests validating architecture page table helpers
On Tue, Feb 11, 2020 at 06:33:47AM +0100, Christophe Leroy wrote: > > > Le 11/02/2020 à 03:25, Anshuman Khandual a écrit : > > > > > > On 02/10/2020 04:36 PM, Russell King - ARM Linux admin wrote: > > > There are good reasons for the way ARM does stuff. The generic crap was > > > written without regard for the circumstances that ARM has, and thus is > > > entirely unsuitable for 32-bit ARM. > > > > Since we dont have an agreement here, lets just settle with disabling the > > test for now on platforms where the build fails. CONFIG_EXPERT is enabling > > this test for better adaptability and coverage, hence how about re framing > > the config like this ? This at the least conveys the fact that EXPERT only > > works when platform is neither IA64 or ARM. > > Agreed > > > > > config DEBUG_VM_PGTABLE > > bool "Debug arch page table for semantics compliance" > > depends on MMU > > depends on ARCH_HAS_DEBUG_VM_PGTABLE || (EXPERT && !(IA64 || ARM)) > > I think it's maybe better to have a dedicated depends line: > > depends on !IA64 && !ARM > depends on ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT > > The day arm and/or ia64 is ready for building the test, we can remove that > depends. Never going to happen as its technically infeasible, sorry. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH V13] mm/debug: Add tests validating architecture page table helpers
On Mon, Feb 10, 2020 at 11:46:23AM +0100, Christophe Leroy wrote:
>
>
> Le 10/02/2020 à 11:02, Russell King - ARM Linux admin a écrit :
> > On Mon, Feb 10, 2020 at 07:38:38AM +0100, Christophe Leroy wrote:
> > >
> > >
> > > Le 10/02/2020 à 06:35, Anshuman Khandual a écrit :
> > > >
> > > >
> > > > On 02/10/2020 10:22 AM, Andrew Morton wrote:
> > > > > On Thu, 6 Feb 2020 13:49:35 +0530 Anshuman Khandual
> > > > > wrote:
> > > > >
> > > > > >
> > > > > > On 02/06/2020 04:40 AM, kbuild test robot wrote:
> > > > > > > Hi Anshuman,
> > > > > > >
> > > > > > > Thank you for the patch! Yet something to improve:
> > > > > > >
> > > > > > > [auto build test ERROR on powerpc/next]
> > > > > > > [also build test ERROR on s390/features linus/master arc/for-next
> > > > > > > v5.5]
> > > > > > > [cannot apply to mmotm/master tip/x86/core arm64/for-next/core
> > > > > > > next-20200205]
> > > > > > > [if your patch is applied to the wrong git tree, please drop us a
> > > > > > > note to help
> > > > > > > improve the system. BTW, we also suggest to use '--base' option
> > > > > > > to specify the
> > > > > > > base tree in git format-patch, please see
> > > > > > > https://stackoverflow.com/a/37406982]
> > > > > > >
> > > > > > > url:
> > > > > > > https://github.com/0day-ci/linux/commits/Anshuman-Khandual/mm-debug-Add-tests-validating-architecture-page-table-helpers/20200205-215507
> > > > > > > base:
> > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git
> > > > > > > next
> > > > > > > config: ia64-allmodconfig (attached as .config)
> > > > > > > compiler: ia64-linux-gcc (GCC) 7.5.0
> > > > > > > reproduce:
> > > > > > > wget
> > > > > > > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
> > > > > > > -O ~/bin/make.cross
> > > > > > > chmod +x ~/bin/make.cross
> > > > > > > # save the attached .config to linux build tree
> > > > > > > GCC_VERSION=7.5.0 make.cross ARCH=ia64
> > > > > > >
> > > > > > > If you fix the issue, kindly add following tag
> > > > > > > Reported-by: kbuild test robot
> > > > > > >
> > > > > > > All error/warnings (new ones prefixed by >>):
> > > > > > >
> > > > > > > In file included from
> > > > > > > include/asm-generic/pgtable-nopud.h:8:0,
> > > > > > > from arch/ia64/include/asm/pgtable.h:586,
> > > > > > > from include/linux/mm.h:99,
> > > > > > > from include/linux/highmem.h:8,
> > > > > > > from mm/debug_vm_pgtable.c:14:
> > > > > > > mm/debug_vm_pgtable.c: In function 'pud_clear_tests':
> > > > > > > > > include/asm-generic/pgtable-nop4d-hack.h:47:32: error:
> > > > > > > > > implicit declaration of function '__pgd'; did you mean
> > > > > > > > > '__p4d'? [-Werror=implicit-function-declaration]
> > > > > > > #define __pud(x)((pud_t) { __pgd(x) })
> > > > > > > ^
> > > > > > > > > mm/debug_vm_pgtable.c:141:8: note: in expansion of macro
> > > > > > > > > '__pud'
> > > > > > >pud = __pud(pud_val(pud) | RANDOM_ORVALUE);
> > > > > > > ^
> > > > > > > > > include/asm-generic/pgtable-nop4d-hack.h:47:22: warning:
> > > > > > > > > missing braces around initializer [-Wmissing-braces]
> > > > > > > #define __pud(x)((pud_t) { __pgd(x) })
> > > > > > >^
> > > > > > > > > mm/debug_vm_pgtable.c:141:8: note: in expansion of macro
> > > > > > > > > '__pud'
>
Re: [PATCH V13] mm/debug: Add tests validating architecture page table helpers
On Mon, Feb 10, 2020 at 07:38:38AM +0100, Christophe Leroy wrote:
>
>
> Le 10/02/2020 à 06:35, Anshuman Khandual a écrit :
> >
> >
> > On 02/10/2020 10:22 AM, Andrew Morton wrote:
> > > On Thu, 6 Feb 2020 13:49:35 +0530 Anshuman Khandual
> > > wrote:
> > >
> > > >
> > > > On 02/06/2020 04:40 AM, kbuild test robot wrote:
> > > > > Hi Anshuman,
> > > > >
> > > > > Thank you for the patch! Yet something to improve:
> > > > >
> > > > > [auto build test ERROR on powerpc/next]
> > > > > [also build test ERROR on s390/features linus/master arc/for-next
> > > > > v5.5]
> > > > > [cannot apply to mmotm/master tip/x86/core arm64/for-next/core
> > > > > next-20200205]
> > > > > [if your patch is applied to the wrong git tree, please drop us a
> > > > > note to help
> > > > > improve the system. BTW, we also suggest to use '--base' option to
> > > > > specify the
> > > > > base tree in git format-patch, please see
> > > > > https://stackoverflow.com/a/37406982]
> > > > >
> > > > > url:
> > > > > https://github.com/0day-ci/linux/commits/Anshuman-Khandual/mm-debug-Add-tests-validating-architecture-page-table-helpers/20200205-215507
> > > > > base:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> > > > > config: ia64-allmodconfig (attached as .config)
> > > > > compiler: ia64-linux-gcc (GCC) 7.5.0
> > > > > reproduce:
> > > > > wget
> > > > > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
> > > > > -O ~/bin/make.cross
> > > > > chmod +x ~/bin/make.cross
> > > > > # save the attached .config to linux build tree
> > > > > GCC_VERSION=7.5.0 make.cross ARCH=ia64
> > > > >
> > > > > If you fix the issue, kindly add following tag
> > > > > Reported-by: kbuild test robot
> > > > >
> > > > > All error/warnings (new ones prefixed by >>):
> > > > >
> > > > > In file included from include/asm-generic/pgtable-nopud.h:8:0,
> > > > > from arch/ia64/include/asm/pgtable.h:586,
> > > > > from include/linux/mm.h:99,
> > > > > from include/linux/highmem.h:8,
> > > > > from mm/debug_vm_pgtable.c:14:
> > > > > mm/debug_vm_pgtable.c: In function 'pud_clear_tests':
> > > > > > > include/asm-generic/pgtable-nop4d-hack.h:47:32: error: implicit
> > > > > > > declaration of function '__pgd'; did you mean '__p4d'?
> > > > > > > [-Werror=implicit-function-declaration]
> > > > > #define __pud(x)((pud_t) { __pgd(x) })
> > > > > ^
> > > > > > > mm/debug_vm_pgtable.c:141:8: note: in expansion of macro '__pud'
> > > > > pud = __pud(pud_val(pud) | RANDOM_ORVALUE);
> > > > > ^
> > > > > > > include/asm-generic/pgtable-nop4d-hack.h:47:22: warning: missing
> > > > > > > braces around initializer [-Wmissing-braces]
> > > > > #define __pud(x)((pud_t) { __pgd(x) })
> > > > > ^
> > > > > > > mm/debug_vm_pgtable.c:141:8: note: in expansion of macro '__pud'
> > > > > pud = __pud(pud_val(pud) | RANDOM_ORVALUE);
> > > > > ^
> > > > > cc1: some warnings being treated as errors
> > > >
> > > > This build failure is expected now given that we have allowed
> > > > DEBUG_VM_PGTABLE
> > > > with EXPERT without platform requiring ARCH_HAS_DEBUG_VM_PGTABLE. This
> > > > problem
> > > > i.e build failure caused without a platform __pgd(), is known to exist
> > > > both on
> > > > ia64 and arm (32bit) platforms. Please refer
> > > > https://lkml.org/lkml/2019/9/24/314
> > > > for details where this was discussed earlier.
> > > >
> > >
> > > I'd prefer not to merge a patch which is known to cause build
> > > regressions. Is there some temporary thing we can do to prevent these
> > > errors until arch maintainers(?) get around to implementing the
> > > long-term fixes?
> >
> > We could explicitly disable CONFIG_DEBUG_VM_PGTABLE on ia64 and arm
> > platforms
> > which will ensure that others can still use the EXPERT path.
> >
> > config DEBUG_VM_PGTABLE
> > bool "Debug arch page table for semantics compliance"
> > depends on MMU
> > depends on !(IA64 || ARM)
> > depends on ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT
> > default n if !ARCH_HAS_DEBUG_VM_PGTABLE
> > default y if DEBUG_VM
> >
>
> On both ia32 and arm, the fix is trivial.
>
> Can we include the fix within this patch, just the same way as the
> mm_p4d_folded() fix for x86 ?
Why should arm include a macro for something that nothing (apart from
this checker) requires? If the checker requires it but the rest of
the kernel does not, it suggests that the checker isn't actually
correct, and the results can't be relied upon.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH 00/50] Add log level to show_stack()
On Fri, Nov 08, 2019 at 04:28:30PM +, Dmitry Safonov wrote: > On 11/6/19 8:34 PM, Peter Zijlstra wrote: > > On Wed, Nov 06, 2019 at 04:27:33PM +, Dmitry Safonov wrote: > [..] > >> Sorry, I should have tried to describe better. > >> > >> I'm trying to remove external users of console_loglevel by following > >> reasons: > > > > I suppose since all my machines have 'debug ignore_loglevel > > earlyprintk=serial,ttyS0,115200 console=ttyS0,115200' I don't have this > > experience. > > Yeah, I remember you avoid all those functionalities of printk(), fair > enough. On the other side, regular users and I'm betting most of > the non-tuned distributions use /proc/sys/kernel/printk by default. > (Checking on my Arch & Fedora - loglevel 4 from the box) > > >> - changing console_loglevel on SMP means that unwanted messages from > >> other CPUs will appear (that have lower log level) > >> - on UMP unwanted messages may appear if the code is preempted while it > >> hasn't set the console_loglevel back to old > >> - rising console_loglevel to print wanted message(s) may not work at all > >> if printk() has being delayed and the console_loglevel is already set > >> back to old value > > > > Sure, frobbing the global console_loglevel is bad. > > > >> I also have patches in wip those needs to print backtrace with specific > >> loglevel (higher when it's critical, lower when it's notice and > >> shouldn't go to serial console). > > > > (everything always should go to serial, serial is awesome :-) > > Personally I agree. Unfortunately, here @Arista there are switches (I'm > speaking about the order of thousands at least) those have baud-rate 9600. > It's a bit expensive being elaborate with such setup. > > >> Besides on local tests I see hits those have headers (messages like > >> "Backtrace: ") without an actual backtrace and the reverse - a backtrace > >> without a reason for it. It's quite annoying and worth addressing by > >> syncing headers log levels to backtraces. > > > > I suppose I'm surprised there are backtraces that are not important. > > Either badness happened and it needs printing, or the user asked for it > > and it needs printing. > > > > Perhaps we should be removing backtraces if they're not important > > instead of allowing to print them as lower loglevels? > > Well, the use-case for lower log-level is that everything goes into logs > (/var/log/dmesg or /var/log/messages whatever rsyslog has settting). > > That has it's value: > - after a failure (i.e. panic) messages, those were only signs that > something goes wrong can be seen in logs which can give ideas what has > happened. No they don't. When the kernel panics, userspace generally stops running, so rsyslog won't be able to write them to /var/log/messages. How, by "kernel panics" I mean a real kernel panic, which probably isn't what you're talking about there. You are probably talking about the whole shebang of non-fatal kernel oops, kernel warnings and the like. If so, I'd ask you to stop confuzzilating terminology. If you really want to capture such events, then you need to have the kernel write the panic to (e.g.) flash - see the mtdoops driver. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH 00/50] Add log level to show_stack()
On Wed, Nov 06, 2019 at 09:34:40PM +0100, Peter Zijlstra wrote: > I suppose I'm surprised there are backtraces that are not important. > Either badness happened and it needs printing, or the user asked for it > and it needs printing. Or utterly meaningless. > Perhaps we should be removing backtraces if they're not important > instead of allowing to print them as lower loglevels? Definitely! WARN_ON() is well overused - and as is typical, used without much thought. Bound to happen after Linus got shirty about BUG_ON() being over used. Everyone just grabbed the next nearest thing to assert(). As a kind of example, I've recently come across one WARN_ON() in a driver subsystem (that shall remain nameless at the moment) which very likely has multiple different devices on a platform. The WARN_ON() triggers as a result of a problem with the hardware, but because it's a WARN_ON(), you've no idea which device has a problem. The backtrace is mostly meaningless. So you know that a problem has occurred, but the kernel prints *useless* backtrace to let you know, and totally omits the *useful* information. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
On Wed, Oct 23, 2019 at 08:52:33AM -0500, Rob Herring wrote: > > I think this should have been done the other way around and default to > > coherent since most traditional OF platforms are coherent, and you > > can't just require those DTs to change. > > You can blame me. This was really only intended for cases where > coherency is configurable on a per peripheral basis and can't be > determined in other ways. > > The simple solution here is simply to use the compatible string for > the device to determine coherent or not. It really isn't that simple. There are two aspects to coherency, both of which must match: 1) The configuration of the device 2) The configuration of the kernel's DMA API (1) is controlled by the driver, which can make the decision any way it pleases. (2) on ARM64 is controlled depending on whether or not "dma-coherent" is specified in the device tree, since ARM64 can have a mixture of DMA coherent and non-coherent devices. A mismatch between (1) and (2) results in data corruption, potentially eating your filesystem. So, it's very important that the two match. These didn't match for the LX2160A, but, due to the way CMA was working, we sort of got away with it, but it was very dangerous as far as data safety went. Then, a change to CMA happened which moved where it was located, which caused a regression. Reverting the CMA changes didn't seem to be an option, so another solution had to be found. I started a discussion on how best to solve this: https://archive.armlinux.org.uk/lurker/thread/20190919.041320.1e53541f.en.html and the solution that the discussion came out with was the one that has been merged - which we now know caused a regression on PPC. Using compatible strings doesn't solve the issue: there is no way to tell the DMA API from the driver that the device is coherent. The only way to do that is via the "dma-coherent" property in DT on ARM64. To say that this is a mess is an under-statement, but we seem to have ended up here because of a series of piece-meal changes that don't seem to have been thought through enough. So, what's the right way to solve this, and ensure that the DMA API and device match as far as their coherency expectations go? Revert all the changes for sdhci-of-esdhc and CMA back to 5.0 or 5.1 state? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
On Wed, Oct 23, 2019 at 08:52:33AM -0500, Rob Herring wrote:
> On Wed, Oct 23, 2019 at 1:41 AM Benjamin Herrenschmidt
> wrote:
> >
> > On Wed, 2019-10-23 at 16:42 +1100, Michael Ellerman wrote:
> > >
> > > Right, it seems of_dma_is_coherent() has baked in the assumption that
> > > devices are non-coherent unless explicitly marked as coherent.
> > >
> > > Which is wrong on all or at least most existing powerpc systems
> > > according to Ben.
> >
> > This is probably broken on sparc(64) as well and whatever else uses
> > DT and is an intrinsicly coherent architecture (did we ever have
> > DT enabled x86s ? Wasn't OLPC such a beast ?).
>
> Only if those platforms use one of the 5 drivers that call this function:
>
> drivers/ata/ahci_qoriq.c: qoriq_priv->is_dmacoherent =
> of_dma_is_coherent(np);
> drivers/crypto/ccree/cc_driver.c: new_drvdata->coherent =
> of_dma_is_coherent(np);
> drivers/iommu/arm-smmu-v3.c:if (of_dma_is_coherent(dev->of_node))
> drivers/iommu/arm-smmu.c: if (of_dma_is_coherent(dev->of_node))
> drivers/mmc/host/sdhci-of-esdhc.c: if (of_dma_is_coherent(dev->of_node))
>
> Curious that a PPC specific driver (ahci_qoriq) calls it...
Maybe because it is not PPC specific:
arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi:
compatible = "fsl,lx2160a-ahci";
arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi:
compatible = "fsl,lx2160a-ahci";
arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi:
compatible = "fsl,lx2160a-ahci";
arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi:
compatible = "fsl,lx2160a-ahci";
drivers/ata/ahci_qoriq.c: { .compatible = "fsl,lx2160a-ahci", .data =
(void *)AHCI_LX2160A},
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH v2 3/3] arm: Add support for function error injection
I'm sorry, I can't apply this, it produces loads of:
include/linux/error-injection.h:7:10: fatal error: asm/error-injection.h: No
such file or directory
Since your patch 1 has been merged by the ARM64 people, I can't take
it until next cycle.
On Mon, Aug 19, 2019 at 05:18:08PM +0800, Leo Yan wrote:
> Hi Russell,
>
> On Tue, Aug 06, 2019 at 06:00:15PM +0800, Leo Yan wrote:
> > This patch implements arm specific functions regs_set_return_value() and
> > override_function_with_return() to support function error injection.
> >
> > In the exception flow, it updates pt_regs::ARM_pc with pt_regs::ARM_lr
> > so can override the probed function return.
>
> Gentle ping ... Could you review this patch?
>
> Thanks,
> Leo.
>
> > Signed-off-by: Leo Yan
> > ---
> > arch/arm/Kconfig | 1 +
> > arch/arm/include/asm/ptrace.h | 5 +
> > arch/arm/lib/Makefile | 2 ++
> > arch/arm/lib/error-inject.c | 19 +++
> > 4 files changed, 27 insertions(+)
> > create mode 100644 arch/arm/lib/error-inject.c
> >
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 33b00579beff..2d3d44a037f6 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -77,6 +77,7 @@ config ARM
> > select HAVE_EXIT_THREAD
> > select HAVE_FAST_GUP if ARM_LPAE
> > select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
> > + select HAVE_FUNCTION_ERROR_INJECTION if !THUMB2_KERNEL
> > select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL && !CC_IS_CLANG
> > select HAVE_FUNCTION_TRACER if !XIP_KERNEL
> > select HAVE_GCC_PLUGINS
> > diff --git a/arch/arm/include/asm/ptrace.h b/arch/arm/include/asm/ptrace.h
> > index 91d6b7856be4..3b41f37b361a 100644
> > --- a/arch/arm/include/asm/ptrace.h
> > +++ b/arch/arm/include/asm/ptrace.h
> > @@ -89,6 +89,11 @@ static inline long regs_return_value(struct pt_regs
> > *regs)
> > return regs->ARM_r0;
> > }
> >
> > +static inline void regs_set_return_value(struct pt_regs *regs, unsigned
> > long rc)
> > +{
> > + regs->ARM_r0 = rc;
> > +}
> > +
> > #define instruction_pointer(regs) (regs)->ARM_pc
> >
> > #ifdef CONFIG_THUMB2_KERNEL
> > diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
> > index b25c54585048..8f56484a7156 100644
> > --- a/arch/arm/lib/Makefile
> > +++ b/arch/arm/lib/Makefile
> > @@ -42,3 +42,5 @@ ifeq ($(CONFIG_KERNEL_MODE_NEON),y)
> >CFLAGS_xor-neon.o+= $(NEON_FLAGS)
> >obj-$(CONFIG_XOR_BLOCKS) += xor-neon.o
> > endif
> > +
> > +obj-$(CONFIG_FUNCTION_ERROR_INJECTION) += error-inject.o
> > diff --git a/arch/arm/lib/error-inject.c b/arch/arm/lib/error-inject.c
> > new file mode 100644
> > index ..2d696dc94893
> > --- /dev/null
> > +++ b/arch/arm/lib/error-inject.c
> > @@ -0,0 +1,19 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include
> > +#include
> > +
> > +void override_function_with_return(struct pt_regs *regs)
> > +{
> > + /*
> > +* 'regs' represents the state on entry of a predefined function in
> > +* the kernel/module and which is captured on a kprobe.
> > +*
> > +* 'regs->ARM_lr' contains the the link register for the probed
> > +* function, when kprobe returns back from exception it will override
> > +* the end of probed function and directly return to the predefined
> > +* function's caller.
> > +*/
> > + instruction_pointer_set(regs, regs->ARM_lr);
> > +}
> > +NOKPROBE_SYMBOL(override_function_with_return);
> > --
> > 2.17.1
> >
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH] dma-mapping: fix page attributes for dma_mmap_*
On Tue, Aug 06, 2019 at 05:08:54PM +0100, Will Deacon wrote: > On Sat, Aug 03, 2019 at 08:48:12AM +0200, Christoph Hellwig wrote: > > On Fri, Aug 02, 2019 at 11:38:03AM +0100, Will Deacon wrote: > > > > > > So this boils down to a terminology mismatch. The Arm architecture > > > doesn't have > > > anything called "write combine", so in Linux we instead provide what the > > > Arm > > > architecture calls "Normal non-cacheable" memory for > > > pgprot_writecombine(). > > > Amongst other things, this memory type permits speculation, unaligned > > > accesses > > > and merging of writes. I found something in the architecture spec about > > > non-cachable memory, but it's written in Armglish[1]. > > > > > > pgprot_noncached(), on the other hand, provides what the architecture > > > calls > > > Strongly Ordered or Device-nGnRnE memory. This is intended for mapping > > > MMIO > > > (i.e. PCI config space) and therefore forbids speculation, preserves > > > access > > > size, requires strict alignment and also forces write responses to come > > > from > > > the endpoint. > > > > > > I think the naming mismatch is historical, but on arm64 we wanted to use > > > the > > > same names as arm32 so that any drivers using these things directly would > > > get > > > the same behaviour. > > > > That all makes sense, but it totally needs a comment. I'll try to draft > > one based on this. I've also looked at the arm32 code a bit more, and > > it seems arm always (?) supported Normal non-cacheable attribute, but > > Linux only optionally uses it for arm v6+ because of fears of drivers > > missing barriers. > > I think it was also to do with aliasing, but I don't recall all of the > details. ARMv6+ is where the architecture significantly changed to introduce the idea of [Normal, Device, Strongly Ordered] where Normal has the cache attributes. Before that, we had just "uncached/unbuffered, uncached/buffered, cached/unbuffered, cached/buffered" modes. The write buffer (enabled by buffered modes) has no architected guarantees about how long writes will sit in it, and there is only the "drain write buffer" instruction to push writes out. Up to and including ARMv5, we took the easy approach of just using the "uncached/unbuffered" mode since that is (a) the safest, and (b) avoids write buffers that alias when there are multiple different mappings. We could have used a different approach, making all IO writes contain a "drain write buffer" instruction, and map DMA memory as "buffered", but as there were no Linux barriers defined to order memory accesses to DMA memory (so, for example, ring buffers can be updated in the correct order) back in those days, using the uncached/unbuffered mode was the sanest and most reliable solution. > > > The other really weird things is that in arm32 > > pgprot_dmacoherent incudes the L_PTE_XN bit, which from my understanding > > is the no-execture bit, but pgprot_writecombine does not. This seems to > > not very unintentional. So minus that the whole DMA_ATTR_WRITE_COMBІNE > > seems to be about flagging old arm specific drivers as having the proper > > barriers in places and otherwise is a no-op. > > I think it only matters for Armv7 CPUs, but yes, we should probably be > setting L_PTE_XN for both of these memory types. Conventionally, pgprot_writecombine() has only been used to change the memory type and not the permissions. Since writecombine memory is still capable of being executed, I don't see any reason to set XN for it. If the user wishes to mmap() using PROT_READ|PROT_EXEC, then is there really a reason for writecombine to set XN overriding the user? That said, pgprot_writecombine() is mostly used for framebuffers, which arguably shouldn't be executable anyway - but who'd want to mmap() the framebuffer with PROT_EXEC? > > > Here is my tentative plan: > > > > - respin this patch with a small fix to handle the > >DMA_ATTR_NON_CONSISTENT (as in ignore it unless actually supported), > >but keep the name as-is to avoid churn. This should allow 5.3 > >inclusion and backports > > - remove DMA_ATTR_WRITE_COMBINE support from mips, probably also 5.3 > >material. > > - move all architectures but arm over to just define > >pgprot_dmacoherent, including a comment with the above explanation > >for arm64. > > That would be great, thanks. > > > - make DMA_ATTR_WRITE_COMBINE a no-op and schedule it for removal, > >thus removing the last instances of arch_dma_mmap_pgprot > > All sounds good to me, although I suppose 32-bit Arm platforms without > CONFIG_ARM_DMA_MEM_BUFFERABLE may run into issues if DMA_ATTR_WRITE_COMBINE > disappears. Only one way to find out... Looking at the results of grep, I think only OMAP2+ and Exynos may be affected. However, removing writecombine support from the DMA API is going to have a huge impact for framebuffers on earlier ARMs - that's where we do expect framebuffers to be mapped "uncached/buffered" for
Re: [PATCH] dma-mapping: fix page attributes for dma_mmap_*
On Tue, Aug 06, 2019 at 05:45:03PM +0100, Russell King - ARM Linux admin wrote: > On Tue, Aug 06, 2019 at 05:08:54PM +0100, Will Deacon wrote: > > On Sat, Aug 03, 2019 at 08:48:12AM +0200, Christoph Hellwig wrote: > > > On Fri, Aug 02, 2019 at 11:38:03AM +0100, Will Deacon wrote: > > > > > > > > So this boils down to a terminology mismatch. The Arm architecture > > > > doesn't have > > > > anything called "write combine", so in Linux we instead provide what > > > > the Arm > > > > architecture calls "Normal non-cacheable" memory for > > > > pgprot_writecombine(). > > > > Amongst other things, this memory type permits speculation, unaligned > > > > accesses > > > > and merging of writes. I found something in the architecture spec about > > > > non-cachable memory, but it's written in Armglish[1]. > > > > > > > > pgprot_noncached(), on the other hand, provides what the architecture > > > > calls > > > > Strongly Ordered or Device-nGnRnE memory. This is intended for mapping > > > > MMIO > > > > (i.e. PCI config space) and therefore forbids speculation, preserves > > > > access > > > > size, requires strict alignment and also forces write responses to come > > > > from > > > > the endpoint. > > > > > > > > I think the naming mismatch is historical, but on arm64 we wanted to > > > > use the > > > > same names as arm32 so that any drivers using these things directly > > > > would get > > > > the same behaviour. > > > > > > That all makes sense, but it totally needs a comment. I'll try to draft > > > one based on this. I've also looked at the arm32 code a bit more, and > > > it seems arm always (?) supported Normal non-cacheable attribute, but > > > Linux only optionally uses it for arm v6+ because of fears of drivers > > > missing barriers. > > > > I think it was also to do with aliasing, but I don't recall all of the > > details. > > ARMv6+ is where the architecture significantly changed to introduce > the idea of [Normal, Device, Strongly Ordered] where Normal has the > cache attributes. > > Before that, we had just "uncached/unbuffered, uncached/buffered, > cached/unbuffered, cached/buffered" modes. > > The write buffer (enabled by buffered modes) has no architected > guarantees about how long writes will sit in it, and there is only > the "drain write buffer" instruction to push writes out. > > Up to and including ARMv5, we took the easy approach of just using > the "uncached/unbuffered" mode since that is (a) the safest, and (b) > avoids write buffers that alias when there are multiple different > mappings. > > We could have used a different approach, making all IO writes contain > a "drain write buffer" instruction, and map DMA memory as "buffered", > but as there were no Linux barriers defined to order memory accesses > to DMA memory (so, for example, ring buffers can be updated in the > correct order) back in those days, using the uncached/unbuffered mode > was the sanest and most reliable solution. > > > > > > The other really weird things is that in arm32 > > > pgprot_dmacoherent incudes the L_PTE_XN bit, which from my understanding > > > is the no-execture bit, but pgprot_writecombine does not. This seems to > > > not very unintentional. So minus that the whole DMA_ATTR_WRITE_COMBІNE > > > seems to be about flagging old arm specific drivers as having the proper > > > barriers in places and otherwise is a no-op. > > > > I think it only matters for Armv7 CPUs, but yes, we should probably be > > setting L_PTE_XN for both of these memory types. > > Conventionally, pgprot_writecombine() has only been used to change > the memory type and not the permissions. Since writecombine memory > is still capable of being executed, I don't see any reason to set XN > for it. > > If the user wishes to mmap() using PROT_READ|PROT_EXEC, then is there > really a reason for writecombine to set XN overriding the user? > > That said, pgprot_writecombine() is mostly used for framebuffers, which > arguably shouldn't be executable anyway - but who'd want to mmap() the > framebuffer with PROT_EXEC? > > > > > > Here is my tentative plan: > > > > > > - respin this patch with a small fix to handle the > > >DMA_ATTR_NON_CONSISTENT (as in ignore it unless actually supported), > > &g
Re: [PATCH v5 3/3] locking/rwsem: Optimize down_read_trylock()
On Fri, Mar 22, 2019 at 10:30:08AM -0400, Waiman Long wrote: > Modify __down_read_trylock() to optimize for an unlocked rwsem and make > it generate slightly better code. > > Before this patch, down_read_trylock: > >0x <+0>: callq 0x5 >0x0005 <+5>: jmp0x18 >0x0007 <+7>: lea0x1(%rdx),%rcx >0x000b <+11>:mov%rdx,%rax >0x000e <+14>:lock cmpxchg %rcx,(%rdi) >0x0013 <+19>:cmp%rax,%rdx >0x0016 <+22>:je 0x23 >0x0018 <+24>:mov(%rdi),%rdx >0x001b <+27>:test %rdx,%rdx >0x001e <+30>:jns0x7 >0x0020 <+32>:xor%eax,%eax >0x0022 <+34>:retq >0x0023 <+35>:mov%gs:0x0,%rax >0x002c <+44>:or $0x3,%rax >0x0030 <+48>:mov%rax,0x20(%rdi) >0x0034 <+52>:mov$0x1,%eax >0x0039 <+57>:retq > > After patch, down_read_trylock: > >0x <+0>: callq 0x5 >0x0005 <+5>: xor%eax,%eax >0x0007 <+7>: lea0x1(%rax),%rdx >0x000b <+11>: lock cmpxchg %rdx,(%rdi) >0x0010 <+16>: jne0x29 >0x0012 <+18>: mov%gs:0x0,%rax >0x001b <+27>: or $0x3,%rax >0x001f <+31>: mov%rax,0x20(%rdi) >0x0023 <+35>: mov$0x1,%eax >0x0028 <+40>: retq >0x0029 <+41>: test %rax,%rax >0x002c <+44>: jns0x7 >0x002e <+46>: xor%eax,%eax >0x0030 <+48>: retq > > By using a rwsem microbenchmark, the down_read_trylock() rate (with a > load of 10 to lengthen the lock critical section) on a x86-64 system > before and after the patch were: > > Before PatchAfter Patch ># of Threads rlock rlock > - - > 1 14,496 14,716 > 28,644 8,453 > 46,799 6,983 > 85,664 7,190 > > On a ARM64 system, the performance results were: > > Before PatchAfter Patch ># of Threads rlock rlock > - - > 1 23,676 24,488 > 27,697 9,502 > 44,945 3,440 > 82,641 1,603 > > For the uncontended case (1 thread), the new down_read_trylock() is a > little bit faster. For the contended cases, the new down_read_trylock() > perform pretty well in x86-64, but performance degrades at high > contention level on ARM64. So, 70% for 4 threads, 61% for 4 threads - does this trend continue tailing off as the number of threads (and cores) increase? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH v2 07/29] ARM: add kexec_file_load system call number
On Fri, Jan 25, 2019 at 03:43:59PM +, Catalin Marinas wrote: > On Fri, Jan 18, 2019 at 05:18:13PM +0100, Arnd Bergmann wrote: > > diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl > > index 86de9eb34296..20ed7e026723 100644 > > --- a/arch/arm/tools/syscall.tbl > > +++ b/arch/arm/tools/syscall.tbl > > @@ -415,3 +415,4 @@ > > 398common rseqsys_rseq > > 399common io_pgetevents sys_io_pgetevents > > 400common migrate_pages sys_migrate_pages > > +401common kexec_file_load sys_kexec_file_load > > I presume on arm32 this would still return -ENOSYS. Yes, I already checked. If CONFIG_KEXEC_FILE is not set, then kernel/kexec_file.c is not built, and we fall back to the stub in kernel/sys_ni.c. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH v2 29/29] y2038: add 64-bit time_t syscalls to all 32-bit architectures
On Fri, Jan 18, 2019 at 11:53:25AM -0800, Andy Lutomirski wrote: > On Fri, Jan 18, 2019 at 11:33 AM Arnd Bergmann wrote: > > > > On Fri, Jan 18, 2019 at 7:50 PM Andy Lutomirski wrote: > > > On Fri, Jan 18, 2019 at 8:25 AM Arnd Bergmann wrote: > > > > - Once we get to 512, we clash with the x32 numbers (unless > > > > we remove x32 support first), and probably have to skip > > > > a few more. I also considered using the 512..547 space > > > > for 32-bit-only calls (which never clash with x32), but > > > > that also seems to add a bit of complexity. > > > > > > I have a patch that I'll send soon to make x32 use its own table. As > > > far as I'm concerned, 547 is *it*. 548 is just a normal number and is > > > not special. But let's please not reuse 512..547 for other purposes > > > on x86 variants -- that way lies even more confusion, IMO. > > > > Fair enough, the space for those numbers is cheap enough here. > > I take it you mean we also should not reuse that number space if > > we were to decide to remove x32 soon, but you are not worried > > about clashing with arch/alpha when everything else uses consistent > > numbers? > > > > I think we have two issues if we reuse those numbers for new syscalls. > First, I'd really like to see new syscalls be numbered consistently > everywhere, or at least on all x86 variants, and we can't on x32 > because they mean something else. Perhaps more importantly, due to > what is arguably a rather severe bug, issuing a native x86_64 syscall > (x32 bit clear) with nr in the range 512..547 does *not* return > -ENOSYS on a kernel with x32 enabled. Instead it does something that > is somewhat arbitrary. With my patch applied, it will return -ENOSYS, > but old kernels will still exist, and this will break syscall probing. > > Can we perhaps just start the consistent numbers above 547 or maybe > block out 512..547 in the new regime? I don't think you gain much with that kind of scheme - it won't take very long before an architecture misses having a syscall added, and then someone else adds their own. Been there with ARM - I was keeping the syscall table in the same order as x86 for new syscalls, but now that others have been adding syscalls to the table since I converted ARM to the tabular form, that's now gone out the window. So, I think it's completely pointless to do what you're suggesting. We'll just end up with a big hole in the middle of the syscall table and then revert back to random numbering of syscalls thereafter again. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH 15/15] arch: add pkey and rseq syscall numbers everywhere
On Thu, Jan 10, 2019 at 05:24:35PM +0100, Arnd Bergmann wrote: > Most architectures define system call numbers for the rseq and pkey system > calls, even when they don't support the features, and perhaps never will. > > Only a few architectures are missing these, so just define them anyway > for consistency. If we decide to add them later to one of these, the > system call numbers won't get out of sync then. I was lambasted for adding the pkey syscalls for 32-bit ARM in 2016, which will probably never support it. Why has the attitude towards this kind of thing now apparently become acceptable? > Signed-off-by: Arnd Bergmann > --- > arch/alpha/include/asm/unistd.h | 4 > arch/alpha/kernel/syscalls/syscall.tbl | 4 > arch/ia64/kernel/syscalls/syscall.tbl | 4 > arch/m68k/kernel/syscalls/syscall.tbl | 4 > arch/parisc/include/asm/unistd.h| 3 --- > arch/parisc/kernel/syscalls/syscall.tbl | 4 > arch/s390/include/asm/unistd.h | 3 --- > arch/s390/kernel/syscalls/syscall.tbl | 3 +++ > arch/sh/kernel/syscalls/syscall.tbl | 4 > arch/sparc/include/asm/unistd.h | 5 - > arch/sparc/kernel/syscalls/syscall.tbl | 4 > arch/xtensa/kernel/syscalls/syscall.tbl | 1 + > 12 files changed, 28 insertions(+), 15 deletions(-) > > diff --git a/arch/alpha/include/asm/unistd.h b/arch/alpha/include/asm/unistd.h > index 564ba87bdc38..31ad350b58a0 100644 > --- a/arch/alpha/include/asm/unistd.h > +++ b/arch/alpha/include/asm/unistd.h > @@ -29,9 +29,5 @@ > #define __IGNORE_getppid > #define __IGNORE_getuid > > -/* Alpha doesn't have protection keys. */ > -#define __IGNORE_pkey_mprotect > -#define __IGNORE_pkey_alloc > -#define __IGNORE_pkey_free > > #endif /* _ALPHA_UNISTD_H */ > diff --git a/arch/alpha/kernel/syscalls/syscall.tbl > b/arch/alpha/kernel/syscalls/syscall.tbl > index b0e247287908..25b4a7e76943 100644 > --- a/arch/alpha/kernel/syscalls/syscall.tbl > +++ b/arch/alpha/kernel/syscalls/syscall.tbl > @@ -452,3 +452,7 @@ > 521 common pwritev2sys_pwritev2 > 522 common statx sys_statx > 523 common io_pgetevents sys_io_pgetevents > +524 common pkey_alloc sys_pkey_alloc > +525 common pkey_free sys_pkey_free > +526 common pkey_mprotect sys_pkey_mprotect > +527 common rseqsys_rseq > diff --git a/arch/ia64/kernel/syscalls/syscall.tbl > b/arch/ia64/kernel/syscalls/syscall.tbl > index 2e93dbdcdb80..84e03de00177 100644 > --- a/arch/ia64/kernel/syscalls/syscall.tbl > +++ b/arch/ia64/kernel/syscalls/syscall.tbl > @@ -339,3 +339,7 @@ > 327 common io_pgetevents sys_io_pgetevents > 328 common perf_event_open sys_perf_event_open > 329 common seccomp sys_seccomp > +330 common pkey_alloc sys_pkey_alloc > +331 common pkey_free sys_pkey_free > +332 common pkey_mprotect sys_pkey_mprotect > +333 common rseqsys_rseq > diff --git a/arch/m68k/kernel/syscalls/syscall.tbl > b/arch/m68k/kernel/syscalls/syscall.tbl > index 5354ba02eed2..ae88b85d068e 100644 > --- a/arch/m68k/kernel/syscalls/syscall.tbl > +++ b/arch/m68k/kernel/syscalls/syscall.tbl > @@ -388,6 +388,10 @@ > 378 common pwritev2sys_pwritev2 > 379 common statx sys_statx > 380 common seccomp sys_seccomp > +381 common pkey_alloc sys_pkey_alloc > +382 common pkey_free sys_pkey_free > +383 common pkey_mprotect sys_pkey_mprotect > +384 common rseqsys_rseq > # room for arch specific calls > 393 common semget sys_semget > 394 common semctl sys_semctl > diff --git a/arch/parisc/include/asm/unistd.h > b/arch/parisc/include/asm/unistd.h > index c2c2afb28941..9ec1026af877 100644 > --- a/arch/parisc/include/asm/unistd.h > +++ b/arch/parisc/include/asm/unistd.h > @@ -12,9 +12,6 @@ > > #define __IGNORE_select /* newselect */ > #define __IGNORE_fadvise64 /* fadvise64_64 */ > -#define __IGNORE_pkey_mprotect > -#define __IGNORE_pkey_alloc > -#define __IGNORE_pkey_free > > #ifndef ASM_LINE_SEP > # define ASM_LINE_SEP ; > diff --git a/arch/parisc/kernel/syscalls/syscall.tbl > b/arch/parisc/kernel/syscalls/syscall.tbl > index 9bbd2f9f56c8..e07231de3597 100644 > --- a/arch/parisc/kernel/syscalls/syscall.tbl > +++ b/arch/parisc/kernel/syscalls/syscall.tbl > @@ -367,3 +367,7 @@ > 348 common pwritev2sys_pwritev2 > compat_sys_pwritev2 > 349 common statx sys_statx > 350 common io_pgetevents sys_io_pgetevents >
