| Message ID | 20190523162209.9543-7-mimu@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390: virtio: support protected virtualization | expand |
On Thu, 23 May 2019 18:22:07 +0200 Michael Mueller <mimu@linux.ibm.com> wrote: > From: Halil Pasic <pasic@linux.ibm.com> > > This will come in handy soon when we pull out the indicators from > virtio_ccw_device to a memory area that is shared with the hypervisor > (in particular for protected virtualization guests). > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> > --- > drivers/s390/virtio/virtio_ccw.c | 40 +++++++++++++++++++++++++--------------- > 1 file changed, 25 insertions(+), 15 deletions(-) > > @@ -338,17 +348,17 @@ 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), > + indicatorp = kmalloc(sizeof(indicators(vcdev)), > GFP_DMA | GFP_KERNEL); > if (!indicatorp) > return; > *indicatorp = 0; > ccw->cmd_code = CCW_CMD_SET_IND; > - ccw->count = sizeof(&vcdev->indicators); > + ccw->count = sizeof(indicators(vcdev)); > ccw->cda = (__u32)(unsigned long) indicatorp; > } > /* Deregister indicators from host. */ > - vcdev->indicators = 0; > + *indicators(vcdev) = 0; I'm not too hot about this notation, but it's not wrong and a minor thing :) > ccw->flags = 0; > ret = ccw_io_helper(vcdev, ccw, > vcdev->is_thinint ? Patch looks reasonable and not dependent on the other patches here.
On Mon, 27 May 2019 13:00:28 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Thu, 23 May 2019 18:22:07 +0200 > Michael Mueller <mimu@linux.ibm.com> wrote: > > > From: Halil Pasic <pasic@linux.ibm.com> > > > > This will come in handy soon when we pull out the indicators from > > virtio_ccw_device to a memory area that is shared with the hypervisor > > (in particular for protected virtualization guests). > > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > > Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> > > --- > > drivers/s390/virtio/virtio_ccw.c | 40 +++++++++++++++++++++++++--------------- > > 1 file changed, 25 insertions(+), 15 deletions(-) > > > > > @@ -338,17 +348,17 @@ 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), > > + indicatorp = kmalloc(sizeof(indicators(vcdev)), > > GFP_DMA | GFP_KERNEL); > > if (!indicatorp) > > return; > > *indicatorp = 0; > > ccw->cmd_code = CCW_CMD_SET_IND; > > - ccw->count = sizeof(&vcdev->indicators); > > + ccw->count = sizeof(indicators(vcdev)); > > ccw->cda = (__u32)(unsigned long) indicatorp; > > } > > /* Deregister indicators from host. */ > > - vcdev->indicators = 0; > > + *indicators(vcdev) = 0; > > I'm not too hot about this notation, but it's not wrong and a minor > thing :) I don't have any better ideas :/ > > > ccw->flags = 0; > > ret = ccw_io_helper(vcdev, ccw, > > vcdev->is_thinint ? > > Patch looks reasonable and not dependent on the other patches here. > looks reasonable == r-b? Not dependent in a sense that this patch could be made a first patch in the series. A subsequent patch depends on it. Regards, Halil
On Mon, 27 May 2019 13:57:06 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > On Mon, 27 May 2019 13:00:28 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > > > On Thu, 23 May 2019 18:22:07 +0200 > > Michael Mueller <mimu@linux.ibm.com> wrote: > > > > > From: Halil Pasic <pasic@linux.ibm.com> > > > > > > This will come in handy soon when we pull out the indicators from > > > virtio_ccw_device to a memory area that is shared with the hypervisor > > > (in particular for protected virtualization guests). > > > > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > > > Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> > > > --- > > > drivers/s390/virtio/virtio_ccw.c | 40 +++++++++++++++++++++++++--------------- > > > 1 file changed, 25 insertions(+), 15 deletions(-) > > > > > > > > @@ -338,17 +348,17 @@ 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), > > > + indicatorp = kmalloc(sizeof(indicators(vcdev)), > > > GFP_DMA | GFP_KERNEL); > > > if (!indicatorp) > > > return; > > > *indicatorp = 0; > > > ccw->cmd_code = CCW_CMD_SET_IND; > > > - ccw->count = sizeof(&vcdev->indicators); > > > + ccw->count = sizeof(indicators(vcdev)); > > > ccw->cda = (__u32)(unsigned long) indicatorp; > > > } > > > /* Deregister indicators from host. */ > > > - vcdev->indicators = 0; > > > + *indicators(vcdev) = 0; > > > > I'm not too hot about this notation, but it's not wrong and a minor > > thing :) > > I don't have any better ideas :/ > > > > > > ccw->flags = 0; > > > ret = ccw_io_helper(vcdev, ccw, > > > vcdev->is_thinint ? > > > > Patch looks reasonable and not dependent on the other patches here. > > > > looks reasonable == r-b? > > Not dependent in a sense that this patch could be made a first patch in > the series. A subsequent patch depends on it. What is the plan with these patches? I can either pick patch 5+6 and let them go through the virtio tree, or give my r-b and let them go through the s390 tree. The former is probably the quicker route, but the latter has less potential for dependency issues.
On 27.05.19 14:10, Cornelia Huck wrote: > On Mon, 27 May 2019 13:57:06 +0200 > Halil Pasic <pasic@linux.ibm.com> wrote: > >> On Mon, 27 May 2019 13:00:28 +0200 >> Cornelia Huck <cohuck@redhat.com> wrote: >> >>> On Thu, 23 May 2019 18:22:07 +0200 >>> Michael Mueller <mimu@linux.ibm.com> wrote: >>> >>>> From: Halil Pasic <pasic@linux.ibm.com> >>>> >>>> This will come in handy soon when we pull out the indicators from >>>> virtio_ccw_device to a memory area that is shared with the hypervisor >>>> (in particular for protected virtualization guests). >>>> >>>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com> >>>> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> >>>> --- >>>> drivers/s390/virtio/virtio_ccw.c | 40 +++++++++++++++++++++++++--------------- >>>> 1 file changed, 25 insertions(+), 15 deletions(-) >>>> >>> >>>> @@ -338,17 +348,17 @@ 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), >>>> + indicatorp = kmalloc(sizeof(indicators(vcdev)), >>>> GFP_DMA | GFP_KERNEL); >>>> if (!indicatorp) >>>> return; >>>> *indicatorp = 0; >>>> ccw->cmd_code = CCW_CMD_SET_IND; >>>> - ccw->count = sizeof(&vcdev->indicators); >>>> + ccw->count = sizeof(indicators(vcdev)); >>>> ccw->cda = (__u32)(unsigned long) indicatorp; >>>> } >>>> /* Deregister indicators from host. */ >>>> - vcdev->indicators = 0; >>>> + *indicators(vcdev) = 0; >>> >>> I'm not too hot about this notation, but it's not wrong and a minor >>> thing :) >> >> I don't have any better ideas :/ >> >>> >>>> ccw->flags = 0; >>>> ret = ccw_io_helper(vcdev, ccw, >>>> vcdev->is_thinint ? >>> >>> Patch looks reasonable and not dependent on the other patches here. >>> >> >> looks reasonable == r-b? >> >> Not dependent in a sense that this patch could be made a first patch in >> the series. A subsequent patch depends on it. > > What is the plan with these patches? I can either pick patch 5+6 and > let them go through the virtio tree, or give my r-b and let them go > through the s390 tree. The former is probably the quicker route, but > the latter has less potential for dependency issues. please give your r-b then for these. > Michael
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c index 1da7430f94c8..e96a8cc56ec2 100644 --- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c @@ -68,6 +68,16 @@ struct virtio_ccw_device { void *airq_info; }; +static inline unsigned long *indicators(struct virtio_ccw_device *vcdev) +{ + return &vcdev->indicators; +} + +static inline unsigned long *indicators2(struct virtio_ccw_device *vcdev) +{ + return &vcdev->indicators2; +} + struct vq_info_block_legacy { __u64 queue; __u32 align; @@ -338,17 +348,17 @@ 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), + indicatorp = kmalloc(sizeof(indicators(vcdev)), GFP_DMA | GFP_KERNEL); if (!indicatorp) return; *indicatorp = 0; ccw->cmd_code = CCW_CMD_SET_IND; - ccw->count = sizeof(&vcdev->indicators); + ccw->count = sizeof(indicators(vcdev)); ccw->cda = (__u32)(unsigned long) indicatorp; } /* Deregister indicators from host. */ - vcdev->indicators = 0; + *indicators(vcdev) = 0; ccw->flags = 0; ret = ccw_io_helper(vcdev, ccw, vcdev->is_thinint ? @@ -657,10 +667,10 @@ static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs, * We need a data area under 2G to communicate. Our payload is * the address of the indicators. */ - indicatorp = kmalloc(sizeof(&vcdev->indicators), GFP_DMA | GFP_KERNEL); + indicatorp = kmalloc(sizeof(indicators(vcdev)), GFP_DMA | GFP_KERNEL); if (!indicatorp) goto out; - *indicatorp = (unsigned long) &vcdev->indicators; + *indicatorp = (unsigned long) indicators(vcdev); if (vcdev->is_thinint) { ret = virtio_ccw_register_adapter_ind(vcdev, vqs, nvqs, ccw); if (ret) @@ -669,21 +679,21 @@ static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs, } if (!vcdev->is_thinint) { /* Register queue indicators with host. */ - vcdev->indicators = 0; + *indicators(vcdev) = 0; ccw->cmd_code = CCW_CMD_SET_IND; ccw->flags = 0; - ccw->count = sizeof(&vcdev->indicators); + ccw->count = sizeof(indicators(vcdev)); ccw->cda = (__u32)(unsigned long) indicatorp; ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_SET_IND); if (ret) goto out; } /* Register indicators2 with host for config changes */ - *indicatorp = (unsigned long) &vcdev->indicators2; - vcdev->indicators2 = 0; + *indicatorp = (unsigned long) indicators2(vcdev); + *indicators2(vcdev) = 0; ccw->cmd_code = CCW_CMD_SET_CONF_IND; ccw->flags = 0; - ccw->count = sizeof(&vcdev->indicators2); + ccw->count = sizeof(indicators2(vcdev)); ccw->cda = (__u32)(unsigned long) indicatorp; ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_SET_CONF_IND); if (ret) @@ -1093,17 +1103,17 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev, vcdev->err = -EIO; } virtio_ccw_check_activity(vcdev, activity); - for_each_set_bit(i, &vcdev->indicators, - sizeof(vcdev->indicators) * BITS_PER_BYTE) { + for_each_set_bit(i, indicators(vcdev), + sizeof(*indicators(vcdev)) * BITS_PER_BYTE) { /* The bit clear must happen before the vring kick. */ - clear_bit(i, &vcdev->indicators); + clear_bit(i, indicators(vcdev)); barrier(); vq = virtio_ccw_vq_by_ind(vcdev, i); vring_interrupt(0, vq); } - if (test_bit(0, &vcdev->indicators2)) { + if (test_bit(0, indicators2(vcdev))) { virtio_config_changed(&vcdev->vdev); - clear_bit(0, &vcdev->indicators2); + clear_bit(0, indicators2(vcdev)); } }