| Message ID | 20190404231622.52531-11-pasic@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390: virtio: support protected virtualization | expand |
On Fri, 5 Apr 2019 01:16:20 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > We can reduce the number of DMA allocations by pulling the bits > of memory that belongs to virtio_ccw_device and needs to be DMA > memory into a single area. That makes a lot of sense (maybe start with introducing the dma area from the beginning?), but... > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > --- > drivers/s390/virtio/virtio_ccw.c | 48 ++++++++++++++-------------------------- > 1 file changed, 16 insertions(+), 32 deletions(-) > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > index aa45a6a027ae..7268149f2ee8 100644 > --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -49,12 +49,12 @@ struct vq_config_block { > struct vcdev_dma_area { > unsigned long indicators; > unsigned long indicators2; > + struct vq_config_block config_block; > + __u8 status; /* TODO check __aligned(8); */ ...I think that needs attention. > }; > > struct virtio_ccw_device { > struct virtio_device vdev; > - __u8 *status; > - dma_addr_t status_dma_addr; > __u8 config[VIRTIO_CCW_CONFIG_SIZE]; > struct ccw_device *cdev; > __u32 curr_io;
On Wed, 10 Apr 2019 10:46:49 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Fri, 5 Apr 2019 01:16:20 +0200 > Halil Pasic <pasic@linux.ibm.com> wrote: > > > We can reduce the number of DMA allocations by pulling the bits > > of memory that belongs to virtio_ccw_device and needs to be DMA > > memory into a single area. > > That makes a lot of sense (maybe start with introducing the dma area > from the beginning?), but... > Yeah, as I wrote at #7 once we are in clear how the code should look like in the end, it might make sense to squash a couple of patches together. > > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > > --- > > drivers/s390/virtio/virtio_ccw.c | 48 ++++++++++++++-------------------------- > > 1 file changed, 16 insertions(+), 32 deletions(-) > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > index aa45a6a027ae..7268149f2ee8 100644 > > --- a/drivers/s390/virtio/virtio_ccw.c > > +++ b/drivers/s390/virtio/virtio_ccw.c > > @@ -49,12 +49,12 @@ struct vq_config_block { > > struct vcdev_dma_area { > > unsigned long indicators; > > unsigned long indicators2; > > + struct vq_config_block config_block; > > + __u8 status; /* TODO check __aligned(8); */ > > ...I think that needs attention. Yes I wanted to discuss this with you. I could not find anything in the virtio spec that would put requirements on how this status field needs to be aligned. But I did not look to hard. The ccw.cda can hold an arbitrary data address AFAIR (for indirect, of course we do have alignment requirements). Apparently status used to be a normal field, and became a pointer with 73fa21ea4fc6 "KVM: s390: Dynamic allocation of virtio-ccw I/O data." (Cornelia Huck, 2013-01-07). I could not quite figure out why. So maybe dropping the TODO comment will do just fine. What do you think? Regards, Halil > > > }; > > > > struct virtio_ccw_device { > > struct virtio_device vdev; > > - __u8 *status; > > - dma_addr_t status_dma_addr; > > __u8 config[VIRTIO_CCW_CONFIG_SIZE]; > > struct ccw_device *cdev; > > __u32 curr_io; >
On Wed, 10 Apr 2019 17:12:54 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > On Wed, 10 Apr 2019 10:46:49 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > > > On Fri, 5 Apr 2019 01:16:20 +0200 > > Halil Pasic <pasic@linux.ibm.com> wrote: > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > > index aa45a6a027ae..7268149f2ee8 100644 > > > --- a/drivers/s390/virtio/virtio_ccw.c > > > +++ b/drivers/s390/virtio/virtio_ccw.c > > > @@ -49,12 +49,12 @@ struct vq_config_block { > > > struct vcdev_dma_area { > > > unsigned long indicators; > > > unsigned long indicators2; > > > + struct vq_config_block config_block; > > > + __u8 status; /* TODO check __aligned(8); */ > > > > ...I think that needs attention. > > Yes I wanted to discuss this with you. I could not find anything > in the virtio spec that would put requirements on how this > status field needs to be aligned. But I did not look to hard. > > The ccw.cda can hold an arbitrary data address AFAIR (for indirect, > of course we do have alignment requirements). I think it needs to be doubleword aligned. > > Apparently status used to be a normal field, and became a pointer with > 73fa21ea4fc6 "KVM: s390: Dynamic allocation of virtio-ccw I/O > data." (Cornelia Huck, 2013-01-07). I could not quite figure out why. In the beginning, the code used a below-2G-area for all commands. Rather than adding locking to avoid races there, that commit switches to allocating the needed structures individually. The status field needed to be below 2G, so it needed to be allocated separately. > > So maybe dropping the TODO comment will do just fine. What do you think?
On Wed, 10 Apr 2019 18:36:43 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Wed, 10 Apr 2019 17:12:54 +0200 > Halil Pasic <pasic@linux.ibm.com> wrote: > > > On Wed, 10 Apr 2019 10:46:49 +0200 > > Cornelia Huck <cohuck@redhat.com> wrote: > > > > > On Fri, 5 Apr 2019 01:16:20 +0200 > > > Halil Pasic <pasic@linux.ibm.com> wrote: > > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > > > index aa45a6a027ae..7268149f2ee8 100644 > > > > --- a/drivers/s390/virtio/virtio_ccw.c > > > > +++ b/drivers/s390/virtio/virtio_ccw.c > > > > @@ -49,12 +49,12 @@ struct vq_config_block { > > > > struct vcdev_dma_area { > > > > unsigned long indicators; > > > > unsigned long indicators2; > > > > + struct vq_config_block config_block; > > > > + __u8 status; /* TODO check __aligned(8); */ > > > > > > ...I think that needs attention. > > > > Yes I wanted to discuss this with you. I could not find anything > > in the virtio spec that would put requirements on how this > > status field needs to be aligned. But I did not look to hard. > > > > The ccw.cda can hold an arbitrary data address AFAIR (for indirect, > > of course we do have alignment requirements). > > I think it needs to be doubleword aligned. > I've re-read the part of the PoP that describes the ccw formats. And it reinforced my position: for IDA and MIDA we need proper alignment, but if the CCW ain't an indirect one there is no alignment requirement. QEMU also does not seem to check either. Can you double-check and provide me with a reference that proves me wrong if I'm wrong. > > > > Apparently status used to be a normal field, and became a pointer with > > 73fa21ea4fc6 "KVM: s390: Dynamic allocation of virtio-ccw I/O > > data." (Cornelia Huck, 2013-01-07). I could not quite figure out why. > > In the beginning, the code used a below-2G-area for all commands. > Rather than adding locking to avoid races there, that commit switches > to allocating the needed structures individually. The status field > needed to be below 2G, so it needed to be allocated separately. > I get it now. The confusing part was that the field 'area' was about holding the address of the also previously dynamically allocated below 2G area that was used for talking to the hypervisor via CCW I/O. > > > > So maybe dropping the TODO comment will do just fine. What do you think? > I still think we just need to drop the comment, as we don't have to align it. Regards, Halil
On Wed, 10 Apr 2019 19:48:49 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > On Wed, 10 Apr 2019 18:36:43 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > > > On Wed, 10 Apr 2019 17:12:54 +0200 > > Halil Pasic <pasic@linux.ibm.com> wrote: > > > > > On Wed, 10 Apr 2019 10:46:49 +0200 > > > Cornelia Huck <cohuck@redhat.com> wrote: > > > > > > > On Fri, 5 Apr 2019 01:16:20 +0200 > > > > Halil Pasic <pasic@linux.ibm.com> wrote: > > > > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > > > > index aa45a6a027ae..7268149f2ee8 100644 > > > > > --- a/drivers/s390/virtio/virtio_ccw.c > > > > > +++ b/drivers/s390/virtio/virtio_ccw.c > > > > > @@ -49,12 +49,12 @@ struct vq_config_block { > > > > > struct vcdev_dma_area { > > > > > unsigned long indicators; > > > > > unsigned long indicators2; > > > > > + struct vq_config_block config_block; > > > > > + __u8 status; /* TODO check __aligned(8); */ > > > > > > > > ...I think that needs attention. > > > > > > Yes I wanted to discuss this with you. I could not find anything > > > in the virtio spec that would put requirements on how this > > > status field needs to be aligned. But I did not look to hard. > > > > > > The ccw.cda can hold an arbitrary data address AFAIR (for indirect, > > > of course we do have alignment requirements). > > > > I think it needs to be doubleword aligned. > > > > I've re-read the part of the PoP that describes the ccw formats. And > it reinforced my position: for IDA and MIDA we need proper alignment, > but if the CCW ain't an indirect one there is no alignment requirement. > > QEMU also does not seem to check either. > > Can you double-check and provide me with a reference that proves me > wrong if I'm wrong. Ah, it was the ccw itself, not the cda. Indeed, there do not seem to be any requirements for direct addressing. > > > > > > > Apparently status used to be a normal field, and became a pointer with > > > 73fa21ea4fc6 "KVM: s390: Dynamic allocation of virtio-ccw I/O > > > data." (Cornelia Huck, 2013-01-07). I could not quite figure out why. > > > > In the beginning, the code used a below-2G-area for all commands. > > Rather than adding locking to avoid races there, that commit switches > > to allocating the needed structures individually. The status field > > needed to be below 2G, so it needed to be allocated separately. > > > > I get it now. The confusing part was that the field 'area' was about > holding the address of the also previously dynamically allocated > below 2G area that was used for talking to the hypervisor via CCW I/O. > > > > > > > So maybe dropping the TODO comment will do just fine. What do you think? > > > > I still think we just need to drop the comment, as we don't have to > align it. Agreed.
On Thu, 11 Apr 2019 11:24:46 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Wed, 10 Apr 2019 19:48:49 +0200 > Halil Pasic <pasic@linux.ibm.com> wrote: > > > On Wed, 10 Apr 2019 18:36:43 +0200 > > Cornelia Huck <cohuck@redhat.com> wrote: > > > > > On Wed, 10 Apr 2019 17:12:54 +0200 > > > Halil Pasic <pasic@linux.ibm.com> wrote: > > > > > > > On Wed, 10 Apr 2019 10:46:49 +0200 > > > > Cornelia Huck <cohuck@redhat.com> wrote: > > > > > > > > > On Fri, 5 Apr 2019 01:16:20 +0200 > > > > > Halil Pasic <pasic@linux.ibm.com> wrote: > > > > > > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > > > > > index aa45a6a027ae..7268149f2ee8 100644 > > > > > > --- a/drivers/s390/virtio/virtio_ccw.c > > > > > > +++ b/drivers/s390/virtio/virtio_ccw.c > > > > > > @@ -49,12 +49,12 @@ struct vq_config_block { > > > > > > struct vcdev_dma_area { > > > > > > unsigned long indicators; > > > > > > unsigned long indicators2; > > > > > > + struct vq_config_block config_block; > > > > > > + __u8 status; /* TODO check __aligned(8); */ > > > > > > > > > > ...I think that needs attention. > > > > > > > > Yes I wanted to discuss this with you. I could not find anything > > > > in the virtio spec that would put requirements on how this > > > > status field needs to be aligned. But I did not look to hard. > > > > > > > > The ccw.cda can hold an arbitrary data address AFAIR (for indirect, > > > > of course we do have alignment requirements). > > > > > > I think it needs to be doubleword aligned. > > > > > > > I've re-read the part of the PoP that describes the ccw formats. And > > it reinforced my position: for IDA and MIDA we need proper alignment, > > but if the CCW ain't an indirect one there is no alignment requirement. > > > > QEMU also does not seem to check either. > > > > Can you double-check and provide me with a reference that proves me > > wrong if I'm wrong. > > Ah, it was the ccw itself, not the cda. Indeed, there do not seem to be > any requirements for direct addressing. > > > > > > > > > > > Apparently status used to be a normal field, and became a pointer with > > > > 73fa21ea4fc6 "KVM: s390: Dynamic allocation of virtio-ccw I/O > > > > data." (Cornelia Huck, 2013-01-07). I could not quite figure out why. > > > > > > In the beginning, the code used a below-2G-area for all commands. > > > Rather than adding locking to avoid races there, that commit switches > > > to allocating the needed structures individually. The status field > > > needed to be below 2G, so it needed to be allocated separately. > > > > > > > I get it now. The confusing part was that the field 'area' was about > > holding the address of the also previously dynamically allocated > > below 2G area that was used for talking to the hypervisor via CCW I/O. > > > > > > > > > > So maybe dropping the TODO comment will do just fine. What do you think? > > > > > > > I still think we just need to drop the comment, as we don't have to > > align it. > > Agreed. > Will do. Thanks for double-checking! Regards, Halil
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c index aa45a6a027ae..7268149f2ee8 100644 --- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c @@ -49,12 +49,12 @@ struct vq_config_block { struct vcdev_dma_area { unsigned long indicators; unsigned long indicators2; + struct vq_config_block config_block; + __u8 status; /* TODO check __aligned(8); */ }; struct virtio_ccw_device { struct virtio_device vdev; - __u8 *status; - dma_addr_t status_dma_addr; __u8 config[VIRTIO_CCW_CONFIG_SIZE]; struct ccw_device *cdev; __u32 curr_io; @@ -64,8 +64,6 @@ struct virtio_ccw_device { spinlock_t lock; struct mutex io_lock; /* Serializes I/O requests */ struct list_head virtqueues; - struct vq_config_block *config_block; - dma_addr_t config_block_dma_addr; bool is_thinint; bool going_away; bool device_lost; @@ -450,15 +448,15 @@ static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev, { int ret; - vcdev->config_block->index = index; + vcdev->dma_area->config_block.index = index; ccw->cmd_code = CCW_CMD_READ_VQ_CONF; ccw->flags = 0; ccw->count = sizeof(struct vq_config_block); - ccw->cda = (__u32)(unsigned long)(vcdev->config_block); + ccw->cda = (__u32)(unsigned long)(&vcdev->dma_area->config_block); ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_READ_VQ_CONF); if (ret) return ret; - return vcdev->config_block->num ?: -ENOENT; + return vcdev->dma_area->config_block.num ?: -ENOENT; } static void virtio_ccw_del_vq(struct virtqueue *vq, struct ccw1 *ccw) @@ -754,7 +752,7 @@ static void virtio_ccw_reset(struct virtio_device *vdev) return; /* Zero status bits. */ - *vcdev->status = 0; + vcdev->dma_area->status = 0; /* Send a reset ccw on device. */ ccw->cmd_code = CCW_CMD_VDEV_RESET; @@ -967,11 +965,11 @@ static void virtio_ccw_set_config(struct virtio_device *vdev, static u8 virtio_ccw_get_status(struct virtio_device *vdev) { struct virtio_ccw_device *vcdev = to_vc_device(vdev); - u8 old_status = *vcdev->status; + u8 old_status = vcdev->dma_area->status; vc_dma_decl_struct(ccw1, ccw); if (vcdev->revision < 1) - return *vcdev->status; + return vcdev->dma_area->status; vc_dma_alloc_struct(vdev, ccw); if (!ccw) @@ -979,24 +977,24 @@ static u8 virtio_ccw_get_status(struct virtio_device *vdev) ccw->cmd_code = CCW_CMD_READ_STATUS; ccw->flags = 0; - ccw->count = sizeof(*vcdev->status); - ccw->cda = (__u32)(unsigned long)vcdev->status; + ccw->count = sizeof(vcdev->dma_area->status); + ccw->cda = (__u32)(unsigned long)&vcdev->dma_area->status; ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_READ_STATUS); /* * If the channel program failed (should only happen if the device * was hotunplugged, and then we clean up via the machine check - * handler anyway), vcdev->status was not overwritten and we just + * handler anyway), vcdev->dma_area->status was not overwritten and we just * return the old status, which is fine. */ vc_dma_free_struct(vdev, ccw); - return *vcdev->status; + return vcdev->dma_area->status; } static void virtio_ccw_set_status(struct virtio_device *vdev, u8 status) { struct virtio_ccw_device *vcdev = to_vc_device(vdev); - u8 old_status = *vcdev->status; + u8 old_status = vcdev->dma_area->status; vc_dma_decl_struct(ccw1, ccw); int ret; @@ -1005,15 +1003,15 @@ static void virtio_ccw_set_status(struct virtio_device *vdev, u8 status) return; /* Write the status to the host. */ - *vcdev->status = status; + vcdev->dma_area->status = status; ccw->cmd_code = CCW_CMD_WRITE_STATUS; ccw->flags = 0; ccw->count = sizeof(status); - ccw->cda = (__u32)(unsigned long)vcdev->status; + ccw->cda = (__u32)(unsigned long)&vcdev->dma_area->status; ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_STATUS); /* Write failed? We assume status is unchanged. */ if (ret) - *vcdev->status = old_status; + vcdev->dma_area->status = old_status; vc_dma_free_struct(vdev, ccw); } @@ -1047,8 +1045,6 @@ static void virtio_ccw_release_dev(struct device *_d) struct virtio_device *dev = dev_to_virtio(_d); struct virtio_ccw_device *vcdev = to_vc_device(dev); - vc_dma_free_struct(dev, vcdev->status); - vc_dma_free_struct(dev, vcdev->config_block); __vc_dma_free(&vcdev->vdev, PAGE_SIZE, vcdev->dma_area, vcdev->dma_area_dma_addr); kfree(vcdev); @@ -1322,16 +1318,6 @@ static int virtio_ccw_online(struct ccw_device *cdev) goto out_free; } - vc_dma_alloc_struct(&vcdev->vdev, vcdev->config_block); - if (!vcdev->config_block) { - ret = -ENOMEM; - goto out_free; - } - vc_dma_alloc_struct(&vcdev->vdev, vcdev->status); - if (!vcdev->status) { - ret = -ENOMEM; - goto out_free; - } vcdev->dma_area = __vc_dma_alloc(&vcdev->vdev, PAGE_SIZE, &vcdev->dma_area_dma_addr); if (!vcdev->dma_area) { @@ -1374,8 +1360,6 @@ static int virtio_ccw_online(struct ccw_device *cdev) return ret; out_free: if (vcdev) { - vc_dma_free_struct(&vcdev->vdev, vcdev->status); - vc_dma_free_struct(&vcdev->vdev, vcdev->config_block); __vc_dma_free(&vcdev->vdev, PAGE_SIZE, vcdev->dma_area, vcdev->dma_area_dma_addr); }
We can reduce the number of DMA allocations by pulling the bits of memory that belongs to virtio_ccw_device and needs to be DMA memory into a single area. Signed-off-by: Halil Pasic <pasic@linux.ibm.com> --- drivers/s390/virtio/virtio_ccw.c | 48 ++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 32 deletions(-)