Re: [PATCH v2 0/4] Fix restricted DMA vs swiotlb_exit()
From: Christian Borntraeger
Date: Fri Jul 23 2021 - 04:51:25 EST
On 23.07.21 10:47, Halil Pasic wrote:
On Fri, 23 Jul 2021 08:14:19 +0200
Christian Borntraeger <borntraeger@xxxxxxxxxx> 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 <borntraeger@xxxxxxxxxx> 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/20210719123054.6844-1-will@xxxxxxxxxx
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 <tientzu@xxxxxxxxxxxx>
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 <pasic@xxxxxxxxxxxxx>
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 <pasic@xxxxxxxxxxxxx>
I can confirm that this fixes the problem. This also makes sense codewise.
Tested-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
Reviewed-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
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)