Re: [PATCH v2 0/4] Fix restricted DMA vs swiotlb_exit()

[Christian Borntraeger <borntraeger@de.ibm.com>]

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 <borntraeger@de.ibm.com> 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@de.ibm.com> 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@kernel.org
>>>>>>>
>>>>>>> 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@chromium.org>
>>>>>> 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@linux.ibm.com>
>>>>> 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 <borntraeger@de.ibm.com>

so that you can take this via the swiotlb tree.

>>
>>>
>>>>
>>>>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
>>>>
>>>> I can confirm that this fixes the problem. This also makes sense codewise.
>>>>
>>>> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>
>>> 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)
>>>