* Re: [PATCH v6 07/11] powerpc/kvm/e500: Use functions to track lockless pgtbl walks
From: Leonardo Bras @ 2020-02-07 3:10 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Arnd Bergmann, Andrew Morton, Aneesh Kumar K.V,
Nicholas Piggin, Steven Price, Robin Murphy, Mahesh Salgaonkar,
Balbir Singh, Reza Arbab, Thomas Gleixner, Allison Randal,
Greg Kroah-Hartman, Mike Rapoport, Michal Suchanek
Cc: linux-arch, linux-mm, linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <fae235d5-78b6-87aa-ed3f-1a908d61abf4@c-s.fr>
[-- Attachment #1: Type: text/plain, Size: 548 bytes --]
Hello Christophe,
On Thu, 2020-02-06 at 07:18 +0100, Christophe Leroy wrote:
>
> I don't really like unbalanced begin/end.
>
> Something like the following would be cleaner:
>
>
> begin_lockless_pgtbl_walk()
> ptep = find()
> if (ptep) {
> pte = READ_ONCE()
> if (pte_present(pte))
> wing=
> else
> ret = -EINVAL;
> }
> end_lockless_pgtbl_walk()
>
> if (ret) {
> pr_err_rate...()
> goto out;
> }
>
>
Sure, looks better that way. I will change that for v7.
Thanks for the feedback,
Leonardo Bras
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] vfio-pci/nvlink2: Allow fallback to ibm,mmio-atsd[0]
From: Sam Bobroff @ 2020-02-07 2:39 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: Alex Williamson, linuxppc-dev, kvm
In-Reply-To: <84147d70-3409-e216-495d-fc54366b92a6@ozlabs.ru>
[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]
On Thu, Feb 06, 2020 at 03:23:03PM +1100, Alexey Kardashevskiy wrote:
>
>
> On 06/02/2020 14:17, Sam Bobroff wrote:
> > Older versions of skiboot only provide a single value in the device
> > tree property "ibm,mmio-atsd", even when multiple Address Translation
> > Shoot Down (ATSD) registers are present. This prevents NVLink2 devices
> > (other than the first) from being used with vfio-pci because vfio-pci
> > expects to be able to assign a dedicated ATSD register to each NVLink2
> > device.
> >
> > However, ATSD registers can be shared among devices. This change
> > allows vfio-pci to fall back to sharing the register at index 0 if
> > necessary.
> >
> > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> > ---
> > drivers/vfio/pci/vfio_pci_nvlink2.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_nvlink2.c b/drivers/vfio/pci/vfio_pci_nvlink2.c
> > index f2983f0f84be..851ba673882b 100644
> > --- a/drivers/vfio/pci/vfio_pci_nvlink2.c
> > +++ b/drivers/vfio/pci/vfio_pci_nvlink2.c
> > @@ -420,8 +420,17 @@ int vfio_pci_ibm_npu2_init(struct vfio_pci_device *vdev)
> >
> > if (of_property_read_u64_index(hose->dn, "ibm,mmio-atsd", nvlink_index,
> > &mmio_atsd)) {
> > - dev_warn(&vdev->pdev->dev, "No available ATSD found\n");
> > - mmio_atsd = 0;
> > + dev_warn(&vdev->pdev->dev,
> > + "No ibm,mmio-atsd[%d] found: trying ibm,mmio-atsd[0]\n",
> > + nvlink_index);
>
>
> We do not really need this warning (nvlink_index doesn't matter that
> much, we can work out from the device tree what happened), warnings
> below are enough (if you really want, you can print nvlink_index there).
>
> Either way,
>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
Thanks,
I'll change it if there's some reason to do another version but
otherwise leave it as is.
Sam.
>
>
>
> > + if (of_property_read_u64_index(hose->dn, "ibm,mmio-atsd", 0,
> > + &mmio_atsd)) {
> > + dev_warn(&vdev->pdev->dev, "No available ATSD found\n");
> > + mmio_atsd = 0;
> > + } else {
> > + dev_warn(&vdev->pdev->dev,
> > + "Using fallback ibm,mmio-atsd[0] for ATSD.\n");
> > + }
> > }
> >
> > if (of_property_read_u64(npu_node, "ibm,device-tgt-addr", &tgt)) {
> >
>
> --
> Alexey
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 6/6] powerpc/eeh: Rework eeh_ops->probe()
From: Sam Bobroff @ 2020-02-07 2:37 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: tyreld, linuxppc-dev
In-Reply-To: <20200203083521.16549-7-oohall@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 9210 bytes --]
On Mon, Feb 03, 2020 at 07:35:21PM +1100, Oliver O'Halloran wrote:
> With the EEH early probe now being pseries specific there's no need for
> eeh_ops->probe() to take a pci_dn. Instead, we can make it take a pci_dev
> and use the probe function to map a pci_dev to an eeh_dev. This allows
> the platform to implement it's own method for finding (or creating) an
> eeh_dev for a given pci_dev which also removes a use of pci_dn in
> generic EEH code.
>
> This patch also renames eeh_device_add_late() to eeh_device_probe(). This
> better reflects what it does does and removes the last vestiges of the
> early/late EEH probe split.
Nice!
Just one nit, below.
Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> arch/powerpc/include/asm/eeh.h | 6 ++--
> arch/powerpc/kernel/eeh.c | 42 +++++++++++++++-------------
> arch/powerpc/platforms/powernv/eeh-powernv.c | 30 ++++++++++----------
> arch/powerpc/platforms/pseries/eeh_pseries.c | 23 ++++++++++++++-
> 4 files changed, 61 insertions(+), 40 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index 8580238..964a542 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -215,7 +215,7 @@ enum {
> struct eeh_ops {
> char *name;
> int (*init)(void);
> - void* (*probe)(struct pci_dn *pdn, void *data);
> + struct eeh_dev *(*probe)(struct pci_dev *pdev);
> int (*set_option)(struct eeh_pe *pe, int option);
> int (*get_pe_addr)(struct eeh_pe *pe);
> int (*get_state)(struct eeh_pe *pe, int *delay);
> @@ -301,7 +301,7 @@ int __exit eeh_ops_unregister(const char *name);
> int eeh_check_failure(const volatile void __iomem *token);
> int eeh_dev_check_failure(struct eeh_dev *edev);
> void eeh_addr_cache_init(void);
> -void eeh_add_device_late(struct pci_dev *);
> +void eeh_probe_device(struct pci_dev *pdev);
> void eeh_remove_device(struct pci_dev *);
> int eeh_unfreeze_pe(struct eeh_pe *pe);
> int eeh_pe_reset_and_recover(struct eeh_pe *pe);
> @@ -356,7 +356,7 @@ static inline int eeh_check_failure(const volatile void __iomem *token)
>
> static inline void eeh_addr_cache_init(void) { }
>
> -static inline void eeh_add_device_late(struct pci_dev *dev) { }
> +static inline void eeh_probe_device(struct pci_dev *dev) { }
>
> static inline void eeh_remove_device(struct pci_dev *dev) { }
>
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index 55d3ef6..2c5f7a6 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -1113,29 +1113,37 @@ core_initcall_sync(eeh_init);
> * This routine must be used to complete EEH initialization for PCI
> * devices that were added after system boot (e.g. hotplug, dlpar).
> */
You can't see it in the patch but up a few lines in the comment block,
there's a leftover "eeh_add_device_late".
> -void eeh_add_device_late(struct pci_dev *dev)
> +void eeh_probe_device(struct pci_dev *dev)
> {
> - struct pci_dn *pdn;
> struct eeh_dev *edev;
>
> - if (!dev)
> + pr_debug("EEH: Adding device %s\n", pci_name(dev));
> +
> + /*
> + * pci_dev_to_eeh_dev() can only work if eeh_probe_dev() was
> + * already called for this device.
> + */
> + if (WARN_ON_ONCE(pci_dev_to_eeh_dev(dev))) {
> + eeh_edev_dbg(edev, "Already bound to an eeh_dev!\n");
> return;
> + }
>
> - pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
> - edev = pdn_to_eeh_dev(pdn);
> - eeh_edev_dbg(edev, "Adding device\n");
> - if (edev->pdev == dev) {
> - eeh_edev_dbg(edev, "Device already referenced!\n");
> + edev = eeh_ops->probe(dev);
> + if (!edev) {
> + pr_debug("EEH: Adding device failed\n");
> return;
> }
>
> /*
> - * The EEH cache might not be removed correctly because of
> - * unbalanced kref to the device during unplug time, which
> - * relies on pcibios_release_device(). So we have to remove
> - * that here explicitly.
> + * FIXME: We rely on pcibios_release_device() to remove the
> + * existing EEH state. The release function is only called if
> + * the pci_dev's refcount drops to zero so if something is
> + * keeping a ref to a device (e.g. a filesystem) we need to
> + * remove the old EEH state.
> + *
> + * FIXME: HEY MA, LOOK AT ME, NO LOCKING!
> */
> - if (edev->pdev) {
> + if (edev->pdev && edev->pdev != dev) {
> eeh_rmv_from_parent_pe(edev);
> eeh_addr_cache_rmv_dev(edev->pdev);
> eeh_sysfs_remove_device(edev->pdev);
> @@ -1146,17 +1154,11 @@ void eeh_add_device_late(struct pci_dev *dev)
> * into error handler afterwards.
> */
> edev->mode |= EEH_DEV_NO_HANDLER;
> -
> - edev->pdev = NULL;
> - dev->dev.archdata.edev = NULL;
> }
>
> - if (eeh_has_flag(EEH_PROBE_MODE_DEV))
> - eeh_ops->probe(pdn, NULL);
> -
> + /* bind the pdev and the edev together */
> edev->pdev = dev;
> dev->dev.archdata.edev = edev;
> -
> eeh_addr_cache_insert_dev(dev);
> eeh_sysfs_add_device(dev);
> }
> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
> index eaa8dfe..79409e0 100644
> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
> @@ -41,7 +41,7 @@ static int eeh_event_irq = -EINVAL;
> void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
> {
> dev_dbg(&pdev->dev, "EEH: Setting up device\n");
> - eeh_add_device_late(pdev);
> + eeh_probe_device(pdev);
> }
>
> static int pnv_eeh_init(void)
> @@ -340,23 +340,13 @@ static int pnv_eeh_find_ecap(struct pci_dn *pdn, int cap)
>
> /**
> * pnv_eeh_probe - Do probe on PCI device
> - * @pdn: PCI device node
> - * @data: unused
> + * @pdev: pci_dev to probe
> *
> - * When EEH module is installed during system boot, all PCI devices
> - * are checked one by one to see if it supports EEH. The function
> - * is introduced for the purpose. By default, EEH has been enabled
> - * on all PCI devices. That's to say, we only need do necessary
> - * initialization on the corresponding eeh device and create PE
> - * accordingly.
> - *
> - * It's notable that's unsafe to retrieve the EEH device through
> - * the corresponding PCI device. During the PCI device hotplug, which
> - * was possiblly triggered by EEH core, the binding between EEH device
> - * and the PCI device isn't built yet.
> + * Create, or find the existing, eeh_dev for this pci_dev.
> */
> -static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
> +static struct eeh_dev *pnv_eeh_probe(struct pci_dev *pdev)
> {
> + struct pci_dn *pdn = pci_get_pdn(pdev);
> struct pci_controller *hose = pdn->phb;
> struct pnv_phb *phb = hose->private_data;
> struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
> @@ -373,6 +363,14 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
> if (!edev || edev->pe)
> return NULL;
>
> + /* already configured? */
> + if (edev->pdev) {
> + pr_debug("%s: found existing edev for %04x:%02x:%02x.%01x\n",
> + __func__, hose->global_number, config_addr >> 8,
> + PCI_SLOT(config_addr), PCI_FUNC(config_addr));
> + return edev;
> + }
> +
> /* Skip for PCI-ISA bridge */
> if ((pdn->class_code >> 8) == PCI_CLASS_BRIDGE_ISA)
> return NULL;
> @@ -464,7 +462,7 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
>
> eeh_edev_dbg(edev, "EEH enabled on device\n");
>
> - return NULL;
> + return edev;
> }
>
> /**
> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
> index 1ca7cf0..8453428 100644
> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> @@ -77,7 +77,7 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
> eeh_add_to_parent_pe(edev); /* Add as VF PE type */
> }
> #endif
> - eeh_add_device_late(pdev);
> + eeh_probe_device(pdev);
> }
>
> /*
> @@ -335,6 +335,26 @@ void pseries_eeh_init_edev(struct pci_dn *pdn)
> eeh_save_bars(edev);
> }
>
> +static struct eeh_dev *pseries_eeh_probe(struct pci_dev *pdev)
> +{
> + struct eeh_dev *edev;
> + struct pci_dn *pdn;
> +
> + pdn = pci_get_pdn_by_devfn(pdev->bus, pdev->devfn);
> + if (!pdn)
> + return NULL;
> +
> + /*
> + * If the system supports EEH on this device then the eeh_dev was
> + * configured and inserted into a PE in pseries_eeh_init_edev()
> + */
> + edev = pdn_to_eeh_dev(pdn);
> + if (!edev || !edev->pe)
> + return NULL;
> +
> + return edev;
> +}
> +
> /**
> * pseries_eeh_init_edev_recursive - Enable EEH for the indicated device
> * @pdn: PCI device node
> @@ -813,6 +833,7 @@ static int pseries_notify_resume(struct pci_dn *pdn)
> static struct eeh_ops pseries_eeh_ops = {
> .name = "pseries",
> .init = pseries_eeh_init,
> + .probe = pseries_eeh_probe,
> .set_option = pseries_eeh_set_option,
> .get_pe_addr = pseries_eeh_get_pe_addr,
> .get_state = pseries_eeh_get_state,
> --
> 2.9.5
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 5/6] powerpc/eeh: Make early EEH init pseries specific
From: Sam Bobroff @ 2020-02-07 2:24 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: tyreld, linuxppc-dev
In-Reply-To: <20200203083521.16549-6-oohall@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 13378 bytes --]
On Mon, Feb 03, 2020 at 07:35:20PM +1100, Oliver O'Halloran wrote:
> The eeh_ops->probe() function is called from two different contexts:
>
> 1. On pseries, where set set EEH_PROBE_MODE_DEVTREE, it's called in
"set set" -> "we set"
> eeh_add_device_early() which is supposed to run before we create
> a pci_dev.
>
> 2. On PowerNV, where we set EEH_PROBE_MODE_DEV, it's called in
> eeh_device_add_late() which is supposed to run *after* the
> pci_dev is created.
>
> The "early" probe is required because PAPR requires that we perform an RTAS
> call to enable EEH support on a device before we start interacting with it
> via config space or MMIO. This requirement doesn't exist on PowerNV and
> shoehorning two completely separate initialisation paths into a common
> interface just results in a convoluted code everywhere.
>
> Additionally the early probe requires the probe function to take an pci_dn
> rather than a pci_dev argument. We'd like to make pci_dn a pseries specific
> data structure since there's no real requirement for them on PowerNV. To
> help both goals move the early probe into the pseries containment zone
> so the platform depedence is more explicit.
>
I had a look around near your comment:
> + // XXX: uh, do we have the rescan lock held here?
And we definitely don't have the lock when it gets called via the module
init path (as rpaphp is loaded) -- I tried it and there was no deadlock.
I don't think we have the lock in other situations but I haven't
unravelled it all enough yet to tell, either.
Regardless, good cleanup.
Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> arch/powerpc/include/asm/eeh.h | 14 +++---
> arch/powerpc/kernel/eeh.c | 46 --------------------
> arch/powerpc/kernel/of_platform.c | 6 +--
> arch/powerpc/platforms/powernv/eeh-powernv.c | 6 ---
> arch/powerpc/platforms/pseries/eeh_pseries.c | 65 ++++++++++++++++++++++------
> arch/powerpc/platforms/pseries/pci_dlpar.c | 2 +-
> drivers/pci/hotplug/rpadlpar_core.c | 2 +-
> drivers/pci/hotplug/rpaphp_core.c | 2 +-
> drivers/pci/hotplug/rpaphp_pci.c | 3 +-
> 9 files changed, 65 insertions(+), 81 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index 5d10781..8580238 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -301,8 +301,6 @@ int __exit eeh_ops_unregister(const char *name);
> int eeh_check_failure(const volatile void __iomem *token);
> int eeh_dev_check_failure(struct eeh_dev *edev);
> void eeh_addr_cache_init(void);
> -void eeh_add_device_early(struct pci_dn *);
> -void eeh_add_device_tree_early(struct pci_dn *);
> void eeh_add_device_late(struct pci_dev *);
> void eeh_remove_device(struct pci_dev *);
> int eeh_unfreeze_pe(struct eeh_pe *pe);
> @@ -358,10 +356,6 @@ static inline int eeh_check_failure(const volatile void __iomem *token)
>
> static inline void eeh_addr_cache_init(void) { }
>
> -static inline void eeh_add_device_early(struct pci_dn *pdn) { }
> -
> -static inline void eeh_add_device_tree_early(struct pci_dn *pdn) { }
> -
> static inline void eeh_add_device_late(struct pci_dev *dev) { }
>
> static inline void eeh_remove_device(struct pci_dev *dev) { }
> @@ -370,6 +364,14 @@ static inline void eeh_remove_device(struct pci_dev *dev) { }
> #define EEH_IO_ERROR_VALUE(size) (-1UL)
> #endif /* CONFIG_EEH */
>
> +#if defined(CONFIG_PPC_PSERIES) && defined(CONFIG_EEH)
> +void pseries_eeh_init_edev(struct pci_dn *pdn);
> +void pseries_eeh_init_edev_recursive(struct pci_dn *pdn);
> +#else
> +static inline void pseries_eeh_add_device_early(struct pci_dn *pdn) { }
> +static inline void pseries_eeh_add_device_tree_early(struct pci_dn *pdn) { }
> +#endif
> +
> #ifdef CONFIG_PPC64
> /*
> * MMIO read/write operations with EEH support.
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index a9e4ca7..55d3ef6 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -1107,52 +1107,6 @@ static int eeh_init(void)
> core_initcall_sync(eeh_init);
>
> /**
> - * eeh_add_device_early - Enable EEH for the indicated device node
> - * @pdn: PCI device node for which to set up EEH
> - *
> - * This routine must be used to perform EEH initialization for PCI
> - * devices that were added after system boot (e.g. hotplug, dlpar).
> - * This routine must be called before any i/o is performed to the
> - * adapter (inluding any config-space i/o).
> - * Whether this actually enables EEH or not for this device depends
> - * on the CEC architecture, type of the device, on earlier boot
> - * command-line arguments & etc.
> - */
> -void eeh_add_device_early(struct pci_dn *pdn)
> -{
> - struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
> -
> - if (!edev)
> - return;
> -
> - if (!eeh_has_flag(EEH_PROBE_MODE_DEVTREE))
> - return;
> -
> - eeh_ops->probe(pdn, NULL);
> -}
> -
> -/**
> - * eeh_add_device_tree_early - Enable EEH for the indicated device
> - * @pdn: PCI device node
> - *
> - * This routine must be used to perform EEH initialization for the
> - * indicated PCI device that was added after system boot (e.g.
> - * hotplug, dlpar).
> - */
> -void eeh_add_device_tree_early(struct pci_dn *pdn)
> -{
> - struct pci_dn *n;
> -
> - if (!pdn)
> - return;
> -
> - list_for_each_entry(n, &pdn->child_list, list)
> - eeh_add_device_tree_early(n);
> - eeh_add_device_early(pdn);
> -}
> -EXPORT_SYMBOL_GPL(eeh_add_device_tree_early);
> -
> -/**
> * eeh_add_device_late - Perform EEH initialization for the indicated pci device
> * @dev: pci device for which to set up EEH
> *
> diff --git a/arch/powerpc/kernel/of_platform.c b/arch/powerpc/kernel/of_platform.c
> index 64edac81..71a3f97 100644
> --- a/arch/powerpc/kernel/of_platform.c
> +++ b/arch/powerpc/kernel/of_platform.c
> @@ -62,13 +62,9 @@ static int of_pci_phb_probe(struct platform_device *dev)
> /* Init pci_dn data structures */
> pci_devs_phb_init_dynamic(phb);
>
> - /* Create EEH devices for the PHB */
> + /* Create EEH PEs for the PHB */
> eeh_dev_phb_init_dynamic(phb);
>
> - /* Register devices with EEH */
> - if (dev->dev.of_node->child)
> - eeh_add_device_tree_early(PCI_DN(dev->dev.of_node));
> -
> /* Scan the bus */
> pcibios_scan_phb(phb);
> if (phb->bus == NULL)
> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
> index ef727ec..eaa8dfe 100644
> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
> @@ -40,13 +40,7 @@ static int eeh_event_irq = -EINVAL;
>
> void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
> {
> - struct pci_dn *pdn = pci_get_pdn(pdev);
> -
> - if (!pdn || eeh_has_flag(EEH_FORCE_DISABLED))
> - return;
> -
> dev_dbg(&pdev->dev, "EEH: Setting up device\n");
> - eeh_add_device_early(pdn);
> eeh_add_device_late(pdev);
> }
>
> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
> index 95bbf91..1ca7cf0 100644
> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> @@ -67,7 +67,7 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
> pdn->pe_number = physfn_pdn->pe_num_map[pdn->vf_index];
> }
> #endif
> - eeh_add_device_early(pdn);
> + pseries_eeh_init_edev(pdn);
> #ifdef CONFIG_PCI_IOV
> if (pdev->is_virtfn) {
> struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
> @@ -221,15 +221,16 @@ static int pseries_eeh_find_ecap(struct pci_dn *pdn, int cap)
> }
>
> /**
> - * pseries_eeh_probe - EEH probe on the given device
> + * pseries_eeh_init_edev - initialise the eeh_dev and eeh_pe for a pci_dn
> + *
> * @pdn: PCI device node
> - * @data: Unused
> *
> - * When EEH module is installed during system boot, all PCI devices
> - * are checked one by one to see if it supports EEH. The function
> - * is introduced for the purpose.
> + * When we discover a new PCI device via the device-tree we create a
> + * corresponding pci_dn and we allocate, but don't initialise, an eeh_dev.
> + * This function takes care of the initialisation and inserts the eeh_dev
> + * into the correct eeh_pe. If no eeh_pe exists we'll allocate one.
> */
> -static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
> +void pseries_eeh_init_edev(struct pci_dn *pdn)
> {
> struct eeh_dev *edev;
> struct eeh_pe pe;
> @@ -237,18 +238,35 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
> int enable = 0;
> int ret;
>
> - /* Retrieve OF node and eeh device */
> + if (WARN_ON_ONCE(!eeh_has_flag(EEH_PROBE_MODE_DEVTREE)))
> + return;
> +
> + /*
> + * Find the eeh_dev for this pdn. The storage for the eeh_dev was
> + * allocated at the same time as the pci_dn.
> + *
> + * XXX: We should probably re-visit that.
> + */
> edev = pdn_to_eeh_dev(pdn);
> - if (!edev || edev->pe)
> - return NULL;
> + if (!edev)
> + return;
> +
> + /*
> + * If ->pe is set then we've already probed this device. We hit
> + * this path when a pci_dev is removed and rescanned while recovering
> + * a PE (i.e. for devices where the driver doesn't support error
> + * recovery).
> + */
> + if (edev->pe)
> + return;
>
> /* Check class/vendor/device IDs */
> if (!pdn->vendor_id || !pdn->device_id || !pdn->class_code)
> - return NULL;
> + return;
>
> /* Skip for PCI-ISA bridge */
> if ((pdn->class_code >> 8) == PCI_CLASS_BRIDGE_ISA)
> - return NULL;
> + return;
>
> eeh_edev_dbg(edev, "Probing device\n");
>
> @@ -315,9 +333,29 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
>
> /* Save memory bars */
> eeh_save_bars(edev);
> +}
> +
> +/**
> + * pseries_eeh_init_edev_recursive - Enable EEH for the indicated device
> + * @pdn: PCI device node
> + *
> + * This routine must be used to perform EEH initialization for the
> + * indicated PCI device that was added after system boot (e.g.
> + * hotplug, dlpar).
> + */
> +void pseries_eeh_init_edev_recursive(struct pci_dn *pdn)
> +{
> + struct pci_dn *n;
> +
> + if (!pdn)
> + return;
> +
> + list_for_each_entry(n, &pdn->child_list, list)
> + pseries_eeh_init_edev_recursive(n);
>
> - return NULL;
> + pseries_eeh_init_edev(pdn);
> }
> +EXPORT_SYMBOL_GPL(pseries_eeh_init_edev_recursive);
>
> /**
> * pseries_eeh_set_option - Initialize EEH or MMIO/DMA reenable
> @@ -775,7 +813,6 @@ static int pseries_notify_resume(struct pci_dn *pdn)
> static struct eeh_ops pseries_eeh_ops = {
> .name = "pseries",
> .init = pseries_eeh_init,
> - .probe = pseries_eeh_probe,
> .set_option = pseries_eeh_set_option,
> .get_pe_addr = pseries_eeh_get_pe_addr,
> .get_state = pseries_eeh_get_state,
> diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c
> index 361986e..b3a38f5 100644
> --- a/arch/powerpc/platforms/pseries/pci_dlpar.c
> +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
> @@ -37,7 +37,7 @@ struct pci_controller *init_phb_dynamic(struct device_node *dn)
> eeh_dev_phb_init_dynamic(phb);
>
> if (dn->child)
> - eeh_add_device_tree_early(PCI_DN(dn));
> + pseries_eeh_init_edev_recursive(PCI_DN(dn));
>
> pcibios_scan_phb(phb);
> pcibios_finish_adding_to_bus(phb->bus);
> diff --git a/drivers/pci/hotplug/rpadlpar_core.c b/drivers/pci/hotplug/rpadlpar_core.c
> index 977946e..c5eb509 100644
> --- a/drivers/pci/hotplug/rpadlpar_core.c
> +++ b/drivers/pci/hotplug/rpadlpar_core.c
> @@ -140,7 +140,7 @@ static void dlpar_pci_add_bus(struct device_node *dn)
> struct pci_controller *phb = pdn->phb;
> struct pci_dev *dev = NULL;
>
> - eeh_add_device_tree_early(pdn);
> + pseries_eeh_init_edev_recursive(pdn);
>
> /* Add EADS device to PHB bus, adding new entry to bus->devices */
> dev = of_create_pci_dev(dn, phb->bus, pdn->devfn);
> diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
> index 9c1e43e..b89d5ff 100644
> --- a/drivers/pci/hotplug/rpaphp_core.c
> +++ b/drivers/pci/hotplug/rpaphp_core.c
> @@ -494,7 +494,7 @@ static int enable_slot(struct hotplug_slot *hotplug_slot)
> return retval;
>
> if (state == PRESENT) {
> - eeh_add_device_tree_early(PCI_DN(slot->dn));
> + pseries_eeh_init_edev_recursive(PCI_DN(slot->dn));
>
> pci_lock_rescan_remove();
> pci_hp_add_devices(slot->bus);
> diff --git a/drivers/pci/hotplug/rpaphp_pci.c b/drivers/pci/hotplug/rpaphp_pci.c
> index 61ebbd8..e116ffe 100644
> --- a/drivers/pci/hotplug/rpaphp_pci.c
> +++ b/drivers/pci/hotplug/rpaphp_pci.c
> @@ -96,7 +96,8 @@ int rpaphp_enable_slot(struct slot *slot)
> }
>
> if (list_empty(&bus->devices)) {
> - eeh_add_device_tree_early(PCI_DN(slot->dn));
> + // XXX: uh, do we have the rescan lock held here?
> + pseries_eeh_init_edev_recursive(PCI_DN(slot->dn));
> pci_hp_add_devices(bus);
> }
>
> --
> 2.9.5
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v6 01/11] asm-generic/pgtable: Adds generic functions to track lockless pgtable walks
From: Leonardo Bras @ 2020-02-07 2:19 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Arnd Bergmann, Andrew Morton, Aneesh Kumar K.V,
Nicholas Piggin, Steven Price, Robin Murphy, Mahesh Salgaonkar,
Balbir Singh, Reza Arbab, Thomas Gleixner, Allison Randal,
Greg Kroah-Hartman, Mike Rapoport, Michal Suchanek
Cc: linux-arch, linux-mm, linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <f55e593c-27d5-df12-602f-ea217f62c5a1@c-s.fr>
[-- Attachment #1: Type: text/plain, Size: 1298 bytes --]
Hello Christophe, thanks for the feedback!
On Thu, 2020-02-06 at 06:54 +0100, Christophe Leroy wrote:
> > A memory barrier was also added just to make sure there is no speculative
> > read outside the interrupt disabled area. Other than that, it is not
> > supposed to have any change of behavior from current code.
>
> Is that speculative barrier necessary for all architectures ? Does it
> impact performance ? Shouldn't this be another patch ?
It makes sense, better keep the code as much as possible as it was. If
any arch finds this barrier needed, it can implement it's own version
of this function (or another patch to add this to generic, if proved to
be needed in every arch).
> > +#ifndef __HAVE_ARCH_LOCKLESS_PGTBL_WALK_CONTROL
> > +/*
> > + * begin_lockless_pgtbl_walk: Must be inserted before a function call that does
> > + * lockless pagetable walks, such as __find_linux_pte()
> > + */
> > +static inline
> > +unsigned long begin_lockless_pgtbl_walk(void)
>
> What about keeping the same syntax as local_irq_save(), something like:
>
> #define begin_lockless_pgtbl_walk(flags) \
> do {
> local_irq_save(flags);
> smp_mb();
> } while (0)
>
Makes sense. But wouldn't inlining have the same code output?
Best regards,
Leonardo Bras
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v6 10/11] powerpc/mm: Adds counting method to track lockless pagetable walks
From: Leonardo Bras @ 2020-02-07 1:56 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Arnd Bergmann, Andrew Morton, Aneesh Kumar K.V,
Nicholas Piggin, Steven Price, Robin Murphy, Mahesh Salgaonkar,
Balbir Singh, Reza Arbab, Thomas Gleixner, Allison Randal,
Greg Kroah-Hartman, Mike Rapoport, Michal Suchanek
Cc: linux-arch, linux-mm, linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <d9bf6878-43d5-b45a-7abb-cdcb712a0d7a@c-s.fr>
[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]
Hello Christophe, thanks for the feedback!
On Thu, 2020-02-06 at 07:23 +0100, Christophe Leroy wrote:
> > Due to not locking nor using atomic variables, the impact on the
> > lockless pagetable walk is intended to be minimum.
>
> atomic variables have a lot less impact than preempt_enable/disable.
>
> preemt_disable forces a re-scheduling, it really has impact. Why not use
> atomic variables instead ?
In fact, v5 of this patch used atomic variables. But it seems to cause
contention on a single exclusive cacheline, which had no better
performance than locking.
(discussion here: http://patchwork.ozlabs.org/patch/1171012/)
When I try to understand the effect of preempt_disable(), all I can
see is a barrier() and possibly a preempt_count_inc(), which updates a
member of current thread struct if CONFIG_PREEMPT_COUNT is enabled.
If CONFIG_PREEMPTION is also enabled, preempt_enable() can run a
__preempt_schedule() on unlikely(__preempt_count_dec_and_test()).
On most configs available, CONFIG_PREEMPTION is not set, being replaced
either by CONFIG_PREEMPT_NONE (kernel defconfigs) or
CONFIG_PREEMPT_VOLUNTARY in most supported distros. With that, most
probably CONFIG_PREEMPT_COUNT will also not be set, and
preempt_{en,dis}able() are replaced by a barrier().
Using preempt_disable approach, I intent to get better performance for
most used cases.
What do you think of it?
I am still new on this subject, and I am still trying to better
understand how it works. If you notice something I am missing, please
let me know.
Best regards,
Leonardo Bras
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v6 02/11] mm/gup: Use functions to track lockless pgtbl walks on gup_pgd_range
From: kbuild test robot @ 2020-02-07 1:19 UTC (permalink / raw)
To: Leonardo Bras
Cc: linux-mm, Paul Mackerras, Mike Rapoport, linux-arch,
Aneesh Kumar K.V, Steven Price, Mahesh Salgaonkar, Leonardo Bras,
Michal Suchanek, Arnd Bergmann, Robin Murphy, Nicholas Piggin,
kvm-ppc, Thomas Gleixner, Reza Arbab, Allison Randal, kbuild-all,
Greg Kroah-Hartman, linux-kernel, Andrew Morton, linuxppc-dev
In-Reply-To: <20200206030900.147032-3-leonardo@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 3079 bytes --]
Hi Leonardo,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on paulus-powerpc/kvm-ppc-next linus/master v5.5 next-20200206]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Leonardo-Bras/Introduces-new-functions-for-tracking-lockless-pagetable-walks/20200207-071035
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.5.0-3) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
mm/gup.c: In function 'get_user_pages_fast':
>> mm/gup.c:2435:27: error: 'IRQS_ENABLED' undeclared (first use in this function); did you mean 'IS_ENABLED'?
end_lockless_pgtbl_walk(IRQS_ENABLED);
^~~~~~~~~~~~
IS_ENABLED
mm/gup.c:2435:27: note: each undeclared identifier is reported only once for each function it appears in
vim +2435 mm/gup.c
2395
2396 /**
2397 * get_user_pages_fast() - pin user pages in memory
2398 * @start: starting user address
2399 * @nr_pages: number of pages from start to pin
2400 * @gup_flags: flags modifying pin behaviour
2401 * @pages: array that receives pointers to the pages pinned.
2402 * Should be at least nr_pages long.
2403 *
2404 * Attempt to pin user pages in memory without taking mm->mmap_sem.
2405 * If not successful, it will fall back to taking the lock and
2406 * calling get_user_pages().
2407 *
2408 * Returns number of pages pinned. This may be fewer than the number
2409 * requested. If nr_pages is 0 or negative, returns 0. If no pages
2410 * were pinned, returns -errno.
2411 */
2412 int get_user_pages_fast(unsigned long start, int nr_pages,
2413 unsigned int gup_flags, struct page **pages)
2414 {
2415 unsigned long addr, len, end;
2416 int nr = 0, ret = 0;
2417
2418 if (WARN_ON_ONCE(gup_flags & ~(FOLL_WRITE | FOLL_LONGTERM)))
2419 return -EINVAL;
2420
2421 start = untagged_addr(start) & PAGE_MASK;
2422 addr = start;
2423 len = (unsigned long) nr_pages << PAGE_SHIFT;
2424 end = start + len;
2425
2426 if (end <= start)
2427 return 0;
2428 if (unlikely(!access_ok((void __user *)start, len)))
2429 return -EFAULT;
2430
2431 if (IS_ENABLED(CONFIG_HAVE_FAST_GUP) &&
2432 gup_fast_permitted(start, end)) {
2433 begin_lockless_pgtbl_walk();
2434 gup_pgd_range(addr, end, gup_flags, pages, &nr);
> 2435 end_lockless_pgtbl_walk(IRQS_ENABLED);
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7206 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim
From: Michael Neuling @ 2020-02-07 0:41 UTC (permalink / raw)
To: Gustavo Luiz Duarte, Gustavo Luiz Duarte, linuxppc-dev; +Cc: stable, gromero
In-Reply-To: <3cc875cb-a149-1d9b-5a4d-57c8c370d60e@linux.vnet.ibm.com>
On Thu, 2020-02-06 at 19:13 -0300, Gustavo Luiz Duarte wrote:
>
> On 2/5/20 1:58 AM, Michael Neuling wrote:
> > Other than the minor things below that I think you need, the patch good with
> > me.
> >
> > Acked-by: Michael Neuling <mikey@neuling.org>
> >
> > > Subject: Re: [PATCH v2 1/3] powerpc/tm: Clear the current thread's MSR[TS]
> > > after treclaim
> >
> > The subject should mention "signals".
>
> How about "powerpc/tm: Clear the current thread's MSR[TS] when
> transaction is reclaimed on signal delivery" ?
mpe also likes the word "fix" in the subject if it's a fix. So maybe:
powerpc/tm: Fix clearing MSR[TS] in current when reclaiming on signal delivery
Thanks,
Mikey
^ permalink raw reply
* Re: [PATCH 06/18] powerpc sstep: Add support for prefixed integer load/stores
From: Jordan Niethe @ 2020-02-07 0:20 UTC (permalink / raw)
To: Balamuruhan S; +Cc: Alistair Popple, linuxppc-dev
In-Reply-To: <20200110151336.GA22466@localhost.localdomain>
On Sat, Jan 11, 2020 at 2:13 AM Balamuruhan S <bala24@linux.ibm.com> wrote:
>
> On Tue, Nov 26, 2019 at 04:21:29PM +1100, Jordan Niethe wrote:
> > This adds emulation support for the following prefixed integer
> > load/stores:
> > * Prefixed Load Byte and Zero (plbz)
> > * Prefixed Load Halfword and Zero (plhz)
> > * Prefixed Load Halfword Algebraic (plha)
> > * Prefixed Load Word and Zero (plwz)
> > * Prefixed Load Word Algebraic (plwa)
> > * Prefixed Load Doubleword (pld)
> > * Prefixed Store Byte (pstb)
> > * Prefixed Store Halfword (psth)
> > * Prefixed Store Word (pstw)
> > * Prefixed Store Doubleword (pstd)
> > * Prefixed Load Quadword (plq)
> > * Prefixed Store Quadword (pstq)
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > arch/powerpc/lib/sstep.c | 110 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 110 insertions(+)
> >
> > diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> > index ade3f5eba2e5..4f5ad1f602d8 100644
> > --- a/arch/powerpc/lib/sstep.c
> > +++ b/arch/powerpc/lib/sstep.c
> > @@ -187,6 +187,43 @@ static nokprobe_inline unsigned long xform_ea(unsigned int instr,
> > return ea;
> > }
> >
> > +/*
> > + * Calculate effective address for a MLS:D-form / 8LS:D-form prefixed instruction
> > + */
> > +static nokprobe_inline unsigned long mlsd_8lsd_ea(unsigned int instr,
> > + unsigned int sufx,
> > + const struct pt_regs *regs)
> > +{
> > + int ra, prefix_r;
> > + unsigned int dd;
> > + unsigned long ea, d0, d1, d;
> > +
> > + prefix_r = instr & (1ul << 20);
> > + ra = (sufx >> 16) & 0x1f;
> > +
> > + d0 = instr & 0x3ffff;
> > + d1 = sufx & 0xffff;
> > + d = (d0 << 16) | d1;
> > +
> > + /*
> > + * sign extend a 34 bit number
> > + */
> > + dd = (unsigned int) (d >> 2);
> > + ea = (signed int) dd;
> > + ea = (ea << 2) | (d & 0x3);
> > +
> > + if (!prefix_r && ra)
> > + ea += regs->gpr[ra];
> > + else if (!prefix_r && !ra)
> > + ; /* Leave ea as is */
> > + else if (prefix_r && !ra)
> > + ea += regs->nip;
> > + else if (prefix_r && ra)
> > + ; /* Invalid form. Should already be checked for by caller! */
> > +
> > + return ea;
> > +}
> > +
> > /*
> > * Return the largest power of 2, not greater than sizeof(unsigned long),
> > * such that x is a multiple of it.
> > @@ -1166,6 +1203,7 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> > unsigned int instr, unsigned int sufx)
> > {
> > unsigned int opcode, ra, rb, rc, rd, spr, u;
> > + unsigned int sufxopcode, prefixtype, prefix_r;
> > unsigned long int imm;
> > unsigned long int val, val2;
> > unsigned int mb, me, sh;
> > @@ -2652,6 +2690,78 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> >
> > }
> >
> > +/*
> > + * Prefixed instructions
> > + */
> > + switch (opcode) {
> > + case 1:
> > + prefix_r = instr & (1ul << 20);
> > + ra = (sufx >> 16) & 0x1f;
> > + op->update_reg = ra;
> > + rd = (sufx >> 21) & 0x1f;
> > + op->reg = rd;
> > + op->val = regs->gpr[rd];
> > +
> > + sufxopcode = sufx >> 26;
> > + prefixtype = (instr >> 24) & 0x3;
> > + switch (prefixtype) {
> > + case 0: /* Type 00 Eight-Byte Load/Store */
> > + if (prefix_r && ra)
> > + break;
> > + op->ea = mlsd_8lsd_ea(instr, sufx, regs);
> > + switch (sufxopcode) {
> > + case 41: /* plwa */
> > + op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 4);
> > + break;
> > + case 56: /* plq */
> > + op->type = MKOP(LOAD, PREFIXED, 16);
> > + break;
> > + case 57: /* pld */
> > + op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 8);
> > + break;
> > + case 60: /* stq */
> > + op->type = MKOP(STORE, PREFIXED, 16);
> > + break;
> > + case 61: /* pstd */
> > + op->type = MKOP(STORE, PREFIXED | SIGNEXT, 8);
>
> sorry, we don't do SIGNEXT for 1 byte below in Type 10, so does 8 byte is used
> conscious without definition in `do_signext()` as we don't really need to do
> anything ?
No, it was mistake. Those instructions should not have been marked as SIGNEXT.
>
> -- Bala
>
> > + break;
> > + }
> > + break;
> > + case 1: /* Type 01 Modified Register-to-Register */
> > + break;
> > + case 2: /* Type 10 Modified Load/Store */
> > + if (prefix_r && ra)
> > + break;
> > + op->ea = mlsd_8lsd_ea(instr, sufx, regs);
> > + switch (sufxopcode) {
> > + case 32: /* plwz */
> > + op->type = MKOP(LOAD, PREFIXED, 4);
> > + break;
> > + case 34: /* plbz */
> > + op->type = MKOP(LOAD, PREFIXED, 1);
> > + break;
> > + case 36: /* pstw */
> > + op->type = MKOP(STORE, PREFIXED, 4);
> > + break;
> > + case 38: /* pstb */
> > + op->type = MKOP(STORE, PREFIXED, 1);
> > + break;
> > + case 40: /* plhz */
> > + op->type = MKOP(LOAD, PREFIXED, 2);
> > + break;
> > + case 42: /* plha */
> > + op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 2);
> > + break;
> > + case 44: /* psth */
> > + op->type = MKOP(STORE, PREFIXED, 2);
> > + break;
> > + }
> > + break;
> > + case 3: /* Type 11 Modified Register-to-Register */
> > + break;
> > + }
> > + }
> > +
> > #ifdef CONFIG_VSX
> > if ((GETTYPE(op->type) == LOAD_VSX ||
> > GETTYPE(op->type) == STORE_VSX) &&
> > --
> > 2.20.1
> >
>
^ permalink raw reply
* Re: [PATCH 06/18] powerpc sstep: Add support for prefixed integer load/stores
From: Jordan Niethe @ 2020-02-07 0:18 UTC (permalink / raw)
To: Balamuruhan S; +Cc: Alistair Popple, linuxppc-dev
In-Reply-To: <20200110103849.GA18460@dhcp-9-109-246-161.in.ibm.com>
On Fri, Jan 10, 2020 at 9:38 PM Balamuruhan S <bala24@linux.ibm.com> wrote:
>
> On Tue, Nov 26, 2019 at 04:21:29PM +1100, Jordan Niethe wrote:
> > This adds emulation support for the following prefixed integer
> > load/stores:
> > * Prefixed Load Byte and Zero (plbz)
> > * Prefixed Load Halfword and Zero (plhz)
> > * Prefixed Load Halfword Algebraic (plha)
> > * Prefixed Load Word and Zero (plwz)
> > * Prefixed Load Word Algebraic (plwa)
> > * Prefixed Load Doubleword (pld)
> > * Prefixed Store Byte (pstb)
> > * Prefixed Store Halfword (psth)
> > * Prefixed Store Word (pstw)
> > * Prefixed Store Doubleword (pstd)
> > * Prefixed Load Quadword (plq)
> > * Prefixed Store Quadword (pstq)
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > arch/powerpc/lib/sstep.c | 110 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 110 insertions(+)
> >
> > diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> > index ade3f5eba2e5..4f5ad1f602d8 100644
> > --- a/arch/powerpc/lib/sstep.c
> > +++ b/arch/powerpc/lib/sstep.c
> > @@ -187,6 +187,43 @@ static nokprobe_inline unsigned long xform_ea(unsigned int instr,
> > return ea;
> > }
> >
> > +/*
> > + * Calculate effective address for a MLS:D-form / 8LS:D-form prefixed instruction
> > + */
> > +static nokprobe_inline unsigned long mlsd_8lsd_ea(unsigned int instr,
> > + unsigned int sufx,
> > + const struct pt_regs *regs)
> > +{
> > + int ra, prefix_r;
> > + unsigned int dd;
> > + unsigned long ea, d0, d1, d;
> > +
> > + prefix_r = instr & (1ul << 20);
> > + ra = (sufx >> 16) & 0x1f;
> > +
> > + d0 = instr & 0x3ffff;
> > + d1 = sufx & 0xffff;
> > + d = (d0 << 16) | d1;
> > +
> > + /*
> > + * sign extend a 34 bit number
> > + */
> > + dd = (unsigned int) (d >> 2);
> > + ea = (signed int) dd;
> > + ea = (ea << 2) | (d & 0x3);
> > +
> > + if (!prefix_r && ra)
> > + ea += regs->gpr[ra];
> > + else if (!prefix_r && !ra)
> > + ; /* Leave ea as is */
> > + else if (prefix_r && !ra)
> > + ea += regs->nip;
> > + else if (prefix_r && ra)
> > + ; /* Invalid form. Should already be checked for by caller! */
> > +
> > + return ea;
> > +}
> > +
> > /*
> > * Return the largest power of 2, not greater than sizeof(unsigned long),
> > * such that x is a multiple of it.
> > @@ -1166,6 +1203,7 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> > unsigned int instr, unsigned int sufx)
> > {
> > unsigned int opcode, ra, rb, rc, rd, spr, u;
> > + unsigned int sufxopcode, prefixtype, prefix_r;
> > unsigned long int imm;
> > unsigned long int val, val2;
> > unsigned int mb, me, sh;
> > @@ -2652,6 +2690,78 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> >
> > }
> >
> > +/*
> > + * Prefixed instructions
> > + */
> > + switch (opcode) {
> > + case 1:
> > + prefix_r = instr & (1ul << 20);
> > + ra = (sufx >> 16) & 0x1f;
> > + op->update_reg = ra;
> > + rd = (sufx >> 21) & 0x1f;
> > + op->reg = rd;
> > + op->val = regs->gpr[rd];
> > +
> > + sufxopcode = sufx >> 26;
> > + prefixtype = (instr >> 24) & 0x3;
> > + switch (prefixtype) {
> > + case 0: /* Type 00 Eight-Byte Load/Store */
> > + if (prefix_r && ra)
> > + break;
> > + op->ea = mlsd_8lsd_ea(instr, sufx, regs);
> > + switch (sufxopcode) {
> > + case 41: /* plwa */
> > + op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 4);
> > + break;
> > + case 56: /* plq */
> > + op->type = MKOP(LOAD, PREFIXED, 16);
> > + break;
> > + case 57: /* pld */
> > + op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 8);
> > + break;
> > + case 60: /* stq */
> > + op->type = MKOP(STORE, PREFIXED, 16);
> > + break;
> > + case 61: /* pstd */
> > + op->type = MKOP(STORE, PREFIXED | SIGNEXT, 8);
>
> For 8 byte and and 1 byte (mentioned below for Type 10 instructions), we
> do not have their respective definitions in `do_signext()`, I am not
> sure whether it is typo/miss.
This was a mistake. pstd/pld should not have been flagged with SIGNEXT.
There still are only algebraic loads for word and halfword lengths so
do_signext()
is fine.
>
> > + break;
> > + }
> > + break;
> > + case 1: /* Type 01 Modified Register-to-Register */
>
> Type 01 would be Eight-Byte Register-to-Register.
Thanks, you are right.
>
> -- Bala
> > + break;
> > + case 2: /* Type 10 Modified Load/Store */
> > + if (prefix_r && ra)
> > + break;
> > + op->ea = mlsd_8lsd_ea(instr, sufx, regs);
> > + switch (sufxopcode) {
> > + case 32: /* plwz */
> > + op->type = MKOP(LOAD, PREFIXED, 4);
> > + break;
> > + case 34: /* plbz */
> > + op->type = MKOP(LOAD, PREFIXED, 1);
> > + break;
> > + case 36: /* pstw */
> > + op->type = MKOP(STORE, PREFIXED, 4);
> > + break;
> > + case 38: /* pstb */
> > + op->type = MKOP(STORE, PREFIXED, 1);
> > + break;
> > + case 40: /* plhz */
> > + op->type = MKOP(LOAD, PREFIXED, 2);
> > + break;
> > + case 42: /* plha */
> > + op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 2);
> > + break;
> > + case 44: /* psth */
> > + op->type = MKOP(STORE, PREFIXED, 2);
> > + break;
> > + }
> > + break;
> > + case 3: /* Type 11 Modified Register-to-Register */
> > + break;
> > + }
> > + }
> > +
> > #ifdef CONFIG_VSX
> > if ((GETTYPE(op->type) == LOAD_VSX ||
> > GETTYPE(op->type) == STORE_VSX) &&
> > --
> > 2.20.1
> >
>
^ permalink raw reply
* Re: [PATCH 05/18] powerpc sstep: Prepare to support prefixed instructions
From: Jordan Niethe @ 2020-02-06 23:12 UTC (permalink / raw)
To: Balamuruhan S; +Cc: Alistair Popple, linuxppc-dev
In-Reply-To: <20200113061847.GA21193@dhcp-9-109-246-161.in.ibm.com>
On Mon, Jan 13, 2020 at 5:18 PM Balamuruhan S <bala24@linux.ibm.com> wrote:
>
> On Tue, Nov 26, 2019 at 04:21:28PM +1100, Jordan Niethe wrote:
> > Currently all instructions are a single word long. A future ISA version
> > will include prefixed instructions which have a double word length. The
> > functions used for analysing and emulating instructions need to be
> > modified so that they can handle these new instruction types.
> >
> > A prefixed instruction is a word prefix followed by a word suffix. All
> > prefixes uniquely have the primary op-code 1. Suffixes may be valid word
> > instructions or instructions that only exist as suffixes.
> >
> > In handling prefixed instructions it will be convenient to treat the
> > suffix and prefix as separate words. To facilitate this modify
> > analyse_instr() and emulate_step() to take a take a suffix as a
>
> typo - s/take a take a/take a
Thanks for catching this.
>
> > parameter. For word instructions it does not matter what is passed in
> > here - it will be ignored.
> >
> > We also define a new flag, PREFIXED, to be used in instruction_op:type.
> > This flag will indicate when emulating an analysed instruction if the
> > NIP should be advanced by word length or double word length.
> >
> > The callers of analyse_instr() and emulate_step() will need their own
> > changes to be able to support prefixed instructions. For now modify them
> > to pass in 0 as a suffix.
> >
> > Note that at this point no prefixed instructions are emulated or
> > analysed - this is just making it possible to do so.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > arch/powerpc/include/asm/ppc-opcode.h | 3 +++
> > arch/powerpc/include/asm/sstep.h | 8 +++++--
> > arch/powerpc/include/asm/uaccess.h | 30 +++++++++++++++++++++++++++
> > arch/powerpc/kernel/align.c | 2 +-
> > arch/powerpc/kernel/hw_breakpoint.c | 4 ++--
> > arch/powerpc/kernel/kprobes.c | 2 +-
> > arch/powerpc/kernel/mce_power.c | 2 +-
> > arch/powerpc/kernel/optprobes.c | 2 +-
> > arch/powerpc/kernel/uprobes.c | 2 +-
> > arch/powerpc/kvm/emulate_loadstore.c | 2 +-
> > arch/powerpc/lib/sstep.c | 12 ++++++-----
> > arch/powerpc/lib/test_emulate_step.c | 30 +++++++++++++--------------
> > arch/powerpc/xmon/xmon.c | 4 ++--
> > 13 files changed, 71 insertions(+), 32 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
> > index c1df75edde44..a1dfa4bdd22f 100644
> > --- a/arch/powerpc/include/asm/ppc-opcode.h
> > +++ b/arch/powerpc/include/asm/ppc-opcode.h
> > @@ -377,6 +377,9 @@
> > #define PPC_INST_VCMPEQUD 0x100000c7
> > #define PPC_INST_VCMPEQUB 0x10000006
> >
> > +/* macro to check if a word is a prefix */
> > +#define IS_PREFIX(x) (((x) >> 26) == 1)
> > +
> > /* macros to insert fields into opcodes */
> > #define ___PPC_RA(a) (((a) & 0x1f) << 16)
> > #define ___PPC_RB(b) (((b) & 0x1f) << 11)
> > diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
> > index 769f055509c9..6d4cb602e231 100644
> > --- a/arch/powerpc/include/asm/sstep.h
> > +++ b/arch/powerpc/include/asm/sstep.h
> > @@ -89,6 +89,9 @@ enum instruction_type {
> > #define VSX_LDLEFT 4 /* load VSX register from left */
> > #define VSX_CHECK_VEC 8 /* check MSR_VEC not MSR_VSX for reg >= 32 */
> >
> > +/* Prefixed flag, ORed in with type */
> > +#define PREFIXED 0x800
> > +
> > /* Size field in type word */
> > #define SIZE(n) ((n) << 12)
> > #define GETSIZE(w) ((w) >> 12)
> > @@ -132,7 +135,7 @@ union vsx_reg {
> > * otherwise.
> > */
> > extern int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> > - unsigned int instr);
> > + unsigned int instr, unsigned int sufx);
> >
> > /*
> > * Emulate an instruction that can be executed just by updating
> > @@ -149,7 +152,8 @@ void emulate_update_regs(struct pt_regs *reg, struct instruction_op *op);
> > * 0 if it could not be emulated, or -1 for an instruction that
> > * should not be emulated (rfid, mtmsrd clearing MSR_RI, etc.).
> > */
> > -extern int emulate_step(struct pt_regs *regs, unsigned int instr);
> > +extern int emulate_step(struct pt_regs *regs, unsigned int instr,
> > + unsigned int sufx);
> >
> > /*
> > * Emulate a load or store instruction by reading/writing the
> > diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> > index 15002b51ff18..bc585399e0c7 100644
> > --- a/arch/powerpc/include/asm/uaccess.h
> > +++ b/arch/powerpc/include/asm/uaccess.h
> > @@ -423,4 +423,34 @@ extern long __copy_from_user_flushcache(void *dst, const void __user *src,
> > extern void memcpy_page_flushcache(char *to, struct page *page, size_t offset,
> > size_t len);
> >
> > +/*
> > + * When reading an instruction iff it is a prefix, the suffix needs to be also
> > + * loaded.
> > + */
> > +#define __get_user_instr(x, y, ptr) \
>
> It will be better to have `__get_user_instr()` and
> `__get_user_instr_inatomic()` in separate commit or squashed with patch
> [PATCH 10/18] powerpc: Support prefixed instructions in alignment handler.
>
> -- Bala
Will do.
>
> > +({ \
> > + long __gui_ret = 0; \
> > + y = 0; \
> > + __gui_ret = __get_user(x, ptr); \
> > + if (!__gui_ret) { \
> > + if (IS_PREFIX(x)) \
> > + __gui_ret = __get_user(y, ptr + 1); \
> > + } \
> > + \
> > + __gui_ret; \
> > +})
> > +
> > +#define __get_user_instr_inatomic(x, y, ptr) \
> > +({ \
> > + long __gui_ret = 0; \
> > + y = 0; \
> > + __gui_ret = __get_user_inatomic(x, ptr); \
> > + if (!__gui_ret) { \
> > + if (IS_PREFIX(x)) \
> > + __gui_ret = __get_user_inatomic(y, ptr + 1); \
> > + } \
> > + \
> > + __gui_ret; \
> > +})
> > +
> > #endif /* _ARCH_POWERPC_UACCESS_H */
> > diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
> > index 92045ed64976..245e79792a01 100644
> > --- a/arch/powerpc/kernel/align.c
> > +++ b/arch/powerpc/kernel/align.c
> > @@ -334,7 +334,7 @@ int fix_alignment(struct pt_regs *regs)
> > if ((instr & 0xfc0006fe) == (PPC_INST_COPY & 0xfc0006fe))
> > return -EIO;
> >
> > - r = analyse_instr(&op, regs, instr);
> > + r = analyse_instr(&op, regs, instr, 0);
> > if (r < 0)
> > return -EINVAL;
> >
> > diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> > index 58ce3d37c2a3..f4530961998c 100644
> > --- a/arch/powerpc/kernel/hw_breakpoint.c
> > +++ b/arch/powerpc/kernel/hw_breakpoint.c
> > @@ -248,7 +248,7 @@ static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
> > if (__get_user_inatomic(instr, (unsigned int *)regs->nip))
> > goto fail;
> >
> > - ret = analyse_instr(&op, regs, instr);
> > + ret = analyse_instr(&op, regs, instr, 0);
> > type = GETTYPE(op.type);
> > size = GETSIZE(op.type);
> >
> > @@ -272,7 +272,7 @@ static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
> > return false;
> > }
> >
> > - if (!emulate_step(regs, instr))
> > + if (!emulate_step(regs, instr, 0))
> > goto fail;
> >
> > return true;
> > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> > index 2d27ec4feee4..7303fe3856cc 100644
> > --- a/arch/powerpc/kernel/kprobes.c
> > +++ b/arch/powerpc/kernel/kprobes.c
> > @@ -219,7 +219,7 @@ static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
> > unsigned int insn = *p->ainsn.insn;
> >
> > /* regs->nip is also adjusted if emulate_step returns 1 */
> > - ret = emulate_step(regs, insn);
> > + ret = emulate_step(regs, insn, 0);
> > if (ret > 0) {
> > /*
> > * Once this instruction has been boosted
> > diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c
> > index 1cbf7f1a4e3d..d862bb549158 100644
> > --- a/arch/powerpc/kernel/mce_power.c
> > +++ b/arch/powerpc/kernel/mce_power.c
> > @@ -374,7 +374,7 @@ static int mce_find_instr_ea_and_phys(struct pt_regs *regs, uint64_t *addr,
> > if (pfn != ULONG_MAX) {
> > instr_addr = (pfn << PAGE_SHIFT) + (regs->nip & ~PAGE_MASK);
> > instr = *(unsigned int *)(instr_addr);
> > - if (!analyse_instr(&op, &tmp, instr)) {
> > + if (!analyse_instr(&op, &tmp, instr, 0)) {
> > pfn = addr_to_pfn(regs, op.ea);
> > *addr = op.ea;
> > *phys_addr = (pfn << PAGE_SHIFT);
> > diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
> > index 024f7aad1952..82dc8a589c87 100644
> > --- a/arch/powerpc/kernel/optprobes.c
> > +++ b/arch/powerpc/kernel/optprobes.c
> > @@ -100,7 +100,7 @@ static unsigned long can_optimize(struct kprobe *p)
> > * and that can be emulated.
> > */
> > if (!is_conditional_branch(*p->ainsn.insn) &&
> > - analyse_instr(&op, ®s, *p->ainsn.insn) == 1) {
> > + analyse_instr(&op, ®s, *p->ainsn.insn, 0) == 1) {
> > emulate_update_regs(®s, &op);
> > nip = regs.nip;
> > }
> > diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
> > index 1cfef0e5fec5..ab1077dc6148 100644
> > --- a/arch/powerpc/kernel/uprobes.c
> > +++ b/arch/powerpc/kernel/uprobes.c
> > @@ -173,7 +173,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
> > * emulate_step() returns 1 if the insn was successfully emulated.
> > * For all other cases, we need to single-step in hardware.
> > */
> > - ret = emulate_step(regs, auprobe->insn);
> > + ret = emulate_step(regs, auprobe->insn, 0);
> > if (ret > 0)
> > return true;
> >
> > diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c
> > index 2e496eb86e94..fcab1f31b48d 100644
> > --- a/arch/powerpc/kvm/emulate_loadstore.c
> > +++ b/arch/powerpc/kvm/emulate_loadstore.c
> > @@ -100,7 +100,7 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> >
> > emulated = EMULATE_FAIL;
> > vcpu->arch.regs.msr = vcpu->arch.shared->msr;
> > - if (analyse_instr(&op, &vcpu->arch.regs, inst) == 0) {
> > + if (analyse_instr(&op, &vcpu->arch.regs, inst, 0) == 0) {
> > int type = op.type & INSTR_TYPE_MASK;
> > int size = GETSIZE(op.type);
> >
> > diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> > index c077acb983a1..ade3f5eba2e5 100644
> > --- a/arch/powerpc/lib/sstep.c
> > +++ b/arch/powerpc/lib/sstep.c
> > @@ -1163,7 +1163,7 @@ static nokprobe_inline int trap_compare(long v1, long v2)
> > * otherwise.
> > */
> > int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> > - unsigned int instr)
> > + unsigned int instr, unsigned int sufx)
> > {
> > unsigned int opcode, ra, rb, rc, rd, spr, u;
> > unsigned long int imm;
> > @@ -2756,7 +2756,8 @@ void emulate_update_regs(struct pt_regs *regs, struct instruction_op *op)
> > {
> > unsigned long next_pc;
> >
> > - next_pc = truncate_if_32bit(regs->msr, regs->nip + 4);
> > + next_pc = truncate_if_32bit(regs->msr,
> > + regs->nip + ((op->type & PREFIXED) ? 8 : 4));
> > switch (GETTYPE(op->type)) {
> > case COMPUTE:
> > if (op->type & SETREG)
> > @@ -3101,14 +3102,14 @@ NOKPROBE_SYMBOL(emulate_loadstore);
> > * or -1 if the instruction is one that should not be stepped,
> > * such as an rfid, or a mtmsrd that would clear MSR_RI.
> > */
> > -int emulate_step(struct pt_regs *regs, unsigned int instr)
> > +int emulate_step(struct pt_regs *regs, unsigned int instr, unsigned int sufx)
> > {
> > struct instruction_op op;
> > int r, err, type;
> > unsigned long val;
> > unsigned long ea;
> >
> > - r = analyse_instr(&op, regs, instr);
> > + r = analyse_instr(&op, regs, instr, sufx);
> > if (r < 0)
> > return r;
> > if (r > 0) {
> > @@ -3200,7 +3201,8 @@ int emulate_step(struct pt_regs *regs, unsigned int instr)
> > return 0;
> >
> > instr_done:
> > - regs->nip = truncate_if_32bit(regs->msr, regs->nip + 4);
> > + regs->nip = truncate_if_32bit(regs->msr,
> > + regs->nip + ((op.type & PREFIXED) ? 8 : 4));
> > return 1;
> > }
> > NOKPROBE_SYMBOL(emulate_step);
> > diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
> > index 42347067739c..9288dc6fc715 100644
> > --- a/arch/powerpc/lib/test_emulate_step.c
> > +++ b/arch/powerpc/lib/test_emulate_step.c
> > @@ -103,7 +103,7 @@ static void __init test_ld(void)
> > regs.gpr[3] = (unsigned long) &a;
> >
> > /* ld r5, 0(r3) */
> > - stepped = emulate_step(®s, TEST_LD(5, 3, 0));
> > + stepped = emulate_step(®s, TEST_LD(5, 3, 0), 0);
> >
> > if (stepped == 1 && regs.gpr[5] == a)
> > show_result("ld", "PASS");
> > @@ -121,7 +121,7 @@ static void __init test_lwz(void)
> > regs.gpr[3] = (unsigned long) &a;
> >
> > /* lwz r5, 0(r3) */
> > - stepped = emulate_step(®s, TEST_LWZ(5, 3, 0));
> > + stepped = emulate_step(®s, TEST_LWZ(5, 3, 0), 0);
> >
> > if (stepped == 1 && regs.gpr[5] == a)
> > show_result("lwz", "PASS");
> > @@ -141,7 +141,7 @@ static void __init test_lwzx(void)
> > regs.gpr[5] = 0x8765;
> >
> > /* lwzx r5, r3, r4 */
> > - stepped = emulate_step(®s, TEST_LWZX(5, 3, 4));
> > + stepped = emulate_step(®s, TEST_LWZX(5, 3, 4), 0);
> > if (stepped == 1 && regs.gpr[5] == a[2])
> > show_result("lwzx", "PASS");
> > else
> > @@ -159,7 +159,7 @@ static void __init test_std(void)
> > regs.gpr[5] = 0x5678;
> >
> > /* std r5, 0(r3) */
> > - stepped = emulate_step(®s, TEST_STD(5, 3, 0));
> > + stepped = emulate_step(®s, TEST_STD(5, 3, 0), 0);
> > if (stepped == 1 || regs.gpr[5] == a)
> > show_result("std", "PASS");
> > else
> > @@ -184,7 +184,7 @@ static void __init test_ldarx_stdcx(void)
> > regs.gpr[5] = 0x5678;
> >
> > /* ldarx r5, r3, r4, 0 */
> > - stepped = emulate_step(®s, TEST_LDARX(5, 3, 4, 0));
> > + stepped = emulate_step(®s, TEST_LDARX(5, 3, 4, 0), 0);
> >
> > /*
> > * Don't touch 'a' here. Touching 'a' can do Load/store
> > @@ -202,7 +202,7 @@ static void __init test_ldarx_stdcx(void)
> > regs.gpr[5] = 0x9ABC;
> >
> > /* stdcx. r5, r3, r4 */
> > - stepped = emulate_step(®s, TEST_STDCX(5, 3, 4));
> > + stepped = emulate_step(®s, TEST_STDCX(5, 3, 4), 0);
> >
> > /*
> > * Two possible scenarios that indicates successful emulation
> > @@ -242,7 +242,7 @@ static void __init test_lfsx_stfsx(void)
> > regs.gpr[4] = 0;
> >
> > /* lfsx frt10, r3, r4 */
> > - stepped = emulate_step(®s, TEST_LFSX(10, 3, 4));
> > + stepped = emulate_step(®s, TEST_LFSX(10, 3, 4), 0);
> >
> > if (stepped == 1)
> > show_result("lfsx", "PASS");
> > @@ -255,7 +255,7 @@ static void __init test_lfsx_stfsx(void)
> > c.a = 678.91;
> >
> > /* stfsx frs10, r3, r4 */
> > - stepped = emulate_step(®s, TEST_STFSX(10, 3, 4));
> > + stepped = emulate_step(®s, TEST_STFSX(10, 3, 4), 0);
> >
> > if (stepped == 1 && c.b == cached_b)
> > show_result("stfsx", "PASS");
> > @@ -285,7 +285,7 @@ static void __init test_lfdx_stfdx(void)
> > regs.gpr[4] = 0;
> >
> > /* lfdx frt10, r3, r4 */
> > - stepped = emulate_step(®s, TEST_LFDX(10, 3, 4));
> > + stepped = emulate_step(®s, TEST_LFDX(10, 3, 4), 0);
> >
> > if (stepped == 1)
> > show_result("lfdx", "PASS");
> > @@ -298,7 +298,7 @@ static void __init test_lfdx_stfdx(void)
> > c.a = 987654.32;
> >
> > /* stfdx frs10, r3, r4 */
> > - stepped = emulate_step(®s, TEST_STFDX(10, 3, 4));
> > + stepped = emulate_step(®s, TEST_STFDX(10, 3, 4), 0);
> >
> > if (stepped == 1 && c.b == cached_b)
> > show_result("stfdx", "PASS");
> > @@ -344,7 +344,7 @@ static void __init test_lvx_stvx(void)
> > regs.gpr[4] = 0;
> >
> > /* lvx vrt10, r3, r4 */
> > - stepped = emulate_step(®s, TEST_LVX(10, 3, 4));
> > + stepped = emulate_step(®s, TEST_LVX(10, 3, 4), 0);
> >
> > if (stepped == 1)
> > show_result("lvx", "PASS");
> > @@ -360,7 +360,7 @@ static void __init test_lvx_stvx(void)
> > c.b[3] = 498532;
> >
> > /* stvx vrs10, r3, r4 */
> > - stepped = emulate_step(®s, TEST_STVX(10, 3, 4));
> > + stepped = emulate_step(®s, TEST_STVX(10, 3, 4), 0);
> >
> > if (stepped == 1 && cached_b[0] == c.b[0] && cached_b[1] == c.b[1] &&
> > cached_b[2] == c.b[2] && cached_b[3] == c.b[3])
> > @@ -401,7 +401,7 @@ static void __init test_lxvd2x_stxvd2x(void)
> > regs.gpr[4] = 0;
> >
> > /* lxvd2x vsr39, r3, r4 */
> > - stepped = emulate_step(®s, TEST_LXVD2X(39, 3, 4));
> > + stepped = emulate_step(®s, TEST_LXVD2X(39, 3, 4), 0);
> >
> > if (stepped == 1 && cpu_has_feature(CPU_FTR_VSX)) {
> > show_result("lxvd2x", "PASS");
> > @@ -421,7 +421,7 @@ static void __init test_lxvd2x_stxvd2x(void)
> > c.b[3] = 4;
> >
> > /* stxvd2x vsr39, r3, r4 */
> > - stepped = emulate_step(®s, TEST_STXVD2X(39, 3, 4));
> > + stepped = emulate_step(®s, TEST_STXVD2X(39, 3, 4), 0);
> >
> > if (stepped == 1 && cached_b[0] == c.b[0] && cached_b[1] == c.b[1] &&
> > cached_b[2] == c.b[2] && cached_b[3] == c.b[3] &&
> > @@ -848,7 +848,7 @@ static int __init emulate_compute_instr(struct pt_regs *regs,
> > if (!regs || !instr)
> > return -EINVAL;
> >
> > - if (analyse_instr(&op, regs, instr) != 1 ||
> > + if (analyse_instr(&op, regs, instr, 0) != 1 ||
> > GETTYPE(op.type) != COMPUTE) {
> > pr_info("emulation failed, instruction = 0x%08x\n", instr);
> > return -EFAULT;
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index a7056049709e..f47bd843dc52 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -705,7 +705,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
> > if ((regs->msr & (MSR_IR|MSR_PR|MSR_64BIT)) == (MSR_IR|MSR_64BIT)) {
> > bp = at_breakpoint(regs->nip);
> > if (bp != NULL) {
> > - int stepped = emulate_step(regs, bp->instr[0]);
> > + int stepped = emulate_step(regs, bp->instr[0], 0);
> > if (stepped == 0) {
> > regs->nip = (unsigned long) &bp->instr[0];
> > atomic_inc(&bp->ref_count);
> > @@ -1170,7 +1170,7 @@ static int do_step(struct pt_regs *regs)
> > /* check we are in 64-bit kernel mode, translation enabled */
> > if ((regs->msr & (MSR_64BIT|MSR_PR|MSR_IR)) == (MSR_64BIT|MSR_IR)) {
> > if (mread(regs->nip, &instr, 4) == 4) {
> > - stepped = emulate_step(regs, instr);
> > + stepped = emulate_step(regs, instr, 0);
> > if (stepped < 0) {
> > printf("Couldn't single-step %s instruction\n",
> > (IS_RFID(instr)? "rfid": "mtmsrd"));
> > --
> > 2.20.1
> >
>
^ permalink raw reply
* Re: [PATCH 15/18] powerpc/uprobes: Add support for prefixed instructions
From: Jordan Niethe @ 2020-02-06 23:09 UTC (permalink / raw)
To: Balamuruhan S; +Cc: Alistair Popple, linuxppc-dev
In-Reply-To: <20200113113015.GA5532@dhcp-9-109-246-161.in.ibm.com>
On Mon, Jan 13, 2020 at 10:30 PM Balamuruhan S <bala24@linux.ibm.com> wrote:
>
> On Tue, Nov 26, 2019 at 04:21:38PM +1100, Jordan Niethe wrote:
> > Uprobes can execute instructions out of line. Increase the size of the
> > buffer used for this so that this works for prefixed instructions. Take
> > into account the length of prefixed instructions when fixing up the nip.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > arch/powerpc/include/asm/uprobes.h | 18 ++++++++++++++----
> > arch/powerpc/kernel/uprobes.c | 4 ++--
> > 2 files changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/uprobes.h b/arch/powerpc/include/asm/uprobes.h
> > index 2bbdf27d09b5..5b5e8a3d2f55 100644
> > --- a/arch/powerpc/include/asm/uprobes.h
> > +++ b/arch/powerpc/include/asm/uprobes.h
> > @@ -14,18 +14,28 @@
> >
> > typedef ppc_opcode_t uprobe_opcode_t;
> >
> > +/*
> > + * We have to ensure we have enought space for prefixed instructions, which
>
> minor typo of `enought` and we can have something like below,
Thanks for catching that.
>
> s/We have to ensure we have enought/Ensure we have enough
Will do.
>
> -- Bala
>
> > + * are double the size of a word instruction, i.e. 8 bytes. However,
> > + * sometimes it is simpler to treat a prefixed instruction like 2 word
> > + * instructions.
> > + */
> > #define MAX_UINSN_BYTES 4
> > -#define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES)
> > +#define UPROBE_XOL_SLOT_BYTES (2 * MAX_UINSN_BYTES)
> >
> > /* The following alias is needed for reference from arch-agnostic code */
> > #define UPROBE_SWBP_INSN BREAKPOINT_INSTRUCTION
> > #define UPROBE_SWBP_INSN_SIZE 4 /* swbp insn size in bytes */
> >
> > struct arch_uprobe {
> > + /*
> > + * Ensure there is enough space for prefixed instructions. Prefixed
> > + * instructions must not cross 64-byte boundaries.
> > + */
> > union {
> > - u32 insn;
> > - u32 ixol;
> > - };
> > + uprobe_opcode_t insn[2];
> > + uprobe_opcode_t ixol[2];
> > + } __aligned(64);
> > };
> >
> > struct arch_uprobe_task {
> > diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
> > index ab1077dc6148..cfcea6946f8b 100644
> > --- a/arch/powerpc/kernel/uprobes.c
> > +++ b/arch/powerpc/kernel/uprobes.c
> > @@ -111,7 +111,7 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> > * support doesn't exist and have to fix-up the next instruction
> > * to be executed.
> > */
> > - regs->nip = utask->vaddr + MAX_UINSN_BYTES;
> > + regs->nip = utask->vaddr + ((IS_PREFIX(auprobe->insn[0])) ? 8 : 4);
> >
> > user_disable_single_step(current);
> > return 0;
> > @@ -173,7 +173,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
> > * emulate_step() returns 1 if the insn was successfully emulated.
> > * For all other cases, we need to single-step in hardware.
> > */
> > - ret = emulate_step(regs, auprobe->insn, 0);
> > + ret = emulate_step(regs, auprobe->insn[0], auprobe->insn[1]);
> > if (ret > 0)
> > return true;
> >
> > --
> > 2.20.1
> >
>
^ permalink raw reply
* Re: [PATCH 0/3] soc: fsl: dpio: Enable QMAN batch enqueuing
From: Roy Pledge @ 2020-02-06 20:39 UTC (permalink / raw)
To: Youri Querry, Roy Pledge, Leo Li, linux-kernel@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org, Ioana Ciornei,
Alexandru Marginean
In-Reply-To: <1576170032-3124-1-git-send-email-youri.querry_1@nxp.com>
On 12/12/2019 12:01 PM, Youri Querry wrote:
> This patch set consists of:
> - We added an interface to enqueue several packets at a time and
> improve performance.
> - Make the algorithm decisions once at initialization and use
> function pointers to improve performance.
> - Replaced the QMAN enqueue array mode algorithm with a ring
> mode algorithm. This is to make the enqueue of several frames
> at a time more effective.
>
> Youri Querry (3):
> soc: fsl: dpio: Adding QMAN multiple enqueue interface.
> soc: fsl: dpio: QMAN performance improvement. Function pointer
> indirection.
> soc: fsl: dpio: Replace QMAN array mode by ring mode enqueue.
>
> drivers/soc/fsl/dpio/dpio-service.c | 69 +++-
> drivers/soc/fsl/dpio/qbman-portal.c | 766 ++++++++++++++++++++++++++++++++----
> drivers/soc/fsl/dpio/qbman-portal.h | 158 +++++++-
> include/soc/fsl/dpaa2-io.h | 6 +-
> 4 files changed, 907 insertions(+), 92 deletions(-)
>
Acked-by: Roy Pledge <roy.pledge@nxp.com>
Leo - can you look at this series so we can get it integrated? Thanks
^ permalink raw reply
* Re: [PATCH v2 2/3] selftests/powerpc: Add tm-signal-pagefault test
From: Gustavo Luiz Duarte @ 2020-02-06 22:16 UTC (permalink / raw)
To: Michael Ellerman, Gustavo Luiz Duarte, linuxppc-dev; +Cc: mikey, gromero
In-Reply-To: <8736bp38o7.fsf@mpe.ellerman.id.au>
On 2/5/20 2:27 AM, Michael Ellerman wrote:
> Gustavo Luiz Duarte <gustavold@linux.ibm.com> writes:
>> This test triggers a TM Bad Thing by raising a signal in transactional state
>> and forcing a pagefault to happen in kernelspace when the kernel signal
>> handling code first touches the user signal stack.
>>
>> This is inspired by the test tm-signal-context-force-tm but uses userfaultfd to
>> make the test deterministic. While this test always triggers the bug in one
>> run, I had to execute tm-signal-context-force-tm several times (the test runs
>> 5000 times each execution) to trigger the same bug.
>
> Using userfaultfd is a very nice touch. But it's not always enabled,
> which leads to eg:
>
> root@mpe-ubuntu-le:~# /home/michael/tm-signal-pagefault
> test: tm_signal_pagefault
> tags: git_version:v5.5-9354-gc1e346e7fc44
> userfaultfd() failed: Function not implemented
> failure: tm_signal_pagefault
>
> It would be nice if that resulted in a skip, not a failure.
>
> It looks like it shouldn't be too hard to skip if the userfaultfd call
> returns ENOSYS.
Good point. I will fix that on v3.
>
> cheers
>
^ permalink raw reply
* Re: [PATCH v2 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim
From: Gustavo Luiz Duarte @ 2020-02-06 22:13 UTC (permalink / raw)
To: Michael Neuling, Gustavo Luiz Duarte, linuxppc-dev; +Cc: stable, gromero
In-Reply-To: <0af388c6a08d83ee7816fc3fc6053c905dc58344.camel@neuling.org>
On 2/5/20 1:58 AM, Michael Neuling wrote:
> Other than the minor things below that I think you need, the patch good with me.
>
> Acked-by: Michael Neuling <mikey@neuling.org>
>
>> Subject: Re: [PATCH v2 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim
>
> The subject should mention "signals".
How about "powerpc/tm: Clear the current thread's MSR[TS] when
transaction is reclaimed on signal delivery" ?
>
> On Mon, 2020-02-03 at 13:09 -0300, Gustavo Luiz Duarte wrote:
>> After a treclaim, we expect to be in non-transactional state. If we don't
>> immediately clear the current thread's MSR[TS] and we get preempted, then
>> tm_recheckpoint_new_task() will recheckpoint and we get rescheduled in
>> suspended transaction state.
>
> It's not "immediately", it's before re-enabling preemption.
>
> There is a similar comment in the code that needs to be fixed too.
OK.
>
>> When handling a signal caught in transactional state, handle_rt_signal64()
>> calls get_tm_stackpointer() that treclaims the transaction using
>> tm_reclaim_current() but without clearing the thread's MSR[TS]. This can cause
>> the TM Bad Thing exception below if later we pagefault and get preempted trying
>> to access the user's sigframe, using __put_user(). Afterwards, when we are
>> rescheduled back into do_page_fault() (but now in suspended state since the
>> thread's MSR[TS] was not cleared), upon executing 'rfid' after completion of
>> the page fault handling, the exception is raised because a transition from
>> suspended to non-transactional state is invalid.
>>
>> Unexpected TM Bad Thing exception at c00000000000de44 (msr 0x8000000302a03031) tm_scratch=800000010280b033
>> Oops: Unrecoverable exception, sig: 6 [#1]
>> LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
>> Modules linked in: nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip6_tables ip_tables nft_compat ip_set nf_tables nfnetlink xts vmx_crypto sg virtio_balloon
>> r_mod cdrom virtio_net net_failover virtio_blk virtio_scsi failover dm_mirror dm_region_hash dm_log dm_mod
>> CPU: 25 PID: 15547 Comm: a.out Not tainted 5.4.0-rc2 #32
>> NIP: c00000000000de44 LR: c000000000034728 CTR: 0000000000000000
>> REGS: c00000003fe7bd70 TRAP: 0700 Not tainted (5.4.0-rc2)
>> MSR: 8000000302a03031 <SF,VEC,VSX,FP,ME,IR,DR,LE,TM[SE]> CR: 44000884 XER: 00000000
>> CFAR: c00000000000dda4 IRQMASK: 0
>> PACATMSCRATCH: 800000010280b033
>> GPR00: c000000000034728 c000000f65a17c80 c000000001662800 00007fffacf3fd78
>> GPR04: 0000000000001000 0000000000001000 0000000000000000 c000000f611f8af0
>> GPR08: 0000000000000000 0000000078006001 0000000000000000 000c000000000000
>> GPR12: c000000f611f84b0 c00000003ffcb200 0000000000000000 0000000000000000
>> GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> GPR20: 0000000000000000 0000000000000000 0000000000000000 c000000f611f8140
>> GPR24: 0000000000000000 00007fffacf3fd68 c000000f65a17d90 c000000f611f7800
>> GPR28: c000000f65a17e90 c000000f65a17e90 c000000001685e18 00007fffacf3f000
>> NIP [c00000000000de44] fast_exception_return+0xf4/0x1b0
>> LR [c000000000034728] handle_rt_signal64+0x78/0xc50
>> Call Trace:
>> [c000000f65a17c80] [c000000000034710] handle_rt_signal64+0x60/0xc50 (unreliable)
>> [c000000f65a17d30] [c000000000023640] do_notify_resume+0x330/0x460
>> [c000000f65a17e20] [c00000000000dcc4] ret_from_except_lite+0x70/0x74
>> Instruction dump:
>> 7c4ff120 e8410170 7c5a03a6 38400000 f8410060 e8010070 e8410080 e8610088
>> 60000000 60000000 e8810090 e8210078 <4c000024> 48000000 e8610178 88ed0989
>> ---[ end trace 93094aa44b442f87 ]---
>>
>> The simplified sequence of events that triggers the above exception is:
>>
>> ... # userspace in NON-TRANSACTIONAL state
>> tbegin # userspace in TRANSACTIONAL state
>> signal delivery # kernelspace in SUSPENDED state
>> handle_rt_signal64()
>> get_tm_stackpointer()
>> treclaim # kernelspace in NON-TRANSACTIONAL state
>> __put_user()
>> page fault happens. We will never get back here because of the TM Bad Thing exception.
>>
>> page fault handling kicks in and we voluntarily preempt ourselves
>> do_page_fault()
>> __schedule()
>> __switch_to(other_task)
>>
>> our task is rescheduled and we recheckpoint because the thread's MSR[TS] was not cleared
>> __switch_to(our_task)
>> switch_to_tm()
>> tm_recheckpoint_new_task()
>> trechkpt # kernelspace in SUSPENDED state
>>
>> The page fault handling resumes, but now we are in suspended transaction state
>> do_page_fault() completes
>> rfid <----- trying to get back where the page fault happened (we were non-transactional back then)
>> TM Bad Thing # illegal transition from suspended to non-transactional
>>
>> This patch fixes that issue by clearing the current thread's MSR[TS] just after
>> treclaim in get_tm_stackpointer() so that we stay in non-transactional state in
>> case we are preempted. In order to make treclaim and clearing the thread's
>> MSR[TS] atomic from a preemption perspective when CONFIG_PREEMPT is set,
>> preempt_disable/enable() is used. It's also necessary to save the previous
>> value of the thread's MSR before get_tm_stackpointer() is called so that it can
>> be exposed to the signal handler later in setup_tm_sigcontexts() to inform the
>> userspace MSR at the moment of the signal delivery.
>>
>> Found with tm-signal-context-force-tm kernel selftest on P8 KVM.
>
> Why are you mentioning KVM?
That is just what I used... I agree that the issue has nothing to do
with KVM. I will remove that on v3.
>
>>
>> v2: Fix build failure when tm is disabled.
>>
>> Fixes: 2b0a576d15e0 ("powerpc: Add new transactional memory state to the signal context")
>> Cc: stable@vger.kernel.org # v3.9
>> Signed-off-by: Gustavo Luiz Duarte <gustavold@linux.ibm.com>
>> ---
>> arch/powerpc/kernel/signal.c | 17 +++++++++++++++--
>> arch/powerpc/kernel/signal_32.c | 28 ++++++++++++++--------------
>> arch/powerpc/kernel/signal_64.c | 22 ++++++++++------------
>> 3 files changed, 39 insertions(+), 28 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
>> index e6c30cee6abf..1660be1061ac 100644
>> --- a/arch/powerpc/kernel/signal.c
>> +++ b/arch/powerpc/kernel/signal.c
>> @@ -200,14 +200,27 @@ unsigned long get_tm_stackpointer(struct task_struct *tsk)
>> * normal/non-checkpointed stack pointer.
>> */
>>
>> + unsigned long ret = tsk->thread.regs->gpr[1];
>> +
>> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> BUG_ON(tsk != current);
>>
>> if (MSR_TM_ACTIVE(tsk->thread.regs->msr)) {
>> + preempt_disable();
>> tm_reclaim_current(TM_CAUSE_SIGNAL);
>> if (MSR_TM_TRANSACTIONAL(tsk->thread.regs->msr))
>> - return tsk->thread.ckpt_regs.gpr[1];
>> + ret = tsk->thread.ckpt_regs.gpr[1];
>> +
>> + /* If we treclaim, we must immediately clear the current
>> + * thread's TM bits. Otherwise we might be preempted and have
>> + * the live MSR[TS] changed behind our back
>> + * (tm_recheckpoint_new_task() would recheckpoint).
>> + * Besides, we enter the signal handler in non-transactional
>> + * state.
>> + */
>> + tsk->thread.regs->msr &= ~MSR_TS_MASK;
>> + preempt_enable();
>> }
>> #endif
>> - return tsk->thread.regs->gpr[1];
>> + return ret;
>> }
>> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
>> index 98600b276f76..1b090a76b444 100644
>> --- a/arch/powerpc/kernel/signal_32.c
>> +++ b/arch/powerpc/kernel/signal_32.c
>> @@ -489,19 +489,11 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
>> */
>> static int save_tm_user_regs(struct pt_regs *regs,
>> struct mcontext __user *frame,
>> - struct mcontext __user *tm_frame, int sigret)
>> + struct mcontext __user *tm_frame, int sigret,
>> + unsigned long msr)
>> {
>> - unsigned long msr = regs->msr;
>> -
>> WARN_ON(tm_suspend_disabled);
>>
>> - /* Remove TM bits from thread's MSR. The MSR in the sigcontext
>> - * just indicates to userland that we were doing a transaction, but we
>> - * don't want to return in transactional state. This also ensures
>> - * that flush_fp_to_thread won't set TIF_RESTORE_TM again.
>> - */
>> - regs->msr &= ~MSR_TS_MASK;
>> -
>> /* Save both sets of general registers */
>> if (save_general_regs(¤t->thread.ckpt_regs, frame)
>> || save_general_regs(regs, tm_frame))
>> @@ -912,6 +904,10 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
>> int sigret;
>> unsigned long tramp;
>> struct pt_regs *regs = tsk->thread.regs;
>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> + /* Save the thread's msr before get_tm_stackpointer() changes it */
>> + unsigned long msr = regs->msr;
>> +#endif
>>
>> BUG_ON(tsk != current);
>>
>> @@ -944,13 +940,13 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
>>
>> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> tm_frame = &rt_sf->uc_transact.uc_mcontext;
>> - if (MSR_TM_ACTIVE(regs->msr)) {
>> + if (MSR_TM_ACTIVE(msr)) {
>> if (__put_user((unsigned long)&rt_sf->uc_transact,
>> &rt_sf->uc.uc_link) ||
>> __put_user((unsigned long)tm_frame,
>> &rt_sf->uc_transact.uc_regs))
>> goto badframe;
>> - if (save_tm_user_regs(regs, frame, tm_frame, sigret))
>> + if (save_tm_user_regs(regs, frame, tm_frame, sigret, msr))
>> goto badframe;
>> }
>> else
>> @@ -1369,6 +1365,10 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
>> int sigret;
>> unsigned long tramp;
>> struct pt_regs *regs = tsk->thread.regs;
>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> + /* Save the thread's msr before get_tm_stackpointer() changes it */
>> + unsigned long msr = regs->msr;
>> +#endif
>>
>> BUG_ON(tsk != current);
>>
>> @@ -1402,9 +1402,9 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
>>
>> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> tm_mctx = &frame->mctx_transact;
>> - if (MSR_TM_ACTIVE(regs->msr)) {
>> + if (MSR_TM_ACTIVE(msr)) {
>> if (save_tm_user_regs(regs, &frame->mctx, &frame->mctx_transact,
>> - sigret))
>> + sigret, msr))
>> goto badframe;
>> }
>> else
>> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
>> index 117515564ec7..84ed2e77ef9c 100644
>> --- a/arch/powerpc/kernel/signal_64.c
>> +++ b/arch/powerpc/kernel/signal_64.c
>> @@ -192,7 +192,8 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>> static long setup_tm_sigcontexts(struct sigcontext __user *sc,
>> struct sigcontext __user *tm_sc,
>> struct task_struct *tsk,
>> - int signr, sigset_t *set, unsigned long handler)
>> + int signr, sigset_t *set, unsigned long handler,
>> + unsigned long msr)
>> {
>> /* When CONFIG_ALTIVEC is set, we _always_ setup v_regs even if the
>> * process never used altivec yet (MSR_VEC is zero in pt_regs of
>> @@ -207,12 +208,11 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
>> elf_vrreg_t __user *tm_v_regs = sigcontext_vmx_regs(tm_sc);
>> #endif
>> struct pt_regs *regs = tsk->thread.regs;
>> - unsigned long msr = tsk->thread.regs->msr;
>> long err = 0;
>>
>> BUG_ON(tsk != current);
>>
>> - BUG_ON(!MSR_TM_ACTIVE(regs->msr));
>> + BUG_ON(!MSR_TM_ACTIVE(msr));
>>
>> WARN_ON(tm_suspend_disabled);
>>
>> @@ -222,13 +222,6 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
>> */
>> msr |= tsk->thread.ckpt_regs.msr & (MSR_FP | MSR_VEC | MSR_VSX);
>>
>> - /* Remove TM bits from thread's MSR. The MSR in the sigcontext
>> - * just indicates to userland that we were doing a transaction, but we
>> - * don't want to return in transactional state. This also ensures
>> - * that flush_fp_to_thread won't set TIF_RESTORE_TM again.
>> - */
>> - regs->msr &= ~MSR_TS_MASK;
>> -
>> #ifdef CONFIG_ALTIVEC
>> err |= __put_user(v_regs, &sc->v_regs);
>> err |= __put_user(tm_v_regs, &tm_sc->v_regs);
>> @@ -824,6 +817,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>> unsigned long newsp = 0;
>> long err = 0;
>> struct pt_regs *regs = tsk->thread.regs;
>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> + /* Save the thread's msr before get_tm_stackpointer() changes it */
>> + unsigned long msr = regs->msr;
>> +#endif
>>
>> BUG_ON(tsk != current);
>>
>> @@ -841,7 +838,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>> err |= __put_user(0, &frame->uc.uc_flags);
>> err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]);
>> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> - if (MSR_TM_ACTIVE(regs->msr)) {
>> + if (MSR_TM_ACTIVE(msr)) {
>> /* The ucontext_t passed to userland points to the second
>> * ucontext_t (for transactional state) with its uc_link ptr.
>> */
>> @@ -849,7 +846,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>> err |= setup_tm_sigcontexts(&frame->uc.uc_mcontext,
>> &frame->uc_transact.uc_mcontext,
>> tsk, ksig->sig, NULL,
>> - (unsigned long)ksig->ka.sa.sa_handler);
>> + (unsigned long)ksig->ka.sa.sa_handler,
>> + msr);
>> } else
>> #endif
>> {
>
^ permalink raw reply
* RE: [PATCH 0/3] soc: fsl: dpio: Enable QMAN batch enqueuing
From: Leo Li @ 2020-02-06 22:05 UTC (permalink / raw)
To: Roy Pledge (OSS), Youri Querry, Roy Pledge,
linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org, Ioana Ciornei,
Alexandru Marginean
In-Reply-To: <a46accbc-becf-ad23-8504-70ce619e2b11@oss.nxp.com>
> -----Original Message-----
> From: Roy Pledge (OSS) <roy.pledge@oss.nxp.com>
> Sent: Thursday, February 6, 2020 2:40 PM
> To: Youri Querry <youri.querry_1@nxp.com>; Roy Pledge
> <roy.pledge@nxp.com>; Leo Li <leoyang.li@nxp.com>; linux-
> kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-arm-
> kernel@lists.infradead.org; Ioana Ciornei <ioana.ciornei@nxp.com>;
> Alexandru Marginean <alexandru.marginean@nxp.com>
> Subject: Re: [PATCH 0/3] soc: fsl: dpio: Enable QMAN batch enqueuing
>
> On 12/12/2019 12:01 PM, Youri Querry wrote:
> > This patch set consists of:
> > - We added an interface to enqueue several packets at a time and
> > improve performance.
> > - Make the algorithm decisions once at initialization and use
> > function pointers to improve performance.
> > - Replaced the QMAN enqueue array mode algorithm with a ring
> > mode algorithm. This is to make the enqueue of several frames
> > at a time more effective.
> >
> > Youri Querry (3):
> > soc: fsl: dpio: Adding QMAN multiple enqueue interface.
> > soc: fsl: dpio: QMAN performance improvement. Function pointer
> > indirection.
> > soc: fsl: dpio: Replace QMAN array mode by ring mode enqueue.
> >
> > drivers/soc/fsl/dpio/dpio-service.c | 69 +++-
> > drivers/soc/fsl/dpio/qbman-portal.c | 766
> ++++++++++++++++++++++++++++++++----
> > drivers/soc/fsl/dpio/qbman-portal.h | 158 +++++++-
> > include/soc/fsl/dpaa2-io.h | 6 +-
> > 4 files changed, 907 insertions(+), 92 deletions(-)
> >
> Acked-by: Roy Pledge <roy.pledge@nxp.com>
>
> Leo - can you look at this series so we can get it integrated? Thanks
Sure. Thanks for the review. I will queue them up for v5.7.
Regards,
Leo
^ permalink raw reply
* Re: [PATCH v5 17/17] powerpc/32s: Enable CONFIG_VMAP_STACK
From: Guenter Roeck @ 2020-02-06 20:31 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-kernel, linux-mm, Paul Mackerras, linuxppc-dev, dja
In-Reply-To: <2e2509a242fd5f3e23df4a06530c18060c4d321e.1576916812.git.christophe.leroy@c-s.fr>
On Sat, Dec 21, 2019 at 08:32:38AM +0000, Christophe Leroy wrote:
> A few changes to retrieve DAR and DSISR from struct regs
> instead of retrieving them directly, as they may have
> changed due to a TLB miss.
>
> Also modifies hash_page() and friends to work with virtual
> data addresses instead of physical ones. Same on load_up_fpu()
> and load_up_altivec().
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
This patch results in qemu boot failures (mac99 with pmac32_defconfig).
Images fail silently; there is no console output. Reverting the patch
fixes the problem. Bisect log is attached below.
Assuming this was tested on real hardware, am I correct to assume that qemu
for ppc32 (more specifically, qemu's mac99 and g3beige machines) no longer
works with the upstream kernel ?
Thanks,
Guenter
---
# bad: [33b40134e5cfbbccad7f3040d1919889537a3df7] Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
# good: [d4e9056daedca3891414fe3c91de3449a5dad0f2] initramfs: do not show compression mode choice if INITRAMFS_SOURCE is empty
git bisect start '33b40134e5cf' 'd4e9056daedc'
# good: [9717c1cea16e3eae81ca226f4c3670bb799b61ad] Merge tag 'drm-next-2020-02-04' of git://anongit.freedesktop.org/drm/drm
git bisect good 9717c1cea16e3eae81ca226f4c3670bb799b61ad
# bad: [4c25df5640ae6e4491ee2c50d3f70c1559ef037d] Merge branch 'topic/user-access-begin' into next
git bisect bad 4c25df5640ae6e4491ee2c50d3f70c1559ef037d
# good: [846a17a53aeeee426cbc1252f517a6660eab1427] powerpc/powernv: Use common code for the symbol_map export
git bisect good 846a17a53aeeee426cbc1252f517a6660eab1427
# good: [afe1ec5ab83029baf0f8368a255dc6b998bde576] powerpc/8xx: Split breakpoint exception
git bisect good afe1ec5ab83029baf0f8368a255dc6b998bde576
# bad: [43e76cd368fbb67e767da5363ffeaa3989993c8c] powerpc: Do not consider weak unresolved symbol relocations as bad
git bisect bad 43e76cd368fbb67e767da5363ffeaa3989993c8c
# bad: [af1725d2493dcad4eeb7e63141678181fcd8a2ff] powerpc/kconfig: Move CONFIG_PPC32 into Kconfig.cputype
git bisect bad af1725d2493dcad4eeb7e63141678181fcd8a2ff
# good: [94dd54c51a410b9ffa6356c3ed2ab0317f998ded] powerpc/32s: Avoid crossing page boundary while changing SRR0/1.
git bisect good 94dd54c51a410b9ffa6356c3ed2ab0317f998ded
# bad: [0f9aee0cb9da7db7d96f63cfa2dc5e4f1bffeb87] powerpc/mm: Don't log user reads to 0xffffffff
git bisect bad 0f9aee0cb9da7db7d96f63cfa2dc5e4f1bffeb87
# bad: [cd08f109e26231b279bcc0388428afcac6408ec6] powerpc/32s: Enable CONFIG_VMAP_STACK
git bisect bad cd08f109e26231b279bcc0388428afcac6408ec6
# first bad commit: [cd08f109e26231b279bcc0388428afcac6408ec6] powerpc/32s: Enable CONFIG_VMAP_STACK
^ permalink raw reply
* Re: [PATCH V13] mm/debug: Add tests validating architecture page table helpers
From: Anshuman Khandual @ 2020-02-06 8:19 UTC (permalink / raw)
To: kbuild test robot
Cc: Mark Rutland, uclinux-h8-devel, linux-ia64, linux-sh,
Peter Zijlstra, James Hogan, Tetsuo Handa, Heiko Carstens,
Michal Hocko, linux-mm, Dave Hansen, Paul Mackerras, sparclinux,
linux-hexagon, linux-riscv, Thomas Gleixner, linux-s390,
linux-c6x-dev, Yoshinori Sato, Max Filippov, x86,
Russell King - ARM Linux, Matthew Wilcox, Steven Price,
Jason Gunthorpe, Geert Uytterhoeven, Gerald Schaefer, Mark Salter,
Matt Turner, linux-snps-arc, linux-arm-kernel, Ingo Molnar,
linux-xtensa, Kees Cook, Masahiro Yamada, linux-alpha,
Aurelien Jacquiot, linux-m68k, Mark Brown, Ivan Kokshaysky,
Paul Walmsley, Kirill A . Shutemov, Dan Williams, Guan Xuetao,
Vlastimil Babka, Richard Henderson, Chris Zankel, Michal Simek,
kbuild-all, Brian Cain, Ard Biesheuvel, Greg Kroah-Hartman,
linux-mips, Ralf Baechle, linux-kernel, Paul Burton,
Mike Rapoport, Palmer Dabbelt, Vineet Gupta, Martin Schwidefsky,
Qian Cai, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <202002060619.wEOdAZU1%lkp@intel.com>
On 02/06/2020 04:40 AM, kbuild test robot wrote:
> Hi Anshuman,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on powerpc/next]
> [also build test ERROR on s390/features linus/master arc/for-next v5.5]
> [cannot apply to mmotm/master tip/x86/core arm64/for-next/core next-20200205]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url: https://github.com/0day-ci/linux/commits/Anshuman-Khandual/mm-debug-Add-tests-validating-architecture-page-table-helpers/20200205-215507
> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> config: ia64-allmodconfig (attached as .config)
> compiler: ia64-linux-gcc (GCC) 7.5.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> GCC_VERSION=7.5.0 make.cross ARCH=ia64
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All error/warnings (new ones prefixed by >>):
>
> In file included from include/asm-generic/pgtable-nopud.h:8:0,
> from arch/ia64/include/asm/pgtable.h:586,
> from include/linux/mm.h:99,
> from include/linux/highmem.h:8,
> from mm/debug_vm_pgtable.c:14:
> mm/debug_vm_pgtable.c: In function 'pud_clear_tests':
>>> include/asm-generic/pgtable-nop4d-hack.h:47:32: error: implicit declaration of function '__pgd'; did you mean '__p4d'? [-Werror=implicit-function-declaration]
> #define __pud(x) ((pud_t) { __pgd(x) })
> ^
>>> mm/debug_vm_pgtable.c:141:8: note: in expansion of macro '__pud'
> pud = __pud(pud_val(pud) | RANDOM_ORVALUE);
> ^~~~~
>>> include/asm-generic/pgtable-nop4d-hack.h:47:22: warning: missing braces around initializer [-Wmissing-braces]
> #define __pud(x) ((pud_t) { __pgd(x) })
> ^
>>> mm/debug_vm_pgtable.c:141:8: note: in expansion of macro '__pud'
> pud = __pud(pud_val(pud) | RANDOM_ORVALUE);
> ^~~~~
> cc1: some warnings being treated as errors
This build failure is expected now given that we have allowed DEBUG_VM_PGTABLE
with EXPERT without platform requiring ARCH_HAS_DEBUG_VM_PGTABLE. This problem
i.e build failure caused without a platform __pgd(), is known to exist both on
ia64 and arm (32bit) platforms. Please refer https://lkml.org/lkml/2019/9/24/314
for details where this was discussed earlier.
- Anshuman
^ permalink raw reply
* [PATCH 2/2] powerpc/8xx: Make 8M hugepages also compatible with the 512K pages slice
From: Christophe Leroy @ 2020-02-06 19:21 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
aneesh.kumar
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <4ad03047ac61bfbdad3edb92542dedc807fc3cf4.1581011735.git.christophe.leroy@c-s.fr>
On 8xx, slices are used because hugepages (512k or 8M) and small
pages (4k or 16k) cannot share the same PGD entry. However, as 8M
entirely covers two PGD entries (One PGD entry covers 4M), there
will implicitely be no conflict between 8M pages and any other size.
So 8M is also compatible with the 512K page slices as well.
Handle 512K page slices as fallback compat mask for 8M pages.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/mm/slice.c | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/mm/slice.c b/arch/powerpc/mm/slice.c
index dffe1a45b6ed..cb5b532d3d48 100644
--- a/arch/powerpc/mm/slice.c
+++ b/arch/powerpc/mm/slice.c
@@ -422,6 +422,8 @@ static inline void slice_andnot_mask(struct slice_mask *dst,
#ifdef CONFIG_PPC_64K_PAGES
#define MMU_PAGE_BASE MMU_PAGE_64K
+#elif defined(CONFIG_PPC_16K_PAGES)
+#define MMU_PAGE_BASE MMU_PAGE_16K
#else
#define MMU_PAGE_BASE MMU_PAGE_4K
#endif
@@ -514,9 +516,19 @@ unsigned long slice_get_unmapped_area(unsigned long addr, unsigned long len,
* If we support combo pages, we can allow 64k pages in 4k slices
* The mask copies could be avoided in most cases here if we had
* a pointer to good mask for the next code to use.
+ *
+ * On 8xx, slices are used because hugepages (512k or 8M) and small
+ * pages (4k or 16k) cannot share the same PGD entry. However, as 8M
+ * entirely covers two PGD entries, there will implicitely be no
+ * conflict between 8M pages and any other size. So 8M is compatible
+ * with the 512k page slice as well.
*/
- if (IS_ENABLED(CONFIG_PPC_64K_PAGES) && psize == MMU_PAGE_64K) {
- compat_maskp = slice_mask_for_size(&mm->context, MMU_PAGE_4K);
+ if ((IS_ENABLED(CONFIG_PPC_64K_PAGES) && psize == MMU_PAGE_64K) ||
+ (IS_ENABLED(CONFIG_PPC_8xx) && psize == MMU_PAGE_8M)) {
+ if (IS_ENABLED(CONFIG_PPC_8xx))
+ compat_maskp = slice_mask_for_size(&mm->context, MMU_PAGE_512K);
+ else
+ compat_maskp = slice_mask_for_size(&mm->context, MMU_PAGE_4K);
if (fixed)
slice_or_mask(&good_mask, maskp, compat_maskp);
else
@@ -593,9 +605,8 @@ unsigned long slice_get_unmapped_area(unsigned long addr, unsigned long len,
newaddr = slice_find_area(mm, len, &potential_mask,
psize, topdown, high_limit);
- if (IS_ENABLED(CONFIG_PPC_64K_PAGES) && newaddr == -ENOMEM &&
- psize == MMU_PAGE_64K) {
- /* retry the search with 4k-page slices included */
+ if (compat_maskp && newaddr == -ENOMEM) {
+ /* retry the search with compatible slices included */
slice_or_mask(&potential_mask, &potential_mask, compat_maskp);
newaddr = slice_find_area(mm, len, &potential_mask,
psize, topdown, high_limit);
@@ -767,12 +778,16 @@ int slice_is_hugepage_only_range(struct mm_struct *mm, unsigned long addr,
maskp = slice_mask_for_size(&mm->context, psize);
- /* We need to account for 4k slices too */
- if (IS_ENABLED(CONFIG_PPC_64K_PAGES) && psize == MMU_PAGE_64K) {
+ /* We need to account for compatible slices too */
+ if ((IS_ENABLED(CONFIG_PPC_64K_PAGES) && psize == MMU_PAGE_64K) ||
+ (IS_ENABLED(CONFIG_PPC_8xx) && psize == MMU_PAGE_8M)) {
const struct slice_mask *compat_maskp;
struct slice_mask available;
- compat_maskp = slice_mask_for_size(&mm->context, MMU_PAGE_4K);
+ if (IS_ENABLED(CONFIG_PPC_8xx))
+ compat_maskp = slice_mask_for_size(&mm->context, MMU_PAGE_512K);
+ else
+ compat_maskp = slice_mask_for_size(&mm->context, MMU_PAGE_4K);
slice_or_mask(&available, maskp, compat_maskp);
return !slice_check_range_fits(mm, &available, addr, len);
}
--
2.25.0
^ permalink raw reply related
* [PATCH 1/2] powerpc/8xx: Merge 8M hugepage slice and basepage slice
From: Christophe Leroy @ 2020-02-06 19:21 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
aneesh.kumar
Cc: linuxppc-dev, linux-kernel
On 8xx, slices are used because hugepages (512k or 8M) and small
pages (4k or 16k) cannot share the same PGD entry. However, as 8M
entirely covers two PGD entries (One PGD entry covers 4M), there
will implicitely be no conflict between 8M pages and any other size.
So 8M is compatible with the basepage size as well.
Remove the struct slice_mask mask_8m from mm_context_t and make
vma_mmu_pagesize() rely on vma_kernel_pagesize() as the base
slice can now host several sizes.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/nohash/32/mmu-8xx.h | 7 ++-----
arch/powerpc/mm/hugetlbpage.c | 3 ++-
2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/include/asm/nohash/32/mmu-8xx.h b/arch/powerpc/include/asm/nohash/32/mmu-8xx.h
index 76af5b0cb16e..54f7f3362edb 100644
--- a/arch/powerpc/include/asm/nohash/32/mmu-8xx.h
+++ b/arch/powerpc/include/asm/nohash/32/mmu-8xx.h
@@ -215,9 +215,8 @@ typedef struct {
unsigned char low_slices_psize[SLICE_ARRAY_SIZE];
unsigned char high_slices_psize[0];
unsigned long slb_addr_limit;
- struct slice_mask mask_base_psize; /* 4k or 16k */
+ struct slice_mask mask_base_psize; /* 4k or 16k or 8M */
struct slice_mask mask_512k;
- struct slice_mask mask_8m;
#endif
void *pte_frag;
} mm_context_t;
@@ -257,10 +256,8 @@ static inline struct slice_mask *slice_mask_for_size(mm_context_t *ctx, int psiz
{
if (psize == MMU_PAGE_512K)
return &ctx->mask_512k;
- if (psize == MMU_PAGE_8M)
- return &ctx->mask_8m;
- BUG_ON(psize != mmu_virtual_psize);
+ BUG_ON(psize != mmu_virtual_psize && psize != MMU_PAGE_8M);
return &ctx->mask_base_psize;
}
diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index edf511c2a30a..0b4ab741bf09 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c
@@ -551,7 +551,8 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
unsigned long vma_mmu_pagesize(struct vm_area_struct *vma)
{
/* With radix we don't use slice, so derive it from vma*/
- if (IS_ENABLED(CONFIG_PPC_MM_SLICES) && !radix_enabled()) {
+ if (IS_ENABLED(CONFIG_PPC_MM_SLICES) && !IS_ENABLED(CONFIG_PPC_8xx) &&
+ !radix_enabled()) {
unsigned int psize = get_slice_psize(vma->vm_mm, vma->vm_start);
return 1UL << mmu_psize_to_shift(psize);
--
2.25.0
^ permalink raw reply related
* Re: [PATCH v6 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Alexey Budankov @ 2020-02-06 18:38 UTC (permalink / raw)
To: Stephen Smalley, James Morris, Serge Hallyn, Peter Zijlstra,
Arnaldo Carvalho de Melo, Ingo Molnar,
joonas.lahtinen@linux.intel.com, Alexei Starovoitov, Will Deacon,
Paul Mackerras, Michael Ellerman
Cc: Andi Kleen, linux-parisc@vger.kernel.org, selinux@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, intel-gfx@lists.freedesktop.org,
Igor Lubashev, linux-kernel, Stephane Eranian,
linux-security-module@vger.kernel.org, oprofile-list,
Thomas Gleixner, Jiri Olsa, linux-arm-kernel
In-Reply-To: <06cdca0e-65f2-b58d-a84e-5a1907aa9eb5@tycho.nsa.gov>
On 06.02.2020 21:30, Stephen Smalley wrote:
> On 2/6/20 1:26 PM, Alexey Budankov wrote:
>>
>> On 06.02.2020 21:23, Stephen Smalley wrote:
>>> On 2/5/20 12:30 PM, Alexey Budankov wrote:
>>>>
>>>> Introduce CAP_PERFMON capability designed to secure system performance
>>>> monitoring and observability operations so that CAP_PERFMON would assist
>>>> CAP_SYS_ADMIN capability in its governing role for performance monitoring
>>>> and observability subsystems.
>>>>
>>>> CAP_PERFMON hardens system security and integrity during performance
>>>> monitoring and observability operations by decreasing attack surface that
>>>> is available to a CAP_SYS_ADMIN privileged process [2]. Providing the access
>>>> to system performance monitoring and observability operations under CAP_PERFMON
>>>> capability singly, without the rest of CAP_SYS_ADMIN credentials, excludes
>>>> chances to misuse the credentials and makes the operation more secure.
>>>> Thus, CAP_PERFMON implements the principal of least privilege for performance
>>>> monitoring and observability operations (POSIX IEEE 1003.1e: 2.2.2.39 principle
>>>> of least privilege: A security design principle that states that a process
>>>> or program be granted only those privileges (e.g., capabilities) necessary
>>>> to accomplish its legitimate function, and only for the time that such
>>>> privileges are actually required)
>>>>
>>>> CAP_PERFMON meets the demand to secure system performance monitoring and
>>>> observability operations for adoption in security sensitive, restricted,
>>>> multiuser production environments (e.g. HPC clusters, cloud and virtual compute
>>>> environments), where root or CAP_SYS_ADMIN credentials are not available to
>>>> mass users of a system, and securely unblocks accessibility of system performance monitoring and observability operations beyond root and CAP_SYS_ADMIN use cases.
>>>>
>>>> CAP_PERFMON takes over CAP_SYS_ADMIN credentials related to system performance
>>>> monitoring and observability operations and balances amount of CAP_SYS_ADMIN
>>>> credentials following the recommendations in the capabilities man page [1]
>>>> for CAP_SYS_ADMIN: "Note: this capability is overloaded; see Notes to kernel
>>>> developers, below." For backward compatibility reasons access to system
>>>> performance monitoring and observability subsystems of the kernel remains
>>>> open for CAP_SYS_ADMIN privileged processes but CAP_SYS_ADMIN capability
>>>> usage for secure system performance monitoring and observability operations
>>>> is discouraged with respect to the designed CAP_PERFMON capability.
>>>>
>>>> Although the software running under CAP_PERFMON can not ensure avoidance
>>>> of related hardware issues, the software can still mitigate these issues
>>>> following the official hardware issues mitigation procedure [2]. The bugs
>>>> in the software itself can be fixed following the standard kernel development
>>>> process [3] to maintain and harden security of system performance monitoring
>>>> and observability operations.
>>>>
>>>> [1] http://man7.org/linux/man-pages/man7/capabilities.7.html
>>>> [2] https://www.kernel.org/doc/html/latest/process/embargoed-hardware-issues.html
>>>> [3] https://www.kernel.org/doc/html/latest/admin-guide/security-bugs.html
>>>>
>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>
>>> This will require a small update to the selinux-testsuite to correctly reflect the new capability requirements, but that's easy enough.
>>
>> Is the suite a part of the kernel sources or something else?
>
> It is external,
> https://github.com/SELinuxProject/selinux-testsuite
>
> I wasn't suggesting that your patch be blocked on updating the testsuite, just noting that it will need to be done.
Ok. Thanks!
~Alexey
^ permalink raw reply
* Re: [PATCH v6 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Stephen Smalley @ 2020-02-06 18:30 UTC (permalink / raw)
To: Alexey Budankov, James Morris, Serge Hallyn, Peter Zijlstra,
Arnaldo Carvalho de Melo, Ingo Molnar,
joonas.lahtinen@linux.intel.com, Alexei Starovoitov, Will Deacon,
Paul Mackerras, Michael Ellerman
Cc: Andi Kleen, linux-parisc@vger.kernel.org, selinux@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, intel-gfx@lists.freedesktop.org,
Igor Lubashev, linux-kernel, Stephane Eranian,
linux-security-module@vger.kernel.org, oprofile-list,
Thomas Gleixner, Jiri Olsa, linux-arm-kernel
In-Reply-To: <1bcb4cb1-98c4-cc1a-b8e3-fd8a0e1e606f@linux.intel.com>
On 2/6/20 1:26 PM, Alexey Budankov wrote:
>
> On 06.02.2020 21:23, Stephen Smalley wrote:
>> On 2/5/20 12:30 PM, Alexey Budankov wrote:
>>>
>>> Introduce CAP_PERFMON capability designed to secure system performance
>>> monitoring and observability operations so that CAP_PERFMON would assist
>>> CAP_SYS_ADMIN capability in its governing role for performance monitoring
>>> and observability subsystems.
>>>
>>> CAP_PERFMON hardens system security and integrity during performance
>>> monitoring and observability operations by decreasing attack surface that
>>> is available to a CAP_SYS_ADMIN privileged process [2]. Providing the access
>>> to system performance monitoring and observability operations under CAP_PERFMON
>>> capability singly, without the rest of CAP_SYS_ADMIN credentials, excludes
>>> chances to misuse the credentials and makes the operation more secure.
>>> Thus, CAP_PERFMON implements the principal of least privilege for performance
>>> monitoring and observability operations (POSIX IEEE 1003.1e: 2.2.2.39 principle
>>> of least privilege: A security design principle that states that a process
>>> or program be granted only those privileges (e.g., capabilities) necessary
>>> to accomplish its legitimate function, and only for the time that such
>>> privileges are actually required)
>>>
>>> CAP_PERFMON meets the demand to secure system performance monitoring and
>>> observability operations for adoption in security sensitive, restricted,
>>> multiuser production environments (e.g. HPC clusters, cloud and virtual compute
>>> environments), where root or CAP_SYS_ADMIN credentials are not available to
>>> mass users of a system, and securely unblocks accessibility of system performance monitoring and observability operations beyond root and CAP_SYS_ADMIN use cases.
>>>
>>> CAP_PERFMON takes over CAP_SYS_ADMIN credentials related to system performance
>>> monitoring and observability operations and balances amount of CAP_SYS_ADMIN
>>> credentials following the recommendations in the capabilities man page [1]
>>> for CAP_SYS_ADMIN: "Note: this capability is overloaded; see Notes to kernel
>>> developers, below." For backward compatibility reasons access to system
>>> performance monitoring and observability subsystems of the kernel remains
>>> open for CAP_SYS_ADMIN privileged processes but CAP_SYS_ADMIN capability
>>> usage for secure system performance monitoring and observability operations
>>> is discouraged with respect to the designed CAP_PERFMON capability.
>>>
>>> Although the software running under CAP_PERFMON can not ensure avoidance
>>> of related hardware issues, the software can still mitigate these issues
>>> following the official hardware issues mitigation procedure [2]. The bugs
>>> in the software itself can be fixed following the standard kernel development
>>> process [3] to maintain and harden security of system performance monitoring
>>> and observability operations.
>>>
>>> [1] http://man7.org/linux/man-pages/man7/capabilities.7.html
>>> [2] https://www.kernel.org/doc/html/latest/process/embargoed-hardware-issues.html
>>> [3] https://www.kernel.org/doc/html/latest/admin-guide/security-bugs.html
>>>
>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>
>> This will require a small update to the selinux-testsuite to correctly reflect the new capability requirements, but that's easy enough.
>
> Is the suite a part of the kernel sources or something else?
It is external,
https://github.com/SELinuxProject/selinux-testsuite
I wasn't suggesting that your patch be blocked on updating the
testsuite, just noting that it will need to be done.
^ permalink raw reply
* Re: [PATCH v6 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Alexey Budankov @ 2020-02-06 18:26 UTC (permalink / raw)
To: Stephen Smalley, James Morris, Serge Hallyn, Peter Zijlstra,
Arnaldo Carvalho de Melo, Ingo Molnar,
joonas.lahtinen@linux.intel.com, Alexei Starovoitov, Will Deacon,
Paul Mackerras, Michael Ellerman
Cc: Andi Kleen, linux-parisc@vger.kernel.org, selinux@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, intel-gfx@lists.freedesktop.org,
Igor Lubashev, linux-kernel, Stephane Eranian,
linux-security-module@vger.kernel.org, oprofile-list,
Thomas Gleixner, Jiri Olsa, linux-arm-kernel
In-Reply-To: <5be0f67c-17e2-7861-37f3-a0f8a82be8f0@tycho.nsa.gov>
On 06.02.2020 21:23, Stephen Smalley wrote:
> On 2/5/20 12:30 PM, Alexey Budankov wrote:
>>
>> Introduce CAP_PERFMON capability designed to secure system performance
>> monitoring and observability operations so that CAP_PERFMON would assist
>> CAP_SYS_ADMIN capability in its governing role for performance monitoring
>> and observability subsystems.
>>
>> CAP_PERFMON hardens system security and integrity during performance
>> monitoring and observability operations by decreasing attack surface that
>> is available to a CAP_SYS_ADMIN privileged process [2]. Providing the access
>> to system performance monitoring and observability operations under CAP_PERFMON
>> capability singly, without the rest of CAP_SYS_ADMIN credentials, excludes
>> chances to misuse the credentials and makes the operation more secure.
>> Thus, CAP_PERFMON implements the principal of least privilege for performance
>> monitoring and observability operations (POSIX IEEE 1003.1e: 2.2.2.39 principle
>> of least privilege: A security design principle that states that a process
>> or program be granted only those privileges (e.g., capabilities) necessary
>> to accomplish its legitimate function, and only for the time that such
>> privileges are actually required)
>>
>> CAP_PERFMON meets the demand to secure system performance monitoring and
>> observability operations for adoption in security sensitive, restricted,
>> multiuser production environments (e.g. HPC clusters, cloud and virtual compute
>> environments), where root or CAP_SYS_ADMIN credentials are not available to
>> mass users of a system, and securely unblocks accessibility of system performance monitoring and observability operations beyond root and CAP_SYS_ADMIN use cases.
>>
>> CAP_PERFMON takes over CAP_SYS_ADMIN credentials related to system performance
>> monitoring and observability operations and balances amount of CAP_SYS_ADMIN
>> credentials following the recommendations in the capabilities man page [1]
>> for CAP_SYS_ADMIN: "Note: this capability is overloaded; see Notes to kernel
>> developers, below." For backward compatibility reasons access to system
>> performance monitoring and observability subsystems of the kernel remains
>> open for CAP_SYS_ADMIN privileged processes but CAP_SYS_ADMIN capability
>> usage for secure system performance monitoring and observability operations
>> is discouraged with respect to the designed CAP_PERFMON capability.
>>
>> Although the software running under CAP_PERFMON can not ensure avoidance
>> of related hardware issues, the software can still mitigate these issues
>> following the official hardware issues mitigation procedure [2]. The bugs
>> in the software itself can be fixed following the standard kernel development
>> process [3] to maintain and harden security of system performance monitoring
>> and observability operations.
>>
>> [1] http://man7.org/linux/man-pages/man7/capabilities.7.html
>> [2] https://www.kernel.org/doc/html/latest/process/embargoed-hardware-issues.html
>> [3] https://www.kernel.org/doc/html/latest/admin-guide/security-bugs.html
>>
>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>
> This will require a small update to the selinux-testsuite to correctly reflect the new capability requirements, but that's easy enough.
Is the suite a part of the kernel sources or something else?
~Alexey
>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
>
>> ---
>> include/linux/capability.h | 4 ++++
>> include/uapi/linux/capability.h | 8 +++++++-
>> security/selinux/include/classmap.h | 4 ++--
>> 3 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/capability.h b/include/linux/capability.h
>> index ecce0f43c73a..027d7e4a853b 100644
>> --- a/include/linux/capability.h
>> +++ b/include/linux/capability.h
>> @@ -251,6 +251,10 @@ extern bool privileged_wrt_inode_uidgid(struct user_namespace *ns, const struct
>> extern bool capable_wrt_inode_uidgid(const struct inode *inode, int cap);
>> extern bool file_ns_capable(const struct file *file, struct user_namespace *ns, int cap);
>> extern bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns);
>> +static inline bool perfmon_capable(void)
>> +{
>> + return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
>> +}
>> /* audit system wants to get cap info from files as well */
>> extern int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data *cpu_caps);
>> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
>> index 240fdb9a60f6..8b416e5f3afa 100644
>> --- a/include/uapi/linux/capability.h
>> +++ b/include/uapi/linux/capability.h
>> @@ -366,8 +366,14 @@ struct vfs_ns_cap_data {
>> #define CAP_AUDIT_READ 37
>> +/*
>> + * Allow system performance and observability privileged operations
>> + * using perf_events, i915_perf and other kernel subsystems
>> + */
>> +
>> +#define CAP_PERFMON 38
>> -#define CAP_LAST_CAP CAP_AUDIT_READ
>> +#define CAP_LAST_CAP CAP_PERFMON
>> #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
>> index 7db24855e12d..c599b0c2b0e7 100644
>> --- a/security/selinux/include/classmap.h
>> +++ b/security/selinux/include/classmap.h
>> @@ -27,9 +27,9 @@
>> "audit_control", "setfcap"
>> #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
>> - "wake_alarm", "block_suspend", "audit_read"
>> + "wake_alarm", "block_suspend", "audit_read", "perfmon"
>> -#if CAP_LAST_CAP > CAP_AUDIT_READ
>> +#if CAP_LAST_CAP > CAP_PERFMON
>> #error New capability defined, please update COMMON_CAP2_PERMS.
>> #endif
>>
>
^ permalink raw reply
* Re: [PATCH v6 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Stephen Smalley @ 2020-02-06 18:23 UTC (permalink / raw)
To: Alexey Budankov, James Morris, Serge Hallyn, Peter Zijlstra,
Arnaldo Carvalho de Melo, Ingo Molnar,
joonas.lahtinen@linux.intel.com, Alexei Starovoitov, Will Deacon,
Paul Mackerras, Michael Ellerman
Cc: Andi Kleen, linux-parisc@vger.kernel.org, selinux@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, intel-gfx@lists.freedesktop.org,
Igor Lubashev, linux-kernel, Stephane Eranian,
linux-security-module@vger.kernel.org, oprofile-list,
Thomas Gleixner, Jiri Olsa, linux-arm-kernel
In-Reply-To: <a4c5da70-b6d1-b133-9b64-34e164834b03@linux.intel.com>
On 2/5/20 12:30 PM, Alexey Budankov wrote:
>
> Introduce CAP_PERFMON capability designed to secure system performance
> monitoring and observability operations so that CAP_PERFMON would assist
> CAP_SYS_ADMIN capability in its governing role for performance monitoring
> and observability subsystems.
>
> CAP_PERFMON hardens system security and integrity during performance
> monitoring and observability operations by decreasing attack surface that
> is available to a CAP_SYS_ADMIN privileged process [2]. Providing the access
> to system performance monitoring and observability operations under CAP_PERFMON
> capability singly, without the rest of CAP_SYS_ADMIN credentials, excludes
> chances to misuse the credentials and makes the operation more secure.
> Thus, CAP_PERFMON implements the principal of least privilege for performance
> monitoring and observability operations (POSIX IEEE 1003.1e: 2.2.2.39 principle
> of least privilege: A security design principle that states that a process
> or program be granted only those privileges (e.g., capabilities) necessary
> to accomplish its legitimate function, and only for the time that such
> privileges are actually required)
>
> CAP_PERFMON meets the demand to secure system performance monitoring and
> observability operations for adoption in security sensitive, restricted,
> multiuser production environments (e.g. HPC clusters, cloud and virtual compute
> environments), where root or CAP_SYS_ADMIN credentials are not available to
> mass users of a system, and securely unblocks accessibility of system performance monitoring and observability operations beyond root and CAP_SYS_ADMIN use cases.
>
> CAP_PERFMON takes over CAP_SYS_ADMIN credentials related to system performance
> monitoring and observability operations and balances amount of CAP_SYS_ADMIN
> credentials following the recommendations in the capabilities man page [1]
> for CAP_SYS_ADMIN: "Note: this capability is overloaded; see Notes to kernel
> developers, below." For backward compatibility reasons access to system
> performance monitoring and observability subsystems of the kernel remains
> open for CAP_SYS_ADMIN privileged processes but CAP_SYS_ADMIN capability
> usage for secure system performance monitoring and observability operations
> is discouraged with respect to the designed CAP_PERFMON capability.
>
> Although the software running under CAP_PERFMON can not ensure avoidance
> of related hardware issues, the software can still mitigate these issues
> following the official hardware issues mitigation procedure [2]. The bugs
> in the software itself can be fixed following the standard kernel development
> process [3] to maintain and harden security of system performance monitoring
> and observability operations.
>
> [1] http://man7.org/linux/man-pages/man7/capabilities.7.html
> [2] https://www.kernel.org/doc/html/latest/process/embargoed-hardware-issues.html
> [3] https://www.kernel.org/doc/html/latest/admin-guide/security-bugs.html
>
> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
This will require a small update to the selinux-testsuite to correctly
reflect the new capability requirements, but that's easy enough.
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> include/linux/capability.h | 4 ++++
> include/uapi/linux/capability.h | 8 +++++++-
> security/selinux/include/classmap.h | 4 ++--
> 3 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index ecce0f43c73a..027d7e4a853b 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -251,6 +251,10 @@ extern bool privileged_wrt_inode_uidgid(struct user_namespace *ns, const struct
> extern bool capable_wrt_inode_uidgid(const struct inode *inode, int cap);
> extern bool file_ns_capable(const struct file *file, struct user_namespace *ns, int cap);
> extern bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns);
> +static inline bool perfmon_capable(void)
> +{
> + return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
> +}
>
> /* audit system wants to get cap info from files as well */
> extern int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data *cpu_caps);
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index 240fdb9a60f6..8b416e5f3afa 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -366,8 +366,14 @@ struct vfs_ns_cap_data {
>
> #define CAP_AUDIT_READ 37
>
> +/*
> + * Allow system performance and observability privileged operations
> + * using perf_events, i915_perf and other kernel subsystems
> + */
> +
> +#define CAP_PERFMON 38
>
> -#define CAP_LAST_CAP CAP_AUDIT_READ
> +#define CAP_LAST_CAP CAP_PERFMON
>
> #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 7db24855e12d..c599b0c2b0e7 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -27,9 +27,9 @@
> "audit_control", "setfcap"
>
> #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
> - "wake_alarm", "block_suspend", "audit_read"
> + "wake_alarm", "block_suspend", "audit_read", "perfmon"
>
> -#if CAP_LAST_CAP > CAP_AUDIT_READ
> +#if CAP_LAST_CAP > CAP_PERFMON
> #error New capability defined, please update COMMON_CAP2_PERMS.
> #endif
>
>
^ permalink raw reply
* Re: [PATCH v5 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Alexey Budankov @ 2020-02-06 18:03 UTC (permalink / raw)
To: Stephen Smalley, Serge Hallyn, James Morris
Cc: Mark Rutland, Song Liu, Peter Zijlstra,
joonas.lahtinen@linux.intel.com, Will Deacon, Alexei Starovoitov,
Lionel Landwerlin, Paul Mackerras, Jiri Olsa, Alexei Starovoitov,
Andi Kleen, Igor Lubashev, Alexander Shishkin, Ingo Molnar,
oprofile-list, linux-arm-kernel, Robert Richter,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
jani.nikula@linux.intel.com, Arnaldo Carvalho de Melo,
rodrigo.vivi@intel.com, Namhyung Kim, Thomas Gleixner,
Stephane Eranian, linux-parisc@vger.kernel.org, linux-kernel,
Andy Lutomirski, linux-perf-users@vger.kernel.org,
linux-security-module@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <ac0dbab7-de47-ee34-bb88-4c43d3538b7d@linux.intel.com>
On 22.01.2020 17:25, Alexey Budankov wrote:
>
> On 22.01.2020 17:07, Stephen Smalley wrote:
>> On 1/22/20 5:45 AM, Alexey Budankov wrote:
>>>
>>> On 21.01.2020 21:27, Alexey Budankov wrote:
>>>>
>>>> On 21.01.2020 20:55, Alexei Starovoitov wrote:
>>>>> On Tue, Jan 21, 2020 at 9:31 AM Alexey Budankov
>>>>> <alexey.budankov@linux.intel.com> wrote:
>>>>>>
>>>>>>
>>>>>> On 21.01.2020 17:43, Stephen Smalley wrote:
>>>>>>> On 1/20/20 6:23 AM, Alexey Budankov wrote:
>>>>>>>>
>>>>>>>> Introduce CAP_PERFMON capability designed to secure system performance
>>>>>>>> monitoring and observability operations so that CAP_PERFMON would assist
>>>>>>>> CAP_SYS_ADMIN capability in its governing role for perf_events, i915_perf
>>>>>>>> and other performance monitoring and observability subsystems.
>>>>>>>>
>>>>>>>> CAP_PERFMON intends to harden system security and integrity during system
>>>>>>>> performance monitoring and observability operations by decreasing attack
>>>>>>>> surface that is available to a CAP_SYS_ADMIN privileged process [1].
>>>>>>>> Providing access to system performance monitoring and observability
>>>>>>>> operations under CAP_PERFMON capability singly, without the rest of
>>>>>>>> CAP_SYS_ADMIN credentials, excludes chances to misuse the credentials and
>>>>>>>> makes operation more secure.
>>>>>>>>
>>>>>>>> CAP_PERFMON intends to take over CAP_SYS_ADMIN credentials related to
>>>>>>>> system performance monitoring and observability operations and balance
>>>>>>>> amount of CAP_SYS_ADMIN credentials following the recommendations in the
>>>>>>>> capabilities man page [1] for CAP_SYS_ADMIN: "Note: this capability is
>>>>>>>> overloaded; see Notes to kernel developers, below."
>>>>>>>>
>>>>>>>> Although the software running under CAP_PERFMON can not ensure avoidance
>>>>>>>> of related hardware issues, the software can still mitigate these issues
>>>>>>>> following the official embargoed hardware issues mitigation procedure [2].
>>>>>>>> The bugs in the software itself could be fixed following the standard
>>>>>>>> kernel development process [3] to maintain and harden security of system
>>>>>>>> performance monitoring and observability operations.
>>>>>>>>
>>>>>>>> [1] http://man7.org/linux/man-pages/man7/capabilities.7.html
>>>>>>>> [2] https://www.kernel.org/doc/html/latest/process/embargoed-hardware-issues.html
>>>>>>>> [3] https://www.kernel.org/doc/html/latest/admin-guide/security-bugs.html
<SNIP>
>>>>>>>>
>>>>>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>>>>>
>>>>>>> Why _noaudit()? Normally only used when a permission failure is non-fatal to the operation. Otherwise, we want the audit message.
>>>
>>> So far so good, I suggest using the simplest version for v6:
>>>
>>> static inline bool perfmon_capable(void)
>>> {
>>> return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
>>> }
>>>
>>> It keeps the implementation simple and readable. The implementation is more
>>> performant in the sense of calling the API - one capable() call for CAP_PERFMON
>>> privileged process.
>>>
>>> Yes, it bloats audit log for CAP_SYS_ADMIN privileged and unprivileged processes,
>>> but this bloating also advertises and leverages using more secure CAP_PERFMON
>>> based approach to use perf_event_open system call.
>>
>> I can live with that. We just need to document that when you see both a CAP_PERFMON and a CAP_SYS_ADMIN audit message for a process, try only allowing CAP_PERFMON first and see if that resolves the issue. We have a similar issue with CAP_DAC_READ_SEARCH versus CAP_DAC_OVERRIDE.
>
> perf security [1] document can be updated, at least, to align and document
> this audit logging specifics.
And I plan to update the document right after this patch set is accepted.
Feel free to let me know of the places in the kernel docs that also
require update w.r.t CAP_PERFMON extension.
~Alexey
>
> ~Alexey
>
> [1] https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html
>
^ 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