* Re: [PATCH v2 9/9] powerpc/hugetlb: Enable hugetlb migration for ppc64
From: Aneesh Kumar K.V @ 2018-05-04 8:25 UTC (permalink / raw)
To: Christophe LEROY, mpe; +Cc: akpm, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <69b4fae5-d413-4866-7ce4-3873d3c6590f@c-s.fr>
Christophe LEROY <christophe.leroy@c-s.fr> writes:
> Le 16/05/2017 =C3=A0 11:23, Aneesh Kumar K.V a =C3=A9crit=C2=A0:
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/platforms/Kconfig.cputype | 5 +++++
>> 1 file changed, 5 insertions(+)
>>=20
>> diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platf=
orms/Kconfig.cputype
>> index 80175000042d..8acc4f27d101 100644
>> --- a/arch/powerpc/platforms/Kconfig.cputype
>> +++ b/arch/powerpc/platforms/Kconfig.cputype
>> @@ -351,6 +351,11 @@ config PPC_RADIX_MMU
>> is only implemented by IBM Power9 CPUs, if you don't have one of th=
em
>> you can probably disable this.
>>=20=20=20
>> +config ARCH_ENABLE_HUGEPAGE_MIGRATION
>> + def_bool y
>> + depends on PPC_BOOK3S_64 && HUGETLB_PAGE && MIGRATION
>> +
>> +
>
> Is there a reason why you redefine ARCH_ENABLE_HUGEPAGE_MIGRATION=20
> instead of doing a 'select' as it is already defined in mm/Kconfig ?
>
That got copied from x86 Kconfig i guess.
-aneesh
^ permalink raw reply
* Re: [PATCH 08/13] powerpc/eeh: Introduce eeh_for_each_pe()
From: Russell Currey @ 2018-05-04 6:59 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <8fd8b61f3e04aadd476116404b9047a898870f0b.1525242772.git.sbobroff@linux.ibm.com>
On Wed, 2018-05-02 at 16:36 +1000, Sam Bobroff wrote:
> Add a for_each-style macro for iterating through PEs without the
> boilerplate required by a traversal function. eeh_pe_next() is now
> exported, as it is now used directly in place.
>
> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
Reviewed-by: Russell Currey <ruscur@russell.cc>
^ permalink raw reply
* Re: [PATCH 07/13] powerpc/eeh: Clean up pci_ers_result handling
From: Russell Currey @ 2018-05-04 6:58 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <a4aae83e02d8a5424a92321659a2100a5589d366.1525242772.git.sbobroff@linux.ibm.com>
On Wed, 2018-05-02 at 16:36 +1000, Sam Bobroff wrote:
> As EEH event handling progresses, a cumulative result of type
> pci_ers_result is built up by (some of) the eeh_report_*() functions
> using either:
> if (rc == PCI_ERS_RESULT_NEED_RESET) *res = rc;
> if (*res == PCI_ERS_RESULT_NONE) *res = rc;
> or:
> if ((*res == PCI_ERS_RESULT_NONE) ||
> (*res == PCI_ERS_RESULT_RECOVERED)) *res = rc;
> if (*res == PCI_ERS_RESULT_DISCONNECT &&
> rc == PCI_ERS_RESULT_NEED_RESET) *res = rc;
> (Where *res is the accumulator.)
>
> However, the intent is not immediately clear and the result in some
> situations is order dependent.
>
> Address this by assigning a priority to each result value, and always
> merging to the highest priority. This renders the intent clear, and
> provides a stable value for all orderings.
>
> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> ---
> arch/powerpc/kernel/eeh_driver.c | 36 ++++++++++++++++++++++++++--
> --------
> 1 file changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/kernel/eeh_driver.c
> b/arch/powerpc/kernel/eeh_driver.c
> index 188d15c4fe3a..f33dd68a9ca2 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -39,6 +39,29 @@ struct eeh_rmv_data {
> int removed;
> };
>
> +static int eeh_result_priority(enum pci_ers_result result)
> +{
> + switch (result) {
> + case PCI_ERS_RESULT_NONE: return 0;
> + case PCI_ERS_RESULT_NO_AER_DRIVER: return 1;
> + case PCI_ERS_RESULT_RECOVERED: return 2;
> + case PCI_ERS_RESULT_CAN_RECOVER: return 3;
> + case PCI_ERS_RESULT_DISCONNECT: return 4;
> + case PCI_ERS_RESULT_NEED_RESET: return 5;
> + default:
> + WARN_ONCE(1, "Unknown pci_ers_result value");
Missing newline and also would be good to print the value was
> + return 0;
> + }
> +};
> +
> +static enum pci_ers_result merge_result(enum pci_ers_result old,
> + enum pci_ers_result new)
merge_result() sounds like something really generic, maybe call it
pci_ers_merge_result() or something?
Note: just learned that it stands for Error Recovery System and that's
a thing (?)
> +{
> + if (eeh_result_priority(new) > eeh_result_priority(old))
> + return new;
> + return old;
MAX would be nicer as per mpe's suggestion
> +}
> +
> /**
> * eeh_pcid_get - Get the PCI device driver
> * @pdev: PCI device
> @@ -206,9 +229,7 @@ static void *eeh_report_error(struct eeh_dev
> *edev, void *userdata)
>
> rc = driver->err_handler->error_detected(dev,
> pci_channel_io_frozen);
>
> - /* A driver that needs a reset trumps all others */
> - if (rc == PCI_ERS_RESULT_NEED_RESET) *res = rc;
> - if (*res == PCI_ERS_RESULT_NONE) *res = rc;
> + *res = merge_result(*res, rc);
>
> edev->in_error = true;
> pci_uevent_ers(dev, PCI_ERS_RESULT_NONE);
> @@ -249,9 +270,7 @@ static void *eeh_report_mmio_enabled(struct
> eeh_dev *edev, void *userdata)
>
> rc = driver->err_handler->mmio_enabled(dev);
>
> - /* A driver that needs a reset trumps all others */
> - if (rc == PCI_ERS_RESULT_NEED_RESET) *res = rc;
> - if (*res == PCI_ERS_RESULT_NONE) *res = rc;
> + *res = merge_result(*res, rc);
>
> out:
> eeh_pcid_put(dev);
> @@ -294,10 +313,7 @@ static void *eeh_report_reset(struct eeh_dev
> *edev, void *userdata)
> goto out;
>
> rc = driver->err_handler->slot_reset(dev);
> - if ((*res == PCI_ERS_RESULT_NONE) ||
> - (*res == PCI_ERS_RESULT_RECOVERED)) *res = rc;
> - if (*res == PCI_ERS_RESULT_DISCONNECT &&
> - rc == PCI_ERS_RESULT_NEED_RESET) *res = rc;
> + *res = merge_result(*res, rc);
>
> out:
> eeh_pcid_put(dev);
^ permalink raw reply
* Re: [PATCH 06/13] powerpc/eeh: Add message when PE processing at parent
From: Russell Currey @ 2018-05-04 6:51 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <dcc5a3ed61264ac43265fd4be8509be71824cff6.1525242772.git.sbobroff@linux.ibm.com>
On Wed, 2018-05-02 at 16:35 +1000, Sam Bobroff wrote:
> To aid debugging, add a message to show when EEH processing for a PE
> will be done at the device's parent, rather than directly at the
> device.
>
> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
Good idea!
Reviewed-by: Russell Currey <ruscur@russell.cc>
^ permalink raw reply
* Re: [PATCH 05/13] powerpc/eeh: Strengthen types of eeh traversal functions
From: Russell Currey @ 2018-05-04 6:32 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <4d86d04e2c48a80e604e790c48d6a608c5d45498.1525242772.git.sbobroff@linux.ibm.com>
On Wed, 2018-05-02 at 16:35 +1000, Sam Bobroff wrote:
> The traversal functions eeh_pe_traverse() and eeh_pe_dev_traverse()
> both provide their first argument as void * but every single user
> casts
> it to the expected type.
>
> Change the type of the first parameter from void * to the appropriate
> type, and clean up all uses.
>
> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
Reviewed-by: Russell Currey <ruscur@russell.cc>
^ permalink raw reply
* Re: [PATCH 04/13] powerpc/eeh: Remove unused eeh_pcid_name()
From: Russell Currey @ 2018-05-04 6:29 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <e3aa92a372b8c729f53562b2450c9160e71e6a59.1525242772.git.sbobroff@linux.ibm.com>
On Wed, 2018-05-02 at 16:35 +1000, Sam Bobroff wrote:
> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
Wow, this has been around since 2006.
Reviewed-by: Russell Currey <ruscur@russell.cc>
^ permalink raw reply
* Re: [PATCH 02/13] powerpc/eeh: Add final message for successful recovery
From: Russell Currey @ 2018-05-04 6:08 UTC (permalink / raw)
To: Michael Ellerman, Sam Bobroff, linuxppc-dev
In-Reply-To: <874ljo9ljl.fsf@concordia.ellerman.id.au>
On Fri, 2018-05-04 at 12:55 +1000, Michael Ellerman wrote:
> Sam Bobroff <sbobroff@linux.ibm.com> writes:
>
> > Add a single log line at the end of successful EEH recovery, so
> > that
> > it's clear that event processing has finished.
> >
> > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> > ---
> > arch/powerpc/kernel/eeh_driver.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/powerpc/kernel/eeh_driver.c
> > b/arch/powerpc/kernel/eeh_driver.c
> > index 56a60b9eb397..07e0a42035ce 100644
> > --- a/arch/powerpc/kernel/eeh_driver.c
> > +++ b/arch/powerpc/kernel/eeh_driver.c
> > @@ -910,6 +910,7 @@ void eeh_handle_normal_event(struct eeh_pe *pe)
> > pr_info("EEH: Notify device driver to resume\n");
> > eeh_pe_dev_traverse(pe, eeh_report_resume, NULL);
> >
> > + pr_info("EEH: Recovery successful.\n");
> Is it possible for recovery for multiple devices to be interleaved?
>
> Should that message include the device?
Pretty sure EEH will only process a single error at a time so this
*should* always let you infer from context, but PHB and PE should
probably be included anyway. It'd be cool to move pe_{err/warn/info}()
out of powernv for messages like this.
- Russell
>
> cheers
^ permalink raw reply
* Re: [PATCH 01/13] powerpc/eeh: Add eeh_max_freezes to initial EEH log line
From: Russell Currey @ 2018-05-04 5:46 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <ac861431c98e0c259fec18a0d220994ad6b362ae.1525242772.git.sbobroff@linux.ibm.com>
On Wed, 2018-05-02 at 16:34 +1000, Sam Bobroff wrote:
> The current failure message includes the number of failures that have
> occurred in the last hour (for a device) but it does not indicate
> how many failures will be tolerated before the device is permanently
> disabled.
>
> Include the limit (eeh_max_freezes) to make this less surprising when
> it happens.
>
> Also remove the embedded newline from the existing message to make it
> easier to grep for.
>
> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
Good idea.
Reviewed-by: Russell Currey <ruscur@russell.cc>
^ permalink raw reply
* Re: [PATCH 04/15] powerpc/powernv: opal-kmsg use flush fallback from console code
From: Nicholas Piggin @ 2018-05-04 5:37 UTC (permalink / raw)
To: Michael Ellerman
Cc: linuxppc-dev, Jiri Slaby, linux-kernel, Greg Kroah-Hartman
In-Reply-To: <87muxg80ga.fsf@concordia.ellerman.id.au>
On Fri, 04 May 2018 15:16:37 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
>
> > Use the more refined and tested event polling loop from opal_put_chars
> > as the fallback console flush in the opal-kmsg path. This loop is used
> > by the console driver today, whereas the opal-kmsg fallback is not
> > likely to have been used for years.
> >
> > Use WARN_ONCE rather than a printk when the fallback is invoked to
> > prepare for moving the console flush into a common function.
>
> Do we want to add a WARN in that path? If we're panicking things might
> get worse if we WARN (which takes a trap).
True, probably a good idea not to... oh there's a printk_once so
that'll work nicely.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 04/15] powerpc/powernv: opal-kmsg use flush fallback from console code
From: Michael Ellerman @ 2018-05-04 5:16 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
Cc: Jiri Slaby, linux-kernel, Nicholas Piggin, Greg Kroah-Hartman
In-Reply-To: <20180430145558.4308-5-npiggin@gmail.com>
Nicholas Piggin <npiggin@gmail.com> writes:
> Use the more refined and tested event polling loop from opal_put_chars
> as the fallback console flush in the opal-kmsg path. This loop is used
> by the console driver today, whereas the opal-kmsg fallback is not
> likely to have been used for years.
>
> Use WARN_ONCE rather than a printk when the fallback is invoked to
> prepare for moving the console flush into a common function.
Do we want to add a WARN in that path? If we're panicking things might
get worse if we WARN (which takes a trap).
cheers
^ permalink raw reply
* Re: [PATCH 11/13] powerpc/eeh: Introduce eeh_set_irq_state()
From: Michael Ellerman @ 2018-05-04 3:02 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <facebfcf7726c381957cc5fac0fc9510ac27c472.1525242772.git.sbobroff@linux.ibm.com>
Sam Bobroff <sbobroff@linux.ibm.com> writes:
> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
> index f63a01d336ee..b3edd0df04b8 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -210,6 +206,23 @@ static void eeh_set_channel_state(struct eeh_pe *root, enum pci_channel_state s)
> edev->pdev->error_state = s;
> }
>
> +static void eeh_set_irq_state(struct eeh_pe *root, bool enable)
> +{
> + struct eeh_pe *pe;
> + struct eeh_dev *edev, *tmp;
> +
> + eeh_for_each_pe(root, pe)
> + eeh_pe_for_each_dev(pe, edev, tmp)
> + if (eeh_edev_actionable(edev))
> + if (eeh_pcid_get(edev->pdev)) {
> + if (enable)
> + eeh_enable_irq(edev);
> + else
> + eeh_disable_irq(edev);
> + eeh_pcid_put(edev->pdev);
> + }
Yikes.
What about?
eeh_for_each_pe(root, pe) {
eeh_pe_for_each_dev(pe, edev, tmp) {
if (!eeh_edev_actionable(edev))
continue;
if (!eeh_pcid_get(edev->pdev))
continue;
if (enable)
eeh_enable_irq(edev);
else
eeh_disable_irq(edev);
eeh_pcid_put(edev->pdev);
}
}
cheers
^ permalink raw reply
* Re: [PATCH 07/13] powerpc/eeh: Clean up pci_ers_result handling
From: Michael Ellerman @ 2018-05-04 2:58 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <a4aae83e02d8a5424a92321659a2100a5589d366.1525242772.git.sbobroff@linux.ibm.com>
Sam Bobroff <sbobroff@linux.ibm.com> writes:
> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
> index 188d15c4fe3a..f33dd68a9ca2 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -39,6 +39,29 @@ struct eeh_rmv_data {
> int removed;
> };
>
> +static int eeh_result_priority(enum pci_ers_result result)
> +{
> + switch (result) {
> + case PCI_ERS_RESULT_NONE: return 0;
> + case PCI_ERS_RESULT_NO_AER_DRIVER: return 1;
> + case PCI_ERS_RESULT_RECOVERED: return 2;
> + case PCI_ERS_RESULT_CAN_RECOVER: return 3;
> + case PCI_ERS_RESULT_DISCONNECT: return 4;
> + case PCI_ERS_RESULT_NEED_RESET: return 5;
> + default:
> + WARN_ONCE(1, "Unknown pci_ers_result value");
> + return 0;
> + }
> +};
> +
> +static enum pci_ers_result merge_result(enum pci_ers_result old,
> + enum pci_ers_result new)
> +{
> + if (eeh_result_priority(new) > eeh_result_priority(old))
> + return new;
> + return old;
max() ?
cheers
^ permalink raw reply
* Re: [PATCH 03/13] powerpc/eeh: Fix use-after-release of EEH driver
From: Michael Ellerman @ 2018-05-04 2:56 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <e50359f9211fcdc342401806ab65aa092c63e7b9.1525242772.git.sbobroff@linux.ibm.com>
Sam Bobroff <sbobroff@linux.ibm.com> writes:
> Correct two cases where eeh_pcid_get() is used to reference the driver's
> module but the reference is dropped before the driver pointer is used.
>
> In eeh_rmv_device() also refactor a little so that only two calls to
> eeh_pcid_put() are needed, rather than three and the reference isn't
> taken at all if it wasn't needed.
This sounds like a crash or memory corruption bug?
But it doesn't have Fixes or Cc: stable. Is it not a major problem for
some reason?
cheers
^ permalink raw reply
* Re: [PATCH 02/13] powerpc/eeh: Add final message for successful recovery
From: Michael Ellerman @ 2018-05-04 2:55 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <e2cc8562dcce1d8bc38084bc45414c048b5a441d.1525242772.git.sbobroff@linux.ibm.com>
Sam Bobroff <sbobroff@linux.ibm.com> writes:
> Add a single log line at the end of successful EEH recovery, so that
> it's clear that event processing has finished.
>
> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> ---
> arch/powerpc/kernel/eeh_driver.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
> index 56a60b9eb397..07e0a42035ce 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -910,6 +910,7 @@ void eeh_handle_normal_event(struct eeh_pe *pe)
> pr_info("EEH: Notify device driver to resume\n");
> eeh_pe_dev_traverse(pe, eeh_report_resume, NULL);
>
> + pr_info("EEH: Recovery successful.\n");
Is it possible for recovery for multiple devices to be interleaved?
Should that message include the device?
cheers
^ permalink raw reply
* Re: [Skiboot] [PATCH 1/2] SLW: Remove stop1_lite and stop0 stop states
From: Michael Ellerman @ 2018-05-04 1:02 UTC (permalink / raw)
To: Stewart Smith, Nicholas Piggin, Akshay Adiga; +Cc: skiboot, linuxppc-dev
In-Reply-To: <87lgd13vjo.fsf@linux.vnet.ibm.com>
Stewart Smith <stewart@linux.vnet.ibm.com> writes:
...
>
> Slightly stupid question: should we be disabling these here or should
> Linux be better and deciding what states to use?
>
> I'm inclined to say this is a Linux problem as it should make the
> decision of what hardware feature to used based on the ones OPAL says
> *can* be used.
Yeah I agree.
Firmware shouldn't be implementing the policy around what states to use,
it should tell the operating system (which might be Linux) what states
are available and what their features are.
The exception to that would be that we have unfixable crash bugs in
existing kernels, in that case firmware might have to filter out states
that are known to cause those.
cheers
^ permalink raw reply
* Re: [PATCH 4/6] powerpc/64s: Enable barrier_nospec based on firmware settings
From: Michael Ellerman @ 2018-05-04 0:58 UTC (permalink / raw)
To: Michal Suchánek; +Cc: linuxppc-dev, linux-kernel, npiggin
In-Reply-To: <20180502134128.4a31b0fc@kitsune.suse.cz>
Michal Such=C3=A1nek <msuchanek@suse.de> writes:
> On Tue, 01 May 2018 21:11:06 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Michal Such=C3=A1nek <msuchanek@suse.de> writes:
>> > On Tue, 24 Apr 2018 14:15:57 +1000
>> > Michael Ellerman <mpe@ellerman.id.au> wrote:
>> >=20=20
>> >> From: Michal Suchanek <msuchanek@suse.de>
>> >>=20
>> >> Check what firmware told us and enable/disable the barrier_nospec
>> >> as appropriate.
>> >>=20
>> >> We err on the side of enabling the barrier, as it's no-op on older
>> >> systems, see the comment for more detail.
>> >>=20
>> >> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>=20=20
>> ...
>> >
>> > I am missing the option for the barrier to be disabled by a kernel
>> > commandline argument here.
>> >
>> > It does make sense to add a kernel parameter that is checked on
>> > boot to be compatible with other platforms that implement one.=20=20
>>=20
>> No other platforms have an option to disable variant 1 mitigations, so
>> there isn't an existing parameter we can use.
>
> Right, I was looking at an older implementation which turned off both
> v1 and v2 with same parameter. In current kernel the v1 mitigation is
> not turned off at all.
Ah OK.
>> Which is not to say we can't add one, but I wasn't sure if it was
>> really worth it.
>
> The current thinking is that most performance relevant cases are
> covered with array_nospec which has little overhead. The less code we
> have for this the better ;-)
Amen to that :)
cheers
^ permalink raw reply
* Re: [PATCH 1/6] powerpc/syscalls: Switch trivial cases to SYSCALL_DEFINE
From: Mathieu Malaterre @ 2018-05-03 18:51 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, Al Viro, LKML
In-Reply-To: <20180502132051.28861-1-mpe@ellerman.id.au>
On Wed, May 2, 2018 at 3:20 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
Maybe add:
Link: https://github.com/linuxppc/linux/issues/131
For the series: compile, build without error (W=1), and works on my system.
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> arch/powerpc/kernel/pci_32.c | 6 +++---
> arch/powerpc/kernel/pci_64.c | 4 ++--
> arch/powerpc/mm/subpage-prot.c | 4 +++-
> arch/powerpc/platforms/cell/spu_syscalls.c | 3 ++-
> 4 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
> index 85ad2f78b889..af36e46c3ed6 100644
> --- a/arch/powerpc/kernel/pci_32.c
> +++ b/arch/powerpc/kernel/pci_32.c
> @@ -16,6 +16,7 @@
> #include <linux/of.h>
> #include <linux/slab.h>
> #include <linux/export.h>
> +#include <linux/syscalls.h>
>
> #include <asm/processor.h>
> #include <asm/io.h>
> @@ -283,7 +284,8 @@ pci_bus_to_hose(int bus)
> * Note that the returned IO or memory base is a physical address
> */
>
> -long sys_pciconfig_iobase(long which, unsigned long bus, unsigned long devfn)
> +SYSCALL_DEFINE3(pciconfig_iobase, long, which,
> + unsigned long, bus, unsigned long, devfn)
> {
> struct pci_controller* hose;
> long result = -EOPNOTSUPP;
> @@ -307,5 +309,3 @@ long sys_pciconfig_iobase(long which, unsigned long bus, unsigned long devfn)
>
> return result;
> }
> -
> -
> diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
> index 15ce0306b092..dff28f903512 100644
> --- a/arch/powerpc/kernel/pci_64.c
> +++ b/arch/powerpc/kernel/pci_64.c
> @@ -203,8 +203,8 @@ void pcibios_setup_phb_io_space(struct pci_controller *hose)
> #define IOBASE_ISA_IO 3
> #define IOBASE_ISA_MEM 4
>
> -long sys_pciconfig_iobase(long which, unsigned long in_bus,
> - unsigned long in_devfn)
> +SYSCALL_DEFINE3(pciconfig_iobase, long, which, unsigned long, in_bus,
> + unsigned long, in_devfn)
> {
> struct pci_controller* hose;
> struct pci_bus *tmp_bus, *bus = NULL;
> diff --git a/arch/powerpc/mm/subpage-prot.c b/arch/powerpc/mm/subpage-prot.c
> index f14a07c2fb90..9d16ee251fc0 100644
> --- a/arch/powerpc/mm/subpage-prot.c
> +++ b/arch/powerpc/mm/subpage-prot.c
> @@ -13,6 +13,7 @@
> #include <linux/types.h>
> #include <linux/mm.h>
> #include <linux/hugetlb.h>
> +#include <linux/syscalls.h>
>
> #include <asm/pgtable.h>
> #include <linux/uaccess.h>
> @@ -185,7 +186,8 @@ static void subpage_mark_vma_nohuge(struct mm_struct *mm, unsigned long addr,
> * in a 2-bit field won't allow writes to a page that is otherwise
> * write-protected.
> */
> -long sys_subpage_prot(unsigned long addr, unsigned long len, u32 __user *map)
> +SYSCALL_DEFINE3(subpage_prot, unsigned long, addr,
> + unsigned long, len, u32 __user *, map)
> {
> struct mm_struct *mm = current->mm;
> struct subpage_prot_table *spt = &mm->context.spt;
> diff --git a/arch/powerpc/platforms/cell/spu_syscalls.c b/arch/powerpc/platforms/cell/spu_syscalls.c
> index 5e6e0bad6db6..263413a34823 100644
> --- a/arch/powerpc/platforms/cell/spu_syscalls.c
> +++ b/arch/powerpc/platforms/cell/spu_syscalls.c
> @@ -26,6 +26,7 @@
> #include <linux/syscalls.h>
> #include <linux/rcupdate.h>
> #include <linux/binfmts.h>
> +#include <linux/syscalls.h>
>
> #include <asm/spu.h>
>
> @@ -90,7 +91,7 @@ SYSCALL_DEFINE4(spu_create, const char __user *, name, unsigned int, flags,
> return ret;
> }
>
> -asmlinkage long sys_spu_run(int fd, __u32 __user *unpc, __u32 __user *ustatus)
> +SYSCALL_DEFINE3(spu_run,int, fd, __u32 __user *, unpc, __u32 __user *, ustatus)
> {
> long ret;
> struct fd arg;
> --
> 2.14.1
>
^ permalink raw reply
* Re: [PATCH v10 12/25] mm: cache some VMA fields in the vm_fault structure
From: Minchan Kim @ 2018-05-03 15:42 UTC (permalink / raw)
To: Laurent Dufour
Cc: akpm, mhocko, peterz, kirill, ak, dave, jack, Matthew Wilcox,
benh, mpe, paulus, Thomas Gleixner, Ingo Molnar, hpa, Will Deacon,
Sergey Senozhatsky, Andrea Arcangeli, Alexei Starovoitov,
kemi.wang, sergey.senozhatsky.work, Daniel Jordan, David Rientjes,
Jerome Glisse, Ganesh Mahendran, linux-kernel, linux-mm, haren,
khandual, npiggin, bsingharora, paulmck, Tim Chen, linuxppc-dev,
x86
In-Reply-To: <cd27f249-6c78-ccbb-c8f4-a8d8f7a3cd60@linux.vnet.ibm.com>
On Thu, May 03, 2018 at 02:25:18PM +0200, Laurent Dufour wrote:
> On 23/04/2018 09:42, Minchan Kim wrote:
> > On Tue, Apr 17, 2018 at 04:33:18PM +0200, Laurent Dufour wrote:
> >> When handling speculative page fault, the vma->vm_flags and
> >> vma->vm_page_prot fields are read once the page table lock is released. So
> >> there is no more guarantee that these fields would not change in our back
> >> They will be saved in the vm_fault structure before the VMA is checked for
> >> changes.
> >
> > Sorry. I cannot understand.
> > If it is changed under us, what happens? If it's critical, why cannot we
> > check with seqcounter?
> > Clearly, I'm not understanding the logic here. However, it's a global
> > change without CONFIG_SPF so I want to be more careful.
> > It would be better to describe why we need to sanpshot those values
> > into vm_fault rather than preventing the race.
>
> The idea is to go forward processing the page fault using the VMA's fields
> values saved in the vm_fault structure. Then once the pte are locked, the
> vma->sequence_counter is checked again and if something has changed in our back
> the speculative page fault processing is aborted.
Sorry, still I don't understand why we should capture some fields to vm_fault.
If we found vma->seq_cnt is changed under pte lock, can't we just bail out and
fallback to classic fault handling?
Maybe, I'm missing something clear now. It would be really helpful to understand
if you give some exmaple.
Thanks.
>
> Thanks,
> Laurent.
>
>
> >
> > Thanks.
> >
> >>
> >> This patch also set the fields in hugetlb_no_page() and
> >> __collapse_huge_page_swapin even if it is not need for the callee.
> >>
> >> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> >> ---
> >> include/linux/mm.h | 10 ++++++++--
> >> mm/huge_memory.c | 6 +++---
> >> mm/hugetlb.c | 2 ++
> >> mm/khugepaged.c | 2 ++
> >> mm/memory.c | 50 ++++++++++++++++++++++++++------------------------
> >> mm/migrate.c | 2 +-
> >> 6 files changed, 42 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/include/linux/mm.h b/include/linux/mm.h
> >> index f6edd15563bc..c65205c8c558 100644
> >> --- a/include/linux/mm.h
> >> +++ b/include/linux/mm.h
> >> @@ -367,6 +367,12 @@ struct vm_fault {
> >> * page table to avoid allocation from
> >> * atomic context.
> >> */
> >> + /*
> >> + * These entries are required when handling speculative page fault.
> >> + * This way the page handling is done using consistent field values.
> >> + */
> >> + unsigned long vma_flags;
> >> + pgprot_t vma_page_prot;
> >> };
> >>
> >> /* page entry size for vm->huge_fault() */
> >> @@ -687,9 +693,9 @@ void free_compound_page(struct page *page);
> >> * pte_mkwrite. But get_user_pages can cause write faults for mappings
> >> * that do not have writing enabled, when used by access_process_vm.
> >> */
> >> -static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
> >> +static inline pte_t maybe_mkwrite(pte_t pte, unsigned long vma_flags)
> >> {
> >> - if (likely(vma->vm_flags & VM_WRITE))
> >> + if (likely(vma_flags & VM_WRITE))
> >> pte = pte_mkwrite(pte);
> >> return pte;
> >> }
> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >> index a3a1815f8e11..da2afda67e68 100644
> >> --- a/mm/huge_memory.c
> >> +++ b/mm/huge_memory.c
> >> @@ -1194,8 +1194,8 @@ static int do_huge_pmd_wp_page_fallback(struct vm_fault *vmf, pmd_t orig_pmd,
> >>
> >> for (i = 0; i < HPAGE_PMD_NR; i++, haddr += PAGE_SIZE) {
> >> pte_t entry;
> >> - entry = mk_pte(pages[i], vma->vm_page_prot);
> >> - entry = maybe_mkwrite(pte_mkdirty(entry), vma);
> >> + entry = mk_pte(pages[i], vmf->vma_page_prot);
> >> + entry = maybe_mkwrite(pte_mkdirty(entry), vmf->vma_flags);
> >> memcg = (void *)page_private(pages[i]);
> >> set_page_private(pages[i], 0);
> >> page_add_new_anon_rmap(pages[i], vmf->vma, haddr, false);
> >> @@ -2168,7 +2168,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> >> entry = pte_swp_mksoft_dirty(entry);
> >> } else {
> >> entry = mk_pte(page + i, READ_ONCE(vma->vm_page_prot));
> >> - entry = maybe_mkwrite(entry, vma);
> >> + entry = maybe_mkwrite(entry, vma->vm_flags);
> >> if (!write)
> >> entry = pte_wrprotect(entry);
> >> if (!young)
> >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> >> index 218679138255..774864153407 100644
> >> --- a/mm/hugetlb.c
> >> +++ b/mm/hugetlb.c
> >> @@ -3718,6 +3718,8 @@ static int hugetlb_no_page(struct mm_struct *mm, struct vm_area_struct *vma,
> >> .vma = vma,
> >> .address = address,
> >> .flags = flags,
> >> + .vma_flags = vma->vm_flags,
> >> + .vma_page_prot = vma->vm_page_prot,
> >> /*
> >> * Hard to debug if it ends up being
> >> * used by a callee that assumes
> >> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> >> index 0b28af4b950d..2b02a9f9589e 100644
> >> --- a/mm/khugepaged.c
> >> +++ b/mm/khugepaged.c
> >> @@ -887,6 +887,8 @@ static bool __collapse_huge_page_swapin(struct mm_struct *mm,
> >> .flags = FAULT_FLAG_ALLOW_RETRY,
> >> .pmd = pmd,
> >> .pgoff = linear_page_index(vma, address),
> >> + .vma_flags = vma->vm_flags,
> >> + .vma_page_prot = vma->vm_page_prot,
> >> };
> >>
> >> /* we only decide to swapin, if there is enough young ptes */
> >> diff --git a/mm/memory.c b/mm/memory.c
> >> index f76f5027d251..2fb9920e06a5 100644
> >> --- a/mm/memory.c
> >> +++ b/mm/memory.c
> >> @@ -1826,7 +1826,7 @@ static int insert_pfn(struct vm_area_struct *vma, unsigned long addr,
> >> out_mkwrite:
> >> if (mkwrite) {
> >> entry = pte_mkyoung(entry);
> >> - entry = maybe_mkwrite(pte_mkdirty(entry), vma);
> >> + entry = maybe_mkwrite(pte_mkdirty(entry), vma->vm_flags);
> >> }
> >>
> >> set_pte_at(mm, addr, pte, entry);
> >> @@ -2472,7 +2472,7 @@ static inline void wp_page_reuse(struct vm_fault *vmf)
> >>
> >> flush_cache_page(vma, vmf->address, pte_pfn(vmf->orig_pte));
> >> entry = pte_mkyoung(vmf->orig_pte);
> >> - entry = maybe_mkwrite(pte_mkdirty(entry), vma);
> >> + entry = maybe_mkwrite(pte_mkdirty(entry), vmf->vma_flags);
> >> if (ptep_set_access_flags(vma, vmf->address, vmf->pte, entry, 1))
> >> update_mmu_cache(vma, vmf->address, vmf->pte);
> >> pte_unmap_unlock(vmf->pte, vmf->ptl);
> >> @@ -2548,8 +2548,8 @@ static int wp_page_copy(struct vm_fault *vmf)
> >> inc_mm_counter_fast(mm, MM_ANONPAGES);
> >> }
> >> flush_cache_page(vma, vmf->address, pte_pfn(vmf->orig_pte));
> >> - entry = mk_pte(new_page, vma->vm_page_prot);
> >> - entry = maybe_mkwrite(pte_mkdirty(entry), vma);
> >> + entry = mk_pte(new_page, vmf->vma_page_prot);
> >> + entry = maybe_mkwrite(pte_mkdirty(entry), vmf->vma_flags);
> >> /*
> >> * Clear the pte entry and flush it first, before updating the
> >> * pte with the new entry. This will avoid a race condition
> >> @@ -2614,7 +2614,7 @@ static int wp_page_copy(struct vm_fault *vmf)
> >> * Don't let another task, with possibly unlocked vma,
> >> * keep the mlocked page.
> >> */
> >> - if (page_copied && (vma->vm_flags & VM_LOCKED)) {
> >> + if (page_copied && (vmf->vma_flags & VM_LOCKED)) {
> >> lock_page(old_page); /* LRU manipulation */
> >> if (PageMlocked(old_page))
> >> munlock_vma_page(old_page);
> >> @@ -2650,7 +2650,7 @@ static int wp_page_copy(struct vm_fault *vmf)
> >> */
> >> int finish_mkwrite_fault(struct vm_fault *vmf)
> >> {
> >> - WARN_ON_ONCE(!(vmf->vma->vm_flags & VM_SHARED));
> >> + WARN_ON_ONCE(!(vmf->vma_flags & VM_SHARED));
> >> if (!pte_map_lock(vmf))
> >> return VM_FAULT_RETRY;
> >> /*
> >> @@ -2752,7 +2752,7 @@ static int do_wp_page(struct vm_fault *vmf)
> >> * We should not cow pages in a shared writeable mapping.
> >> * Just mark the pages writable and/or call ops->pfn_mkwrite.
> >> */
> >> - if ((vma->vm_flags & (VM_WRITE|VM_SHARED)) ==
> >> + if ((vmf->vma_flags & (VM_WRITE|VM_SHARED)) ==
> >> (VM_WRITE|VM_SHARED))
> >> return wp_pfn_shared(vmf);
> >>
> >> @@ -2799,7 +2799,7 @@ static int do_wp_page(struct vm_fault *vmf)
> >> return VM_FAULT_WRITE;
> >> }
> >> unlock_page(vmf->page);
> >> - } else if (unlikely((vma->vm_flags & (VM_WRITE|VM_SHARED)) ==
> >> + } else if (unlikely((vmf->vma_flags & (VM_WRITE|VM_SHARED)) ==
> >> (VM_WRITE|VM_SHARED))) {
> >> return wp_page_shared(vmf);
> >> }
> >> @@ -3078,9 +3078,9 @@ int do_swap_page(struct vm_fault *vmf)
> >>
> >> inc_mm_counter_fast(vma->vm_mm, MM_ANONPAGES);
> >> dec_mm_counter_fast(vma->vm_mm, MM_SWAPENTS);
> >> - pte = mk_pte(page, vma->vm_page_prot);
> >> + pte = mk_pte(page, vmf->vma_page_prot);
> >> if ((vmf->flags & FAULT_FLAG_WRITE) && reuse_swap_page(page, NULL)) {
> >> - pte = maybe_mkwrite(pte_mkdirty(pte), vma);
> >> + pte = maybe_mkwrite(pte_mkdirty(pte), vmf->vma_flags);
> >> vmf->flags &= ~FAULT_FLAG_WRITE;
> >> ret |= VM_FAULT_WRITE;
> >> exclusive = RMAP_EXCLUSIVE;
> >> @@ -3105,7 +3105,7 @@ int do_swap_page(struct vm_fault *vmf)
> >>
> >> swap_free(entry);
> >> if (mem_cgroup_swap_full(page) ||
> >> - (vma->vm_flags & VM_LOCKED) || PageMlocked(page))
> >> + (vmf->vma_flags & VM_LOCKED) || PageMlocked(page))
> >> try_to_free_swap(page);
> >> unlock_page(page);
> >> if (page != swapcache && swapcache) {
> >> @@ -3163,7 +3163,7 @@ static int do_anonymous_page(struct vm_fault *vmf)
> >> pte_t entry;
> >>
> >> /* File mapping without ->vm_ops ? */
> >> - if (vma->vm_flags & VM_SHARED)
> >> + if (vmf->vma_flags & VM_SHARED)
> >> return VM_FAULT_SIGBUS;
> >>
> >> /*
> >> @@ -3187,7 +3187,7 @@ static int do_anonymous_page(struct vm_fault *vmf)
> >> if (!(vmf->flags & FAULT_FLAG_WRITE) &&
> >> !mm_forbids_zeropage(vma->vm_mm)) {
> >> entry = pte_mkspecial(pfn_pte(my_zero_pfn(vmf->address),
> >> - vma->vm_page_prot));
> >> + vmf->vma_page_prot));
> >> if (!pte_map_lock(vmf))
> >> return VM_FAULT_RETRY;
> >> if (!pte_none(*vmf->pte))
> >> @@ -3220,8 +3220,8 @@ static int do_anonymous_page(struct vm_fault *vmf)
> >> */
> >> __SetPageUptodate(page);
> >>
> >> - entry = mk_pte(page, vma->vm_page_prot);
> >> - if (vma->vm_flags & VM_WRITE)
> >> + entry = mk_pte(page, vmf->vma_page_prot);
> >> + if (vmf->vma_flags & VM_WRITE)
> >> entry = pte_mkwrite(pte_mkdirty(entry));
> >>
> >> if (!pte_map_lock(vmf)) {
> >> @@ -3418,7 +3418,7 @@ static int do_set_pmd(struct vm_fault *vmf, struct page *page)
> >> for (i = 0; i < HPAGE_PMD_NR; i++)
> >> flush_icache_page(vma, page + i);
> >>
> >> - entry = mk_huge_pmd(page, vma->vm_page_prot);
> >> + entry = mk_huge_pmd(page, vmf->vma_page_prot);
> >> if (write)
> >> entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
> >>
> >> @@ -3492,11 +3492,11 @@ int alloc_set_pte(struct vm_fault *vmf, struct mem_cgroup *memcg,
> >> return VM_FAULT_NOPAGE;
> >>
> >> flush_icache_page(vma, page);
> >> - entry = mk_pte(page, vma->vm_page_prot);
> >> + entry = mk_pte(page, vmf->vma_page_prot);
> >> if (write)
> >> - entry = maybe_mkwrite(pte_mkdirty(entry), vma);
> >> + entry = maybe_mkwrite(pte_mkdirty(entry), vmf->vma_flags);
> >> /* copy-on-write page */
> >> - if (write && !(vma->vm_flags & VM_SHARED)) {
> >> + if (write && !(vmf->vma_flags & VM_SHARED)) {
> >> inc_mm_counter_fast(vma->vm_mm, MM_ANONPAGES);
> >> page_add_new_anon_rmap(page, vma, vmf->address, false);
> >> mem_cgroup_commit_charge(page, memcg, false, false);
> >> @@ -3535,7 +3535,7 @@ int finish_fault(struct vm_fault *vmf)
> >>
> >> /* Did we COW the page? */
> >> if ((vmf->flags & FAULT_FLAG_WRITE) &&
> >> - !(vmf->vma->vm_flags & VM_SHARED))
> >> + !(vmf->vma_flags & VM_SHARED))
> >> page = vmf->cow_page;
> >> else
> >> page = vmf->page;
> >> @@ -3789,7 +3789,7 @@ static int do_fault(struct vm_fault *vmf)
> >> ret = VM_FAULT_SIGBUS;
> >> else if (!(vmf->flags & FAULT_FLAG_WRITE))
> >> ret = do_read_fault(vmf);
> >> - else if (!(vma->vm_flags & VM_SHARED))
> >> + else if (!(vmf->vma_flags & VM_SHARED))
> >> ret = do_cow_fault(vmf);
> >> else
> >> ret = do_shared_fault(vmf);
> >> @@ -3846,7 +3846,7 @@ static int do_numa_page(struct vm_fault *vmf)
> >> * accessible ptes, some can allow access by kernel mode.
> >> */
> >> pte = ptep_modify_prot_start(vma->vm_mm, vmf->address, vmf->pte);
> >> - pte = pte_modify(pte, vma->vm_page_prot);
> >> + pte = pte_modify(pte, vmf->vma_page_prot);
> >> pte = pte_mkyoung(pte);
> >> if (was_writable)
> >> pte = pte_mkwrite(pte);
> >> @@ -3880,7 +3880,7 @@ static int do_numa_page(struct vm_fault *vmf)
> >> * Flag if the page is shared between multiple address spaces. This
> >> * is later used when determining whether to group tasks together
> >> */
> >> - if (page_mapcount(page) > 1 && (vma->vm_flags & VM_SHARED))
> >> + if (page_mapcount(page) > 1 && (vmf->vma_flags & VM_SHARED))
> >> flags |= TNF_SHARED;
> >>
> >> last_cpupid = page_cpupid_last(page);
> >> @@ -3925,7 +3925,7 @@ static inline int wp_huge_pmd(struct vm_fault *vmf, pmd_t orig_pmd)
> >> return vmf->vma->vm_ops->huge_fault(vmf, PE_SIZE_PMD);
> >>
> >> /* COW handled on pte level: split pmd */
> >> - VM_BUG_ON_VMA(vmf->vma->vm_flags & VM_SHARED, vmf->vma);
> >> + VM_BUG_ON_VMA(vmf->vma_flags & VM_SHARED, vmf->vma);
> >> __split_huge_pmd(vmf->vma, vmf->pmd, vmf->address, false, NULL);
> >>
> >> return VM_FAULT_FALLBACK;
> >> @@ -4072,6 +4072,8 @@ static int __handle_mm_fault(struct vm_area_struct *vma, unsigned long address,
> >> .flags = flags,
> >> .pgoff = linear_page_index(vma, address),
> >> .gfp_mask = __get_fault_gfp_mask(vma),
> >> + .vma_flags = vma->vm_flags,
> >> + .vma_page_prot = vma->vm_page_prot,
> >> };
> >> unsigned int dirty = flags & FAULT_FLAG_WRITE;
> >> struct mm_struct *mm = vma->vm_mm;
> >> diff --git a/mm/migrate.c b/mm/migrate.c
> >> index bb6367d70a3e..44d7007cfc1c 100644
> >> --- a/mm/migrate.c
> >> +++ b/mm/migrate.c
> >> @@ -240,7 +240,7 @@ static bool remove_migration_pte(struct page *page, struct vm_area_struct *vma,
> >> */
> >> entry = pte_to_swp_entry(*pvmw.pte);
> >> if (is_write_migration_entry(entry))
> >> - pte = maybe_mkwrite(pte, vma);
> >> + pte = maybe_mkwrite(pte, vma->vm_flags);
> >>
> >> if (unlikely(is_zone_device_page(new))) {
> >> if (is_device_private_page(new)) {
> >> --
> >> 2.7.4
> >>
> >
>
^ permalink raw reply
* Re: [RFC][PATCH bpf] tools: bpftool: Fix tags for bpf-to-bpf calls
From: Naveen N. Rao @ 2018-05-03 15:20 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Sandipan Das
Cc: jakub.kicinski, linuxppc-dev, netdev, Michael Ellerman
In-Reply-To: <415b415e-f47f-082c-1bc9-87d3e9d3aed1__9575.16645216874$1520270545$gmane$org@ fb.com>
Alexei Starovoitov wrote:
> On 3/1/18 12:51 AM, Naveen N. Rao wrote:
>> Daniel Borkmann wrote:
>>>
>>> Worst case if there's nothing better, potentially what one could do in
>>> bpf_prog_get_info_by_fd() is to dump an array of full addresses and
>>> have the imm part as the index pointing to one of them, just unfortunat=
e
>>> that it's likely only needed in ppc64.
>>
>> Ok. We seem to have discussed a few different aspects in this thread.
>> Let me summarize the different aspects we have discussed:
>> 1. Passing address of JIT'ed function to the JIT engines:
>> Two approaches discussed:
>> a. Existing approach, where the subprog address is encoded as an
>> offset from __bpf_call_base() in imm32 field of the BPF call
>> instruction. This requires the JIT'ed function to be within 2GB of
>> __bpf_call_base(), which won't be true on ppc64, at the least. So,
>> this won't on ppc64 (and any other architectures where vmalloc'ed
>> (module_alloc()) memory is from a different, far, address range).
>=20
> it looks like ppc64 doesn't guarantee today that all of module_alloc()
> will be within 32-bit, but I think it should be trivial to add such
> guarantee. If so, we can define another __bpf_call_base specifically
> for bpf-to-bpf calls when jit is on.
Ok, we prefer not to do that for powerpc (atleast, not for all of=20
module_alloc()) at this point.
And since option (c) below is not preferable, I think we will implement=20
what Daniel suggested above. This patchset already handles communicating=20
the BPF function addresses to the JIT engine, and enhancing=20
bpf_prog_get_info_by_fd() should address the concerns with bpftool.
- Naveen
=
^ permalink raw reply
* Re: [PATCH v10 24/25] x86/mm: add speculative pagefault handling
From: Laurent Dufour @ 2018-05-03 14:59 UTC (permalink / raw)
To: Punit Agrawal
Cc: akpm, mhocko, peterz, kirill, ak, dave, jack, Matthew Wilcox,
benh, mpe, paulus, Thomas Gleixner, Ingo Molnar, hpa, Will Deacon,
Sergey Senozhatsky, Andrea Arcangeli, Alexei Starovoitov,
kemi.wang, sergey.senozhatsky.work, Daniel Jordan, David Rientjes,
Jerome Glisse, Ganesh Mahendran, linux-kernel, linux-mm, haren,
khandual, npiggin, bsingharora, paulmck, Tim Chen, linuxppc-dev,
x86
In-Reply-To: <CAD4BONd5DZiKkGPGaYqEcVb+YubVDy43MNNQ8_yztDHWpf0Y7w@mail.gmail.com>
On 30/04/2018 20:43, Punit Agrawal wrote:
> Hi Laurent,
>
> I am looking to add support for speculative page fault handling to
> arm64 (effectively porting this patch) and had a few questions.
> Apologies if I've missed an obvious explanation for my queries. I'm
> jumping in bit late to the discussion.
Hi Punit,
Thanks for giving this series a review.
I don't have arm64 hardware to play with, but I'll be happy to add arm64
patches to my series and to try to maintain them.
>
> On Tue, Apr 17, 2018 at 3:33 PM, Laurent Dufour
> <ldufour@linux.vnet.ibm.com> wrote:
>> From: Peter Zijlstra <peterz@infradead.org>
>>
>> Try a speculative fault before acquiring mmap_sem, if it returns with
>> VM_FAULT_RETRY continue with the mmap_sem acquisition and do the
>> traditional fault.
>>
>> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>>
>> [Clearing of FAULT_FLAG_ALLOW_RETRY is now done in
>> handle_speculative_fault()]
>> [Retry with usual fault path in the case VM_ERROR is returned by
>> handle_speculative_fault(). This allows signal to be delivered]
>> [Don't build SPF call if !CONFIG_SPECULATIVE_PAGE_FAULT]
>> [Try speculative fault path only for multi threaded processes]
>> [Try reuse to the VMA fetch during the speculative path in case of retry]
>> [Call reuse_spf_or_find_vma()]
>> [Handle memory protection key fault]
>> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>> ---
>> arch/x86/mm/fault.c | 42 ++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 38 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>> index 73bd8c95ac71..59f778386df5 100644
>> --- a/arch/x86/mm/fault.c
>> +++ b/arch/x86/mm/fault.c
>> @@ -1220,7 +1220,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
>> struct mm_struct *mm;
>> int fault, major = 0;
>> unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
>> - u32 pkey;
>> + u32 pkey, *pt_pkey = &pkey;
>>
>> tsk = current;
>> mm = tsk->mm;
>> @@ -1310,6 +1310,30 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
>> flags |= FAULT_FLAG_INSTRUCTION;
>>
>> /*
>> + * Do not try speculative page fault for kernel's pages and if
>> + * the fault was due to protection keys since it can't be resolved.
>> + */
>> + if (IS_ENABLED(CONFIG_SPECULATIVE_PAGE_FAULT) &&
>> + !(error_code & X86_PF_PK)) {
>
> You can simplify this condition by dropping the IS_ENABLED() check as
> you already provide an alternate implementation of
> handle_speculative_fault() when CONFIG_SPECULATIVE_PAGE_FAULT is not
> defined.
Yes you're right, I completely forgot about that define of
handle_speculative_fault() when CONFIG_SPECULATIVE_PAGE_FAULT is not set, that
will definitively makes that part of code more readable.
>
>> + fault = handle_speculative_fault(mm, address, flags, &vma);
>> + if (fault != VM_FAULT_RETRY) {
>> + perf_sw_event(PERF_COUNT_SW_SPF, 1, regs, address);
>> + /*
>> + * Do not advertise for the pkey value since we don't
>> + * know it.
>> + * This is not a matter as we checked for X86_PF_PK
>> + * earlier, so we should not handle pkey fault here,
>> + * but to be sure that mm_fault_error() callees will
>> + * not try to use it, we invalidate the pointer.
>> + */
>> + pt_pkey = NULL;
>> + goto done;
>> + }
>> + } else {
>> + vma = NULL;
>> + }
>
> The else part can be dropped if vma is initialised to NULL when it is
> declared at the top of the function.
Sure.
>
>> +
>> + /*
>> * When running in the kernel we expect faults to occur only to
>> * addresses in user space. All other faults represent errors in
>> * the kernel and should generate an OOPS. Unfortunately, in the
>> @@ -1342,7 +1366,8 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
>> might_sleep();
>> }
>>
>> - vma = find_vma(mm, address);
>> + if (!vma || !can_reuse_spf_vma(vma, address))
>> + vma = find_vma(mm, address);
>
> Is there a measurable benefit from reusing the vma?
>
> Dropping the vma reference unconditionally after speculative page
> fault handling gets rid of the implicit state when "vma != NULL"
> (increased ref-count). I found it a bit confusing to follow.
I do agree, this is quite confusing. My initial goal was to be able to reuse
the VMA in the case a protection key error was detected, but it's not really
necessary on x86 since we know at the beginning of the fault operation that
protection key are in the loop. This is not the case on ppc64 but I couldn't
find a way to easily rely on the speculatively fetched VMA neither, so for
protection keys, this didn't help.
Regarding the measurable benefit of reusing the fetched vma, I did further
tests using will-it-scale/page_fault2_threads test, and I'm no more really
convince that this worth the added complexity. I think I'll drop the patch "mm:
speculative page fault handler return VMA" of the series, and thus remove the
call to can_reuse_spf_vma().
Thanks,
Laurent.
>
>> if (unlikely(!vma)) {
>> bad_area(regs, error_code, address);
>> return;
>> @@ -1409,8 +1434,15 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
>> if (flags & FAULT_FLAG_ALLOW_RETRY) {
>> flags &= ~FAULT_FLAG_ALLOW_RETRY;
>> flags |= FAULT_FLAG_TRIED;
>> - if (!fatal_signal_pending(tsk))
>> + if (!fatal_signal_pending(tsk)) {
>> + /*
>> + * Do not try to reuse this vma and fetch it
>> + * again since we will release the mmap_sem.
>> + */
>> + if (IS_ENABLED(CONFIG_SPECULATIVE_PAGE_FAULT))
>> + vma = NULL;
>
> Regardless of the above comment, can the vma be reset here unconditionally?
>
> Thanks,
> Punit
>
^ permalink raw reply
* Re: [PATCH v10 08/25] mm: VMA sequence count
From: Laurent Dufour @ 2018-05-03 14:45 UTC (permalink / raw)
To: Minchan Kim
Cc: akpm, mhocko, peterz, kirill, ak, dave, jack, Matthew Wilcox,
benh, mpe, paulus, Thomas Gleixner, Ingo Molnar, hpa, Will Deacon,
Sergey Senozhatsky, Andrea Arcangeli, Alexei Starovoitov,
kemi.wang, sergey.senozhatsky.work, Daniel Jordan, David Rientjes,
Jerome Glisse, Ganesh Mahendran, linux-kernel, linux-mm, haren,
khandual, npiggin, bsingharora, paulmck, Tim Chen, linuxppc-dev,
x86
In-Reply-To: <20180501131646.GB118722@rodete-laptop-imager.corp.google.com>
On 01/05/2018 15:16, Minchan Kim wrote:
> On Mon, Apr 30, 2018 at 05:14:27PM +0200, Laurent Dufour wrote:
>>
>>
>> On 23/04/2018 08:42, Minchan Kim wrote:
>>> On Tue, Apr 17, 2018 at 04:33:14PM +0200, Laurent Dufour wrote:
>>>> From: Peter Zijlstra <peterz@infradead.org>
>>>>
>>>> Wrap the VMA modifications (vma_adjust/unmap_page_range) with sequence
>>>> counts such that we can easily test if a VMA is changed.
>>>
>>> So, seqcount is to protect modifying all attributes of vma?
>>
>> The seqcount is used to protect fields that will be used during the speculative
>> page fault like boundaries, protections.
>
> a VMA is changed, it was rather vague to me at this point.
> If you could specify detail fields or some example what seqcount aim for,
> it would help to review.
Got it, I'll try to make it more explicit in the commit message, mentioning
which fields are used in the speculative path and to be protected using the VMA
sequence counter.
>
>>
>>>>
>>>> The unmap_page_range() one allows us to make assumptions about
>>>> page-tables; when we find the seqcount hasn't changed we can assume
>>>> page-tables are still valid.
>>>
>>> Hmm, seqcount covers page-table, too.
>>> Please describe what the seqcount want to protect.
>>
>> The calls to vm_write_begin/end() in unmap_page_range() are used to detect when
>> a VMA is being unmap and thus that new page fault should not be satisfied for
>> this VMA. This is protecting the VMA unmapping operation, not the page tables
>> themselves.
>
> Thanks for the detail. yes, please include this phrase instead of "page-table
> are still valid". It makes me confused.
Sure, will do.
>
>>
>>>>
>>>> The flip side is that we cannot distinguish between a vma_adjust() and
>>>> the unmap_page_range() -- where with the former we could have
>>>> re-checked the vma bounds against the address.
>>>>
>>>> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>>>>
>>>> [Port to 4.12 kernel]
>>>> [Build depends on CONFIG_SPECULATIVE_PAGE_FAULT]
>>>> [Introduce vm_write_* inline function depending on
>>>> CONFIG_SPECULATIVE_PAGE_FAULT]
>>>> [Fix lock dependency between mapping->i_mmap_rwsem and vma->vm_sequence by
>>>> using vm_raw_write* functions]
>>>> [Fix a lock dependency warning in mmap_region() when entering the error
>>>> path]
>>>> [move sequence initialisation INIT_VMA()]
>>>> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>>>> ---
>>>> include/linux/mm.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>>> include/linux/mm_types.h | 3 +++
>>>> mm/memory.c | 2 ++
>>>> mm/mmap.c | 31 +++++++++++++++++++++++++++++++
>>>> 4 files changed, 80 insertions(+)
>>>>
>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>> index efc1248b82bd..988daf7030c9 100644
>>>> --- a/include/linux/mm.h
>>>> +++ b/include/linux/mm.h
>>>> @@ -1264,6 +1264,9 @@ struct zap_details {
>>>> static inline void INIT_VMA(struct vm_area_struct *vma)
>>>> {
>>>> INIT_LIST_HEAD(&vma->anon_vma_chain);
>>>> +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT
>>>> + seqcount_init(&vma->vm_sequence);
>>>> +#endif
>>>> }
>>>>
>>>> struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
>>>> @@ -1386,6 +1389,47 @@ static inline void unmap_shared_mapping_range(struct address_space *mapping,
>>>> unmap_mapping_range(mapping, holebegin, holelen, 0);
>>>> }
>>>>
>>>> +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT
>>>> +static inline void vm_write_begin(struct vm_area_struct *vma)
>>>> +{
>>>> + write_seqcount_begin(&vma->vm_sequence);
>>>> +}
>>>> +static inline void vm_write_begin_nested(struct vm_area_struct *vma,
>>>> + int subclass)
>>>> +{
>>>> + write_seqcount_begin_nested(&vma->vm_sequence, subclass);
>>>> +}
>>>> +static inline void vm_write_end(struct vm_area_struct *vma)
>>>> +{
>>>> + write_seqcount_end(&vma->vm_sequence);
>>>> +}
>>>> +static inline void vm_raw_write_begin(struct vm_area_struct *vma)
>>>> +{
>>>> + raw_write_seqcount_begin(&vma->vm_sequence);
>>>> +}
>>>> +static inline void vm_raw_write_end(struct vm_area_struct *vma)
>>>> +{
>>>> + raw_write_seqcount_end(&vma->vm_sequence);
>>>> +}
>>>> +#else
>>>> +static inline void vm_write_begin(struct vm_area_struct *vma)
>>>> +{
>>>> +}
>>>> +static inline void vm_write_begin_nested(struct vm_area_struct *vma,
>>>> + int subclass)
>>>> +{
>>>> +}
>>>> +static inline void vm_write_end(struct vm_area_struct *vma)
>>>> +{
>>>> +}
>>>> +static inline void vm_raw_write_begin(struct vm_area_struct *vma)
>>>> +{
>>>> +}
>>>> +static inline void vm_raw_write_end(struct vm_area_struct *vma)
>>>> +{
>>>> +}
>>>> +#endif /* CONFIG_SPECULATIVE_PAGE_FAULT */
>>>> +
>>>> extern int access_process_vm(struct task_struct *tsk, unsigned long addr,
>>>> void *buf, int len, unsigned int gup_flags);
>>>> extern int access_remote_vm(struct mm_struct *mm, unsigned long addr,
>>>> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
>>>> index 21612347d311..db5e9d630e7a 100644
>>>> --- a/include/linux/mm_types.h
>>>> +++ b/include/linux/mm_types.h
>>>> @@ -335,6 +335,9 @@ struct vm_area_struct {
>>>> struct mempolicy *vm_policy; /* NUMA policy for the VMA */
>>>> #endif
>>>> struct vm_userfaultfd_ctx vm_userfaultfd_ctx;
>>>> +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT
>>>> + seqcount_t vm_sequence;
>>>> +#endif
>>>> } __randomize_layout;
>>>>
>>>> struct core_thread {
>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>> index f86efcb8e268..f7fed053df80 100644
>>>> --- a/mm/memory.c
>>>> +++ b/mm/memory.c
>>>> @@ -1503,6 +1503,7 @@ void unmap_page_range(struct mmu_gather *tlb,
>>>> unsigned long next;
>>>>
>>>> BUG_ON(addr >= end);
>>>
>>> The comment about saying it aims for page-table stability will help.
>>
>> A comment may be added mentioning that we use the seqcount to indicate that the
>> VMA is modified, being unmapped. But there is not a real page table protection,
>> and I think this may be confusing to talk about page table stability here.
>
> Okay, so here you mean seqcount is not protecting VMA's fields but vma unmap
> operation like you mentioned above. I was confused like below description.
>
> "The unmap_page_range() one allows us to make assumptions about
> page-tables; when we find the seqcount hasn't changed we can assume
> page-tables are still valid"
>
> Instead of using page-tables's validness in descriptoin, it would be better
> to use scenario you mentioned about VMA unmap operation and page fault race.
Ok will do that.
Thanks.
^ permalink raw reply
* Re: [PATCH] perf tools: allow overriding MAX_NR_CPUS at compile time
From: Arnaldo Carvalho de Melo @ 2018-05-03 13:40 UTC (permalink / raw)
To: Christophe Leroy
Cc: Peter Zijlstra, Ingo Molnar, Alexander Shishkin, linux-kernel,
linuxppc-dev
In-Reply-To: <20170922112043.8349468C57@po15668-vm-win7.idsi0.si.c-s.fr>
Em Fri, Sep 22, 2017 at 01:20:43PM +0200, Christophe Leroy escreveu:
> After update of kernel, perf tool doesn't run anymore on my
> 32MB RAM powerpc board, but still runs on a 128MB RAM board:
Cleaning up my inbox, found this one, simple enough, still applies,
applied.
These all needs to be dynamicly allocated, but still, with this one can
get a functioning tool, apply it.
- Arnaldo
> ~# strace perf
> execve("/usr/sbin/perf", ["perf"], [/* 12 vars */]) = -1 ENOMEM (Cannot allocate memory)
> --- SIGSEGV {si_signo=SIGSEGV, si_code=SI_KERNEL, si_addr=0} ---
> +++ killed by SIGSEGV +++
> Segmentation fault
>
> objdump -x shows that .bss section has a huge size of 24Mbytes:
>
> 27 .bss 016baca8 101cebb8 101cebb8 001cd988 2**3
>
> With especially the following objects having quite big size
>
> 10205f80 l O .bss 00140000 runtime_cycles_stats
> 10345f80 l O .bss 00140000 runtime_stalled_cycles_front_stats
> 10485f80 l O .bss 00140000 runtime_stalled_cycles_back_stats
> 105c5f80 l O .bss 00140000 runtime_branches_stats
> 10705f80 l O .bss 00140000 runtime_cacherefs_stats
> 10845f80 l O .bss 00140000 runtime_l1_dcache_stats
> 10985f80 l O .bss 00140000 runtime_l1_icache_stats
> 10ac5f80 l O .bss 00140000 runtime_ll_cache_stats
> 10c05f80 l O .bss 00140000 runtime_itlb_cache_stats
> 10d45f80 l O .bss 00140000 runtime_dtlb_cache_stats
> 10e85f80 l O .bss 00140000 runtime_cycles_in_tx_stats
> 10fc5f80 l O .bss 00140000 runtime_transaction_stats
> 11105f80 l O .bss 00140000 runtime_elision_stats
> 11245f80 l O .bss 00140000 runtime_topdown_total_slots
> 11385f80 l O .bss 00140000 runtime_topdown_slots_retired
> 114c5f80 l O .bss 00140000 runtime_topdown_slots_issued
> 11605f80 l O .bss 00140000 runtime_topdown_fetch_bubbles
> 11745f80 l O .bss 00140000 runtime_topdown_recovery_bubbles
>
> This is due to commit 4d255766d28b1 ("perf: Bump max number of cpus
> to 1024"), because many tables are sized with MAX_NR_CPUS
>
> This patch gives the opportunity to redefine MAX_NR_CPUS via
>
> make EXTRA_CFLAGS=-DMAX_NR_CPUS=1
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> tools/perf/perf.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tools/perf/perf.h b/tools/perf/perf.h
> index dc442ba21bf6..a9db563da0a9 100644
> --- a/tools/perf/perf.h
> +++ b/tools/perf/perf.h
> @@ -23,7 +23,9 @@ static inline unsigned long long rdclock(void)
> return ts.tv_sec * 1000000000ULL + ts.tv_nsec;
> }
>
> +#ifndef MAX_NR_CPUS
> #define MAX_NR_CPUS 1024
> +#endif
>
> extern const char *input_name;
> extern bool perf_host, perf_guest;
> --
> 2.13.3
^ permalink raw reply
* Re: [PATCH 13/13] powerpc/eeh: Refactor report functions
From: Michael Ellerman @ 2018-05-03 13:27 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <313ced0e1c4f5140566846bb41f1c42c26c96a0b.1525242772.git.sbobroff@linux.ibm.com>
Sam Bobroff <sbobroff@linux.ibm.com> writes:
> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
> index eb4feee81ff4..1c4336dcf9f5 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -54,6 +54,25 @@ static int eeh_result_priority(enum pci_ers_result result)
> }
> };
>
> +const char *pci_ers_result_name(enum pci_ers_result r)
> +{
> + switch (r) {
> + case PCI_ERS_RESULT_NONE: return "none";
> + case PCI_ERS_RESULT_CAN_RECOVER: return "can recover";
> + case PCI_ERS_RESULT_NEED_RESET: return "need reset";
> + case PCI_ERS_RESULT_DISCONNECT: return "disconnect";
> + case PCI_ERS_RESULT_RECOVERED: return "recovered";
> + case PCI_ERS_RESULT_NO_AER_DRIVER: return "no AER driver";
> + default:
> + WARN_ONCE(1, "Unknown result type");
> + return "unknown";
> + }
> +};
> +
> +#define eeh_infoline(EDEV, FMT, ...) \
> +pr_info("EEH: PE#%x (PCI %s): " pr_fmt(FMT) "\n", EDEV->pe_config_addr, \
> +((EDEV->pdev) ? dev_name(&EDEV->pdev->dev) : "NONE"), ##__VA_ARGS__)
Ooof, I guess.
It would be nicer as a function.
"infoline" annoys me for some reason, "eeh_info" ?
> @@ -223,123 +242,118 @@ static void eeh_set_irq_state(struct eeh_pe *root, bool enable)
> }
> }
>
> +static void eeh_pe_report(const char *name, struct eeh_pe *root,
> + enum pci_ers_result (*fn)(struct eeh_dev *,
> + struct pci_driver *),
> + enum pci_ers_result *result)
> +{
> + struct eeh_pe *pe;
> + struct eeh_dev *edev, *tmp;
> + enum pci_ers_result new_result;
> +
> + pr_info("EEH: Beginning: '%s'\n", name);
> + eeh_for_each_pe(root, pe) {
> + eeh_pe_for_each_dev(pe, edev, tmp) {
This should be in a separate function.
> + device_lock(&edev->pdev->dev);
> + if (eeh_edev_actionable(edev)) {
> + struct pci_driver *driver;
> +
> + driver = eeh_pcid_get(edev->pdev);
> +
> + if (!driver)
> + eeh_infoline(edev, "no driver");
> + else if (!driver->err_handler)
> + eeh_infoline(edev,
> + "driver not EEH aware");
> + else if (edev->mode & EEH_DEV_NO_HANDLER)
> + eeh_infoline(edev,
> + "driver bound too late");
> + else {
> + new_result = fn(edev, driver);
> + eeh_infoline(edev,
> + "%s driver reports: '%s'",
> + driver->name,
> + pci_ers_result_name(new_result));
> + if (result)
> + *result = merge_result(*result,
> + new_result);
> + }
> + if (driver)
> + eeh_pcid_put(edev->pdev);
> + } else {
> + eeh_infoline(edev, "not actionable (%d,%d,%d)",
> + !!edev->pdev,
> + !eeh_dev_removed(edev),
> + !eeh_pe_passed(edev->pe));
> + }
> + device_unlock(&edev->pdev->dev);
^^^
cheers
^ permalink raw reply
* Re: [PATCH 3/6] powerpc/64s: Patch barrier_nospec in modules
From: Michal Suchánek @ 2018-05-03 13:15 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, linux-kernel, npiggin
In-Reply-To: <20180424041559.32410-3-mpe@ellerman.id.au>
On Tue, 24 Apr 2018 14:15:56 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:
> From: Michal Suchanek <msuchanek@suse.de>
>
> Note that unlike RFI which is patched only in kernel the nospec state
> reflects settings at the time the module was loaded.
>
> Iterating all modules and re-patching every time the settings change
> is not implemented.
>
> Based on lwsync patching.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/include/asm/setup.h | 6 ++++++
> arch/powerpc/kernel/module.c | 6 ++++++
> arch/powerpc/lib/feature-fixups.c | 16 +++++++++++++---
> 3 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/setup.h
> b/arch/powerpc/include/asm/setup.h index afc7280cce3b..4335cddc1cf2
> 100644 --- a/arch/powerpc/include/asm/setup.h
> +++ b/arch/powerpc/include/asm/setup.h
> @@ -54,6 +54,12 @@ void setup_rfi_flush(enum l1d_flush_type, bool
> enable); void do_rfi_flush_fixups(enum l1d_flush_type types);
> void do_barrier_nospec_fixups(bool enable);
>
> +#ifdef CONFIG_PPC_BOOK3S_64
> +void do_barrier_nospec_fixups_range(bool enable, void *start, void
> *end); +#else
> +static inline void do_barrier_nospec_fixups_range(bool enable, void
> *start, void *end) { }; +#endif
> +
> #endif /* !__ASSEMBLY__ */
>
> #endif /* _ASM_POWERPC_SETUP_H */
> diff --git a/arch/powerpc/kernel/module.c
> b/arch/powerpc/kernel/module.c index 3f7ba0f5bf29..a72698cd3dd0 100644
> --- a/arch/powerpc/kernel/module.c
> +++ b/arch/powerpc/kernel/module.c
> @@ -72,6 +72,12 @@ int module_finalize(const Elf_Ehdr *hdr,
> do_feature_fixups(powerpc_firmware_features,
> (void *)sect->sh_addr,
> (void *)sect->sh_addr +
> sect->sh_size); +
> + sect = find_section(hdr, sechdrs, "__spec_barrier_fixup");
> + if (sect != NULL)
> + do_barrier_nospec_fixups_range(true,
This saves a global but always patches modules with the ORI
flush. The patching also produces a log message which can be confusing
to the user. Even when barrier is disabled "barrier-nospec: using ORI
speculation barrier" is logged when modules are loaded.
For consistency when debugging the settings I would suggest to keep the
export.
It is also no longer consistent with the commit message.
Thanks
Michal
> + (void *)sect->sh_addr,
> + (void *)sect->sh_addr +
> sect->sh_size); #endif
>
> sect = find_section(hdr, sechdrs, "__lwsync_fixup");
> diff --git a/arch/powerpc/lib/feature-fixups.c
> b/arch/powerpc/lib/feature-fixups.c index 093c1d2ea5fd..3b37529f82f8
> 100644 --- a/arch/powerpc/lib/feature-fixups.c
> +++ b/arch/powerpc/lib/feature-fixups.c
> @@ -163,14 +163,14 @@ void do_rfi_flush_fixups(enum l1d_flush_type
> types) : "unknown");
> }
>
> -void do_barrier_nospec_fixups(bool enable)
> +void do_barrier_nospec_fixups_range(bool enable, void *fixup_start,
> void *fixup_end) {
> unsigned int instr, *dest;
> long *start, *end;
> int i;
>
> - start = PTRRELOC(&__start___barrier_nospec_fixup),
> - end = PTRRELOC(&__stop___barrier_nospec_fixup);
> + start = fixup_start;
> + end = fixup_end;
>
> instr = 0x60000000; /* nop */
>
> @@ -189,6 +189,16 @@ void do_barrier_nospec_fixups(bool enable)
> printk(KERN_DEBUG "barrier-nospec: patched %d locations\n",
> i); }
>
> +void do_barrier_nospec_fixups(bool enable)
> +{
> + void *start, *end;
> +
> + start = PTRRELOC(&__start___barrier_nospec_fixup),
> + end = PTRRELOC(&__stop___barrier_nospec_fixup);
> +
> + do_barrier_nospec_fixups_range(enable, start, end);
> +}
> +
> #endif /* CONFIG_PPC_BOOK3S_64 */
>
> void do_lwsync_fixups(unsigned long value, void *fixup_start, void
> *fixup_end)
^ permalink raw reply
* Re: [PATCH NEXT 2/4] powerpc/pasemi: Add Nemo board IRQ init routine
From: Michael Ellerman @ 2018-05-03 13:06 UTC (permalink / raw)
To: Darren Stevens, linuxppc-dev; +Cc: Christian Zigotzky
In-Reply-To: <4b3d1fd01f9.613e9d05@auth.smtp.1and1.co.uk>
Darren Stevens <darren@stevens-zone.net> writes:
> diff --git a/arch/powerpc/platforms/pasemi/setup.c b/arch/powerpc/platforms/pasemi/setup.c
> index c4a3e93..c583c17 100644
> --- a/arch/powerpc/platforms/pasemi/setup.c
> +++ b/arch/powerpc/platforms/pasemi/setup.c
> @@ -183,6 +184,99 @@ static int __init pas_setup_mce_regs(void)
> }
> machine_device_initcall(pasemi, pas_setup_mce_regs);
>
> +#ifdef CONFIG_PPC_PASEMI_NEMO
> +static void sb600_8259_cascade(struct irq_desc *desc)
> +{
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + unsigned int cascade_irq = i8259_irq();
> +
> + if (cascade_irq)
> + generic_handle_irq(cascade_irq);
> +
> + chip->irq_eoi(&desc->irq_data);
> +}
> +
> +static __init void nemo_init_IRQ(void)
> +{
> + struct device_node *np;
> + struct device_node *root, *mpic_node, *i8259_node;
> + unsigned long openpic_addr;
> + const unsigned int *opprop;
> + int naddr, opplen;
> + int mpic_flags;
> + const unsigned int *nmiprop;
> + struct mpic *mpic;
> + int gpio_virq;
> +
> + mpic_node = NULL;
This is basically a copy of the existing routine.
> + for_each_node_by_type(np, "interrupt-controller")
> + if (of_device_is_compatible(np, "open-pic")) {
> + mpic_node = np;
> + break;
> + }
> + if (!mpic_node)
> + for_each_node_by_type(np, "open-pic") {
> + mpic_node = np;
> + break;
> + }
> + if (!mpic_node) {
> + printk(KERN_ERR
> + "Failed to locate the MPIC interrupt controller\n");
> + return;
> + }
> +
> + /* Find address list in /platform-open-pic */
> + root = of_find_node_by_path("/");
> + naddr = of_n_addr_cells(root);
> + opprop = of_get_property(root, "platform-open-pic", &opplen);
> + if (!opprop) {
> + printk(KERN_ERR "No platform-open-pic property.\n");
> + of_node_put(root);
> + return;
> + }
> + openpic_addr = of_read_number(opprop, naddr);
> + printk(KERN_DEBUG "OpenPIC addr: %lx\n", openpic_addr);
> +
> + mpic_flags = MPIC_LARGE_VECTORS | MPIC_NO_BIAS | MPIC_NO_RESET;
> +
> + nmiprop = of_get_property(mpic_node, "nmi-source", NULL);
> + if (nmiprop)
> + mpic_flags |= MPIC_ENABLE_MCK;
> +
> + mpic = mpic_alloc(mpic_node, openpic_addr,
> + mpic_flags, 0, 0, "PASEMI-OPIC");
> + BUG_ON(!mpic);
> +
> + mpic_assign_isu(mpic, 0, mpic->paddr + 0x10000);
> + mpic_init(mpic);
> + /* The NMI/MCK source needs to be prio 15 */
> + if (nmiprop) {
> + nmi_virq = irq_create_mapping(NULL, *nmiprop);
> + mpic_irq_set_priority(nmi_virq, 15);
> + irq_set_irq_type(nmi_virq, IRQ_TYPE_EDGE_RISING);
> + mpic_unmask_irq(irq_get_irq_data(nmi_virq));
> + }
Except for this bit:
> + /* Connect the SB600's legacy i8259 controller */
> + i8259_node = of_find_node_by_path("/pxp@0,e0000000");
> + i8259_init(i8259_node, 0);
> + of_node_put(i8259_node);
> +
> + gpio_virq = irq_create_mapping(NULL, 3);
> + irq_set_irq_type(gpio_virq, IRQ_TYPE_LEVEL_HIGH);
> + irq_set_chained_handler(gpio_virq, sb600_8259_cascade);
> + mpic_unmask_irq(irq_get_irq_data(gpio_virq));
> +
> + irq_set_default_host(mpic->irqhost);
So that should just go in a separate routine that is empty when NEMO=n.
cheers
^ 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