Hi again, folks,
This is version two of the patch series I posted yesterday:
https://lore.kernel.org/r/[email protected]
The only changes since v1 are:
* Squash patches 2 and 3, amending the commit message accordingly
* Add Reviewed-by and Tested-by tags from Christoph and Claire (thanks!)
I'd usually leave it a bit longer between postings, but since this fixes
issues with patches in -next I thought I'd spin a new version immediately.
Cheers,
Will
Cc: Guenter Roeck <[email protected]>
Cc: Claire Chang <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Cc: Nathan Chancellor <[email protected]>
--->8
Will Deacon (4):
of: Return success from of_dma_set_restricted_buffer() when
!OF_ADDRESS
swiotlb: Convert io_default_tlb_mem to static allocation
swiotlb: Emit diagnostic in swiotlb_exit()
swiotlb: Free tbl memory in swiotlb_exit()
drivers/base/core.c | 2 +-
drivers/of/of_private.h | 3 +-
drivers/xen/swiotlb-xen.c | 4 +-
include/linux/swiotlb.h | 4 +-
kernel/dma/swiotlb.c | 82 +++++++++++++++++++++++----------------
5 files changed, 56 insertions(+), 39 deletions(-)
--
2.32.0.402.g57bb445576-goog
When CONFIG_OF_ADDRESS=n, of_dma_set_restricted_buffer() returns -ENODEV
and breaks the boot for sparc[64] machines. Return 0 instead, since the
function is essentially a glorified NOP in this configuration.
Cc: Claire Chang <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Reported-by: Guenter Roeck <[email protected]>
Suggested-by: Robin Murphy <[email protected]>
Tested-by: Guenter Roeck <[email protected]>
Tested-by: Claire Chang <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Fixes: fec9b625095f ("of: Add plumbing for restricted DMA pool")
Signed-off-by: Will Deacon <[email protected]>
---
drivers/of/of_private.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 376462798f7e..f557bd22b0cf 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -173,7 +173,8 @@ static inline int of_dma_get_range(struct device_node *np,
static inline int of_dma_set_restricted_buffer(struct device *dev,
struct device_node *np)
{
- return -ENODEV;
+ /* Do nothing, successfully. */
+ return 0;
}
#endif
--
2.32.0.402.g57bb445576-goog
Although swiotlb_exit() frees the 'slots' metadata array referenced by
'io_tlb_default_mem', it leaves the underlying buffer pages allocated
despite no longer being usable.
Extend swiotlb_exit() to free the buffer pages as well as the slots
array.
Cc: Claire Chang <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Tested-by: Nathan Chancellor <[email protected]>
Tested-by: Claire Chang <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
kernel/dma/swiotlb.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index b3c793ed9e64..87c40517e822 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -328,18 +328,27 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
void __init swiotlb_exit(void)
{
- size_t size;
struct io_tlb_mem *mem = &io_tlb_default_mem;
+ unsigned long tbl_vaddr;
+ size_t tbl_size, slots_size;
if (!mem->nslabs)
return;
pr_info("tearing down default memory pool\n");
- size = array_size(sizeof(*mem->slots), mem->nslabs);
- if (mem->late_alloc)
- free_pages((unsigned long)mem->slots, get_order(size));
- else
- memblock_free_late(__pa(mem->slots), PAGE_ALIGN(size));
+ tbl_vaddr = (unsigned long)phys_to_virt(mem->start);
+ tbl_size = PAGE_ALIGN(mem->end - mem->start);
+ slots_size = PAGE_ALIGN(array_size(sizeof(*mem->slots), mem->nslabs));
+
+ set_memory_encrypted(tbl_vaddr, tbl_size >> PAGE_SHIFT);
+ if (mem->late_alloc) {
+ free_pages(tbl_vaddr, get_order(tbl_size));
+ free_pages((unsigned long)mem->slots, get_order(slots_size));
+ } else {
+ memblock_free_late(mem->start, tbl_size);
+ memblock_free_late(__pa(mem->slots), slots_size);
+ }
+
memset(mem, 0, sizeof(*mem));
}
--
2.32.0.402.g57bb445576-goog
A recent debugging session would have been made a little bit easier if
we had noticed sooner that swiotlb_exit() was being called during boot.
Add a simple diagnostic message to swiotlb_exit() to complement the one
from swiotlb_print_info() during initialisation.
Cc: Claire Chang <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Robin Murphy <[email protected]>
Link: https://lore.kernel.org/r/20210705190352.GA19461@willie-the-truck
Suggested-by: Konrad Rzeszutek Wilk <[email protected]>
Tested-by: Nathan Chancellor <[email protected]>
Tested-by: Claire Chang <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
kernel/dma/swiotlb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 7948f274f9bb..b3c793ed9e64 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -334,6 +334,7 @@ void __init swiotlb_exit(void)
if (!mem->nslabs)
return;
+ pr_info("tearing down default memory pool\n");
size = array_size(sizeof(*mem->slots), mem->nslabs);
if (mem->late_alloc)
free_pages((unsigned long)mem->slots, get_order(size));
--
2.32.0.402.g57bb445576-goog
Since commit 69031f500865 ("swiotlb: Set dev->dma_io_tlb_mem to the
swiotlb pool used"), 'struct device' may hold a copy of the global
'io_default_tlb_mem' pointer if the device is using swiotlb for DMA. A
subsequent call to swiotlb_exit() will therefore leave dangling pointers
behind in these device structures, resulting in KASAN splats such as:
| BUG: KASAN: use-after-free in __iommu_dma_unmap_swiotlb+0x64/0xb0
| Read of size 8 at addr ffff8881d7830000 by task swapper/0/0
|
| CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.12.0-rc3-debug #1
| Hardware name: HP HP Desktop M01-F1xxx/87D6, BIOS F.12 12/17/2020
| Call Trace:
| <IRQ>
| dump_stack+0x9c/0xcf
| print_address_description.constprop.0+0x18/0x130
| kasan_report.cold+0x7f/0x111
| __iommu_dma_unmap_swiotlb+0x64/0xb0
| nvme_pci_complete_rq+0x73/0x130
| blk_complete_reqs+0x6f/0x80
| __do_softirq+0xfc/0x3be
Convert 'io_default_tlb_mem' to a static structure, so that the
per-device pointers remain valid after swiotlb_exit() has been invoked.
All users are updated to reference the static structure directly, using
the 'nslabs' field to determine whether swiotlb has been initialised.
The 'slots' array is still allocated dynamically and referenced via a
pointer rather than a flexible array member.
Cc: Claire Chang <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Fixes: 69031f500865 ("swiotlb: Set dev->dma_io_tlb_mem to the swiotlb pool used")
Reported-by: Nathan Chancellor <[email protected]>
Tested-by: Nathan Chancellor <[email protected]>
Tested-by: Claire Chang <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
drivers/base/core.c | 2 +-
drivers/xen/swiotlb-xen.c | 4 +--
include/linux/swiotlb.h | 4 +--
kernel/dma/swiotlb.c | 66 +++++++++++++++++++++------------------
4 files changed, 41 insertions(+), 35 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index ea5b85354526..b49824001cfa 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2848,7 +2848,7 @@ void device_initialize(struct device *dev)
dev->dma_coherent = dma_default_coherent;
#endif
#ifdef CONFIG_SWIOTLB
- dev->dma_io_tlb_mem = io_tlb_default_mem;
+ dev->dma_io_tlb_mem = &io_tlb_default_mem;
#endif
}
EXPORT_SYMBOL_GPL(device_initialize);
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 785ec7e8be01..f06d9b4f1e0f 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -164,7 +164,7 @@ int __ref xen_swiotlb_init(void)
int rc = -ENOMEM;
char *start;
- if (io_tlb_default_mem != NULL) {
+ if (io_tlb_default_mem.nslabs) {
pr_warn("swiotlb buffer already initialized\n");
return -EEXIST;
}
@@ -547,7 +547,7 @@ xen_swiotlb_sync_sg_for_device(struct device *dev, struct scatterlist *sgl,
static int
xen_swiotlb_dma_supported(struct device *hwdev, u64 mask)
{
- return xen_phys_to_dma(hwdev, io_tlb_default_mem->end - 1) <= mask;
+ return xen_phys_to_dma(hwdev, io_tlb_default_mem.end - 1) <= mask;
}
const struct dma_map_ops xen_swiotlb_dma_ops = {
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index 39284ff2a6cd..b0cb2a9973f4 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -103,9 +103,9 @@ struct io_tlb_mem {
phys_addr_t orig_addr;
size_t alloc_size;
unsigned int list;
- } slots[];
+ } *slots;
};
-extern struct io_tlb_mem *io_tlb_default_mem;
+extern struct io_tlb_mem io_tlb_default_mem;
static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr)
{
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index f1a9ae7fad8f..7948f274f9bb 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -70,7 +70,7 @@
enum swiotlb_force swiotlb_force;
-struct io_tlb_mem *io_tlb_default_mem;
+struct io_tlb_mem io_tlb_default_mem;
/*
* Max segment that we can provide which (if pages are contingous) will
@@ -101,7 +101,7 @@ early_param("swiotlb", setup_io_tlb_npages);
unsigned int swiotlb_max_segment(void)
{
- return io_tlb_default_mem ? max_segment : 0;
+ return io_tlb_default_mem.nslabs ? max_segment : 0;
}
EXPORT_SYMBOL_GPL(swiotlb_max_segment);
@@ -134,9 +134,9 @@ void __init swiotlb_adjust_size(unsigned long size)
void swiotlb_print_info(void)
{
- struct io_tlb_mem *mem = io_tlb_default_mem;
+ struct io_tlb_mem *mem = &io_tlb_default_mem;
- if (!mem) {
+ if (!mem->nslabs) {
pr_warn("No low mem\n");
return;
}
@@ -163,11 +163,11 @@ static inline unsigned long nr_slots(u64 val)
*/
void __init swiotlb_update_mem_attributes(void)
{
- struct io_tlb_mem *mem = io_tlb_default_mem;
+ struct io_tlb_mem *mem = &io_tlb_default_mem;
void *vaddr;
unsigned long bytes;
- if (!mem || mem->late_alloc)
+ if (!mem->nslabs || mem->late_alloc)
return;
vaddr = phys_to_virt(mem->start);
bytes = PAGE_ALIGN(mem->nslabs << IO_TLB_SHIFT);
@@ -201,25 +201,24 @@ static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t start,
int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
{
- struct io_tlb_mem *mem;
+ struct io_tlb_mem *mem = &io_tlb_default_mem;
size_t alloc_size;
if (swiotlb_force == SWIOTLB_NO_FORCE)
return 0;
/* protect against double initialization */
- if (WARN_ON_ONCE(io_tlb_default_mem))
+ if (WARN_ON_ONCE(mem->nslabs))
return -ENOMEM;
- alloc_size = PAGE_ALIGN(struct_size(mem, slots, nslabs));
- mem = memblock_alloc(alloc_size, PAGE_SIZE);
- if (!mem)
+ alloc_size = PAGE_ALIGN(array_size(sizeof(*mem->slots), nslabs));
+ mem->slots = memblock_alloc(alloc_size, PAGE_SIZE);
+ if (!mem->slots)
panic("%s: Failed to allocate %zu bytes align=0x%lx\n",
__func__, alloc_size, PAGE_SIZE);
swiotlb_init_io_tlb_mem(mem, __pa(tlb), nslabs, false);
- io_tlb_default_mem = mem;
if (verbose)
swiotlb_print_info();
swiotlb_set_max_segment(mem->nslabs << IO_TLB_SHIFT);
@@ -304,26 +303,24 @@ swiotlb_late_init_with_default_size(size_t default_size)
int
swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
{
- struct io_tlb_mem *mem;
+ struct io_tlb_mem *mem = &io_tlb_default_mem;
unsigned long bytes = nslabs << IO_TLB_SHIFT;
if (swiotlb_force == SWIOTLB_NO_FORCE)
return 0;
/* protect against double initialization */
- if (WARN_ON_ONCE(io_tlb_default_mem))
+ if (WARN_ON_ONCE(mem->nslabs))
return -ENOMEM;
- mem = (void *)__get_free_pages(GFP_KERNEL,
- get_order(struct_size(mem, slots, nslabs)));
- if (!mem)
+ mem->slots = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
+ get_order(array_size(sizeof(*mem->slots), nslabs)));
+ if (!mem->slots)
return -ENOMEM;
- memset(mem, 0, sizeof(*mem));
set_memory_decrypted((unsigned long)tlb, bytes >> PAGE_SHIFT);
swiotlb_init_io_tlb_mem(mem, virt_to_phys(tlb), nslabs, true);
- io_tlb_default_mem = mem;
swiotlb_print_info();
swiotlb_set_max_segment(mem->nslabs << IO_TLB_SHIFT);
return 0;
@@ -331,18 +328,18 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
void __init swiotlb_exit(void)
{
- struct io_tlb_mem *mem = io_tlb_default_mem;
size_t size;
+ struct io_tlb_mem *mem = &io_tlb_default_mem;
- if (!mem)
+ if (!mem->nslabs)
return;
- size = struct_size(mem, slots, mem->nslabs);
+ size = array_size(sizeof(*mem->slots), mem->nslabs);
if (mem->late_alloc)
- free_pages((unsigned long)mem, get_order(size));
+ free_pages((unsigned long)mem->slots, get_order(size));
else
- memblock_free_late(__pa(mem), PAGE_ALIGN(size));
- io_tlb_default_mem = NULL;
+ memblock_free_late(__pa(mem->slots), PAGE_ALIGN(size));
+ memset(mem, 0, sizeof(*mem));
}
/*
@@ -696,7 +693,9 @@ size_t swiotlb_max_mapping_size(struct device *dev)
bool is_swiotlb_active(struct device *dev)
{
- return dev->dma_io_tlb_mem != NULL;
+ struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
+
+ return mem && mem->nslabs;
}
EXPORT_SYMBOL_GPL(is_swiotlb_active);
@@ -711,10 +710,10 @@ static void swiotlb_create_debugfs_files(struct io_tlb_mem *mem)
static int __init swiotlb_create_default_debugfs(void)
{
- struct io_tlb_mem *mem = io_tlb_default_mem;
+ struct io_tlb_mem *mem = &io_tlb_default_mem;
debugfs_dir = debugfs_create_dir("swiotlb", NULL);
- if (mem) {
+ if (mem->nslabs) {
mem->debugfs = debugfs_dir;
swiotlb_create_debugfs_files(mem);
}
@@ -783,10 +782,17 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
* to it.
*/
if (!mem) {
- mem = kzalloc(struct_size(mem, slots, nslabs), GFP_KERNEL);
+ mem = kzalloc(sizeof(*mem), GFP_KERNEL);
if (!mem)
return -ENOMEM;
+ mem->slots = kzalloc(array_size(sizeof(*mem->slots), nslabs),
+ GFP_KERNEL);
+ if (!mem->slots) {
+ kfree(mem);
+ return -ENOMEM;
+ }
+
set_memory_decrypted((unsigned long)phys_to_virt(rmem->base),
rmem->size >> PAGE_SHIFT);
swiotlb_init_io_tlb_mem(mem, rmem->base, nslabs, false);
@@ -806,7 +812,7 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
static void rmem_swiotlb_device_release(struct reserved_mem *rmem,
struct device *dev)
{
- dev->dma_io_tlb_mem = io_tlb_default_mem;
+ dev->dma_io_tlb_mem = &io_tlb_default_mem;
}
static const struct reserved_mem_ops rmem_swiotlb_ops = {
--
2.32.0.402.g57bb445576-goog
On 20.07.21 15:38, Will Deacon wrote:
> Hi again, folks,
>
> This is version two of the patch series I posted yesterday:
>
> https://lore.kernel.org/r/[email protected]
>
> The only changes since v1 are:
>
> * Squash patches 2 and 3, amending the commit message accordingly
> * Add Reviewed-by and Tested-by tags from Christoph and Claire (thanks!)
>
> I'd usually leave it a bit longer between postings, but since this fixes
> issues with patches in -next I thought I'd spin a new version immediately.
>
> Cheers,
FWIW, I just bisected virtio-errors with secure execution mode
qemu-system-s390x: virtio-serial-bus: Unexpected port id 4205794771 for device virtio-serial0.0
to
commit 903cd0f315fe426c6a64c54ed389de0becb663dc
Author: Claire Chang <[email protected]>
Date: Thu Jun 24 23:55:20 2021 +0800
swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
Unfortunately this patch series does NOT fix this issue, so it seems that even more
things are broken.
Any idea what else might be broken?
Shall we rather revert these patches from next until we have things under control?
On Thu, 22 Jul 2021 21:22:58 +0200
Christian Borntraeger <[email protected]> wrote:
> On 20.07.21 15:38, Will Deacon wrote:
> > Hi again, folks,
> >
> > This is version two of the patch series I posted yesterday:
> >
> > https://lore.kernel.org/r/[email protected]
> >
> > The only changes since v1 are:
> >
> > * Squash patches 2 and 3, amending the commit message accordingly
> > * Add Reviewed-by and Tested-by tags from Christoph and Claire (thanks!)
> >
> > I'd usually leave it a bit longer between postings, but since this fixes
> > issues with patches in -next I thought I'd spin a new version immediately.
> >
> > Cheers,
>
> FWIW, I just bisected virtio-errors with secure execution mode
> qemu-system-s390x: virtio-serial-bus: Unexpected port id 4205794771 for device virtio-serial0.0
>
> to
> commit 903cd0f315fe426c6a64c54ed389de0becb663dc
> Author: Claire Chang <[email protected]>
> Date: Thu Jun 24 23:55:20 2021 +0800
>
> swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
>
> Unfortunately this patch series does NOT fix this issue, so it seems that even more
> things are broken.
>
> Any idea what else might be broken?
I've done some debugging, and I think I know what is going on. Since
that commit we need to set force_swiotlb before the swiotlb itself is
initialized. So the patch below should fix the problem.
--------------------8<-------------------------------------
From: Halil Pasic <[email protected]>
Date: Fri, 23 Jul 2021 02:57:06 +0200
Subject: [PATCH 1/1] s390/pv: fix the forcing of the swiotlb
Since commit 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for
swiotlb data bouncing") if code sets swiotlb_force it needs to do so
before the swiotlb is initialised. Otherwise
io_tlb_default_mem->force_bounce will not get set to true, and devices
that use (the default) swiotlb will not bounce despite switolb_force
having the value of SWIOTLB_FORCE.
Let us restore swiotlb functionality for PV by fulfilling this new
requirement.
Signed-off-by: Halil Pasic <[email protected]>
---
arch/s390/mm/init.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 8ac710de1ab1..07bbee9b7320 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -186,9 +186,9 @@ static void pv_init(void)
return;
/* make sure bounce buffers are shared */
+ swiotlb_force = SWIOTLB_FORCE;
swiotlb_init(1);
swiotlb_update_mem_attributes();
- swiotlb_force = SWIOTLB_FORCE;
}
void __init mem_init(void)
--
2.29.2
On 23.07.21 03:12, Halil Pasic wrote:
> On Thu, 22 Jul 2021 21:22:58 +0200
> Christian Borntraeger <[email protected]> wrote:
>
>> On 20.07.21 15:38, Will Deacon wrote:
>>> Hi again, folks,
>>>
>>> This is version two of the patch series I posted yesterday:
>>>
>>> https://lore.kernel.org/r/[email protected]
>>>
>>> The only changes since v1 are:
>>>
>>> * Squash patches 2 and 3, amending the commit message accordingly
>>> * Add Reviewed-by and Tested-by tags from Christoph and Claire (thanks!)
>>>
>>> I'd usually leave it a bit longer between postings, but since this fixes
>>> issues with patches in -next I thought I'd spin a new version immediately.
>>>
>>> Cheers,
>>
>> FWIW, I just bisected virtio-errors with secure execution mode
>> qemu-system-s390x: virtio-serial-bus: Unexpected port id 4205794771 for device virtio-serial0.0
>>
>> to
>> commit 903cd0f315fe426c6a64c54ed389de0becb663dc
>> Author: Claire Chang <[email protected]>
>> Date: Thu Jun 24 23:55:20 2021 +0800
>>
>> swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
>>
>> Unfortunately this patch series does NOT fix this issue, so it seems that even more
>> things are broken.
>>
>> Any idea what else might be broken?
>
> I've done some debugging, and I think I know what is going on. Since
> that commit we need to set force_swiotlb before the swiotlb itself is
> initialized. So the patch below should fix the problem.
>
> --------------------8<-------------------------------------
>
> From: Halil Pasic <[email protected]>
> Date: Fri, 23 Jul 2021 02:57:06 +0200
> Subject: [PATCH 1/1] s390/pv: fix the forcing of the swiotlb
>
> Since commit 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for
> swiotlb data bouncing") if code sets swiotlb_force it needs to do so
> before the swiotlb is initialised. Otherwise
> io_tlb_default_mem->force_bounce will not get set to true, and devices
> that use (the default) swiotlb will not bounce despite switolb_force
> having the value of SWIOTLB_FORCE.
>
> Let us restore swiotlb functionality for PV by fulfilling this new
> requirement.
>
I would add:
Fixes: 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing")
as this patch breaks things
and
Fixes: 64e1f0c531d1 ("s390/mm: force swiotlb for protected virtualization")
to make the s390 init code more robust in case people start backporting things.
> Signed-off-by: Halil Pasic <[email protected]>
I can confirm that this fixes the problem. This also makes sense codewise.
Tested-by: Christian Borntraeger <[email protected]>
Reviewed-by: Christian Borntraeger <[email protected]>
Konrad, Heiko, Vasily, any preference which tree this goes? I think s390
would be easiest, but that requires that the patches in the swiotlb tree have
fixed commit IDs.
> ---
> arch/s390/mm/init.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 8ac710de1ab1..07bbee9b7320 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -186,9 +186,9 @@ static void pv_init(void)
> return;
>
> /* make sure bounce buffers are shared */
> + swiotlb_force = SWIOTLB_FORCE;
> swiotlb_init(1);
> swiotlb_update_mem_attributes();
> - swiotlb_force = SWIOTLB_FORCE;
> }
>
> void __init mem_init(void)
>
Resending with the correct email of Heiko....
On 23.07.21 03:12, Halil Pasic wrote:
> On Thu, 22 Jul 2021 21:22:58 +0200
> Christian Borntraeger <[email protected]> wrote:
>
>> On 20.07.21 15:38, Will Deacon wrote:
>>> Hi again, folks,
>>>
>>> This is version two of the patch series I posted yesterday:
>>>
>>> https://lore.kernel.org/r/[email protected]
>>>
>>> The only changes since v1 are:
>>>
>>> * Squash patches 2 and 3, amending the commit message accordingly
>>> * Add Reviewed-by and Tested-by tags from Christoph and Claire (thanks!)
>>>
>>> I'd usually leave it a bit longer between postings, but since this fixes
>>> issues with patches in -next I thought I'd spin a new version immediately.
>>>
>>> Cheers,
>>
>> FWIW, I just bisected virtio-errors with secure execution mode
>> qemu-system-s390x: virtio-serial-bus: Unexpected port id 4205794771 for device virtio-serial0.0
>>
>> to
>> commit 903cd0f315fe426c6a64c54ed389de0becb663dc
>> Author: Claire Chang <[email protected]>
>> Date: Thu Jun 24 23:55:20 2021 +0800
>>
>> swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
>>
>> Unfortunately this patch series does NOT fix this issue, so it seems that even more
>> things are broken.
>>
>> Any idea what else might be broken?
>
> I've done some debugging, and I think I know what is going on. Since
> that commit we need to set force_swiotlb before the swiotlb itself is
> initialized. So the patch below should fix the problem.
>
> --------------------8<-------------------------------------
>
> From: Halil Pasic <[email protected]>
> Date: Fri, 23 Jul 2021 02:57:06 +0200
> Subject: [PATCH 1/1] s390/pv: fix the forcing of the swiotlb
>
> Since commit 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for
> swiotlb data bouncing") if code sets swiotlb_force it needs to do so
> before the swiotlb is initialised. Otherwise
> io_tlb_default_mem->force_bounce will not get set to true, and devices
> that use (the default) swiotlb will not bounce despite switolb_force
> having the value of SWIOTLB_FORCE.
>
> Let us restore swiotlb functionality for PV by fulfilling this new
> requirement.
>
I would add:
Fixes: 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing")
as this patch breaks things
and
Fixes: 64e1f0c531d1 ("s390/mm: force swiotlb for protected virtualization")
to make the s390 init code more robust in case people start backporting things.
> Signed-off-by: Halil Pasic <[email protected]>
I can confirm that this fixes the problem. This also makes sense codewise.
Tested-by: Christian Borntraeger <[email protected]>
Reviewed-by: Christian Borntraeger <[email protected]>
Konrad, Heiko, Vasily, any preference which tree this goes? I think s390
would be easiest, but that requires that the patches in the swiotlb tree have
fixed commit IDs.
> ---
> arch/s390/mm/init.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 8ac710de1ab1..07bbee9b7320 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -186,9 +186,9 @@ static void pv_init(void)
> return;
>
> /* make sure bounce buffers are shared */
> + swiotlb_force = SWIOTLB_FORCE;
> swiotlb_init(1);
> swiotlb_update_mem_attributes();
> - swiotlb_force = SWIOTLB_FORCE;
> }
>
> void __init mem_init(void)
>
On Fri, 23 Jul 2021 08:14:19 +0200
Christian Borntraeger <[email protected]> wrote:
> Resending with the correct email of Heiko....
>
> On 23.07.21 03:12, Halil Pasic wrote:
> > On Thu, 22 Jul 2021 21:22:58 +0200
> > Christian Borntraeger <[email protected]> wrote:
> >
> >> On 20.07.21 15:38, Will Deacon wrote:
> >>> Hi again, folks,
> >>>
> >>> This is version two of the patch series I posted yesterday:
> >>>
> >>> https://lore.kernel.org/r/[email protected]
> >>>
> >>> The only changes since v1 are:
> >>>
> >>> * Squash patches 2 and 3, amending the commit message accordingly
> >>> * Add Reviewed-by and Tested-by tags from Christoph and Claire (thanks!)
> >>>
> >>> I'd usually leave it a bit longer between postings, but since this fixes
> >>> issues with patches in -next I thought I'd spin a new version immediately.
> >>>
> >>> Cheers,
> >>
> >> FWIW, I just bisected virtio-errors with secure execution mode
> >> qemu-system-s390x: virtio-serial-bus: Unexpected port id 4205794771 for device virtio-serial0.0
> >>
> >> to
> >> commit 903cd0f315fe426c6a64c54ed389de0becb663dc
> >> Author: Claire Chang <[email protected]>
> >> Date: Thu Jun 24 23:55:20 2021 +0800
> >>
> >> swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
> >>
> >> Unfortunately this patch series does NOT fix this issue, so it seems that even more
> >> things are broken.
> >>
> >> Any idea what else might be broken?
> >
> > I've done some debugging, and I think I know what is going on. Since
> > that commit we need to set force_swiotlb before the swiotlb itself is
> > initialized. So the patch below should fix the problem.
> >
> > --------------------8<-------------------------------------
> >
> > From: Halil Pasic <[email protected]>
> > Date: Fri, 23 Jul 2021 02:57:06 +0200
> > Subject: [PATCH 1/1] s390/pv: fix the forcing of the swiotlb
> >
> > Since commit 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for
> > swiotlb data bouncing") if code sets swiotlb_force it needs to do so
> > before the swiotlb is initialised. Otherwise
> > io_tlb_default_mem->force_bounce will not get set to true, and devices
> > that use (the default) swiotlb will not bounce despite switolb_force
> > having the value of SWIOTLB_FORCE.
> >
> > Let us restore swiotlb functionality for PV by fulfilling this new
> > requirement.
> >
> I would add:
> Fixes: 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing")
> as this patch breaks things
> and
> Fixes: 64e1f0c531d1 ("s390/mm: force swiotlb for protected virtualization")
>
> to make the s390 init code more robust in case people start backporting things.
I agree. Do we want this backported to the stable releases that have
64e1f0c531d1 (i.e. do we need a cc stable) or should the fixes tag just
serve as metadata? My guess is, it's the former. In that sense should I
add the tags along with an explanation for the second fixes respin with
cc stable?
(BTW I don't think this formally qualifies for the stable backports, but
I hope we can make an exception...)
>
> > Signed-off-by: Halil Pasic <[email protected]>
>
> I can confirm that this fixes the problem. This also makes sense codewise.
>
> Tested-by: Christian Borntraeger <[email protected]>
> Reviewed-by: Christian Borntraeger <[email protected]>
Thanks!
Regards,
Halil
>
> Konrad, Heiko, Vasily, any preference which tree this goes? I think s390
> would be easiest, but that requires that the patches in the swiotlb tree have
> fixed commit IDs.
>
> > ---
> > arch/s390/mm/init.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> > index 8ac710de1ab1..07bbee9b7320 100644
> > --- a/arch/s390/mm/init.c
> > +++ b/arch/s390/mm/init.c
> > @@ -186,9 +186,9 @@ static void pv_init(void)
> > return;
> >
> > /* make sure bounce buffers are shared */
> > + swiotlb_force = SWIOTLB_FORCE;
> > swiotlb_init(1);
> > swiotlb_update_mem_attributes();
> > - swiotlb_force = SWIOTLB_FORCE;
> > }
> >
> > void __init mem_init(void)
> >
On 23.07.21 10:47, Halil Pasic wrote:
> On Fri, 23 Jul 2021 08:14:19 +0200
> Christian Borntraeger <[email protected]> wrote:
>
>> Resending with the correct email of Heiko....
>>
>> On 23.07.21 03:12, Halil Pasic wrote:
>>> On Thu, 22 Jul 2021 21:22:58 +0200
>>> Christian Borntraeger <[email protected]> wrote:
>>>
>>>> On 20.07.21 15:38, Will Deacon wrote:
>>>>> Hi again, folks,
>>>>>
>>>>> This is version two of the patch series I posted yesterday:
>>>>>
>>>>> https://lore.kernel.org/r/[email protected]
>>>>>
>>>>> The only changes since v1 are:
>>>>>
>>>>> * Squash patches 2 and 3, amending the commit message accordingly
>>>>> * Add Reviewed-by and Tested-by tags from Christoph and Claire (thanks!)
>>>>>
>>>>> I'd usually leave it a bit longer between postings, but since this fixes
>>>>> issues with patches in -next I thought I'd spin a new version immediately.
>>>>>
>>>>> Cheers,
>>>>
>>>> FWIW, I just bisected virtio-errors with secure execution mode
>>>> qemu-system-s390x: virtio-serial-bus: Unexpected port id 4205794771 for device virtio-serial0.0
>>>>
>>>> to
>>>> commit 903cd0f315fe426c6a64c54ed389de0becb663dc
>>>> Author: Claire Chang <[email protected]>
>>>> Date: Thu Jun 24 23:55:20 2021 +0800
>>>>
>>>> swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
>>>>
>>>> Unfortunately this patch series does NOT fix this issue, so it seems that even more
>>>> things are broken.
>>>>
>>>> Any idea what else might be broken?
>>>
>>> I've done some debugging, and I think I know what is going on. Since
>>> that commit we need to set force_swiotlb before the swiotlb itself is
>>> initialized. So the patch below should fix the problem.
>>>
>>> --------------------8<-------------------------------------
>>>
>>> From: Halil Pasic <[email protected]>
>>> Date: Fri, 23 Jul 2021 02:57:06 +0200
>>> Subject: [PATCH 1/1] s390/pv: fix the forcing of the swiotlb
>>>
>>> Since commit 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for
>>> swiotlb data bouncing") if code sets swiotlb_force it needs to do so
>>> before the swiotlb is initialised. Otherwise
>>> io_tlb_default_mem->force_bounce will not get set to true, and devices
>>> that use (the default) swiotlb will not bounce despite switolb_force
>>> having the value of SWIOTLB_FORCE.
>>>
>>> Let us restore swiotlb functionality for PV by fulfilling this new
>>> requirement.
>>>
>> I would add:
>> Fixes: 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing")
>> as this patch breaks things
>> and
>> Fixes: 64e1f0c531d1 ("s390/mm: force swiotlb for protected virtualization")
>>
>> to make the s390 init code more robust in case people start backporting things.
>
> I agree. Do we want this backported to the stable releases that have
> 64e1f0c531d1 (i.e. do we need a cc stable) or should the fixes tag just
> serve as metadata? My guess is, it's the former. In that sense should I
> add the tags along with an explanation for the second fixes respin with
> cc stable?
>
> (BTW I don't think this formally qualifies for the stable backports, but
> I hope we can make an exception...)
I think it makes sense for stable as it is cleaner to set the flags before
calling the init function. cc stable would be better and the right way
according to process, but the Fixes tag is mostly enough.
>
>>
>>> Signed-off-by: Halil Pasic <[email protected]>
>>
>> I can confirm that this fixes the problem. This also makes sense codewise.
>>
>> Tested-by: Christian Borntraeger <[email protected]>
>> Reviewed-by: Christian Borntraeger <[email protected]>
>
> Thanks!
>
> Regards,
> Halil
>>
>> Konrad, Heiko, Vasily, any preference which tree this goes? I think s390
>> would be easiest, but that requires that the patches in the swiotlb tree have
>> fixed commit IDs.
>>
>>> ---
>>> arch/s390/mm/init.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>>> index 8ac710de1ab1..07bbee9b7320 100644
>>> --- a/arch/s390/mm/init.c
>>> +++ b/arch/s390/mm/init.c
>>> @@ -186,9 +186,9 @@ static void pv_init(void)
>>> return;
>>>
>>> /* make sure bounce buffers are shared */
>>> + swiotlb_force = SWIOTLB_FORCE;
>>> swiotlb_init(1);
>>> swiotlb_update_mem_attributes();
>>> - swiotlb_force = SWIOTLB_FORCE;
>>> }
>>>
>>> void __init mem_init(void)
>>>
>
On Fri, Jul 23, 2021 at 10:50:57AM +0200, Christian Borntraeger wrote:
>
>
> On 23.07.21 10:47, Halil Pasic wrote:
> > On Fri, 23 Jul 2021 08:14:19 +0200
> > Christian Borntraeger <[email protected]> wrote:
> >
> > > Resending with the correct email of Heiko....
> > >
> > > On 23.07.21 03:12, Halil Pasic wrote:
> > > > On Thu, 22 Jul 2021 21:22:58 +0200
> > > > Christian Borntraeger <[email protected]> wrote:
> > > > > On 20.07.21 15:38, Will Deacon wrote:
> > > > > > Hi again, folks,
> > > > > >
> > > > > > This is version two of the patch series I posted yesterday:
> > > > > >
> > > > > > https://lore.kernel.org/r/[email protected]
> > > > > >
> > > > > > The only changes since v1 are:
> > > > > >
> > > > > > * Squash patches 2 and 3, amending the commit message accordingly
> > > > > > * Add Reviewed-by and Tested-by tags from Christoph and Claire (thanks!)
> > > > > >
> > > > > > I'd usually leave it a bit longer between postings, but since this fixes
> > > > > > issues with patches in -next I thought I'd spin a new version immediately.
> > > > > >
> > > > > > Cheers,
> > > > >
> > > > > FWIW, I just bisected virtio-errors with secure execution mode
> > > > > qemu-system-s390x: virtio-serial-bus: Unexpected port id 4205794771 for device virtio-serial0.0
> > > > >
> > > > > to
> > > > > commit 903cd0f315fe426c6a64c54ed389de0becb663dc
> > > > > Author: Claire Chang <[email protected]>
> > > > > Date: Thu Jun 24 23:55:20 2021 +0800
> > > > >
> > > > > swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
> > > > >
> > > > > Unfortunately this patch series does NOT fix this issue, so it seems that even more
> > > > > things are broken.
> > > > >
> > > > > Any idea what else might be broken?
> > > >
> > > > I've done some debugging, and I think I know what is going on. Since
> > > > that commit we need to set force_swiotlb before the swiotlb itself is
> > > > initialized. So the patch below should fix the problem.
> > > >
> > > > --------------------8<-------------------------------------
> > > >
> > > > From: Halil Pasic <[email protected]>
> > > > Date: Fri, 23 Jul 2021 02:57:06 +0200
> > > > Subject: [PATCH 1/1] s390/pv: fix the forcing of the swiotlb
> > > >
> > > > Since commit 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for
> > > > swiotlb data bouncing") if code sets swiotlb_force it needs to do so
> > > > before the swiotlb is initialised. Otherwise
> > > > io_tlb_default_mem->force_bounce will not get set to true, and devices
> > > > that use (the default) swiotlb will not bounce despite switolb_force
> > > > having the value of SWIOTLB_FORCE.
> > > >
> > > > Let us restore swiotlb functionality for PV by fulfilling this new
> > > > requirement.
> > > I would add:
> > > Fixes: 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing")
> > > as this patch breaks things
> > > and
> > > Fixes: 64e1f0c531d1 ("s390/mm: force swiotlb for protected virtualization")
> > >
> > > to make the s390 init code more robust in case people start backporting things.
> >
> > I agree. Do we want this backported to the stable releases that have
> > 64e1f0c531d1 (i.e. do we need a cc stable) or should the fixes tag just
> > serve as metadata? My guess is, it's the former. In that sense should I
> > add the tags along with an explanation for the second fixes respin with
> > cc stable?
> >
> > (BTW I don't think this formally qualifies for the stable backports, but
> > I hope we can make an exception...)
>
> I think it makes sense for stable as it is cleaner to set the flags before
> calling the init function. cc stable would be better and the right way
> according to process, but the Fixes tag is mostly enough.
But the reaso for fixing this is for code that is not yet in Linus's
tree?
I can just pick this patch up and add it in the pile I have for the next
merge window?
>
> >
> > >
> > > > Signed-off-by: Halil Pasic <[email protected]>
> > >
> > > I can confirm that this fixes the problem. This also makes sense codewise.
> > >
> > > Tested-by: Christian Borntraeger <[email protected]>
> > > Reviewed-by: Christian Borntraeger <[email protected]>
> >
> > Thanks!
> >
> > Regards,
> > Halil
> > >
> > > Konrad, Heiko, Vasily, any preference which tree this goes? I think s390
> > > would be easiest, but that requires that the patches in the swiotlb tree have
> > > fixed commit IDs.
> > >
> > > > ---
> > > > arch/s390/mm/init.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> > > > index 8ac710de1ab1..07bbee9b7320 100644
> > > > --- a/arch/s390/mm/init.c
> > > > +++ b/arch/s390/mm/init.c
> > > > @@ -186,9 +186,9 @@ static void pv_init(void)
> > > > return;
> > > > /* make sure bounce buffers are shared */
> > > > + swiotlb_force = SWIOTLB_FORCE;
> > > > swiotlb_init(1);
> > > > swiotlb_update_mem_attributes();
> > > > - swiotlb_force = SWIOTLB_FORCE;
> > > > }
> > > > void __init mem_init(void)
> >
On 23.07.21 16:01, Konrad Rzeszutek Wilk wrote:
> On Fri, Jul 23, 2021 at 10:50:57AM +0200, Christian Borntraeger wrote:
>>
>>
>> On 23.07.21 10:47, Halil Pasic wrote:
>>> On Fri, 23 Jul 2021 08:14:19 +0200
>>> Christian Borntraeger <[email protected]> wrote:
>>>
>>>> Resending with the correct email of Heiko....
>>>>
>>>> On 23.07.21 03:12, Halil Pasic wrote:
>>>>> On Thu, 22 Jul 2021 21:22:58 +0200
>>>>> Christian Borntraeger <[email protected]> wrote:
>>>>>> On 20.07.21 15:38, Will Deacon wrote:
>>>>>>> Hi again, folks,
>>>>>>>
>>>>>>> This is version two of the patch series I posted yesterday:
>>>>>>>
>>>>>>> https://lore.kernel.org/r/[email protected]
>>>>>>>
>>>>>>> The only changes since v1 are:
>>>>>>>
>>>>>>> * Squash patches 2 and 3, amending the commit message accordingly
>>>>>>> * Add Reviewed-by and Tested-by tags from Christoph and Claire (thanks!)
>>>>>>>
>>>>>>> I'd usually leave it a bit longer between postings, but since this fixes
>>>>>>> issues with patches in -next I thought I'd spin a new version immediately.
>>>>>>>
>>>>>>> Cheers,
>>>>>>
>>>>>> FWIW, I just bisected virtio-errors with secure execution mode
>>>>>> qemu-system-s390x: virtio-serial-bus: Unexpected port id 4205794771 for device virtio-serial0.0
>>>>>>
>>>>>> to
>>>>>> commit 903cd0f315fe426c6a64c54ed389de0becb663dc
>>>>>> Author: Claire Chang <[email protected]>
>>>>>> Date: Thu Jun 24 23:55:20 2021 +0800
>>>>>>
>>>>>> swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
>>>>>>
>>>>>> Unfortunately this patch series does NOT fix this issue, so it seems that even more
>>>>>> things are broken.
>>>>>>
>>>>>> Any idea what else might be broken?
>>>>>
>>>>> I've done some debugging, and I think I know what is going on. Since
>>>>> that commit we need to set force_swiotlb before the swiotlb itself is
>>>>> initialized. So the patch below should fix the problem.
>>>>>
>>>>> --------------------8<-------------------------------------
>>>>>
>>>>> From: Halil Pasic <[email protected]>
>>>>> Date: Fri, 23 Jul 2021 02:57:06 +0200
>>>>> Subject: [PATCH 1/1] s390/pv: fix the forcing of the swiotlb
>>>>>
>>>>> Since commit 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for
>>>>> swiotlb data bouncing") if code sets swiotlb_force it needs to do so
>>>>> before the swiotlb is initialised. Otherwise
>>>>> io_tlb_default_mem->force_bounce will not get set to true, and devices
>>>>> that use (the default) swiotlb will not bounce despite switolb_force
>>>>> having the value of SWIOTLB_FORCE.
>>>>>
>>>>> Let us restore swiotlb functionality for PV by fulfilling this new
>>>>> requirement.
>>>> I would add:
>>>> Fixes: 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing")
>>>> as this patch breaks things
>>>> and
>>>> Fixes: 64e1f0c531d1 ("s390/mm: force swiotlb for protected virtualization")
>>>>
>>>> to make the s390 init code more robust in case people start backporting things.
>>>
>>> I agree. Do we want this backported to the stable releases that have
>>> 64e1f0c531d1 (i.e. do we need a cc stable) or should the fixes tag just
>>> serve as metadata? My guess is, it's the former. In that sense should I
>>> add the tags along with an explanation for the second fixes respin with
>>> cc stable?
>>>
>>> (BTW I don't think this formally qualifies for the stable backports, but
>>> I hope we can make an exception...)
>>
>> I think it makes sense for stable as it is cleaner to set the flags before
>> calling the init function. cc stable would be better and the right way
>> according to process, but the Fixes tag is mostly enough.
>
> But the reaso for fixing this is for code that is not yet in Linus's
> tree?
>
> I can just pick this patch up and add it in the pile I have for the next
> merge window?
That would also work for me. I think Halil wanted to send out and v2.
In any case
Acked-by: Christian Borntraeger <[email protected]>
so that you can take this via the swiotlb tree.
>>
>>>
>>>>
>>>>> Signed-off-by: Halil Pasic <[email protected]>
>>>>
>>>> I can confirm that this fixes the problem. This also makes sense codewise.
>>>>
>>>> Tested-by: Christian Borntraeger <[email protected]>
>>>> Reviewed-by: Christian Borntraeger <[email protected]>
>>>
>>> Thanks!
>>>
>>> Regards,
>>> Halil
>>>>
>>>> Konrad, Heiko, Vasily, any preference which tree this goes? I think s390
>>>> would be easiest, but that requires that the patches in the swiotlb tree have
>>>> fixed commit IDs.
>>>>
>>>>> ---
>>>>> arch/s390/mm/init.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>>>>> index 8ac710de1ab1..07bbee9b7320 100644
>>>>> --- a/arch/s390/mm/init.c
>>>>> +++ b/arch/s390/mm/init.c
>>>>> @@ -186,9 +186,9 @@ static void pv_init(void)
>>>>> return;
>>>>> /* make sure bounce buffers are shared */
>>>>> + swiotlb_force = SWIOTLB_FORCE;
>>>>> swiotlb_init(1);
>>>>> swiotlb_update_mem_attributes();
>>>>> - swiotlb_force = SWIOTLB_FORCE;
>>>>> }
>>>>> void __init mem_init(void)
>>>
On Fri, 23 Jul 2021 19:53:58 +0200
Christian Borntraeger <[email protected]> wrote:
> On 23.07.21 16:01, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jul 23, 2021 at 10:50:57AM +0200, Christian Borntraeger wrote:
> >>
> >>
> >> On 23.07.21 10:47, Halil Pasic wrote:
> >>> On Fri, 23 Jul 2021 08:14:19 +0200
> >>> Christian Borntraeger <[email protected]> wrote:
> >>>
> >>>> Resending with the correct email of Heiko....
> >>>>
> >>>> On 23.07.21 03:12, Halil Pasic wrote:
> >>>>> On Thu, 22 Jul 2021 21:22:58 +0200
> >>>>> Christian Borntraeger <[email protected]> wrote:
> >>>>>> On 20.07.21 15:38, Will Deacon wrote:
> >>>>>>> Hi again, folks,
> >>>>>>>
> >>>>>>> This is version two of the patch series I posted yesterday:
> >>>>>>>
> >>>>>>> https://lore.kernel.org/r/[email protected]
> >>>>>>>
> >>>>>>> The only changes since v1 are:
> >>>>>>>
> >>>>>>> * Squash patches 2 and 3, amending the commit message accordingly
> >>>>>>> * Add Reviewed-by and Tested-by tags from Christoph and Claire (thanks!)
> >>>>>>>
> >>>>>>> I'd usually leave it a bit longer between postings, but since this fixes
> >>>>>>> issues with patches in -next I thought I'd spin a new version immediately.
> >>>>>>>
> >>>>>>> Cheers,
> >>>>>>
> >>>>>> FWIW, I just bisected virtio-errors with secure execution mode
> >>>>>> qemu-system-s390x: virtio-serial-bus: Unexpected port id 4205794771 for device virtio-serial0.0
> >>>>>>
> >>>>>> to
> >>>>>> commit 903cd0f315fe426c6a64c54ed389de0becb663dc
> >>>>>> Author: Claire Chang <[email protected]>
> >>>>>> Date: Thu Jun 24 23:55:20 2021 +0800
> >>>>>>
> >>>>>> swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
> >>>>>>
> >>>>>> Unfortunately this patch series does NOT fix this issue, so it seems that even more
> >>>>>> things are broken.
> >>>>>>
> >>>>>> Any idea what else might be broken?
> >>>>>
> >>>>> I've done some debugging, and I think I know what is going on. Since
> >>>>> that commit we need to set force_swiotlb before the swiotlb itself is
> >>>>> initialized. So the patch below should fix the problem.
> >>>>>
> >>>>> --------------------8<-------------------------------------
> >>>>>
> >>>>> From: Halil Pasic <[email protected]>
> >>>>> Date: Fri, 23 Jul 2021 02:57:06 +0200
> >>>>> Subject: [PATCH 1/1] s390/pv: fix the forcing of the swiotlb
> >>>>>
> >>>>> Since commit 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for
> >>>>> swiotlb data bouncing") if code sets swiotlb_force it needs to do so
> >>>>> before the swiotlb is initialised. Otherwise
> >>>>> io_tlb_default_mem->force_bounce will not get set to true, and devices
> >>>>> that use (the default) swiotlb will not bounce despite switolb_force
> >>>>> having the value of SWIOTLB_FORCE.
> >>>>>
> >>>>> Let us restore swiotlb functionality for PV by fulfilling this new
> >>>>> requirement.
> >>>> I would add:
> >>>> Fixes: 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing")
> >>>> as this patch breaks things
> >>>> and
> >>>> Fixes: 64e1f0c531d1 ("s390/mm: force swiotlb for protected virtualization")
> >>>>
> >>>> to make the s390 init code more robust in case people start backporting things.
> >>>
> >>> I agree. Do we want this backported to the stable releases that have
> >>> 64e1f0c531d1 (i.e. do we need a cc stable) or should the fixes tag just
> >>> serve as metadata? My guess is, it's the former. In that sense should I
> >>> add the tags along with an explanation for the second fixes respin with
> >>> cc stable?
> >>>
> >>> (BTW I don't think this formally qualifies for the stable backports, but
> >>> I hope we can make an exception...)
> >>
> >> I think it makes sense for stable as it is cleaner to set the flags before
> >> calling the init function. cc stable would be better and the right way
> >> according to process, but the Fixes tag is mostly enough.
> >
> > But the reaso for fixing this is for code that is not yet in Linus's
> > tree?
> >
> > I can just pick this patch up and add it in the pile I have for the next
> > merge window?
>
> That would also work for me. I think Halil wanted to send out and v2.
Sorry I didn't interpret your answer correctly. (I interpreted it
like the fixes tags are enough, and those can be added by the maintainer
that is going to merge the patch.) I will send out a v2 right away.
Regards,
Halil
On Tue, Jul 20, 2021 at 02:38:22PM +0100, Will Deacon wrote:
> Hi again, folks,
>
> This is version two of the patch series I posted yesterday:
>
> https://lore.kernel.org/r/[email protected]
>
> The only changes since v1 are:
>
> * Squash patches 2 and 3, amending the commit message accordingly
> * Add Reviewed-by and Tested-by tags from Christoph and Claire (thanks!)
>
> I'd usually leave it a bit longer between postings, but since this fixes
> issues with patches in -next I thought I'd spin a new version immediately.
Thank you!
I put them in devel/for-linus-5.15 and linux-next.
Hi,
On Tue, Jul 20, 2021 at 02:38:26PM +0100, Will Deacon wrote:
> Although swiotlb_exit() frees the 'slots' metadata array referenced by
> 'io_tlb_default_mem', it leaves the underlying buffer pages allocated
> despite no longer being usable.
>
> Extend swiotlb_exit() to free the buffer pages as well as the slots
> array.
>
This patch causes qemu pseries emulations to crash. Backtrace and bisect
log see below. Reverting it fixes the problem.
Guenter
---
Crash log:
...
[ 0.937801][ T1] software IO TLB: tearing down default memory pool
[ 0.938939][ T1] ------------[ cut here ]------------
[ 0.940331][ T1] kernel BUG at arch/powerpc/kernel/interrupt.c:98!
[ 0.940787][ T1] Oops: Exception in kernel mode, sig: 5 [#1]
[ 0.940883][ T1] BE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
[ 0.940999][ T1] Modules linked in:
[ 0.941240][ T1] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3-next-20210729 #1
[ 0.941523][ T1] NIP: c000000000031310 LR: c0000000000312f4 CTR: c00000000000c5f0
[ 0.941608][ T1] REGS: c000000008687ac0 TRAP: 0700 Not tainted (5.14.0-rc3-next-20210729)
[ 0.941795][ T1] MSR: 8000000000029032 <SF,EE,ME,IR,DR,RI> CR: 24002222 XER: 00000000
[ 0.942031][ T1] CFAR: c000000000300360 IRQMASK: 3
[ 0.942031][ T1] GPR00: c0000000000312f4 c000000008687d60 c0000000022ee300 0000000000000003
[ 0.942031][ T1] GPR04: 000000000000033f 0000000000000400 0000000000000000 000000003e5a0000
[ 0.942031][ T1] GPR08: 000000003e5a0000 0000000000000001 0000000000000000 0000000000000003
[ 0.942031][ T1] GPR12: ffffffffffffffff c000000002fb0000 c0000000000129c0 0000000000000000
[ 0.942031][ T1] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[ 0.942031][ T1] GPR20: 0000000000000000 0000000000000000 0000000000000000 c0000000021d8b00
[ 0.942031][ T1] GPR24: 0000000000000000 0000000000000000 ffffffffffffffff 0000000000000400
[ 0.942031][ T1] GPR28: 000000000000033f 000000000000f134 c00000000176c068 c000000008687e80
[ 0.942884][ T1] NIP [c000000000031310] .system_call_exception+0x70/0x2d0
[ 0.943399][ T1] LR [c0000000000312f4] .system_call_exception+0x54/0x2d0
[ 0.943594][ T1] Call Trace:
[ 0.943667][ T1] [c000000008687d60] [c0000000000312f4] .system_call_exception+0x54/0x2d0 (unreliable)
[ 0.943919][ T1] [c000000008687e10] [c00000000000c6e4] system_call_common+0xf4/0x258
[ 0.944066][ T1] --- interrupt: c00 at .ucall_norets+0x4/0x14
[ 0.944172][ T1] NIP: c000000000079ce0 LR: c0000000000fa274 CTR: 0000000000000000
[ 0.944245][ T1] REGS: c000000008687e80 TRAP: 0c00 Not tainted (5.14.0-rc3-next-20210729)
[ 0.944323][ T1] MSR: 8000000002009032 <SF,VEC,EE,ME,IR,DR,RI> CR: 24002222 XER: 00000000
[ 0.944463][ T1] IRQMASK: 0
[ 0.944463][ T1] GPR00: c00000000176c068 c000000008687a30 c0000000022ee300 000000000000f134
[ 0.944463][ T1] GPR04: 000000000000033f 0000000000000400 ffffffffffffffff 0000000000000000
[ 0.944463][ T1] GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[ 0.944463][ T1] GPR12: 0000000000000000 c000000002fb0000 c0000000000129c0 0000000000000000
[ 0.944463][ T1] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[ 0.944463][ T1] GPR20: 0000000000000000 0000000000000000 0000000000000000 c0000000021d8b00
[ 0.944463][ T1] GPR24: c0000000017c1088 0000000000000004 c00000000171c450 c000000002338e40
[ 0.944463][ T1] GPR28: c0000000017c10b0 c0000000033f0000 0000000000000400 c0000000033f0000
[ 0.945188][ T1] NIP [c000000000079ce0] .ucall_norets+0x4/0x14
[ 0.945285][ T1] LR [c0000000000fa274] .set_memory_encrypted+0x44/0x80
[ 0.945375][ T1] --- interrupt: c00
[ 0.945419][ T1] [c000000008687a30] [c00000000176c068] .swiotlb_exit+0xbc/0x180 (unreliable)
[ 0.945612][ T1] Instruction dump:
[ 0.945745][ T1] 7cbb2b78 7cda3378 7cf93b78 7d184378 482cefd1 60000000 e93f0108 692a0002
[ 0.945883][ T1] 794affe2 0b0a0000 69294000 792997e2 <0b090000> e93f0138 792907e0 0b090000
[ 0.946751][ T1] ---[ end trace 600e218cfc83c24b ]---
[ 0.955185][ T1]
[ 1.955805][ T1] BUG: sleeping function called from invalid context at ./include/linux/percpu-rwsem.h:49
[ 1.955918][ T1] in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: 1, name: swapper/0
[ 1.956024][ T1] INFO: lockdep is turned off.
[ 1.956094][ T1] irq event stamp: 14792
[ 1.956142][ T1] hardirqs last enabled at (14791): [<c0000000001f8fe8>] .__up_console_sem+0xc8/0x110
[ 1.956269][ T1] hardirqs last disabled at (14792): [<c0000000000312f4>] .system_call_exception+0x54/0x2d0
[ 1.956384][ T1] softirqs last enabled at (10586): [<c000000000efd6d4>] .release_sock+0xb4/0x100
[ 1.956497][ T1] softirqs last disabled at (10584): [<c000000000efd654>] .release_sock+0x34/0x100
[ 1.956726][ T1] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G D 5.14.0-rc3-next-20210729 #1
[ 1.956846][ T1] Call Trace:
[ 1.956885][ T1] [c000000008687700] [c0000000009fe088] .dump_stack_lvl+0xa4/0x100 (unreliable)
[ 1.957011][ T1] [c000000008687790] [c000000000193060] .___might_sleep+0x2b0/0x2f0
[ 1.957130][ T1] [c000000008687820] [c00000000015fba0] .exit_signals+0x50/0x540
[ 1.957238][ T1] [c0000000086878e0] [c000000000148dbc] .do_exit+0xec/0xe20
[ 1.957335][ T1] [c0000000086879c0] [c000000000029bc4] .oops_end+0x144/0x210
[ 1.957441][ T1] [c000000008687a50] [c0000000000095d4] program_check_common_virt+0x2d4/0x320
[ 1.957561][ T1] --- interrupt: 700 at .system_call_exception+0x70/0x2d0
[ 1.957645][ T1] NIP: c000000000031310 LR: c0000000000312f4 CTR: c00000000000c5f0
[ 1.957715][ T1] REGS: c000000008687ac0 TRAP: 0700 Tainted: G D (5.14.0-rc3-next-20210729)
[ 1.957799][ T1] MSR: 8000000000029032 <SF,EE,ME,IR,DR,RI> CR: 24002222 XER: 00000000
[ 1.957919][ T1] CFAR: c000000000300360 IRQMASK: 3
[ 1.957919][ T1] GPR00: c0000000000312f4 c000000008687d60 c0000000022ee300 0000000000000003
[ 1.957919][ T1] GPR04: 000000000000033f 0000000000000400 0000000000000000 000000003e5a0000
[ 1.957919][ T1] GPR08: 000000003e5a0000 0000000000000001 0000000000000000 0000000000000003
[ 1.957919][ T1] GPR12: ffffffffffffffff c000000002fb0000 c0000000000129c0 0000000000000000
[ 1.957919][ T1] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[ 1.957919][ T1] GPR20: 0000000000000000 0000000000000000 0000000000000000 c0000000021d8b00
[ 1.957919][ T1] GPR24: 0000000000000000 0000000000000000 ffffffffffffffff 0000000000000400
[ 1.957919][ T1] GPR28: 000000000000033f 000000000000f134 c00000000176c068 c000000008687e80
[ 1.958633][ T1] NIP [c000000000031310] .system_call_exception+0x70/0x2d0
[ 1.958720][ T1] LR [c0000000000312f4] .system_call_exception+0x54/0x2d0
[ 1.958803][ T1] --- interrupt: 700
[ 1.958842][ T1] [c000000008687e10] [c00000000000c6e4] system_call_common+0xf4/0x258
[ 1.958961][ T1] --- interrupt: c00 at .ucall_norets+0x4/0x14
[ 1.959055][ T1] NIP: c000000000079ce0 LR: c0000000000fa274 CTR: 0000000000000000
[ 1.959127][ T1] REGS: c000000008687e80 TRAP: 0c00 Tainted: G D (5.14.0-rc3-next-20210729)
[ 1.959210][ T1] MSR: 8000000002009032 <SF,VEC,EE,ME,IR,DR,RI> CR: 24002222 XER: 00000000
[ 1.959337][ T1] IRQMASK: 0
[ 1.959337][ T1] GPR00: c00000000176c068 c000000008687a30 c0000000022ee300 000000000000f134
[ 1.959337][ T1] GPR04: 000000000000033f 0000000000000400 ffffffffffffffff 0000000000000000
[ 1.959337][ T1] GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[ 1.959337][ T1] GPR12: 0000000000000000 c000000002fb0000 c0000000000129c0 0000000000000000
[ 1.959337][ T1] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[ 1.959337][ T1] GPR20: 0000000000000000 0000000000000000 0000000000000000 c0000000021d8b00
[ 1.959337][ T1] GPR24: c0000000017c1088 0000000000000004 c00000000171c450 c000000002338e40
[ 1.959337][ T1] GPR28: c0000000017c10b0 c0000000033f0000 0000000000000400 c0000000033f0000
[ 1.960042][ T1] NIP [c000000000079ce0] .ucall_norets+0x4/0x14
[ 1.960137][ T1] LR [c0000000000fa274] .set_memory_encrypted+0x44/0x80
[ 1.960219][ T1] --- interrupt: c00
[ 1.960257][ T1] [c000000008687a30] [c00000000176c068] .swiotlb_exit+0xbc/0x180 (unreliable)
[ 1.960845][ T1] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000005
qemu-system-ppc64: OS terminated: OS panic: Attempted to kill init! exitcode=0x00000005
---
Bisect log:
# bad: [8d4b477da1a807199ca60e0829357ce7aa6758d5] Add linux-next specific files for 20210730
# good: [ff1176468d368232b684f75e82563369208bc371] Linux 5.14-rc3
git bisect start 'HEAD' 'v5.14-rc3'
# bad: [8f3eb1f5c702ef868d89799b03c21d122f2fe197] Merge remote-tracking branch 'bpf-next/for-next'
git bisect bad 8f3eb1f5c702ef868d89799b03c21d122f2fe197
# good: [02afbb8f68dc8b37c07e457d2f69d440781af23a] Merge remote-tracking branch 'cifsd/cifsd-for-next'
git bisect good 02afbb8f68dc8b37c07e457d2f69d440781af23a
# good: [3e12361b6d23f793580a50a6008633501c56ea1d] bcm63xx_enet: delete a redundant assignment
git bisect good 3e12361b6d23f793580a50a6008633501c56ea1d
# good: [78d788681492abe4980d5cc9b93b70df9f028880] Merge remote-tracking branch 'jc_docs/docs-next'
git bisect good 78d788681492abe4980d5cc9b93b70df9f028880
# good: [266234e7659f731cf471a1bdc4fd1ead4caa8303] Merge remote-tracking branch 'v4l-dvb-next/master'
git bisect good 266234e7659f731cf471a1bdc4fd1ead4caa8303
# bad: [be1841fe3ca0010139fe4fb44dd42dea7c0e3401] Merge remote-tracking branch 'swiotlb/linux-next'
git bisect bad be1841fe3ca0010139fe4fb44dd42dea7c0e3401
# good: [7b8798617c7975d10678d99dcc59d103e237b4cd] Merge remote-tracking branch 'ieee1394/for-next'
git bisect good 7b8798617c7975d10678d99dcc59d103e237b4cd
# good: [463e862ac63ef27fca423782536f6465abc3f180] swiotlb: Convert io_default_tlb_mem to static allocation
git bisect good 463e862ac63ef27fca423782536f6465abc3f180
# good: [2dc6b1158c28c3a5e86d162628810312f98d5e97] fs: dlm: introduce generic listen
git bisect good 2dc6b1158c28c3a5e86d162628810312f98d5e97
# good: [62699b3f0a62435fceb8debf295e90a5ea259e04] fs: dlm: move receive loop into receive handler
git bisect good 62699b3f0a62435fceb8debf295e90a5ea259e04
# bad: [ad6c00283163cb7ad52cdf97d2850547446f7d98] swiotlb: Free tbl memory in swiotlb_exit()
git bisect bad ad6c00283163cb7ad52cdf97d2850547446f7d98
# good: [1efd3fc0ccf52e1aa5f0bf5b0d82847180d20951] swiotlb: Emit diagnostic in swiotlb_exit()
git bisect good 1efd3fc0ccf52e1aa5f0bf5b0d82847180d20951
# first bad commit: [ad6c00283163cb7ad52cdf97d2850547446f7d98] swiotlb: Free tbl memory in swiotlb_exit()
On Sat, Jul 31, 2021 at 11:26:11AM -0700, Guenter Roeck wrote:
> Hi,
>
> On Tue, Jul 20, 2021 at 02:38:26PM +0100, Will Deacon wrote:
> > Although swiotlb_exit() frees the 'slots' metadata array referenced by
> > 'io_tlb_default_mem', it leaves the underlying buffer pages allocated
> > despite no longer being usable.
> >
> > Extend swiotlb_exit() to free the buffer pages as well as the slots
> > array.
> >
>
> This patch causes qemu pseries emulations to crash. Backtrace and bisect
> log see below. Reverting it fixes the problem.
I am 99% sure it is fixed by this patch (which should be in linux-next
in 5 minutes):
From a449ffaf9181b5a2dc705d8a06b13e0068207fd4 Mon Sep 17 00:00:00 2001
From: Will Deacon <[email protected]>
Date: Fri, 30 Jul 2021 12:42:31 +0100
Subject: [PATCH] powerpc/svm: Don't issue ultracalls if !mem_encrypt_active()
Commit ad6c00283163 ("swiotlb: Free tbl memory in swiotlb_exit()")
introduced a set_memory_encrypted() call to swiotlb_exit() so that the
buffer pages are returned to an encrypted state prior to being freed.
Sachin reports that this leads to the following crash on a Power server:
[ 0.010799] software IO TLB: tearing down default memory pool
[ 0.010805] ------------[ cut here ]------------
[ 0.010808] kernel BUG at arch/powerpc/kernel/interrupt.c:98!
Nick spotted that this is because set_memory_encrypted() is issuing an
ultracall which doesn't exist for the processor, and should therefore
be gated by mem_encrypt_active() to mirror the x86 implementation.
Cc: Konrad Rzeszutek Wilk <[email protected]>
Cc: Claire Chang <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Robin Murphy <[email protected]>
Fixes: ad6c00283163 ("swiotlb: Free tbl memory in swiotlb_exit()")
Suggested-by: Nicholas Piggin <[email protected]>
Reported-by: Sachin Sant <[email protected]>
Tested-by: Sachin Sant <[email protected]>
Tested-by: Nathan Chancellor <[email protected]>
Link: https://lore.kernel.org/r/[email protected]/
Signed-off-by: Will Deacon <[email protected]>
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
---
arch/powerpc/platforms/pseries/svm.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/powerpc/platforms/pseries/svm.c b/arch/powerpc/platforms/pseries/svm.c
index 1d829e257996..87f001b4c4e4 100644
--- a/arch/powerpc/platforms/pseries/svm.c
+++ b/arch/powerpc/platforms/pseries/svm.c
@@ -63,6 +63,9 @@ void __init svm_swiotlb_init(void)
int set_memory_encrypted(unsigned long addr, int numpages)
{
+ if (!mem_encrypt_active())
+ return 0;
+
if (!PAGE_ALIGNED(addr))
return -EINVAL;
@@ -73,6 +76,9 @@ int set_memory_encrypted(unsigned long addr, int numpages)
int set_memory_decrypted(unsigned long addr, int numpages)
{
+ if (!mem_encrypt_active())
+ return 0;
+
if (!PAGE_ALIGNED(addr))
return -EINVAL;
--
2.31.1
>
> Guenter
>
> ---
> Crash log:
>
> ...
> [ 0.937801][ T1] software IO TLB: tearing down default memory pool
> [ 0.938939][ T1] ------------[ cut here ]------------
> [ 0.940331][ T1] kernel BUG at arch/powerpc/kernel/interrupt.c:98!
> [ 0.940787][ T1] Oops: Exception in kernel mode, sig: 5 [#1]
> [ 0.940883][ T1] BE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
> [ 0.940999][ T1] Modules linked in:
> [ 0.941240][ T1] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3-next-20210729 #1
> [ 0.941523][ T1] NIP: c000000000031310 LR: c0000000000312f4 CTR: c00000000000c5f0
> [ 0.941608][ T1] REGS: c000000008687ac0 TRAP: 0700 Not tainted (5.14.0-rc3-next-20210729)
> [ 0.941795][ T1] MSR: 8000000000029032 <SF,EE,ME,IR,DR,RI> CR: 24002222 XER: 00000000
> [ 0.942031][ T1] CFAR: c000000000300360 IRQMASK: 3
> [ 0.942031][ T1] GPR00: c0000000000312f4 c000000008687d60 c0000000022ee300 0000000000000003
> [ 0.942031][ T1] GPR04: 000000000000033f 0000000000000400 0000000000000000 000000003e5a0000
> [ 0.942031][ T1] GPR08: 000000003e5a0000 0000000000000001 0000000000000000 0000000000000003
> [ 0.942031][ T1] GPR12: ffffffffffffffff c000000002fb0000 c0000000000129c0 0000000000000000
> [ 0.942031][ T1] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [ 0.942031][ T1] GPR20: 0000000000000000 0000000000000000 0000000000000000 c0000000021d8b00
> [ 0.942031][ T1] GPR24: 0000000000000000 0000000000000000 ffffffffffffffff 0000000000000400
> [ 0.942031][ T1] GPR28: 000000000000033f 000000000000f134 c00000000176c068 c000000008687e80
> [ 0.942884][ T1] NIP [c000000000031310] .system_call_exception+0x70/0x2d0
> [ 0.943399][ T1] LR [c0000000000312f4] .system_call_exception+0x54/0x2d0
> [ 0.943594][ T1] Call Trace:
> [ 0.943667][ T1] [c000000008687d60] [c0000000000312f4] .system_call_exception+0x54/0x2d0 (unreliable)
> [ 0.943919][ T1] [c000000008687e10] [c00000000000c6e4] system_call_common+0xf4/0x258
> [ 0.944066][ T1] --- interrupt: c00 at .ucall_norets+0x4/0x14
> [ 0.944172][ T1] NIP: c000000000079ce0 LR: c0000000000fa274 CTR: 0000000000000000
> [ 0.944245][ T1] REGS: c000000008687e80 TRAP: 0c00 Not tainted (5.14.0-rc3-next-20210729)
> [ 0.944323][ T1] MSR: 8000000002009032 <SF,VEC,EE,ME,IR,DR,RI> CR: 24002222 XER: 00000000
> [ 0.944463][ T1] IRQMASK: 0
> [ 0.944463][ T1] GPR00: c00000000176c068 c000000008687a30 c0000000022ee300 000000000000f134
> [ 0.944463][ T1] GPR04: 000000000000033f 0000000000000400 ffffffffffffffff 0000000000000000
> [ 0.944463][ T1] GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [ 0.944463][ T1] GPR12: 0000000000000000 c000000002fb0000 c0000000000129c0 0000000000000000
> [ 0.944463][ T1] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [ 0.944463][ T1] GPR20: 0000000000000000 0000000000000000 0000000000000000 c0000000021d8b00
> [ 0.944463][ T1] GPR24: c0000000017c1088 0000000000000004 c00000000171c450 c000000002338e40
> [ 0.944463][ T1] GPR28: c0000000017c10b0 c0000000033f0000 0000000000000400 c0000000033f0000
> [ 0.945188][ T1] NIP [c000000000079ce0] .ucall_norets+0x4/0x14
> [ 0.945285][ T1] LR [c0000000000fa274] .set_memory_encrypted+0x44/0x80
> [ 0.945375][ T1] --- interrupt: c00
> [ 0.945419][ T1] [c000000008687a30] [c00000000176c068] .swiotlb_exit+0xbc/0x180 (unreliable)
> [ 0.945612][ T1] Instruction dump:
> [ 0.945745][ T1] 7cbb2b78 7cda3378 7cf93b78 7d184378 482cefd1 60000000 e93f0108 692a0002
> [ 0.945883][ T1] 794affe2 0b0a0000 69294000 792997e2 <0b090000> e93f0138 792907e0 0b090000
> [ 0.946751][ T1] ---[ end trace 600e218cfc83c24b ]---
> [ 0.955185][ T1]
> [ 1.955805][ T1] BUG: sleeping function called from invalid context at ./include/linux/percpu-rwsem.h:49
> [ 1.955918][ T1] in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: 1, name: swapper/0
> [ 1.956024][ T1] INFO: lockdep is turned off.
> [ 1.956094][ T1] irq event stamp: 14792
> [ 1.956142][ T1] hardirqs last enabled at (14791): [<c0000000001f8fe8>] .__up_console_sem+0xc8/0x110
> [ 1.956269][ T1] hardirqs last disabled at (14792): [<c0000000000312f4>] .system_call_exception+0x54/0x2d0
> [ 1.956384][ T1] softirqs last enabled at (10586): [<c000000000efd6d4>] .release_sock+0xb4/0x100
> [ 1.956497][ T1] softirqs last disabled at (10584): [<c000000000efd654>] .release_sock+0x34/0x100
> [ 1.956726][ T1] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G D 5.14.0-rc3-next-20210729 #1
> [ 1.956846][ T1] Call Trace:
> [ 1.956885][ T1] [c000000008687700] [c0000000009fe088] .dump_stack_lvl+0xa4/0x100 (unreliable)
> [ 1.957011][ T1] [c000000008687790] [c000000000193060] .___might_sleep+0x2b0/0x2f0
> [ 1.957130][ T1] [c000000008687820] [c00000000015fba0] .exit_signals+0x50/0x540
> [ 1.957238][ T1] [c0000000086878e0] [c000000000148dbc] .do_exit+0xec/0xe20
> [ 1.957335][ T1] [c0000000086879c0] [c000000000029bc4] .oops_end+0x144/0x210
> [ 1.957441][ T1] [c000000008687a50] [c0000000000095d4] program_check_common_virt+0x2d4/0x320
> [ 1.957561][ T1] --- interrupt: 700 at .system_call_exception+0x70/0x2d0
> [ 1.957645][ T1] NIP: c000000000031310 LR: c0000000000312f4 CTR: c00000000000c5f0
> [ 1.957715][ T1] REGS: c000000008687ac0 TRAP: 0700 Tainted: G D (5.14.0-rc3-next-20210729)
> [ 1.957799][ T1] MSR: 8000000000029032 <SF,EE,ME,IR,DR,RI> CR: 24002222 XER: 00000000
> [ 1.957919][ T1] CFAR: c000000000300360 IRQMASK: 3
> [ 1.957919][ T1] GPR00: c0000000000312f4 c000000008687d60 c0000000022ee300 0000000000000003
> [ 1.957919][ T1] GPR04: 000000000000033f 0000000000000400 0000000000000000 000000003e5a0000
> [ 1.957919][ T1] GPR08: 000000003e5a0000 0000000000000001 0000000000000000 0000000000000003
> [ 1.957919][ T1] GPR12: ffffffffffffffff c000000002fb0000 c0000000000129c0 0000000000000000
> [ 1.957919][ T1] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [ 1.957919][ T1] GPR20: 0000000000000000 0000000000000000 0000000000000000 c0000000021d8b00
> [ 1.957919][ T1] GPR24: 0000000000000000 0000000000000000 ffffffffffffffff 0000000000000400
> [ 1.957919][ T1] GPR28: 000000000000033f 000000000000f134 c00000000176c068 c000000008687e80
> [ 1.958633][ T1] NIP [c000000000031310] .system_call_exception+0x70/0x2d0
> [ 1.958720][ T1] LR [c0000000000312f4] .system_call_exception+0x54/0x2d0
> [ 1.958803][ T1] --- interrupt: 700
> [ 1.958842][ T1] [c000000008687e10] [c00000000000c6e4] system_call_common+0xf4/0x258
> [ 1.958961][ T1] --- interrupt: c00 at .ucall_norets+0x4/0x14
> [ 1.959055][ T1] NIP: c000000000079ce0 LR: c0000000000fa274 CTR: 0000000000000000
> [ 1.959127][ T1] REGS: c000000008687e80 TRAP: 0c00 Tainted: G D (5.14.0-rc3-next-20210729)
> [ 1.959210][ T1] MSR: 8000000002009032 <SF,VEC,EE,ME,IR,DR,RI> CR: 24002222 XER: 00000000
> [ 1.959337][ T1] IRQMASK: 0
> [ 1.959337][ T1] GPR00: c00000000176c068 c000000008687a30 c0000000022ee300 000000000000f134
> [ 1.959337][ T1] GPR04: 000000000000033f 0000000000000400 ffffffffffffffff 0000000000000000
> [ 1.959337][ T1] GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [ 1.959337][ T1] GPR12: 0000000000000000 c000000002fb0000 c0000000000129c0 0000000000000000
> [ 1.959337][ T1] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [ 1.959337][ T1] GPR20: 0000000000000000 0000000000000000 0000000000000000 c0000000021d8b00
> [ 1.959337][ T1] GPR24: c0000000017c1088 0000000000000004 c00000000171c450 c000000002338e40
> [ 1.959337][ T1] GPR28: c0000000017c10b0 c0000000033f0000 0000000000000400 c0000000033f0000
> [ 1.960042][ T1] NIP [c000000000079ce0] .ucall_norets+0x4/0x14
> [ 1.960137][ T1] LR [c0000000000fa274] .set_memory_encrypted+0x44/0x80
> [ 1.960219][ T1] --- interrupt: c00
> [ 1.960257][ T1] [c000000008687a30] [c00000000176c068] .swiotlb_exit+0xbc/0x180 (unreliable)
> [ 1.960845][ T1] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000005
> qemu-system-ppc64: OS terminated: OS panic: Attempted to kill init! exitcode=0x00000005
>
> ---
> Bisect log:
>
> # bad: [8d4b477da1a807199ca60e0829357ce7aa6758d5] Add linux-next specific files for 20210730
> # good: [ff1176468d368232b684f75e82563369208bc371] Linux 5.14-rc3
> git bisect start 'HEAD' 'v5.14-rc3'
> # bad: [8f3eb1f5c702ef868d89799b03c21d122f2fe197] Merge remote-tracking branch 'bpf-next/for-next'
> git bisect bad 8f3eb1f5c702ef868d89799b03c21d122f2fe197
> # good: [02afbb8f68dc8b37c07e457d2f69d440781af23a] Merge remote-tracking branch 'cifsd/cifsd-for-next'
> git bisect good 02afbb8f68dc8b37c07e457d2f69d440781af23a
> # good: [3e12361b6d23f793580a50a6008633501c56ea1d] bcm63xx_enet: delete a redundant assignment
> git bisect good 3e12361b6d23f793580a50a6008633501c56ea1d
> # good: [78d788681492abe4980d5cc9b93b70df9f028880] Merge remote-tracking branch 'jc_docs/docs-next'
> git bisect good 78d788681492abe4980d5cc9b93b70df9f028880
> # good: [266234e7659f731cf471a1bdc4fd1ead4caa8303] Merge remote-tracking branch 'v4l-dvb-next/master'
> git bisect good 266234e7659f731cf471a1bdc4fd1ead4caa8303
> # bad: [be1841fe3ca0010139fe4fb44dd42dea7c0e3401] Merge remote-tracking branch 'swiotlb/linux-next'
> git bisect bad be1841fe3ca0010139fe4fb44dd42dea7c0e3401
> # good: [7b8798617c7975d10678d99dcc59d103e237b4cd] Merge remote-tracking branch 'ieee1394/for-next'
> git bisect good 7b8798617c7975d10678d99dcc59d103e237b4cd
> # good: [463e862ac63ef27fca423782536f6465abc3f180] swiotlb: Convert io_default_tlb_mem to static allocation
> git bisect good 463e862ac63ef27fca423782536f6465abc3f180
> # good: [2dc6b1158c28c3a5e86d162628810312f98d5e97] fs: dlm: introduce generic listen
> git bisect good 2dc6b1158c28c3a5e86d162628810312f98d5e97
> # good: [62699b3f0a62435fceb8debf295e90a5ea259e04] fs: dlm: move receive loop into receive handler
> git bisect good 62699b3f0a62435fceb8debf295e90a5ea259e04
> # bad: [ad6c00283163cb7ad52cdf97d2850547446f7d98] swiotlb: Free tbl memory in swiotlb_exit()
> git bisect bad ad6c00283163cb7ad52cdf97d2850547446f7d98
> # good: [1efd3fc0ccf52e1aa5f0bf5b0d82847180d20951] swiotlb: Emit diagnostic in swiotlb_exit()
> git bisect good 1efd3fc0ccf52e1aa5f0bf5b0d82847180d20951
> # first bad commit: [ad6c00283163cb7ad52cdf97d2850547446f7d98] swiotlb: Free tbl memory in swiotlb_exit()
> _______________________________________________
> iommu mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
On 7/31/21 7:29 PM, Konrad Rzeszutek Wilk wrote:
> On Sat, Jul 31, 2021 at 11:26:11AM -0700, Guenter Roeck wrote:
>> Hi,
>>
>> On Tue, Jul 20, 2021 at 02:38:26PM +0100, Will Deacon wrote:
>>> Although swiotlb_exit() frees the 'slots' metadata array referenced by
>>> 'io_tlb_default_mem', it leaves the underlying buffer pages allocated
>>> despite no longer being usable.
>>>
>>> Extend swiotlb_exit() to free the buffer pages as well as the slots
>>> array.
>>>
>>
>> This patch causes qemu pseries emulations to crash. Backtrace and bisect
>> log see below. Reverting it fixes the problem.
>
> I am 99% sure it is fixed by this patch (which should be in linux-next
> in 5 minutes):
>
Yes, it does.
FWIW:
Tested-by: Guenter Roeck <[email protected]>
Thanks!
Guenter
>
>>From a449ffaf9181b5a2dc705d8a06b13e0068207fd4 Mon Sep 17 00:00:00 2001
> From: Will Deacon <[email protected]>
> Date: Fri, 30 Jul 2021 12:42:31 +0100
> Subject: [PATCH] powerpc/svm: Don't issue ultracalls if !mem_encrypt_active()
>
> Commit ad6c00283163 ("swiotlb: Free tbl memory in swiotlb_exit()")
> introduced a set_memory_encrypted() call to swiotlb_exit() so that the
> buffer pages are returned to an encrypted state prior to being freed.
>
> Sachin reports that this leads to the following crash on a Power server:
>
> [ 0.010799] software IO TLB: tearing down default memory pool
> [ 0.010805] ------------[ cut here ]------------
> [ 0.010808] kernel BUG at arch/powerpc/kernel/interrupt.c:98!
>
> Nick spotted that this is because set_memory_encrypted() is issuing an
> ultracall which doesn't exist for the processor, and should therefore
> be gated by mem_encrypt_active() to mirror the x86 implementation.
>
> Cc: Konrad Rzeszutek Wilk <[email protected]>
> Cc: Claire Chang <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Fixes: ad6c00283163 ("swiotlb: Free tbl memory in swiotlb_exit()")
> Suggested-by: Nicholas Piggin <[email protected]>
> Reported-by: Sachin Sant <[email protected]>
> Tested-by: Sachin Sant <[email protected]>
> Tested-by: Nathan Chancellor <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]/
> Signed-off-by: Will Deacon <[email protected]>
> Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
> ---
> arch/powerpc/platforms/pseries/svm.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/svm.c b/arch/powerpc/platforms/pseries/svm.c
> index 1d829e257996..87f001b4c4e4 100644
> --- a/arch/powerpc/platforms/pseries/svm.c
> +++ b/arch/powerpc/platforms/pseries/svm.c
> @@ -63,6 +63,9 @@ void __init svm_swiotlb_init(void)
>
> int set_memory_encrypted(unsigned long addr, int numpages)
> {
> + if (!mem_encrypt_active())
> + return 0;
> +
> if (!PAGE_ALIGNED(addr))
> return -EINVAL;
>
> @@ -73,6 +76,9 @@ int set_memory_encrypted(unsigned long addr, int numpages)
>
> int set_memory_decrypted(unsigned long addr, int numpages)
> {
> + if (!mem_encrypt_active())
> + return 0;
> +
> if (!PAGE_ALIGNED(addr))
> return -EINVAL;
>
>