* Re: [RESEND][PATCH 0/7] Avoid overflow at boundary_size
From: Christoph Hellwig @ 2020-09-01 7:36 UTC (permalink / raw)
To: Nicolin Chen
Cc: linux-ia64, James.Bottomley, paulus, hpa, sparclinux, hch, sfr,
deller, x86, borntraeger, mingo, mattst88, fenghua.yu, gor,
schnelle, hca, ink, tglx, gerald.schaefer, rth, tony.luck,
linux-parisc, linux-s390, linux-kernel, linux-alpha, bp,
linuxppc-dev, davem
In-Reply-To: <20200831203811.8494-1-nicoleotsuka@gmail.com>
I really don't like all the open coded smarts in the various drivers.
What do you think about a helper like the one in the untested patch
below (on top of your series). Also please include the original
segment boundary patch with the next resend so that the series has
the full context.
diff --git a/arch/alpha/kernel/pci_iommu.c b/arch/alpha/kernel/pci_iommu.c
index 1ef2c647bd3ec2..6f7de4f4e191e7 100644
--- a/arch/alpha/kernel/pci_iommu.c
+++ b/arch/alpha/kernel/pci_iommu.c
@@ -141,10 +141,7 @@ iommu_arena_find_pages(struct device *dev, struct pci_iommu_arena *arena,
unsigned long boundary_size;
base = arena->dma_base >> PAGE_SHIFT;
-
- boundary_size = dev ? dma_get_seg_boundary(dev) : U32_MAX;
- /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
- boundary_size = (boundary_size >> PAGE_SHIFT) + 1;
+ boundary_size = dma_get_seg_boundary_nr_pages(dev, PAGE_SHIFT);
/* Search forward for the first mask-aligned sequence of N free ptes */
ptes = arena->ptes;
diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c
index 945954903bb0ba..b49b73a95067d2 100644
--- a/arch/ia64/hp/common/sba_iommu.c
+++ b/arch/ia64/hp/common/sba_iommu.c
@@ -485,8 +485,7 @@ sba_search_bitmap(struct ioc *ioc, struct device *dev,
ASSERT(((unsigned long) ioc->res_hint & (sizeof(unsigned long) - 1UL)) == 0);
ASSERT(res_ptr < res_end);
- /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
- boundary_size = (dma_get_seg_boundary(dev) >> iovp_shift) + 1;
+ boundary_size = dma_get_seg_boundary_nr_pages(dev, iovp_shift);
BUG_ON(ioc->ibase & ~iovp_mask);
shift = ioc->ibase >> iovp_shift;
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index c01ccbf8afdd42..cbc2e62db597cf 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -236,11 +236,7 @@ static unsigned long iommu_range_alloc(struct device *dev,
}
}
- /* 4GB boundary for iseries_hv_alloc and iseries_hv_map */
- boundary_size = dev ? dma_get_seg_boundary(dev) : U32_MAX;
-
- /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
- boundary_size = (boundary_size >> tbl->it_page_shift) + 1;
+ boundary_size = dma_get_seg_boundary_nr_pages(dev, tbl->it_page_shift);
n = iommu_area_alloc(tbl->it_map, limit, start, npages, tbl->it_offset,
boundary_size, align_mask);
diff --git a/arch/s390/pci/pci_dma.c b/arch/s390/pci/pci_dma.c
index ecb067acc6d532..4a37d8f4de9d9d 100644
--- a/arch/s390/pci/pci_dma.c
+++ b/arch/s390/pci/pci_dma.c
@@ -261,13 +261,11 @@ static unsigned long __dma_alloc_iommu(struct device *dev,
unsigned long start, int size)
{
struct zpci_dev *zdev = to_zpci(to_pci_dev(dev));
- unsigned long boundary_size;
- /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
- boundary_size = (dma_get_seg_boundary(dev) >> PAGE_SHIFT) + 1;
return iommu_area_alloc(zdev->iommu_bitmap, zdev->iommu_pages,
start, size, zdev->start_dma >> PAGE_SHIFT,
- boundary_size, 0);
+ dma_get_seg_boundary_nr_pages(dev, PAGE_SHIFT),
+ 0);
}
static dma_addr_t dma_alloc_address(struct device *dev, int size)
diff --git a/arch/sparc/kernel/iommu-common.c b/arch/sparc/kernel/iommu-common.c
index 843e71894d0482..e6139c99762e11 100644
--- a/arch/sparc/kernel/iommu-common.c
+++ b/arch/sparc/kernel/iommu-common.c
@@ -166,10 +166,6 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
}
}
- boundary_size = dev ? dma_get_seg_boundary(dev) : U32_MAX;
-
- /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
- boundary_size = (boundary_size >> iommu->table_shift) + 1;
/*
* if the skip_span_boundary_check had been set during init, we set
* things up so that iommu_is_span_boundary() merely checks if the
@@ -178,7 +174,11 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
if ((iommu->flags & IOMMU_NO_SPAN_BOUND) != 0) {
shift = 0;
boundary_size = iommu->poolsize * iommu->nr_pools;
+ } else {
+ boundary_size = dma_get_seg_boundary_nr_pages(dev,
+ iommu->table_shift);
}
+
n = iommu_area_alloc(iommu->map, limit, start, npages, shift,
boundary_size, align_mask);
if (n == -1) {
diff --git a/arch/sparc/kernel/iommu.c b/arch/sparc/kernel/iommu.c
index d981c37305ae31..c3e4e2df26a8b8 100644
--- a/arch/sparc/kernel/iommu.c
+++ b/arch/sparc/kernel/iommu.c
@@ -472,8 +472,7 @@ static int dma_4u_map_sg(struct device *dev, struct scatterlist *sglist,
outs->dma_length = 0;
max_seg_size = dma_get_max_seg_size(dev);
- /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
- seg_boundary_size = (dma_get_seg_boundary(dev) >> IO_PAGE_SHIFT) + 1;
+ seg_boundary_size = dma_get_seg_boundary_nr_pages(dev, IO_PAGE_SHIFT);
base_shift = iommu->tbl.table_map_base >> IO_PAGE_SHIFT;
for_each_sg(sglist, s, nelems, i) {
unsigned long paddr, npages, entry, out_entry = 0, slen;
diff --git a/arch/sparc/kernel/pci_sun4v.c b/arch/sparc/kernel/pci_sun4v.c
index 233fe8a20cec33..6b92dd51c0026f 100644
--- a/arch/sparc/kernel/pci_sun4v.c
+++ b/arch/sparc/kernel/pci_sun4v.c
@@ -508,8 +508,7 @@ static int dma_4v_map_sg(struct device *dev, struct scatterlist *sglist,
iommu_batch_start(dev, prot, ~0UL);
max_seg_size = dma_get_max_seg_size(dev);
- /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
- seg_boundary_size = (dma_get_seg_boundary(dev) >> IO_PAGE_SHIFT) + 1;
+ seg_boundary_size = dma_get_seg_boundary_nr_pages(dev, IO_PAGE_SHIFT);
mask = *dev->dma_mask;
if (!iommu_use_atu(iommu, mask))
diff --git a/arch/x86/kernel/amd_gart_64.c b/arch/x86/kernel/amd_gart_64.c
index 7fa0bb490065a1..bccc5357bffd6c 100644
--- a/arch/x86/kernel/amd_gart_64.c
+++ b/arch/x86/kernel/amd_gart_64.c
@@ -96,8 +96,7 @@ static unsigned long alloc_iommu(struct device *dev, int size,
base_index = ALIGN(iommu_bus_base & dma_get_seg_boundary(dev),
PAGE_SIZE) >> PAGE_SHIFT;
- /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
- boundary_size = (dma_get_seg_boundary(dev) >> PAGE_SHIFT) + 1;
+ boundary_size = dma_get_seg_boundary_nr_pages(dev, PAGE_SHIFT);
spin_lock_irqsave(&iommu_bitmap_lock, flags);
offset = iommu_area_alloc(iommu_gart_bitmap, iommu_pages, next_bit,
diff --git a/drivers/parisc/ccio-dma.c b/drivers/parisc/ccio-dma.c
index c667d6aba7646e..ba16b7f8f80612 100644
--- a/drivers/parisc/ccio-dma.c
+++ b/drivers/parisc/ccio-dma.c
@@ -356,8 +356,7 @@ ccio_alloc_range(struct ioc *ioc, struct device *dev, size_t size)
** ggg sacrifices another 710 to the computer gods.
*/
- /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
- boundary_size = (dma_get_seg_boundary(dev) >> IOVP_SHIFT) + 1;
+ boundary_size = dma_get_seg_boundary_nr_pages(dev, IOVP_SHIFT);
if (pages_needed <= 8) {
/*
diff --git a/drivers/parisc/sba_iommu.c b/drivers/parisc/sba_iommu.c
index 96bc2c617cbd19..959bda193b9603 100644
--- a/drivers/parisc/sba_iommu.c
+++ b/drivers/parisc/sba_iommu.c
@@ -342,8 +342,7 @@ sba_search_bitmap(struct ioc *ioc, struct device *dev,
unsigned long shift;
int ret;
- /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
- boundary_size = (dma_get_seg_boundary(dev) >> IOVP_SHIFT) + 1;
+ boundary_size = dma_get_seg_boundary_nr_pages(dev, IOVP_SHIFT);
#if defined(ZX1_SUPPORT)
BUG_ON(ioc->ibase & ~IOVP_MASK);
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 52635e91143b25..7477a164500adb 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -632,6 +632,25 @@ static inline unsigned long dma_get_seg_boundary(struct device *dev)
return DMA_BIT_MASK(32);
}
+/**
+ * dma_get_seg_boundary_nr_pages - return the segment boundary in "page" units
+ * @dev: device to guery the boundary for
+ * @page_shift: ilog() of the the IOMMU page size
+ *
+ * Return the segment boundary in IOMMU page units (which may be different from
+ * the CPU page size) for the passed in device.
+ *
+ * If @dev is NULL a boundary of U32_MAX is assumed, this case is just for
+ * non-DMA API callers.
+ */
+static inline unsigned long dma_get_seg_boundary_nr_pages(struct device *dev,
+ unsigned int page_shift)
+{
+ if (!dev)
+ return (U32_MAX >> page_shift) + 1;
+ return (dma_get_seg_boundary(dev) >> page_shift) + 1;
+}
+
static inline int dma_set_seg_boundary(struct device *dev, unsigned long mask)
{
if (dev->dma_parms) {
^ permalink raw reply related
* Re: [PATCH v3 03/13] mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value
From: Aneesh Kumar K.V @ 2020-09-01 7:36 UTC (permalink / raw)
To: Anshuman Khandual, linux-mm, akpm
Cc: linux-arch, linux-s390, Christophe Leroy, x86, Mike Rapoport,
Qian Cai, Gerald Schaefer, Vineet Gupta, linux-snps-arc,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <75771782-734b-69f6-4a07-2d3542458319@arm.com>
On 9/1/20 1:02 PM, Anshuman Khandual wrote:
>
>
> On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote:
>> On 9/1/20 8:45 AM, Anshuman Khandual wrote:
>>>
>>>
>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>>> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
>>>> random value.
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>> ---
>>>> mm/debug_vm_pgtable.c | 13 ++++++++++---
>>>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>> index 086309fb9b6f..bbf9df0e64c6 100644
>>>> --- a/mm/debug_vm_pgtable.c
>>>> +++ b/mm/debug_vm_pgtable.c
>>>> @@ -44,10 +44,17 @@
>>>> * entry type. But these bits might affect the ability to clear entries with
>>>> * pxx_clear() because of how dynamic page table folding works on s390. So
>>>> * while loading up the entries do not change the lower 4 bits. It does not
>>>> - * have affect any other platform.
>>>> + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is
>>>> + * used to mark a pte entry.
>>>> */
>>>> -#define S390_MASK_BITS 4
>>>> -#define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS)
>>>> +#define S390_SKIP_MASK GENMASK(3, 0)
>>>> +#ifdef CONFIG_PPC_BOOK3S_64
>>>> +#define PPC64_SKIP_MASK GENMASK(62, 62)
>>>> +#else
>>>> +#define PPC64_SKIP_MASK 0x0
>>>> +#endif
>>>
>>> Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
>>> bits for a s390 platform requirement and can also do so for ppc64 as well. As
>>> mentioned before, please avoid adding any platform specific constructs in the
>>> test.
>>>
>>
>>
>> that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux
>
> Could not (#if __BITS_PER_LONG == 32) be used instead or something like
> that. But should be a generic conditional check identifying 32 bit arch
> not anything platform specific.
>
that _PAGE_PTE bit is pretty much specific to PPC BOOK3S_64. Not sure
why other architectures need to bothered about ignoring bit 62.
-aneesh
^ permalink raw reply
* Re: fsl_espi errors on v5.7.15
From: Heiner Kallweit @ 2020-09-01 7:38 UTC (permalink / raw)
To: Chris Packham, Nicholas Piggin, benh@kernel.crashing.org,
broonie@kernel.org, mpe@ellerman.id.au, paulus@samba.org
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
linux-spi@vger.kernel.org
In-Reply-To: <88a92234-450b-f559-d112-31d5c996c256@alliedtelesis.co.nz>
On 01.09.2020 04:52, Chris Packham wrote:
>
> On 1/09/20 12:33 am, Heiner Kallweit wrote:
>> On 30.08.2020 23:59, Chris Packham wrote:
>>> On 31/08/20 9:41 am, Heiner Kallweit wrote:
>>>> On 30.08.2020 23:00, Chris Packham wrote:
>>>>> On 31/08/20 12:30 am, Nicholas Piggin wrote:
>>>>>> Excerpts from Chris Packham's message of August 28, 2020 8:07 am:
>>>>> <snip>
>>>>>
>>>>>>>>>>> I've also now seen the RX FIFO not empty error on the T2080RDB
>>>>>>>>>>>
>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>>>>>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>>>>
>>>>>>>>>>> With my current workaround of emptying the RX FIFO. It seems
>>>>>>>>>>> survivable. Interestingly it only ever seems to be 1 extra byte in the
>>>>>>>>>>> RX FIFO and it seems to be after either a READ_SR or a READ_FSR.
>>>>>>>>>>>
>>>>>>>>>>> fsl_espi ffe110000.spi: tx 70
>>>>>>>>>>> fsl_espi ffe110000.spi: rx 03
>>>>>>>>>>> fsl_espi ffe110000.spi: Extra RX 00
>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>>>>>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>>>> fsl_espi ffe110000.spi: tx 05
>>>>>>>>>>> fsl_espi ffe110000.spi: rx 00
>>>>>>>>>>> fsl_espi ffe110000.spi: Extra RX 03
>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>>>>>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>>>> fsl_espi ffe110000.spi: tx 05
>>>>>>>>>>> fsl_espi ffe110000.spi: rx 00
>>>>>>>>>>> fsl_espi ffe110000.spi: Extra RX 03
>>>>>>>>>>>
>>>>>>>>>>> From all the Micron SPI-NOR datasheets I've got access to it is
>>>>>>>>>>> possible to continually read the SR/FSR. But I've no idea why it
>>>>>>>>>>> happens some times and not others.
>>>>>>>>>> So I think I've got a reproduction and I think I've bisected the problem
>>>>>>>>>> to commit 3282a3da25bd ("powerpc/64: Implement soft interrupt replay in
>>>>>>>>>> C"). My day is just finishing now so I haven't applied too much scrutiny
>>>>>>>>>> to this result. Given the various rabbit holes I've been down on this
>>>>>>>>>> issue already I'd take this information with a good degree of skepticism.
>>>>>>>>>>
>>>>>>>>> OK, so an easy test should be to re-test with a 5.4 kernel.
>>>>>>>>> It doesn't have yet the change you're referring to, and the fsl-espi driver
>>>>>>>>> is basically the same as in 5.7 (just two small changes in 5.7).
>>>>>>>> There's 6cc0c16d82f88 and maybe also other interrupt related patches
>>>>>>>> around this time that could affect book E, so it's good if that exact
>>>>>>>> patch is confirmed.
>>>>>>> My confirmation is basically that I can induce the issue in a 5.4 kernel
>>>>>>> by cherry-picking 3282a3da25bd. I'm also able to "fix" the issue in
>>>>>>> 5.9-rc2 by reverting that one commit.
>>>>>>>
>>>>>>> I both cases it's not exactly a clean cherry-pick/revert so I also
>>>>>>> confirmed the bisection result by building at 3282a3da25bd (which sees
>>>>>>> the issue) and the commit just before (which does not).
>>>>>> Thanks for testing, that confirms it well.
>>>>>>
>>>>>> [snip patch]
>>>>>>
>>>>>>> I still saw the issue with this change applied. PPC_IRQ_SOFT_MASK_DEBUG
>>>>>>> didn't report anything (either with or without the change above).
>>>>>> Okay, it was a bit of a shot in the dark. I still can't see what
>>>>>> else has changed.
>>>>>>
>>>>>> What would cause this, a lost interrupt? A spurious interrupt? Or
>>>>>> higher interrupt latency?
>>>>>>
>>>>>> I don't think the patch should cause significantly worse latency,
>>>>>> (it's supposed to be a bit better if anything because it doesn't set
>>>>>> up the full interrupt frame). But it's possible.
>>>>> My working theory is that the SPI_DON indication is all about the TX
>>>>> direction an now that the interrupts are faster we're hitting an error
>>>>> because there is still RX activity going on. Heiner disagrees with my
>>>>> interpretation of the SPI_DON indication and the fact that it doesn't
>>>>> happen every time does throw doubt on it.
>>>>>
>>>> It's right that the eSPI spec can be interpreted that SPI_DON refers to
>>>> TX only. However this wouldn't really make sense, because also for RX
>>>> we program the frame length, and therefore want to be notified once the
>>>> full frame was received. Also practical experience shows that SPI_DON
>>>> is set also after RX-only transfers.
>>>> Typical SPI NOR use case is that you write read command + start address,
>>>> followed by a longer read. If the TX-only interpretation would be right,
>>>> we'd always end up with SPI_DON not being set.
>>>>
>>>>> I can't really explain the extra RX byte in the fifo. We know how many
>>>>> bytes to expect and we pull that many from the fifo so it's not as if
>>>>> we're missing an interrupt causing us to skip the last byte. I've been
>>>>> looking for some kind of off-by-one calculation but again if it were
>>>>> something like that it'd happen all the time.
>>>>>
>>>> Maybe it helps to know what value this extra byte in the FIFO has. Is it:
>>>> - a duplicate of the last read byte
>>>> - or the next byte (at <end address> + 1)
>>>> - or a fixed value, e.g. always 0x00 or 0xff
>>> The values were up thread a bit but I'll repeat them here
>>>
>>> fsl_espi ffe110000.spi: tx 70
>>> fsl_espi ffe110000.spi: rx 03
>>> fsl_espi ffe110000.spi: Extra RX 00
>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>> fsl_espi ffe110000.spi: tx 05
>>> fsl_espi ffe110000.spi: rx 00
>>> fsl_espi ffe110000.spi: Extra RX 03
>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>> fsl_espi ffe110000.spi: tx 05
>>> fsl_espi ffe110000.spi: rx 00
>>> fsl_espi ffe110000.spi: Extra RX 03
>>>
>>>
>>> The rx 00 Extra RX 03 is a bit concerning. I've only ever seen them with
>>> either a READ_SR or a READ_FSR. Never a data read.
>>>
>> Just remembered something about SPIE_DON:
>> Transfers are always full duplex,
>
> That's not true in rxskip mode. Setting rxskip forces the eSPI into
> half-duplex mode. So far all the instances of "extra" rx bytes have been
> when rxskip == 1.
>
> I think that could be where our experience with SPIE_DON differ. In
> full-duplex mode yes DON is always set. In half duplex mode we can end
> up with DON set or not set depending on the interrupt timing.
>
Of course I'm not the chip designer, but I think under the hood rxskip
mode doesn't differ from normal full duplex. It just ignores the first
read bytes and sends dummy zero's once the read part starts.
And in normal SPI NOR use cases most transfers are rxskip:
send read command + read data
This can be simply tested / verified: send read command + read 31 bytes
That's less than the FIFO size, therefore only active interrupt source
is SPIE_DON. If your assumption is true the transfer would time out.
>> therefore in case of a read the chip
>> sends dummy zero's. Having said that in case of a read SPIE_DON means
>> that the last dummy zero was shifted out.
>>
>> READ_SR and READ_FSR are the shortest transfers, 1 byte out and 1 byte in.
>> So the issue may have a dependency on the length of the transfer.
>> However I see no good explanation so far. You can try adding a delay of
>> a few miroseconds between the following to commands in fsl_espi_bufs().
>>
>> fsl_espi_write_reg(espi, ESPI_SPIM, mask);
>>
>> /* Prevent filling the fifo from getting interrupted */
>> spin_lock_irq(&espi->lock);
>>
>> Maybe enabling interrupts and seeing the SPIE_DON interrupt are too close.
^ permalink raw reply
* Re: [PATCH v3 13/13] mm/debug_vm_pgtable: populate a pte entry before fetching it
From: Anshuman Khandual @ 2020-09-01 7:38 UTC (permalink / raw)
To: Aneesh Kumar K.V, linux-mm, akpm
Cc: linux-arch, linux-s390, Christophe Leroy, x86, Mike Rapoport,
Qian Cai, Gerald Schaefer, Vineet Gupta, linux-snps-arc,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <abef1791-8779-6b34-3178-3bf3ab36d42b@linux.ibm.com>
On 09/01/2020 12:07 PM, Aneesh Kumar K.V wrote:
> On 9/1/20 8:55 AM, Anshuman Khandual wrote:
>>
>>
>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>> pte_clear_tests operate on an existing pte entry. Make sure that is not a none
>>> pte entry.
>>>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> ---
>>> mm/debug_vm_pgtable.c | 6 ++++--
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>> index 21329c7d672f..8527ebb75f2c 100644
>>> --- a/mm/debug_vm_pgtable.c
>>> +++ b/mm/debug_vm_pgtable.c
>>> @@ -546,7 +546,7 @@ static void __init pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp,
>>> static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep,
>>> unsigned long vaddr)
>>> {
>>> - pte_t pte = ptep_get(ptep);
>>> + pte_t pte = ptep_get_and_clear(mm, vaddr, ptep);
>>
>> Seems like ptep_get_and_clear() here just clears the entry in preparation
>> for a following set_pte_at() which otherwise would have been a problem on
>> ppc64 as you had pointed out earlier i.e set_pte_at() should not update an
>> existing valid entry. So the commit message here is bit misleading.
>>
>
> and also fetch the pte value which is used further.
>
>
>>> pr_debug("Validating PTE clear\n");
>>> pte = __pte(pte_val(pte) | RANDOM_ORVALUE);
>>> @@ -944,7 +944,7 @@ static int __init debug_vm_pgtable(void)
>>> p4d_t *p4dp, *saved_p4dp;
>>> pud_t *pudp, *saved_pudp;
>>> pmd_t *pmdp, *saved_pmdp, pmd;
>>> - pte_t *ptep;
>>> + pte_t *ptep, pte;
>>> pgtable_t saved_ptep;
>>> pgprot_t prot, protnone;
>>> phys_addr_t paddr;
>>> @@ -1049,6 +1049,8 @@ static int __init debug_vm_pgtable(void)
>>> */
>>> ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
>>> + pte = pfn_pte(pte_aligned, prot);
>>> + set_pte_at(mm, vaddr, ptep, pte);
>>
>> Not here, creating and populating an entry must be done in respective
>> test functions itself. Besides, this seems bit redundant as well. The
>> test pte_clear_tests() with the above change added, already
>>
>> - Clears the PTEP entry with ptep_get_and_clear()
>
> and fetch the old value set previously.
In that case, please move above two lines i.e
pte = pfn_pte(pte_aligned, prot);
set_pte_at(mm, vaddr, ptep, pte);
from debug_vm_pgtable() to pte_clear_tests() and update it's arguments
as required.
^ permalink raw reply
* Re: [PATCH v3 08/13] mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP
From: Aneesh Kumar K.V @ 2020-09-01 7:40 UTC (permalink / raw)
To: Christophe Leroy, Anshuman Khandual, linux-mm, akpm
Cc: linux-arch, linux-s390, Christophe Leroy, x86, Mike Rapoport,
Qian Cai, Gerald Schaefer, Vineet Gupta, linux-snps-arc,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <d80a91c3-0edf-7e2f-8101-2d37a371f4bd@csgroup.eu>
On 9/1/20 12:20 PM, Christophe Leroy wrote:
>
>
> Le 01/09/2020 à 08:25, Aneesh Kumar K.V a écrit :
>> On 9/1/20 8:52 AM, Anshuman Khandual wrote:
>>>
>>>
>>>
>>> There is a checkpatch.pl warning here.
>>>
>>> WARNING: Possible unwrapped commit description (prefer a maximum 75
>>> chars per line)
>>> #7:
>>> Architectures like ppc64 use deposited page table while updating the
>>> huge pte
>>>
>>> total: 0 errors, 1 warnings, 40 lines checked
>>>
>>
>> I will ignore all these, because they are not really important IMHO.
>>
>
> When doing a git log in a 80 chars terminal window, having wrapping
> lines is not really convenient. It should be easy to avoid it.
>
We have been ignoring that for a long time isn't it?
For example ppc64 checkpatch already had
--max-line-length=90
There was also recent discussion whether 80 character limit is valid any
more. But I do keep it restricted to 80 character where ever it is
easy/make sense.
-aneesh
^ permalink raw reply
* Re: [PATCH v3 03/13] mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value
From: Anshuman Khandual @ 2020-09-01 7:46 UTC (permalink / raw)
To: Aneesh Kumar K.V, linux-mm, akpm
Cc: linux-arch, linux-s390, Christophe Leroy, x86, Mike Rapoport,
Qian Cai, Gerald Schaefer, Vineet Gupta, linux-snps-arc,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <e5d32d12-d904-ed8c-8963-d43d2c3744d9@linux.ibm.com>
On 09/01/2020 01:06 PM, Aneesh Kumar K.V wrote:
> On 9/1/20 1:02 PM, Anshuman Khandual wrote:
>>
>>
>> On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote:
>>> On 9/1/20 8:45 AM, Anshuman Khandual wrote:
>>>>
>>>>
>>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>>>> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
>>>>> random value.
>>>>>
>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>>> ---
>>>>> mm/debug_vm_pgtable.c | 13 ++++++++++---
>>>>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>>> index 086309fb9b6f..bbf9df0e64c6 100644
>>>>> --- a/mm/debug_vm_pgtable.c
>>>>> +++ b/mm/debug_vm_pgtable.c
>>>>> @@ -44,10 +44,17 @@
>>>>> * entry type. But these bits might affect the ability to clear entries with
>>>>> * pxx_clear() because of how dynamic page table folding works on s390. So
>>>>> * while loading up the entries do not change the lower 4 bits. It does not
>>>>> - * have affect any other platform.
>>>>> + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is
>>>>> + * used to mark a pte entry.
>>>>> */
>>>>> -#define S390_MASK_BITS 4
>>>>> -#define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS)
>>>>> +#define S390_SKIP_MASK GENMASK(3, 0)
>>>>> +#ifdef CONFIG_PPC_BOOK3S_64
>>>>> +#define PPC64_SKIP_MASK GENMASK(62, 62)
>>>>> +#else
>>>>> +#define PPC64_SKIP_MASK 0x0
>>>>> +#endif
>>>>
>>>> Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
>>>> bits for a s390 platform requirement and can also do so for ppc64 as well. As
>>>> mentioned before, please avoid adding any platform specific constructs in the
>>>> test.
>>>>
>>>
>>>
>>> that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux
>>
>> Could not (#if __BITS_PER_LONG == 32) be used instead or something like
>> that. But should be a generic conditional check identifying 32 bit arch
>> not anything platform specific.
>>
>
> that _PAGE_PTE bit is pretty much specific to PPC BOOK3S_64. Not sure why other architectures need to bothered about ignoring bit 62.
Thats okay as long it does not adversely affect other architectures, ignoring
some more bits is acceptable. Like existing S390_MASK_BITS gets ignored on all
other platforms even if it is a s390 specific constraint. Not having platform
specific #ifdef here, is essential.
^ permalink raw reply
* Re: [PATCH v3 08/13] mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP
From: Christophe Leroy @ 2020-09-01 7:51 UTC (permalink / raw)
To: Aneesh Kumar K.V, Anshuman Khandual, linux-mm, akpm
Cc: linux-arch, linux-s390, x86, Mike Rapoport, Qian Cai,
Gerald Schaefer, Vineet Gupta, linux-snps-arc, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <2fb4ac88-d417-2bdd-3c56-a816c356636a@linux.ibm.com>
Le 01/09/2020 à 09:40, Aneesh Kumar K.V a écrit :
> On 9/1/20 12:20 PM, Christophe Leroy wrote:
>>
>>
>> Le 01/09/2020 à 08:25, Aneesh Kumar K.V a écrit :
>>> On 9/1/20 8:52 AM, Anshuman Khandual wrote:
>>>>
>>>>
>>>>
>>>> There is a checkpatch.pl warning here.
>>>>
>>>> WARNING: Possible unwrapped commit description (prefer a maximum 75
>>>> chars per line)
>>>> #7:
>>>> Architectures like ppc64 use deposited page table while updating the
>>>> huge pte
>>>>
>>>> total: 0 errors, 1 warnings, 40 lines checked
>>>>
>>>
>>> I will ignore all these, because they are not really important IMHO.
>>>
>>
>> When doing a git log in a 80 chars terminal window, having wrapping
>> lines is not really convenient. It should be easy to avoid it.
>>
>
> We have been ignoring that for a long time isn't it?
>
> For example ppc64 checkpatch already had
> --max-line-length=90
>
>
> There was also recent discussion whether 80 character limit is valid any
> more. But I do keep it restricted to 80 character where ever it is
> easy/make sense.
>
Here we are not talking about the code, but the commit log.
As far as I know, the discussions about 80 character lines, 90 lines in
powerpc etc ... is for the code.
We still aim at keeping lines not longer than 75 chars in the commit log.
Christophe
Christophe
^ permalink raw reply
* Re: [RESEND][PATCH 0/7] Avoid overflow at boundary_size
From: Nicolin Chen @ 2020-09-01 7:54 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-ia64, James.Bottomley, paulus, hpa, sparclinux, sfr, deller,
x86, borntraeger, mingo, mattst88, fenghua.yu, gor, schnelle, hca,
ink, tglx, gerald.schaefer, rth, tony.luck, linux-parisc,
linux-s390, linux-kernel, linux-alpha, bp, linuxppc-dev, davem
In-Reply-To: <20200901073623.GA30418@lst.de>
Hi Christoph,
On Tue, Sep 01, 2020 at 09:36:23AM +0200, Christoph Hellwig wrote:
> I really don't like all the open coded smarts in the various drivers.
> What do you think about a helper like the one in the untested patch
A helper function will be actually better. I was thinking of
one yet not very sure about the naming and where to put it.
> below (on top of your series). Also please include the original
> segment boundary patch with the next resend so that the series has
> the full context.
I will use your change instead and resend with the ULONG_MAX
change. But in that case, should I make separate changes for
different files like this series, or just one single change
like yours?
Asking this as I was expecting that those changes would get
applied by different maintainers. But now it feels like you
will merge it to your tree at once?
Thanks
Nic
> diff --git a/arch/alpha/kernel/pci_iommu.c b/arch/alpha/kernel/pci_iommu.c
> index 1ef2c647bd3ec2..6f7de4f4e191e7 100644
> --- a/arch/alpha/kernel/pci_iommu.c
> +++ b/arch/alpha/kernel/pci_iommu.c
> @@ -141,10 +141,7 @@ iommu_arena_find_pages(struct device *dev, struct pci_iommu_arena *arena,
> unsigned long boundary_size;
>
> base = arena->dma_base >> PAGE_SHIFT;
> -
> - boundary_size = dev ? dma_get_seg_boundary(dev) : U32_MAX;
> - /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> - boundary_size = (boundary_size >> PAGE_SHIFT) + 1;
> + boundary_size = dma_get_seg_boundary_nr_pages(dev, PAGE_SHIFT);
>
> /* Search forward for the first mask-aligned sequence of N free ptes */
> ptes = arena->ptes;
> diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c
> index 945954903bb0ba..b49b73a95067d2 100644
> --- a/arch/ia64/hp/common/sba_iommu.c
> +++ b/arch/ia64/hp/common/sba_iommu.c
> @@ -485,8 +485,7 @@ sba_search_bitmap(struct ioc *ioc, struct device *dev,
> ASSERT(((unsigned long) ioc->res_hint & (sizeof(unsigned long) - 1UL)) == 0);
> ASSERT(res_ptr < res_end);
>
> - /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> - boundary_size = (dma_get_seg_boundary(dev) >> iovp_shift) + 1;
> + boundary_size = dma_get_seg_boundary_nr_pages(dev, iovp_shift);
>
> BUG_ON(ioc->ibase & ~iovp_mask);
> shift = ioc->ibase >> iovp_shift;
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index c01ccbf8afdd42..cbc2e62db597cf 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -236,11 +236,7 @@ static unsigned long iommu_range_alloc(struct device *dev,
> }
> }
>
> - /* 4GB boundary for iseries_hv_alloc and iseries_hv_map */
> - boundary_size = dev ? dma_get_seg_boundary(dev) : U32_MAX;
> -
> - /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> - boundary_size = (boundary_size >> tbl->it_page_shift) + 1;
> + boundary_size = dma_get_seg_boundary_nr_pages(dev, tbl->it_page_shift);
>
> n = iommu_area_alloc(tbl->it_map, limit, start, npages, tbl->it_offset,
> boundary_size, align_mask);
> diff --git a/arch/s390/pci/pci_dma.c b/arch/s390/pci/pci_dma.c
> index ecb067acc6d532..4a37d8f4de9d9d 100644
> --- a/arch/s390/pci/pci_dma.c
> +++ b/arch/s390/pci/pci_dma.c
> @@ -261,13 +261,11 @@ static unsigned long __dma_alloc_iommu(struct device *dev,
> unsigned long start, int size)
> {
> struct zpci_dev *zdev = to_zpci(to_pci_dev(dev));
> - unsigned long boundary_size;
>
> - /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> - boundary_size = (dma_get_seg_boundary(dev) >> PAGE_SHIFT) + 1;
> return iommu_area_alloc(zdev->iommu_bitmap, zdev->iommu_pages,
> start, size, zdev->start_dma >> PAGE_SHIFT,
> - boundary_size, 0);
> + dma_get_seg_boundary_nr_pages(dev, PAGE_SHIFT),
> + 0);
> }
>
> static dma_addr_t dma_alloc_address(struct device *dev, int size)
> diff --git a/arch/sparc/kernel/iommu-common.c b/arch/sparc/kernel/iommu-common.c
> index 843e71894d0482..e6139c99762e11 100644
> --- a/arch/sparc/kernel/iommu-common.c
> +++ b/arch/sparc/kernel/iommu-common.c
> @@ -166,10 +166,6 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
> }
> }
>
> - boundary_size = dev ? dma_get_seg_boundary(dev) : U32_MAX;
> -
> - /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> - boundary_size = (boundary_size >> iommu->table_shift) + 1;
> /*
> * if the skip_span_boundary_check had been set during init, we set
> * things up so that iommu_is_span_boundary() merely checks if the
> @@ -178,7 +174,11 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
> if ((iommu->flags & IOMMU_NO_SPAN_BOUND) != 0) {
> shift = 0;
> boundary_size = iommu->poolsize * iommu->nr_pools;
> + } else {
> + boundary_size = dma_get_seg_boundary_nr_pages(dev,
> + iommu->table_shift);
> }
> +
> n = iommu_area_alloc(iommu->map, limit, start, npages, shift,
> boundary_size, align_mask);
> if (n == -1) {
> diff --git a/arch/sparc/kernel/iommu.c b/arch/sparc/kernel/iommu.c
> index d981c37305ae31..c3e4e2df26a8b8 100644
> --- a/arch/sparc/kernel/iommu.c
> +++ b/arch/sparc/kernel/iommu.c
> @@ -472,8 +472,7 @@ static int dma_4u_map_sg(struct device *dev, struct scatterlist *sglist,
> outs->dma_length = 0;
>
> max_seg_size = dma_get_max_seg_size(dev);
> - /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> - seg_boundary_size = (dma_get_seg_boundary(dev) >> IO_PAGE_SHIFT) + 1;
> + seg_boundary_size = dma_get_seg_boundary_nr_pages(dev, IO_PAGE_SHIFT);
> base_shift = iommu->tbl.table_map_base >> IO_PAGE_SHIFT;
> for_each_sg(sglist, s, nelems, i) {
> unsigned long paddr, npages, entry, out_entry = 0, slen;
> diff --git a/arch/sparc/kernel/pci_sun4v.c b/arch/sparc/kernel/pci_sun4v.c
> index 233fe8a20cec33..6b92dd51c0026f 100644
> --- a/arch/sparc/kernel/pci_sun4v.c
> +++ b/arch/sparc/kernel/pci_sun4v.c
> @@ -508,8 +508,7 @@ static int dma_4v_map_sg(struct device *dev, struct scatterlist *sglist,
> iommu_batch_start(dev, prot, ~0UL);
>
> max_seg_size = dma_get_max_seg_size(dev);
> - /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> - seg_boundary_size = (dma_get_seg_boundary(dev) >> IO_PAGE_SHIFT) + 1;
> + seg_boundary_size = dma_get_seg_boundary_nr_pages(dev, IO_PAGE_SHIFT);
>
> mask = *dev->dma_mask;
> if (!iommu_use_atu(iommu, mask))
> diff --git a/arch/x86/kernel/amd_gart_64.c b/arch/x86/kernel/amd_gart_64.c
> index 7fa0bb490065a1..bccc5357bffd6c 100644
> --- a/arch/x86/kernel/amd_gart_64.c
> +++ b/arch/x86/kernel/amd_gart_64.c
> @@ -96,8 +96,7 @@ static unsigned long alloc_iommu(struct device *dev, int size,
>
> base_index = ALIGN(iommu_bus_base & dma_get_seg_boundary(dev),
> PAGE_SIZE) >> PAGE_SHIFT;
> - /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> - boundary_size = (dma_get_seg_boundary(dev) >> PAGE_SHIFT) + 1;
> + boundary_size = dma_get_seg_boundary_nr_pages(dev, PAGE_SHIFT);
>
> spin_lock_irqsave(&iommu_bitmap_lock, flags);
> offset = iommu_area_alloc(iommu_gart_bitmap, iommu_pages, next_bit,
> diff --git a/drivers/parisc/ccio-dma.c b/drivers/parisc/ccio-dma.c
> index c667d6aba7646e..ba16b7f8f80612 100644
> --- a/drivers/parisc/ccio-dma.c
> +++ b/drivers/parisc/ccio-dma.c
> @@ -356,8 +356,7 @@ ccio_alloc_range(struct ioc *ioc, struct device *dev, size_t size)
> ** ggg sacrifices another 710 to the computer gods.
> */
>
> - /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> - boundary_size = (dma_get_seg_boundary(dev) >> IOVP_SHIFT) + 1;
> + boundary_size = dma_get_seg_boundary_nr_pages(dev, IOVP_SHIFT);
>
> if (pages_needed <= 8) {
> /*
> diff --git a/drivers/parisc/sba_iommu.c b/drivers/parisc/sba_iommu.c
> index 96bc2c617cbd19..959bda193b9603 100644
> --- a/drivers/parisc/sba_iommu.c
> +++ b/drivers/parisc/sba_iommu.c
> @@ -342,8 +342,7 @@ sba_search_bitmap(struct ioc *ioc, struct device *dev,
> unsigned long shift;
> int ret;
>
> - /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> - boundary_size = (dma_get_seg_boundary(dev) >> IOVP_SHIFT) + 1;
> + boundary_size = dma_get_seg_boundary_nr_pages(dev, IOVP_SHIFT);
>
> #if defined(ZX1_SUPPORT)
> BUG_ON(ioc->ibase & ~IOVP_MASK);
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 52635e91143b25..7477a164500adb 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -632,6 +632,25 @@ static inline unsigned long dma_get_seg_boundary(struct device *dev)
> return DMA_BIT_MASK(32);
> }
>
> +/**
> + * dma_get_seg_boundary_nr_pages - return the segment boundary in "page" units
> + * @dev: device to guery the boundary for
> + * @page_shift: ilog() of the the IOMMU page size
> + *
> + * Return the segment boundary in IOMMU page units (which may be different from
> + * the CPU page size) for the passed in device.
> + *
> + * If @dev is NULL a boundary of U32_MAX is assumed, this case is just for
> + * non-DMA API callers.
> + */
> +static inline unsigned long dma_get_seg_boundary_nr_pages(struct device *dev,
> + unsigned int page_shift)
> +{
> + if (!dev)
> + return (U32_MAX >> page_shift) + 1;
> + return (dma_get_seg_boundary(dev) >> page_shift) + 1;
> +}
> +
> static inline int dma_set_seg_boundary(struct device *dev, unsigned long mask)
> {
> if (dev->dma_parms) {
^ permalink raw reply
* Re: [PATCH v3 03/13] mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value
From: Aneesh Kumar K.V @ 2020-09-01 7:55 UTC (permalink / raw)
To: Anshuman Khandual, linux-mm, akpm
Cc: linux-arch, linux-s390, Christophe Leroy, x86, Mike Rapoport,
Qian Cai, Gerald Schaefer, Vineet Gupta, linux-snps-arc,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <c371e316-7533-62c7-a1c6-9b6745d8d1ea@arm.com>
On 9/1/20 1:16 PM, Anshuman Khandual wrote:
>
>
> On 09/01/2020 01:06 PM, Aneesh Kumar K.V wrote:
>> On 9/1/20 1:02 PM, Anshuman Khandual wrote:
>>>
>>>
>>> On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote:
>>>> On 9/1/20 8:45 AM, Anshuman Khandual wrote:
>>>>>
>>>>>
>>>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>>>>> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
>>>>>> random value.
>>>>>>
>>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>>>> ---
>>>>>> mm/debug_vm_pgtable.c | 13 ++++++++++---
>>>>>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>>>> index 086309fb9b6f..bbf9df0e64c6 100644
>>>>>> --- a/mm/debug_vm_pgtable.c
>>>>>> +++ b/mm/debug_vm_pgtable.c
>>>>>> @@ -44,10 +44,17 @@
>>>>>> * entry type. But these bits might affect the ability to clear entries with
>>>>>> * pxx_clear() because of how dynamic page table folding works on s390. So
>>>>>> * while loading up the entries do not change the lower 4 bits. It does not
>>>>>> - * have affect any other platform.
>>>>>> + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is
>>>>>> + * used to mark a pte entry.
>>>>>> */
>>>>>> -#define S390_MASK_BITS 4
>>>>>> -#define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS)
>>>>>> +#define S390_SKIP_MASK GENMASK(3, 0)
>>>>>> +#ifdef CONFIG_PPC_BOOK3S_64
>>>>>> +#define PPC64_SKIP_MASK GENMASK(62, 62)
>>>>>> +#else
>>>>>> +#define PPC64_SKIP_MASK 0x0
>>>>>> +#endif
>>>>>
>>>>> Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
>>>>> bits for a s390 platform requirement and can also do so for ppc64 as well. As
>>>>> mentioned before, please avoid adding any platform specific constructs in the
>>>>> test.
>>>>>
>>>>
>>>>
>>>> that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux
>>>
>>> Could not (#if __BITS_PER_LONG == 32) be used instead or something like
>>> that. But should be a generic conditional check identifying 32 bit arch
>>> not anything platform specific.
>>>
>>
>> that _PAGE_PTE bit is pretty much specific to PPC BOOK3S_64. Not sure why other architectures need to bothered about ignoring bit 62.
>
> Thats okay as long it does not adversely affect other architectures, ignoring
> some more bits is acceptable. Like existing S390_MASK_BITS gets ignored on all
> other platforms even if it is a s390 specific constraint. Not having platform
> specific #ifdef here, is essential.
>
Why is it essential?
-aneesh
^ permalink raw reply
* Re: [PATCH v3 12/13] mm/debug_vm_pgtable/hugetlb: Disable hugetlb test on ppc64
From: Christophe Leroy @ 2020-09-01 7:59 UTC (permalink / raw)
To: Aneesh Kumar K.V, Anshuman Khandual, linux-mm, akpm
Cc: linux-arch, linux-s390, Christophe Leroy, x86, Mike Rapoport,
Erhard F., Qian Cai, Gerald Schaefer, Vineet Gupta,
linux-snps-arc, linuxppc-dev, linux-arm-kernel
In-Reply-To: <68f90b44-b830-58be-3c21-424fee05da37@linux.ibm.com>
Le 01/09/2020 à 08:30, Aneesh Kumar K.V a écrit :
>>
>
> I actually wanted to add #ifdef BROKEN. That test is completely broken.
> Infact I would suggest to remove that test completely.
>
>
>
>> #ifdef will not be required here as there would be a stub definition
>> for hugetlb_advanced_tests() when CONFIG_PPC_BOOK3S_64 is enabled.
>>
>>> spin_lock(&mm->page_table_lock);
>>> p4d_clear_tests(mm, p4dp);
>>>
>>
>> But again, we should really try and avoid taking this path.
>>
>
> To be frank i am kind of frustrated with how this patch series is being
> looked at. We pushed a completely broken test to upstream and right now
> we have a code in upstream that crash when booted on ppc64. My attempt
> has been to make progress here and you definitely seems to be not in
> agreement to that.
>
> At this point I am tempted to suggest we remove the DEBUG_VM_PGTABLE
> support on ppc64 because AFAIU it doesn't add any value.
>
Note that a bug has been filed at
https://bugzilla.kernel.org/show_bug.cgi?id=209029
Christophe
^ permalink raw reply
* [PATCH] powerpc/mm: Remove DEBUG_VM_PGTABLE support on ppc64
From: Aneesh Kumar K.V @ 2020-09-01 8:02 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, Anshuman Khandual
The test is broken w.r.t page table update rules and results in kernel
crash as below. Disable the support untill we get the tests updated.
[ 21.083506] ------------[ cut here ]------------
[ 21.083519] kernel BUG at arch/powerpc/mm/pgtable.c:304!
cpu 0x0: Vector: 700 (Program Check) at [c000000c6d1e76c0]
pc: c00000000009a5ec: assert_pte_locked+0x14c/0x380
lr: c0000000005eeeec: pte_update+0x11c/0x190
sp: c000000c6d1e7950
msr: 8000000002029033
current = 0xc000000c6d172c80
paca = 0xc000000003ba0000 irqmask: 0x03 irq_happened: 0x01
pid = 1, comm = swapper/0
kernel BUG at arch/powerpc/mm/pgtable.c:304!
Linux version 5.9.0-rc2-34902-g4da73871507c (kvaneesh@ltczz75-lp2) (gcc (Ubuntu 9.3.0-10ubuntu2) 9.3.0, GNU ld (GNU Binutils for Ubuntu) 2.34) #301 SMP Tue Sep 1 02:28:29 CDT 2020
enter ? for help
[link register ] c0000000005eeeec pte_update+0x11c/0x190
[c000000c6d1e7950] 0000000000000001 (unreliable)
[c000000c6d1e79b0] c0000000005eee14 pte_update+0x44/0x190
[c000000c6d1e7a10] c000000001a2ca9c pte_advanced_tests+0x160/0x3d8
[c000000c6d1e7ab0] c000000001a2d4fc debug_vm_pgtable+0x7e8/0x1338
[c000000c6d1e7ba0] c0000000000116ec do_one_initcall+0xac/0x5f0
[c000000c6d1e7c80] c0000000019e4fac kernel_init_freeable+0x4dc/0x5a4
[c000000c6d1e7db0] c000000000012474 kernel_init+0x24/0x160
[c000000c6d1e7e20] c00000000000cbd0 ret_from_kernel_thread+0x5c/0x6c
With DEBUG_VM disabled
[ 20.530152] BUG: Kernel NULL pointer dereference on read at 0x00000000
[ 20.530183] Faulting instruction address: 0xc0000000000df330
cpu 0x33: Vector: 380 (Data SLB Access) at [c000000c6d19f700]
pc: c0000000000df330: memset+0x68/0x104
lr: c00000000009f6d8: hash__pmdp_huge_get_and_clear+0xe8/0x1b0
sp: c000000c6d19f990
msr: 8000000002009033
dar: 0
current = 0xc000000c6d177480
paca = 0xc00000001ec4f400 irqmask: 0x03 irq_happened: 0x01
pid = 1, comm = swapper/0
Linux version 5.9.0-rc2-34902-g4da73871507c (kvaneesh@ltczz75-lp2) (gcc (Ubuntu 9.3.0-10ubuntu2) 9.3.0, GNU ld (GNU Binutils for Ubuntu) 2.34) #302 SMP Tue Sep 1 02:56:02 CDT 2020
enter ? for help
[link register ] c00000000009f6d8 hash__pmdp_huge_get_and_clear+0xe8/0x1b0
[c000000c6d19f990] c00000000009f748 hash__pmdp_huge_get_and_clear+0x158/0x1b0 (unreliable)
[c000000c6d19fa10] c0000000019ebf30 pmd_advanced_tests+0x1f0/0x378
[c000000c6d19fab0] c0000000019ed088 debug_vm_pgtable+0x79c/0x1244
[c000000c6d19fba0] c0000000000116ec do_one_initcall+0xac/0x5f0
[c000000c6d19fc80] c0000000019a4fac kernel_init_freeable+0x4dc/0x5a4
[c000000c6d19fdb0] c000000000012474 kernel_init+0x24/0x160
[c000000c6d19fe20] c00000000000cbd0 ret_from_kernel_thread+0x5c/0x6c
33:mon>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/Kconfig | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 65bed1fdeaad..787e829b6f25 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -116,7 +116,6 @@ config PPC
#
select ARCH_32BIT_OFF_T if PPC32
select ARCH_HAS_DEBUG_VIRTUAL
- select ARCH_HAS_DEBUG_VM_PGTABLE
select ARCH_HAS_DEVMEM_IS_ALLOWED
select ARCH_HAS_ELF_RANDOMIZE
select ARCH_HAS_FORTIFY_SOURCE
--
2.26.2
^ permalink raw reply related
* Re: [PATCH v2 00/13] mm/debug_vm_pgtable fixes
From: Christophe Leroy @ 2020-09-01 8:03 UTC (permalink / raw)
To: Anshuman Khandual, Aneesh Kumar K.V, linux-mm, akpm
Cc: Linux-Arch, linux-s390@vger.kernel.org, x86@kernel.org,
Mike Rapoport, Qian Cai, Gerald Schaefer, Vineet Gupta,
linux-snps-arc@lists.infradead.org, linuxppc-dev, Linux ARM
In-Reply-To: <52e9743e-fa2f-3fd2-f50e-2c6c38464b96@arm.com>
Le 21/08/2020 à 10:51, Anshuman Khandual a écrit :
>
> On 08/19/2020 06:30 PM, Aneesh Kumar K.V wrote:
>> This patch series includes fixes for debug_vm_pgtable test code so that
>> they follow page table updates rules correctly. The first two patches introduce
>> changes w.r.t ppc64. The patches are included in this series for completeness. We can
>> merge them via ppc64 tree if required.
>>
>> Hugetlb test is disabled on ppc64 because that needs larger change to satisfy
>> page table update rules.
>>
>
> Changes proposed here will impact other enabled platforms as well.
> Adding the following folks and mailing lists, and hoping to get a
> broader review and test coverage. Please do include them in the
> next iteration as well.
>
> + linux-arm-kernel@lists.infradead.org
> + linux-s390@vger.kernel.org
> + linux-snps-arc@lists.infradead.org
> + x86@kernel.org
> + linux-arch@vger.kernel.org
>
> + Gerald Schaefer <gerald.schaefer@de.ibm.com>
> + Christophe Leroy <christophe.leroy@c-s.fr>
Please don't use anymore the above address. Only use the one below.
> + Christophe Leroy <christophe.leroy@csgroup.eu>
> + Vineet Gupta <vgupta@synopsys.com>
> + Mike Rapoport <rppt@linux.ibm.com>
> + Qian Cai <cai@lca.pw>
>
Thanks
Christophe
^ permalink raw reply
* Re: [PATCH] powerpc/mm: Remove DEBUG_VM_PGTABLE support on ppc64
From: Christophe Leroy @ 2020-09-01 8:10 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev, mpe; +Cc: Anshuman Khandual
In-Reply-To: <20200901080245.67950-1-aneesh.kumar@linux.ibm.com>
Le 01/09/2020 à 10:02, Aneesh Kumar K.V a écrit :
> The test is broken w.r.t page table update rules and results in kernel
> crash as below. Disable the support untill we get the tests updated.
>
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Any Fixes: tag ?
> ---
> arch/powerpc/Kconfig | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 65bed1fdeaad..787e829b6f25 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -116,7 +116,6 @@ config PPC
> #
> select ARCH_32BIT_OFF_T if PPC32
> select ARCH_HAS_DEBUG_VIRTUAL
> - select ARCH_HAS_DEBUG_VM_PGTABLE
You say you remove support for ppc64 but you are removing it for both
PPC64 and PPC32. Should you replace the line by:
select ARCH_HAS_DEBUG_VM_PGTABLE if PPC32
> select ARCH_HAS_DEVMEM_IS_ALLOWED
> select ARCH_HAS_ELF_RANDOMIZE
> select ARCH_HAS_FORTIFY_SOURCE
>
What about Documentation/features/debug/debug-vm-pgtable/arch-support.txt ?
Christophe
^ permalink raw reply
* Re: [PATCH] powerpc/mm: Remove DEBUG_VM_PGTABLE support on ppc64
From: Aneesh Kumar K.V @ 2020-09-01 8:12 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev, mpe; +Cc: Anshuman Khandual
In-Reply-To: <4519baaa-cb95-6ebb-200f-4520badb56f6@csgroup.eu>
On 9/1/20 1:40 PM, Christophe Leroy wrote:
>
>
> Le 01/09/2020 à 10:02, Aneesh Kumar K.V a écrit :
>> The test is broken w.r.t page table update rules and results in kernel
>> crash as below. Disable the support untill we get the tests updated.
>>
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>
> Any Fixes: tag ?
>
>> ---
>> arch/powerpc/Kconfig | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index 65bed1fdeaad..787e829b6f25 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -116,7 +116,6 @@ config PPC
>> #
>> select ARCH_32BIT_OFF_T if PPC32
>> select ARCH_HAS_DEBUG_VIRTUAL
>> - select ARCH_HAS_DEBUG_VM_PGTABLE
>
>
> You say you remove support for ppc64 but you are removing it for both
> PPC64 and PPC32. Should you replace the line by:
Does it work on PPC32 with DEBUG_VM enabled? I was expecting it will be
broken there too.
>
> select ARCH_HAS_DEBUG_VM_PGTABLE if PPC32
>
>> select ARCH_HAS_DEVMEM_IS_ALLOWED
>> select ARCH_HAS_ELF_RANDOMIZE
>> select ARCH_HAS_FORTIFY_SOURCE
>>
>
> What about Documentation/features/debug/debug-vm-pgtable/arch-support.txt ?
>
I am hoping we can enable the config once we resolve the test issues.
may be in next merge window.
-aneesh
^ permalink raw reply
* Re: [PATCH] powerpc/mm: Remove DEBUG_VM_PGTABLE support on ppc64
From: Christophe Leroy @ 2020-09-01 8:15 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev, mpe; +Cc: Anshuman Khandual
In-Reply-To: <39467f79-d213-772d-9ed1-93afedc2fc7a@linux.ibm.com>
Le 01/09/2020 à 10:12, Aneesh Kumar K.V a écrit :
> On 9/1/20 1:40 PM, Christophe Leroy wrote:
>>
>>
>> Le 01/09/2020 à 10:02, Aneesh Kumar K.V a écrit :
>>> The test is broken w.r.t page table update rules and results in kernel
>>> crash as below. Disable the support untill we get the tests updated.
>>>
>>>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>
>> Any Fixes: tag ?
>>
>>> ---
>>> arch/powerpc/Kconfig | 1 -
>>> 1 file changed, 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>> index 65bed1fdeaad..787e829b6f25 100644
>>> --- a/arch/powerpc/Kconfig
>>> +++ b/arch/powerpc/Kconfig
>>> @@ -116,7 +116,6 @@ config PPC
>>> #
>>> select ARCH_32BIT_OFF_T if PPC32
>>> select ARCH_HAS_DEBUG_VIRTUAL
>>> - select ARCH_HAS_DEBUG_VM_PGTABLE
>>
>>
>> You say you remove support for ppc64 but you are removing it for both
>> PPC64 and PPC32. Should you replace the line by:
>
> Does it work on PPC32 with DEBUG_VM enabled? I was expecting it will be
> broken there too.
I was wondering. I have just started a build to test that on my 8xx.
I'll tell you before noon (Paris).
Christophe
>
>>
>> select ARCH_HAS_DEBUG_VM_PGTABLE if PPC32
>>
>>> select ARCH_HAS_DEVMEM_IS_ALLOWED
>>> select ARCH_HAS_ELF_RANDOMIZE
>>> select ARCH_HAS_FORTIFY_SOURCE
>>>
>>
>> What about
>> Documentation/features/debug/debug-vm-pgtable/arch-support.txt ?
>>
>
> I am hoping we can enable the config once we resolve the test issues.
> may be in next merge window.
>
> -aneesh
>
>
>
^ permalink raw reply
* Re: [RESEND][PATCH 0/7] Avoid overflow at boundary_size
From: Christoph Hellwig @ 2020-09-01 9:11 UTC (permalink / raw)
To: Nicolin Chen
Cc: linux-ia64, James.Bottomley, paulus, hpa, sparclinux,
Christoph Hellwig, sfr, deller, x86, borntraeger, mingo, mattst88,
fenghua.yu, gor, schnelle, hca, ink, tglx, gerald.schaefer, rth,
tony.luck, linux-parisc, linux-s390, linux-kernel, linux-alpha,
bp, linuxppc-dev, davem
In-Reply-To: <20200901075401.GA5667@Asurada-Nvidia>
On Tue, Sep 01, 2020 at 12:54:01AM -0700, Nicolin Chen wrote:
> Hi Christoph,
>
> On Tue, Sep 01, 2020 at 09:36:23AM +0200, Christoph Hellwig wrote:
> > I really don't like all the open coded smarts in the various drivers.
> > What do you think about a helper like the one in the untested patch
>
> A helper function will be actually better. I was thinking of
> one yet not very sure about the naming and where to put it.
>
> > below (on top of your series). Also please include the original
> > segment boundary patch with the next resend so that the series has
> > the full context.
>
> I will use your change instead and resend with the ULONG_MAX
> change. But in that case, should I make separate changes for
> different files like this series, or just one single change
> like yours?
>
> Asking this as I was expecting that those changes would get
> applied by different maintainers. But now it feels like you
> will merge it to your tree at once?
I guess one patch is fine. I can queue it up in the dma-mapping
tree as a prep patch for the default boundary change.
^ permalink raw reply
* Re: [PATCH] powerpc/mm: Remove DEBUG_VM_PGTABLE support on ppc64
From: Christophe Leroy @ 2020-09-01 9:10 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev, mpe; +Cc: Anshuman Khandual
In-Reply-To: <6661a001-0a00-17b6-cb34-0f3510ca1fec@csgroup.eu>
Le 01/09/2020 à 10:15, Christophe Leroy a écrit :
>
>
> Le 01/09/2020 à 10:12, Aneesh Kumar K.V a écrit :
>> On 9/1/20 1:40 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 01/09/2020 à 10:02, Aneesh Kumar K.V a écrit :
>>>> The test is broken w.r.t page table update rules and results in kernel
>>>> crash as below. Disable the support untill we get the tests updated.
>>>>
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>
>>> Any Fixes: tag ?
>>>
>>>> ---
>>>> arch/powerpc/Kconfig | 1 -
>>>> 1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>>> index 65bed1fdeaad..787e829b6f25 100644
>>>> --- a/arch/powerpc/Kconfig
>>>> +++ b/arch/powerpc/Kconfig
>>>> @@ -116,7 +116,6 @@ config PPC
>>>> #
>>>> select ARCH_32BIT_OFF_T if PPC32
>>>> select ARCH_HAS_DEBUG_VIRTUAL
>>>> - select ARCH_HAS_DEBUG_VM_PGTABLE
>>>
>>>
>>> You say you remove support for ppc64 but you are removing it for both
>>> PPC64 and PPC32. Should you replace the line by:
>>
>> Does it work on PPC32 with DEBUG_VM enabled? I was expecting it will
>> be broken there too.
>
> I was wondering. I have just started a build to test that on my 8xx.
> I'll tell you before noon (Paris).
>
There are warnings but it boots OK. So up to you, but if you deactivate
it for both PPC32 and PPC64, say so in the subject like.
[ 7.065399] debug_vm_pgtable: [debug_vm_pgtable ]: Validating
architecture page table helpers
[ 7.075155] ------------[ cut here ]------------
[ 7.079590] WARNING: CPU: 0 PID: 1 at arch/powerpc/mm/pgtable.c:185
set_pte_at+0x20/0xf4
[ 7.087542] CPU: 0 PID: 1 Comm: swapper Not tainted
5.9.0-rc2-s3k-dev-01348-g283e890ee4ad #3933
[ 7.096122] NIP: c000f634 LR: c07440f8 CTR: 00000000
[ 7.101124] REGS: c9023c50 TRAP: 0700 Not tainted
(5.9.0-rc2-s3k-dev-01348-g283e890ee4ad)
[ 7.109445] MSR: 00029032 <EE,ME,IR,DR,RI> CR: 53000339 XER: 80006100
[ 7.116072]
[ 7.116072] GPR00: c07440f8 c9023d08 c60e4000 c6ba4000 1efac000
c6ba8eb0 c9023da8 00000001
[ 7.116072] GPR08: 00000004 007346c9 c6ba8ebc 00000000 93000333
00000000 c000390c 00000000
[ 7.116072] GPR16: c0840000 00000ec9 000001ec 00734000 06ba8000
c6bb0000 c05f43e8 1efac000
[ 7.116072] GPR24: fffffff0 c6b96d08 c6ba8eac c6ba4000 1efac000
007346c9 c6ba8eb0 007346c9
[ 7.150796] NIP [c000f634] set_pte_at+0x20/0xf4
[ 7.155274] LR [c07440f8] pte_advanced_tests+0xec/0x2bc
[ 7.160401] Call Trace:
[ 7.162831] [c9023d08] [c080db94] 0xc080db94 (unreliable)
[ 7.168183] [c9023d28] [c07440f8] pte_advanced_tests+0xec/0x2bc
[ 7.174036] [c9023dd8] [c0744498] debug_vm_pgtable+0x1d0/0x668
[ 7.179827] [c9023e98] [c0734cd4] do_one_initcall+0x8c/0x1cc
[ 7.185405] [c9023ef8] [c0735008] kernel_init_freeable+0x178/0x1d0
[ 7.191511] [c9023f28] [c0003920] kernel_init+0x14/0x114
[ 7.196763] [c9023f38] [c000e184] ret_from_kernel_thread+0x14/0x1c
[ 7.202818] Instruction dump:
[ 7.205754] bba10014 7c0803a6 38210020 4e800020 7c0802a6 9421ffe0
bfc10018 90010024
[ 7.213412] 83e60000 81250000 71270001 41820008 <0fe00000> 73e90040
41820080 73ea0001
[ 7.221249] ---[ end trace 95bbebcafa22d0f7 ]---
[ 7.226049] ------------[ cut here ]------------
[ 7.230438] WARNING: CPU: 0 PID: 1 at arch/powerpc/mm/pgtable.c:185
set_pte_at+0x20/0xf4
[ 7.238410] CPU: 0 PID: 1 Comm: swapper Tainted: G W
5.9.0-rc2-s3k-dev-01348-g283e890ee4ad #3933
[ 7.248363] NIP: c000f634 LR: c0744218 CTR: 00000000
[ 7.253368] REGS: c9023c50 TRAP: 0700 Tainted: G W
(5.9.0-rc2-s3k-dev-01348-g283e890ee4ad)
[ 7.263064] MSR: 00029032 <EE,ME,IR,DR,RI> CR: 53000335 XER: a0006100
[ 7.269690]
[ 7.269690] GPR00: c0744218 c9023d08 c60e4000 c6ba4000 1efac000
c6ba8eb0 c9023da8 00000001
[ 7.269690] GPR08: 00000000 007341c9 00000000 007341c9 93000333
00000000 c000390c 00000000
[ 7.269690] GPR16: c0840000 00000ec9 000001ec 00734000 06ba8000
c6bb0000 c05f43e8 1efac000
[ 7.269690] GPR24: fffffff0 c6b96d08 c6ba8eac c6ba4000 1efac000
007346c9 c6ba8eb0 007346c9
[ 7.304418] NIP [c000f634] set_pte_at+0x20/0xf4
[ 7.308892] LR [c0744218] pte_advanced_tests+0x20c/0x2bc
[ 7.314105] Call Trace:
[ 7.316535] [c9023d08] [c080db94] 0xc080db94 (unreliable)
[ 7.321888] [c9023d28] [c0744218] pte_advanced_tests+0x20c/0x2bc
[ 7.327826] [c9023dd8] [c0744498] debug_vm_pgtable+0x1d0/0x668
[ 7.333613] [c9023e98] [c0734cd4] do_one_initcall+0x8c/0x1cc
[ 7.339196] [c9023ef8] [c0735008] kernel_init_freeable+0x178/0x1d0
[ 7.345300] [c9023f28] [c0003920] kernel_init+0x14/0x114
[ 7.350551] [c9023f38] [c000e184] ret_from_kernel_thread+0x14/0x1c
[ 7.356609] Instruction dump:
[ 7.359545] bba10014 7c0803a6 38210020 4e800020 7c0802a6 9421ffe0
bfc10018 90010024
[ 7.367203] 83e60000 81250000 71270001 41820008 <0fe00000> 73e90040
41820080 73ea0001
[ 7.375039] ---[ end trace 95bbebcafa22d0f8 ]---
[ 7.379783] ------------[ cut here ]------------
[ 7.384228] WARNING: CPU: 0 PID: 1 at arch/powerpc/mm/pgtable.c:276
set_huge_pte_at+0x104/0x134
[ 7.392803] CPU: 0 PID: 1 Comm: swapper Tainted: G W
5.9.0-rc2-s3k-dev-01348-g283e890ee4ad #3933
[ 7.402756] NIP: c000f8fc LR: c074465c CTR: 00000000
[ 7.407761] REGS: c9023d00 TRAP: 0700 Tainted: G W
(5.9.0-rc2-s3k-dev-01348-g283e890ee4ad)
[ 7.417456] MSR: 00029032 <EE,ME,IR,DR,RI> CR: 53000339 XER: a0006100
[ 7.424082]
[ 7.424082] GPR00: c074465c c9023db8 c60e4000 c6ba4000 000001ec
c6ba8eb0 c9023e48 00000001
[ 7.424082] GPR08: c6ba90ac 0000000b 007346c9 007341c9 93000333
00000000 c000390c 00000000
[ 7.424082] GPR16: c0840000 c6ba8eac 000001ec 00734000 06ba8000
c6bb0000 c05f43e8 1efac000
[ 7.424082] GPR24: c7fb39a0 c6ba8000 c6b96d08 000001cd 000006c9
c0840000 007346c9 00000080
[ 7.458810] NIP [c000f8fc] set_huge_pte_at+0x104/0x134
[ 7.463885] LR [c074465c] debug_vm_pgtable+0x394/0x668
[ 7.468928] Call Trace:
[ 7.471358] [c9023db8] [c6ba8000] 0xc6ba8000 (unreliable)
[ 7.476709] [c9023dd8] [c074465c] debug_vm_pgtable+0x394/0x668
[ 7.482497] [c9023e98] [c0734cd4] do_one_initcall+0x8c/0x1cc
[ 7.488081] [c9023ef8] [c0735008] kernel_init_freeable+0x178/0x1d0
[ 7.494182] [c9023f28] [c0003920] kernel_init+0x14/0x114
[ 7.499436] [c9023f38] [c000e184] ret_from_kernel_thread+0x14/0x1c
[ 7.505494] Instruction dump:
[ 7.508429] 7d2a482e 712a0800 40a2ff78 812204b4 2f890000 419e0014
812900a0 55290034
[ 7.516088] 2b890400 419e0014 57de06b0 4bffff54 <0fe00000> 4bffff3c
7fa3eb78 90a10008
[ 7.523925] ---[ end trace 95bbebcafa22d0f9 ]---
Christophe
^ permalink raw reply
* Re: [PATCH v2 00/13] mm/debug_vm_pgtable fixes
From: Anshuman Khandual @ 2020-09-01 9:11 UTC (permalink / raw)
To: Christophe Leroy, Aneesh Kumar K.V, linux-mm, akpm
Cc: Linux-Arch, linux-s390@vger.kernel.org, x86@kernel.org,
Mike Rapoport, Qian Cai, Gerald Schaefer, Vineet Gupta,
linux-snps-arc@lists.infradead.org, linuxppc-dev, Linux ARM
In-Reply-To: <c0de2c68-826b-bf0f-dc2c-a501fa7bef38@csgroup.eu>
On 09/01/2020 01:33 PM, Christophe Leroy wrote:
>
>
> Le 21/08/2020 à 10:51, Anshuman Khandual a écrit :
>>
>> On 08/19/2020 06:30 PM, Aneesh Kumar K.V wrote:
>>> This patch series includes fixes for debug_vm_pgtable test code so that
>>> they follow page table updates rules correctly. The first two patches introduce
>>> changes w.r.t ppc64. The patches are included in this series for completeness. We can
>>> merge them via ppc64 tree if required.
>>>
>>> Hugetlb test is disabled on ppc64 because that needs larger change to satisfy
>>> page table update rules.
>>>
>
>>
>> Changes proposed here will impact other enabled platforms as well.
>> Adding the following folks and mailing lists, and hoping to get a
>> broader review and test coverage. Please do include them in the
>> next iteration as well.
>>
>> + linux-arm-kernel@lists.infradead.org
>> + linux-s390@vger.kernel.org
>> + linux-snps-arc@lists.infradead.org
>> + x86@kernel.org
>> + linux-arch@vger.kernel.org
>>
>> + Gerald Schaefer <gerald.schaefer@de.ibm.com>
>> + Christophe Leroy <christophe.leroy@c-s.fr>
>
> Please don't use anymore the above address. Only use the one below.
>
>> + Christophe Leroy <christophe.leroy@csgroup.eu>
Sure, noted.
>> + Vineet Gupta <vgupta@synopsys.com>
>> + Mike Rapoport <rppt@linux.ibm.com>
>> + Qian Cai <cai@lca.pw>
>>
>
> Thanks
> Christophe
>
>
^ permalink raw reply
* Re: [PATCH] powerpc/mm: Remove DEBUG_VM_PGTABLE support on ppc64
From: Aneesh Kumar K.V @ 2020-09-01 9:13 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev, mpe; +Cc: Anshuman Khandual
In-Reply-To: <49fbd2b4-31af-826d-c884-32b7cc1c6e8d@csgroup.eu>
On 9/1/20 2:40 PM, Christophe Leroy wrote:
>
>
> Le 01/09/2020 à 10:15, Christophe Leroy a écrit :
>>
>>
>> Le 01/09/2020 à 10:12, Aneesh Kumar K.V a écrit :
>>> On 9/1/20 1:40 PM, Christophe Leroy wrote:
>>>>
>>>>
>>>> Le 01/09/2020 à 10:02, Aneesh Kumar K.V a écrit :
>>>>> The test is broken w.r.t page table update rules and results in kernel
>>>>> crash as below. Disable the support untill we get the tests updated.
>>>>>
>>>>>
>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>>
>>>> Any Fixes: tag ?
>>>>
>>>>> ---
>>>>> arch/powerpc/Kconfig | 1 -
>>>>> 1 file changed, 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>>>> index 65bed1fdeaad..787e829b6f25 100644
>>>>> --- a/arch/powerpc/Kconfig
>>>>> +++ b/arch/powerpc/Kconfig
>>>>> @@ -116,7 +116,6 @@ config PPC
>>>>> #
>>>>> select ARCH_32BIT_OFF_T if PPC32
>>>>> select ARCH_HAS_DEBUG_VIRTUAL
>>>>> - select ARCH_HAS_DEBUG_VM_PGTABLE
>>>>
>>>>
>>>> You say you remove support for ppc64 but you are removing it for
>>>> both PPC64 and PPC32. Should you replace the line by:
>>>
>>> Does it work on PPC32 with DEBUG_VM enabled? I was expecting it will
>>> be broken there too.
>>
>> I was wondering. I have just started a build to test that on my 8xx.
>> I'll tell you before noon (Paris).
>>
>
> There are warnings but it boots OK. So up to you, but if you deactivate
> it for both PPC32 and PPC64, say so in the subject like.
>
I will update the subject line to indicate powerpc instead of ppc64
-aneesh
^ permalink raw reply
* Re: [PATCH] arch: vdso: add vdso linker script to 'targets' instead of extra-y
From: Catalin Marinas @ 2020-09-01 9:19 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-s390, Vasily Gorbik, Nick Hu, linux-kbuild, Heiko Carstens,
linuxppc-dev, linux-kernel, Christian Borntraeger, Paul Mackerras,
Greentime Hu, Vincent Chen, Will Deacon, linux-arm-kernel
In-Reply-To: <20200831182239.480317-1-masahiroy@kernel.org>
On Tue, Sep 01, 2020 at 03:22:39AM +0900, Masahiro Yamada wrote:
> The vdso linker script is preprocessed on demand.
> Adding it to 'targets' is enough to include the .cmd file.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
For arm64:
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
^ permalink raw reply
* Re: [PATCH v3 09/13] mm/debug_vm_pgtable/locks: Move non page table modifying test together
From: Anshuman Khandual @ 2020-09-01 9:33 UTC (permalink / raw)
To: Aneesh Kumar K.V, linux-mm, akpm
Cc: linux-arch, linux-s390, Christophe Leroy, x86, Mike Rapoport,
Qian Cai, Gerald Schaefer, Vineet Gupta, linux-snps-arc,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <b6240372-4554-8c17-186a-cdc0b0a9089c@linux.ibm.com>
On 09/01/2020 12:08 PM, Aneesh Kumar K.V wrote:
> On 9/1/20 9:11 AM, Anshuman Khandual wrote:
>>
>>
>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>> This will help in adding proper locks in a later patch
>>
>> It really makes sense to classify these tests here as static and dynamic.
>> Static are the ones that test via page table entry values modification
>> without changing anything on the actual page table itself. Where as the
>> dynamic tests do change the page table. Static tests would probably never
>> require any lock synchronization but the dynamic ones will do.
>>
>>>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> ---
>>> mm/debug_vm_pgtable.c | 52 ++++++++++++++++++++++++-------------------
>>> 1 file changed, 29 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>> index 0ce5c6a24c5b..78c8af3445ac 100644
>>> --- a/mm/debug_vm_pgtable.c
>>> +++ b/mm/debug_vm_pgtable.c
>>> @@ -992,7 +992,7 @@ static int __init debug_vm_pgtable(void)
>>> p4dp = p4d_alloc(mm, pgdp, vaddr);
>>> pudp = pud_alloc(mm, p4dp, vaddr);
>>> pmdp = pmd_alloc(mm, pudp, vaddr);
>>> - ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
>>> + ptep = pte_alloc_map(mm, pmdp, vaddr);
>>> /*
>>> * Save all the page table page addresses as the page table
>>> @@ -1012,33 +1012,12 @@ static int __init debug_vm_pgtable(void)
>>> p4d_basic_tests(p4d_aligned, prot);
>>> pgd_basic_tests(pgd_aligned, prot);
>>> - pte_clear_tests(mm, ptep, vaddr);
>>> - pmd_clear_tests(mm, pmdp);
>>> - pud_clear_tests(mm, pudp);
>>> - p4d_clear_tests(mm, p4dp);
>>> - pgd_clear_tests(mm, pgdp);
>>> -
>>> - pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>>> - pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
>>> - pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
>>> - hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>>> -
>>> pmd_leaf_tests(pmd_aligned, prot);
>>> pud_leaf_tests(pud_aligned, prot);
>>> - pmd_huge_tests(pmdp, pmd_aligned, prot);
>>> - pud_huge_tests(pudp, pud_aligned, prot);
>>> -
>>> pte_savedwrite_tests(pte_aligned, protnone);
>>> pmd_savedwrite_tests(pmd_aligned, protnone);
>>> - pte_unmap_unlock(ptep, ptl);
>>> -
>>> - pmd_populate_tests(mm, pmdp, saved_ptep);
>>> - pud_populate_tests(mm, pudp, saved_pmdp);
>>> - p4d_populate_tests(mm, p4dp, saved_pudp);
>>> - pgd_populate_tests(mm, pgdp, saved_p4dp);
>>> -
>>> pte_special_tests(pte_aligned, prot);
>>> pte_protnone_tests(pte_aligned, protnone);
>>> pmd_protnone_tests(pmd_aligned, protnone);
>>> @@ -1056,11 +1035,38 @@ static int __init debug_vm_pgtable(void)
>>> pmd_swap_tests(pmd_aligned, prot);
>>> swap_migration_tests();
>>> - hugetlb_basic_tests(pte_aligned, prot);
>>> pmd_thp_tests(pmd_aligned, prot);
>>> pud_thp_tests(pud_aligned, prot);
>>> + /*
>>> + * Page table modifying tests
>>> + */
>>
>> In this comment, please do add some more details about how these tests
>> need proper locking mechanism unlike the previous static ones. Also add
>> a similar comment section for the static tests that dont really change
>> page table and need not require corresponding locking mechanism. Both
>> comment sections will make this classification clear.
>>
>>> + pte_clear_tests(mm, ptep, vaddr);
>>> + pmd_clear_tests(mm, pmdp);
>>> + pud_clear_tests(mm, pudp);
>>> + p4d_clear_tests(mm, p4dp);
>>> + pgd_clear_tests(mm, pgdp);
>>> +
>>> + ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
>>> + pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>>> + pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
>>> + pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
>>> + hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>>> +
>>> +
>>> + pmd_huge_tests(pmdp, pmd_aligned, prot);
>>> + pud_huge_tests(pudp, pud_aligned, prot);
>>> +
>>> + pte_unmap_unlock(ptep, ptl);
>>> +
>>> + pmd_populate_tests(mm, pmdp, saved_ptep);
>>> + pud_populate_tests(mm, pudp, saved_pmdp);
>>> + p4d_populate_tests(mm, p4dp, saved_pudp);
>>> + pgd_populate_tests(mm, pgdp, saved_p4dp);
>>> +
>>> + hugetlb_basic_tests(pte_aligned, prot);
>>
>> hugetlb_basic_tests() is a non page table modifying static test and
>> should be classified accordingly.
>>
>>> +
>>> p4d_free(mm, saved_p4dp);
>>> pud_free(mm, saved_pudp);
>>> pmd_free(mm, saved_pmdp);
>>>
>>
>> Changes till this patch fails to boot on arm64 platform. This should be
>> folded with the next patch.
>>
>> [ 17.080644] ------------[ cut here ]------------
>> [ 17.081342] kernel BUG at mm/pgtable-generic.c:164!
>> [ 17.082091] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>> [ 17.082977] Modules linked in:
>> [ 17.083481] CPU: 79 PID: 1 Comm: swapper/0 Tainted: G W 5.9.0-rc2-00105-g740e72cd6717 #14
>> [ 17.084998] Hardware name: linux,dummy-virt (DT)
>> [ 17.085745] pstate: 60400005 (nZCv daif +PAN -UAO BTYPE=--)
>> [ 17.086645] pc : pgtable_trans_huge_deposit+0x58/0x60
>> [ 17.087462] lr : debug_vm_pgtable+0x4dc/0x8f0
>> [ 17.088168] sp : ffff80001219bcf0
>> [ 17.088710] x29: ffff80001219bcf0 x28: ffff8000114ac000
>> [ 17.089574] x27: ffff8000114ac000 x26: 0020000000000fd3
>> [ 17.090431] x25: 0020000081400fd3 x24: fffffe00175c12c0
>> [ 17.091286] x23: ffff0005df04d1a8 x22: 0000f18d6e035000
>> [ 17.092143] x21: ffff0005dd790000 x20: ffff0005df18e1a8
>> [ 17.093003] x19: ffff0005df04cb80 x18: 0000000000000014
>> [ 17.093859] x17: 00000000b76667d0 x16: 00000000fd4e6611
>> [ 17.094716] x15: 0000000000000001 x14: 0000000000000002
>> [ 17.095572] x13: 000000000055d966 x12: fffffe001755e400
>> [ 17.096429] x11: 0000000000000008 x10: ffff0005fcb6e210
>> [ 17.097292] x9 : ffff0005fcb6e210 x8 : 0020000081590fd3
>> [ 17.098149] x7 : ffff0005dd71e0c0 x6 : ffff0005df18e1a8
>> [ 17.099006] x5 : 0020000081590fd3 x4 : 0020000081590fd3
>> [ 17.099862] x3 : ffff0005df18e1a8 x2 : fffffe00175c12c0
>> [ 17.100718] x1 : fffffe00175c1300 x0 : 0000000000000000
>> [ 17.101583] Call trace:
>> [ 17.101993] pgtable_trans_huge_deposit+0x58/0x60
>> [ 17.102754] do_one_initcall+0x74/0x1cc
>> [ 17.103381] kernel_init_freeable+0x1d0/0x238
>> [ 17.104089] kernel_init+0x14/0x118
>> [ 17.104658] ret_from_fork+0x10/0x34
>> [ 17.105251] Code: f9000443 f9000843 f9000822 d65f03c0 (d4210000)
>> [ 17.106303] ---[ end trace e63471e00f8c147f ]---
>>
>
> IIUC, this should happen even without the series right? This is
>
> assert_spin_locked(pmd_lockptr(mm, pmdp));
Crash does not happen without this series. A previous patch [PATCH v3 08/13]
added pgtable_trans_huge_deposit/withdraw() in pmd_advanced_tests(). arm64
does not define __HAVE_ARCH_PGTABLE_DEPOSIT and hence falls back on generic
pgtable_trans_huge_deposit() which the asserts the lock.
^ permalink raw reply
* Re: [PATCH v3 09/13] mm/debug_vm_pgtable/locks: Move non page table modifying test together
From: Aneesh Kumar K.V @ 2020-09-01 9:36 UTC (permalink / raw)
To: Anshuman Khandual, linux-mm, akpm
Cc: linux-arch, linux-s390, Christophe Leroy, x86, Mike Rapoport,
Qian Cai, Gerald Schaefer, Vineet Gupta, linux-snps-arc,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <9b75b175-f319-d40e-a95e-b323b3db654a@arm.com>
>>>
>>> [ 17.080644] ------------[ cut here ]------------
>>> [ 17.081342] kernel BUG at mm/pgtable-generic.c:164!
>>> [ 17.082091] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>>> [ 17.082977] Modules linked in:
>>> [ 17.083481] CPU: 79 PID: 1 Comm: swapper/0 Tainted: G W 5.9.0-rc2-00105-g740e72cd6717 #14
>>> [ 17.084998] Hardware name: linux,dummy-virt (DT)
>>> [ 17.085745] pstate: 60400005 (nZCv daif +PAN -UAO BTYPE=--)
>>> [ 17.086645] pc : pgtable_trans_huge_deposit+0x58/0x60
>>> [ 17.087462] lr : debug_vm_pgtable+0x4dc/0x8f0
>>> [ 17.088168] sp : ffff80001219bcf0
>>> [ 17.088710] x29: ffff80001219bcf0 x28: ffff8000114ac000
>>> [ 17.089574] x27: ffff8000114ac000 x26: 0020000000000fd3
>>> [ 17.090431] x25: 0020000081400fd3 x24: fffffe00175c12c0
>>> [ 17.091286] x23: ffff0005df04d1a8 x22: 0000f18d6e035000
>>> [ 17.092143] x21: ffff0005dd790000 x20: ffff0005df18e1a8
>>> [ 17.093003] x19: ffff0005df04cb80 x18: 0000000000000014
>>> [ 17.093859] x17: 00000000b76667d0 x16: 00000000fd4e6611
>>> [ 17.094716] x15: 0000000000000001 x14: 0000000000000002
>>> [ 17.095572] x13: 000000000055d966 x12: fffffe001755e400
>>> [ 17.096429] x11: 0000000000000008 x10: ffff0005fcb6e210
>>> [ 17.097292] x9 : ffff0005fcb6e210 x8 : 0020000081590fd3
>>> [ 17.098149] x7 : ffff0005dd71e0c0 x6 : ffff0005df18e1a8
>>> [ 17.099006] x5 : 0020000081590fd3 x4 : 0020000081590fd3
>>> [ 17.099862] x3 : ffff0005df18e1a8 x2 : fffffe00175c12c0
>>> [ 17.100718] x1 : fffffe00175c1300 x0 : 0000000000000000
>>> [ 17.101583] Call trace:
>>> [ 17.101993] pgtable_trans_huge_deposit+0x58/0x60
>>> [ 17.102754] do_one_initcall+0x74/0x1cc
>>> [ 17.103381] kernel_init_freeable+0x1d0/0x238
>>> [ 17.104089] kernel_init+0x14/0x118
>>> [ 17.104658] ret_from_fork+0x10/0x34
>>> [ 17.105251] Code: f9000443 f9000843 f9000822 d65f03c0 (d4210000)
>>> [ 17.106303] ---[ end trace e63471e00f8c147f ]---
>>>
>>
>> IIUC, this should happen even without the series right? This is
>>
>> assert_spin_locked(pmd_lockptr(mm, pmdp));
>
> Crash does not happen without this series. A previous patch [PATCH v3 08/13]
> added pgtable_trans_huge_deposit/withdraw() in pmd_advanced_tests(). arm64
> does not define __HAVE_ARCH_PGTABLE_DEPOSIT and hence falls back on generic
> pgtable_trans_huge_deposit() which the asserts the lock.
>
I fixed that by moving the pgtable deposit after adding the pmd locks
correctly.
mm/debug_vm_pgtable/locks: Move non page table modifying test together
mm/debug_vm_pgtable/locks: Take correct page table lock
mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP
-aneesh
^ permalink raw reply
* [PATCH v2] powerpc/mm: Remove DEBUG_VM_PGTABLE support on powerpc
From: Aneesh Kumar K.V @ 2020-09-01 9:44 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, Anshuman Khandual
The test is broken w.r.t page table update rules and results in kernel
crash as below. Disable the support until we get the tests updated.
[ 21.083519] kernel BUG at arch/powerpc/mm/pgtable.c:304!
cpu 0x0: Vector: 700 (Program Check) at [c000000c6d1e76c0]
pc: c00000000009a5ec: assert_pte_locked+0x14c/0x380
lr: c0000000005eeeec: pte_update+0x11c/0x190
sp: c000000c6d1e7950
msr: 8000000002029033
current = 0xc000000c6d172c80
paca = 0xc000000003ba0000 irqmask: 0x03 irq_happened: 0x01
pid = 1, comm = swapper/0
kernel BUG at arch/powerpc/mm/pgtable.c:304!
[link register ] c0000000005eeeec pte_update+0x11c/0x190
[c000000c6d1e7950] 0000000000000001 (unreliable)
[c000000c6d1e79b0] c0000000005eee14 pte_update+0x44/0x190
[c000000c6d1e7a10] c000000001a2ca9c pte_advanced_tests+0x160/0x3d8
[c000000c6d1e7ab0] c000000001a2d4fc debug_vm_pgtable+0x7e8/0x1338
[c000000c6d1e7ba0] c0000000000116ec do_one_initcall+0xac/0x5f0
[c000000c6d1e7c80] c0000000019e4fac kernel_init_freeable+0x4dc/0x5a4
[c000000c6d1e7db0] c000000000012474 kernel_init+0x24/0x160
[c000000c6d1e7e20] c00000000000cbd0 ret_from_kernel_thread+0x5c/0x6c
With DEBUG_VM disabled
[ 20.530152] BUG: Kernel NULL pointer dereference on read at 0x00000000
[ 20.530183] Faulting instruction address: 0xc0000000000df330
cpu 0x33: Vector: 380 (Data SLB Access) at [c000000c6d19f700]
pc: c0000000000df330: memset+0x68/0x104
lr: c00000000009f6d8: hash__pmdp_huge_get_and_clear+0xe8/0x1b0
sp: c000000c6d19f990
msr: 8000000002009033
dar: 0
current = 0xc000000c6d177480
paca = 0xc00000001ec4f400 irqmask: 0x03 irq_happened: 0x01
pid = 1, comm = swapper/0
[link register ] c00000000009f6d8 hash__pmdp_huge_get_and_clear+0xe8/0x1b0
[c000000c6d19f990] c00000000009f748 hash__pmdp_huge_get_and_clear+0x158/0x1b0 (unreliable)
[c000000c6d19fa10] c0000000019ebf30 pmd_advanced_tests+0x1f0/0x378
[c000000c6d19fab0] c0000000019ed088 debug_vm_pgtable+0x79c/0x1244
[c000000c6d19fba0] c0000000000116ec do_one_initcall+0xac/0x5f0
[c000000c6d19fc80] c0000000019a4fac kernel_init_freeable+0x4dc/0x5a4
[c000000c6d19fdb0] c000000000012474 kernel_init+0x24/0x160
[c000000c6d19fe20] c00000000000cbd0 ret_from_kernel_thread+0x5c/0x6c
33:mon>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/Kconfig | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 65bed1fdeaad..787e829b6f25 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -116,7 +116,6 @@ config PPC
#
select ARCH_32BIT_OFF_T if PPC32
select ARCH_HAS_DEBUG_VIRTUAL
- select ARCH_HAS_DEBUG_VM_PGTABLE
select ARCH_HAS_DEVMEM_IS_ALLOWED
select ARCH_HAS_ELF_RANDOMIZE
select ARCH_HAS_FORTIFY_SOURCE
--
2.26.2
^ permalink raw reply related
* Re: [PATCH v3 13/13] mm/debug_vm_pgtable: populate a pte entry before fetching it
From: Aneesh Kumar K.V @ 2020-09-01 9:58 UTC (permalink / raw)
To: Anshuman Khandual, linux-mm, akpm
Cc: linux-arch, linux-s390, Christophe Leroy, x86, Mike Rapoport,
Qian Cai, Gerald Schaefer, Vineet Gupta, linux-snps-arc,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <e3140b44-993e-aa4b-130d-ee2230eff2b5@arm.com>
On 9/1/20 1:08 PM, Anshuman Khandual wrote:
>
>
> On 09/01/2020 12:07 PM, Aneesh Kumar K.V wrote:
>> On 9/1/20 8:55 AM, Anshuman Khandual wrote:
>>>
>>>
>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>>> pte_clear_tests operate on an existing pte entry. Make sure that is not a none
>>>> pte entry.
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>> ---
>>>> mm/debug_vm_pgtable.c | 6 ++++--
>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>> index 21329c7d672f..8527ebb75f2c 100644
>>>> --- a/mm/debug_vm_pgtable.c
>>>> +++ b/mm/debug_vm_pgtable.c
>>>> @@ -546,7 +546,7 @@ static void __init pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp,
>>>> static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep,
>>>> unsigned long vaddr)
>>>> {
>>>> - pte_t pte = ptep_get(ptep);
>>>> + pte_t pte = ptep_get_and_clear(mm, vaddr, ptep);
>>>
>>> Seems like ptep_get_and_clear() here just clears the entry in preparation
>>> for a following set_pte_at() which otherwise would have been a problem on
>>> ppc64 as you had pointed out earlier i.e set_pte_at() should not update an
>>> existing valid entry. So the commit message here is bit misleading.
>>>
>>
>> and also fetch the pte value which is used further.
>>
>>
>>>> pr_debug("Validating PTE clear\n");
>>>> pte = __pte(pte_val(pte) | RANDOM_ORVALUE);
>>>> @@ -944,7 +944,7 @@ static int __init debug_vm_pgtable(void)
>>>> p4d_t *p4dp, *saved_p4dp;
>>>> pud_t *pudp, *saved_pudp;
>>>> pmd_t *pmdp, *saved_pmdp, pmd;
>>>> - pte_t *ptep;
>>>> + pte_t *ptep, pte;
>>>> pgtable_t saved_ptep;
>>>> pgprot_t prot, protnone;
>>>> phys_addr_t paddr;
>>>> @@ -1049,6 +1049,8 @@ static int __init debug_vm_pgtable(void)
>>>> */
>>>> ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
>>>> + pte = pfn_pte(pte_aligned, prot);
>>>> + set_pte_at(mm, vaddr, ptep, pte);
>>>
>>> Not here, creating and populating an entry must be done in respective
>>> test functions itself. Besides, this seems bit redundant as well. The
>>> test pte_clear_tests() with the above change added, already
>>>
>>> - Clears the PTEP entry with ptep_get_and_clear()
>>
>> and fetch the old value set previously.
>
> In that case, please move above two lines i.e
>
> pte = pfn_pte(pte_aligned, prot);
> set_pte_at(mm, vaddr, ptep, pte);
>
> from debug_vm_pgtable() to pte_clear_tests() and update it's arguments
> as required.
>
Frankly, I don't understand what these tests are testing. It all looks
like some random clear and set.
static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep,
unsigned long vaddr, unsigned long pfn,
pgprot_t prot)
{
pte_t pte = pfn_pte(pfn, prot);
set_pte_at(mm, vaddr, ptep, pte);
pte = ptep_get_and_clear(mm, vaddr, ptep);
pr_debug("Validating PTE clear\n");
pte = __pte(pte_val(pte) | RANDOM_ORVALUE);
set_pte_at(mm, vaddr, ptep, pte);
barrier();
pte_clear(mm, vaddr, ptep);
pte = ptep_get(ptep);
WARN_ON(!pte_none(pte));
}
-aneesh
^ permalink raw reply
* Re: [PATCH v3 09/13] mm/debug_vm_pgtable/locks: Move non page table modifying test together
From: Anshuman Khandual @ 2020-09-01 9:58 UTC (permalink / raw)
To: Aneesh Kumar K.V, linux-mm, akpm
Cc: linux-arch, linux-s390, Christophe Leroy, x86, Mike Rapoport,
Qian Cai, Gerald Schaefer, Vineet Gupta, linux-snps-arc,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <58a5280f-4882-4a36-c52d-15ad879209d6@linux.ibm.com>
On 09/01/2020 03:06 PM, Aneesh Kumar K.V wrote:
>
>>>>
>>>> [ 17.080644] ------------[ cut here ]------------
>>>> [ 17.081342] kernel BUG at mm/pgtable-generic.c:164!
>>>> [ 17.082091] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>>>> [ 17.082977] Modules linked in:
>>>> [ 17.083481] CPU: 79 PID: 1 Comm: swapper/0 Tainted: G W 5.9.0-rc2-00105-g740e72cd6717 #14
>>>> [ 17.084998] Hardware name: linux,dummy-virt (DT)
>>>> [ 17.085745] pstate: 60400005 (nZCv daif +PAN -UAO BTYPE=--)
>>>> [ 17.086645] pc : pgtable_trans_huge_deposit+0x58/0x60
>>>> [ 17.087462] lr : debug_vm_pgtable+0x4dc/0x8f0
>>>> [ 17.088168] sp : ffff80001219bcf0
>>>> [ 17.088710] x29: ffff80001219bcf0 x28: ffff8000114ac000
>>>> [ 17.089574] x27: ffff8000114ac000 x26: 0020000000000fd3
>>>> [ 17.090431] x25: 0020000081400fd3 x24: fffffe00175c12c0
>>>> [ 17.091286] x23: ffff0005df04d1a8 x22: 0000f18d6e035000
>>>> [ 17.092143] x21: ffff0005dd790000 x20: ffff0005df18e1a8
>>>> [ 17.093003] x19: ffff0005df04cb80 x18: 0000000000000014
>>>> [ 17.093859] x17: 00000000b76667d0 x16: 00000000fd4e6611
>>>> [ 17.094716] x15: 0000000000000001 x14: 0000000000000002
>>>> [ 17.095572] x13: 000000000055d966 x12: fffffe001755e400
>>>> [ 17.096429] x11: 0000000000000008 x10: ffff0005fcb6e210
>>>> [ 17.097292] x9 : ffff0005fcb6e210 x8 : 0020000081590fd3
>>>> [ 17.098149] x7 : ffff0005dd71e0c0 x6 : ffff0005df18e1a8
>>>> [ 17.099006] x5 : 0020000081590fd3 x4 : 0020000081590fd3
>>>> [ 17.099862] x3 : ffff0005df18e1a8 x2 : fffffe00175c12c0
>>>> [ 17.100718] x1 : fffffe00175c1300 x0 : 0000000000000000
>>>> [ 17.101583] Call trace:
>>>> [ 17.101993] pgtable_trans_huge_deposit+0x58/0x60
>>>> [ 17.102754] do_one_initcall+0x74/0x1cc
>>>> [ 17.103381] kernel_init_freeable+0x1d0/0x238
>>>> [ 17.104089] kernel_init+0x14/0x118
>>>> [ 17.104658] ret_from_fork+0x10/0x34
>>>> [ 17.105251] Code: f9000443 f9000843 f9000822 d65f03c0 (d4210000)
>>>> [ 17.106303] ---[ end trace e63471e00f8c147f ]---
>>>>
>>>
>>> IIUC, this should happen even without the series right? This is
>>>
>>> assert_spin_locked(pmd_lockptr(mm, pmdp));
>>
>> Crash does not happen without this series. A previous patch [PATCH v3 08/13]
>> added pgtable_trans_huge_deposit/withdraw() in pmd_advanced_tests(). arm64
>> does not define __HAVE_ARCH_PGTABLE_DEPOSIT and hence falls back on generic
>> pgtable_trans_huge_deposit() which the asserts the lock.
>>
>
>
> I fixed that by moving the pgtable deposit after adding the pmd locks correctly.
>
> mm/debug_vm_pgtable/locks: Move non page table modifying test together
> mm/debug_vm_pgtable/locks: Take correct page table lock
> mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP
Right, it does fix. But then both those patches should be folded/merged in
order to preserve git bisect ability, besides test classification reasons
as mentioned in a previous response and a possible redundant movement of
hugetlb_basic_tests().
^ 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