* Re: [PATCH v7 3/7] of/platform: Add functional dependency link from DT bindings
From: Saravana Kannan @ 2019-08-19 20:49 UTC (permalink / raw)
To: Frank Rowand
Cc: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Jonathan Corbet,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
David Collins, Android Kernel Team, Linux Doc Mailing List
In-Reply-To: <19c99a6e-51c3-68d7-d1d6-640aae754c14@gmail.com>
On Mon, Aug 19, 2019 at 10:16 AM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 8/15/19 6:50 PM, Saravana Kannan wrote:
> > On Wed, Aug 7, 2019 at 7:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
> >>
> >> On 7/23/19 5:10 PM, Saravana Kannan wrote:
> >>> Add device-links after the devices are created (but before they are
> >>> probed) by looking at common DT bindings like clocks and
> >>> interconnects.
>
>
> < very big snip (lots of comments that deserve answers) >
>
>
> >>
> >> /**
> >> * of_link_property - TODO:
> >> * dev:
> >> * con_np:
> >> * prop:
> >> *
> >> * TODO...
> >> *
> >> * Any failed attempt to create a link will NOT result in an immediate return.
> >> * of_link_property() must create all possible links even when one of more
> >> * attempts to create a link fail.
> >>
> >> Why? isn't one failure enough to prevent probing this device?
> >> Continuing to scan just results in extra work... which will be
> >> repeated every time device_link_check_waiting_consumers() is called
> >
> > Context:
> > As I said in the cover letter, avoiding unnecessary probes is just one
> > of the reasons for this patch. The other (arguably more important)
>
> Agree that it is more important.
>
>
> > reason for this patch is to make sure suppliers know that they have
> > consumers that are yet to be probed. That way, suppliers can leave
> > their resource on AND in the right state if they were left on by the
> > bootloader. For example, if a clock was left on and at 200 MHz, the
> > clock provider needs to keep that clock ON and at 200 MHz till all the
> > consumers are probed.
> >
> > Answer: Let's say a consumer device Z has suppliers A, B and C. If the
> > linking fails at A and you return immediately, then B and C could
> > probe and then figure that they have no more consumers (they don't see
> > a link to Z) and turn off their resources. And Z could fail
> > catastrophically.
>
> Then I think that this approach is fatally flawed in the current implementation.
I'm waiting to hear how it is fatally flawed. But maybe this is just a
misunderstanding of the problem?
In the text below, I'm not sure if you mixing up two different things
or just that your wording it a bit ambiguous. So pardon my nitpick to
err on the side of clarity.
> A device can be added by a module that is loaded.
No, in the example I gave, of_platform_default_populate_init() would
add all 3 of those devices during arch_initcall_sync().
> In that case the device
> was not present at late boot when the suppliers may turn off their resources.
In that case, the _drivers_ for those devices aren't present at late
boot. So that they can't request to keep the resources on for their
consumer devices. Since there are no consumer requests on resources,
the suppliers turn off their resources at late boot (since there isn't
a better location as of today). The sync_state() call back added in a
subsequent patche in this series will provide the better location.
> (I am assuming the details since I have not reviewed the patches later in
> the series that implement this part.)
>
> Am I missing something?
I think you are mixing up devices getting added/populated with drivers
getting loaded as modules?
> If I am wrong, then I'll have more comments for your review replies for
> patches 2 and 3.
I'll wait for more review replies?
Thanks,
Saravana
^ permalink raw reply
* Re: [PATCH v7 3/7] of/platform: Add functional dependency link from DT bindings
From: Frank Rowand @ 2019-08-19 21:30 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Jonathan Corbet,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
David Collins, Android Kernel Team, Linux Doc Mailing List
In-Reply-To: <CAGETcx-XcXZq7YFHsFdzBDniQku9cxFUJL_vBoEKKhCH+cDKRw@mail.gmail.com>
On 8/19/19 1:49 PM, Saravana Kannan wrote:
> On Mon, Aug 19, 2019 at 10:16 AM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 8/15/19 6:50 PM, Saravana Kannan wrote:
>>> On Wed, Aug 7, 2019 at 7:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>
>>>> On 7/23/19 5:10 PM, Saravana Kannan wrote:
>>>>> Add device-links after the devices are created (but before they are
>>>>> probed) by looking at common DT bindings like clocks and
>>>>> interconnects.
>>
>>
>> < very big snip (lots of comments that deserve answers) >
>>
>>
>>>>
>>>> /**
>>>> * of_link_property - TODO:
>>>> * dev:
>>>> * con_np:
>>>> * prop:
>>>> *
>>>> * TODO...
>>>> *
>>>> * Any failed attempt to create a link will NOT result in an immediate return.
>>>> * of_link_property() must create all possible links even when one of more
>>>> * attempts to create a link fail.
>>>>
>>>> Why? isn't one failure enough to prevent probing this device?
>>>> Continuing to scan just results in extra work... which will be
>>>> repeated every time device_link_check_waiting_consumers() is called
>>>
>>> Context:
>>> As I said in the cover letter, avoiding unnecessary probes is just one
>>> of the reasons for this patch. The other (arguably more important)
>>
>> Agree that it is more important.
>>
>>
>>> reason for this patch is to make sure suppliers know that they have
>>> consumers that are yet to be probed. That way, suppliers can leave
>>> their resource on AND in the right state if they were left on by the
>>> bootloader. For example, if a clock was left on and at 200 MHz, the
>>> clock provider needs to keep that clock ON and at 200 MHz till all the
>>> consumers are probed.
>>>
>>> Answer: Let's say a consumer device Z has suppliers A, B and C. If the
>>> linking fails at A and you return immediately, then B and C could
>>> probe and then figure that they have no more consumers (they don't see
>>> a link to Z) and turn off their resources. And Z could fail
>>> catastrophically.
>>
>> Then I think that this approach is fatally flawed in the current implementation.
>
> I'm waiting to hear how it is fatally flawed. But maybe this is just a
> misunderstanding of the problem?
Fatally flawed because it does not handle modules that add a consumer
device when the module is loaded.
>
> In the text below, I'm not sure if you mixing up two different things
> or just that your wording it a bit ambiguous. So pardon my nitpick to
> err on the side of clarity.
Please do nitpick. Clarity is good.
>
>> A device can be added by a module that is loaded.
>
> No, in the example I gave, of_platform_default_populate_init() would
> add all 3 of those devices during arch_initcall_sync().
The example you gave does not cover all use cases.
There are modules that add devices when the module is loaded. You can not
ignore systems using such modules.
>
>> In that case the device
>> was not present at late boot when the suppliers may turn off their resources.
>
> In that case, the _drivers_ for those devices aren't present at late
> boot. So that they can't request to keep the resources on for their
> consumer devices. Since there are no consumer requests on resources,
> the suppliers turn off their resources at late boot (since there isn't
> a better location as of today). The sync_state() call back added in a
> subsequent patche in this series will provide the better location.
And the sync_state() call back will not deal with modules that add consumer
devices when the module is loaded, correct?
>
>> (I am assuming the details since I have not reviewed the patches later in
>> the series that implement this part.)
>>
>> Am I missing something?
>
> I think you are mixing up devices getting added/populated with drivers
> getting loaded as modules?
Only some modules add devices when they are loaded. But these modules do
exist.
-Frank
>
>> If I am wrong, then I'll have more comments for your review replies for
>> patches 2 and 3.
>
> I'll wait for more review replies?
>
> Thanks,
> Saravana
>
^ permalink raw reply
* Re: [PATCH v5 1/6] mm/page_idle: Add per-pid idle page tracking using virtual index
From: Joel Fernandes @ 2019-08-19 21:52 UTC (permalink / raw)
To: Michal Hocko
Cc: Jann Horn, Daniel Gruss, kernel list, Alexey Dobriyan,
Andrew Morton, Borislav Petkov, Brendan Gregg, Catalin Marinas,
Christian Hansen, Daniel Colascione, fmayer, H. Peter Anvin,
Ingo Molnar, Jonathan Corbet, Kees Cook, kernel-team, Linux API,
linux-doc, linux-fsdevel, Linux-MM, Mike Rapoport, Minchan Kim,
namhyung, Paul E. McKenney, Robin Murphy, Roman Gushchin,
Thomas Gleixner, Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190814075601.GO17933@dhcp22.suse.cz>
On Wed, Aug 14, 2019 at 09:56:01AM +0200, Michal Hocko wrote:
[snip]
> > > > Can this be used to observe which library pages other processes are
> > > > accessing, even if you don't have access to those processes, as long
> > > > as you can map the same libraries? I realize that there are already a
> > > > bunch of ways to do that with side channels and such; but if you're
> > > > adding an interface that allows this by design, it seems to me like
> > > > something that should be gated behind some sort of privilege check.
> > >
> > > Hmm, you need to be priviledged to get the pfn now and without that you
> > > cannot get to any page so the new interface is weakening the rules.
> > > Maybe we should limit setting the idle state to processes with the write
> > > status. Or do you think that even observing idle status is useful for
> > > practical side channel attacks? If yes, is that a problem of the
> > > profiler which does potentially dangerous things?
> >
> > I suppose read-only access isn't a real problem as long as the
> > profiler isn't writing the idle state in a very tight loop... but I
> > don't see a usecase where you'd actually want that? As far as I can
> > tell, if you can't write the idle state, being able to read it is
> > pretty much useless.
> >
> > If the profiler only wants to profile process-private memory, then
> > that should be implementable in a safe way in principle, I think, but
> > since Joel said that they want to profile CoW memory as well, I think
> > that's inherently somewhat dangerous.
>
> I cannot really say how useful that would be but I can see that
> implementing ownership checks would be really non-trivial for
> shared pages. Reducing the interface to exclusive pages would make it
> easier as you noted but less helpful.
>
> Besides that the attack vector shouldn't be really much different from
> the page cache access, right? So essentially can_do_mincore model.
>
> I guess we want to document that page idle tracking should be used with
> care because it potentially opens a side channel opportunity if used
> on sensitive data.
I have been thinking of this, and discussing with our heap profiler folks.
Not being able to track shared pages would be a limitation, but I don't see
any way forward considering this security concern so maybe we have to
limit what we can do.
I will look into implementing this without doing the rmap but still make it
work on shared pages from the point of view of the process being tracked. It
just would no longer through the PTEs of *other* processes sharing the page.
My current thought is to just rely on the PTE accessed bit, and not use the
PageIdle flag at all. But we'd still set the PageYoung flag so that the
reclaim code still sees the page as accessed. The reason I feel like avoiding
the PageIdle flag is:
1. It looks like mark_page_accessed() can be called from other paths which
can also result in some kind of side-channel issue if a page was shared.
2. I don't think I need the PageIdle flag since the access bit alone should
let me know, although it could be a bit slower. Since previously, I did not
need to check every PTE and if the PageIdle flag was already cleared, then
the page was declared as idle.
At least this series resulted in a bug fix and a tonne of learning, so thank
you everyone!
Any other thoughts?
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v5 00/17] Use MFD framework for SGI IOC3 drivers
From: Jakub Kicinski @ 2019-08-19 23:51 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Jonathan Corbet, Ralf Baechle, Paul Burton, James Hogan,
Dmitry Torokhov, Lee Jones, David S. Miller, Srinivas Kandagatla,
Alessandro Zummo, Alexandre Belloni, Greg Kroah-Hartman,
Jiri Slaby, Evgeniy Polyakov, linux-doc, linux-kernel, linux-mips,
linux-input, netdev, linux-rtc, linux-serial
In-Reply-To: <20190819163144.3478-1-tbogendoerfer@suse.de>
On Mon, 19 Aug 2019 18:31:23 +0200, Thomas Bogendoerfer wrote:
> - requested by Jakub I've splitted ioc3 ethernet driver changes into
> more steps to make the transition more visible;
Thanks a lot for doing that!
^ permalink raw reply
* Re: [PATCH v5 09/17] net: sgi: ioc3-eth: use defines for constants dealing with desc rings
From: Jakub Kicinski @ 2019-08-19 23:53 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Jonathan Corbet, Ralf Baechle, Paul Burton, James Hogan,
Dmitry Torokhov, Lee Jones, David S. Miller, Srinivas Kandagatla,
Alessandro Zummo, Alexandre Belloni, Greg Kroah-Hartman,
Jiri Slaby, Evgeniy Polyakov, linux-doc, linux-kernel, linux-mips,
linux-input, netdev, linux-rtc, linux-serial
In-Reply-To: <20190819163144.3478-10-tbogendoerfer@suse.de>
On Mon, 19 Aug 2019 18:31:32 +0200, Thomas Bogendoerfer wrote:
> Descriptor ring sizes of the IOC3 are more or less fixed size. To
> make clearer where there is a relation to ring sizes use defines.
>
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
^ permalink raw reply
* Re: [PATCH v5 10/17] net: sgi: ioc3-eth: rework skb rx handling
From: Jakub Kicinski @ 2019-08-19 23:55 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Jonathan Corbet, Ralf Baechle, Paul Burton, James Hogan,
Dmitry Torokhov, Lee Jones, David S. Miller, Srinivas Kandagatla,
Alessandro Zummo, Alexandre Belloni, Greg Kroah-Hartman,
Jiri Slaby, Evgeniy Polyakov, linux-doc, linux-kernel, linux-mips,
linux-input, netdev, linux-rtc, linux-serial
In-Reply-To: <20190819163144.3478-11-tbogendoerfer@suse.de>
On Mon, 19 Aug 2019 18:31:33 +0200, Thomas Bogendoerfer wrote:
> Buffers alloacted by alloc_skb() are already cache aligned so there
> is no need for an extra align done by ioc3_alloc_skb. And instead
> of skb_put/skb_trim simply use one skb_put after frame size is known
> during receive.
>
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> ---
> drivers/net/ethernet/sgi/ioc3-eth.c | 50 ++++++++-----------------------------
> 1 file changed, 11 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/net/ethernet/sgi/ioc3-eth.c b/drivers/net/ethernet/sgi/ioc3-eth.c
> index c875640926d6..d862f28887f9 100644
> --- a/drivers/net/ethernet/sgi/ioc3-eth.c
> +++ b/drivers/net/ethernet/sgi/ioc3-eth.c
> @@ -11,7 +11,6 @@
> *
> * To do:
> *
> - * o Handle allocation failures in ioc3_alloc_skb() more gracefully.
> * o Handle allocation failures in ioc3_init_rings().
> * o Use prefetching for large packets. What is a good lower limit for
> * prefetching?
> @@ -72,6 +71,12 @@
> #define TX_RING_ENTRIES 128
> #define TX_RING_MASK (TX_RING_ENTRIES - 1)
>
> +/* BEWARE: The IOC3 documentation documents the size of rx buffers as
> + * 1644 while it's actually 1664. This one was nasty to track down...
> + */
> +#define RX_OFFSET 10
> +#define RX_BUF_SIZE 1664
> +
> #define ETCSR_FD ((17 << ETCSR_IPGR2_SHIFT) | (11 << ETCSR_IPGR1_SHIFT) | 21)
> #define ETCSR_HD ((21 << ETCSR_IPGR2_SHIFT) | (21 << ETCSR_IPGR1_SHIFT) | 21)
>
> @@ -111,31 +116,6 @@ static void ioc3_init(struct net_device *dev);
> static const char ioc3_str[] = "IOC3 Ethernet";
> static const struct ethtool_ops ioc3_ethtool_ops;
>
> -/* We use this to acquire receive skb's that we can DMA directly into. */
> -
> -#define IOC3_CACHELINE 128UL
Is the cache line on the platform this driver works on 128B?
This looks like a DMA engine alignment requirement, more than an
optimization.
The comment in __alloc_skb() says:
/* We do our best to align skb_shared_info on a separate cache
* line. It usually works because kmalloc(X > SMP_CACHE_BYTES) gives
* aligned memory blocks, unless SLUB/SLAB debug is enabled.
* Both skb->head and skb_shared_info are cache line aligned.
*/
note the "unless".
> -static inline unsigned long aligned_rx_skb_addr(unsigned long addr)
> -{
> - return (~addr + 1) & (IOC3_CACHELINE - 1UL);
> -}
> -
> -static inline struct sk_buff *ioc3_alloc_skb(unsigned long length,
> - unsigned int gfp_mask)
> -{
> - struct sk_buff *skb;
> -
> - skb = alloc_skb(length + IOC3_CACHELINE - 1, gfp_mask);
> - if (likely(skb)) {
> - int offset = aligned_rx_skb_addr((unsigned long)skb->data);
> -
> - if (offset)
> - skb_reserve(skb, offset);
> - }
> -
> - return skb;
> -}
> -
> static inline unsigned long ioc3_map(void *ptr, unsigned long vdev)
> {
> #ifdef CONFIG_SGI_IP27
> @@ -148,12 +128,6 @@ static inline unsigned long ioc3_map(void *ptr, unsigned long vdev)
> #endif
> }
>
> -/* BEWARE: The IOC3 documentation documents the size of rx buffers as
> - * 1644 while it's actually 1664. This one was nasty to track down ...
> - */
> -#define RX_OFFSET 10
> -#define RX_BUF_ALLOC_SIZE (1664 + RX_OFFSET + IOC3_CACHELINE)
> -
> #define IOC3_SIZE 0x100000
>
> static inline u32 mcr_pack(u32 pulse, u32 sample)
> @@ -534,10 +508,10 @@ static inline void ioc3_rx(struct net_device *dev)
> err = be32_to_cpu(rxb->err); /* It's valid ... */
> if (err & ERXBUF_GOODPKT) {
> len = ((w0 >> ERXBUF_BYTECNT_SHIFT) & 0x7ff) - 4;
> - skb_trim(skb, len);
> + skb_put(skb, len);
> skb->protocol = eth_type_trans(skb, dev);
>
> - new_skb = ioc3_alloc_skb(RX_BUF_ALLOC_SIZE, GFP_ATOMIC);
> + new_skb = alloc_skb(RX_BUF_SIZE, GFP_ATOMIC);
> if (!new_skb) {
> /* Ouch, drop packet and just recycle packet
> * to keep the ring filled.
> @@ -546,6 +520,7 @@ static inline void ioc3_rx(struct net_device *dev)
> new_skb = skb;
> goto next;
> }
> + new_skb->dev = dev;
Assigning dev pointer seems unrelated to the rest of the patch?
> if (likely(dev->features & NETIF_F_RXCSUM))
> ioc3_tcpudp_checksum(skb,
> @@ -556,8 +531,6 @@ static inline void ioc3_rx(struct net_device *dev)
>
> ip->rx_skbs[rx_entry] = NULL; /* Poison */
>
> - /* Because we reserve afterwards. */
> - skb_put(new_skb, (1664 + RX_OFFSET));
> rxb = (struct ioc3_erxbuf *)new_skb->data;
> skb_reserve(new_skb, RX_OFFSET);
>
> @@ -846,16 +819,15 @@ static void ioc3_alloc_rings(struct net_device *dev)
> for (i = 0; i < RX_BUFFS; i++) {
> struct sk_buff *skb;
>
> - skb = ioc3_alloc_skb(RX_BUF_ALLOC_SIZE, GFP_ATOMIC);
> + skb = alloc_skb(RX_BUF_SIZE, GFP_ATOMIC);
> if (!skb) {
> show_free_areas(0, NULL);
> continue;
> }
> + skb->dev = dev;
>
> ip->rx_skbs[i] = skb;
>
> - /* Because we reserve afterwards. */
> - skb_put(skb, (1664 + RX_OFFSET));
> rxb = (struct ioc3_erxbuf *)skb->data;
> rxr[i] = cpu_to_be64(ioc3_map(rxb, 1));
> skb_reserve(skb, RX_OFFSET);
^ permalink raw reply
* Re: [PATCH v5 11/17] net: sgi: ioc3-eth: no need to stop queue set_multicast_list
From: Jakub Kicinski @ 2019-08-20 0:04 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Jonathan Corbet, Ralf Baechle, Paul Burton, James Hogan,
Dmitry Torokhov, Lee Jones, David S. Miller, Srinivas Kandagatla,
Alessandro Zummo, Alexandre Belloni, Greg Kroah-Hartman,
Jiri Slaby, Evgeniy Polyakov, linux-doc, linux-kernel, linux-mips,
linux-input, netdev, linux-rtc, linux-serial
In-Reply-To: <20190819163144.3478-12-tbogendoerfer@suse.de>
On Mon, 19 Aug 2019 18:31:34 +0200, Thomas Bogendoerfer wrote:
> netif_stop_queue()/netif_wake_qeue() aren't needed for changing
> multicast filters. Use spinlocks instead for proper protection
> of private struct.
>
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> ---
> drivers/net/ethernet/sgi/ioc3-eth.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/sgi/ioc3-eth.c b/drivers/net/ethernet/sgi/ioc3-eth.c
> index d862f28887f9..7f85a3bfef14 100644
> --- a/drivers/net/ethernet/sgi/ioc3-eth.c
> +++ b/drivers/net/ethernet/sgi/ioc3-eth.c
> @@ -1542,8 +1542,7 @@ static void ioc3_set_multicast_list(struct net_device *dev)
> struct netdev_hw_addr *ha;
> u64 ehar = 0;
>
> - netif_stop_queue(dev); /* Lock out others. */
> -
> + spin_lock_irq(&ip->ioc3_lock);
What does this lock protect? 🤔 No question that stopping TX queues
makes little sense, but this function is only called from
ndo_set_rx_mode(), so with rtnl_lock held.
I thought it may protect ip->emcr, but that one is accessed with no
locking from the ioc3_timer() -> ioc3_setup_duplex() path..
> if (dev->flags & IFF_PROMISC) { /* Set promiscuous. */
> ip->emcr |= EMCR_PROMISC;
> writel(ip->emcr, ®s->emcr);
> @@ -1572,7 +1571,7 @@ static void ioc3_set_multicast_list(struct net_device *dev)
> writel(ip->ehar_l, ®s->ehar_l);
> }
>
> - netif_wake_queue(dev); /* Let us get going again. */
> + spin_unlock_irq(&ip->ioc3_lock);
> }
>
> module_pci_driver(ioc3_driver);
^ permalink raw reply
* Re: [PATCH v5 12/17] net: sgi: ioc3-eth: use dma-direct for dma allocations
From: Jakub Kicinski @ 2019-08-20 0:07 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Jonathan Corbet, Ralf Baechle, Paul Burton, James Hogan,
Dmitry Torokhov, Lee Jones, David S. Miller, Srinivas Kandagatla,
Alessandro Zummo, Alexandre Belloni, Greg Kroah-Hartman,
Jiri Slaby, Evgeniy Polyakov, linux-doc, linux-kernel, linux-mips,
linux-input, netdev, linux-rtc, linux-serial
In-Reply-To: <20190819163144.3478-13-tbogendoerfer@suse.de>
On Mon, 19 Aug 2019 18:31:35 +0200, Thomas Bogendoerfer wrote:
> @@ -1386,18 +1427,24 @@ static netdev_tx_t ioc3_start_xmit(struct sk_buff *skb, struct net_device *dev)
> unsigned long b2 = (data | 0x3fffUL) + 1UL;
> unsigned long s1 = b2 - data;
> unsigned long s2 = data + len - b2;
> + dma_addr_t d;
>
> desc->cmd = cpu_to_be32(len | ETXD_INTWHENDONE |
> ETXD_B1V | ETXD_B2V | w0);
> desc->bufcnt = cpu_to_be32((s1 << ETXD_B1CNT_SHIFT) |
> (s2 << ETXD_B2CNT_SHIFT));
> - desc->p1 = cpu_to_be64(ioc3_map(skb->data, 1));
> - desc->p2 = cpu_to_be64(ioc3_map((void *)b2, 1));
> + d = dma_map_single(ip->dma_dev, skb->data, s1, DMA_TO_DEVICE);
You'll need to check the DMA address with dma_mapping_error(dev, addr),
otherwise static checkers will get upset.
> + desc->p1 = cpu_to_be64(ioc3_map(d, PCI64_ATTR_PREF));
> + d = dma_map_single(ip->dma_dev, (void *)b2, s1, DMA_TO_DEVICE);
> + desc->p2 = cpu_to_be64(ioc3_map(d, PCI64_ATTR_PREF));
^ permalink raw reply
* Re: [PATCH v7 3/7] of/platform: Add functional dependency link from DT bindings
From: Saravana Kannan @ 2019-08-20 0:09 UTC (permalink / raw)
To: Frank Rowand
Cc: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Jonathan Corbet,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
David Collins, Android Kernel Team, Linux Doc Mailing List
In-Reply-To: <74931824-f8a1-0435-e00a-5b5cdbe8a8a2@gmail.com>
On Mon, Aug 19, 2019 at 2:30 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 8/19/19 1:49 PM, Saravana Kannan wrote:
> > On Mon, Aug 19, 2019 at 10:16 AM Frank Rowand <frowand.list@gmail.com> wrote:
> >>
> >> On 8/15/19 6:50 PM, Saravana Kannan wrote:
> >>> On Wed, Aug 7, 2019 at 7:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
> >>>>
> >>>> On 7/23/19 5:10 PM, Saravana Kannan wrote:
> >>>>> Add device-links after the devices are created (but before they are
> >>>>> probed) by looking at common DT bindings like clocks and
> >>>>> interconnects.
> >>
> >>
> >> < very big snip (lots of comments that deserve answers) >
> >>
> >>
> >>>>
> >>>> /**
> >>>> * of_link_property - TODO:
> >>>> * dev:
> >>>> * con_np:
> >>>> * prop:
> >>>> *
> >>>> * TODO...
> >>>> *
> >>>> * Any failed attempt to create a link will NOT result in an immediate return.
> >>>> * of_link_property() must create all possible links even when one of more
> >>>> * attempts to create a link fail.
> >>>>
> >>>> Why? isn't one failure enough to prevent probing this device?
> >>>> Continuing to scan just results in extra work... which will be
> >>>> repeated every time device_link_check_waiting_consumers() is called
> >>>
> >>> Context:
> >>> As I said in the cover letter, avoiding unnecessary probes is just one
> >>> of the reasons for this patch. The other (arguably more important)
> >>
> >> Agree that it is more important.
> >>
> >>
> >>> reason for this patch is to make sure suppliers know that they have
> >>> consumers that are yet to be probed. That way, suppliers can leave
> >>> their resource on AND in the right state if they were left on by the
> >>> bootloader. For example, if a clock was left on and at 200 MHz, the
> >>> clock provider needs to keep that clock ON and at 200 MHz till all the
> >>> consumers are probed.
> >>>
> >>> Answer: Let's say a consumer device Z has suppliers A, B and C. If the
> >>> linking fails at A and you return immediately, then B and C could
> >>> probe and then figure that they have no more consumers (they don't see
> >>> a link to Z) and turn off their resources. And Z could fail
> >>> catastrophically.
> >>
> >> Then I think that this approach is fatally flawed in the current implementation.
> >
> > I'm waiting to hear how it is fatally flawed. But maybe this is just a
> > misunderstanding of the problem?
>
> Fatally flawed because it does not handle modules that add a consumer
> device when the module is loaded.
If you are talking about modules adding child devices of the device
they are managing, then that's handled correctly later in the series.
If you are talking about modules adding devices that aren't defined in
DT, then right, I'm not trying to handle that. The module needs to
make sure it keeps the resources needed for new devices it's adding
are in the right state or need to add the right device links.
> > In the text below, I'm not sure if you mixing up two different things
> > or just that your wording it a bit ambiguous. So pardon my nitpick to
> > err on the side of clarity.
>
> Please do nitpick. Clarity is good.
>
>
> >
> >> A device can be added by a module that is loaded.
> >
> > No, in the example I gave, of_platform_default_populate_init() would
> > add all 3 of those devices during arch_initcall_sync().
>
> The example you gave does not cover all use cases.
>
> There are modules that add devices when the module is loaded. You can not
> ignore systems using such modules.
I'll have to agree to disagree on that. While I understand that the
design should be good and I'm happy to work on that, you can't insist
that a patch series shouldn't be allowed because it's only improving
99% of the cases and leaves the other 1% in the status quo. You are
just going to bring the kernel development to a grinding halt.
> >
> >> In that case the device
> >> was not present at late boot when the suppliers may turn off their resources.
> >
> > In that case, the _drivers_ for those devices aren't present at late
> > boot. So that they can't request to keep the resources on for their
> > consumer devices. Since there are no consumer requests on resources,
> > the suppliers turn off their resources at late boot (since there isn't
> > a better location as of today). The sync_state() call back added in a
> > subsequent patche in this series will provide the better location.
>
> And the sync_state() call back will not deal with modules that add consumer
> devices when the module is loaded, correct?
Depends. If it's just more devices from DT, then it'll be fine. If
it's not, then the module needs to take care of the needs of devices
it's adding.
> >
> >> (I am assuming the details since I have not reviewed the patches later in
> >> the series that implement this part.)
> >>
> >> Am I missing something?
> >
> > I think you are mixing up devices getting added/populated with drivers
> > getting loaded as modules?
>
> Only some modules add devices when they are loaded. But these modules do
> exist.
Out of the billions of Android devices, how many do you see this happening in?
Thanks,
Saravana
^ permalink raw reply
* Re: [PATCH v5 0/9] FPGA DFL updates
From: Greg KH @ 2019-08-19 20:51 UTC (permalink / raw)
To: Wu Hao; +Cc: mdf, linux-fpga, linux-kernel, linux-api, linux-doc, atull
In-Reply-To: <20190819053133.GA31244@hao-dev>
On Mon, Aug 19, 2019 at 01:31:33PM +0800, Wu Hao wrote:
> On Mon, Aug 12, 2019 at 10:49:55AM +0800, Wu Hao wrote:
> > Hi Greg,
> >
> > This is v5 patchset which adds more features to FPGA DFL. Marjor changes
> > against v4 are sysfs related code rework to address comments on v4.
> >
> > Please help to take a look. Thanks!
>
> Hi Greg,
>
> Did you get a chance to take a look at this new version patchset? :)
I'm not the FPGA maintainer, what about the review from the other one
first? :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v8 18/27] mm: Introduce do_mmap_locked()
From: Sean Christopherson @ 2019-08-20 1:02 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra, Randy Dunlap,
Ravi V. Shankar, Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <20190813205225.12032-19-yu-cheng.yu@intel.com>
On Tue, Aug 13, 2019 at 01:52:16PM -0700, Yu-cheng Yu wrote:
> There are a few places that need do_mmap() with mm->mmap_sem held.
> Create an in-line function for that.
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> include/linux/mm.h | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index bc58585014c9..275c385f53c6 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2394,6 +2394,24 @@ static inline void mm_populate(unsigned long addr, unsigned long len)
> static inline void mm_populate(unsigned long addr, unsigned long len) {}
> #endif
>
> +static inline unsigned long do_mmap_locked(struct file *file,
> + unsigned long addr, unsigned long len, unsigned long prot,
> + unsigned long flags, vm_flags_t vm_flags, struct list_head *uf)
> +{
> + struct mm_struct *mm = current->mm;
> + unsigned long populate;
> +
> + down_write(&mm->mmap_sem);
> + addr = do_mmap(file, addr, len, prot, flags, vm_flags, 0,
> + &populate, uf);
> + up_write(&mm->mmap_sem);
> +
> + if (populate)
> + mm_populate(addr, populate);
> +
> + return addr;
> +}
Any reason not to put this in cet.c, as suggested by PeterZ? All of the
calls from CET have identical params except for @len, e.g. you can add
'static unsigned long cet_mmap(unsigned long len)' and bury most of the
copy-paste code in there.
https://lkml.kernel.org/r/20190607074707.GD3463@hirez.programming.kicks-ass.net
> +
> /* These take the mm semaphore themselves */
> extern int __must_check vm_brk(unsigned long, unsigned long);
> extern int __must_check vm_brk_flags(unsigned long, unsigned long, unsigned long);
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v5 0/9] FPGA DFL updates
From: Wu Hao @ 2019-08-20 3:14 UTC (permalink / raw)
To: Greg KH, mdf; +Cc: linux-fpga, linux-kernel, linux-api, linux-doc, atull
In-Reply-To: <20190819205124.GA28978@kroah.com>
On Mon, Aug 19, 2019 at 10:51:24PM +0200, Greg KH wrote:
> On Mon, Aug 19, 2019 at 01:31:33PM +0800, Wu Hao wrote:
> > On Mon, Aug 12, 2019 at 10:49:55AM +0800, Wu Hao wrote:
> > > Hi Greg,
> > >
> > > This is v5 patchset which adds more features to FPGA DFL. Marjor changes
> > > against v4 are sysfs related code rework to address comments on v4.
> > >
> > > Please help to take a look. Thanks!
> >
> > Hi Greg,
> >
> > Did you get a chance to take a look at this new version patchset? :)
>
> I'm not the FPGA maintainer, what about the review from the other one
> first? :)
Sure! :)
Hi Moritz
Could you please help review these patches? Thanks! :)
Thanks
Hao
>
> thanks,
>
> greg k-h
^ permalink raw reply
* [PATCH v2 0/3] arm64: KPROBES_ON_FTRACE
From: Jisheng Zhang @ 2019-08-20 3:50 UTC (permalink / raw)
To: Catalin Marinas, Jonathan Corbet, Will Deacon, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86@kernel.org,
Naveen N. Rao, Anil S Keshavamurthy, David S. Miller,
Masami Hiramatsu
Cc: linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
KPROBES_ON_FTRACE avoids much of the overhead with regular kprobes as it
eliminates the need for a trap, as well as the need to emulate or
single-step instructions.
Applied after arm64 FTRACE_WITH_REGS:
http://lists.infradead.org/pipermail/linux-arm-kernel/2019-August/674404.html
Changes since v1:
- make the kprobes/x86: use instruction_pointer and instruction_pointer_set
as patch1
- add Masami's ACK to patch1
- add some description about KPROBES_ON_FTRACE and why we need it on
arm64
- correct the log before the patch
- remove the consolidation patch, make it as TODO
- only adjust kprobe's addr when KPROBE_FLAG_FTRACE is set
- if KPROBES_ON_FTRACE, ftrace_call_adjust() the kprobe's addr before
calling ftrace_location()
- update the kprobes-on-ftrace/arch-support.txt in doc
Jisheng Zhang (3):
kprobes/x86: use instruction_pointer and instruction_pointer_set
kprobes: adjust kprobe addr for KPROBES_ON_FTRACE
arm64: implement KPROBES_ON_FTRACE
.../debug/kprobes-on-ftrace/arch-support.txt | 2 +-
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/probes/Makefile | 1 +
arch/arm64/kernel/probes/ftrace.c | 60 +++++++++++++++++++
arch/x86/kernel/kprobes/ftrace.c | 9 +--
kernel/kprobes.c | 10 +++-
6 files changed, 75 insertions(+), 8 deletions(-)
create mode 100644 arch/arm64/kernel/probes/ftrace.c
--
2.23.0.rc1
^ permalink raw reply
* [PATCH v2 1/3] kprobes/x86: use instruction_pointer and instruction_pointer_set
From: Jisheng Zhang @ 2019-08-20 3:52 UTC (permalink / raw)
To: Catalin Marinas, Jonathan Corbet, Will Deacon, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86@kernel.org,
Naveen N. Rao, Anil S Keshavamurthy, David S. Miller,
Masami Hiramatsu
Cc: linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190820113928.1971900c@xhacker.debian>
This is to make the x86 kprobe_ftrace_handler() more common so that
the code could be reused in future.
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
---
arch/x86/kernel/kprobes/ftrace.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index 681a4b36e9bb..c2ad0b9259ca 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -28,9 +28,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
if (kprobe_running()) {
kprobes_inc_nmissed_count(p);
} else {
- unsigned long orig_ip = regs->ip;
+ unsigned long orig_ip = instruction_pointer(regs);
/* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */
- regs->ip = ip + sizeof(kprobe_opcode_t);
+ instruction_pointer_set(regs, ip + sizeof(kprobe_opcode_t));
__this_cpu_write(current_kprobe, p);
kcb->kprobe_status = KPROBE_HIT_ACTIVE;
@@ -39,12 +39,13 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
* Emulate singlestep (and also recover regs->ip)
* as if there is a 5byte nop
*/
- regs->ip = (unsigned long)p->addr + MCOUNT_INSN_SIZE;
+ instruction_pointer_set(regs,
+ (unsigned long)p->addr + MCOUNT_INSN_SIZE);
if (unlikely(p->post_handler)) {
kcb->kprobe_status = KPROBE_HIT_SSDONE;
p->post_handler(p, regs, 0);
}
- regs->ip = orig_ip;
+ instruction_pointer_set(regs, orig_ip);
}
/*
* If pre_handler returns !0, it changes regs->ip. We have to
--
2.23.0.rc1
^ permalink raw reply related
* [PATCH v2 2/3] kprobes: adjust kprobe addr for KPROBES_ON_FTRACE
From: Jisheng Zhang @ 2019-08-20 3:53 UTC (permalink / raw)
To: Catalin Marinas, Jonathan Corbet, Will Deacon, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86@kernel.org,
Naveen N. Rao, Anil S Keshavamurthy, David S. Miller,
Masami Hiramatsu
Cc: linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190820113928.1971900c@xhacker.debian>
For KPROBES_ON_FTRACE case, we need to adjust the kprobe's addr
correspondingly.
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
kernel/kprobes.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 9873fc627d61..3fd2f68644da 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1484,15 +1484,19 @@ static inline int check_kprobe_rereg(struct kprobe *p)
int __weak arch_check_ftrace_location(struct kprobe *p)
{
- unsigned long ftrace_addr;
+ unsigned long ftrace_addr, addr = (unsigned long)p->addr;
- ftrace_addr = ftrace_location((unsigned long)p->addr);
+#ifdef CONFIG_KPROBES_ON_FTRACE
+ addr = ftrace_call_adjust(addr);
+#endif
+ ftrace_addr = ftrace_location(addr);
if (ftrace_addr) {
#ifdef CONFIG_KPROBES_ON_FTRACE
/* Given address is not on the instruction boundary */
- if ((unsigned long)p->addr != ftrace_addr)
+ if (addr != ftrace_addr)
return -EILSEQ;
p->flags |= KPROBE_FLAG_FTRACE;
+ p->addr = (kprobe_opcode_t *)addr;
#else /* !CONFIG_KPROBES_ON_FTRACE */
return -EINVAL;
#endif
--
2.23.0.rc1
^ permalink raw reply related
* [PATCH v2 3/3] arm64: implement KPROBES_ON_FTRACE
From: Jisheng Zhang @ 2019-08-20 3:54 UTC (permalink / raw)
To: Catalin Marinas, Jonathan Corbet, Will Deacon, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86@kernel.org,
Naveen N. Rao, Anil S Keshavamurthy, David S. Miller,
Masami Hiramatsu
Cc: linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190820113928.1971900c@xhacker.debian>
KPROBES_ON_FTRACE avoids much of the overhead with regular kprobes as it
eliminates the need for a trap, as well as the need to emulate or
single-step instructions.
This patch implements KPROBES_ON_FTRACE for arm64.
Tested on berlin arm64 platform.
~ # mount -t debugfs debugfs /sys/kernel/debug/
~ # cd /sys/kernel/debug/
/sys/kernel/debug # echo 'p _do_fork' > tracing/kprobe_events
before the patch:
/sys/kernel/debug # cat kprobes/list
ffffff801009fe28 k _do_fork+0x0 [DISABLED]
after the patch:
/sys/kernel/debug # cat kprobes/list
ffffff801009ff54 k _do_fork+0x4 [DISABLED][FTRACE]
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
.../debug/kprobes-on-ftrace/arch-support.txt | 2 +-
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/probes/Makefile | 1 +
arch/arm64/kernel/probes/ftrace.c | 60 +++++++++++++++++++
4 files changed, 63 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/kernel/probes/ftrace.c
diff --git a/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt b/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
index 68f266944d5f..e8358a38981c 100644
--- a/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
+++ b/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
@@ -9,7 +9,7 @@
| alpha: | TODO |
| arc: | TODO |
| arm: | TODO |
- | arm64: | TODO |
+ | arm64: | ok |
| c6x: | TODO |
| csky: | TODO |
| h8300: | TODO |
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 663392d1eae2..928700f15e23 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -167,6 +167,7 @@ config ARM64
select HAVE_STACKPROTECTOR
select HAVE_SYSCALL_TRACEPOINTS
select HAVE_KPROBES
+ select HAVE_KPROBES_ON_FTRACE
select HAVE_KRETPROBES
select HAVE_GENERIC_VDSO
select IOMMU_DMA if IOMMU_SUPPORT
diff --git a/arch/arm64/kernel/probes/Makefile b/arch/arm64/kernel/probes/Makefile
index 8e4be92e25b1..4020cfc66564 100644
--- a/arch/arm64/kernel/probes/Makefile
+++ b/arch/arm64/kernel/probes/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_KPROBES) += kprobes.o decode-insn.o \
simulate-insn.o
obj-$(CONFIG_UPROBES) += uprobes.o decode-insn.o \
simulate-insn.o
+obj-$(CONFIG_KPROBES_ON_FTRACE) += ftrace.o
diff --git a/arch/arm64/kernel/probes/ftrace.c b/arch/arm64/kernel/probes/ftrace.c
new file mode 100644
index 000000000000..52901ffff570
--- /dev/null
+++ b/arch/arm64/kernel/probes/ftrace.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Dynamic Ftrace based Kprobes Optimization
+ *
+ * Copyright (C) Hitachi Ltd., 2012
+ * Copyright (C) 2019 Jisheng Zhang <jszhang@kernel.org>
+ * Synaptics Incorporated
+ */
+
+#include <linux/kprobes.h>
+
+/* Ftrace callback handler for kprobes -- called under preepmt disabed */
+void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
+ struct ftrace_ops *ops, struct pt_regs *regs)
+{
+ struct kprobe *p;
+ struct kprobe_ctlblk *kcb;
+
+ /* Preempt is disabled by ftrace */
+ p = get_kprobe((kprobe_opcode_t *)ip);
+ if (unlikely(!p) || kprobe_disabled(p))
+ return;
+
+ kcb = get_kprobe_ctlblk();
+ if (kprobe_running()) {
+ kprobes_inc_nmissed_count(p);
+ } else {
+ unsigned long orig_ip = instruction_pointer(regs);
+ /* Kprobe handler expects regs->pc = pc + 1 as breakpoint hit */
+ instruction_pointer_set(regs, ip + sizeof(kprobe_opcode_t));
+
+ __this_cpu_write(current_kprobe, p);
+ kcb->kprobe_status = KPROBE_HIT_ACTIVE;
+ if (!p->pre_handler || !p->pre_handler(p, regs)) {
+ /*
+ * Emulate singlestep (and also recover regs->pc)
+ * as if there is a nop
+ */
+ instruction_pointer_set(regs,
+ (unsigned long)p->addr + MCOUNT_INSN_SIZE);
+ if (unlikely(p->post_handler)) {
+ kcb->kprobe_status = KPROBE_HIT_SSDONE;
+ p->post_handler(p, regs, 0);
+ }
+ instruction_pointer_set(regs, orig_ip);
+ }
+ /*
+ * If pre_handler returns !0, it changes regs->pc. We have to
+ * skip emulating post_handler.
+ */
+ __this_cpu_write(current_kprobe, NULL);
+ }
+}
+NOKPROBE_SYMBOL(kprobe_ftrace_handler);
+
+int arch_prepare_kprobe_ftrace(struct kprobe *p)
+{
+ p->ainsn.api.insn = NULL;
+ return 0;
+}
--
2.23.0.rc1
^ permalink raw reply related
* Re: [PATCH v7 3/7] of/platform: Add functional dependency link from DT bindings
From: Frank Rowand @ 2019-08-20 4:26 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Jonathan Corbet,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
David Collins, Android Kernel Team, Linux Doc Mailing List
In-Reply-To: <CAGETcx8UHA9kNkjjnBXcf_OYXaaPO9ky60M01Cfz3NFb1c1FZw@mail.gmail.com>
On 8/19/19 5:09 PM, Saravana Kannan wrote:
> On Mon, Aug 19, 2019 at 2:30 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 8/19/19 1:49 PM, Saravana Kannan wrote:
>>> On Mon, Aug 19, 2019 at 10:16 AM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>
>>>> On 8/15/19 6:50 PM, Saravana Kannan wrote:
>>>>> On Wed, Aug 7, 2019 at 7:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>>>
>>>>>> On 7/23/19 5:10 PM, Saravana Kannan wrote:
>>>>>>> Add device-links after the devices are created (but before they are
>>>>>>> probed) by looking at common DT bindings like clocks and
>>>>>>> interconnects.
>>>>
>>>>
>>>> < very big snip (lots of comments that deserve answers) >
>>>>
>>>>
>>>>>>
>>>>>> /**
>>>>>> * of_link_property - TODO:
>>>>>> * dev:
>>>>>> * con_np:
>>>>>> * prop:
>>>>>> *
>>>>>> * TODO...
>>>>>> *
>>>>>> * Any failed attempt to create a link will NOT result in an immediate return.
>>>>>> * of_link_property() must create all possible links even when one of more
>>>>>> * attempts to create a link fail.
>>>>>>
>>>>>> Why? isn't one failure enough to prevent probing this device?
>>>>>> Continuing to scan just results in extra work... which will be
>>>>>> repeated every time device_link_check_waiting_consumers() is called
>>>>>
>>>>> Context:
>>>>> As I said in the cover letter, avoiding unnecessary probes is just one
>>>>> of the reasons for this patch. The other (arguably more important)
>>>>
>>>> Agree that it is more important.
>>>>
>>>>
>>>>> reason for this patch is to make sure suppliers know that they have
>>>>> consumers that are yet to be probed. That way, suppliers can leave
>>>>> their resource on AND in the right state if they were left on by the
>>>>> bootloader. For example, if a clock was left on and at 200 MHz, the
>>>>> clock provider needs to keep that clock ON and at 200 MHz till all the
>>>>> consumers are probed.
>>>>>
>>>>> Answer: Let's say a consumer device Z has suppliers A, B and C. If the
>>>>> linking fails at A and you return immediately, then B and C could
>>>>> probe and then figure that they have no more consumers (they don't see
>>>>> a link to Z) and turn off their resources. And Z could fail
>>>>> catastrophically.
>>>>
>>>> Then I think that this approach is fatally flawed in the current implementation.
>>>
>>> I'm waiting to hear how it is fatally flawed. But maybe this is just a
>>> misunderstanding of the problem?
>>
>> Fatally flawed because it does not handle modules that add a consumer
>> device when the module is loaded.
>
> If you are talking about modules adding child devices of the device
> they are managing, then that's handled correctly later in the series.
They may or they may not. I do not know. I am not going to audit all
current cases of devices being added to check that relationship and I am
not going to monitor all future patches that add devices. Adding devices
is an existing pattern of behavior that the new feature must be able to
handle.
I have not looked at patch 6 yet (the place where modules adding child
devices is handled). I am guessing that patch 6 could be made more
general to remove the parent child relationship restriction.
>
> If you are talking about modules adding devices that aren't defined in
> DT, then right, I'm not trying to handle that. The module needs to
> make sure it keeps the resources needed for new devices it's adding
> are in the right state or need to add the right device links.
I am not talking about devices that are not defined in the devicetree.
>
>>> In the text below, I'm not sure if you mixing up two different things
>>> or just that your wording it a bit ambiguous. So pardon my nitpick to
>>> err on the side of clarity.
>>
>> Please do nitpick. Clarity is good.
>>
>>
>>>
>>>> A device can be added by a module that is loaded.
>>>
>>> No, in the example I gave, of_platform_default_populate_init() would
>>> add all 3 of those devices during arch_initcall_sync().
>>
>> The example you gave does not cover all use cases.
>>
>> There are modules that add devices when the module is loaded. You can not
>> ignore systems using such modules.
>
> I'll have to agree to disagree on that. While I understand that the
> design should be good and I'm happy to work on that, you can't insist
> that a patch series shouldn't be allowed because it's only improving
> 99% of the cases and leaves the other 1% in the status quo. You are
> just going to bring the kernel development to a grinding halt.
No, you do not get to disagree on that. And you are presenting a straw
man argument.
You are proposing a new feature that contributes fragility and complexity
to the house of cards that device instantiation and driver probing already
is.
The feature is clever but it is intertwined into an area that is already
complex and in many cases difficult to work within.
I had hoped that the feature was robust enough and generic enough to
accept. The proposed feature is a hack to paper over a specific problem
that you are facing. I had hoped that the feature would appear generic
enough that I would not have to regard it as an attempt to paper over
the real problem. I have not given up this hope yet but I still am
quite cautious about this approach to addressing your use case.
You have a real bug. I have told you how to fix the real bug. And you
have ignored my suggestion. (To be honest, I do not know for sure that
my suggestion is feasible, but on the surface it appears to be.) Again,
my suggestion is to have the boot loader pass information to the kernel
(via a chosen property) telling the kernel which devices the bootloader
has enabled power to. The power subsystem would use that information
early in boot to do a "get" on the power supplier (I am not using precise
power subsystem terminology, but it should be obvious what I mean).
The consumer device driver would also have to be aware of the information
passed via the chosen property because the power subsystem has done the
"get" on the consumer devices behalf (exactly how the consumer gets
that information is an implementation detail). This approach is
more direct, less subtle, less fragile.
>
>>>
>>>> In that case the device
>>>> was not present at late boot when the suppliers may turn off their resources.
>>>
>>> In that case, the _drivers_ for those devices aren't present at late
>>> boot. So that they can't request to keep the resources on for their
>>> consumer devices. Since there are no consumer requests on resources,
>>> the suppliers turn off their resources at late boot (since there isn't
>>> a better location as of today). The sync_state() call back added in a
>>> subsequent patche in this series will provide the better location.
>>
>> And the sync_state() call back will not deal with modules that add consumer
>> devices when the module is loaded, correct?
>
> Depends. If it's just more devices from DT, then it'll be fine. If
> it's not, then the module needs to take care of the needs of devices
> it's adding.>
>>>
>>>> (I am assuming the details since I have not reviewed the patches later in
>>>> the series that implement this part.)
>>>>
>>>> Am I missing something?
>>>
>>> I think you are mixing up devices getting added/populated with drivers
>>> getting loaded as modules?
>>
>> Only some modules add devices when they are loaded. But these modules do
>> exist.
>
> Out of the billions of Android devices, how many do you see this happening in?
The Linux kernel is not just used by Android devices.
-Frank
>
> Thanks,
> Saravana
>
^ permalink raw reply
* Re: [PATCH v5 15/17] mfd: ioc3: Add driver for SGI IOC3 chip
From: Alexandre Belloni @ 2019-08-20 6:23 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Jonathan Corbet, Ralf Baechle, Paul Burton, James Hogan,
Dmitry Torokhov, Lee Jones, David S. Miller, Srinivas Kandagatla,
Alessandro Zummo, Greg Kroah-Hartman, Jiri Slaby,
Evgeniy Polyakov, linux-doc, linux-kernel, linux-mips,
linux-input, netdev, linux-rtc, linux-serial
In-Reply-To: <20190819163144.3478-16-tbogendoerfer@suse.de>
Hi,
On 19/08/2019 18:31:38+0200, Thomas Bogendoerfer wrote:
> diff --git a/drivers/mfd/ioc3.c b/drivers/mfd/ioc3.c
> new file mode 100644
> index 000000000000..5bcb3461a189
> --- /dev/null
> +++ b/drivers/mfd/ioc3.c
> @@ -0,0 +1,586 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SGI IOC3 multifunction device driver
> + *
> + * Copyright (C) 2018, 2019 Thomas Bogendoerfer <tbogendoerfer@suse.de>
> + *
> + * Based on work by:
> + * Stanislaw Skowronek <skylark@unaligned.org>
> + * Joshua Kinard <kumba@gentoo.org>
> + * Brent Casavant <bcasavan@sgi.com> - IOC4 master driver
> + * Pat Gefre <pfg@sgi.com> - IOC3 serial port IRQ demuxer
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/platform_data/sgi-w1.h>
> +#include <linux/rtc/ds1685.h>
I don't think this include is necessary.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v2 3/3] arm64: implement KPROBES_ON_FTRACE
From: Jisheng Zhang @ 2019-08-20 7:17 UTC (permalink / raw)
To: Catalin Marinas, Jonathan Corbet, Will Deacon, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86@kernel.org,
Naveen N. Rao, Anil S Keshavamurthy, David S. Miller,
Masami Hiramatsu
Cc: linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org
In-Reply-To: <20190820114314.685a3239@xhacker.debian>
On Tue, 20 Aug 2019 03:54:20 +0000 Jisheng Zhang wrote:
>
>
> KPROBES_ON_FTRACE avoids much of the overhead with regular kprobes as it
> eliminates the need for a trap, as well as the need to emulate or
> single-step instructions.
>
> This patch implements KPROBES_ON_FTRACE for arm64.
>
> Tested on berlin arm64 platform.
some performance numbers may be interesting.
HW: Berlin arm64 platform, cpufreq is forced to 800MHZ
SW: getppid syscall micro-benchmark, source code is put at the end of this email.
A. Not probed.
B. Probed at __arm64_sys_getppid w/ non-operation probe functions, w/o KPROBES_ON_FTRACE
C. Probed at __arm64_sys_getppid w/ non-operation probe functions, w/ KPROBES_ON_FTRACE
A: 1905 ns/call
B: 5833 ns/call
C: 2169 ns/call
The overhead of kprobes is 5833 - 1905 = 3928 ns/call
The overhead of kprobes w/ KPROBES_ON_FTRACE is 2169 - 1905 = 264 ns/call
As can be seen, KPROBES_ON_FTRACE significantly reduce the overhead of kprobes.
Thanks
<---8---
#include <stdio.h>
#include <stdlib.h>
#include <sys/time.h>
#include <sys/resource.h>
#include <unistd.h>
int main (int argc, char *argv[])
{
struct timeval tv;
unsigned long count;
struct rusage usage;
for (count = 0; count < 10000000; count++)
getppid();
getrusage(RUSAGE_SELF, &usage);
tv = usage.ru_stime;
tv.tv_sec += usage.ru_utime.tv_sec;
tv.tv_usec += usage.ru_utime.tv_usec;
fprintf(stderr, "getppid was called %u times: %d nsec per call\n",
count, (tv.tv_sec*1000*1000 + tv.tv_usec)/(count/1000));
return 0;
}
>
> ~ # mount -t debugfs debugfs /sys/kernel/debug/
> ~ # cd /sys/kernel/debug/
> /sys/kernel/debug # echo 'p _do_fork' > tracing/kprobe_events
>
> before the patch:
>
> /sys/kernel/debug # cat kprobes/list
> ffffff801009fe28 k _do_fork+0x0 [DISABLED]
>
> after the patch:
>
> /sys/kernel/debug # cat kprobes/list
> ffffff801009ff54 k _do_fork+0x4 [DISABLED][FTRACE]
>
> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> ---
> .../debug/kprobes-on-ftrace/arch-support.txt | 2 +-
> arch/arm64/Kconfig | 1 +
> arch/arm64/kernel/probes/Makefile | 1 +
> arch/arm64/kernel/probes/ftrace.c | 60 +++++++++++++++++++
> 4 files changed, 63 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm64/kernel/probes/ftrace.c
>
> diff --git a/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt b/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
> index 68f266944d5f..e8358a38981c 100644
> --- a/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
> +++ b/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
> @@ -9,7 +9,7 @@
> | alpha: | TODO |
> | arc: | TODO |
> | arm: | TODO |
> - | arm64: | TODO |
> + | arm64: | ok |
> | c6x: | TODO |
> | csky: | TODO |
> | h8300: | TODO |
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 663392d1eae2..928700f15e23 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -167,6 +167,7 @@ config ARM64
> select HAVE_STACKPROTECTOR
> select HAVE_SYSCALL_TRACEPOINTS
> select HAVE_KPROBES
> + select HAVE_KPROBES_ON_FTRACE
> select HAVE_KRETPROBES
> select HAVE_GENERIC_VDSO
> select IOMMU_DMA if IOMMU_SUPPORT
> diff --git a/arch/arm64/kernel/probes/Makefile b/arch/arm64/kernel/probes/Makefile
> index 8e4be92e25b1..4020cfc66564 100644
> --- a/arch/arm64/kernel/probes/Makefile
> +++ b/arch/arm64/kernel/probes/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_KPROBES) += kprobes.o decode-insn.o \
> simulate-insn.o
> obj-$(CONFIG_UPROBES) += uprobes.o decode-insn.o \
> simulate-insn.o
> +obj-$(CONFIG_KPROBES_ON_FTRACE) += ftrace.o
> diff --git a/arch/arm64/kernel/probes/ftrace.c b/arch/arm64/kernel/probes/ftrace.c
> new file mode 100644
> index 000000000000..52901ffff570
> --- /dev/null
> +++ b/arch/arm64/kernel/probes/ftrace.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Dynamic Ftrace based Kprobes Optimization
> + *
> + * Copyright (C) Hitachi Ltd., 2012
> + * Copyright (C) 2019 Jisheng Zhang <jszhang@kernel.org>
> + * Synaptics Incorporated
> + */
> +
> +#include <linux/kprobes.h>
> +
> +/* Ftrace callback handler for kprobes -- called under preepmt disabed */
> +void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
> + struct ftrace_ops *ops, struct pt_regs *regs)
> +{
> + struct kprobe *p;
> + struct kprobe_ctlblk *kcb;
> +
> + /* Preempt is disabled by ftrace */
> + p = get_kprobe((kprobe_opcode_t *)ip);
> + if (unlikely(!p) || kprobe_disabled(p))
> + return;
> +
> + kcb = get_kprobe_ctlblk();
> + if (kprobe_running()) {
> + kprobes_inc_nmissed_count(p);
> + } else {
> + unsigned long orig_ip = instruction_pointer(regs);
> + /* Kprobe handler expects regs->pc = pc + 1 as breakpoint hit */
> + instruction_pointer_set(regs, ip + sizeof(kprobe_opcode_t));
> +
> + __this_cpu_write(current_kprobe, p);
> + kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> + if (!p->pre_handler || !p->pre_handler(p, regs)) {
> + /*
> + * Emulate singlestep (and also recover regs->pc)
> + * as if there is a nop
> + */
> + instruction_pointer_set(regs,
> + (unsigned long)p->addr + MCOUNT_INSN_SIZE);
> + if (unlikely(p->post_handler)) {
> + kcb->kprobe_status = KPROBE_HIT_SSDONE;
> + p->post_handler(p, regs, 0);
> + }
> + instruction_pointer_set(regs, orig_ip);
> + }
> + /*
> + * If pre_handler returns !0, it changes regs->pc. We have to
> + * skip emulating post_handler.
> + */
> + __this_cpu_write(current_kprobe, NULL);
> + }
> +}
> +NOKPROBE_SYMBOL(kprobe_ftrace_handler);
> +
> +int arch_prepare_kprobe_ftrace(struct kprobe *p)
> +{
> + p->ainsn.api.insn = NULL;
> + return 0;
> +}
> --
> 2.23.0.rc1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
^ permalink raw reply
* Re: [PATCH v5 15/17] mfd: ioc3: Add driver for SGI IOC3 chip
From: Thomas Bogendoerfer @ 2019-08-20 8:17 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Jonathan Corbet, Ralf Baechle, Paul Burton, James Hogan,
Dmitry Torokhov, Lee Jones, David S. Miller, Srinivas Kandagatla,
Alessandro Zummo, Greg Kroah-Hartman, Jiri Slaby,
Evgeniy Polyakov, linux-doc, linux-kernel, linux-mips,
linux-input, netdev, linux-rtc, linux-serial
In-Reply-To: <20190820062308.GK3545@piout.net>
On Tue, 20 Aug 2019 08:23:08 +0200
Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:
> On 19/08/2019 18:31:38+0200, Thomas Bogendoerfer wrote:
> > diff --git a/drivers/mfd/ioc3.c b/drivers/mfd/ioc3.c
> > new file mode 100644
> > index 000000000000..5bcb3461a189
> > --- /dev/null
> > +++ b/drivers/mfd/ioc3.c
> > @@ -0,0 +1,586 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * SGI IOC3 multifunction device driver
> > + *
> > + * Copyright (C) 2018, 2019 Thomas Bogendoerfer <tbogendoerfer@suse.de>
> > + *
> > + * Based on work by:
> > + * Stanislaw Skowronek <skylark@unaligned.org>
> > + * Joshua Kinard <kumba@gentoo.org>
> > + * Brent Casavant <bcasavan@sgi.com> - IOC4 master driver
> > + * Pat Gefre <pfg@sgi.com> - IOC3 serial port IRQ demuxer
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/errno.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/platform_data/sgi-w1.h>
> > +#include <linux/rtc/ds1685.h>
> I don't think this include is necessary.
you are right. I'll move it to the patch where IP30 systemboard gets added.
Thanks,
Thomas.
--
SUSE Linux GmbH
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
^ permalink raw reply
* Re: [PATCH v2 3/3] arm64: implement KPROBES_ON_FTRACE
From: Thomas Gleixner @ 2019-08-20 8:53 UTC (permalink / raw)
To: Jisheng Zhang
Cc: Catalin Marinas, Jonathan Corbet, Will Deacon, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, x86@kernel.org, Naveen N. Rao,
Anil S Keshavamurthy, David S. Miller, Masami Hiramatsu,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190820114314.685a3239@xhacker.debian>
On Tue, 20 Aug 2019, Jisheng Zhang wrote:
> KPROBES_ON_FTRACE avoids much of the overhead with regular kprobes as it
> eliminates the need for a trap, as well as the need to emulate or
> single-step instructions.
>
> This patch implements KPROBES_ON_FTRACE for arm64.
git grep 'This patch' Documentation/process/submitting-patches.rst
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v2 1/3] kprobes/x86: use instruction_pointer and instruction_pointer_set
From: Thomas Gleixner @ 2019-08-20 8:53 UTC (permalink / raw)
To: Jisheng Zhang
Cc: Catalin Marinas, Jonathan Corbet, Will Deacon, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, x86@kernel.org, Naveen N. Rao,
Anil S Keshavamurthy, David S. Miller, Masami Hiramatsu,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190820114109.4624d56b@xhacker.debian>
On Tue, 20 Aug 2019, Jisheng Zhang wrote:
> This is to make the x86 kprobe_ftrace_handler() more common so that
> the code could be reused in future.
While I agree with the change in general, I can't find anything which
reuses that code. So the change log is pretty useless and I have no idea
how this is related to the rest of the series.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v2 1/3] kprobes/x86: use instruction_pointer and instruction_pointer_set
From: Jisheng Zhang @ 2019-08-20 9:02 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Catalin Marinas, Jonathan Corbet, Will Deacon, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, x86@kernel.org, Naveen N. Rao,
Anil S Keshavamurthy, David S. Miller, Masami Hiramatsu,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <alpine.DEB.2.21.1908201050370.2223@nanos.tec.linutronix.de>
Hi Thomas,
On Tue, 20 Aug 2019 10:53:58 +0200 (CEST) Thomas Gleixner wrote:
>
>
> On Tue, 20 Aug 2019, Jisheng Zhang wrote:
>
> > This is to make the x86 kprobe_ftrace_handler() more common so that
> > the code could be reused in future.
>
> While I agree with the change in general, I can't find anything which
> reuses that code. So the change log is pretty useless and I have no idea
> how this is related to the rest of the series.
In v1, this code is moved from x86 to common kprobes.c [1]
But I agree with Masami, consolidation could be done when arm64 kprobes
on ftrace is stable.
In v2, actually, the arm64 version's kprobe_ftrace_handler() is the same
as x86's, the only difference is comment, e.g
/* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */
while in arm64
/* Kprobe handler expects regs->pc = ip + 1 as breakpoint hit */
W/ above, any suggestion about the suitable change log?
Thanks
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2019-August/674417.html
^ permalink raw reply
* Re: [PATCH v2 1/3] kprobes/x86: use instruction_pointer and instruction_pointer_set
From: Jisheng Zhang @ 2019-08-20 9:20 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Jonathan Corbet, Catalin Marinas, x86@kernel.org,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
Anil S Keshavamurthy, Ingo Molnar, Borislav Petkov,
Masami Hiramatsu, H. Peter Anvin, Naveen N. Rao, Will Deacon,
David S. Miller, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190820165152.20275268@xhacker.debian>
On Tue, 20 Aug 2019 09:02:59 +0000 Jisheng Zhang wrote:
>
>
> Hi Thomas,
>
> On Tue, 20 Aug 2019 10:53:58 +0200 (CEST) Thomas Gleixner wrote:
>
> >
> >
> > On Tue, 20 Aug 2019, Jisheng Zhang wrote:
> >
> > > This is to make the x86 kprobe_ftrace_handler() more common so that
> > > the code could be reused in future.
> >
> > While I agree with the change in general, I can't find anything which
> > reuses that code. So the change log is pretty useless and I have no idea
> > how this is related to the rest of the series.
Indeed, this isn't related to the rest of the series. So will update the
change log and resend it alone.
>
> In v1, this code is moved from x86 to common kprobes.c [1]
> But I agree with Masami, consolidation could be done when arm64 kprobes
> on ftrace is stable.
>
> In v2, actually, the arm64 version's kprobe_ftrace_handler() is the same
> as x86's, the only difference is comment, e.g
>
> /* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */
>
> while in arm64
>
> /* Kprobe handler expects regs->pc = ip + 1 as breakpoint hit */
>
>
> W/ above, any suggestion about the suitable change log?
>
> Thanks
>
^ permalink raw reply
* Re: [PATCH v8 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file
From: Dave Martin @ 2019-08-20 10:02 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra, Randy Dunlap,
Ravi V. Shankar, Vedvyas Shanbhogue
In-Reply-To: <20190813205225.12032-23-yu-cheng.yu@intel.com>
On Tue, Aug 13, 2019 at 01:52:20PM -0700, Yu-cheng Yu wrote:
> An ELF file's .note.gnu.property indicates features the executable file
> can support. For example, the property GNU_PROPERTY_X86_FEATURE_1_AND
> indicates the file supports GNU_PROPERTY_X86_FEATURE_1_IBT and/or
> GNU_PROPERTY_X86_FEATURE_1_SHSTK.
>
> With this patch, if an arch needs to setup features from ELF properties,
> it needs CONFIG_ARCH_USE_GNU_PROPERTY to be set, and specific
> arch_parse_property() and arch_setup_property().
>
> For example, for X86_64:
>
> int arch_setup_property(void *ehdr, void *phdr, struct file *f, bool inter)
> {
> int r;
> uint32_t property;
>
> r = get_gnu_property(ehdr, phdr, f, GNU_PROPERTY_X86_FEATURE_1_AND,
> &property);
> ...
> }
>
> This patch is derived from code provided by H.J. Lu <hjl.tools@gmail.com>.
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
[...]
For the hell of it, I tried implementing an alternate version [1] that
tries to integrate into the existing ELF loader more directly.
This may or may not be a better approach, but tries to solve some
issues such as not repeatedly reading and parsing the properties.
Cheers
---Dave
[1] [RFC PATCH 0/2] ELF: Alternate program property parser
https://lore.kernel.org/lkml/1566295063-7387-1-git-send-email-Dave.Martin@arm.com/
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox