| Message ID | 20240611214716.1002781-1-pasic@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] s390/virtio_ccw: fix config change notifications | expand |
On 11/06/2024 23.47, Halil Pasic wrote: > Commit e3e9bda38e6d ("s390/virtio_ccw: use DMA handle from DMA API") > broke configuration change notifications for virtio-ccw by putting the > DMA address of *indicatorp directly into ccw->cda disregarding the fact > that if !!(vcdev->is_thinint) then the function > virtio_ccw_register_adapter_ind() will overwrite that ccw->cda value > with the address of the virtio_thinint_area so it can actually set up > the adapter interrupts via CCW_CMD_SET_IND_ADAPTER. Thus we end up > pointing to the wrong object for both CCW_CMD_SET_IND if setting up the > adapter interrupts fails, and for CCW_CMD_SET_CONF_IND regardless > whether it succeeds or fails. > > To fix this, let us save away the dma address of *indicatorp in a local > variable, and copy it to ccw->cda after the "vcdev->is_thinint" branch. > > Reported-by: Boqiao Fu <bfu@redhat.com> > Reported-by: Sebastian Mitterle <smitterl@redhat.com> > Fixes: e3e9bda38e6d ("s390/virtio_ccw: use DMA handle from DMA API") > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > --- > I know that checkpatch.pl complains about a missing 'Closes' tag. > Unfortunately I don't have an appropriate URL at hand. @Sebastian, > @Boqiao: do you have any suggetions? Closes: https://issues.redhat.com/browse/RHEL-39983 ? Anyway, I've tested the patch and it indeed fixes the problem with virtio-balloon and the link state for me: Tested-by: Thomas Huth <thuth@redhat.com>
On Wed, 12 Jun 2024 16:04:15 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 11/06/2024 23.47, Halil Pasic wrote: > > Commit e3e9bda38e6d ("s390/virtio_ccw: use DMA handle from DMA API") > > broke configuration change notifications for virtio-ccw by putting the > > DMA address of *indicatorp directly into ccw->cda disregarding the fact > > that if !!(vcdev->is_thinint) then the function > > virtio_ccw_register_adapter_ind() will overwrite that ccw->cda value > > with the address of the virtio_thinint_area so it can actually set up > > the adapter interrupts via CCW_CMD_SET_IND_ADAPTER. Thus we end up > > pointing to the wrong object for both CCW_CMD_SET_IND if setting up the > > adapter interrupts fails, and for CCW_CMD_SET_CONF_IND regardless > > whether it succeeds or fails. > > > > To fix this, let us save away the dma address of *indicatorp in a local > > variable, and copy it to ccw->cda after the "vcdev->is_thinint" branch. > > > > Reported-by: Boqiao Fu <bfu@redhat.com> > > Reported-by: Sebastian Mitterle <smitterl@redhat.com> > > Fixes: e3e9bda38e6d ("s390/virtio_ccw: use DMA handle from DMA API") > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > > --- > > I know that checkpatch.pl complains about a missing 'Closes' tag. > > Unfortunately I don't have an appropriate URL at hand. @Sebastian, > > @Boqiao: do you have any suggetions? > > Closes: https://issues.redhat.com/browse/RHEL-39983 > ? Yep! That is a public bug tracker bug. Qualifies! @Vasily: Can you guys pick hat one up when picking the patch? > > Anyway, I've tested the patch and it indeed fixes the problem with > virtio-balloon and the link state for me: > > Tested-by: Thomas Huth <thuth@redhat.com> > Thanks!
On Tue, 2024-06-11 at 23:47 +0200, Halil Pasic wrote: > Commit e3e9bda38e6d ("s390/virtio_ccw: use DMA handle from DMA API") > broke configuration change notifications for virtio-ccw by putting > the > DMA address of *indicatorp directly into ccw->cda disregarding the > fact > that if !!(vcdev->is_thinint) then the function > virtio_ccw_register_adapter_ind() will overwrite that ccw->cda value > with the address of the virtio_thinint_area so it can actually set up > the adapter interrupts via CCW_CMD_SET_IND_ADAPTER. Thus we end up > pointing to the wrong object for both CCW_CMD_SET_IND if setting up > the > adapter interrupts fails, and for CCW_CMD_SET_CONF_IND regardless > whether it succeeds or fails. > > To fix this, let us save away the dma address of *indicatorp in a > local > variable, and copy it to ccw->cda after the "vcdev->is_thinint" > branch. > > Reported-by: Boqiao Fu <bfu@redhat.com> > Reported-by: Sebastian Mitterle <smitterl@redhat.com> > Fixes: e3e9bda38e6d ("s390/virtio_ccw: use DMA handle from DMA API") > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > --- > I know that checkpatch.pl complains about a missing 'Closes' tag. > Unfortunately I don't have an appropriate URL at hand. @Sebastian, > @Boqiao: do you have any suggetions? > --- > drivers/s390/virtio/virtio_ccw.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Reviewed-by: Eric Farman <farman@linux.ibm.com>
On Thu, Jun 13, 2024 at 03:21:15PM +0200, Halil Pasic wrote: > On Wed, 12 Jun 2024 16:04:15 +0200 > Thomas Huth <thuth@redhat.com> wrote: > > > On 11/06/2024 23.47, Halil Pasic wrote: > > > Commit e3e9bda38e6d ("s390/virtio_ccw: use DMA handle from DMA API") > > > broke configuration change notifications for virtio-ccw by putting the > > > DMA address of *indicatorp directly into ccw->cda disregarding the fact > > > that if !!(vcdev->is_thinint) then the function > > > virtio_ccw_register_adapter_ind() will overwrite that ccw->cda value > > > with the address of the virtio_thinint_area so it can actually set up > > > the adapter interrupts via CCW_CMD_SET_IND_ADAPTER. Thus we end up > > > pointing to the wrong object for both CCW_CMD_SET_IND if setting up the > > > adapter interrupts fails, and for CCW_CMD_SET_CONF_IND regardless > > > whether it succeeds or fails. > > > > > > To fix this, let us save away the dma address of *indicatorp in a local > > > variable, and copy it to ccw->cda after the "vcdev->is_thinint" branch. > > > > > > Reported-by: Boqiao Fu <bfu@redhat.com> > > > Reported-by: Sebastian Mitterle <smitterl@redhat.com> > > > Fixes: e3e9bda38e6d ("s390/virtio_ccw: use DMA handle from DMA API") > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > > > --- > > > I know that checkpatch.pl complains about a missing 'Closes' tag. > > > Unfortunately I don't have an appropriate URL at hand. @Sebastian, > > > @Boqiao: do you have any suggetions? > > > > Closes: https://issues.redhat.com/browse/RHEL-39983 > > ? > > Yep! That is a public bug tracker bug. Qualifies! > @Vasily: Can you guys pick hat one up when picking the patch? Sure, applied. Thanks!
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c index d7569f395559..d6491fc84e8c 100644 --- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c @@ -698,6 +698,7 @@ static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs, dma64_t *indicatorp = NULL; int ret, i, queue_idx = 0; struct ccw1 *ccw; + dma32_t indicatorp_dma = 0; ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw), NULL); if (!ccw) @@ -725,7 +726,7 @@ static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs, */ indicatorp = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*indicatorp), - &ccw->cda); + &indicatorp_dma); if (!indicatorp) goto out; *indicatorp = indicators_dma(vcdev); @@ -735,6 +736,7 @@ static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs, /* no error, just fall back to legacy interrupts */ vcdev->is_thinint = false; } + ccw->cda = indicatorp_dma; if (!vcdev->is_thinint) { /* Register queue indicators with host. */ *indicators(vcdev) = 0;
Commit e3e9bda38e6d ("s390/virtio_ccw: use DMA handle from DMA API") broke configuration change notifications for virtio-ccw by putting the DMA address of *indicatorp directly into ccw->cda disregarding the fact that if !!(vcdev->is_thinint) then the function virtio_ccw_register_adapter_ind() will overwrite that ccw->cda value with the address of the virtio_thinint_area so it can actually set up the adapter interrupts via CCW_CMD_SET_IND_ADAPTER. Thus we end up pointing to the wrong object for both CCW_CMD_SET_IND if setting up the adapter interrupts fails, and for CCW_CMD_SET_CONF_IND regardless whether it succeeds or fails. To fix this, let us save away the dma address of *indicatorp in a local variable, and copy it to ccw->cda after the "vcdev->is_thinint" branch. Reported-by: Boqiao Fu <bfu@redhat.com> Reported-by: Sebastian Mitterle <smitterl@redhat.com> Fixes: e3e9bda38e6d ("s390/virtio_ccw: use DMA handle from DMA API") Signed-off-by: Halil Pasic <pasic@linux.ibm.com> --- I know that checkpatch.pl complains about a missing 'Closes' tag. Unfortunately I don't have an appropriate URL at hand. @Sebastian, @Boqiao: do you have any suggetions? --- drivers/s390/virtio/virtio_ccw.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) base-commit: 83a7eefedc9b56fe7bfeff13b6c7356688ffa670