On Fri, 5 Apr 2019 01:16:17 +0200 Halil Pasic <[email protected]> wrote:
> Before virtio-ccw could get away with not using DMA API for the pieces of > memory it does ccw I/O with. With protected virtualization this has to > change, since the hypervisor needs to read and sometimes also write these > pieces of memory. > > Let us make sure all ccw I/O is done through shared memory. > > Note: The control blocks of I/O instructions do not need to be shared. > These are marshalled by the ultravisor. Ok, so direct parameters of I/O instructions are handled by the ultravisor? > > Signed-off-by: Halil Pasic <[email protected]> > --- > drivers/s390/virtio/virtio_ccw.c | 177 > +++++++++++++++++++++++---------------- > 1 file changed, 107 insertions(+), 70 deletions(-) > (...) > @@ -167,6 +170,28 @@ static struct virtio_ccw_device *to_vc_device(struct > virtio_device *vdev) > return container_of(vdev, struct virtio_ccw_device, vdev); > } > > +#define vc_dma_decl_struct(type, field) \ > + dma_addr_t field ## _dma_addr; \ > + struct type *field > + > +static inline void *__vc_dma_alloc(struct virtio_device *vdev, size_t size, > + dma_addr_t *dma_handle) > +{ > + return dma_alloc_coherent(vdev->dev.parent, size, dma_handle, > + GFP_DMA | GFP_KERNEL | __GFP_ZERO); > +} > + > +static inline void __vc_dma_free(struct virtio_device *vdev, size_t size, > + void *cpu_addr, dma_addr_t dma_handle) > +{ > + dma_free_coherent(vdev->dev.parent, size, cpu_addr, dma_handle); > +} > + > +#define vc_dma_alloc_struct(vdev, ptr) \ > + ({ ptr = __vc_dma_alloc(vdev, (sizeof(*(ptr))), &(ptr ## _dma_addr)); }) > +#define vc_dma_free_struct(vdev, ptr) \ > + __vc_dma_free(vdev, sizeof(*(ptr)), (ptr), (ptr ## _dma_addr)) Not sure I'm a fan of those wrappers... I think they actually hurt readability of the code. > + > static void drop_airq_indicator(struct virtqueue *vq, struct airq_info *info) > { > unsigned long i, flags; > @@ -322,12 +347,12 @@ static void virtio_ccw_drop_indicator(struct > virtio_ccw_device *vcdev, > { > int ret; > unsigned long *indicatorp = NULL; > - struct virtio_thinint_area *thinint_area = NULL; > + vc_dma_decl_struct(virtio_thinint_area, thinint_area) = NULL; > + dma_addr_t indicatorp_dma_addr; > struct airq_info *airq_info = vcdev->airq_info; > > if (vcdev->is_thinint) { > - thinint_area = kzalloc(sizeof(*thinint_area), > - GFP_DMA | GFP_KERNEL); > + vc_dma_alloc_struct(&vcdev->vdev, thinint_area); > if (!thinint_area) > return; > thinint_area->summary_indicator = > @@ -338,8 +363,9 @@ static void virtio_ccw_drop_indicator(struct > virtio_ccw_device *vcdev, > ccw->cda = (__u32)(unsigned long) thinint_area; > } else { > /* payload is the address of the indicators */ > - indicatorp = kmalloc(sizeof(&vcdev->indicators), > - GFP_DMA | GFP_KERNEL); > + indicatorp = __vc_dma_alloc(&vcdev->vdev, > + sizeof(&vcdev->indicators), > + &indicatorp_dma_addr); > if (!indicatorp) > return; > *indicatorp = 0; > @@ -359,8 +385,10 @@ static void virtio_ccw_drop_indicator(struct > virtio_ccw_device *vcdev, > "Failed to deregister indicators (%d)\n", ret); > else if (vcdev->is_thinint) > virtio_ccw_drop_indicators(vcdev); > - kfree(indicatorp); > - kfree(thinint_area); > + if (indicatorp) > + __vc_dma_free(&vcdev->vdev, sizeof(&vcdev->indicators), > + indicatorp, indicatorp_dma_addr); > + vc_dma_free_struct(&vcdev->vdev, thinint_area); Don't you need to check for !NULL here as well? > } > > static inline long __do_kvm_notify(struct subchannel_id schid, (...) > @@ -1280,7 +1318,6 @@ static int virtio_ccw_online(struct ccw_device *cdev) > > vcdev->is_thinint = virtio_ccw_use_airq; /* at least try */ > > - vcdev->vdev.dev.parent = &cdev->dev; Hm? (You added a line like that in a previous patch; should it simply have been a movement instead? Or am I misremembering?) > vcdev->vdev.dev.release = virtio_ccw_release_dev; > vcdev->vdev.config = &virtio_ccw_config_ops; > vcdev->cdev = cdev; _______________________________________________ Virtualization mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/virtualization
