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

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

[Christian Borntraeger]

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.

> 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>

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)
> 

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

[Christian Borntraeger]

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.

> 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>

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)
> 

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

[Halil Pasic]

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...)

> 
> > 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)
> >   

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

[Christian Borntraeger]

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.

> 
>>
>>> 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)
>>>    
> 

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

[Konrad Rzeszutek Wilk]

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?
> 
> > 
> > > 
> > > > 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)
> > 

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

[Christian Borntraeger]

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)
>>>

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

[Halil Pasic]

On Fri, 23 Jul 2021 19:53:58 +0200
Christian Borntraeger <borntraeger@de.ibm.com> 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 <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.

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