| Message ID | 1484574276-15116-1-git-send-email-lvivier@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
On 16.01.2017 14:44, Laurent Vivier wrote: > virtioscsi_init() uses the result of virtio_get_vring_avail() whereas > the queue is not enabled: on the first boot, its value is NULL and > the driver uses this to communicate with the device. After a reboot, > its value is the one given by the OS driver, and it works if the > address is in a range SLOF can access. > > In some cases, for instance with NUMA nodes and hotplugged memory, > SLOF cannot access the address set by the kernel, and virtioscsi_init() > fails with a data storage exception. > > To set the vring avail buffer address, we need to enable the queue, what > is done by virtio_set_qaddr(). > > This patch fixes the problem by calling virtio_queue_init_vq() (like other > virtio drivers) in virtioscsi_init() as it allocates memory and enables the > queue. virtio_queue_init_vq() also replaces the calls to virtio_vring_size() > and virtio_get_vring_avail(). Good catch, that makes sense! ... I've just got some cosmetic remarks below... > diff --git a/lib/libvirtio/virtio-scsi.c b/lib/libvirtio/virtio-scsi.c > index 36b62d1..4de5b39 100644 > --- a/lib/libvirtio/virtio-scsi.c > +++ b/lib/libvirtio/virtio-scsi.c > @@ -18,6 +18,10 @@ > #include "virtio-internal.h" > #include "virtio-scsi.h" > > +static struct vqs vq_ctrl; > +static struct vqs vq_event; > +static struct vqs vq_request; I know, we've got such static global variables in the other drivers, too, but actually they are rather a coding style no-go (in case there are multiple virtio-scsi devices active at the same point in time, there could be a clash). Could you please move the variables into virtioscsi_init() instead, so that they are only local to that function? > int virtioscsi_send(struct virtio_device *dev, > struct virtio_scsi_req_cmd *req, > struct virtio_scsi_resp_cmd *resp, > @@ -99,9 +103,6 @@ int virtioscsi_send(struct virtio_device *dev, > */ > int virtioscsi_init(struct virtio_device *dev) > { > - struct vring_avail *vq_avail; > - unsigned int idx = 0; > - int qsize = 0; > int status = VIRTIO_STAT_ACKNOWLEDGE; > > /* Reset device */ > @@ -126,17 +127,22 @@ int virtioscsi_init(struct virtio_device *dev) > virtio_set_guest_features(dev, 0); > } > > - while(1) { > - qsize = virtio_get_qsize(dev, idx); > - if (!qsize) > - break; > - virtio_vring_size(qsize); > + if (virtio_queue_init_vq(dev, &vq_ctrl, VIRTIO_SCSI_CONTROL_VQ) || > + virtio_queue_init_vq(dev, &vq_event, VIRTIO_SCSI_EVENT_VQ) || > + virtio_queue_init_vq(dev, &vq_request, VIRTIO_SCSI_REQUEST_VQ)) > + goto dev_error; > > - vq_avail = virtio_get_vring_avail(dev, idx); > - vq_avail->flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); > - vq_avail->idx = 0; > - idx++; > - } > + vq_ctrl.avail->flags = virtio_cpu_to_modern16(dev, > + VRING_AVAIL_F_NO_INTERRUPT); SLOF uses the kernel coding style ... so the 80 columns are recommended, but in cases like this, I think it could also be OK to put everything in one line if that looks better (i.e. please give it a try it and use what you think looks betters). > + vq_ctrl.avail->idx = 0; > + > + vq_event.avail->flags = virtio_cpu_to_modern16(dev, > + VRING_AVAIL_F_NO_INTERRUPT); > + vq_event.avail->idx = 0; > + > + vq_request.avail->flags = virtio_cpu_to_modern16(dev, > + VRING_AVAIL_F_NO_INTERRUPT); > + vq_request.avail->idx = 0; > > /* Tell HV that setup succeeded */ > status |= VIRTIO_STAT_DRIVER_OK; > Thomas
Ping? Laurent On 16/01/2017 14:44, Laurent Vivier wrote: > virtioscsi_init() uses the result of virtio_get_vring_avail() whereas > the queue is not enabled: on the first boot, its value is NULL and > the driver uses this to communicate with the device. After a reboot, > its value is the one given by the OS driver, and it works if the > address is in a range SLOF can access. > > In some cases, for instance with NUMA nodes and hotplugged memory, > SLOF cannot access the address set by the kernel, and virtioscsi_init() > fails with a data storage exception. > > To set the vring avail buffer address, we need to enable the queue, what > is done by virtio_set_qaddr(). > > This patch fixes the problem by calling virtio_queue_init_vq() (like other > virtio drivers) in virtioscsi_init() as it allocates memory and enables the > queue. virtio_queue_init_vq() also replaces the calls to virtio_vring_size() > and virtio_get_vring_avail(). > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > lib/libvirtio/virtio-scsi.c | 32 +++++++++++++++++++------------- > 1 file changed, 19 insertions(+), 13 deletions(-) > > diff --git a/lib/libvirtio/virtio-scsi.c b/lib/libvirtio/virtio-scsi.c > index 36b62d1..4de5b39 100644 > --- a/lib/libvirtio/virtio-scsi.c > +++ b/lib/libvirtio/virtio-scsi.c > @@ -18,6 +18,10 @@ > #include "virtio-internal.h" > #include "virtio-scsi.h" > > +static struct vqs vq_ctrl; > +static struct vqs vq_event; > +static struct vqs vq_request; > + > int virtioscsi_send(struct virtio_device *dev, > struct virtio_scsi_req_cmd *req, > struct virtio_scsi_resp_cmd *resp, > @@ -99,9 +103,6 @@ int virtioscsi_send(struct virtio_device *dev, > */ > int virtioscsi_init(struct virtio_device *dev) > { > - struct vring_avail *vq_avail; > - unsigned int idx = 0; > - int qsize = 0; > int status = VIRTIO_STAT_ACKNOWLEDGE; > > /* Reset device */ > @@ -126,17 +127,22 @@ int virtioscsi_init(struct virtio_device *dev) > virtio_set_guest_features(dev, 0); > } > > - while(1) { > - qsize = virtio_get_qsize(dev, idx); > - if (!qsize) > - break; > - virtio_vring_size(qsize); > + if (virtio_queue_init_vq(dev, &vq_ctrl, VIRTIO_SCSI_CONTROL_VQ) || > + virtio_queue_init_vq(dev, &vq_event, VIRTIO_SCSI_EVENT_VQ) || > + virtio_queue_init_vq(dev, &vq_request, VIRTIO_SCSI_REQUEST_VQ)) > + goto dev_error; > > - vq_avail = virtio_get_vring_avail(dev, idx); > - vq_avail->flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); > - vq_avail->idx = 0; > - idx++; > - } > + vq_ctrl.avail->flags = virtio_cpu_to_modern16(dev, > + VRING_AVAIL_F_NO_INTERRUPT); > + vq_ctrl.avail->idx = 0; > + > + vq_event.avail->flags = virtio_cpu_to_modern16(dev, > + VRING_AVAIL_F_NO_INTERRUPT); > + vq_event.avail->idx = 0; > + > + vq_request.avail->flags = virtio_cpu_to_modern16(dev, > + VRING_AVAIL_F_NO_INTERRUPT); > + vq_request.avail->idx = 0; > > /* Tell HV that setup succeeded */ > status |= VIRTIO_STAT_DRIVER_OK; >
Laurent Vivier <lvivier@redhat.com> writes: > virtioscsi_init() uses the result of virtio_get_vring_avail() whereas > the queue is not enabled: on the first boot, its value is NULL and > the driver uses this to communicate with the device. After a reboot, > its value is the one given by the OS driver, and it works if the > address is in a range SLOF can access. Good find. > > In some cases, for instance with NUMA nodes and hotplugged memory, > SLOF cannot access the address set by the kernel, and virtioscsi_init() > fails with a data storage exception. > > To set the vring avail buffer address, we need to enable the queue, what > is done by virtio_set_qaddr(). That is done in setup-virt-queues, which will be late as virtioscsi_init() is already accessing vq_avail. > This patch fixes the problem by calling virtio_queue_init_vq() (like other > virtio drivers) in virtioscsi_init() as it allocates memory and enables the > queue. virtio_queue_init_vq() also replaces the calls to virtio_vring_size() > and virtio_get_vring_avail(). board-qemu/slof/virtio-scsi.fs : virtio-scsi-init-and-scan ( -- ) [...] virtiodev virtio-scsi-init 0= IF setup-virt-queues [...] ; We will need to get rid of this in slof code. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > lib/libvirtio/virtio-scsi.c | 32 +++++++++++++++++++------------- > 1 file changed, 19 insertions(+), 13 deletions(-) > > diff --git a/lib/libvirtio/virtio-scsi.c b/lib/libvirtio/virtio-scsi.c > index 36b62d1..4de5b39 100644 > --- a/lib/libvirtio/virtio-scsi.c > +++ b/lib/libvirtio/virtio-scsi.c > @@ -18,6 +18,10 @@ > #include "virtio-internal.h" > #include "virtio-scsi.h" > > +static struct vqs vq_ctrl; > +static struct vqs vq_event; > +static struct vqs vq_request; > + > int virtioscsi_send(struct virtio_device *dev, > struct virtio_scsi_req_cmd *req, > struct virtio_scsi_resp_cmd *resp, > @@ -99,9 +103,6 @@ int virtioscsi_send(struct virtio_device *dev, > */ > int virtioscsi_init(struct virtio_device *dev) > { > - struct vring_avail *vq_avail; > - unsigned int idx = 0; > - int qsize = 0; > int status = VIRTIO_STAT_ACKNOWLEDGE; > > /* Reset device */ > @@ -126,17 +127,22 @@ int virtioscsi_init(struct virtio_device *dev) > virtio_set_guest_features(dev, 0); > } > > - while(1) { > - qsize = virtio_get_qsize(dev, idx); > - if (!qsize) > - break; > - virtio_vring_size(qsize); > + if (virtio_queue_init_vq(dev, &vq_ctrl, VIRTIO_SCSI_CONTROL_VQ) || > + virtio_queue_init_vq(dev, &vq_event, VIRTIO_SCSI_EVENT_VQ) || > + virtio_queue_init_vq(dev, &vq_request, VIRTIO_SCSI_REQUEST_VQ)) > + goto dev_error; > > - vq_avail = virtio_get_vring_avail(dev, idx); > - vq_avail->flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); > - vq_avail->idx = 0; > - idx++; > - } > + vq_ctrl.avail->flags = virtio_cpu_to_modern16(dev, > + VRING_AVAIL_F_NO_INTERRUPT); > + vq_ctrl.avail->idx = 0; > + > + vq_event.avail->flags = virtio_cpu_to_modern16(dev, > + VRING_AVAIL_F_NO_INTERRUPT); > + vq_event.avail->idx = 0; > + > + vq_request.avail->flags = virtio_cpu_to_modern16(dev, > + VRING_AVAIL_F_NO_INTERRUPT); > + vq_request.avail->idx = 0; > > /* Tell HV that setup succeeded */ > status |= VIRTIO_STAT_DRIVER_OK; > -- > 2.7.4 > > _______________________________________________ > SLOF mailing list > SLOF@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/slof
diff --git a/lib/libvirtio/virtio-scsi.c b/lib/libvirtio/virtio-scsi.c index 36b62d1..4de5b39 100644 --- a/lib/libvirtio/virtio-scsi.c +++ b/lib/libvirtio/virtio-scsi.c @@ -18,6 +18,10 @@ #include "virtio-internal.h" #include "virtio-scsi.h" +static struct vqs vq_ctrl; +static struct vqs vq_event; +static struct vqs vq_request; + int virtioscsi_send(struct virtio_device *dev, struct virtio_scsi_req_cmd *req, struct virtio_scsi_resp_cmd *resp, @@ -99,9 +103,6 @@ int virtioscsi_send(struct virtio_device *dev, */ int virtioscsi_init(struct virtio_device *dev) { - struct vring_avail *vq_avail; - unsigned int idx = 0; - int qsize = 0; int status = VIRTIO_STAT_ACKNOWLEDGE; /* Reset device */ @@ -126,17 +127,22 @@ int virtioscsi_init(struct virtio_device *dev) virtio_set_guest_features(dev, 0); } - while(1) { - qsize = virtio_get_qsize(dev, idx); - if (!qsize) - break; - virtio_vring_size(qsize); + if (virtio_queue_init_vq(dev, &vq_ctrl, VIRTIO_SCSI_CONTROL_VQ) || + virtio_queue_init_vq(dev, &vq_event, VIRTIO_SCSI_EVENT_VQ) || + virtio_queue_init_vq(dev, &vq_request, VIRTIO_SCSI_REQUEST_VQ)) + goto dev_error; - vq_avail = virtio_get_vring_avail(dev, idx); - vq_avail->flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); - vq_avail->idx = 0; - idx++; - } + vq_ctrl.avail->flags = virtio_cpu_to_modern16(dev, + VRING_AVAIL_F_NO_INTERRUPT); + vq_ctrl.avail->idx = 0; + + vq_event.avail->flags = virtio_cpu_to_modern16(dev, + VRING_AVAIL_F_NO_INTERRUPT); + vq_event.avail->idx = 0; + + vq_request.avail->flags = virtio_cpu_to_modern16(dev, + VRING_AVAIL_F_NO_INTERRUPT); + vq_request.avail->idx = 0; /* Tell HV that setup succeeded */ status |= VIRTIO_STAT_DRIVER_OK;
virtioscsi_init() uses the result of virtio_get_vring_avail() whereas the queue is not enabled: on the first boot, its value is NULL and the driver uses this to communicate with the device. After a reboot, its value is the one given by the OS driver, and it works if the address is in a range SLOF can access. In some cases, for instance with NUMA nodes and hotplugged memory, SLOF cannot access the address set by the kernel, and virtioscsi_init() fails with a data storage exception. To set the vring avail buffer address, we need to enable the queue, what is done by virtio_set_qaddr(). This patch fixes the problem by calling virtio_queue_init_vq() (like other virtio drivers) in virtioscsi_init() as it allocates memory and enables the queue. virtio_queue_init_vq() also replaces the calls to virtio_vring_size() and virtio_get_vring_avail(). Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- lib/libvirtio/virtio-scsi.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)