On Wed, 10 Apr 2019 10:42:51 +0200
Cornelia Huck <cohuck@xxxxxxxxxx> wrote:
> On Fri, 5 Apr 2019 01:16:17 +0200
> Halil Pasic <pasic@xxxxxxxxxxxxx> 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?
>
Yes.
> >
> > Signed-off-by: Halil Pasic <pasic@xxxxxxxxxxxxx>
> > ---
> > 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.
>
By wrappers you mean just the macros or also the inline functions?
If we agree to go with the cio DMA pool instead of using DMA API
facilities for allocation (dma_alloc_coherent or maybe a per ccw-device
dma_pool) I think I could just use cio_dma_zalloc() directly if you like.
I was quite insecure about how this gen_pool idea is going to be received
here. That's why I decided to keep the dma_alloc_coherent() version in
for the RFC.
If you prefer I can squash patches #7 #9 #10 and #11 together and
pull #8 forward. Would you prefer that?
> > +
> > 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?
Good catch!
I could take care of it in __vc_dma_free().
void cio_dma_free(void *cpu_addr, size_t size) {
+ if (!cpu_addr)
+ return;
also seems to me like a good idea right now.
>
> > }
> >
> > 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?)
Right the move was supposed to take place in patch #2. Not sure how
I ended up with this. Maybe a messed up rebase.
>
> > vcdev->vdev.dev.release = virtio_ccw_release_dev;
> > vcdev->vdev.config = &virtio_ccw_config_ops;
> > vcdev->cdev = cdev;
>
