Re: [PATCH v2 1/1] s390/cio: fix virtio-ccw DMA without PV

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

On 30.09.19 17:38, Halil Pasic wrote:
> Commit 37db8985b211 ("s390/cio: add basic protected virtualization
> support") breaks virtio-ccw devices with VIRTIO_F_IOMMU_PLATFORM for non
> Protected Virtualization (PV) guests. The problem is that the dma_mask
> of the ccw device, which is used by virtio core, gets changed from 64 to
> 31 bit, because some of the DMA allocations do require 31 bit
> addressable memory. For PV the only drawback is that some of the virtio
> structures must end up in ZONE_DMA because we have the bounce the
> buffers mapped via DMA API anyway.
> 
> But for non PV guests we have a problem: because of the 31 bit mask
> guests bigger than 2G are likely to try bouncing buffers. The swiotlb
> however is only initialized for PV guests, because we don't want to
> bounce anything for non PV guests. The first such map kills the guest.
> 
> Since the DMA API won't allow us to specify for each allocation whether
> we need memory from ZONE_DMA (31 bit addressable) or any DMA capable
> memory will do, let us use coherent_dma_mask (which is used for
> allocations) to force allocating form ZONE_DMA while changing dma_mask
> to DMA_BIT_MASK(64) so that at least the streaming API will regard
> the whole memory DMA capable.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> Suggested-by: Robin Murphy <robin.murphy@arm.com>
> Fixes: 37db8985b211 ("s390/cio: add basic protected virtualization support")

Thanks applied to the s390 tree. (pending some regression tests)


> ---
> 
> v1 --> v2:
> * Fixed comment: dropped the sentence with workaround.
> 
> The idea of enabling the client code to specify on s390 whether a chunk
> of allocated DMA memory is to be allocated form ZONE_DMA for each
> allocation was not well received [1]. 
> 
> Making the streaming API threat all addresses as DMA capable, while
> restricting the DMA API allocations to  ZONE_DMA (regardless of needed
> or not) is the next best thing we can do (from s390 perspective).
> 
> [1] https://lkml.org/lkml/2019/9/23/531 
> ---
> ---
>  drivers/s390/cio/cio.h    | 1 +
>  drivers/s390/cio/css.c    | 7 ++++++-
>  drivers/s390/cio/device.c | 2 +-
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/cio/cio.h b/drivers/s390/cio/cio.h
> index ba7d2480613b..dcdaba689b20 100644
> --- a/drivers/s390/cio/cio.h
> +++ b/drivers/s390/cio/cio.h
> @@ -113,6 +113,7 @@ struct subchannel {
>  	enum sch_todo todo;
>  	struct work_struct todo_work;
>  	struct schib_config config;
> +	u64 dma_mask;
>  	char *driver_override; /* Driver name to force a match */
>  } __attribute__ ((aligned(8)));
>  
> diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
> index 22c55816100b..a05dbf2e97db 100644
> --- a/drivers/s390/cio/css.c
> +++ b/drivers/s390/cio/css.c
> @@ -232,7 +232,12 @@ struct subchannel *css_alloc_subchannel(struct subchannel_id schid,
>  	 * belong to a subchannel need to fit 31 bit width (e.g. ccw).
>  	 */
>  	sch->dev.coherent_dma_mask = DMA_BIT_MASK(31);
> -	sch->dev.dma_mask = &sch->dev.coherent_dma_mask;
> +	/*
> +	 * But we don't have such restrictions imposed on the stuff that
> +	 * is handled by the streaming API.
> +	 */
> +	sch->dma_mask = DMA_BIT_MASK(64);
> +	sch->dev.dma_mask = &sch->dma_mask;
>  	return sch;
>  
>  err:
> diff --git a/drivers/s390/cio/device.c b/drivers/s390/cio/device.c
> index 131430bd48d9..0c6245fc7706 100644
> --- a/drivers/s390/cio/device.c
> +++ b/drivers/s390/cio/device.c
> @@ -710,7 +710,7 @@ static struct ccw_device * io_subchannel_allocate_dev(struct subchannel *sch)
>  	if (!cdev->private)
>  		goto err_priv;
>  	cdev->dev.coherent_dma_mask = sch->dev.coherent_dma_mask;
> -	cdev->dev.dma_mask = &cdev->dev.coherent_dma_mask;
> +	cdev->dev.dma_mask = sch->dev.dma_mask;
>  	dma_pool = cio_gp_dma_create(&cdev->dev, 1);
>  	if (!dma_pool)
>  		goto err_dma_pool;
>