docs/specs/vhost-user.txt | 118 ++++++++++++++++++++++++- hw/virtio/vhost-backend.c | 130 ++++++++++++++++------------ hw/virtio/vhost-user.c | 177 +++++++++++++++++++++++++++++++++++++- hw/virtio/vhost.c | 27 ++++-- include/hw/virtio/vhost-backend.h | 23 +++-- include/hw/virtio/vhost.h | 2 +- 6 files changed, 397 insertions(+), 80 deletions(-)
This series aims at specifying ans implementing the protocol update required to support device IOTLB with user backends. In this second non-RFC version, main changes are: - spec fixes and clarification - rings information update has been restored back to ring enablement time - Work around GCC 4.4.7 limitation wrt assignment in unnamed union at declaration time. The series can be tested with vhost_iotlb_proto_v2 branch on my gitlab account[0]. The slave requests channel part is re-used from Marc-André's series submitted last year[1], with main changes from original version being request/feature names renaming and addition of the REPLY_ACK feature support. Regarding IOTLB protocol, one noticeable change is the IOTLB miss request reply made optionnal (i.e. only if slave requests it by setting the VHOST_USER_NEED_REPLY flag in the message header). This change provides more flexibility in the backend implementation of the feature. The protocol is very close to kernel backends, except that a new communication channel is introduced to enable the slave to send requests to the master. [0]: https://gitlab.com/mcoquelin/dpdk-next-virtio/commits/vhost_iotlb_proto_v2 [1]: https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html Marc-André Lureau (2): vhost-user: add vhost_user to hold the chr vhost-user: add slave-req-fd support Maxime Coquelin (4): vhost: propagate errors in vhost_device_iotlb_miss() vhost: rework IOTLB messaging vhost: extend ring information update for IOTLB to all rings spec/vhost-user spec: Add IOMMU support docs/specs/vhost-user.txt | 118 ++++++++++++++++++++++++- hw/virtio/vhost-backend.c | 130 ++++++++++++++++------------ hw/virtio/vhost-user.c | 177 +++++++++++++++++++++++++++++++++++++- hw/virtio/vhost.c | 27 ++++-- include/hw/virtio/vhost-backend.h | 23 +++-- include/hw/virtio/vhost.h | 2 +- 6 files changed, 397 insertions(+), 80 deletions(-) -- 2.9.4
Hi,
On 05/26/2017 04:28 PM, Maxime Coquelin wrote:
> This series aims at specifying ans implementing the protocol update
> required to support device IOTLB with user backends.
>
> In this second non-RFC version, main changes are:
> - spec fixes and clarification
> - rings information update has been restored back to ring enablement time
> - Work around GCC 4.4.7 limitation wrt assignment in unnamed union at
> declaration time.
>
> The series can be tested with vhost_iotlb_proto_v2 branch on my gitlab
> account[0].
>
> The slave requests channel part is re-used from Marc-André's series submitted
> last year[1], with main changes from original version being request/feature
> names renaming and addition of the REPLY_ACK feature support.
>
> Regarding IOTLB protocol, one noticeable change is the IOTLB miss request
> reply made optionnal (i.e. only if slave requests it by setting the
> VHOST_USER_NEED_REPLY flag in the message header). This change provides
> more flexibility in the backend implementation of the feature.
>
> The protocol is very close to kernel backends, except that a new
> communication channel is introduced to enable the slave to send
> requests to the master.
>
> [0]: https://gitlab.com/mcoquelin/dpdk-next-virtio/commits/vhost_iotlb_proto_v2
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html
>
> Marc-André Lureau (2):
> vhost-user: add vhost_user to hold the chr
> vhost-user: add slave-req-fd support
>
> Maxime Coquelin (4):
> vhost: propagate errors in vhost_device_iotlb_miss()
> vhost: rework IOTLB messaging
> vhost: extend ring information update for IOTLB to all rings
> spec/vhost-user spec: Add IOMMU support
>
> docs/specs/vhost-user.txt | 118 ++++++++++++++++++++++++-
> hw/virtio/vhost-backend.c | 130 ++++++++++++++++------------
> hw/virtio/vhost-user.c | 177 +++++++++++++++++++++++++++++++++++++-
> hw/virtio/vhost.c | 27 ++++--
> include/hw/virtio/vhost-backend.h | 23 +++--
> include/hw/virtio/vhost.h | 2 +-
> 6 files changed, 397 insertions(+), 80 deletions(-)
>
I just noticed I missed the below change in the series.
I'll wait v2 is reviewed before posting the v3, or I can post v3 now if
you prefer.
Regards,
Maxime
------------------------------------------------------------------
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 22874a9..e037db6 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -77,6 +77,7 @@ static const int user_feature_bits[] = {
VIRTIO_NET_F_HOST_UFO,
VIRTIO_NET_F_MRG_RXBUF,
VIRTIO_NET_F_MTU,
+ VIRTIO_F_IOMMU_PLATFORM,
/* This bit implies RARP isn't sent by QEMU out of band */
VIRTIO_NET_F_GUEST_ANNOUNCE,
On Fri, May 26, 2017 at 04:28:52PM +0200, Maxime Coquelin wrote: > This series aims at specifying ans implementing the protocol update > required to support device IOTLB with user backends. > > In this second non-RFC version, main changes are: > - spec fixes and clarification > - rings information update has been restored back to ring enablement time > - Work around GCC 4.4.7 limitation wrt assignment in unnamed union at > declaration time. > > The series can be tested with vhost_iotlb_proto_v2 branch on my gitlab > account[0]. > > The slave requests channel part is re-used from Marc-André's series submitted > last year[1], with main changes from original version being request/feature > names renaming and addition of the REPLY_ACK feature support. > > Regarding IOTLB protocol, one noticeable change is the IOTLB miss request > reply made optionnal (i.e. only if slave requests it by setting the > VHOST_USER_NEED_REPLY flag in the message header). This change provides > more flexibility in the backend implementation of the feature. > > The protocol is very close to kernel backends, except that a new > communication channel is introduced to enable the slave to send > requests to the master. > > [0]: https://gitlab.com/mcoquelin/dpdk-next-virtio/commits/vhost_iotlb_proto_v2 > [1]: https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html Overall, this looks good to me. I do think patch 3 isn't a good idea though, if slave wants something let it request it. Need to find out why does vhost in kernel want the used ring iotlb at start time - especially considering we aren't even guaranteed one entry covers the whole ring, and invalidates should affect all addresses at least in theory. > Marc-André Lureau (2): > vhost-user: add vhost_user to hold the chr > vhost-user: add slave-req-fd support > > Maxime Coquelin (4): > vhost: propagate errors in vhost_device_iotlb_miss() > vhost: rework IOTLB messaging > vhost: extend ring information update for IOTLB to all rings > spec/vhost-user spec: Add IOMMU support > > docs/specs/vhost-user.txt | 118 ++++++++++++++++++++++++- > hw/virtio/vhost-backend.c | 130 ++++++++++++++++------------ > hw/virtio/vhost-user.c | 177 +++++++++++++++++++++++++++++++++++++- > hw/virtio/vhost.c | 27 ++++-- > include/hw/virtio/vhost-backend.h | 23 +++-- > include/hw/virtio/vhost.h | 2 +- > 6 files changed, 397 insertions(+), 80 deletions(-) > > -- > 2.9.4
On 2017年05月31日 02:20, Michael S. Tsirkin wrote: > On Fri, May 26, 2017 at 04:28:52PM +0200, Maxime Coquelin wrote: >> This series aims at specifying ans implementing the protocol update >> required to support device IOTLB with user backends. >> >> In this second non-RFC version, main changes are: >> - spec fixes and clarification >> - rings information update has been restored back to ring enablement time >> - Work around GCC 4.4.7 limitation wrt assignment in unnamed union at >> declaration time. >> >> The series can be tested with vhost_iotlb_proto_v2 branch on my gitlab >> account[0]. >> >> The slave requests channel part is re-used from Marc-André's series submitted >> last year[1], with main changes from original version being request/feature >> names renaming and addition of the REPLY_ACK feature support. >> >> Regarding IOTLB protocol, one noticeable change is the IOTLB miss request >> reply made optionnal (i.e. only if slave requests it by setting the >> VHOST_USER_NEED_REPLY flag in the message header). This change provides >> more flexibility in the backend implementation of the feature. >> >> The protocol is very close to kernel backends, except that a new >> communication channel is introduced to enable the slave to send >> requests to the master. >> >> [0]:https://gitlab.com/mcoquelin/dpdk-next-virtio/commits/vhost_iotlb_proto_v2 >> [1]:https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html > Overall, this looks good to me. I do think patch 3 isn't a good idea > though, if slave wants something let it request it. > > Need to find out why does vhost in kernel want the used ring iotlb at > start time - especially considering we aren't even guaranteed one entry > covers the whole ring, and invalidates should affect all addresses at > least in theory. > > The reason is probably we want to verify whether or not we could correctly access used ring in vhost_vq_init_access(). It was there since vhost_net is introduced. We can think to remove this limitation maybe. Thanks
Hi, On 05/31/2017 10:33 AM, Jason Wang wrote: > > > On 2017年05月31日 02:20, Michael S. Tsirkin wrote: >> On Fri, May 26, 2017 at 04:28:52PM +0200, Maxime Coquelin wrote: >>> This series aims at specifying ans implementing the protocol update >>> required to support device IOTLB with user backends. >>> >>> In this second non-RFC version, main changes are: >>> - spec fixes and clarification >>> - rings information update has been restored back to ring >>> enablement time >>> - Work around GCC 4.4.7 limitation wrt assignment in unnamed union at >>> declaration time. >>> >>> The series can be tested with vhost_iotlb_proto_v2 branch on my gitlab >>> account[0]. >>> >>> The slave requests channel part is re-used from Marc-André's series >>> submitted >>> last year[1], with main changes from original version being >>> request/feature >>> names renaming and addition of the REPLY_ACK feature support. >>> >>> Regarding IOTLB protocol, one noticeable change is the IOTLB miss >>> request >>> reply made optionnal (i.e. only if slave requests it by setting the >>> VHOST_USER_NEED_REPLY flag in the message header). This change provides >>> more flexibility in the backend implementation of the feature. >>> >>> The protocol is very close to kernel backends, except that a new >>> communication channel is introduced to enable the slave to send >>> requests to the master. >>> >>> [0]:https://gitlab.com/mcoquelin/dpdk-next-virtio/commits/vhost_iotlb_proto_v2 >>> >>> [1]:https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html >> Overall, this looks good to me. I do think patch 3 isn't a good idea >> though, if slave wants something let it request it. >> >> Need to find out why does vhost in kernel want the used ring iotlb at >> start time - especially considering we aren't even guaranteed one entry >> covers the whole ring, and invalidates should affect all addresses at >> least in theory. >> >> > > The reason is probably we want to verify whether or not we could > correctly access used ring in vhost_vq_init_access(). It was there since > vhost_net is introduced. We can think to remove this limitation maybe. Even if we remove the limitation on Kernel side, we will still have to keep this workaround for compatibility with older kernels. Having done the test, I can confirm it is currently necessary. Thanks, Maxime
On 2017年05月31日 23:32, Maxime Coquelin wrote: >>>> [0]:https://gitlab.com/mcoquelin/dpdk-next-virtio/commits/vhost_iotlb_proto_v2 >>>> >>>> [1]:https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html >>>> >>> Overall, this looks good to me. I do think patch 3 isn't a good idea >>> though, if slave wants something let it request it. >>> >>> Need to find out why does vhost in kernel want the used ring iotlb at >>> start time - especially considering we aren't even guaranteed one entry >>> covers the whole ring, and invalidates should affect all addresses at >>> least in theory. >>> >>> >> >> The reason is probably we want to verify whether or not we could >> correctly access used ring in vhost_vq_init_access(). It was there >> since vhost_net is introduced. We can think to remove this limitation >> maybe. > > Even if we remove the limitation on Kernel side, we will still have to > keep this workaround for compatibility with older kernels. Having done > the test, I can confirm it is currently necessary. > > Thanks, > Maxime Right, it was probably too late for the change. Thanks
On 06/01/2017 09:04 AM, Jason Wang wrote: > > > On 2017年05月31日 23:32, Maxime Coquelin wrote: >>>>> [0]:https://gitlab.com/mcoquelin/dpdk-next-virtio/commits/vhost_iotlb_proto_v2 >>>>> >>>>> [1]:https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html >>>>> >>>> Overall, this looks good to me. I do think patch 3 isn't a good idea >>>> though, if slave wants something let it request it. >>>> >>>> Need to find out why does vhost in kernel want the used ring iotlb at >>>> start time - especially considering we aren't even guaranteed one entry >>>> covers the whole ring, and invalidates should affect all addresses at >>>> least in theory. >>>> >>>> >>> >>> The reason is probably we want to verify whether or not we could >>> correctly access used ring in vhost_vq_init_access(). It was there >>> since vhost_net is introduced. We can think to remove this limitation >>> maybe. >> >> Even if we remove the limitation on Kernel side, we will still have to >> keep this workaround for compatibility with older kernels. Having done >> the test, I can confirm it is currently necessary. >> >> Thanks, >> Maxime > > Right, it was probably too late for the change. So, I wonder if we could keep patch 3, that just extends this workaround for vhost-user. Else, we would need to do the ring-related translations in the data path, which will be costly. Michael, what do you think? Thanks, Maxime
On Wed, May 31, 2017 at 04:33:33PM +0800, Jason Wang wrote:
>
>
> On 2017年05月31日 02:20, Michael S. Tsirkin wrote:
> > On Fri, May 26, 2017 at 04:28:52PM +0200, Maxime Coquelin wrote:
> > > This series aims at specifying ans implementing the protocol update
> > > required to support device IOTLB with user backends.
> > >
> > > In this second non-RFC version, main changes are:
> > > - spec fixes and clarification
> > > - rings information update has been restored back to ring enablement time
> > > - Work around GCC 4.4.7 limitation wrt assignment in unnamed union at
> > > declaration time.
> > >
> > > The series can be tested with vhost_iotlb_proto_v2 branch on my gitlab
> > > account[0].
> > >
> > > The slave requests channel part is re-used from Marc-André's series submitted
> > > last year[1], with main changes from original version being request/feature
> > > names renaming and addition of the REPLY_ACK feature support.
> > >
> > > Regarding IOTLB protocol, one noticeable change is the IOTLB miss request
> > > reply made optionnal (i.e. only if slave requests it by setting the
> > > VHOST_USER_NEED_REPLY flag in the message header). This change provides
> > > more flexibility in the backend implementation of the feature.
> > >
> > > The protocol is very close to kernel backends, except that a new
> > > communication channel is introduced to enable the slave to send
> > > requests to the master.
> > >
> > > [0]:https://gitlab.com/mcoquelin/dpdk-next-virtio/commits/vhost_iotlb_proto_v2
> > > [1]:https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html
> > Overall, this looks good to me. I do think patch 3 isn't a good idea
> > though, if slave wants something let it request it.
> >
> > Need to find out why does vhost in kernel want the used ring iotlb at
> > start time - especially considering we aren't even guaranteed one entry
> > covers the whole ring, and invalidates should affect all addresses at
> > least in theory.
> >
> >
>
> The reason is probably we want to verify whether or not we could correctly
> access used ring in vhost_vq_init_access(). It was there since vhost_net is
> introduced. We can think to remove this limitation maybe.
>
> Thanks
Well that's only called if iotlb is disabled:
if (!vq->iotlb &&
!access_ok(VERIFY_READ, &vq->used->idx, sizeof vq->used->idx)) {
r = -EFAULT;
goto err;
}
Could you try removing that and see what breaks?
--
MST
On 2017年06月01日 21:59, Michael S. Tsirkin wrote:
> On Wed, May 31, 2017 at 04:33:33PM +0800, Jason Wang wrote:
>>
>> On 2017年05月31日 02:20, Michael S. Tsirkin wrote:
>>> On Fri, May 26, 2017 at 04:28:52PM +0200, Maxime Coquelin wrote:
>>>> This series aims at specifying ans implementing the protocol update
>>>> required to support device IOTLB with user backends.
>>>>
>>>> In this second non-RFC version, main changes are:
>>>> - spec fixes and clarification
>>>> - rings information update has been restored back to ring enablement time
>>>> - Work around GCC 4.4.7 limitation wrt assignment in unnamed union at
>>>> declaration time.
>>>>
>>>> The series can be tested with vhost_iotlb_proto_v2 branch on my gitlab
>>>> account[0].
>>>>
>>>> The slave requests channel part is re-used from Marc-André's series submitted
>>>> last year[1], with main changes from original version being request/feature
>>>> names renaming and addition of the REPLY_ACK feature support.
>>>>
>>>> Regarding IOTLB protocol, one noticeable change is the IOTLB miss request
>>>> reply made optionnal (i.e. only if slave requests it by setting the
>>>> VHOST_USER_NEED_REPLY flag in the message header). This change provides
>>>> more flexibility in the backend implementation of the feature.
>>>>
>>>> The protocol is very close to kernel backends, except that a new
>>>> communication channel is introduced to enable the slave to send
>>>> requests to the master.
>>>>
>>>> [0]:https://gitlab.com/mcoquelin/dpdk-next-virtio/commits/vhost_iotlb_proto_v2
>>>> [1]:https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html
>>> Overall, this looks good to me. I do think patch 3 isn't a good idea
>>> though, if slave wants something let it request it.
>>>
>>> Need to find out why does vhost in kernel want the used ring iotlb at
>>> start time - especially considering we aren't even guaranteed one entry
>>> covers the whole ring, and invalidates should affect all addresses at
>>> least in theory.
>>>
>>>
>> The reason is probably we want to verify whether or not we could correctly
>> access used ring in vhost_vq_init_access(). It was there since vhost_net is
>> introduced. We can think to remove this limitation maybe.
>>
>> Thanks
>
> Well that's only called if iotlb is disabled:
>
> if (!vq->iotlb &&
> !access_ok(VERIFY_READ, &vq->used->idx, sizeof vq->used->idx)) {
> r = -EFAULT;
> goto err;
> }
>
> Could you try removing that and see what breaks?
>
Looks not, the issue is vhost_update_used_flags() which needs device
IOTLB translation. If we don't fill IOTLB in advance, it will return
-EFAULT.
For simplicity, I don't implement control path device IOTLB miss. If you
care about the incomplete length, we can refine vhost_iotlb_miss() to
make sure it covers all size.
Thanks
On Fri, Jun 02, 2017 at 01:53:11PM +0800, Jason Wang wrote:
>
>
> On 2017年06月01日 21:59, Michael S. Tsirkin wrote:
> > On Wed, May 31, 2017 at 04:33:33PM +0800, Jason Wang wrote:
> > >
> > > On 2017年05月31日 02:20, Michael S. Tsirkin wrote:
> > > > On Fri, May 26, 2017 at 04:28:52PM +0200, Maxime Coquelin wrote:
> > > > > This series aims at specifying ans implementing the protocol update
> > > > > required to support device IOTLB with user backends.
> > > > >
> > > > > In this second non-RFC version, main changes are:
> > > > > - spec fixes and clarification
> > > > > - rings information update has been restored back to ring enablement time
> > > > > - Work around GCC 4.4.7 limitation wrt assignment in unnamed union at
> > > > > declaration time.
> > > > >
> > > > > The series can be tested with vhost_iotlb_proto_v2 branch on my gitlab
> > > > > account[0].
> > > > >
> > > > > The slave requests channel part is re-used from Marc-André's series submitted
> > > > > last year[1], with main changes from original version being request/feature
> > > > > names renaming and addition of the REPLY_ACK feature support.
> > > > >
> > > > > Regarding IOTLB protocol, one noticeable change is the IOTLB miss request
> > > > > reply made optionnal (i.e. only if slave requests it by setting the
> > > > > VHOST_USER_NEED_REPLY flag in the message header). This change provides
> > > > > more flexibility in the backend implementation of the feature.
> > > > >
> > > > > The protocol is very close to kernel backends, except that a new
> > > > > communication channel is introduced to enable the slave to send
> > > > > requests to the master.
> > > > >
> > > > > [0]:https://gitlab.com/mcoquelin/dpdk-next-virtio/commits/vhost_iotlb_proto_v2
> > > > > [1]:https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html
> > > > Overall, this looks good to me. I do think patch 3 isn't a good idea
> > > > though, if slave wants something let it request it.
> > > >
> > > > Need to find out why does vhost in kernel want the used ring iotlb at
> > > > start time - especially considering we aren't even guaranteed one entry
> > > > covers the whole ring, and invalidates should affect all addresses at
> > > > least in theory.
> > > >
> > > >
> > > The reason is probably we want to verify whether or not we could correctly
> > > access used ring in vhost_vq_init_access(). It was there since vhost_net is
> > > introduced. We can think to remove this limitation maybe.
> > >
> > > Thanks
> >
> > Well that's only called if iotlb is disabled:
> >
> > if (!vq->iotlb &&
> > !access_ok(VERIFY_READ, &vq->used->idx, sizeof vq->used->idx)) {
> > r = -EFAULT;
> > goto err;
> > }
> >
> > Could you try removing that and see what breaks?
> >
>
> Looks not, the issue is vhost_update_used_flags() which needs device IOTLB
> translation. If we don't fill IOTLB in advance, it will return -EFAULT.
Same for vhost_get_used, right?
>
> For simplicity, I don't implement control path device IOTLB miss.
OK so this should be documented in vhost.h. SET_BACKEND immediately
writes and reads used ring. User must know this and pre-fault used flags
and index before setting backend.
> If you
> care about the incomplete length, we can refine vhost_iotlb_miss() to make
> sure it covers all size.
>
> Thanks
No need imho, it's only the used flags field that's written, and the
index that's read right? BTW I don't really know why do we do the write
when event index is setup. We probably shouldn't, should we?
It's worth considering whether we want this write into used ring at all.
I put it there originally to help make sure we don't miss the first exit, but
event index seems to get by fine without this. So why does non event
index code want it?
I think that if we could get rid of both accesses, it would be
nice. Would need a feature bit naturally and we'd need to
support old kernels but at least it will be contained and
well documented.
--
MST
On 2017年06月02日 23:24, Michael S. Tsirkin wrote:
> On Fri, Jun 02, 2017 at 01:53:11PM +0800, Jason Wang wrote:
>>
>> On 2017年06月01日 21:59, Michael S. Tsirkin wrote:
>>> On Wed, May 31, 2017 at 04:33:33PM +0800, Jason Wang wrote:
>>>> On 2017年05月31日 02:20, Michael S. Tsirkin wrote:
>>>>> On Fri, May 26, 2017 at 04:28:52PM +0200, Maxime Coquelin wrote:
>>>>>> This series aims at specifying ans implementing the protocol update
>>>>>> required to support device IOTLB with user backends.
>>>>>>
>>>>>> In this second non-RFC version, main changes are:
>>>>>> - spec fixes and clarification
>>>>>> - rings information update has been restored back to ring enablement time
>>>>>> - Work around GCC 4.4.7 limitation wrt assignment in unnamed union at
>>>>>> declaration time.
>>>>>>
>>>>>> The series can be tested with vhost_iotlb_proto_v2 branch on my gitlab
>>>>>> account[0].
>>>>>>
>>>>>> The slave requests channel part is re-used from Marc-André's series submitted
>>>>>> last year[1], with main changes from original version being request/feature
>>>>>> names renaming and addition of the REPLY_ACK feature support.
>>>>>>
>>>>>> Regarding IOTLB protocol, one noticeable change is the IOTLB miss request
>>>>>> reply made optionnal (i.e. only if slave requests it by setting the
>>>>>> VHOST_USER_NEED_REPLY flag in the message header). This change provides
>>>>>> more flexibility in the backend implementation of the feature.
>>>>>>
>>>>>> The protocol is very close to kernel backends, except that a new
>>>>>> communication channel is introduced to enable the slave to send
>>>>>> requests to the master.
>>>>>>
>>>>>> [0]:https://gitlab.com/mcoquelin/dpdk-next-virtio/commits/vhost_iotlb_proto_v2
>>>>>> [1]:https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html
>>>>> Overall, this looks good to me. I do think patch 3 isn't a good idea
>>>>> though, if slave wants something let it request it.
>>>>>
>>>>> Need to find out why does vhost in kernel want the used ring iotlb at
>>>>> start time - especially considering we aren't even guaranteed one entry
>>>>> covers the whole ring, and invalidates should affect all addresses at
>>>>> least in theory.
>>>>>
>>>>>
>>>> The reason is probably we want to verify whether or not we could correctly
>>>> access used ring in vhost_vq_init_access(). It was there since vhost_net is
>>>> introduced. We can think to remove this limitation maybe.
>>>>
>>>> Thanks
>>> Well that's only called if iotlb is disabled:
>>>
>>> if (!vq->iotlb &&
>>> !access_ok(VERIFY_READ, &vq->used->idx, sizeof vq->used->idx)) {
>>> r = -EFAULT;
>>> goto err;
>>> }
>>>
>>> Could you try removing that and see what breaks?
>>>
>> Looks not, the issue is vhost_update_used_flags() which needs device IOTLB
>> translation. If we don't fill IOTLB in advance, it will return -EFAULT.
> Same for vhost_get_used, right?
Yes.
>
>> For simplicity, I don't implement control path device IOTLB miss.
>
> OK so this should be documented in vhost.h. SET_BACKEND immediately
> writes and reads used ring. User must know this and pre-fault used flags
> and index before setting backend.
Ok.
>> If you
>> care about the incomplete length, we can refine vhost_iotlb_miss() to make
>> sure it covers all size.
>>
>> Thanks
> No need imho, it's only the used flags field that's written, and the
> index that's read right?
Yes.
> BTW I don't really know why do we do the write
> when event index is setup. We probably shouldn't, should we?
Yes, looks like we shouldn't.
>
> It's worth considering whether we want this write into used ring at all.
> I put it there originally to help make sure we don't miss the first exit, but
> event index seems to get by fine without this. So why does non event
> index code want it?
Spec said driver must initialize it to zero, so unless we want to
workaround a buggy driver we can remove this.
>
> I think that if we could get rid of both accesses, it would be
> nice. Would need a feature bit naturally and we'd need to
> support old kernels but at least it will be contained and
> well documented.
>
Technically we can, but we still need a workaround for old kernels. So
I'm not sure it's worth to do.
Thanks
On Mon, Jun 05, 2017 at 04:51:55PM +0800, Jason Wang wrote: > > > > I think that if we could get rid of both accesses, it would be > > nice. Would need a feature bit naturally and we'd need to > > support old kernels but at least it will be contained and > > well documented. > > > > Technically we can, but we still need a workaround for old kernels. So I'm > not sure it's worth to do. > > Thanks I see it as valuable for documentation purposes. -- MST
© 2016 - 2024 Red Hat, Inc.