search for: vhost_init_is_l

Currently, under certain circumstances vhost_init_is_le does just a part of the initialization job, and depends on vhost_reset_is_le being called too. For this reason vhost_vq_init_access used to call vhost_reset_is_le when vq->private_data is NULL. This is not only counter intuitive, but also real a problem because it breaks vhost_net. The bug was...

Currently, under certain circumstances vhost_init_is_le does just a part of the initialization job, and depends on vhost_reset_is_le being called too. For this reason vhost_vq_init_access used to call vhost_reset_is_le when vq->private_data is NULL. This is not only counter intuitive, but also real a problem because it breaks vhost_net. The bug was...

...e *vq) >> int r; >> bool is_le = vq->is_le; >> >> - if (!vq->private_data) { >> - vhost_reset_is_le(vq); >> + if (!vq->private_data) >> return 0; >> - } >> >> vhost_init_is_le(vq); > > > I think you do need to reset it, just maybe within vhost_init_is_le. > > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) > vq->is_le = true; > else > vhost_reset_is_le(vq); > > That is a very good point! I have overlooked...

...e *vq) >> int r; >> bool is_le = vq->is_le; >> >> - if (!vq->private_data) { >> - vhost_reset_is_le(vq); >> + if (!vq->private_data) >> return 0; >> - } >> >> vhost_init_is_le(vq); > > > I think you do need to reset it, just maybe within vhost_init_is_le. > > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) > vq->is_le = true; > else > vhost_reset_is_le(vq); > > That is a very good point! I have overlooked...

...> >> > > >> - if (!vq->private_data) { > > >> - vhost_reset_is_le(vq); > > >> + if (!vq->private_data) > > >> return 0; > > >> - } > > >> > > >> vhost_init_is_le(vq); > > > > > > > > > I think you do need to reset it, just maybe within vhost_init_is_le. > > > > > > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) > > > vq->is_le = true; > > > else > > >...

...> >> > > >> - if (!vq->private_data) { > > >> - vhost_reset_is_le(vq); > > >> + if (!vq->private_data) > > >> return 0; > > >> - } > > >> > > >> vhost_init_is_le(vq); > > > > > > > > > I think you do need to reset it, just maybe within vhost_init_is_le. > > > > > > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) > > > vq->is_le = true; > > > else > > >...

...ess than the host Both cases rely on the VHOST_SET_FEATURES ioctl, but 2) also needs the VHOST_SET_VRING_ENDIAN ioctl to be called by userspace. Since vq->is_le is only needed when the backend is active, it was decided to set it at backend start. This is currently done in vhost_init_used()->vhost_init_is_le() but it obfuscates the core vhost code. This patch moves the is_le setup to a dedicated function that is called from the backend code. Note vhost_net is the only backend that can pass vq->private_data == NULL to vhost_init_used(), hence the "if (sock)" branch. No behaviour change....

...ess than the host Both cases rely on the VHOST_SET_FEATURES ioctl, but 2) also needs the VHOST_SET_VRING_ENDIAN ioctl to be called by userspace. Since vq->is_le is only needed when the backend is active, it was decided to set it at backend start. This is currently done in vhost_init_used()->vhost_init_is_le() but it obfuscates the core vhost code. This patch moves the is_le setup to a dedicated function that is called from the backend code. Note vhost_net is the only backend that can pass vq->private_data == NULL to vhost_init_used(), hence the "if (sock)" branch. No behaviour change....

...T_FEATURES ioctl, but 2) also needs the > > VHOST_SET_VRING_ENDIAN ioctl to be called by userspace. Since vq->is_le > > is only needed when the backend is active, it was decided to set it at > > backend start. > > > > This is currently done in vhost_init_used()->vhost_init_is_le() but it > > obfuscates the core vhost code. This patch moves the is_le setup to a > > dedicated function that is called from the backend code. > > > > Note vhost_net is the only backend that can pass vq->private_data == NULL to > > vhost_init_used(), hence the &q...

...T_FEATURES ioctl, but 2) also needs the > > VHOST_SET_VRING_ENDIAN ioctl to be called by userspace. Since vq->is_le > > is only needed when the backend is active, it was decided to set it at > > backend start. > > > > This is currently done in vhost_init_used()->vhost_init_is_le() but it > > obfuscates the core vhost code. This patch moves the is_le setup to a > > dedicated function that is called from the backend code. > > > > Note vhost_net is the only backend that can pass vq->private_data == NULL to > > vhost_init_used(), hence the &q...

...+ return -EFAULT; > + > + if (s.num && s.num != 1) Checking for s.num > 1 might be more obvious at a glance? > + return -EINVAL; > + > + vq->user_be = s.num; > + > + return 0; > +} > + (...) > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY > +static void vhost_init_is_le(struct vhost_virtqueue *vq) > +{ > + vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be; So if the endianness is not explicitly set to BE, it will be forced to LE (instead of native endian)? Won't that break userspace that does not yet use the new interface when...

...+ return -EFAULT; > + > + if (s.num && s.num != 1) Checking for s.num > 1 might be more obvious at a glance? > + return -EINVAL; > + > + vq->user_be = s.num; > + > + return 0; > +} > + (...) > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY > +static void vhost_init_is_le(struct vhost_virtqueue *vq) > +{ > + vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be; So if the endianness is not explicitly set to BE, it will be forced to LE (instead of native endian)? Won't that break userspace that does not yet use the new interface when...

...ess than the host Both cases rely on the VHOST_SET_FEATURES ioctl, but 2) also needs the VHOST_SET_VRING_ENDIAN ioctl to be called by userspace. Since vq->is_le is only needed when the backend is active, it was decided to set it at backend start. This is currently done in vhost_init_used()->vhost_init_is_le() but it obfuscates the core vhost code. This patch moves the is_le setup to a dedicated function that is called from the backend code. Note vhost_net is the only backend that can pass vq->private_data == NULL to vhost_init_used(), hence the "if (sock)" branch. No behaviour change....

...ess than the host Both cases rely on the VHOST_SET_FEATURES ioctl, but 2) also needs the VHOST_SET_VRING_ENDIAN ioctl to be called by userspace. Since vq->is_le is only needed when the backend is active, it was decided to set it at backend start. This is currently done in vhost_init_used()->vhost_init_is_le() but it obfuscates the core vhost code. This patch moves the is_le setup to a dedicated function that is called from the backend code. Note vhost_net is the only backend that can pass vq->private_data == NULL to vhost_init_used(), hence the "if (sock)" branch. No behaviour change....

...bool is_le = vq->is_le; > >> > >> - if (!vq->private_data) { > >> - vhost_reset_is_le(vq); > >> + if (!vq->private_data) > >> return 0; > >> - } > >> > >> vhost_init_is_le(vq); > > > > > > I think you do need to reset it, just maybe within vhost_init_is_le. > > > > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) > > vq->is_le = true; > > else > > vhost_reset_is_le(vq); > > >...

On 01/30/2017 08:06 PM, Greg Kurz wrote: >> Currently, under certain circumstances vhost_init_is_le does just a part >> of the initialization job, and depends on vhost_reset_is_le being called >> too. For this reason vhost_vq_init_access used to call vhost_reset_is_le >> when vq->private_data is NULL. This is not only counter intuitive, but >> also real a problem becau...

On 01/30/2017 08:06 PM, Greg Kurz wrote: >> Currently, under certain circumstances vhost_init_is_le does just a part >> of the initialization job, and depends on vhost_reset_is_le being called >> too. For this reason vhost_vq_init_access used to call vhost_reset_is_le >> when vq->private_data is NULL. This is not only counter intuitive, but >> also real a problem becau...

...init_access(struct vhost_virtqueue *vq) { __virtio16 last_used_idx; int r; bool is_le = vq->is_le; if (!vq->private_data) { ==> vhost_reset_is_le(vq); /* resets to native endianness and returns */ return 0; } ==> vhost_init_is_le(vq); /* here we init is_le */ r = vhost_update_used_flags(vq); if (r) goto err; vq->signalled_used_valid = false; if (!vq->iotlb && !access_ok(VERIFY_READ, &vq->used->idx, sizeof vq->used->idx)) {...

...init_access(struct vhost_virtqueue *vq) { __virtio16 last_used_idx; int r; bool is_le = vq->is_le; if (!vq->private_data) { ==> vhost_reset_is_le(vq); /* resets to native endianness and returns */ return 0; } ==> vhost_init_is_le(vq); /* here we init is_le */ r = vhost_update_used_flags(vq); if (r) goto err; vq->signalled_used_valid = false; if (!vq->iotlb && !access_ok(VERIFY_READ, &vq->used->idx, sizeof vq->used->idx)) {...

...ases rely on the VHOST_SET_FEATURES ioctl, but 2) also needs the > VHOST_SET_VRING_ENDIAN ioctl to be called by userspace. Since vq->is_le > is only needed when the backend is active, it was decided to set it at > backend start. > > This is currently done in vhost_init_used()->vhost_init_is_le() but it > obfuscates the core vhost code. This patch moves the is_le setup to a > dedicated function that is called from the backend code. > > Note vhost_net is the only backend that can pass vq->private_data == NULL to > vhost_init_used(), hence the "if (sock)" branc...