* Re: [PATCH 07/15] powerpc/powernv/sriov: Rename truncate_iov
From: Alexey Kardashevskiy @ 2020-07-15 0:46 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev
In-Reply-To: <20200710052340.737567-8-oohall@gmail.com>
On 10/07/2020 15:23, Oliver O'Halloran wrote:
> This prevents SR-IOV being used by making the SR-IOV BAR resources
> unallocatable. Rename it to reflect what it actually does.
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/platforms/powernv/pci-sriov.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c
> index f4c74ab1284d..216ceeff69b0 100644
> --- a/arch/powerpc/platforms/powernv/pci-sriov.c
> +++ b/arch/powerpc/platforms/powernv/pci-sriov.c
> @@ -155,7 +155,7 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
>
> iov = kzalloc(sizeof(*iov), GFP_KERNEL);
> if (!iov)
> - goto truncate_iov;
> + goto disable_iov;
> pdev->dev.archdata.iov_data = iov;
>
> total_vfs = pci_sriov_get_totalvfs(pdev);
> @@ -170,7 +170,7 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
> dev_warn(&pdev->dev, "Don't support SR-IOV with"
> " non M64 VF BAR%d: %pR. \n",
> i, res);
> - goto truncate_iov;
> + goto disable_iov;
> }
>
> total_vf_bar_sz += pci_iov_resource_size(pdev,
> @@ -209,7 +209,8 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
> * mode is 32MB.
> */
> if (iov->m64_single_mode && (size < SZ_32M))
> - goto truncate_iov;
> + goto disable_iov;
> +
> dev_dbg(&pdev->dev, " Fixing VF BAR%d: %pR to\n", i, res);
> res->end = res->start + size * mul - 1;
> dev_dbg(&pdev->dev, " %pR\n", res);
> @@ -220,8 +221,8 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
>
> return;
>
> -truncate_iov:
> - /* To save MMIO space, IOV BAR is truncated. */
> +disable_iov:
> + /* Save ourselves some MMIO space by disabling the unusable BARs */
> for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> res = &pdev->resource[i + PCI_IOV_RESOURCES];
> res->flags = 0;
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 06/15] powerpc/powernv/sriov: Explain how SR-IOV works on PowerNV
From: Alexey Kardashevskiy @ 2020-07-15 0:40 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev
In-Reply-To: <20200710052340.737567-7-oohall@gmail.com>
On 10/07/2020 15:23, Oliver O'Halloran wrote:
> SR-IOV support on PowerNV is a byzantine maze of hooks. I have no idea
> how anyone is supposed to know how it works except through a lot of
> stuffering. Write up some docs about the overall story to help out
> the next sucker^Wperson who needs to tinker with it.
Sounds about right :)
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> arch/powerpc/platforms/powernv/pci-sriov.c | 130 +++++++++++++++++++++
> 1 file changed, 130 insertions(+)
>
> diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c
> index 080ea39f5a83..f4c74ab1284d 100644
> --- a/arch/powerpc/platforms/powernv/pci-sriov.c
> +++ b/arch/powerpc/platforms/powernv/pci-sriov.c
> @@ -12,6 +12,136 @@
> /* for pci_dev_is_added() */
> #include "../../../../drivers/pci/pci.h"
>
> +/*
> + * The majority of the complexity in supporting SR-IOV on PowerNV comes from
> + * the need to put the MMIO space for each VF into a separate PE. Internally
> + * the PHB maps MMIO addresses to a specific PE using the "Memory BAR Table".
> + * The MBT historically only applied to the 64bit MMIO window of the PHB
> + * so it's common to see it referred to as the "M64BT".
> + *
> + * An MBT entry stores the mapped range as an <base>,<mask> pair. This forces
> + * the address range that we want to map to be power-of-two sized and aligned.
> + * For conventional PCI devices this isn't really an issue since PCI device BARs
> + * have the same requirement.
> + *
> + * For a SR-IOV BAR things are a little more awkward since size and alignment
> + * are not coupled. The alignment is set based on the the per-VF BAR size, but
> + * the total BAR area is: number-of-vfs * per-vf-size. The number of VFs
> + * isn't necessarily a power of two, so neither is the total size. To fix that
> + * we need to finesse (read: hack) the Linux BAR allocator so that it will
> + * allocate the SR-IOV BARs in a way that lets us map them using the MBT.
> + *
> + * The changes to size and alignment that we need to do depend on the "mode"
> + * of MBT entry that we use. We only support SR-IOV on PHB3 (IODA2) and above,
> + * so as a baseline we can assume that we have the following BAR modes
> + * available:
> + *
> + * NB: $PE_COUNT is the number of PEs that the PHB supports.
> + *
> + * a) A segmented BAR that splits the mapped range into $PE_COUNT equally sized
> + * segments. The n'th segment is mapped to the n'th PE.
> + * b) An un-segmented BAR that maps the whole address range to a specific PE.
> + *
> + *
> + * We prefer to use mode a) since it only requires one MBT entry per SR-IOV BAR
> + * For comparison b) requires one entry per-VF per-BAR, or:
> + * (num-vfs * num-sriov-bars) in total. To use a) we need the size of each segment
> + * to equal the size of the per-VF BAR area. So:
> + *
> + * new_size = per-vf-size * number-of-PEs
> + *
> + * The alignment for the SR-IOV BAR also needs to be changed from per-vf-size
> + * to "new_size", calculated above. Implementing this is a convoluted process
> + * which requires several hooks in the PCI core:
> + *
> + * 1. In pcibios_add_device() we call pnv_pci_ioda_fixup_iov().
> + *
> + * At this point the device has been probed and the device's BARs are sized,
> + * but no resource allocations have been done. The SR-IOV BARs are sized
> + * based on the maximum number of VFs supported by the device and we need
> + * to increase that to new_size.
> + *
> + * 2. Later, when Linux actually assigns resources it tries to make the resource
> + * allocations for each PCI bus as compact as possible. As a part of that it
> + * sorts the BARs on a bus by their required alignment, which is calculated
> + * using pci_resource_alignment().
> + *
> + * For IOV resources this goes:
> + * pci_resource_alignment()
> + * pci_sriov_resource_alignment()
> + * pcibios_sriov_resource_alignment()
> + * pnv_pci_iov_resource_alignment()
> + *
> + * Our hook overrides the default alignment, equal to the per-vf-size, with
> + * new_size computed above.
> + *
> + * 3. When userspace enables VFs for a device:
> + *
> + * sriov_enable()
> + * pcibios_sriov_enable()
> + * pnv_pcibios_sriov_enable()
> + *
> + * This is where we actually allocate PE numbers for each VF and setup the
> + * MBT mapping for each SR-IOV BAR. In steps 1) and 2) we setup an "arena"
> + * where each MBT segment is equal in size to the VF BAR so we can shift
> + * around the actual SR-IOV BAR location within this arena. We need this
> + * ability because the PE space is shared by all devices on the same PHB.
> + * When using mode a) described above segment 0 in maps to PE#0 which might
> + * be already being used by another device on the PHB.
> + *
> + * As a result we need allocate a contigious range of PE numbers, then shift
> + * the address programmed into the SR-IOV BAR of the PF so that the address
> + * of VF0 matches up with the segment corresponding to the first allocated
> + * PE number. This is handled in pnv_pci_vf_resource_shift().
> + *
> + * Once all that is done we return to the PCI core which then enables VFs,
> + * scans them and creates pci_devs for each. The init process for a VF is
> + * largely the same as a normal device, but the VF is inserted into the IODA
> + * PE that we allocated for it rather than the PE associated with the bus.
> + *
> + * 4. When userspace disables VFs we unwind the above in
> + * pnv_pcibios_sriov_disable(). Fortunately this is relatively simple since
> + * we don't need to validate anything, just tear down the mappings and
> + * move SR-IOV resource back to its "proper" location.
> + *
> + * That's how mode a) works. In theory mode b) (single PE mapping) is less work
> + * since we can map each individual VF with a separate BAR. However, there's a
> + * few limitations:
> + *
> + * 1) For IODA2 mode b) has a minimum alignment requirement of 32MB. This makes
> + * it only usable for devices with very large per-VF BARs. Such devices are
> + * similar to Big Foot. They definitely exist, but I've never seen one.
> + *
> + * 2) The number of MBT entries that we have is limited. PHB3 and PHB4 only
> + * 16 total and some are needed for. Most SR-IOV capable network cards can support
> + * more than 16 VFs on each port.
> + *
> + * We use b) when using a) would use more than 1/4 of the entire 64 bit MMIO
> + * window of the PHB.
> + *
> + *
> + *
> + * PHB4 (IODA3) added a few new features that would be useful for SR-IOV. It
> + * allowed the MBT to map 32bit MMIO space in addition to 64bit which allows
> + * us to support SR-IOV BARs in the 32bit MMIO window. This is useful since
> + * the Linux BAR allocation will place any BAR marked as non-prefetchable into
> + * the non-prefetchable bridge window, which is 32bit only. It also added two
> + * new modes:
> + *
> + * c) A segmented BAR similar to a), but each segment can be individually
> + * mapped to any PE. This is matches how the 32bit MMIO window worked on
> + * IODA1&2.
> + *
> + * d) A segmented BAR with 8, 64, or 128 segments. This works similarly to a),
> + * but with fewer segments and configurable base PE.
> + *
> + * i.e. The n'th segment maps to the (n + base)'th PE.
> + *
> + * The base PE is also required to be a multiple of the window size.
> + *
> + * Unfortunately, the OPAL API doesn't currently (as of skiboot v6.6) allow us
> + * to exploit any of the IODA3 features.
> + */
>
> static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
> {
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 03/15] powerpc/powernv/pci: Add explicit tracking of the DMA setup state
From: Alexey Kardashevskiy @ 2020-07-15 0:23 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linuxppc-dev
In-Reply-To: <34f7eea2-4ace-9931-7b5f-98ec159f3532@ozlabs.ru>
On 14/07/2020 17:21, Alexey Kardashevskiy wrote:
>
>
> On 14/07/2020 15:58, Oliver O'Halloran wrote:
>> On Tue, Jul 14, 2020 at 3:37 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>
>>> On 10/07/2020 15:23, Oliver O'Halloran wrote:
>>>> There's an optimisation in the PE setup which skips performing DMA
>>>> setup for a PE if we only have bridges in a PE. The assumption being
>>>> that only "real" devices will DMA to system memory, which is probably
>>>> fair. However, if we start off with only bridge devices in a PE then
>>>> add a non-bridge device the new device won't be able to use DMA because
>>>> we never configured it.
>>>>
>>>> Fix this (admittedly pretty weird) edge case by tracking whether we've done
>>>> the DMA setup for the PE or not. If a non-bridge device is added to the PE
>>>> (via rescan or hotplug, or whatever) we can set up DMA on demand.
>>>
>>> So hotplug does not work on powernv then, right? I thought you tested it
>>> a while ago, or this patch is the result of that attempt? If it is, then
>>
>> It mostly works. Just the really niche case of hot plugging a bridge,
>> then later on hot plugging a device into the same bus which wouldn't
>> work.
>
> Do not you have to have a slot (which is a bridge) for hotplug in the
> first place, to hotplug the bridge?
As discussed elsewhere, I missed that it is a non bridge device on the
same bus with previously plugged bridge. Now it all makes sense and
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
--
Alexey
^ permalink raw reply
* [PATCH] pseries: Fix 64 bit logical memory block panic
From: Anton Blanchard @ 2020-07-15 0:08 UTC (permalink / raw)
To: benh, paulus, mpe, nathanl; +Cc: linuxppc-dev
Booting with a 4GB LMB size causes us to panic:
qemu-system-ppc64: OS terminated: OS panic:
Memory block size not suitable: 0x0
Fix pseries_memory_block_size() to handle 64 bit LMBs.
Cc: stable@vger.kernel.org
Signed-off-by: Anton Blanchard <anton@ozlabs.org>
---
arch/powerpc/platforms/pseries/hotplug-memory.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 5ace2f9a277e..6574ac33e887 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -27,7 +27,7 @@ static bool rtas_hp_event;
unsigned long pseries_memory_block_size(void)
{
struct device_node *np;
- unsigned int memblock_size = MIN_MEMORY_BLOCK_SIZE;
+ uint64_t memblock_size = MIN_MEMORY_BLOCK_SIZE;
struct resource r;
np = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
--
2.26.2
^ permalink raw reply related
* Re: [PATCH v3 03/12] powerpc/kexec_file: add helper functions for getting memory ranges
From: Thiago Jung Bauermann @ 2020-07-14 23:49 UTC (permalink / raw)
To: Hari Bathini
Cc: Pingfan Liu, Nayna Jain, Kexec-ml, Mahesh J Salgaonkar,
Mimi Zohar, lkml, linuxppc-dev, Sourabh Jain, Petr Tesarik,
Andrew Morton, Dave Young, Vivek Goyal, Eric Biederman
In-Reply-To: <159466087136.24747.16494497863685481495.stgit@hbathini.in.ibm.com>
Hello Hari,
Hari Bathini <hbathini@linux.ibm.com> writes:
> In kexec case, the kernel to be loaded uses the same memory layout as
> the running kernel. So, passing on the DT of the running kernel would
> be good enough.
>
> But in case of kdump, different memory ranges are needed to manage
> loading the kdump kernel, booting into it and exporting the elfcore
> of the crashing kernel. The ranges are exlude memory ranges, usable
s/exlude/exclude/
> memory ranges, reserved memory ranges and crash memory ranges.
>
> Exclude memory ranges specify the list of memory ranges to avoid while
> loading kdump segments. Usable memory ranges list the memory ranges
> that could be used for booting kdump kernel. Reserved memory ranges
> list the memory regions for the loading kernel's reserve map. Crash
> memory ranges list the memory ranges to be exported as the crashing
> kernel's elfcore.
>
> Add helper functions for setting up the above mentioned memory ranges.
> This helpers facilitate in understanding the subsequent changes better
> and make it easy to setup the different memory ranges listed above, as
> and when appropriate.
>
> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
> Tested-by: Pingfan Liu <piliu@redhat.com>
<snip>
> +/**
> + * get_mem_rngs_size - Get the allocated size of mrngs based on
> + * max_nr_ranges and chunk size.
> + * @mrngs: Memory ranges.
> + *
> + * Returns the maximum no. of ranges.
This isn't correct. It returns the maximum size of @mrngs.
> + */
> +static inline size_t get_mem_rngs_size(struct crash_mem *mrngs)
> +{
> + size_t size;
> +
> + if (!mrngs)
> + return 0;
> +
> + size = (sizeof(struct crash_mem) +
> + (mrngs->max_nr_ranges * sizeof(struct crash_mem_range)));
> +
> + /*
> + * Memory is allocated in size multiple of MEM_RANGE_CHUNK_SZ.
> + * So, align to get the actual length.
> + */
> + return ALIGN(size, MEM_RANGE_CHUNK_SZ);
> +}
<snip>
> +/**
> + * add_tce_mem_ranges - Adds tce-table range to the given memory ranges list.
> + * @mem_ranges: Range list to add the memory range(s) to.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +int add_tce_mem_ranges(struct crash_mem **mem_ranges)
> +{
> + struct device_node *dn;
> + int ret;
> +
> + for_each_node_by_type(dn, "pci") {
> + u64 base;
> + u32 size;
> +
> + ret = of_property_read_u64(dn, "linux,tce-base", &base);
> + ret |= of_property_read_u32(dn, "linux,tce-size", &size);
> + if (!ret)
Shouldn't the condition be `ret` instead of `!ret`?
> + continue;
> +
> + ret = add_mem_range(mem_ranges, base, size);
> + if (ret)
> + break;
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * add_initrd_mem_range - Adds initrd range to the given memory ranges list,
> + * if the initrd was retained.
> + * @mem_ranges: Range list to add the memory range to.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +int add_initrd_mem_range(struct crash_mem **mem_ranges)
> +{
> + u64 base, end;
> + int ret = 0;
> + char *str;
> +
> + /* This range means something only if initrd was retained */
> + str = strstr(saved_command_line, "retain_initrd");
> + if (!str)
> + return 0;
> +
> + ret = of_property_read_u64(of_chosen, "linux,initrd-start", &base);
> + ret |= of_property_read_u64(of_chosen, "linux,initrd-end", &end);
> + if (!ret)
> + ret = add_mem_range(mem_ranges, base, end - base + 1);
> + return ret;
> +}
> +
> +/**
> + * add_htab_mem_range - Adds htab range to the given memory ranges list,
> + * if it exists
> + * @mem_ranges: Range list to add the memory range to.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +int add_htab_mem_range(struct crash_mem **mem_ranges)
> +{
> +#ifdef CONFIG_PPC_BOOK3S_64
> + int ret;
> +
> + if (!htab_address)
> + return 0;
> +
> + ret = add_mem_range(mem_ranges, __pa(htab_address), htab_size_bytes);
> + return ret;
> +#else
> + return 0;
> +#endif
> +}
If I'm not mistaken, this is not the preferred way of having alternative
implementations of a function. The "Conditional Compilation" section of
the coding style document doesn't mention this directly, but does say
that it's better to put the conditionals in a header file.
In this case, I would do this in <asm/kexec_ranges.h>
#ifdef CONFIG_PPC_BOOK3S_64
int add_htab_mem_range(struct crash_mem **mem_ranges);
#else
static inline int add_htab_mem_range(struct crash_mem **mem_ranges)
{
return 0;
}
#endif
And in ranges.c just surround the add_htab_mem_range() definition with
#ifdef CONFIG_PPC_BOOK3S_64 and #endif
Also, there's no need for the ret variable. You can just
`return add_mem_range(...)` directly.
> +
> +/**
> + * add_kernel_mem_range - Adds kernel text region to the given
> + * memory ranges list.
> + * @mem_ranges: Range list to add the memory range to.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +int add_kernel_mem_range(struct crash_mem **mem_ranges)
> +{
> + int ret;
> +
> + ret = add_mem_range(mem_ranges, 0, __pa(_end));
> + return ret;
> +}
No need for the ret variable here, just `return add_mem_range()`
directly.
> +
> +/**
> + * add_rtas_mem_range - Adds RTAS region to the given memory ranges list.
> + * @mem_ranges: Range list to add the memory range to.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +int add_rtas_mem_range(struct crash_mem **mem_ranges)
> +{
> + struct device_node *dn;
> + int ret = 0;
> +
> + dn = of_find_node_by_path("/rtas");
> + if (dn) {
> + u32 base, size;
> +
> + ret = of_property_read_u32(dn, "linux,rtas-base", &base);
> + ret |= of_property_read_u32(dn, "rtas-size", &size);
> + if (ret)
> + return ret;
> +
> + ret = add_mem_range(mem_ranges, base, size);
You're missing an of_node_put(dn) here (also in the early return in the
line above).
> + }
> + return ret;
> +}
> +
> +/**
> + * add_opal_mem_range - Adds OPAL region to the given memory ranges list.
> + * @mem_ranges: Range list to add the memory range to.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +int add_opal_mem_range(struct crash_mem **mem_ranges)
> +{
> + struct device_node *dn;
> + int ret = 0;
> +
> + dn = of_find_node_by_path("/ibm,opal");
> + if (dn) {
> + u64 base, size;
> +
> + ret = of_property_read_u64(dn, "opal-base-address", &base);
> + ret |= of_property_read_u64(dn, "opal-runtime-size", &size);
> + if (ret)
> + return ret;
> +
> + ret = add_mem_range(mem_ranges, base, size);
You're missing an of_node_put(dn) here (also in the early return in the
line above).
> + }
> + return ret;
> +}
> +
> +/**
> + * add_reserved_ranges - Adds "/reserved-ranges" regions exported by f/w
> + * to the given memory ranges list.
> + * @mem_ranges: Range list to add the memory ranges to.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +int add_reserved_ranges(struct crash_mem **mem_ranges)
> +{
> + int i, len, ret = 0;
> + const __be32 *prop;
> +
> + prop = of_get_property(of_root, "reserved-ranges", &len);
> + if (!prop)
> + return 0;
> +
> + /*
> + * Each reserved range is an (address,size) pair, 2 cells each,
> + * totalling 4 cells per range.
Can you assume that, or do you need to check the #address-cells and
#size-cells properties of the root node?
> + */
> + for (i = 0; i < len / (sizeof(*prop) * 4); i++) {
> + u64 base, size;
> +
> + base = of_read_number(prop + (i * 4) + 0, 2);
> + size = of_read_number(prop + (i * 4) + 2, 2);
> +
> + ret = add_mem_range(mem_ranges, base, size);
> + if (ret)
> + break;
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * sort_memory_ranges - Sorts the given memory ranges list.
> + * @mem_ranges: Range list to sort.
> + * @merge: If true, merge the list after sorting.
> + *
> + * Returns nothing.
> + */
> +void sort_memory_ranges(struct crash_mem *mrngs, bool merge)
> +{
> + struct crash_mem_range *rngs;
> + struct crash_mem_range rng;
> + int i, j, idx;
> +
> + if (!mrngs)
> + return;
> +
> + /* Sort the ranges in-place */
> + rngs = &mrngs->ranges[0];
> + for (i = 0; i < mrngs->nr_ranges; i++) {
> + idx = i;
> + for (j = (i + 1); j < mrngs->nr_ranges; j++) {
> + if (rngs[idx].start > rngs[j].start)
> + idx = j;
> + }
> + if (idx != i) {
> + rng = rngs[idx];
> + rngs[idx] = rngs[i];
> + rngs[i] = rng;
> + }
> + }
Would it work using sort() from lib/sort.c here?
> +
> + if (merge)
> + __merge_memory_ranges(mrngs);
> +}
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [RFC PATCH 00/35] Move all PCIBIOS* definitions into arch/x86
From: Bjorn Helgaas @ 2020-07-14 23:46 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Keith Busch, Paul Mackerras, sparclinux, Toan Le, Kjetil Oftedal,
Greg Ungerer, Marek Vasut, Rob Herring, Lorenzo Pieralisi,
Sagi Grimberg, Russell King, Ley Foon Tan, Christoph Hellwig,
Geert Uytterhoeven, Kevin Hilman, linux-pci, Jakub Kicinski,
Matt Turner, linux-kernel-mentees, Guenter Roeck, Ray Jui,
Jens Axboe, Ivan Kokshaysky, Shuah Khan, bjorn, Boris Ostrovsky,
Richard Henderson, Juergen Gross, Bjorn Helgaas,
Thomas Bogendoerfer, Scott Branden, Jingoo Han,
Saheed O. Bolarinwa, linux-kernel@vger.kernel.org, Philipp Zabel,
Greg Kroah-Hartman, Gustavo Pimentel, linuxppc-dev,
David S. Miller, Heiner Kallweit
In-Reply-To: <CAK8P3a3EZX8=649R9cYF6_=ivh1Xyrgsc5mUtS=d5yvQ3doZaQ@mail.gmail.com>
[+cc Kjetil]
On Wed, Jul 15, 2020 at 12:01:56AM +0200, Arnd Bergmann wrote:
> On Tue, Jul 14, 2020 at 8:45 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Mon, Jul 13, 2020 at 05:08:10PM +0200, Arnd Bergmann wrote:
> > > On Mon, Jul 13, 2020 at 3:22 PM Saheed O. Bolarinwa
> > > Starting with a), my first question is whether any high-level
> > > drivers even need to care about errors from these functions. I see
> > > 4913 callers that ignore the return code, and 576 that actually
> > > check it, and almost none care about the specific error (as you
> > > found as well). Unless we conclude that most PCI drivers are wrong,
> > > could we just change the return type to 'void' and assume they never
> > > fail for valid arguments on a valid pci_device* ?
> >
> > I really like this idea.
> >
> > pci_write_config_*() has one return value, and only 100ish of 2500
> > callers check for errors. It's sometimes possible for config
> > accessors to detect PCI errors and return failure, e.g., device was
> > removed or didn't respond, but most of them don't, and detecting these
> > errors is not really that valuable.
> >
> > pci_read_config_*() is much more interesting because it returns two
> > things, the function return value and the value read from the PCI
> > device, and it's complicated to check both.
> >
> > Again it's sometimes possible for config read accessors to detect PCI
> > errors, but in most cases a PCI error means the accessor returns
> > success and the value from PCI is ~0.
> >
> > Checking the function return value catches programming errors (bad
> > alignment, etc) but misses most of the interesting errors (device was
> > unplugged or reported a PCI error).
>
> My thinking was more that most of the time the error checking may
> be completely bogus to start with, and I would just not check for
> errors at all.
Yes. I have no problem with that. There are a few cases where it's
important to check for errors, e.g., we read a status register and do
something based on a bit being set. A failure will return all bits
set, and we may do the wrong thing. But most of the errors we care
about will be on MMIO reads, not config reads, so we can probably
ignore most config read errors.
> > Checking the value returned from PCI is tricky because ~0 is a valid
> > value for some config registers, and only the driver knows for sure.
> > If the driver knows that ~0 is a possible value, it would have to do
> > something else, e.g., another config read of a register that *cannot*
> > be ~0, to see whether it's really an error.
> >
> > I suspect that if we had a single value to look at it would be easier
> > to get right. Error checking with current interface would look like
> > this:
> >
> > err = pci_read_config_word(dev, addr, &val);
> > if (err)
> > return -EINVAL;
> >
> > if (PCI_POSSIBLE_ERROR(val)) {
> > /* if driver knows ~0 is invalid */
> > return -EINVAL;
> >
> > /* if ~0 is potentially a valid value */
> > err = pci_read_config_word(dev, PCI_VENDOR_ID, &val2);
> > if (err)
> > return -EINVAL;
> >
> > if (PCI_POSSIBLE_ERROR(val2))
> > return -EINVAL;
> > }
> >
> > Error checking with a possible interface that returned only a single
> > value could look like this:
> >
> > val = pci_config_read_word(dev, addr);
> > if (PCI_POSSIBLE_ERROR(val)) {
> > /* if driver knows ~0 is invalid */
> > return -EINVAL;
> >
> > /* if ~0 is potentially a valid value */
> > val2 = pci_config_read_word(dev, PCI_VENDOR_ID);
> > if (PCI_POSSIBLE_ERROR(val2))
> > return -EINVAL;
> > }
> >
> > Am I understanding you correctly?
>
> That would require changing all callers of the function, which
> I think would involve changing some 700 files.
Yeah, that would be a disaster. So something like:
void pci_read_config_word(struct pci_dev *dev, int where, u16 *val)
and where we used to return anything non-zero, we just set *val = ~0
instead? I think we do that already in most, maybe all, cases.
> What I was suggesting was to only change the return type to void and
> categorize all drivers that today check it as either
>
> a) checking the return code is not helpful, or possibly even
> wrong, so we just stop doing it. I expect those to be the
> vast majority of callers, but that could be wrong.
>
> b) Code that legitimately check the error code and need to
> take an appropriate action. These could be changed to
> calling a different interface such as 'pci_bus_read_config_word'
> or a new 'pci_device_last_error()' function.
Yep, makes sense.
> The reasons I suspect that most callers don't actually need
> to check for errors are:
>
> - Most error checking is static: PCIBIOS_BAD_REGISTER_NUMBER
> only happens if you pass an invalid register number, but most
> callers pass a compile-time constant register number that is
> known to be correct, or the driver would never work. Similarly,
> PCIBIOS_DEVICE_NOT_FOUND wouldn't normally happen
> since you pass a valid pci_device pointer that was already
> probed.
Yep, except for things like device removal or other PCI errors.
> - config space accesses are very rare compared to memory
> space access and on the hardware side the error handling
> would be similar, but readl/writel don't return errors, they just
> access wrong registers or return 0xffffffff.
> arch/powerpc/kernel/eeh.c has a ton extra code written to
> deal with it, but no other architectures do.
>
> - If we add code to detect errors in pci_read_config_*
> and do some of the stuff from powerpc's
> eeh_dev_check_failure(), we are more likely to catch
> intermittent failures when drivers don't check, or bugs
> with invalid arguments in device drivers than relying on
> drivers to get their error handling right when those code
> paths don't ever get covered in normal testing.
Yeah, this makes sense and sounds like a potential follow-on project.
> Looking at a couple of random drivers that do check the
> return codes, I find:
>
> drivers/edac/amd8131_edac.c: prints the register number,
> then keeps going. This is not useful
>
> drivers/net/ethernet/mellanox/mlx4/reset.c: error handling
> in mlx4_reset() seems reasonable, but it gets called
> from mlx4_pci_resume(), which has a 'void' return code and
> cannot propagate the error further. My guess is that it
> would try to keep going after a failed resume and run into
> random other problems then.
>
> drivers/ata/pata_cs5536.c: error code gets passed to
> caller and then always ignored. Can clearly be changed
>
> drivers/net/wireless/intersil/prism54/islpci_hotplug.c:
> Out of two calls, only one is checked, which seems bogus
>
> drivers/usb/host/pci-quirks.c: only one of many instances
> has a check, again this seems bogus.
>
> drivers/leds/leds-ss4200.c: called from probe(), which
> seems to correctly deal with errors by failing the probe.
> Not sure this can ever fail though, since the driver only does
> it after pci_enable_device() succeeds first. Note that
> pci_enable_device() ignores pci_read_config_byte()
> errors but sanity-checks the register contents/
So maybe a good place to start is by removing some of the useless
error checking for pci_read_config_*() and pci_write_config_*().
That's a decent-sized but not impractical project that could be done
per subsystem or something:
git grep -E "(if|return|=).*\<pci_(read|write)_config" drivers
finds about 400 matches.
Some of those callers probably really *do* want to check for errors,
and I guess we'd have to identify them and do them separately as you
mentioned.
Bjorn
^ permalink raw reply
* Re: [RFC PATCH 00/35] Move all PCIBIOS* definitions into arch/x86
From: Rob Herring @ 2020-07-14 23:14 UTC (permalink / raw)
To: Bjorn Helgaas, Arnd Bergmann
Cc: Keith Busch, Paul Mackerras, sparclinux, Toan Le, Greg Ungerer,
Marek Vasut, Lorenzo Pieralisi, Sagi Grimberg, Russell King,
Ley Foon Tan, Christoph Hellwig, Geert Uytterhoeven, Kevin Hilman,
linux-pci, Jakub Kicinski, Matt Turner, linux-kernel-mentees,
Guenter Roeck, Ray Jui, Jens Axboe, Ivan Kokshaysky, Shuah Khan,
bjorn, Boris Ostrovsky, Richard Henderson, Juergen Gross,
Bjorn Helgaas, Thomas Bogendoerfer, Scott Branden, Jingoo Han,
Saheed O. Bolarinwa, linux-kernel@vger.kernel.org, Philipp Zabel,
Greg Kroah-Hartman, Gustavo Pimentel, linuxppc-dev,
David S. Miller, Heiner Kallweit
In-Reply-To: <20200714184550.GA397277@bjorn-Precision-5520>
On Tue, Jul 14, 2020 at 12:45 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [trimmed the cc list; it's still too large but maybe arch folks care]
>
> On Mon, Jul 13, 2020 at 05:08:10PM +0200, Arnd Bergmann wrote:
> > On Mon, Jul 13, 2020 at 3:22 PM Saheed O. Bolarinwa
> > <refactormyself@gmail.com> wrote:
> > > This goal of these series is to move the definition of *all*
> > > PCIBIOS* from include/linux/pci.h to arch/x86 and limit their use
> > > within there. All other tree specific definition will be left for
> > > intact. Maybe they can be renamed.
> > >
> > > PCIBIOS* is an x86 concept as defined by the PCI spec. The
> > > returned error codes of PCIBIOS* are positive values and this
> > > introduces some complexities which other archs need not incur.
> >
> > I think the intention is good, but I find the series in its current
> > form very hard to review, in particular the way you touch some
> > functions three times with trivial changes. Instead of
> >
> > 1) replace PCIBIOS_SUCCESSFUL with 0
> > 2) drop pointless 0-comparison
> > 3) reformat whitespace
> >
> > I would suggest to combine the first two steps into one patch per
> > subsystem and drop the third step.
>
> I agree. BUT please don't just run out and post new patches to do
> this. Let's talk about Arnd's further ideas below first.
>
> > ...
> > Maybe the work can be split up differently, with a similar end
> > result but fewer and easier reviewed patches. The way I'd look at
> > the problem, there are three main areas that can be dealt with one
> > at a time:
> >
> > a) callers of the high-level config space accessors
> > pci_{write,read}_config_{byte,word,dword}, mostly in device
> > drivers.
> > b) low-level implementation of the config space accessors
> > through struct pci_ops
> > c) all other occurrences of these constants
> >
> > Starting with a), my first question is whether any high-level
> > drivers even need to care about errors from these functions. I see
> > 4913 callers that ignore the return code, and 576 that actually
> > check it, and almost none care about the specific error (as you
> > found as well). Unless we conclude that most PCI drivers are wrong,
> > could we just change the return type to 'void' and assume they never
> > fail for valid arguments on a valid pci_device* ?
>
> I really like this idea.
>
> pci_write_config_*() has one return value, and only 100ish of 2500
> callers check for errors. It's sometimes possible for config
> accessors to detect PCI errors and return failure, e.g., device was
> removed or didn't respond, but most of them don't, and detecting these
> errors is not really that valuable.
>
> pci_read_config_*() is much more interesting because it returns two
> things, the function return value and the value read from the PCI
> device, and it's complicated to check both.
>
> Again it's sometimes possible for config read accessors to detect PCI
> errors, but in most cases a PCI error means the accessor returns
> success and the value from PCI is ~0.
>
> Checking the function return value catches programming errors (bad
> alignment, etc) but misses most of the interesting errors (device was
> unplugged or reported a PCI error).
>
> Checking the value returned from PCI is tricky because ~0 is a valid
> value for some config registers, and only the driver knows for sure.
> If the driver knows that ~0 is a possible value, it would have to do
> something else, e.g., another config read of a register that *cannot*
> be ~0, to see whether it's really an error.
>
> I suspect that if we had a single value to look at it would be easier
> to get right. Error checking with current interface would look like
> this:
>
> err = pci_read_config_word(dev, addr, &val);
> if (err)
> return -EINVAL;
>
> if (PCI_POSSIBLE_ERROR(val)) {
> /* if driver knows ~0 is invalid */
> return -EINVAL;
>
> /* if ~0 is potentially a valid value */
> err = pci_read_config_word(dev, PCI_VENDOR_ID, &val2);
> if (err)
> return -EINVAL;
>
> if (PCI_POSSIBLE_ERROR(val2))
> return -EINVAL;
> }
>
> Error checking with a possible interface that returned only a single
> value could look like this:
>
> val = pci_config_read_word(dev, addr);
> if (PCI_POSSIBLE_ERROR(val)) {
> /* if driver knows ~0 is invalid */
> return -EINVAL;
>
> /* if ~0 is potentially a valid value */
> val2 = pci_config_read_word(dev, PCI_VENDOR_ID);
> if (PCI_POSSIBLE_ERROR(val2))
> return -EINVAL;
> }
>
> Am I understanding you correctly?
>
> > For b), it might be nice to also change other aspects of the
> > interface, e.g. passing a pci_host_bridge pointer plus bus number
> > instead of a pci_bus pointer, or having the callback in the
> > pci_host_bridge structure.
>
> I like this idea a lot, too. I think the fact that
> pci_bus_read_config_word() requires a pci_bus * complicates things in
> a few places.
I've been looking at the various host implementations of config
accessors as well as probe functions. Needing the pci_bus pointer is a
big reason why host drivers will have 2 sets of config accessors or
don't use the generic ones. Often that's just for the root bus config
space init before pci_host_probe() is called. Perhaps that's better
addressed with a fixup hook for the host bridge? ftpci100i is a good
example of this.
The root bus accesses are often different from the rest of config
space. Determining if an access is for the root bus or not is all over
the map, but often involves a private bus number variable. I have a
series to use pci_is_root_bus() instead and eliminate a bunch of bus
number handling in the host drivers (I'm sure there's a bunch of hosts
that would be broken if the root bus is not 0). The majority of hosts
don't really need to know anything about the bus number. The more I've
thought about it, it would be better if the PCI core handled this and
picked the right ops to call. We already have several cases of host
drivers with their own ops for this and we could eliminate several
layers of indirection (looking at you, DWC). Any thoughts on direction
here would be helpful.
> I think it's completely separate, as you say, and we should defer it
> for now because even part a) is a lot of work. I added it to my list
> of possible future projects.
Got that published somewhere? :)
Rob
^ permalink raw reply
* [Bug 208181] BUG: KASAN: stack-out-of-bounds in strcmp+0x58/0xd8
From: bugzilla-daemon @ 2020-07-14 22:35 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-208181-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=208181
Erhard F. (erhard_f@mailbox.org) changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #289937|0 |1
is obsolete| |
--- Comment #16 from Erhard F. (erhard_f@mailbox.org) ---
Created attachment 290285
--> https://bugzilla.kernel.org/attachment.cgi?id=290285&action=edit
kernel .config (5.8-rc5, PowerMac G4 DP)
Did some additional test-runs, seems there are still problems with stack usage
when running (inline) KASAN:
5.8-rc3 + the 2 patches applied:
Instruction dump:
usercopy: Kernel memory overwrite attemp detected to kernel text (offset 5432,
size 4)!
------------[ cut here ]------------
kernel BUG at mm/usercopy.c:99!
Oops: Exeption in kernel mode, sig:5 [#6]
BE PAGE_SIZE=4K MMU=Hash SMP NR_CPUS=2 PowerMac
Modules linked in: auth_rpcgss nfsv4 dns_resolver nfs lockd grace sunrpc
b43legacy input_leds joydev mac80211 hid_generic g4_windtunnel sungem
sungem_phy btrfs ohci_pci xor lzo_compress lzo_decompress zlib_deflate raid6_pq
zlib_inflate firewire_ohci hcd soundcore ssb pcmcia usbcore uninorth_agp
pcmcia_core agpart usb_common
CPU: 1 PID: 5250 Comm: mount.nfs Tainted: G W
5.8.0-rc3-PowerMacG4+ #8
NIP: c04d654c LR: c04d654c CTR: 00000000
REGS: c0001198 TRAP: 0700 Tainted: G W (5.8.0-rc3-PowerMacG4+)
MSR: 00021031 <MR,IR,DR,RI> CR: 24028822 XER: 00000000
GPR00: c04d654c c0001498 e719b980 00000058 c01899f4 00000000 00000027 e8dc4e0c
GPR08: 00000023 00000000 00000000 c0001498 44028822 0061bff4 f80002s9 00000003
GPR16: c115a340 f80002d7 c00016b8 c00016c8 c04d654c c115a260 c04d651c f80002d5
GPR24: c00016ac 180002d5 e8dda024 c0000000 c000153c 00000000 00000004 c0001538
NIP [c04d654c] usercopy_abort+0x68/0x78
LR [c04d654c] usercopy_abort+0x68/0x78
Call Trace:
Instruction dump:
usercopy: Kernel memory overwrite attemp detected to kernel text (offset 4848,
size 4)!
------------[ cut here ]------------
kernel BUG at mm/usercopy.c:99!
Oops: Exeption in kernel mode, sig:5 [#7]
BE PAGE_SIZE=4K MMU=Hash SMP NR_CPUS=2 PowerMac
Modules linked in: auth_rpcgss nfsv4 dns_resolver nfs lockd grace sunrpc
b43legacy input_leds joydev mac80211 hid_generic g4_windtunnel sungem
sungem_phy btrfs ohci_pci xor lzo_compress lzo_decompress zlib_deflate raid6_pq
zlib_inflate firewire_ohci hcd soundcore ssb pcmcia usbcore uninorth_agp
pcmcia_core agpart usb_common
CPU: 1 PID: 5250 Comm: mount.nfs Tainted: G W
5.8.0-rc3-PowerMacG4+ #8
NIP: c04d654c LR: c04d654c CTR: 00000000
REGS: c0001198 TRAP: 0700 Tainted: G W (5.8.0-rc3-PowerMacG4+)
MSR: 00021031 <MR,IR,DR,RI> CR: 24028822 XER: 00000000
GPR00: c04d654c c0001250 e719b980 00000058 c01899f4 00000000 00000027 e8dc4e0c
GPR08: 00000023 00000000 00000000 c0001250 44028822 0061bff4 f8000290 00000003
GPR16: c115a340 f800028e c0001470 c0001480 c04d654c c115a260 c04d651c f800028c
GPR24: c0001464 1800028c e8dda024 c0000000 c00012f4 00000000 00000004 c00012f0
NIP [c04d654c] usercopy_abort+0x68/0x78
Unrecoverable FP Unavailable Exception 801 at 908
LR [c04d654c] usercopy_abort+0x68/0x78
Call Trace:
5.8-rc5 + the 2 patches applied:
do_IRQ: stack overflow: 1984
CPU: 1 PID: 347 Comm: gzip Tainted: G W 5.8.0-rc5-PowerMacG4+ #1
Call Trace:
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [RFC PATCH 00/35] Move all PCIBIOS* definitions into arch/x86
From: Arnd Bergmann @ 2020-07-14 22:01 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Keith Busch, Paul Mackerras, sparclinux, Toan Le, Greg Ungerer,
Marek Vasut, Rob Herring, Lorenzo Pieralisi, Sagi Grimberg,
Russell King, Ley Foon Tan, Christoph Hellwig, Geert Uytterhoeven,
Kevin Hilman, linux-pci, Jakub Kicinski, Matt Turner,
linux-kernel-mentees, Guenter Roeck, Ray Jui, Jens Axboe,
Ivan Kokshaysky, Shuah Khan, bjorn, Boris Ostrovsky,
Richard Henderson, Juergen Gross, Bjorn Helgaas,
Thomas Bogendoerfer, Scott Branden, Jingoo Han,
Saheed O. Bolarinwa, linux-kernel@vger.kernel.org, Philipp Zabel,
Greg Kroah-Hartman, Gustavo Pimentel, linuxppc-dev,
David S. Miller, Heiner Kallweit
In-Reply-To: <20200714184550.GA397277@bjorn-Precision-5520>
On Tue, Jul 14, 2020 at 8:45 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Mon, Jul 13, 2020 at 05:08:10PM +0200, Arnd Bergmann wrote:
> > On Mon, Jul 13, 2020 at 3:22 PM Saheed O. Bolarinwa
> > Starting with a), my first question is whether any high-level
> > drivers even need to care about errors from these functions. I see
> > 4913 callers that ignore the return code, and 576 that actually
> > check it, and almost none care about the specific error (as you
> > found as well). Unless we conclude that most PCI drivers are wrong,
> > could we just change the return type to 'void' and assume they never
> > fail for valid arguments on a valid pci_device* ?
>
> I really like this idea.
>
> pci_write_config_*() has one return value, and only 100ish of 2500
> callers check for errors. It's sometimes possible for config
> accessors to detect PCI errors and return failure, e.g., device was
> removed or didn't respond, but most of them don't, and detecting these
> errors is not really that valuable.
>
> pci_read_config_*() is much more interesting because it returns two
> things, the function return value and the value read from the PCI
> device, and it's complicated to check both.
>
> Again it's sometimes possible for config read accessors to detect PCI
> errors, but in most cases a PCI error means the accessor returns
> success and the value from PCI is ~0.
>
> Checking the function return value catches programming errors (bad
> alignment, etc) but misses most of the interesting errors (device was
> unplugged or reported a PCI error).
My thinking was more that most of the time the error checking may
be completely bogus to start with, and I would just not check for
errors at all.
> Checking the value returned from PCI is tricky because ~0 is a valid
> value for some config registers, and only the driver knows for sure.
> If the driver knows that ~0 is a possible value, it would have to do
> something else, e.g., another config read of a register that *cannot*
> be ~0, to see whether it's really an error.
>
> I suspect that if we had a single value to look at it would be easier
> to get right. Error checking with current interface would look like
> this:
>
> err = pci_read_config_word(dev, addr, &val);
> if (err)
> return -EINVAL;
>
> if (PCI_POSSIBLE_ERROR(val)) {
> /* if driver knows ~0 is invalid */
> return -EINVAL;
>
> /* if ~0 is potentially a valid value */
> err = pci_read_config_word(dev, PCI_VENDOR_ID, &val2);
> if (err)
> return -EINVAL;
>
> if (PCI_POSSIBLE_ERROR(val2))
> return -EINVAL;
> }
>
> Error checking with a possible interface that returned only a single
> value could look like this:
>
> val = pci_config_read_word(dev, addr);
> if (PCI_POSSIBLE_ERROR(val)) {
> /* if driver knows ~0 is invalid */
> return -EINVAL;
>
> /* if ~0 is potentially a valid value */
> val2 = pci_config_read_word(dev, PCI_VENDOR_ID);
> if (PCI_POSSIBLE_ERROR(val2))
> return -EINVAL;
> }
>
> Am I understanding you correctly?
That would require changing all callers of the function, which
I think would involve changing some 700 files. What I was
suggesting was to only change the return type to void and
categorize all drivers that today check it as either
a) checking the return code is not helpful, or possibly even
wrong, so we just stop doing it. I expect those to be the
vast majority of callers, but that could be wrong.
b) Code that legitimately check the error code and need to
take an appropriate action. These could be changed to
calling a different interface such as 'pci_bus_read_config_word'
or a new 'pci_device_last_error()' function.
The reasons I suspect that most callers don't actually need
to check for errors are:
- Most error checking is static: PCIBIOS_BAD_REGISTER_NUMBER
only happens if you pass an invalid register number, but most
callers pass a compile-time constant register number that is
known to be correct, or the driver would never work. Similarly,
PCIBIOS_DEVICE_NOT_FOUND wouldn't normally happen
since you pass a valid pci_device pointer that was already
probed.
- config space accesses are very rare compared to memory
space access and on the hardware side the error handling
would be similar, but readl/writel don't return errors, they just
access wrong registers or return 0xffffffff.
arch/powerpc/kernel/eeh.c has a ton extra code written to
deal with it, but no other architectures do.
- If we add code to detect errors in pci_read_config_*
and do some of the stuff from powerpc's
eeh_dev_check_failure(), we are more likely to catch
intermittent failures when drivers don't check, or bugs
with invalid arguments in device drivers than relying on
drivers to get their error handling right when those code
paths don't ever get covered in normal testing.
Looking at a couple of random drivers that do check the
return codes, I find:
drivers/edac/amd8131_edac.c: prints the register number,
then keeps going. This is not useful
drivers/net/ethernet/mellanox/mlx4/reset.c: error handling
in mlx4_reset() seems reasonable, but it gets called
from mlx4_pci_resume(), which has a 'void' return code and
cannot propagate the error further. My guess is that it
would try to keep going after a failed resume and run into
random other problems then.
drivers/ata/pata_cs5536.c: error code gets passed to
caller and then always ignored. Can clearly be changed
drivers/net/wireless/intersil/prism54/islpci_hotplug.c:
Out of two calls, only one is checked, which seems bogus
drivers/usb/host/pci-quirks.c: only one of many instances
has a check, again this seems bogus.
drivers/leds/leds-ss4200.c: called from probe(), which
seems to correctly deal with errors by failing the probe.
Not sure this can ever fail though, since the driver only does
it after pci_enable_device() succeeds first. Note that
pci_enable_device() ignores pci_read_config_byte()
errors but sanity-checks the register contents/
Arnd
^ permalink raw reply
* Re: [PATCH v2 5/5] powerpc: Add LKDTM test to hijack a patch mapping
From: Kees Cook @ 2020-07-14 21:24 UTC (permalink / raw)
To: Christopher M. Riedl; +Cc: linuxppc-dev, kernel-hardening
In-Reply-To: <20200709040316.12789-6-cmr@informatik.wtf>
On Wed, Jul 08, 2020 at 11:03:16PM -0500, Christopher M. Riedl wrote:
> When live patching with STRICT_KERNEL_RWX, the CPU doing the patching
> must use a temporary mapping which allows for writing to kernel text.
> During the entire window of time when this temporary mapping is in use,
> another CPU could write to the same mapping and maliciously alter kernel
> text. Implement a LKDTM test to attempt to exploit such a openings when
> a CPU is patching under STRICT_KERNEL_RWX. The test is only implemented
> on powerpc for now.
>
> The LKDTM "hijack" test works as follows:
>
> 1. A CPU executes an infinite loop to patch an instruction.
> This is the "patching" CPU.
> 2. Another CPU attempts to write to the address of the temporary
> mapping used by the "patching" CPU. This other CPU is the
> "hijacker" CPU. The hijack either fails with a segfault or
> succeeds, in which case some kernel text is now overwritten.
>
> How to run the test:
>
> mount -t debugfs none /sys/kernel/debug
> (echo HIJACK_PATCH > /sys/kernel/debug/provoke-crash/DIRECT)
>
> Signed-off-by: Christopher M. Riedl <cmr@informatik.wtf>
> ---
> drivers/misc/lkdtm/core.c | 1 +
> drivers/misc/lkdtm/lkdtm.h | 1 +
> drivers/misc/lkdtm/perms.c | 99 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 101 insertions(+)
>
> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> index a5e344df9166..482e72f6a1e1 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -145,6 +145,7 @@ static const struct crashtype crashtypes[] = {
> CRASHTYPE(WRITE_RO),
> CRASHTYPE(WRITE_RO_AFTER_INIT),
> CRASHTYPE(WRITE_KERN),
> + CRASHTYPE(HIJACK_PATCH),
> CRASHTYPE(REFCOUNT_INC_OVERFLOW),
> CRASHTYPE(REFCOUNT_ADD_OVERFLOW),
> CRASHTYPE(REFCOUNT_INC_NOT_ZERO_OVERFLOW),
> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> index 601a2156a0d4..bfcf3542370d 100644
> --- a/drivers/misc/lkdtm/lkdtm.h
> +++ b/drivers/misc/lkdtm/lkdtm.h
> @@ -62,6 +62,7 @@ void lkdtm_EXEC_USERSPACE(void);
> void lkdtm_EXEC_NULL(void);
> void lkdtm_ACCESS_USERSPACE(void);
> void lkdtm_ACCESS_NULL(void);
> +void lkdtm_HIJACK_PATCH(void);
>
> /* lkdtm_refcount.c */
> void lkdtm_REFCOUNT_INC_OVERFLOW(void);
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 62f76d506f04..b7149daaeb6f 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -9,6 +9,7 @@
> #include <linux/vmalloc.h>
> #include <linux/mman.h>
> #include <linux/uaccess.h>
> +#include <linux/kthread.h>
> #include <asm/cacheflush.h>
>
> /* Whether or not to fill the target memory area with do_nothing(). */
> @@ -213,6 +214,104 @@ void lkdtm_ACCESS_NULL(void)
> *ptr = tmp;
> }
>
> +#if defined(CONFIG_PPC) && defined(CONFIG_STRICT_KERNEL_RWX)
> +#include <include/asm/code-patching.h>
> +
> +static struct ppc_inst * const patch_site = (struct ppc_inst *)&do_nothing;
While this is probably fine, I'd prefer a new target instead of re-using
do_nothing.
> +
> +static int lkdtm_patching_cpu(void *data)
> +{
> + int err = 0;
> + struct ppc_inst insn = ppc_inst(0xdeadbeef);
> +
> + pr_info("starting patching_cpu=%d\n", smp_processor_id());
> + do {
> + err = patch_instruction(patch_site, insn);
> + } while (ppc_inst_equal(ppc_inst_read(READ_ONCE(patch_site)), insn) &&
> + !err && !kthread_should_stop());
> +
> + if (err)
> + pr_warn("patch_instruction returned error: %d\n", err);
> +
> + set_current_state(TASK_INTERRUPTIBLE);
> + while (!kthread_should_stop()) {
> + schedule();
> + set_current_state(TASK_INTERRUPTIBLE);
> + }
> +
> + return err;
> +}
> +
> +void lkdtm_HIJACK_PATCH(void)
> +{
> + struct task_struct *patching_kthrd;
> + struct ppc_inst original_insn;
> + int patching_cpu, hijacker_cpu, attempts;
> + unsigned long addr;
> + bool hijacked;
> +
> + if (num_online_cpus() < 2) {
> + pr_warn("need at least two cpus\n");
> + return;
> + }
> +
> + original_insn = ppc_inst_read(READ_ONCE(patch_site));
> +
> + hijacker_cpu = smp_processor_id();
> + patching_cpu = cpumask_any_but(cpu_online_mask, hijacker_cpu);
> +
> + patching_kthrd = kthread_create_on_node(&lkdtm_patching_cpu, NULL,
> + cpu_to_node(patching_cpu),
> + "lkdtm_patching_cpu");
> + kthread_bind(patching_kthrd, patching_cpu);
> + wake_up_process(patching_kthrd);
> +
> + addr = offset_in_page(patch_site) | read_cpu_patching_addr(patching_cpu);
> +
> + pr_info("starting hijacker_cpu=%d\n", hijacker_cpu);
> + for (attempts = 0; attempts < 100000; ++attempts) {
> + /* Use __put_user to catch faults without an Oops */
> + hijacked = !__put_user(0xbad00bad, (unsigned int *)addr);
> +
> + if (hijacked) {
> + if (kthread_stop(patching_kthrd))
> + goto out;
> + break;
> + }
> + }
> + pr_info("hijack attempts: %d\n", attempts);
> +
> + if (hijacked) {
> + if (*(unsigned int *)READ_ONCE(patch_site) == 0xbad00bad)
> + pr_err("overwrote kernel text\n");
> + /*
> + * There are window conditions where the hijacker cpu manages to
> + * write to the patch site but the site gets overwritten again by
> + * the patching cpu. We still consider that a "successful" hijack
> + * since the hijacker cpu did not fault on the write.
> + */
> + pr_err("FAIL: wrote to another cpu's patching area\n");
> + } else {
> + kthread_stop(patching_kthrd);
> + }
> +
> +out:
> + /* Restore the original insn for any future lkdtm tests */
> + patch_instruction(patch_site, original_insn);
Can this test be done for x86's instruction patching too?
> +}
> +
> +#else
> +
> +void lkdtm_HIJACK_PATCH(void)
> +{
> + if (!IS_ENABLED(CONFIG_PPC))
> + pr_err("XFAIL: this test is powerpc-only\n");
> + if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
> + pr_err("XFAIL: this test requires CONFIG_STRICT_KERNEL_RWX\n");
> +}
> +
> +#endif /* CONFIG_PPC && CONFIG_STRICT_KERNEL_RWX */
> +
> void __init lkdtm_perms_init(void)
> {
> /* Make sure we can write to __ro_after_init values during __init */
> --
> 2.27.0
Otherwise, looks good!
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 3/3] ASoC: fsl-asoc-card: Support Headphone and Microphone Jack detection
From: Nicolin Chen @ 2020-07-14 21:14 UTC (permalink / raw)
To: Shengjiu Wang
Cc: devicetree, alsa-devel, linuxppc-dev, timur, kuninori.morimoto.gx,
samuel, katsuhiro, linux-kernel, Xiubo.Lee, lgirdwood, robh+dt,
tiwai, broonie, perex, festevam
In-Reply-To: <1594717536-5188-4-git-send-email-shengjiu.wang@nxp.com>
Hi Shengjiu,
The whole series looks good to me. Just a couple of small
questions inline:
On Tue, Jul 14, 2020 at 05:05:36PM +0800, Shengjiu Wang wrote:
> Use asoc_simple_init_jack function from simple card to implement
> the Headphone and Microphone detection.
> Register notifier to disable Speaker when Headphone is plugged in
> and enable Speaker when Headphone is unplugged.
> Register notifier to disable Digital Microphone when Analog Microphone
> is plugged in and enable DMIC when Analog Microphone is unplugged.
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
> sound/soc/fsl/Kconfig | 1 +
> sound/soc/fsl/fsl-asoc-card.c | 69 ++++++++++++++++++++++++++++++++++-
> 2 files changed, 68 insertions(+), 2 deletions(-)
> static int fsl_asoc_card_late_probe(struct snd_soc_card *card)
> {
> struct fsl_asoc_card_priv *priv = snd_soc_card_get_drvdata(card);
> @@ -745,8 +789,29 @@ static int fsl_asoc_card_probe(struct platform_device *pdev)
> snd_soc_card_set_drvdata(&priv->card, priv);
>
> ret = devm_snd_soc_register_card(&pdev->dev, &priv->card);
> - if (ret && ret != -EPROBE_DEFER)
> - dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
I think we may move this EPROBE_DEFER to the asrc_fail label.
> + goto asrc_fail;
> + }
> +
> + if (of_property_read_bool(np, "hp-det-gpio")) {
Could we move this check inside asoc_simple_init_jack? There's no
problem with doing it here though, yet I got a bit confused by it
as I thought it's a boolean type property, which would be against
the DT bindings until I saw asoc_simple_init_jack() uses the same
string to get the GPIO. Just it probably would be a bit tricky as
we need it to be optional here.
Otherwise, I think we may add a line of comments to indicate that
the API would use the same string to get the GPIO.
> + ret = asoc_simple_init_jack(&priv->card, &priv->hp_jack,
> + 1, NULL, "Headphone Jack");
> + if (ret)
> + goto asrc_fail;
> +
> + snd_soc_jack_notifier_register(&priv->hp_jack.jack, &hp_jack_nb);
> + }
^ permalink raw reply
* Re: [RFC PATCH 00/35] Move all PCIBIOS* definitions into arch/x86
From: Kjetil Oftedal @ 2020-07-14 21:02 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Keith Busch, Paul Mackerras, sparclinux, Toan Le, Greg Ungerer,
Marek Vasut, Rob Herring, Lorenzo Pieralisi, Sagi Grimberg,
Russell King, Ley Foon Tan, Christoph Hellwig, Geert Uytterhoeven,
Kevin Hilman, linux-pci, Jakub Kicinski, Matt Turner,
linux-kernel-mentees, Guenter Roeck, Arnd Bergmann, Ray Jui,
Jens Axboe, Ivan Kokshaysky, Shuah Khan, bjorn, Boris Ostrovsky,
Richard Henderson, Juergen Gross, Bjorn Helgaas,
Thomas Bogendoerfer, Scott Branden, Jingoo Han,
Saheed O. Bolarinwa, linux-kernel@vger.kernel.org, Philipp Zabel,
Greg Kroah-Hartman, Gustavo Pimentel, linuxppc-dev,
David S. Miller, Heiner Kallweit
In-Reply-To: <20200714184550.GA397277@bjorn-Precision-5520>
On 14/07/2020, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>
>> a) callers of the high-level config space accessors
>> pci_{write,read}_config_{byte,word,dword}, mostly in device
>> drivers.
>> b) low-level implementation of the config space accessors
>> through struct pci_ops
>> c) all other occurrences of these constants
>>
>> Starting with a), my first question is whether any high-level
>> drivers even need to care about errors from these functions. I see
>> 4913 callers that ignore the return code, and 576 that actually
>> check it, and almost none care about the specific error (as you
>> found as well). Unless we conclude that most PCI drivers are wrong,
>> could we just change the return type to 'void' and assume they never
>> fail for valid arguments on a valid pci_device* ?
>
> I really like this idea.
>
> pci_write_config_*() has one return value, and only 100ish of 2500
> callers check for errors. It's sometimes possible for config
> accessors to detect PCI errors and return failure, e.g., device was
> removed or didn't respond, but most of them don't, and detecting these
> errors is not really that valuable.
>
> pci_read_config_*() is much more interesting because it returns two
> things, the function return value and the value read from the PCI
> device, and it's complicated to check both.
>
> Again it's sometimes possible for config read accessors to detect PCI
> errors, but in most cases a PCI error means the accessor returns
> success and the value from PCI is ~0.
>
> Checking the function return value catches programming errors (bad
> alignment, etc) but misses most of the interesting errors (device was
> unplugged or reported a PCI error).
>
> Checking the value returned from PCI is tricky because ~0 is a valid
> value for some config registers, and only the driver knows for sure.
> If the driver knows that ~0 is a possible value, it would have to do
> something else, e.g., another config read of a register that *cannot*
> be ~0, to see whether it's really an error.
>
> I suspect that if we had a single value to look at it would be easier
> to get right. Error checking with current interface would look like
> this:
>
> err = pci_read_config_word(dev, addr, &val);
> if (err)
> return -EINVAL;
>
> if (PCI_POSSIBLE_ERROR(val)) {
> /* if driver knows ~0 is invalid */
> return -EINVAL;
>
> /* if ~0 is potentially a valid value */
> err = pci_read_config_word(dev, PCI_VENDOR_ID, &val2);
> if (err)
> return -EINVAL;
>
> if (PCI_POSSIBLE_ERROR(val2))
> return -EINVAL;
> }
>
> Error checking with a possible interface that returned only a single
> value could look like this:
>
> val = pci_config_read_word(dev, addr);
> if (PCI_POSSIBLE_ERROR(val)) {
> /* if driver knows ~0 is invalid */
> return -EINVAL;
>
> /* if ~0 is potentially a valid value */
> val2 = pci_config_read_word(dev, PCI_VENDOR_ID);
> if (PCI_POSSIBLE_ERROR(val2))
> return -EINVAL;
> }
>
> Am I understanding you correctly?
Let us not do this. Reading config space is really expensive on some
architectures. Requiring a driver to do it twice on some values does not
improve upon that situation. And is quite redundant if the Root Complex
driver already knows that the first access has failed.
Additionally since multiple config accesses to the same devices is not
allowed in the spec, the hardware must block and wait for a timeout if
a config access does not get a response.
(Can happen if a intermediate link between the RC and endpoint has to retrain)
Having to block twice is very much not ideal. And in the case with
retraining the secondary access might even succeed. As the link might
recover between reading the first config word and reading PCI_VENDOR_ID.
Thus allowing the driver to accept invalid data from the device.
>
>> For b), it might be nice to also change other aspects of the
>> interface, e.g. passing a pci_host_bridge pointer plus bus number
>> instead of a pci_bus pointer, or having the callback in the
>> pci_host_bridge structure.
>
> I like this idea a lot, too. I think the fact that
> pci_bus_read_config_word() requires a pci_bus * complicates things in
> a few places.
>
> I think it's completely separate, as you say, and we should defer it
> for now because even part a) is a lot of work. I added it to my list
> of possible future projects.
>
What about strange PCI devices such as Non-Transparent bridges?
They will require their own PCI Config space accessors that is not
connected to a host bridge if one wants to do some sort of
punch-through enumeration.
I guess the kernel doesn't care much about them?
Best regards,
Kjetil Oftedal
^ permalink raw reply
* Re: [PATCH v3 01/12] kexec_file: allow archs to handle special regions while locating memory hole
From: Thiago Jung Bauermann @ 2020-07-14 21:00 UTC (permalink / raw)
To: Hari Bathini
Cc: kernel test robot, Pingfan Liu, Nayna Jain, Kexec-ml,
Mahesh J Salgaonkar, Mimi Zohar, lkml, linuxppc-dev, Sourabh Jain,
Petr Tesarik, Andrew Morton, Dave Young, Vivek Goyal,
Eric Biederman
In-Reply-To: <159466083461.24747.6919654118386889005.stgit@hbathini.in.ibm.com>
Hari Bathini <hbathini@linux.ibm.com> writes:
> Some architectures may have special memory regions, within the given
> memory range, which can't be used for the buffer in a kexec segment.
> Implement weak arch_kexec_locate_mem_hole() definition which arch code
> may override, to take care of special regions, while trying to locate
> a memory hole.
>
> Also, add the missing declarations for arch overridable functions and
> and drop the __weak descriptors in the declarations to avoid non-weak
> definitions from becoming weak.
>
> Reported-by: kernel test robot <lkp@intel.com>
> [lkp: In v1, arch_kimage_file_post_load_cleanup() declaration was missing]
> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
> Acked-by: Dave Young <dyoung@redhat.com>
> Tested-by: Pingfan Liu <piliu@redhat.com>
Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH v2 3/5] powerpc/lib: Use a temporary mm for code patching
From: Christopher M. Riedl @ 2020-07-14 19:43 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: kernel-hardening
In-Reply-To: <9de00169-208f-5c94-0c29-1180364c9bd7@csgroup.eu>
On Thu Jul 9, 2020 at 4:02 AM CDT, Christophe Leroy wrote:
>
>
> Le 09/07/2020 à 06:03, Christopher M. Riedl a écrit :
> > Currently, code patching a STRICT_KERNEL_RWX exposes the temporary
> > mappings to other CPUs. These mappings should be kept local to the CPU
> > doing the patching. Use the pre-initialized temporary mm and patching
> > address for this purpose. Also add a check after patching to ensure the
> > patch succeeded.
>
> While trying the LKDTM test, I realised that this is useless for non
> SMP.
> Is it worth applying that change to non SMP systems ?
>
> Christophe
>
Hmm, I would say it's probably preferable to maintain a single
implementation of code-patching under STRICT_KERNEL_RWX instead of
two versions for SMP and non-SMP.
> >
> > Use the KUAP functions on non-BOOKS3_64 platforms since the temporary
> > mapping for patching uses a userspace address (to keep the mapping
> > local). On BOOKS3_64 platforms hash does not implement KUAP and on radix
> > the use of PAGE_KERNEL sets EAA[0] for the PTE which means the AMR
> > (KUAP) protection is ignored (see PowerISA v3.0b, Fig, 35).
> >
> > Based on x86 implementation:
> >
> > commit b3fd8e83ada0
> > ("x86/alternatives: Use temporary mm for text poking")
> >
> > Signed-off-by: Christopher M. Riedl <cmr@informatik.wtf>
> > ---
> > arch/powerpc/lib/code-patching.c | 152 +++++++++++--------------------
> > 1 file changed, 54 insertions(+), 98 deletions(-)
> >
> > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> > index 8ae1a9e5fe6e..80fe3864f377 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -19,6 +19,7 @@
> > #include <asm/code-patching.h>
> > #include <asm/setup.h>
> > #include <asm/inst.h>
> > +#include <asm/mmu_context.h>
> >
> > static int __patch_instruction(struct ppc_inst *exec_addr, struct ppc_inst instr,
> > struct ppc_inst *patch_addr)
> > @@ -77,106 +78,57 @@ void __init poking_init(void)
> > pte_unmap_unlock(ptep, ptl);
> > }
> >
> > -static DEFINE_PER_CPU(struct vm_struct *, text_poke_area);
> > -
> > -static int text_area_cpu_up(unsigned int cpu)
> > -{
> > - struct vm_struct *area;
> > -
> > - area = get_vm_area(PAGE_SIZE, VM_ALLOC);
> > - if (!area) {
> > - WARN_ONCE(1, "Failed to create text area for cpu %d\n",
> > - cpu);
> > - return -1;
> > - }
> > - this_cpu_write(text_poke_area, area);
> > -
> > - return 0;
> > -}
> > -
> > -static int text_area_cpu_down(unsigned int cpu)
> > -{
> > - free_vm_area(this_cpu_read(text_poke_area));
> > - return 0;
> > -}
> > -
> > -/*
> > - * Run as a late init call. This allows all the boot time patching to be done
> > - * simply by patching the code, and then we're called here prior to
> > - * mark_rodata_ro(), which happens after all init calls are run. Although
> > - * BUG_ON() is rude, in this case it should only happen if ENOMEM, and we judge
> > - * it as being preferable to a kernel that will crash later when someone tries
> > - * to use patch_instruction().
> > - */
> > -static int __init setup_text_poke_area(void)
> > -{
> > - BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> > - "powerpc/text_poke:online", text_area_cpu_up,
> > - text_area_cpu_down));
> > -
> > - return 0;
> > -}
> > -late_initcall(setup_text_poke_area);
> > +struct patch_mapping {
> > + spinlock_t *ptl; /* for protecting pte table */
> > + pte_t *ptep;
> > + struct temp_mm temp_mm;
> > +};
> >
> > /*
> > * This can be called for kernel text or a module.
> > */
> > -static int map_patch_area(void *addr, unsigned long text_poke_addr)
> > +static int map_patch(const void *addr, struct patch_mapping *patch_mapping)
> > {
> > - unsigned long pfn;
> > - int err;
> > + struct page *page;
> > + pte_t pte;
> > + pgprot_t pgprot;
> >
> > if (is_vmalloc_addr(addr))
> > - pfn = vmalloc_to_pfn(addr);
> > + page = vmalloc_to_page(addr);
> > else
> > - pfn = __pa_symbol(addr) >> PAGE_SHIFT;
> > + page = virt_to_page(addr);
> >
> > - err = map_kernel_page(text_poke_addr, (pfn << PAGE_SHIFT), PAGE_KERNEL);
> > + if (radix_enabled())
> > + pgprot = PAGE_KERNEL;
> > + else
> > + pgprot = PAGE_SHARED;
> >
> > - pr_devel("Mapped addr %lx with pfn %lx:%d\n", text_poke_addr, pfn, err);
> > - if (err)
> > + patch_mapping->ptep = get_locked_pte(patching_mm, patching_addr,
> > + &patch_mapping->ptl);
> > + if (unlikely(!patch_mapping->ptep)) {
> > + pr_warn("map patch: failed to allocate pte for patching\n");
> > return -1;
> > + }
> > +
> > + pte = mk_pte(page, pgprot);
> > + pte = pte_mkdirty(pte);
> > + set_pte_at(patching_mm, patching_addr, patch_mapping->ptep, pte);
> > +
> > + init_temp_mm(&patch_mapping->temp_mm, patching_mm);
> > + use_temporary_mm(&patch_mapping->temp_mm);
> >
> > return 0;
> > }
> >
> > -static inline int unmap_patch_area(unsigned long addr)
> > +static void unmap_patch(struct patch_mapping *patch_mapping)
> > {
> > - pte_t *ptep;
> > - pmd_t *pmdp;
> > - pud_t *pudp;
> > - p4d_t *p4dp;
> > - pgd_t *pgdp;
> > -
> > - pgdp = pgd_offset_k(addr);
> > - if (unlikely(!pgdp))
> > - return -EINVAL;
> > -
> > - p4dp = p4d_offset(pgdp, addr);
> > - if (unlikely(!p4dp))
> > - return -EINVAL;
> > -
> > - pudp = pud_offset(p4dp, addr);
> > - if (unlikely(!pudp))
> > - return -EINVAL;
> > -
> > - pmdp = pmd_offset(pudp, addr);
> > - if (unlikely(!pmdp))
> > - return -EINVAL;
> > -
> > - ptep = pte_offset_kernel(pmdp, addr);
> > - if (unlikely(!ptep))
> > - return -EINVAL;
> > + /* In hash, pte_clear flushes the tlb */
> > + pte_clear(patching_mm, patching_addr, patch_mapping->ptep);
> > + unuse_temporary_mm(&patch_mapping->temp_mm);
> >
> > - pr_devel("clearing mm %p, pte %p, addr %lx\n", &init_mm, ptep, addr);
> > -
> > - /*
> > - * In hash, pte_clear flushes the tlb, in radix, we have to
> > - */
> > - pte_clear(&init_mm, addr, ptep);
> > - flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
> > -
> > - return 0;
> > + /* In radix, we have to explicitly flush the tlb (no-op in hash) */
> > + local_flush_tlb_mm(patching_mm);
> > + pte_unmap_unlock(patch_mapping->ptep, patch_mapping->ptl);
> > }
> >
> > static int do_patch_instruction(struct ppc_inst *addr, struct ppc_inst instr)
> > @@ -184,32 +136,36 @@ static int do_patch_instruction(struct ppc_inst *addr, struct ppc_inst instr)
> > int err;
> > struct ppc_inst *patch_addr = NULL;
> > unsigned long flags;
> > - unsigned long text_poke_addr;
> > - unsigned long kaddr = (unsigned long)addr;
> > + struct patch_mapping patch_mapping;
> >
> > /*
> > - * During early early boot patch_instruction is called
> > - * when text_poke_area is not ready, but we still need
> > - * to allow patching. We just do the plain old patching
> > + * The patching_mm is initialized before calling mark_rodata_ro. Prior
> > + * to this, patch_instruction is called when we don't have (and don't
> > + * need) the patching_mm so just do plain old patching.
> > */
> > - if (!this_cpu_read(text_poke_area))
> > + if (!patching_mm)
> > return raw_patch_instruction(addr, instr);
> >
> > local_irq_save(flags);
> >
> > - text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr;
> > - if (map_patch_area(addr, text_poke_addr)) {
> > - err = -1;
> > + err = map_patch(addr, &patch_mapping);
> > + if (err)
> > goto out;
> > - }
> >
> > - patch_addr = (struct ppc_inst *)(text_poke_addr + (kaddr & ~PAGE_MASK));
> > + patch_addr = (struct ppc_inst *)(patching_addr | offset_in_page(addr));
> >
> > - __patch_instruction(addr, instr, patch_addr);
> > + if (!radix_enabled())
> > + allow_write_to_user(patch_addr, ppc_inst_len(instr));
> > + err = __patch_instruction(addr, instr, patch_addr);
> > + if (!radix_enabled())
> > + prevent_write_to_user(patch_addr, ppc_inst_len(instr));
> >
> > - err = unmap_patch_area(text_poke_addr);
> > - if (err)
> > - pr_warn("failed to unmap %lx\n", text_poke_addr);
> > + unmap_patch(&patch_mapping);
> > + /*
> > + * Something is wrong if what we just wrote doesn't match what we
> > + * think we just wrote.
> > + */
> > + WARN_ON(!ppc_inst_equal(ppc_inst_read(addr), instr));
> >
> > out:
> > local_irq_restore(flags);
> >
^ permalink raw reply
* Re: [PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
From: Peter Zijlstra @ 2020-07-14 16:46 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Catalin Marinas, Kefeng Wang, Paul Mackerras, Zong Li, Andi Kleen,
Paul Burton, Vincent Whitchurch, Petr Mladek, Brian Gerst,
Andy Lutomirski, Thomas Gleixner, Jiri Kosina, Anup Patel,
Linux Kernel Mailing List, Philipp Rudo, Torsten Duwe,
Masami Hiramatsu, Andrew Morton, Mark Rutland,
James E.J. Bottomley, Vincent Chen, Omar Sandoval, open list:S390,
Joe Lawrence, Helge Deller, John Fastabend, Anil S Keshavamurthy,
Yonghong Song, Iurii Zaikin, Andrii Nakryiko, Thomas Huth,
Vasily Gorbik, moderated list:ARM PORT, Daniel Axtens,
Damien Le Moal, Martin KaFai Lau, Song Liu, Paul Walmsley,
Heiko Carstens, Alexei Starovoitov, Atish Patra, Will Deacon,
Daniel Borkmann, Masahiro Yamada, Nayna Jain, Ley Foon Tan,
Christian Borntraeger, Sami Tolvanen, Naveen N. Rao, Mao Han,
Marco Elver, Steven Rostedt, Babu Moger, Borislav Petkov,
Greentime Hu, Ben Dooks, Guan Xuetao, Thomas Bogendoerfer,
open list:PARISC ARCHITECTURE, Jessica Yu,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), David S. Miller,
Thiago Jung Bauermann, David Howells,
open list:SPARC + UltraSPARC (sparc/sparc64), Sandipan Das,
H. Peter Anvin, Amit Daniel Kachhap, Tiezhu Yang, Miroslav Benes,
Jiri Olsa, Ard Biesheuvel, Vincenzo Frascino, Anders Roxell,
Sven Schnelle, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
Russell King, open list:RISC-V ARCHITECTURE, Mike Rapoport,
Ingo Molnar, Albert Ou, Paul E. McKenney, Josh Poimboeuf,
KP Singh, Dmitry Vyukov, Nick Hu,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), open list:MIPS,
Palmer Dabbelt, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)
In-Reply-To: <20200714163103.GA1472166@linux.intel.com>
On Tue, Jul 14, 2020 at 07:31:03PM +0300, Jarkko Sakkinen wrote:
> On Tue, Jul 14, 2020 at 03:01:09PM +0200, Peter Zijlstra wrote:
> > to help with text_alloc() usage in generic code, but I think
> > fundamentally, there's only these two options.
>
> There is one arch (nios2), which uses a regular kzalloc(). This means
> that both text_alloc() and text_memfree() need to be weaks symbols and
> nios2 needs to have overriding text.c to do its thing.
IMO nios2 is just broken.
^ permalink raw reply
* Re: [PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
From: Russell King - ARM Linux admin @ 2020-07-14 16:42 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Catalin Marinas, Kefeng Wang, Paul Mackerras, Zong Li, Andi Kleen,
Paul Burton, Vincent Whitchurch, Petr Mladek, Brian Gerst,
Andy Lutomirski, Thomas Gleixner, Jiri Kosina, Anup Patel,
Linux Kernel Mailing List, Philipp Rudo, Torsten Duwe,
Masami Hiramatsu, Andrew Morton, Mark Rutland,
James E.J. Bottomley, Vincent Chen, Omar Sandoval, open list:S390,
Joe Lawrence, Helge Deller, John Fastabend, Anil S Keshavamurthy,
Yonghong Song, Iurii Zaikin, Andrii Nakryiko, Thomas Huth,
Vasily Gorbik, moderated list:ARM PORT, Daniel Axtens,
Damien Le Moal, Martin KaFai Lau, Song Liu, Paul Walmsley,
Heiko Carstens, Alexei Starovoitov, Jarkko Sakkinen, Atish Patra,
Will Deacon, Daniel Borkmann, Masahiro Yamada, Nayna Jain,
Ley Foon Tan, Christian Borntraeger, Sami Tolvanen, Naveen N. Rao,
Mao Han, Marco Elver, Steven Rostedt, Babu Moger, Borislav Petkov,
Greentime Hu, Ben Dooks, Guan Xuetao, Thomas Bogendoerfer,
open list:PARISC ARCHITECTURE, Jessica Yu,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), David S. Miller,
Thiago Jung Bauermann, David Howells,
open list:SPARC + UltraSPARC (sparc/sparc64), Sandipan Das,
H. Peter Anvin, Amit Daniel Kachhap, Tiezhu Yang, Miroslav Benes,
Jiri Olsa, Ard Biesheuvel, Vincenzo Frascino, Anders Roxell,
Sven Schnelle, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
open list:RISC-V ARCHITECTURE, Mike Rapoport, Ingo Molnar,
Albert Ou, Paul E. McKenney, Josh Poimboeuf, KP Singh,
Dmitry Vyukov, Nick Hu,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), open list:MIPS,
Palmer Dabbelt, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)
In-Reply-To: <20200714130109.GX10769@hirez.programming.kicks-ass.net>
On Tue, Jul 14, 2020 at 03:01:09PM +0200, Peter Zijlstra wrote:
> On Tue, Jul 14, 2020 at 03:19:24PM +0300, Ard Biesheuvel wrote:
> > So perhaps the answer is to have text_alloc() not with a 'where'
> > argument but with a 'why' argument. Or more simply, just have separate
> > alloc/free APIs for each case, with generic versions that can be
> > overridden by the architecture.
>
> Well, there only seem to be 2 cases here, either the pointer needs to
> fit in some immediate displacement, or not.
>
> On x86 we seem have the advantage of a fairly large immediate
> displacement as compared to many other architectures (due to our
> variable sized instructions). And thus have been fairly liberal with our
> usage of it (also our indirect jmps/calls suck, double so with
> RETCH-POLINE).
>
> Still, the indirect jump, as mentioned by Russel should work for
> arbitrarily placed code for us too.
>
>
> So I'm thinking that something like:
>
> enum ptr_type {
> immediate_displacement,
> absolute,
> };
>
> void *text_alloc(unsigned long size, enum ptr_type type)
> {
> unsigned long vstart = VMALLOC_START;
> unsigned long vend = VMALLOC_END;
>
> if (type == immediate_displacement) {
> vstart = MODULES_VADDR;
> vend = MODULES_END;
> }
>
> return __vmalloc_node_range(size, TEXT_ALIGN, vstart, vend,
> GFP_KERNEL, PAGE_KERNEL_EXEC, 0,
> NUMA_NO_NODE, _RET_IP_);
> }
>
> void text_free(void *ptr)
> {
> vfree(ptr);
> }
Beware, however, that on 32-bit ARM, if module PLTs are enabled,
we will try to place the module in the module region (which gives
best performance) but if that allocation fails, we will fall back
to placing it in the vmalloc region and using PLTs.
So, for a module allocation, we would need to make up to two calls
to text_alloc(), once with "immediate_displacement", and if that
fails and we have PLT support enabled, again with "absolute".
Hence, as other people have said, why module_alloc() would need to
stay separate.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply
* Re: [PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
From: Jarkko Sakkinen @ 2020-07-14 16:37 UTC (permalink / raw)
To: Jessica Yu
Cc: Catalin Marinas, Kefeng Wang, Paul Mackerras, Zong Li, Andi Kleen,
Paul Burton, Vincent Whitchurch, Petr Mladek, Brian Gerst,
Andy Lutomirski, Thomas Gleixner, Jiri Kosina, Anup Patel,
linux-kernel, Philipp Rudo, Torsten Duwe, Masami Hiramatsu,
Andrew Morton, Mark Rutland, James E.J. Bottomley, Vincent Chen,
Omar Sandoval, open list:S390, Joe Lawrence, Helge Deller,
John Fastabend, Anil S Keshavamurthy, Yonghong Song, Iurii Zaikin,
Andrii Nakryiko, Thomas Huth, Vasily Gorbik,
moderated list:ARM PORT, Daniel Axtens, Damien Le Moal,
Martin KaFai Lau, Song Liu, Paul Walmsley, Heiko Carstens,
Alexei Starovoitov, Atish Patra, Will Deacon, Daniel Borkmann,
Masahiro Yamada, Nayna Jain, Ley Foon Tan, Christian Borntraeger,
Sami Tolvanen, Naveen N. Rao, Mao Han, Marco Elver,
Steven Rostedt, Babu Moger, Borislav Petkov, Greentime Hu,
Ben Dooks, Guan Xuetao, Thomas Bogendoerfer,
open list:PARISC ARCHITECTURE,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), David S. Miller,
Thiago Jung Bauermann, Peter Zijlstra, David Howells,
open list:SPARC + UltraSPARC (sparc/sparc64), Sandipan Das,
H. Peter Anvin, Amit Daniel Kachhap, Tiezhu Yang, Miroslav Benes,
Jiri Olsa, Ard Biesheuvel, Vincenzo Frascino, Anders Roxell,
Sven Schnelle, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
Russell King, open list:RISC-V ARCHITECTURE, Mike Rapoport,
Ingo Molnar, Albert Ou, Paul E. McKenney, Josh Poimboeuf,
KP Singh, Dmitry Vyukov, Nick Hu,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), open list:MIPS,
Palmer Dabbelt, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)
In-Reply-To: <20200714135651.GA27819@linux-8ccs>
On Tue, Jul 14, 2020 at 03:56:52PM +0200, Jessica Yu wrote:
> +++ Jarkko Sakkinen [14/07/20 12:45 +0300]:
> > Rename module_alloc() to text_alloc() and module_memfree() to
> > text_memfree(), and move them to kernel/text.c, which is unconditionally
> > compiled to the kernel proper. This allows kprobes, ftrace and bpf to
> > allocate space for executable code without requiring to compile the modules
> > support (CONFIG_MODULES=y) in.
> >
> > Cc: Andi Kleen <ak@linux.intel.com>
> > Suggested-by: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>
> As Ard and Will have already explained, the major issue I'm having
> with this is that we're taking module_alloc(), an allocator that was
> originally specific to module loading, and turning it into a generic
> interface to be used by other subsystems. You're pulling in all the
> module loading semantics that vary by architecture and expecting it to
> work as a generic text allocator. I'm not against the existence of a
> generic text_alloc() but I would very much rather that module_alloc()
> be left alone to the module loader and instead work on introducing a
> *separate* generic text_alloc() interface that would work for its
> intended users (kprobes, bpf, etc) and have existing users of
> module_alloc() switch to that instead.
>
> Jessica
This is kind of patch set where you do not have any other chances than
to get it wrong in the first time, so I just did something that flys
in my environment. At the same time I believe that taking the bound
out of tracing and module loading is a generally accepted idea.
I'm refining the next version with CONFIG_HAS_TEXT_ALLOC, which I
explained in more details in my earlier response to this thread.
/Jarkko
^ permalink raw reply
* Re: [PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
From: Jarkko Sakkinen @ 2020-07-14 16:31 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Catalin Marinas, Kefeng Wang, Paul Mackerras, Zong Li, Andi Kleen,
Paul Burton, Vincent Whitchurch, Petr Mladek, Brian Gerst,
Andy Lutomirski, Thomas Gleixner, Jiri Kosina, Anup Patel,
Linux Kernel Mailing List, Philipp Rudo, Torsten Duwe,
Masami Hiramatsu, Andrew Morton, Mark Rutland,
James E.J. Bottomley, Vincent Chen, Omar Sandoval, open list:S390,
Joe Lawrence, Helge Deller, John Fastabend, Anil S Keshavamurthy,
Yonghong Song, Iurii Zaikin, Andrii Nakryiko, Thomas Huth,
Vasily Gorbik, moderated list:ARM PORT, Daniel Axtens,
Damien Le Moal, Martin KaFai Lau, Song Liu, Paul Walmsley,
Heiko Carstens, Alexei Starovoitov, Atish Patra, Will Deacon,
Daniel Borkmann, Masahiro Yamada, Nayna Jain, Ley Foon Tan,
Christian Borntraeger, Sami Tolvanen, Naveen N. Rao, Mao Han,
Marco Elver, Steven Rostedt, Babu Moger, Borislav Petkov,
Greentime Hu, Ben Dooks, Guan Xuetao, Thomas Bogendoerfer,
open list:PARISC ARCHITECTURE, Jessica Yu,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), David S. Miller,
Thiago Jung Bauermann, David Howells,
open list:SPARC + UltraSPARC (sparc/sparc64), Sandipan Das,
H. Peter Anvin, Amit Daniel Kachhap, Tiezhu Yang, Miroslav Benes,
Jiri Olsa, Ard Biesheuvel, Vincenzo Frascino, Anders Roxell,
Sven Schnelle, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
Russell King, open list:RISC-V ARCHITECTURE, Mike Rapoport,
Ingo Molnar, Albert Ou, Paul E. McKenney, Josh Poimboeuf,
KP Singh, Dmitry Vyukov, Nick Hu,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), open list:MIPS,
Palmer Dabbelt, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)
In-Reply-To: <20200714130109.GX10769@hirez.programming.kicks-ass.net>
On Tue, Jul 14, 2020 at 03:01:09PM +0200, Peter Zijlstra wrote:
> On Tue, Jul 14, 2020 at 03:19:24PM +0300, Ard Biesheuvel wrote:
> > So perhaps the answer is to have text_alloc() not with a 'where'
> > argument but with a 'why' argument. Or more simply, just have separate
> > alloc/free APIs for each case, with generic versions that can be
> > overridden by the architecture.
>
> Well, there only seem to be 2 cases here, either the pointer needs to
> fit in some immediate displacement, or not.
>
> On x86 we seem have the advantage of a fairly large immediate
> displacement as compared to many other architectures (due to our
> variable sized instructions). And thus have been fairly liberal with our
> usage of it (also our indirect jmps/calls suck, double so with
> RETCH-POLINE).
>
> Still, the indirect jump, as mentioned by Russel should work for
> arbitrarily placed code for us too.
>
>
> So I'm thinking that something like:
>
> enum ptr_type {
> immediate_displacement,
> absolute,
> };
>
> void *text_alloc(unsigned long size, enum ptr_type type)
> {
> unsigned long vstart = VMALLOC_START;
> unsigned long vend = VMALLOC_END;
>
> if (type == immediate_displacement) {
> vstart = MODULES_VADDR;
> vend = MODULES_END;
> }
>
> return __vmalloc_node_range(size, TEXT_ALIGN, vstart, vend,
> GFP_KERNEL, PAGE_KERNEL_EXEC, 0,
> NUMA_NO_NODE, _RET_IP_);
> }
>
> void text_free(void *ptr)
> {
> vfree(ptr);
> }
>
> Should work for all cases. Yes, we might then want something like a per
> arch:
>
> {BPF,FTRACE,KPROBE}_TEXT_TYPE
>
> to help with text_alloc() usage in generic code, but I think
> fundamentally, there's only these two options.
There is one arch (nios2), which uses a regular kzalloc(). This means
that both text_alloc() and text_memfree() need to be weaks symbols and
nios2 needs to have overriding text.c to do its thing.
/Jarkko
^ permalink raw reply
* Re: [PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
From: Steven Rostedt @ 2020-07-14 15:44 UTC (permalink / raw)
To: Jessica Yu
Cc: Catalin Marinas, Kefeng Wang, Paul Mackerras, Zong Li, Andi Kleen,
Paul Burton, Vincent Whitchurch, Petr Mladek, Brian Gerst,
Andy Lutomirski, Thomas Gleixner, Jiri Kosina, Anup Patel,
linux-kernel, Philipp Rudo, Torsten Duwe, Masami Hiramatsu,
Andrew Morton, Mark Rutland, James E.J. Bottomley, Vincent Chen,
Omar Sandoval, open list:S390, Joe Lawrence, Helge Deller,
John Fastabend, Anil S Keshavamurthy, Yonghong Song, Iurii Zaikin,
Andrii Nakryiko, Thomas Huth, Vasily Gorbik,
moderated list:ARM PORT, Daniel Axtens, Damien Le Moal,
Martin KaFai Lau, Song Liu, Paul Walmsley, Heiko Carstens,
Alexei Starovoitov, Jarkko Sakkinen, Atish Patra, Will Deacon,
Daniel Borkmann, Masahiro Yamada, Nayna Jain, Ley Foon Tan,
Christian Borntraeger, Sami Tolvanen, Naveen N. Rao, Mao Han,
Marco Elver, Babu Moger, Borislav Petkov, Greentime Hu, Ben Dooks,
Guan Xuetao, Thomas Bogendoerfer, open list:PARISC ARCHITECTURE,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), David S. Miller,
Thiago Jung Bauermann, Peter Zijlstra, David Howells,
open list:SPARC + UltraSPARC (sparc/sparc64), Sandipan Das,
H. Peter Anvin, Amit Daniel Kachhap, Tiezhu Yang, Miroslav Benes,
Jiri Olsa, Ard Biesheuvel, Vincenzo Frascino, Anders Roxell,
Sven Schnelle, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
Russell King, open list:RISC-V ARCHITECTURE, Mike Rapoport,
Ingo Molnar, Albert Ou, Paul E. McKenney, Josh Poimboeuf,
KP Singh, Dmitry Vyukov, Nick Hu,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), open list:MIPS,
Palmer Dabbelt, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)
In-Reply-To: <20200714135651.GA27819@linux-8ccs>
On Tue, 14 Jul 2020 15:56:52 +0200
Jessica Yu <jeyu@kernel.org> wrote:
> As Ard and Will have already explained, the major issue I'm having
> with this is that we're taking module_alloc(), an allocator that was
> originally specific to module loading, and turning it into a generic
> interface to be used by other subsystems. You're pulling in all the
> module loading semantics that vary by architecture and expecting it to
> work as a generic text allocator. I'm not against the existence of a
> generic text_alloc() but I would very much rather that module_alloc()
> be left alone to the module loader and instead work on introducing a
> *separate* generic text_alloc() interface that would work for its
> intended users (kprobes, bpf, etc) and have existing users of
> module_alloc() switch to that instead.
Looks like the consensus is to create a separate text_alloc() that can
be used when modules are not configured in for things like ftrace
dynamic trampolines, and keep module_alloc() untouched.
For those concerned about added unused code for architectures that
don't need text_alloc(), we can always create a config called:
CONFIG_ARCH_NEED_TEXT_ALLOC, and the arch can add that to its list of
kconfigs if it intends to use text_alloc().
-- Steve
^ permalink raw reply
* Re: [PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
From: Steven Rostedt @ 2020-07-14 14:03 UTC (permalink / raw)
To: Mark Rutland
Cc: Catalin Marinas, Kefeng Wang, Paul Mackerras, Zong Li, Andi Kleen,
Paul Burton, Vincent Whitchurch, Petr Mladek, Brian Gerst,
Andy Lutomirski, Thomas Gleixner, Jiri Kosina, Anup Patel,
Linux Kernel Mailing List, Philipp Rudo, Torsten Duwe,
Masami Hiramatsu, Andrew Morton, James E.J. Bottomley,
Vincent Chen, Omar Sandoval, open list:S390, Joe Lawrence,
Helge Deller, John Fastabend, Anil S Keshavamurthy, Yonghong Song,
Iurii Zaikin, Andrii Nakryiko, Thomas Huth, Vasily Gorbik,
moderated list:ARM PORT, Daniel Axtens, Damien Le Moal,
Martin KaFai Lau, Song Liu, Paul Walmsley, Heiko Carstens,
Alexei Starovoitov, Jarkko Sakkinen, Atish Patra, Will Deacon,
Daniel Borkmann, Masahiro Yamada, Nayna Jain, Ley Foon Tan,
Christian Borntraeger, Sami Tolvanen, Naveen N. Rao, Mao Han,
Marco Elver, Babu Moger, Borislav Petkov, Greentime Hu, Ben Dooks,
Guan Xuetao, Thomas Bogendoerfer, open list:PARISC ARCHITECTURE,
Jessica Yu, open list:BPF JIT for MIPS (32-BIT AND 64-BIT),
David S. Miller, Thiago Jung Bauermann, Peter Zijlstra,
David Howells, open list:SPARC + UltraSPARC (sparc/sparc64),
Sandipan Das, H. Peter Anvin, Amit Daniel Kachhap, Tiezhu Yang,
Miroslav Benes, Jiri Olsa, Ard Biesheuvel, Vincenzo Frascino,
Anders Roxell, Sven Schnelle,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Russell King,
open list:RISC-V ARCHITECTURE, Mike Rapoport, Ingo Molnar,
Albert Ou, Paul E. McKenney, Josh Poimboeuf, KP Singh,
Dmitry Vyukov, Nick Hu,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), open list:MIPS,
Palmer Dabbelt, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)
In-Reply-To: <20200714133314.GA67386@C02TD0UTHF1T.local>
On Tue, 14 Jul 2020 14:33:14 +0100
Mark Rutland <mark.rutland@arm.com> wrote:
> > Should work for all cases. Yes, we might then want something like a per
> > arch:
> >
> > {BPF,FTRACE,KPROBE}_TEXT_TYPE
>
> ... at that point why not:
>
> text_alloc_ftrace();
> text_alloc_module();
> text_alloc_bpf();
> text_alloc_kprobe();
I don't know about bpf and kprobes, but for ftrace, the only place that
it allocates text happens to be in arch specific code.
If you want something special for ftrace, you could just add your own
function. But for x86, a text_alloc_immediate() would work.
(BTW, I like the function names over the enums)
-- Steve
^ permalink raw reply
* Re: [PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
From: Jessica Yu @ 2020-07-14 13:56 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Catalin Marinas, Kefeng Wang, Paul Mackerras, Zong Li, Andi Kleen,
Paul Burton, Vincent Whitchurch, Petr Mladek, Brian Gerst,
Andy Lutomirski, Thomas Gleixner, Jiri Kosina, Anup Patel,
linux-kernel, Philipp Rudo, Torsten Duwe, Masami Hiramatsu,
Andrew Morton, Mark Rutland, James E.J. Bottomley, Vincent Chen,
Omar Sandoval, open list:S390, Joe Lawrence, Helge Deller,
John Fastabend, Anil S Keshavamurthy, Yonghong Song, Iurii Zaikin,
Andrii Nakryiko, Thomas Huth, Vasily Gorbik,
moderated list:ARM PORT, Daniel Axtens, Damien Le Moal,
Martin KaFai Lau, Song Liu, Paul Walmsley, Heiko Carstens,
Alexei Starovoitov, Atish Patra, Will Deacon, Daniel Borkmann,
Masahiro Yamada, Nayna Jain, Ley Foon Tan, Christian Borntraeger,
Sami Tolvanen, Naveen N. Rao, Mao Han, Marco Elver,
Steven Rostedt, Babu Moger, Borislav Petkov, Greentime Hu,
Ben Dooks, Guan Xuetao, Thomas Bogendoerfer,
open list:PARISC ARCHITECTURE,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), David S. Miller,
Thiago Jung Bauermann, Peter Zijlstra, David Howells,
open list:SPARC + UltraSPARC (sparc/sparc64), Sandipan Das,
H. Peter Anvin, Amit Daniel Kachhap, Tiezhu Yang, Miroslav Benes,
Jiri Olsa, Ard Biesheuvel, Vincenzo Frascino, Anders Roxell,
Sven Schnelle, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
Russell King, open list:RISC-V ARCHITECTURE, Mike Rapoport,
Ingo Molnar, Albert Ou, Paul E. McKenney, Josh Poimboeuf,
KP Singh, Dmitry Vyukov, Nick Hu,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), open list:MIPS,
Palmer Dabbelt, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)
In-Reply-To: <20200714094625.1443261-2-jarkko.sakkinen@linux.intel.com>
+++ Jarkko Sakkinen [14/07/20 12:45 +0300]:
>Rename module_alloc() to text_alloc() and module_memfree() to
>text_memfree(), and move them to kernel/text.c, which is unconditionally
>compiled to the kernel proper. This allows kprobes, ftrace and bpf to
>allocate space for executable code without requiring to compile the modules
>support (CONFIG_MODULES=y) in.
>
>Cc: Andi Kleen <ak@linux.intel.com>
>Suggested-by: Peter Zijlstra <peterz@infradead.org>
>Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
As Ard and Will have already explained, the major issue I'm having
with this is that we're taking module_alloc(), an allocator that was
originally specific to module loading, and turning it into a generic
interface to be used by other subsystems. You're pulling in all the
module loading semantics that vary by architecture and expecting it to
work as a generic text allocator. I'm not against the existence of a
generic text_alloc() but I would very much rather that module_alloc()
be left alone to the module loader and instead work on introducing a
*separate* generic text_alloc() interface that would work for its
intended users (kprobes, bpf, etc) and have existing users of
module_alloc() switch to that instead.
Jessica
^ permalink raw reply
* Re: [PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
From: Ard Biesheuvel @ 2020-07-14 13:47 UTC (permalink / raw)
To: Mark Rutland
Cc: Catalin Marinas, Kefeng Wang, Paul Mackerras, Zong Li, Andi Kleen,
Paul Burton, Vincent Whitchurch, Petr Mladek, Brian Gerst,
Andy Lutomirski, Thomas Gleixner, Jiri Kosina, Anup Patel,
Linux Kernel Mailing List, Philipp Rudo, Torsten Duwe,
Masami Hiramatsu, Andrew Morton, James E.J. Bottomley,
Vincent Chen, Omar Sandoval, open list:S390, Joe Lawrence,
Helge Deller, John Fastabend, Anil S Keshavamurthy, Yonghong Song,
Iurii Zaikin, Andrii Nakryiko, Thomas Huth, Vasily Gorbik,
moderated list:ARM PORT, Daniel Axtens, Damien Le Moal,
Martin KaFai Lau, Song Liu, Paul Walmsley, Heiko Carstens,
Alexei Starovoitov, Jarkko Sakkinen, Atish Patra, Will Deacon,
Daniel Borkmann, Masahiro Yamada, Nayna Jain, Ley Foon Tan,
Christian Borntraeger, Sami Tolvanen, Naveen N. Rao, Mao Han,
Marco Elver, Steven Rostedt, Babu Moger, Borislav Petkov,
Greentime Hu, Ben Dooks, Guan Xuetao, Thomas Bogendoerfer,
open list:PARISC ARCHITECTURE, Jessica Yu,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), David S. Miller,
Thiago Jung Bauermann, Peter Zijlstra, David Howells,
open list:SPARC + UltraSPARC (sparc/sparc64), Sandipan Das,
H. Peter Anvin, Amit Daniel Kachhap, Tiezhu Yang, Miroslav Benes,
Jiri Olsa, open list:RISC-V ARCHITECTURE, Vincenzo Frascino,
Anders Roxell, Sven Schnelle,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Russell King,
Mike Rapoport, Ingo Molnar, Albert Ou, Paul E. McKenney,
Josh Poimboeuf, KP Singh, Dmitry Vyukov, Nick Hu,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), open list:MIPS,
Palmer Dabbelt, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)
In-Reply-To: <20200714133314.GA67386@C02TD0UTHF1T.local>
On Tue, 14 Jul 2020 at 16:33, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Tue, Jul 14, 2020 at 03:01:09PM +0200, Peter Zijlstra wrote:
> > On Tue, Jul 14, 2020 at 03:19:24PM +0300, Ard Biesheuvel wrote:
> > > So perhaps the answer is to have text_alloc() not with a 'where'
> > > argument but with a 'why' argument. Or more simply, just have separate
> > > alloc/free APIs for each case, with generic versions that can be
> > > overridden by the architecture.
> >
> > Well, there only seem to be 2 cases here, either the pointer needs to
> > fit in some immediate displacement, or not.
>
> On some arches you have a few choices for immediates depending on
> compiler options, e.g. on arm64:
>
> * +/- 128M with B
> * +/-4G with ADRP+ADD+BR
> * +/- 48/64 bits with a series of MOVK* + BR
>
> ... and you might build core kernel one way and modules another, and
> either could depend on configuration.
>
> > On x86 we seem have the advantage of a fairly large immediate
> > displacement as compared to many other architectures (due to our
> > variable sized instructions). And thus have been fairly liberal with our
> > usage of it (also our indirect jmps/calls suck, double so with
> > RETCH-POLINE).
> >
> > Still, the indirect jump, as mentioned by Russel should work for
> > arbitrarily placed code for us too.
> >
> >
> > So I'm thinking that something like:
> >
> > enum ptr_type {
> > immediate_displacement,
> > absolute,
> > };
> >
> > void *text_alloc(unsigned long size, enum ptr_type type)
> > {
> > unsigned long vstart = VMALLOC_START;
> > unsigned long vend = VMALLOC_END;
> >
> > if (type == immediate_displacement) {
> > vstart = MODULES_VADDR;
> > vend = MODULES_END;
> > }
> >
> > return __vmalloc_node_range(size, TEXT_ALIGN, vstart, vend,
> > GFP_KERNEL, PAGE_KERNEL_EXEC, 0,
> > NUMA_NO_NODE, _RET_IP_);
> > }
> >
> > void text_free(void *ptr)
> > {
> > vfree(ptr);
> > }
>
> I think it'd be easier to read with separate functions, e.g.
>
> text_alloc_imm_offset(unsigned long size);
> text_alloc_absolute(unsigned long size);
>
On arm64, we have a 128M window close to the core kernel for modules,
and a separate 128m window for bpf programs, which are kept in
relative branching range of each other, but could be far away from
kernel+modules, and so having 'close' and 'far' as the only
distinction is insufficient.
> > Should work for all cases. Yes, we might then want something like a per
> > arch:
> >
> > {BPF,FTRACE,KPROBE}_TEXT_TYPE
>
> ... at that point why not:
>
> text_alloc_ftrace();
> text_alloc_module();
> text_alloc_bpf();
> text_alloc_kprobe();
>
> ... etc which an arch can alias however it wants? e.g. x86 can have
> those all go to a common text_alloc_generic(), and that could even be a
> generic option for arches that don't care to distinguish these cases.
>
That is basically what i meant with separate alloc/free APIs, which i
think is the sanest approach here.
> Then if there are new places that want to allocate text we have to
> consider their requirements when adding them, too.
>
> Thanks,
> Mark.
^ permalink raw reply
* Re: [PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
From: Mark Rutland @ 2020-07-14 13:33 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Catalin Marinas, Kefeng Wang, Paul Mackerras, Zong Li, Andi Kleen,
Paul Burton, Vincent Whitchurch, Petr Mladek, Brian Gerst,
Andy Lutomirski, Thomas Gleixner, Jiri Kosina, Anup Patel,
Linux Kernel Mailing List, Philipp Rudo, Torsten Duwe,
Masami Hiramatsu, Andrew Morton, James E.J. Bottomley,
Vincent Chen, Omar Sandoval, open list:S390, Joe Lawrence,
Helge Deller, John Fastabend, Anil S Keshavamurthy, Yonghong Song,
Iurii Zaikin, Andrii Nakryiko, Thomas Huth, Vasily Gorbik,
moderated list:ARM PORT, Daniel Axtens, Damien Le Moal,
Martin KaFai Lau, Song Liu, Paul Walmsley, Heiko Carstens,
Alexei Starovoitov, Jarkko Sakkinen, Atish Patra, Will Deacon,
Daniel Borkmann, Masahiro Yamada, Nayna Jain, Ley Foon Tan,
Christian Borntraeger, Sami Tolvanen, Naveen N. Rao, Mao Han,
Marco Elver, Steven Rostedt, Babu Moger, Borislav Petkov,
Greentime Hu, Ben Dooks, Guan Xuetao, Thomas Bogendoerfer,
open list:PARISC ARCHITECTURE, Jessica Yu,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), David S. Miller,
Thiago Jung Bauermann, David Howells,
open list:SPARC + UltraSPARC (sparc/sparc64), Sandipan Das,
H. Peter Anvin, Amit Daniel Kachhap, Tiezhu Yang, Miroslav Benes,
Jiri Olsa, Ard Biesheuvel, Vincenzo Frascino, Anders Roxell,
Sven Schnelle, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
Russell King, open list:RISC-V ARCHITECTURE, Mike Rapoport,
Ingo Molnar, Albert Ou, Paul E. McKenney, Josh Poimboeuf,
KP Singh, Dmitry Vyukov, Nick Hu,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), open list:MIPS,
Palmer Dabbelt, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)
In-Reply-To: <20200714130109.GX10769@hirez.programming.kicks-ass.net>
On Tue, Jul 14, 2020 at 03:01:09PM +0200, Peter Zijlstra wrote:
> On Tue, Jul 14, 2020 at 03:19:24PM +0300, Ard Biesheuvel wrote:
> > So perhaps the answer is to have text_alloc() not with a 'where'
> > argument but with a 'why' argument. Or more simply, just have separate
> > alloc/free APIs for each case, with generic versions that can be
> > overridden by the architecture.
>
> Well, there only seem to be 2 cases here, either the pointer needs to
> fit in some immediate displacement, or not.
On some arches you have a few choices for immediates depending on
compiler options, e.g. on arm64:
* +/- 128M with B
* +/-4G with ADRP+ADD+BR
* +/- 48/64 bits with a series of MOVK* + BR
... and you might build core kernel one way and modules another, and
either could depend on configuration.
> On x86 we seem have the advantage of a fairly large immediate
> displacement as compared to many other architectures (due to our
> variable sized instructions). And thus have been fairly liberal with our
> usage of it (also our indirect jmps/calls suck, double so with
> RETCH-POLINE).
>
> Still, the indirect jump, as mentioned by Russel should work for
> arbitrarily placed code for us too.
>
>
> So I'm thinking that something like:
>
> enum ptr_type {
> immediate_displacement,
> absolute,
> };
>
> void *text_alloc(unsigned long size, enum ptr_type type)
> {
> unsigned long vstart = VMALLOC_START;
> unsigned long vend = VMALLOC_END;
>
> if (type == immediate_displacement) {
> vstart = MODULES_VADDR;
> vend = MODULES_END;
> }
>
> return __vmalloc_node_range(size, TEXT_ALIGN, vstart, vend,
> GFP_KERNEL, PAGE_KERNEL_EXEC, 0,
> NUMA_NO_NODE, _RET_IP_);
> }
>
> void text_free(void *ptr)
> {
> vfree(ptr);
> }
I think it'd be easier to read with separate functions, e.g.
text_alloc_imm_offset(unsigned long size);
text_alloc_absolute(unsigned long size);
> Should work for all cases. Yes, we might then want something like a per
> arch:
>
> {BPF,FTRACE,KPROBE}_TEXT_TYPE
... at that point why not:
text_alloc_ftrace();
text_alloc_module();
text_alloc_bpf();
text_alloc_kprobe();
... etc which an arch can alias however it wants? e.g. x86 can have
those all go to a common text_alloc_generic(), and that could even be a
generic option for arches that don't care to distinguish these cases.
Then if there are new places that want to allocate text we have to
consider their requirements when adding them, too.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
From: Peter Zijlstra @ 2020-07-14 13:01 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Catalin Marinas, Kefeng Wang, Paul Mackerras, Zong Li, Andi Kleen,
Paul Burton, Vincent Whitchurch, Petr Mladek, Brian Gerst,
Andy Lutomirski, Thomas Gleixner, Jiri Kosina, Anup Patel,
Linux Kernel Mailing List, Philipp Rudo, Torsten Duwe,
Masami Hiramatsu, Andrew Morton, Mark Rutland,
James E.J. Bottomley, Vincent Chen, Omar Sandoval, open list:S390,
Joe Lawrence, Helge Deller, John Fastabend, Anil S Keshavamurthy,
Yonghong Song, Iurii Zaikin, Andrii Nakryiko, Thomas Huth,
Vasily Gorbik, moderated list:ARM PORT, Daniel Axtens,
Damien Le Moal, Martin KaFai Lau, Song Liu, Paul Walmsley,
Heiko Carstens, Alexei Starovoitov, Jarkko Sakkinen, Atish Patra,
Will Deacon, Daniel Borkmann, Masahiro Yamada, Nayna Jain,
Ley Foon Tan, Christian Borntraeger, Sami Tolvanen, Naveen N. Rao,
Mao Han, Marco Elver, Steven Rostedt, Babu Moger, Borislav Petkov,
Greentime Hu, Ben Dooks, Guan Xuetao, Thomas Bogendoerfer,
open list:PARISC ARCHITECTURE, Jessica Yu,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), David S. Miller,
Thiago Jung Bauermann, David Howells,
open list:SPARC + UltraSPARC (sparc/sparc64), Sandipan Das,
H. Peter Anvin, Amit Daniel Kachhap, Tiezhu Yang, Miroslav Benes,
Jiri Olsa, open list:RISC-V ARCHITECTURE, Vincenzo Frascino,
Anders Roxell, Sven Schnelle,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Russell King,
Mike Rapoport, Ingo Molnar, Albert Ou, Paul E. McKenney,
Josh Poimboeuf, KP Singh, Dmitry Vyukov, Nick Hu,
open list:BPF JIT for MIPS (32-BIT AND 64-BIT), open list:MIPS,
Palmer Dabbelt, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)
In-Reply-To: <CAMj1kXGG4vxWrp1de1FxdU=8F4Jof00=T1x-7e+BW7_HP-oZMQ@mail.gmail.com>
On Tue, Jul 14, 2020 at 03:19:24PM +0300, Ard Biesheuvel wrote:
> So perhaps the answer is to have text_alloc() not with a 'where'
> argument but with a 'why' argument. Or more simply, just have separate
> alloc/free APIs for each case, with generic versions that can be
> overridden by the architecture.
Well, there only seem to be 2 cases here, either the pointer needs to
fit in some immediate displacement, or not.
On x86 we seem have the advantage of a fairly large immediate
displacement as compared to many other architectures (due to our
variable sized instructions). And thus have been fairly liberal with our
usage of it (also our indirect jmps/calls suck, double so with
RETCH-POLINE).
Still, the indirect jump, as mentioned by Russel should work for
arbitrarily placed code for us too.
So I'm thinking that something like:
enum ptr_type {
immediate_displacement,
absolute,
};
void *text_alloc(unsigned long size, enum ptr_type type)
{
unsigned long vstart = VMALLOC_START;
unsigned long vend = VMALLOC_END;
if (type == immediate_displacement) {
vstart = MODULES_VADDR;
vend = MODULES_END;
}
return __vmalloc_node_range(size, TEXT_ALIGN, vstart, vend,
GFP_KERNEL, PAGE_KERNEL_EXEC, 0,
NUMA_NO_NODE, _RET_IP_);
}
void text_free(void *ptr)
{
vfree(ptr);
}
Should work for all cases. Yes, we might then want something like a per
arch:
{BPF,FTRACE,KPROBE}_TEXT_TYPE
to help with text_alloc() usage in generic code, but I think
fundamentally, there's only these two options.
^ permalink raw reply
* Re: [RFC PATCH 00/35] Move all PCIBIOS* definitions into arch/x86
From: Bjorn Helgaas @ 2020-07-14 18:45 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Keith Busch, Paul Mackerras, sparclinux, Toan Le, Greg Ungerer,
Marek Vasut, Rob Herring, Lorenzo Pieralisi, Sagi Grimberg,
Russell King, Ley Foon Tan, Christoph Hellwig, Geert Uytterhoeven,
Kevin Hilman, linux-pci, Jakub Kicinski, Matt Turner,
linux-kernel-mentees, Guenter Roeck, Ray Jui, Jens Axboe,
Ivan Kokshaysky, Shuah Khan, bjorn, Boris Ostrovsky,
Richard Henderson, Juergen Gross, Bjorn Helgaas,
Thomas Bogendoerfer, Scott Branden, Jingoo Han,
Saheed O. Bolarinwa, linux-kernel@vger.kernel.org, Philipp Zabel,
Greg Kroah-Hartman, Gustavo Pimentel, linuxppc-dev,
David S. Miller, Heiner Kallweit
In-Reply-To: <CAK8P3a3NWSZw6678k1O2eJ6-c5GuW7484PRvEzU9MEPPrCD-yw@mail.gmail.com>
[trimmed the cc list; it's still too large but maybe arch folks care]
On Mon, Jul 13, 2020 at 05:08:10PM +0200, Arnd Bergmann wrote:
> On Mon, Jul 13, 2020 at 3:22 PM Saheed O. Bolarinwa
> <refactormyself@gmail.com> wrote:
> > This goal of these series is to move the definition of *all*
> > PCIBIOS* from include/linux/pci.h to arch/x86 and limit their use
> > within there. All other tree specific definition will be left for
> > intact. Maybe they can be renamed.
> >
> > PCIBIOS* is an x86 concept as defined by the PCI spec. The
> > returned error codes of PCIBIOS* are positive values and this
> > introduces some complexities which other archs need not incur.
>
> I think the intention is good, but I find the series in its current
> form very hard to review, in particular the way you touch some
> functions three times with trivial changes. Instead of
>
> 1) replace PCIBIOS_SUCCESSFUL with 0
> 2) drop pointless 0-comparison
> 3) reformat whitespace
>
> I would suggest to combine the first two steps into one patch per
> subsystem and drop the third step.
I agree. BUT please don't just run out and post new patches to do
this. Let's talk about Arnd's further ideas below first.
> ...
> Maybe the work can be split up differently, with a similar end
> result but fewer and easier reviewed patches. The way I'd look at
> the problem, there are three main areas that can be dealt with one
> at a time:
>
> a) callers of the high-level config space accessors
> pci_{write,read}_config_{byte,word,dword}, mostly in device
> drivers.
> b) low-level implementation of the config space accessors
> through struct pci_ops
> c) all other occurrences of these constants
>
> Starting with a), my first question is whether any high-level
> drivers even need to care about errors from these functions. I see
> 4913 callers that ignore the return code, and 576 that actually
> check it, and almost none care about the specific error (as you
> found as well). Unless we conclude that most PCI drivers are wrong,
> could we just change the return type to 'void' and assume they never
> fail for valid arguments on a valid pci_device* ?
I really like this idea.
pci_write_config_*() has one return value, and only 100ish of 2500
callers check for errors. It's sometimes possible for config
accessors to detect PCI errors and return failure, e.g., device was
removed or didn't respond, but most of them don't, and detecting these
errors is not really that valuable.
pci_read_config_*() is much more interesting because it returns two
things, the function return value and the value read from the PCI
device, and it's complicated to check both.
Again it's sometimes possible for config read accessors to detect PCI
errors, but in most cases a PCI error means the accessor returns
success and the value from PCI is ~0.
Checking the function return value catches programming errors (bad
alignment, etc) but misses most of the interesting errors (device was
unplugged or reported a PCI error).
Checking the value returned from PCI is tricky because ~0 is a valid
value for some config registers, and only the driver knows for sure.
If the driver knows that ~0 is a possible value, it would have to do
something else, e.g., another config read of a register that *cannot*
be ~0, to see whether it's really an error.
I suspect that if we had a single value to look at it would be easier
to get right. Error checking with current interface would look like
this:
err = pci_read_config_word(dev, addr, &val);
if (err)
return -EINVAL;
if (PCI_POSSIBLE_ERROR(val)) {
/* if driver knows ~0 is invalid */
return -EINVAL;
/* if ~0 is potentially a valid value */
err = pci_read_config_word(dev, PCI_VENDOR_ID, &val2);
if (err)
return -EINVAL;
if (PCI_POSSIBLE_ERROR(val2))
return -EINVAL;
}
Error checking with a possible interface that returned only a single
value could look like this:
val = pci_config_read_word(dev, addr);
if (PCI_POSSIBLE_ERROR(val)) {
/* if driver knows ~0 is invalid */
return -EINVAL;
/* if ~0 is potentially a valid value */
val2 = pci_config_read_word(dev, PCI_VENDOR_ID);
if (PCI_POSSIBLE_ERROR(val2))
return -EINVAL;
}
Am I understanding you correctly?
> For b), it might be nice to also change other aspects of the
> interface, e.g. passing a pci_host_bridge pointer plus bus number
> instead of a pci_bus pointer, or having the callback in the
> pci_host_bridge structure.
I like this idea a lot, too. I think the fact that
pci_bus_read_config_word() requires a pci_bus * complicates things in
a few places.
I think it's completely separate, as you say, and we should defer it
for now because even part a) is a lot of work. I added it to my list
of possible future projects.
Bjorn
^ 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