* Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support
From: Jason Wang @ 2018-11-09 10:04 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtio-dev, netdev, linux-kernel, virtualization, wexu
In-Reply-To: <20181108225555-mutt-send-email-mst@kernel.org>
On 2018/11/9 上午11:58, Michael S. Tsirkin wrote:
> On Fri, Nov 09, 2018 at 10:25:28AM +0800, Jason Wang wrote:
>> On 2018/11/8 下午10:14, Michael S. Tsirkin wrote:
>>> On Thu, Nov 08, 2018 at 04:18:25PM +0800, Jason Wang wrote:
>>>> On 2018/11/8 上午9:38, Tiwei Bie wrote:
>>>>>>> +
>>>>>>> + if (vq->vq.num_free < descs_used) {
>>>>>>> + pr_debug("Can't add buf len %i - avail = %i\n",
>>>>>>> + descs_used, vq->vq.num_free);
>>>>>>> + /* FIXME: for historical reasons, we force a notify here if
>>>>>>> + * there are outgoing parts to the buffer. Presumably the
>>>>>>> + * host should service the ring ASAP. */
>>>>>> I don't think we have a reason to do this for packed ring.
>>>>>> No historical baggage there, right?
>>>>> Based on the original commit log, it seems that the notify here
>>>>> is just an "optimization". But I don't quite understand what does
>>>>> the "the heuristics which KVM uses" refer to. If it's safe to drop
>>>>> this in packed ring, I'd like to do it.
>>>> According to the commit log, it seems like a workaround of lguest networking
>>>> backend. I agree to drop it, we should not have such burden.
>>>>
>>>> But we should notice that, with this removed, the compare between packed vs
>>>> split is kind of unfair.
>>> I don't think this ever triggers to be frank. When would it?
>>
>> I think it can happen e.g in the path of XDP transmission in
>> __virtnet_xdp_xmit_one():
>>
>>
>> err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdpf, GFP_ATOMIC);
>> if (unlikely(err))
>> return -ENOSPC; /* Caller handle free/refcnt */
>>
> I see. We used to do it for regular xmit but stopped
> doing it. Is it fine for xdp then?
There's no traffic control in XDP, so it was the only thing we can do.
>
>>>> Consider the removal of lguest support recently,
>>>> maybe we can drop this for split ring as well?
>>>>
>>>> Thanks
>>> If it's helpful, then for sure we can drop it for virtio 1.
>>> Can you see any perf differences at all? With which device?
>>
>> I don't test but consider the case of XDP_TX in guest plus vhost_net in
>> host. Since vhost_net is half duplex, it's pretty easier to trigger this
>> condition.
>>
>> Thanks
> Sounds reasonable. Worth testing before we change things though.
Let me test and submit a patch.
Thanks
>
>>>>> commit 44653eae1407f79dff6f52fcf594ae84cb165ec4
>>>>> Author: Rusty Russell<rusty@rustcorp.com.au>
>>>>> Date: Fri Jul 25 12:06:04 2008 -0500
>>>>>
>>>>> virtio: don't always force a notification when ring is full
>>>>> We force notification when the ring is full, even if the host has
>>>>> indicated it doesn't want to know. This seemed like a good idea at
>>>>> the time: if we fill the transmit ring, we should tell the host
>>>>> immediately.
>>>>> Unfortunately this logic also applies to the receiving ring, which is
>>>>> refilled constantly. We should introduce real notification thesholds
>>>>> to replace this logic. Meanwhile, removing the logic altogether breaks
>>>>> the heuristics which KVM uses, so we use a hack: only notify if there are
>>>>> outgoing parts of the new buffer.
>>>>> Here are the number of exits with lguest's crappy network implementation:
>>>>> Before:
>>>>> network xmit 7859051 recv 236420
>>>>> After:
>>>>> network xmit 7858610 recv 118136
>>>>> Signed-off-by: Rusty Russell<rusty@rustcorp.com.au>
>>>>>
>>>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>>>> index 72bf8bc09014..21d9a62767af 100644
>>>>> --- a/drivers/virtio/virtio_ring.c
>>>>> +++ b/drivers/virtio/virtio_ring.c
>>>>> @@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueue *_vq,
>>>>> if (vq->num_free < out + in) {
>>>>> pr_debug("Can't add buf len %i - avail = %i\n",
>>>>> out + in, vq->num_free);
>>>>> - /* We notify*even if* VRING_USED_F_NO_NOTIFY is set here. */
>>>>> - vq->notify(&vq->vq);
>>>>> + /* FIXME: for historical reasons, we force a notify here if
>>>>> + * there are outgoing parts to the buffer. Presumably the
>>>>> + * host should service the ring ASAP. */
>>>>> + if (out)
>>>>> + vq->notify(&vq->vq);
>>>>> END_USE(vq);
>>>>> return -ENOSPC;
>>>>> }
>>>>>
>>>>>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support
From: Michael S. Tsirkin @ 2018-11-09 4:00 UTC (permalink / raw)
To: Jason Wang; +Cc: virtio-dev, netdev, linux-kernel, virtualization, wexu
In-Reply-To: <67bd6a88-00f2-ed13-ad13-bdfe92ceeffc@redhat.com>
On Fri, Nov 09, 2018 at 10:30:50AM +0800, Jason Wang wrote:
>
> On 2018/11/8 下午11:56, Michael S. Tsirkin wrote:
> > On Thu, Nov 08, 2018 at 07:51:48PM +0800, Tiwei Bie wrote:
> > > On Thu, Nov 08, 2018 at 04:18:25PM +0800, Jason Wang wrote:
> > > > On 2018/11/8 上午9:38, Tiwei Bie wrote:
> > > > > > > +
> > > > > > > + if (vq->vq.num_free < descs_used) {
> > > > > > > + pr_debug("Can't add buf len %i - avail = %i\n",
> > > > > > > + descs_used, vq->vq.num_free);
> > > > > > > + /* FIXME: for historical reasons, we force a notify here if
> > > > > > > + * there are outgoing parts to the buffer. Presumably the
> > > > > > > + * host should service the ring ASAP. */
> > > > > > I don't think we have a reason to do this for packed ring.
> > > > > > No historical baggage there, right?
> > > > > Based on the original commit log, it seems that the notify here
> > > > > is just an "optimization". But I don't quite understand what does
> > > > > the "the heuristics which KVM uses" refer to. If it's safe to drop
> > > > > this in packed ring, I'd like to do it.
> > > >
> > > > According to the commit log, it seems like a workaround of lguest networking
> > > > backend.
> > > Do you know why removing this notify in Tx will break "the
> > > heuristics which KVM uses"? Or what does "the heuristics
> > > which KVM uses" refer to?
> > Yes. QEMU has a mode where it disables notifications and processes TX
> > ring periodically from a timer. It's off by default but used to be on
> > by default a long time ago. If ring becomes full this causes traffic
> > stalls.
>
>
> Do you mean tx-timer? If yes, we can still enable it for packed ring
Yes we can but I doubt anyone does.
> and the
> timer will finally fired and we can go.
on tx ring full we probably don't want to wait for timer.
But I think we can just prevent qemu from using tx timer
with virtio 1.
>
> > As a work-around Rusty put in this hack to kick on ring full
> > even with notifications disabled.
>
>
> From the commit log it looks more like a performance workaround instead of a
> bug fix.
it's a quality of implementation issue, yes.
>
> > It's easy enough to make sure QEMU
> > does not combine devices with packed ring support with the timer hack.
> > And I am guessing it's safe enough to also block that option completely
> > e.g. when virtio 1.0 is enabled.
>
>
> I agree.
>
> Thanks
>
>
> > > > I agree to drop it, we should not have such burden.
> > > >
> > > > But we should notice that, with this removed, the compare between packed vs
> > > > split is kind of unfair. Consider the removal of lguest support recently,
> > > > maybe we can drop this for split ring as well?
> > > >
> > > > Thanks
> > > >
> > > >
> > > > > commit 44653eae1407f79dff6f52fcf594ae84cb165ec4
> > > > > Author: Rusty Russell<rusty@rustcorp.com.au>
> > > > > Date: Fri Jul 25 12:06:04 2008 -0500
> > > > >
> > > > > virtio: don't always force a notification when ring is full
> > > > > We force notification when the ring is full, even if the host has
> > > > > indicated it doesn't want to know. This seemed like a good idea at
> > > > > the time: if we fill the transmit ring, we should tell the host
> > > > > immediately.
> > > > > Unfortunately this logic also applies to the receiving ring, which is
> > > > > refilled constantly. We should introduce real notification thesholds
> > > > > to replace this logic. Meanwhile, removing the logic altogether breaks
> > > > > the heuristics which KVM uses, so we use a hack: only notify if there are
> > > > > outgoing parts of the new buffer.
> > > > > Here are the number of exits with lguest's crappy network implementation:
> > > > > Before:
> > > > > network xmit 7859051 recv 236420
> > > > > After:
> > > > > network xmit 7858610 recv 118136
> > > > > Signed-off-by: Rusty Russell<rusty@rustcorp.com.au>
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > index 72bf8bc09014..21d9a62767af 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueue *_vq,
> > > > > if (vq->num_free < out + in) {
> > > > > pr_debug("Can't add buf len %i - avail = %i\n",
> > > > > out + in, vq->num_free);
> > > > > - /* We notify*even if* VRING_USED_F_NO_NOTIFY is set here. */
> > > > > - vq->notify(&vq->vq);
> > > > > + /* FIXME: for historical reasons, we force a notify here if
> > > > > + * there are outgoing parts to the buffer. Presumably the
> > > > > + * host should service the ring ASAP. */
> > > > > + if (out)
> > > > > + vq->notify(&vq->vq);
> > > > > END_USE(vq);
> > > > > return -ENOSPC;
> > > > > }
> > > > >
> > > > >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support
From: Michael S. Tsirkin @ 2018-11-09 3:58 UTC (permalink / raw)
To: Jason Wang; +Cc: virtio-dev, netdev, linux-kernel, virtualization, wexu
In-Reply-To: <21d6dbd9-8f78-6939-0e80-27b470aeb00a@redhat.com>
On Fri, Nov 09, 2018 at 10:25:28AM +0800, Jason Wang wrote:
>
> On 2018/11/8 下午10:14, Michael S. Tsirkin wrote:
> > On Thu, Nov 08, 2018 at 04:18:25PM +0800, Jason Wang wrote:
> > > On 2018/11/8 上午9:38, Tiwei Bie wrote:
> > > > > > +
> > > > > > + if (vq->vq.num_free < descs_used) {
> > > > > > + pr_debug("Can't add buf len %i - avail = %i\n",
> > > > > > + descs_used, vq->vq.num_free);
> > > > > > + /* FIXME: for historical reasons, we force a notify here if
> > > > > > + * there are outgoing parts to the buffer. Presumably the
> > > > > > + * host should service the ring ASAP. */
> > > > > I don't think we have a reason to do this for packed ring.
> > > > > No historical baggage there, right?
> > > > Based on the original commit log, it seems that the notify here
> > > > is just an "optimization". But I don't quite understand what does
> > > > the "the heuristics which KVM uses" refer to. If it's safe to drop
> > > > this in packed ring, I'd like to do it.
> > >
> > > According to the commit log, it seems like a workaround of lguest networking
> > > backend. I agree to drop it, we should not have such burden.
> > >
> > > But we should notice that, with this removed, the compare between packed vs
> > > split is kind of unfair.
> > I don't think this ever triggers to be frank. When would it?
>
>
> I think it can happen e.g in the path of XDP transmission in
> __virtnet_xdp_xmit_one():
>
>
> err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdpf, GFP_ATOMIC);
> if (unlikely(err))
> return -ENOSPC; /* Caller handle free/refcnt */
>
I see. We used to do it for regular xmit but stopped
doing it. Is it fine for xdp then?
> >
> > > Consider the removal of lguest support recently,
> > > maybe we can drop this for split ring as well?
> > >
> > > Thanks
> > If it's helpful, then for sure we can drop it for virtio 1.
> > Can you see any perf differences at all? With which device?
>
>
> I don't test but consider the case of XDP_TX in guest plus vhost_net in
> host. Since vhost_net is half duplex, it's pretty easier to trigger this
> condition.
>
> Thanks
Sounds reasonable. Worth testing before we change things though.
>
> >
> > > > commit 44653eae1407f79dff6f52fcf594ae84cb165ec4
> > > > Author: Rusty Russell<rusty@rustcorp.com.au>
> > > > Date: Fri Jul 25 12:06:04 2008 -0500
> > > >
> > > > virtio: don't always force a notification when ring is full
> > > > We force notification when the ring is full, even if the host has
> > > > indicated it doesn't want to know. This seemed like a good idea at
> > > > the time: if we fill the transmit ring, we should tell the host
> > > > immediately.
> > > > Unfortunately this logic also applies to the receiving ring, which is
> > > > refilled constantly. We should introduce real notification thesholds
> > > > to replace this logic. Meanwhile, removing the logic altogether breaks
> > > > the heuristics which KVM uses, so we use a hack: only notify if there are
> > > > outgoing parts of the new buffer.
> > > > Here are the number of exits with lguest's crappy network implementation:
> > > > Before:
> > > > network xmit 7859051 recv 236420
> > > > After:
> > > > network xmit 7858610 recv 118136
> > > > Signed-off-by: Rusty Russell<rusty@rustcorp.com.au>
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index 72bf8bc09014..21d9a62767af 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueue *_vq,
> > > > if (vq->num_free < out + in) {
> > > > pr_debug("Can't add buf len %i - avail = %i\n",
> > > > out + in, vq->num_free);
> > > > - /* We notify*even if* VRING_USED_F_NO_NOTIFY is set here. */
> > > > - vq->notify(&vq->vq);
> > > > + /* FIXME: for historical reasons, we force a notify here if
> > > > + * there are outgoing parts to the buffer. Presumably the
> > > > + * host should service the ring ASAP. */
> > > > + if (out)
> > > > + vq->notify(&vq->vq);
> > > > END_USE(vq);
> > > > return -ENOSPC;
> > > > }
> > > >
> > > >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support
From: Jason Wang @ 2018-11-09 2:30 UTC (permalink / raw)
To: Michael S. Tsirkin, Tiwei Bie
Cc: virtio-dev, netdev, linux-kernel, virtualization, wexu
In-Reply-To: <20181108103155-mutt-send-email-mst@kernel.org>
On 2018/11/8 下午11:56, Michael S. Tsirkin wrote:
> On Thu, Nov 08, 2018 at 07:51:48PM +0800, Tiwei Bie wrote:
>> On Thu, Nov 08, 2018 at 04:18:25PM +0800, Jason Wang wrote:
>>> On 2018/11/8 上午9:38, Tiwei Bie wrote:
>>>>>> +
>>>>>> + if (vq->vq.num_free < descs_used) {
>>>>>> + pr_debug("Can't add buf len %i - avail = %i\n",
>>>>>> + descs_used, vq->vq.num_free);
>>>>>> + /* FIXME: for historical reasons, we force a notify here if
>>>>>> + * there are outgoing parts to the buffer. Presumably the
>>>>>> + * host should service the ring ASAP. */
>>>>> I don't think we have a reason to do this for packed ring.
>>>>> No historical baggage there, right?
>>>> Based on the original commit log, it seems that the notify here
>>>> is just an "optimization". But I don't quite understand what does
>>>> the "the heuristics which KVM uses" refer to. If it's safe to drop
>>>> this in packed ring, I'd like to do it.
>>>
>>> According to the commit log, it seems like a workaround of lguest networking
>>> backend.
>> Do you know why removing this notify in Tx will break "the
>> heuristics which KVM uses"? Or what does "the heuristics
>> which KVM uses" refer to?
> Yes. QEMU has a mode where it disables notifications and processes TX
> ring periodically from a timer. It's off by default but used to be on
> by default a long time ago. If ring becomes full this causes traffic
> stalls.
Do you mean tx-timer? If yes, we can still enable it for packed ring and
the timer will finally fired and we can go.
> As a work-around Rusty put in this hack to kick on ring full
> even with notifications disabled.
From the commit log it looks more like a performance workaround instead
of a bug fix.
> It's easy enough to make sure QEMU
> does not combine devices with packed ring support with the timer hack.
> And I am guessing it's safe enough to also block that option completely
> e.g. when virtio 1.0 is enabled.
I agree.
Thanks
>>> I agree to drop it, we should not have such burden.
>>>
>>> But we should notice that, with this removed, the compare between packed vs
>>> split is kind of unfair. Consider the removal of lguest support recently,
>>> maybe we can drop this for split ring as well?
>>>
>>> Thanks
>>>
>>>
>>>> commit 44653eae1407f79dff6f52fcf594ae84cb165ec4
>>>> Author: Rusty Russell<rusty@rustcorp.com.au>
>>>> Date: Fri Jul 25 12:06:04 2008 -0500
>>>>
>>>> virtio: don't always force a notification when ring is full
>>>> We force notification when the ring is full, even if the host has
>>>> indicated it doesn't want to know. This seemed like a good idea at
>>>> the time: if we fill the transmit ring, we should tell the host
>>>> immediately.
>>>> Unfortunately this logic also applies to the receiving ring, which is
>>>> refilled constantly. We should introduce real notification thesholds
>>>> to replace this logic. Meanwhile, removing the logic altogether breaks
>>>> the heuristics which KVM uses, so we use a hack: only notify if there are
>>>> outgoing parts of the new buffer.
>>>> Here are the number of exits with lguest's crappy network implementation:
>>>> Before:
>>>> network xmit 7859051 recv 236420
>>>> After:
>>>> network xmit 7858610 recv 118136
>>>> Signed-off-by: Rusty Russell<rusty@rustcorp.com.au>
>>>>
>>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>>> index 72bf8bc09014..21d9a62767af 100644
>>>> --- a/drivers/virtio/virtio_ring.c
>>>> +++ b/drivers/virtio/virtio_ring.c
>>>> @@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueue *_vq,
>>>> if (vq->num_free < out + in) {
>>>> pr_debug("Can't add buf len %i - avail = %i\n",
>>>> out + in, vq->num_free);
>>>> - /* We notify*even if* VRING_USED_F_NO_NOTIFY is set here. */
>>>> - vq->notify(&vq->vq);
>>>> + /* FIXME: for historical reasons, we force a notify here if
>>>> + * there are outgoing parts to the buffer. Presumably the
>>>> + * host should service the ring ASAP. */
>>>> + if (out)
>>>> + vq->notify(&vq->vq);
>>>> END_USE(vq);
>>>> return -ENOSPC;
>>>> }
>>>>
>>>>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support
From: Jason Wang @ 2018-11-09 2:25 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtio-dev, netdev, linux-kernel, virtualization, wexu
In-Reply-To: <20181108091337-mutt-send-email-mst@kernel.org>
On 2018/11/8 下午10:14, Michael S. Tsirkin wrote:
> On Thu, Nov 08, 2018 at 04:18:25PM +0800, Jason Wang wrote:
>> On 2018/11/8 上午9:38, Tiwei Bie wrote:
>>>>> +
>>>>> + if (vq->vq.num_free < descs_used) {
>>>>> + pr_debug("Can't add buf len %i - avail = %i\n",
>>>>> + descs_used, vq->vq.num_free);
>>>>> + /* FIXME: for historical reasons, we force a notify here if
>>>>> + * there are outgoing parts to the buffer. Presumably the
>>>>> + * host should service the ring ASAP. */
>>>> I don't think we have a reason to do this for packed ring.
>>>> No historical baggage there, right?
>>> Based on the original commit log, it seems that the notify here
>>> is just an "optimization". But I don't quite understand what does
>>> the "the heuristics which KVM uses" refer to. If it's safe to drop
>>> this in packed ring, I'd like to do it.
>>
>> According to the commit log, it seems like a workaround of lguest networking
>> backend. I agree to drop it, we should not have such burden.
>>
>> But we should notice that, with this removed, the compare between packed vs
>> split is kind of unfair.
> I don't think this ever triggers to be frank. When would it?
I think it can happen e.g in the path of XDP transmission in
__virtnet_xdp_xmit_one():
err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdpf, GFP_ATOMIC);
if (unlikely(err))
return -ENOSPC; /* Caller handle free/refcnt */
>
>> Consider the removal of lguest support recently,
>> maybe we can drop this for split ring as well?
>>
>> Thanks
> If it's helpful, then for sure we can drop it for virtio 1.
> Can you see any perf differences at all? With which device?
I don't test but consider the case of XDP_TX in guest plus vhost_net in
host. Since vhost_net is half duplex, it's pretty easier to trigger this
condition.
Thanks
>
>>> commit 44653eae1407f79dff6f52fcf594ae84cb165ec4
>>> Author: Rusty Russell<rusty@rustcorp.com.au>
>>> Date: Fri Jul 25 12:06:04 2008 -0500
>>>
>>> virtio: don't always force a notification when ring is full
>>> We force notification when the ring is full, even if the host has
>>> indicated it doesn't want to know. This seemed like a good idea at
>>> the time: if we fill the transmit ring, we should tell the host
>>> immediately.
>>> Unfortunately this logic also applies to the receiving ring, which is
>>> refilled constantly. We should introduce real notification thesholds
>>> to replace this logic. Meanwhile, removing the logic altogether breaks
>>> the heuristics which KVM uses, so we use a hack: only notify if there are
>>> outgoing parts of the new buffer.
>>> Here are the number of exits with lguest's crappy network implementation:
>>> Before:
>>> network xmit 7859051 recv 236420
>>> After:
>>> network xmit 7858610 recv 118136
>>> Signed-off-by: Rusty Russell<rusty@rustcorp.com.au>
>>>
>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>> index 72bf8bc09014..21d9a62767af 100644
>>> --- a/drivers/virtio/virtio_ring.c
>>> +++ b/drivers/virtio/virtio_ring.c
>>> @@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueue *_vq,
>>> if (vq->num_free < out + in) {
>>> pr_debug("Can't add buf len %i - avail = %i\n",
>>> out + in, vq->num_free);
>>> - /* We notify*even if* VRING_USED_F_NO_NOTIFY is set here. */
>>> - vq->notify(&vq->vq);
>>> + /* FIXME: for historical reasons, we force a notify here if
>>> + * there are outgoing parts to the buffer. Presumably the
>>> + * host should service the ring ASAP. */
>>> + if (out)
>>> + vq->notify(&vq->vq);
>>> END_USE(vq);
>>> return -ENOSPC;
>>> }
>>>
>>>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support
From: Tiwei Bie @ 2018-11-09 1:50 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtio-dev, netdev, linux-kernel, virtualization, wexu
In-Reply-To: <20181108103155-mutt-send-email-mst@kernel.org>
On Thu, Nov 08, 2018 at 10:56:02AM -0500, Michael S. Tsirkin wrote:
> On Thu, Nov 08, 2018 at 07:51:48PM +0800, Tiwei Bie wrote:
> > On Thu, Nov 08, 2018 at 04:18:25PM +0800, Jason Wang wrote:
> > >
> > > On 2018/11/8 上午9:38, Tiwei Bie wrote:
> > > > > > +
> > > > > > + if (vq->vq.num_free < descs_used) {
> > > > > > + pr_debug("Can't add buf len %i - avail = %i\n",
> > > > > > + descs_used, vq->vq.num_free);
> > > > > > + /* FIXME: for historical reasons, we force a notify here if
> > > > > > + * there are outgoing parts to the buffer. Presumably the
> > > > > > + * host should service the ring ASAP. */
> > > > > I don't think we have a reason to do this for packed ring.
> > > > > No historical baggage there, right?
> > > > Based on the original commit log, it seems that the notify here
> > > > is just an "optimization". But I don't quite understand what does
> > > > the "the heuristics which KVM uses" refer to. If it's safe to drop
> > > > this in packed ring, I'd like to do it.
> > >
> > >
> > > According to the commit log, it seems like a workaround of lguest networking
> > > backend.
> >
> > Do you know why removing this notify in Tx will break "the
> > heuristics which KVM uses"? Or what does "the heuristics
> > which KVM uses" refer to?
>
> Yes. QEMU has a mode where it disables notifications and processes TX
> ring periodically from a timer. It's off by default but used to be on
> by default a long time ago. If ring becomes full this causes traffic
> stalls. As a work-around Rusty put in this hack to kick on ring full
> even with notifications disabled. It's easy enough to make sure QEMU
> does not combine devices with packed ring support with the timer hack.
> And I am guessing it's safe enough to also block that option completely
> e.g. when virtio 1.0 is enabled.
I see. Thanks!
>
> >
> > > I agree to drop it, we should not have such burden.
> > >
> > > But we should notice that, with this removed, the compare between packed vs
> > > split is kind of unfair. Consider the removal of lguest support recently,
> > > maybe we can drop this for split ring as well?
> > >
> > > Thanks
> > >
> > >
> > > >
> > > > commit 44653eae1407f79dff6f52fcf594ae84cb165ec4
> > > > Author: Rusty Russell<rusty@rustcorp.com.au>
> > > > Date: Fri Jul 25 12:06:04 2008 -0500
> > > >
> > > > virtio: don't always force a notification when ring is full
> > > > We force notification when the ring is full, even if the host has
> > > > indicated it doesn't want to know. This seemed like a good idea at
> > > > the time: if we fill the transmit ring, we should tell the host
> > > > immediately.
> > > > Unfortunately this logic also applies to the receiving ring, which is
> > > > refilled constantly. We should introduce real notification thesholds
> > > > to replace this logic. Meanwhile, removing the logic altogether breaks
> > > > the heuristics which KVM uses, so we use a hack: only notify if there are
> > > > outgoing parts of the new buffer.
> > > > Here are the number of exits with lguest's crappy network implementation:
> > > > Before:
> > > > network xmit 7859051 recv 236420
> > > > After:
> > > > network xmit 7858610 recv 118136
> > > > Signed-off-by: Rusty Russell<rusty@rustcorp.com.au>
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index 72bf8bc09014..21d9a62767af 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueue *_vq,
> > > > if (vq->num_free < out + in) {
> > > > pr_debug("Can't add buf len %i - avail = %i\n",
> > > > out + in, vq->num_free);
> > > > - /* We notify*even if* VRING_USED_F_NO_NOTIFY is set here. */
> > > > - vq->notify(&vq->vq);
> > > > + /* FIXME: for historical reasons, we force a notify here if
> > > > + * there are outgoing parts to the buffer. Presumably the
> > > > + * host should service the ring ASAP. */
> > > > + if (out)
> > > > + vq->notify(&vq->vq);
> > > > END_USE(vq);
> > > > return -ENOSPC;
> > > > }
> > > >
> > > >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH 4.9 121/171] x86/paravirt: Fix some warning messages
From: Greg Kroah-Hartman @ 2018-11-08 21:51 UTC (permalink / raw)
To: linux-kernel
Cc: Juergen Gross, Sasha Levin, Peter Zijlstra, Greg Kroah-Hartman,
kernel-janitors, stable, virtualization, H. Peter Anvin,
Thomas Gleixner, Alok Kataria, Dan Carpenter
In-Reply-To: <20181108215127.257643509@linuxfoundation.org>
4.9-stable review patch. If anyone has any objections, please let me know.
------------------
[ Upstream commit 571d0563c8881595f4ab027aef9ed1c55e3e7b7c ]
The first argument to WARN_ONCE() is a condition.
Fixes: 5800dc5c19f3 ("x86/paravirt: Fix spectre-v2 mitigations for paravirt guests")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Juergen Gross <jgross@suse.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alok Kataria <akataria@vmware.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: virtualization@lists.linux-foundation.org
Cc: kernel-janitors@vger.kernel.org
Link: https://lkml.kernel.org/r/20180919103553.GD9238@mwanda
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
arch/x86/kernel/paravirt.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index 29d465627919..bf9552bebb3c 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -90,7 +90,7 @@ unsigned paravirt_patch_call(void *insnbuf,
if (len < 5) {
#ifdef CONFIG_RETPOLINE
- WARN_ONCE("Failing to patch indirect CALL in %ps\n", (void *)addr);
+ WARN_ONCE(1, "Failing to patch indirect CALL in %ps\n", (void *)addr);
#endif
return len; /* call too long for patch site */
}
@@ -110,7 +110,7 @@ unsigned paravirt_patch_jmp(void *insnbuf, const void *target,
if (len < 5) {
#ifdef CONFIG_RETPOLINE
- WARN_ONCE("Failing to patch indirect JMP in %ps\n", (void *)addr);
+ WARN_ONCE(1, "Failing to patch indirect JMP in %ps\n", (void *)addr);
#endif
return len; /* call too long for patch site */
}
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v3 5/7] iommu: Add virtio-iommu driver
From: Jean-Philippe Brucker @ 2018-11-08 16:46 UTC (permalink / raw)
To: Auger Eric, Michael S. Tsirkin
Cc: mark.rutland, peter.maydell, lorenzo.pieralisi, tnowicki,
devicetree, marc.zyngier, linux-pci, joro, will.deacon,
virtualization, iommu, robh+dt, robin.murphy, kvmarm
In-Reply-To: <60605670-0419-3f64-a379-9c4057d90db1@redhat.com>
On 08/11/2018 14:51, Auger Eric wrote:
>>> +/*
>>> + * viommu_replay_mappings - re-send MAP requests
>>> + *
>>> + * When reattaching a domain that was previously detached from all endpoints,
>>> + * mappings were deleted from the device. Re-create the mappings available in
>>> + * the internal tree.
>>> + */
>>> +static int viommu_replay_mappings(struct viommu_domain *vdomain)
>>> +{
>>> + int ret;
> ret needs to be initialized here. Otherwise this can lead to a crash in
> viommu_add_device.
I actually hit this one while testing the other day :) Fixed in next version
Thanks,
Jean
^ permalink raw reply
* Re: [PATCH v3 6/7] iommu/virtio: Add probe request
From: Jean-Philippe Brucker @ 2018-11-08 16:46 UTC (permalink / raw)
To: Auger Eric, iommu, virtualization, devicetree
Cc: mark.rutland, peter.maydell, tnowicki, mst, marc.zyngier,
linux-pci, will.deacon, robh+dt, robin.murphy, kvmarm
In-Reply-To: <295d30bb-5aef-2727-01c0-ec10c7a8fa8c@redhat.com>
On 08/11/2018 14:48, Auger Eric wrote:
>> +struct virtio_iommu_probe_property {
>> + __le16 type;
>> + __le16 length;
> the value[] field has disappeared but still is documented in the v0.8 spec.
Good catch. I removed value[] when reworking the
virtio_iommu_probe_resv_mem definition, because embedding a struct with
a flexible array into another violates the C99 standard, even though GCC
accepts it.
I'll remove it from the spec as well, but I probably won't publish a new
version for this change alone. The virtio spec itself has similar uses
of flexible arrays, that are given for explanation but aren't valid C
(and those wouldn't even compile).
Thanks,
Jean
^ permalink raw reply
* Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support
From: Michael S. Tsirkin @ 2018-11-08 15:56 UTC (permalink / raw)
To: Tiwei Bie; +Cc: virtio-dev, netdev, linux-kernel, virtualization, wexu
In-Reply-To: <20181108115148.GA15701@debian>
On Thu, Nov 08, 2018 at 07:51:48PM +0800, Tiwei Bie wrote:
> On Thu, Nov 08, 2018 at 04:18:25PM +0800, Jason Wang wrote:
> >
> > On 2018/11/8 上午9:38, Tiwei Bie wrote:
> > > > > +
> > > > > + if (vq->vq.num_free < descs_used) {
> > > > > + pr_debug("Can't add buf len %i - avail = %i\n",
> > > > > + descs_used, vq->vq.num_free);
> > > > > + /* FIXME: for historical reasons, we force a notify here if
> > > > > + * there are outgoing parts to the buffer. Presumably the
> > > > > + * host should service the ring ASAP. */
> > > > I don't think we have a reason to do this for packed ring.
> > > > No historical baggage there, right?
> > > Based on the original commit log, it seems that the notify here
> > > is just an "optimization". But I don't quite understand what does
> > > the "the heuristics which KVM uses" refer to. If it's safe to drop
> > > this in packed ring, I'd like to do it.
> >
> >
> > According to the commit log, it seems like a workaround of lguest networking
> > backend.
>
> Do you know why removing this notify in Tx will break "the
> heuristics which KVM uses"? Or what does "the heuristics
> which KVM uses" refer to?
Yes. QEMU has a mode where it disables notifications and processes TX
ring periodically from a timer. It's off by default but used to be on
by default a long time ago. If ring becomes full this causes traffic
stalls. As a work-around Rusty put in this hack to kick on ring full
even with notifications disabled. It's easy enough to make sure QEMU
does not combine devices with packed ring support with the timer hack.
And I am guessing it's safe enough to also block that option completely
e.g. when virtio 1.0 is enabled.
>
> > I agree to drop it, we should not have such burden.
> >
> > But we should notice that, with this removed, the compare between packed vs
> > split is kind of unfair. Consider the removal of lguest support recently,
> > maybe we can drop this for split ring as well?
> >
> > Thanks
> >
> >
> > >
> > > commit 44653eae1407f79dff6f52fcf594ae84cb165ec4
> > > Author: Rusty Russell<rusty@rustcorp.com.au>
> > > Date: Fri Jul 25 12:06:04 2008 -0500
> > >
> > > virtio: don't always force a notification when ring is full
> > > We force notification when the ring is full, even if the host has
> > > indicated it doesn't want to know. This seemed like a good idea at
> > > the time: if we fill the transmit ring, we should tell the host
> > > immediately.
> > > Unfortunately this logic also applies to the receiving ring, which is
> > > refilled constantly. We should introduce real notification thesholds
> > > to replace this logic. Meanwhile, removing the logic altogether breaks
> > > the heuristics which KVM uses, so we use a hack: only notify if there are
> > > outgoing parts of the new buffer.
> > > Here are the number of exits with lguest's crappy network implementation:
> > > Before:
> > > network xmit 7859051 recv 236420
> > > After:
> > > network xmit 7858610 recv 118136
> > > Signed-off-by: Rusty Russell<rusty@rustcorp.com.au>
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 72bf8bc09014..21d9a62767af 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueue *_vq,
> > > if (vq->num_free < out + in) {
> > > pr_debug("Can't add buf len %i - avail = %i\n",
> > > out + in, vq->num_free);
> > > - /* We notify*even if* VRING_USED_F_NO_NOTIFY is set here. */
> > > - vq->notify(&vq->vq);
> > > + /* FIXME: for historical reasons, we force a notify here if
> > > + * there are outgoing parts to the buffer. Presumably the
> > > + * host should service the ring ASAP. */
> > > + if (out)
> > > + vq->notify(&vq->vq);
> > > END_USE(vq);
> > > return -ENOSPC;
> > > }
> > >
> > >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v3 5/7] iommu: Add virtio-iommu driver
From: Auger Eric @ 2018-11-08 14:51 UTC (permalink / raw)
To: Michael S. Tsirkin, Jean-Philippe Brucker
Cc: mark.rutland, peter.maydell, lorenzo.pieralisi, tnowicki,
devicetree, linux-pci, joro, will.deacon, virtualization,
marc.zyngier, iommu, robh+dt, robin.murphy, kvmarm
In-Reply-To: <20181012120953-mutt-send-email-mst@kernel.org>
Hi Jean-Philippe,
On 10/12/18 6:35 PM, Michael S. Tsirkin wrote:
> On Fri, Oct 12, 2018 at 03:59:15PM +0100, Jean-Philippe Brucker wrote:
>> The virtio IOMMU is a para-virtualized device, allowing to send IOMMU
>> requests such as map/unmap over virtio transport without emulating page
>> tables. This implementation handles ATTACH, DETACH, MAP and UNMAP
>> requests.
>>
>> The bulk of the code transforms calls coming from the IOMMU API into
>> corresponding virtio requests. Mappings are kept in an interval tree
>> instead of page tables.
>>
>> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
>> ---
>> MAINTAINERS | 7 +
>> drivers/iommu/Kconfig | 11 +
>> drivers/iommu/Makefile | 1 +
>> drivers/iommu/virtio-iommu.c | 938 ++++++++++++++++++++++++++++++
>> include/uapi/linux/virtio_ids.h | 1 +
>> include/uapi/linux/virtio_iommu.h | 101 ++++
>> 6 files changed, 1059 insertions(+)
>> create mode 100644 drivers/iommu/virtio-iommu.c
>> create mode 100644 include/uapi/linux/virtio_iommu.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 48a65c3a4189..f02fa65f47e2 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -15599,6 +15599,13 @@ S: Maintained
>> F: drivers/virtio/virtio_input.c
>> F: include/uapi/linux/virtio_input.h
>>
>> +VIRTIO IOMMU DRIVER
>> +M: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
>> +L: virtualization@lists.linux-foundation.org
>> +S: Maintained
>> +F: drivers/iommu/virtio-iommu.c
>> +F: include/uapi/linux/virtio_iommu.h
>> +
>> VIRTUAL BOX GUEST DEVICE DRIVER
>> M: Hans de Goede <hdegoede@redhat.com>
>> M: Arnd Bergmann <arnd@arndb.de>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index c60395b7470f..2dc016dc2b92 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -414,4 +414,15 @@ config QCOM_IOMMU
>> help
>> Support for IOMMU on certain Qualcomm SoCs.
>>
>> +config VIRTIO_IOMMU
>> + bool "Virtio IOMMU driver"
>> + depends on VIRTIO=y
>> + select IOMMU_API
>> + select INTERVAL_TREE
>> + select ARM_DMA_USE_IOMMU if ARM
>> + help
>> + Para-virtualised IOMMU driver with virtio.
>> +
>> + Say Y here if you intend to run this kernel as a guest.
>> +
>> endif # IOMMU_SUPPORT
>> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
>> index ab5eba6edf82..4cd643408e49 100644
>> --- a/drivers/iommu/Makefile
>> +++ b/drivers/iommu/Makefile
>> @@ -31,3 +31,4 @@ obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o
>> obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o
>> obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
>> obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o
>> +obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
>> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
>> new file mode 100644
>> index 000000000000..9fb38cd3b727
>> --- /dev/null
>> +++ b/drivers/iommu/virtio-iommu.c
>> @@ -0,0 +1,938 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Virtio driver for the paravirtualized IOMMU
>> + *
>> + * Copyright (C) 2018 Arm Limited
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/amba/bus.h>
>> +#include <linux/delay.h>
>> +#include <linux/dma-iommu.h>
>> +#include <linux/freezer.h>
>> +#include <linux/interval_tree.h>
>> +#include <linux/iommu.h>
>> +#include <linux/module.h>
>> +#include <linux/of_iommu.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/pci.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/virtio.h>
>> +#include <linux/virtio_config.h>
>> +#include <linux/virtio_ids.h>
>> +#include <linux/wait.h>
>> +
>> +#include <uapi/linux/virtio_iommu.h>
>> +
>> +#define MSI_IOVA_BASE 0x8000000
>> +#define MSI_IOVA_LENGTH 0x100000
>> +
>> +#define VIOMMU_REQUEST_VQ 0
>> +#define VIOMMU_NR_VQS 1
>> +
>> +/*
>> + * During development, it is convenient to time out rather than wait
>> + * indefinitely in atomic context when a device misbehaves and a request doesn't
>> + * return. In production however, some requests shouldn't return until they are
>> + * successful.
>> + */
>> +#ifdef DEBUG
>> +#define VIOMMU_REQUEST_TIMEOUT 10000 /* 10s */
>> +#endif
>> +
>> +struct viommu_dev {
>> + struct iommu_device iommu;
>> + struct device *dev;
>> + struct virtio_device *vdev;
>> +
>> + struct ida domain_ids;
>> +
>> + struct virtqueue *vqs[VIOMMU_NR_VQS];
>> + spinlock_t request_lock;
>> + struct list_head requests;
>> +
>> + /* Device configuration */
>> + struct iommu_domain_geometry geometry;
>> + u64 pgsize_bitmap;
>> + u8 domain_bits;
>> +};
>> +
>> +struct viommu_mapping {
>> + phys_addr_t paddr;
>> + struct interval_tree_node iova;
>> + u32 flags;
>> +};
>> +
>> +struct viommu_domain {
>> + struct iommu_domain domain;
>> + struct viommu_dev *viommu;
>> + struct mutex mutex;
>> + unsigned int id;
>> +
>> + spinlock_t mappings_lock;
>> + struct rb_root_cached mappings;
>> +
>> + unsigned long nr_endpoints;
>> +};
>> +
>> +struct viommu_endpoint {
>> + struct viommu_dev *viommu;
>> + struct viommu_domain *vdomain;
>> +};
>> +
>> +struct viommu_request {
>> + struct list_head list;
>> + void *writeback;
>> + unsigned int write_offset;
>> + unsigned int len;
>> + char buf[];
>> +};
>> +
>> +#define to_viommu_domain(domain) \
>> + container_of(domain, struct viommu_domain, domain)
>> +
>> +static int viommu_get_req_errno(void *buf, size_t len)
>> +{
>> + struct virtio_iommu_req_tail *tail = buf + len - sizeof(*tail);
>> +
>> + switch (tail->status) {
>> + case VIRTIO_IOMMU_S_OK:
>> + return 0;
>> + case VIRTIO_IOMMU_S_UNSUPP:
>> + return -ENOSYS;
>> + case VIRTIO_IOMMU_S_INVAL:
>> + return -EINVAL;
>> + case VIRTIO_IOMMU_S_RANGE:
>> + return -ERANGE;
>> + case VIRTIO_IOMMU_S_NOENT:
>> + return -ENOENT;
>> + case VIRTIO_IOMMU_S_FAULT:
>> + return -EFAULT;
>> + case VIRTIO_IOMMU_S_IOERR:
>> + case VIRTIO_IOMMU_S_DEVERR:
>> + default:
>> + return -EIO;
>> + }
>> +}
>> +
>> +static void viommu_set_req_status(void *buf, size_t len, int status)
>> +{
>> + struct virtio_iommu_req_tail *tail = buf + len - sizeof(*tail);
>> +
>> + tail->status = status;
>> +}
>> +
>> +static off_t viommu_get_req_offset(struct viommu_dev *viommu,
>> + struct virtio_iommu_req_head *req,
>> + size_t len)
>> +{
>> + size_t tail_size = sizeof(struct virtio_iommu_req_tail);
>> +
>> + return len - tail_size;
>> +}
>> +
>> +/*
>> + * __viommu_sync_req - Complete all in-flight requests
>> + *
>> + * Wait for all added requests to complete. When this function returns, all
>> + * requests that were in-flight at the time of the call have completed.
>> + */
>> +static int __viommu_sync_req(struct viommu_dev *viommu)
>> +{
>> + int ret = 0;
>> + unsigned int len;
>> + size_t write_len;
>> + ktime_t timeout = 0;
>> + struct viommu_request *req;
>> + struct virtqueue *vq = viommu->vqs[VIOMMU_REQUEST_VQ];
>> +
>> + assert_spin_locked(&viommu->request_lock);
>> +#ifdef DEBUG
>> + timeout = ktime_add_ms(ktime_get(), VIOMMU_REQUEST_TIMEOUT);
>> +#endif
>> + virtqueue_kick(vq);
>> +
>> + while (!list_empty(&viommu->requests)) {
>> + len = 0;
>> + req = virtqueue_get_buf(vq, &len);
>> + if (req == NULL) {
>> + if (!timeout || ktime_before(ktime_get(), timeout))
>> + continue;
>> +
>> + /* After timeout, remove all requests */
>> + req = list_first_entry(&viommu->requests,
>> + struct viommu_request, list);
>> + ret = -ETIMEDOUT;
>> + }
>> +
>> + if (!len)
>> + viommu_set_req_status(req->buf, req->len,
>> + VIRTIO_IOMMU_S_IOERR);
>> +
>> + write_len = req->len - req->write_offset;
>> + if (req->writeback && len >= write_len)
>> + memcpy(req->writeback, req->buf + req->write_offset,
>> + write_len);
>> +
>> + list_del(&req->list);
>> + kfree(req);
>
> So with DEBUG set, this will actually free memory that device still
> DMA's into. Hardly pretty. I think you want to mark device broken,
> queue the request and then wait for device to be reset.
>
>
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int viommu_sync_req(struct viommu_dev *viommu)
>> +{
>> + int ret;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&viommu->request_lock, flags);
>> + ret = __viommu_sync_req(viommu);
>> + if (ret)
>> + dev_dbg(viommu->dev, "could not sync requests (%d)\n", ret);
>> + spin_unlock_irqrestore(&viommu->request_lock, flags);
>> +
>> + return ret;
>> +}
>> +
>> +/*
>> + * __viommu_add_request - Add one request to the queue
>> + * @buf: pointer to the request buffer
>> + * @len: length of the request buffer
>> + * @writeback: copy data back to the buffer when the request completes.
>> + *
>> + * Add a request to the queue. Only synchronize the queue if it's already full.
>> + * Otherwise don't kick the queue nor wait for requests to complete.
>> + *
>> + * When @writeback is true, data written by the device, including the request
>> + * status, is copied into @buf after the request completes. This is unsafe if
>> + * the caller allocates @buf on stack and drops the lock between add_req() and
>> + * sync_req().
>> + *
>> + * Return 0 if the request was successfully added to the queue.
>> + */
>> +static int __viommu_add_req(struct viommu_dev *viommu, void *buf, size_t len,
>> + bool writeback)
>> +{
>> + int ret;
>> + off_t write_offset;
>> + struct viommu_request *req;
>> + struct scatterlist top_sg, bottom_sg;
>> + struct scatterlist *sg[2] = { &top_sg, &bottom_sg };
>> + struct virtqueue *vq = viommu->vqs[VIOMMU_REQUEST_VQ];
>> +
>> + assert_spin_locked(&viommu->request_lock);
>> +
>> + write_offset = viommu_get_req_offset(viommu, buf, len);
>> + if (!write_offset)
>> + return -EINVAL;
>> +
>> + req = kzalloc(sizeof(*req) + len, GFP_ATOMIC);
>> + if (!req)
>> + return -ENOMEM;
>> +
>> + req->len = len;
>> + if (writeback) {
>> + req->writeback = buf + write_offset;
>> + req->write_offset = write_offset;
>> + }
>> + memcpy(&req->buf, buf, write_offset);
>> +
>> + sg_init_one(&top_sg, req->buf, write_offset);
>> + sg_init_one(&bottom_sg, req->buf + write_offset, len - write_offset);
>> +
>> + ret = virtqueue_add_sgs(vq, sg, 1, 1, req, GFP_ATOMIC);
>> + if (ret == -ENOSPC) {
>> + /* If the queue is full, sync and retry */
>> + if (!__viommu_sync_req(viommu))
>> + ret = virtqueue_add_sgs(vq, sg, 1, 1, req, GFP_ATOMIC);
>> + }
>> + if (ret)
>> + goto err_free;
>> +
>> + list_add_tail(&req->list, &viommu->requests);
>> + return 0;
>> +
>> +err_free:
>> + kfree(req);
>> + return ret;
>> +}
>> +
>> +static int viommu_add_req(struct viommu_dev *viommu, void *buf, size_t len)
>> +{
>> + int ret;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&viommu->request_lock, flags);
>> + ret = __viommu_add_req(viommu, buf, len, false);
>> + if (ret)
>> + dev_dbg(viommu->dev, "could not add request: %d\n", ret);
>> + spin_unlock_irqrestore(&viommu->request_lock, flags);
>> +
>> + return ret;
>> +}
>> +
>> +/*
>> + * Send a request and wait for it to complete. Return the request status (as an
>> + * errno)
>> + */
>> +static int viommu_send_req_sync(struct viommu_dev *viommu, void *buf,
>> + size_t len)
>> +{
>> + int ret;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&viommu->request_lock, flags);
>> +
>> + ret = __viommu_add_req(viommu, buf, len, true);
>> + if (ret) {
>> + dev_dbg(viommu->dev, "could not add request (%d)\n", ret);
>> + goto out_unlock;
>> + }
>> +
>> + ret = __viommu_sync_req(viommu);
>> + if (ret) {
>> + dev_dbg(viommu->dev, "could not sync requests (%d)\n", ret);
>> + /* Fall-through (get the actual request status) */
>> + }
>> +
>> + ret = viommu_get_req_errno(buf, len);
>> +out_unlock:
>> + spin_unlock_irqrestore(&viommu->request_lock, flags);
>> + return ret;
>> +}
>> +
>> +/*
>> + * viommu_add_mapping - add a mapping to the internal tree
>> + *
>> + * On success, return the new mapping. Otherwise return NULL.
>> + */
>> +static struct viommu_mapping *
>> +viommu_add_mapping(struct viommu_domain *vdomain, unsigned long iova,
>> + phys_addr_t paddr, size_t size, u32 flags)
>> +{
>> + unsigned long irqflags;
>> + struct viommu_mapping *mapping;
>> +
>> + mapping = kzalloc(sizeof(*mapping), GFP_ATOMIC);
>> + if (!mapping)
>> + return NULL;
>> +
>> + mapping->paddr = paddr;
>> + mapping->iova.start = iova;
>> + mapping->iova.last = iova + size - 1;
>> + mapping->flags = flags;
>> +
>> + spin_lock_irqsave(&vdomain->mappings_lock, irqflags);
>> + interval_tree_insert(&mapping->iova, &vdomain->mappings);
>> + spin_unlock_irqrestore(&vdomain->mappings_lock, irqflags);
>> +
>> + return mapping;
>> +}
>> +
>> +/*
>> + * viommu_del_mappings - remove mappings from the internal tree
>> + *
>> + * @vdomain: the domain
>> + * @iova: start of the range
>> + * @size: size of the range. A size of 0 corresponds to the entire address
>> + * space.
>> + *
>> + * On success, returns the number of unmapped bytes (>= size)
>> + */
>> +static size_t viommu_del_mappings(struct viommu_domain *vdomain,
>> + unsigned long iova, size_t size)
>> +{
>> + size_t unmapped = 0;
>> + unsigned long flags;
>> + unsigned long last = iova + size - 1;
>> + struct viommu_mapping *mapping = NULL;
>> + struct interval_tree_node *node, *next;
>> +
>> + spin_lock_irqsave(&vdomain->mappings_lock, flags);
>> + next = interval_tree_iter_first(&vdomain->mappings, iova, last);
>> + while (next) {
>> + node = next;
>> + mapping = container_of(node, struct viommu_mapping, iova);
>> + next = interval_tree_iter_next(node, iova, last);
>> +
>> + /* Trying to split a mapping? */
>> + if (mapping->iova.start < iova)
>> + break;
>> +
>> + /*
>> + * Note that for a partial range, this will return the full
>> + * mapping so we avoid sending split requests to the device.
>> + */
>> + unmapped += mapping->iova.last - mapping->iova.start + 1;
>> +
>> + interval_tree_remove(node, &vdomain->mappings);
>> + kfree(mapping);
>> + }
>> + spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
>> +
>> + return unmapped;
>> +}
>> +
>> +/*
>> + * viommu_replay_mappings - re-send MAP requests
>> + *
>> + * When reattaching a domain that was previously detached from all endpoints,
>> + * mappings were deleted from the device. Re-create the mappings available in
>> + * the internal tree.
>> + */
>> +static int viommu_replay_mappings(struct viommu_domain *vdomain)
>> +{
>> + int ret;
ret needs to be initialized here. Otherwise this can lead to a crash in
viommu_add_device.
Thanks
Eric
>> + unsigned long flags;
>> + struct viommu_mapping *mapping;
>> + struct interval_tree_node *node;
>> + struct virtio_iommu_req_map map;
>> +
>> + spin_lock_irqsave(&vdomain->mappings_lock, flags);
>> + node = interval_tree_iter_first(&vdomain->mappings, 0, -1UL);
>> + while (node) {
>> + mapping = container_of(node, struct viommu_mapping, iova);
>> + map = (struct virtio_iommu_req_map) {
>> + .head.type = VIRTIO_IOMMU_T_MAP,
>> + .domain = cpu_to_le32(vdomain->id),
>> + .virt_start = cpu_to_le64(mapping->iova.start),
>> + .virt_end = cpu_to_le64(mapping->iova.last),
>> + .phys_start = cpu_to_le64(mapping->paddr),
>> + .flags = cpu_to_le32(mapping->flags),
>> + };
>> +
>> + ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
>> + if (ret)
>> + break;
>> +
>> + node = interval_tree_iter_next(node, 0, -1UL);
>> + }
>> + spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
>> +
>> + return ret;
>> +}
>> +
>> +/* IOMMU API */
>> +
>> +static struct iommu_domain *viommu_domain_alloc(unsigned type)
>> +{
>> + struct viommu_domain *vdomain;
>> +
>> + if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
>> + return NULL;
>> +
>> + vdomain = kzalloc(sizeof(*vdomain), GFP_KERNEL);
>> + if (!vdomain)
>> + return NULL;
>> +
>> + mutex_init(&vdomain->mutex);
>> + spin_lock_init(&vdomain->mappings_lock);
>> + vdomain->mappings = RB_ROOT_CACHED;
>> +
>> + if (type == IOMMU_DOMAIN_DMA &&
>> + iommu_get_dma_cookie(&vdomain->domain)) {
>> + kfree(vdomain);
>> + return NULL;
>> + }
>> +
>> + return &vdomain->domain;
>> +}
>> +
>> +static int viommu_domain_finalise(struct viommu_dev *viommu,
>> + struct iommu_domain *domain)
>> +{
>> + int ret;
>> + struct viommu_domain *vdomain = to_viommu_domain(domain);
>> + unsigned int max_domain = viommu->domain_bits > 31 ? ~0 :
>> + (1U << viommu->domain_bits) - 1;
>> +
>> + vdomain->viommu = viommu;
>> +
>> + domain->pgsize_bitmap = viommu->pgsize_bitmap;
>> + domain->geometry = viommu->geometry;
>> +
>> + ret = ida_alloc_max(&viommu->domain_ids, max_domain, GFP_KERNEL);
>> + if (ret >= 0)
>> + vdomain->id = (unsigned int)ret;
>> +
>> + return ret > 0 ? 0 : ret;
>> +}
>> +
>> +static void viommu_domain_free(struct iommu_domain *domain)
>> +{
>> + struct viommu_domain *vdomain = to_viommu_domain(domain);
>> +
>> + iommu_put_dma_cookie(domain);
>> +
>> + /* Free all remaining mappings (size 2^64) */
>> + viommu_del_mappings(vdomain, 0, 0);
>> +
>> + if (vdomain->viommu)
>> + ida_free(&vdomain->viommu->domain_ids, vdomain->id);
>> +
>> + kfree(vdomain);
>> +}
>> +
>> +static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
>> +{
>> + int i;
>> + int ret = 0;
>> + struct virtio_iommu_req_attach req;
>> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>> + struct viommu_endpoint *vdev = fwspec->iommu_priv;
>> + struct viommu_domain *vdomain = to_viommu_domain(domain);
>> +
>> + mutex_lock(&vdomain->mutex);
>> + if (!vdomain->viommu) {
>> + /*
>> + * Initialize the domain proper now that we know which viommu
>> + * owns it.
>> + */
>> + ret = viommu_domain_finalise(vdev->viommu, domain);
>> + } else if (vdomain->viommu != vdev->viommu) {
>> + dev_err(dev, "cannot attach to foreign vIOMMU\n");
>> + ret = -EXDEV;
>> + }
>> + mutex_unlock(&vdomain->mutex);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * In the virtio-iommu device, when attaching the endpoint to a new
>> + * domain, it is detached from the old one and, if as as a result the
>> + * old domain isn't attached to any endpoint, all mappings are removed
>> + * from the old domain and it is freed.
>> + *
>> + * In the driver the old domain still exists, and its mappings will be
>> + * recreated if it gets reattached to an endpoint. Otherwise it will be
>> + * freed explicitly.
>> + *
>> + * vdev->vdomain is protected by group->mutex
>> + */
>> + if (vdev->vdomain)
>> + vdev->vdomain->nr_endpoints--;
>> +
>> + req = (struct virtio_iommu_req_attach) {
>> + .head.type = VIRTIO_IOMMU_T_ATTACH,
>> + .domain = cpu_to_le32(vdomain->id),
>> + };
>> +
>> + for (i = 0; i < fwspec->num_ids; i++) {
>> + req.endpoint = cpu_to_le32(fwspec->ids[i]);
>> +
>> + ret = viommu_send_req_sync(vdomain->viommu, &req, sizeof(req));
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + if (!vdomain->nr_endpoints) {
>> + /*
>> + * This endpoint is the first to be attached to the domain.
>> + * Replay existing mappings (e.g. SW MSI).
>> + */
>> + ret = viommu_replay_mappings(vdomain);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + vdomain->nr_endpoints++;
>> + vdev->vdomain = vdomain;
>> +
>> + return 0;
>> +}
>> +
>> +static int viommu_map(struct iommu_domain *domain, unsigned long iova,
>> + phys_addr_t paddr, size_t size, int prot)
>> +{
>> + int ret;
>> + int flags;
>> + struct viommu_mapping *mapping;
>> + struct virtio_iommu_req_map map;
>> + struct viommu_domain *vdomain = to_viommu_domain(domain);
>> +
>> + flags = (prot & IOMMU_READ ? VIRTIO_IOMMU_MAP_F_READ : 0) |
>> + (prot & IOMMU_WRITE ? VIRTIO_IOMMU_MAP_F_WRITE : 0) |
>> + (prot & IOMMU_MMIO ? VIRTIO_IOMMU_MAP_F_MMIO : 0);
>> +
>> + mapping = viommu_add_mapping(vdomain, iova, paddr, size, flags);
>> + if (!mapping)
>> + return -ENOMEM;
>> +
>> + map = (struct virtio_iommu_req_map) {
>> + .head.type = VIRTIO_IOMMU_T_MAP,
>> + .domain = cpu_to_le32(vdomain->id),
>> + .virt_start = cpu_to_le64(iova),
>> + .phys_start = cpu_to_le64(paddr),
>> + .virt_end = cpu_to_le64(iova + size - 1),
>> + .flags = cpu_to_le32(flags),
>> + };
>> +
>> + if (!vdomain->nr_endpoints)
>> + return 0;
>> +
>> + ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
>> + if (ret)
>> + viommu_del_mappings(vdomain, iova, size);
>> +
>> + return ret;
>> +}
>> +
>> +static size_t viommu_unmap(struct iommu_domain *domain, unsigned long iova,
>> + size_t size)
>> +{
>> + int ret = 0;
>> + size_t unmapped;
>> + struct virtio_iommu_req_unmap unmap;
>> + struct viommu_domain *vdomain = to_viommu_domain(domain);
>> +
>> + unmapped = viommu_del_mappings(vdomain, iova, size);
>> + if (unmapped < size)
>> + return 0;
>> +
>> + /* Device already removed all mappings after detach. */
>> + if (!vdomain->nr_endpoints)
>> + return unmapped;
>> +
>> + unmap = (struct virtio_iommu_req_unmap) {
>> + .head.type = VIRTIO_IOMMU_T_UNMAP,
>> + .domain = cpu_to_le32(vdomain->id),
>> + .virt_start = cpu_to_le64(iova),
>> + .virt_end = cpu_to_le64(iova + unmapped - 1),
>> + };
>> +
>> + ret = viommu_add_req(vdomain->viommu, &unmap, sizeof(unmap));
>> + return ret ? 0 : unmapped;
>> +}
>> +
>> +static phys_addr_t viommu_iova_to_phys(struct iommu_domain *domain,
>> + dma_addr_t iova)
>> +{
>> + u64 paddr = 0;
>> + unsigned long flags;
>> + struct viommu_mapping *mapping;
>> + struct interval_tree_node *node;
>> + struct viommu_domain *vdomain = to_viommu_domain(domain);
>> +
>> + spin_lock_irqsave(&vdomain->mappings_lock, flags);
>> + node = interval_tree_iter_first(&vdomain->mappings, iova, iova);
>> + if (node) {
>> + mapping = container_of(node, struct viommu_mapping, iova);
>> + paddr = mapping->paddr + (iova - mapping->iova.start);
>> + }
>> + spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
>> +
>> + return paddr;
>> +}
>> +
>> +static void viommu_iotlb_sync(struct iommu_domain *domain)
>> +{
>> + struct viommu_domain *vdomain = to_viommu_domain(domain);
>> +
>> + viommu_sync_req(vdomain->viommu);
>> +}
>> +
>> +static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
>> +{
>> + struct iommu_resv_region *region;
>> + int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>> +
>> + region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH, prot,
>> + IOMMU_RESV_SW_MSI);
>> + if (!region)
>> + return;
>> +
>> + list_add_tail(®ion->list, head);
>> + iommu_dma_get_resv_regions(dev, head);
>> +}
>> +
>> +static void viommu_put_resv_regions(struct device *dev, struct list_head *head)
>> +{
>> + struct iommu_resv_region *entry, *next;
>> +
>> + list_for_each_entry_safe(entry, next, head, list)
>> + kfree(entry);
>> +}
>> +
>> +static struct iommu_ops viommu_ops;
>> +static struct virtio_driver virtio_iommu_drv;
>> +
>> +static int viommu_match_node(struct device *dev, void *data)
>> +{
>> + return dev->parent->fwnode == data;
>> +}
>> +
>> +static struct viommu_dev *viommu_get_by_fwnode(struct fwnode_handle *fwnode)
>> +{
>> + struct device *dev = driver_find_device(&virtio_iommu_drv.driver, NULL,
>> + fwnode, viommu_match_node);
>> + put_device(dev);
>> +
>> + return dev ? dev_to_virtio(dev)->priv : NULL;
>> +}
>> +
>> +static int viommu_add_device(struct device *dev)
>> +{
>> + int ret;
>> + struct iommu_group *group;
>> + struct viommu_endpoint *vdev;
>> + struct viommu_dev *viommu = NULL;
>> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>> +
>> + if (!fwspec || fwspec->ops != &viommu_ops)
>> + return -ENODEV;
>> +
>> + viommu = viommu_get_by_fwnode(fwspec->iommu_fwnode);
>> + if (!viommu)
>> + return -ENODEV;
>> +
>> + vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
>> + if (!vdev)
>> + return -ENOMEM;
>> +
>> + vdev->viommu = viommu;
>> + fwspec->iommu_priv = vdev;
>> +
>> + ret = iommu_device_link(&viommu->iommu, dev);
>> + if (ret)
>> + goto err_free_dev;
>> +
>> + /*
>> + * Last step creates a default domain and attaches to it. Everything
>> + * must be ready.
>> + */
>> + group = iommu_group_get_for_dev(dev);
>> + if (IS_ERR(group)) {
>> + ret = PTR_ERR(group);
>> + goto err_unlink_dev;
>> + }
>> +
>> + iommu_group_put(group);
>> +
>> + return PTR_ERR_OR_ZERO(group);
>> +
>> +err_unlink_dev:
>> + iommu_device_unlink(&viommu->iommu, dev);
>> +
>> +err_free_dev:
>> + kfree(vdev);
>> +
>> + return ret;
>> +}
>> +
>> +static void viommu_remove_device(struct device *dev)
>> +{
>> + struct viommu_endpoint *vdev;
>> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>> +
>> + if (!fwspec || fwspec->ops != &viommu_ops)
>> + return;
>> +
>> + vdev = fwspec->iommu_priv;
>> +
>> + iommu_group_remove_device(dev);
>> + iommu_device_unlink(&vdev->viommu->iommu, dev);
>> + kfree(vdev);
>> +}
>> +
>> +static struct iommu_group *viommu_device_group(struct device *dev)
>> +{
>> + if (dev_is_pci(dev))
>> + return pci_device_group(dev);
>> + else
>> + return generic_device_group(dev);
>> +}
>> +
>> +static int viommu_of_xlate(struct device *dev, struct of_phandle_args *args)
>> +{
>> + return iommu_fwspec_add_ids(dev, args->args, 1);
>> +}
>> +
>> +static struct iommu_ops viommu_ops = {
>> + .domain_alloc = viommu_domain_alloc,
>> + .domain_free = viommu_domain_free,
>> + .attach_dev = viommu_attach_dev,
>> + .map = viommu_map,
>> + .unmap = viommu_unmap,
>> + .iova_to_phys = viommu_iova_to_phys,
>> + .iotlb_sync = viommu_iotlb_sync,
>> + .add_device = viommu_add_device,
>> + .remove_device = viommu_remove_device,
>> + .device_group = viommu_device_group,
>> + .get_resv_regions = viommu_get_resv_regions,
>> + .put_resv_regions = viommu_put_resv_regions,
>> + .of_xlate = viommu_of_xlate,
>> +};
>> +
>> +static int viommu_init_vqs(struct viommu_dev *viommu)
>> +{
>> + struct virtio_device *vdev = dev_to_virtio(viommu->dev);
>> + const char *name = "request";
>> + void *ret;
>> +
>> + ret = virtio_find_single_vq(vdev, NULL, name);
>> + if (IS_ERR(ret)) {
>> + dev_err(viommu->dev, "cannot find VQ\n");
>> + return PTR_ERR(ret);
>> + }
>> +
>> + viommu->vqs[VIOMMU_REQUEST_VQ] = ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int viommu_probe(struct virtio_device *vdev)
>> +{
>> + struct device *parent_dev = vdev->dev.parent;
>> + struct viommu_dev *viommu = NULL;
>> + struct device *dev = &vdev->dev;
>> + u64 input_start = 0;
>> + u64 input_end = -1UL;
>> + int ret;
>> +
>> + if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
>> + return -ENODEV;
>
> I'm a bit confused about what will happen if this device
> happens to be behind an iommu itself.
>
> If we can't handle that, should we clear PLATFORM_IOMMU
> e.g. like the balloon does?
>
>
>> +
>> + viommu = devm_kzalloc(dev, sizeof(*viommu), GFP_KERNEL);
>> + if (!viommu)
>> + return -ENOMEM;
>> +
>> + spin_lock_init(&viommu->request_lock);
>> + ida_init(&viommu->domain_ids);
>> + viommu->dev = dev;
>> + viommu->vdev = vdev;
>> + INIT_LIST_HEAD(&viommu->requests);
>> +
>> + ret = viommu_init_vqs(viommu);
>> + if (ret)
>> + return ret;
>> +
>> + virtio_cread(vdev, struct virtio_iommu_config, page_size_mask,
>> + &viommu->pgsize_bitmap);
>> +
>> + if (!viommu->pgsize_bitmap) {
>> + ret = -EINVAL;
>> + goto err_free_vqs;
>> + }
>> +
>> + viommu->domain_bits = 32;
>> +
>> + /* Optional features */
>> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_INPUT_RANGE,
>> + struct virtio_iommu_config, input_range.start,
>> + &input_start);
>> +
>> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_INPUT_RANGE,
>> + struct virtio_iommu_config, input_range.end,
>> + &input_end);
>> +
>> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_DOMAIN_BITS,
>> + struct virtio_iommu_config, domain_bits,
>> + &viommu->domain_bits);
>> +
>> + viommu->geometry = (struct iommu_domain_geometry) {
>> + .aperture_start = input_start,
>> + .aperture_end = input_end,
>> + .force_aperture = true,
>> + };
>> +
>> + viommu_ops.pgsize_bitmap = viommu->pgsize_bitmap;
>> +
>> + virtio_device_ready(vdev);
>> +
>> + ret = iommu_device_sysfs_add(&viommu->iommu, dev, NULL, "%s",
>> + virtio_bus_name(vdev));
>> + if (ret)
>> + goto err_free_vqs;
>> +
>> + iommu_device_set_ops(&viommu->iommu, &viommu_ops);
>> + iommu_device_set_fwnode(&viommu->iommu, parent_dev->fwnode);
>> +
>> + iommu_device_register(&viommu->iommu);
>> +
>> +#ifdef CONFIG_PCI
>> + if (pci_bus_type.iommu_ops != &viommu_ops) {
>> + pci_request_acs();
>> + ret = bus_set_iommu(&pci_bus_type, &viommu_ops);
>> + if (ret)
>> + goto err_unregister;
>> + }
>> +#endif
>> +#ifdef CONFIG_ARM_AMBA
>> + if (amba_bustype.iommu_ops != &viommu_ops) {
>> + ret = bus_set_iommu(&amba_bustype, &viommu_ops);
>> + if (ret)
>> + goto err_unregister;
>> + }
>> +#endif
>> + if (platform_bus_type.iommu_ops != &viommu_ops) {
>> + ret = bus_set_iommu(&platform_bus_type, &viommu_ops);
>> + if (ret)
>> + goto err_unregister;
>> + }
>> +
>> + vdev->priv = viommu;
>> +
>> + dev_info(dev, "input address: %u bits\n",
>> + order_base_2(viommu->geometry.aperture_end));
>> + dev_info(dev, "page mask: %#llx\n", viommu->pgsize_bitmap);
>> +
>> + return 0;
>> +
>> +err_unregister:
>> + iommu_device_sysfs_remove(&viommu->iommu);
>> + iommu_device_unregister(&viommu->iommu);
>> +err_free_vqs:
>> + vdev->config->del_vqs(vdev);
>> +
>> + return ret;
>> +}
>> +
>> +static void viommu_remove(struct virtio_device *vdev)
>> +{
>> + struct viommu_dev *viommu = vdev->priv;
>> +
>> + iommu_device_sysfs_remove(&viommu->iommu);
>> + iommu_device_unregister(&viommu->iommu);
>> +
>> + /* Stop all virtqueues */
>> + vdev->config->reset(vdev);
>> + vdev->config->del_vqs(vdev);
>> +
>> + dev_info(&vdev->dev, "device removed\n");
>> +}
>> +
>> +static void viommu_config_changed(struct virtio_device *vdev)
>> +{
>> + dev_warn(&vdev->dev, "config changed\n");
>> +}
>> +
>> +static unsigned int features[] = {
>> + VIRTIO_IOMMU_F_MAP_UNMAP,
>> + VIRTIO_IOMMU_F_DOMAIN_BITS,
>> + VIRTIO_IOMMU_F_INPUT_RANGE,
>> +};
>> +
>> +static struct virtio_device_id id_table[] = {
>> + { VIRTIO_ID_IOMMU, VIRTIO_DEV_ANY_ID },
>> + { 0 },
>> +};
>> +
>> +static struct virtio_driver virtio_iommu_drv = {
>> + .driver.name = KBUILD_MODNAME,
>> + .driver.owner = THIS_MODULE,
>> + .id_table = id_table,
>> + .feature_table = features,
>> + .feature_table_size = ARRAY_SIZE(features),
>> + .probe = viommu_probe,
>> + .remove = viommu_remove,
>> + .config_changed = viommu_config_changed,
>> +};
>> +
>> +module_virtio_driver(virtio_iommu_drv);
>> +
>> +MODULE_DESCRIPTION("Virtio IOMMU driver");
>> +MODULE_AUTHOR("Jean-Philippe Brucker <jean-philippe.brucker@arm.com>");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
>> index 6d5c3b2d4f4d..cfe47c5d9a56 100644
>> --- a/include/uapi/linux/virtio_ids.h
>> +++ b/include/uapi/linux/virtio_ids.h
>> @@ -43,5 +43,6 @@
>> #define VIRTIO_ID_INPUT 18 /* virtio input */
>> #define VIRTIO_ID_VSOCK 19 /* virtio vsock transport */
>> #define VIRTIO_ID_CRYPTO 20 /* virtio crypto */
>> +#define VIRTIO_ID_IOMMU 23 /* virtio IOMMU */
>>
>> #endif /* _LINUX_VIRTIO_IDS_H */
>> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
>> new file mode 100644
>> index 000000000000..e808fc7fbe82
>> --- /dev/null
>> +++ b/include/uapi/linux/virtio_iommu.h
>> @@ -0,0 +1,101 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
>> +/*
>> + * Virtio-iommu definition v0.8
>> + *
>> + * Copyright (C) 2018 Arm Ltd.
>> + */
>> +#ifndef _UAPI_LINUX_VIRTIO_IOMMU_H
>> +#define _UAPI_LINUX_VIRTIO_IOMMU_H
>> +
>> +#include <linux/types.h>
>> +
>> +/* Feature bits */
>> +#define VIRTIO_IOMMU_F_INPUT_RANGE 0
>> +#define VIRTIO_IOMMU_F_DOMAIN_BITS 1
>> +#define VIRTIO_IOMMU_F_MAP_UNMAP 2
>> +#define VIRTIO_IOMMU_F_BYPASS 3
>> +
>> +struct virtio_iommu_config {
>> + /* Supported page sizes */
>> + __u64 page_size_mask;
>> + /* Supported IOVA range */
>> + struct virtio_iommu_range {
>
> I'd rather we moved the definition outside even though gcc allows it -
> some old userspace compilers might not.
>
>> + __u64 start;
>> + __u64 end;
>> + } input_range;
>> + /* Max domain ID size */
>> + __u8 domain_bits;
>
> Let's add explicit padding here as well?
>
>> +};
>> +
>> +/* Request types */
>> +#define VIRTIO_IOMMU_T_ATTACH 0x01
>> +#define VIRTIO_IOMMU_T_DETACH 0x02
>> +#define VIRTIO_IOMMU_T_MAP 0x03
>> +#define VIRTIO_IOMMU_T_UNMAP 0x04
>> +
>> +/* Status types */
>> +#define VIRTIO_IOMMU_S_OK 0x00
>> +#define VIRTIO_IOMMU_S_IOERR 0x01
>> +#define VIRTIO_IOMMU_S_UNSUPP 0x02
>> +#define VIRTIO_IOMMU_S_DEVERR 0x03
>> +#define VIRTIO_IOMMU_S_INVAL 0x04
>> +#define VIRTIO_IOMMU_S_RANGE 0x05
>> +#define VIRTIO_IOMMU_S_NOENT 0x06
>> +#define VIRTIO_IOMMU_S_FAULT 0x07
>> +
>> +struct virtio_iommu_req_head {
>> + __u8 type;
>> + __u8 reserved[3];
>> +};
>> +
>> +struct virtio_iommu_req_tail {
>> + __u8 status;
>> + __u8 reserved[3];
>> +};
>> +
>> +struct virtio_iommu_req_attach {
>> + struct virtio_iommu_req_head head;
>> + __le32 domain;
>> + __le32 endpoint;
>> + __u8 reserved[8];
>> + struct virtio_iommu_req_tail tail;
>> +};
>> +
>> +struct virtio_iommu_req_detach {
>> + struct virtio_iommu_req_head head;
>> + __le32 domain;
>> + __le32 endpoint;
>> + __u8 reserved[8];
>> + struct virtio_iommu_req_tail tail;
>> +};
>> +
>> +#define VIRTIO_IOMMU_MAP_F_READ (1 << 0)
>> +#define VIRTIO_IOMMU_MAP_F_WRITE (1 << 1)
>> +#define VIRTIO_IOMMU_MAP_F_EXEC (1 << 2)
>> +#define VIRTIO_IOMMU_MAP_F_MMIO (1 << 3)
>> +
>> +#define VIRTIO_IOMMU_MAP_F_MASK (VIRTIO_IOMMU_MAP_F_READ | \
>> + VIRTIO_IOMMU_MAP_F_WRITE | \
>> + VIRTIO_IOMMU_MAP_F_EXEC | \
>> + VIRTIO_IOMMU_MAP_F_MMIO)
>> +
>> +struct virtio_iommu_req_map {
>> + struct virtio_iommu_req_head head;
>> + __le32 domain;
>> + __le64 virt_start;
>> + __le64 virt_end;
>> + __le64 phys_start;
>> + __le32 flags;
>> + struct virtio_iommu_req_tail tail;
>> +};
>> +
>> +struct virtio_iommu_req_unmap {
>> + struct virtio_iommu_req_head head;
>> + __le32 domain;
>> + __le64 virt_start;
>> + __le64 virt_end;
>> + __u8 reserved[4];
>> + struct virtio_iommu_req_tail tail;
>> +};
>> +
>> +#endif
>> --
>> 2.19.1
^ permalink raw reply
* Re: [PATCH v3 6/7] iommu/virtio: Add probe request
From: Auger Eric @ 2018-11-08 14:48 UTC (permalink / raw)
To: Jean-Philippe Brucker, iommu, virtualization, devicetree
Cc: mark.rutland, peter.maydell, lorenzo.pieralisi, tnowicki, mst,
marc.zyngier, linux-pci, will.deacon, kvmarm, robh+dt,
robin.murphy, joro
In-Reply-To: <20181012145917.6840-7-jean-philippe.brucker@arm.com>
Hi Jean-Philippe,
On 10/12/18 4:59 PM, Jean-Philippe Brucker wrote:
> When the device offers the probe feature, send a probe request for each
> device managed by the IOMMU. Extract RESV_MEM information. When we
> encounter a MSI doorbell region, set it up as a IOMMU_RESV_MSI region.
> This will tell other subsystems that there is no need to map the MSI
> doorbell in the virtio-iommu, because MSIs bypass it.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> ---
> drivers/iommu/virtio-iommu.c | 147 ++++++++++++++++++++++++++++--
> include/uapi/linux/virtio_iommu.h | 39 ++++++++
> 2 files changed, 180 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 9fb38cd3b727..8eaf66770469 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -56,6 +56,7 @@ struct viommu_dev {
> struct iommu_domain_geometry geometry;
> u64 pgsize_bitmap;
> u8 domain_bits;
> + u32 probe_size;
> };
>
> struct viommu_mapping {
> @@ -77,8 +78,10 @@ struct viommu_domain {
> };
>
> struct viommu_endpoint {
> + struct device *dev;
> struct viommu_dev *viommu;
> struct viommu_domain *vdomain;
> + struct list_head resv_regions;
> };
>
> struct viommu_request {
> @@ -129,6 +132,9 @@ static off_t viommu_get_req_offset(struct viommu_dev *viommu,
> {
> size_t tail_size = sizeof(struct virtio_iommu_req_tail);
>
> + if (req->type == VIRTIO_IOMMU_T_PROBE)
> + return len - viommu->probe_size - tail_size;
> +
> return len - tail_size;
> }
>
> @@ -414,6 +420,101 @@ static int viommu_replay_mappings(struct viommu_domain *vdomain)
> return ret;
> }
>
> +static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
> + struct virtio_iommu_probe_resv_mem *mem,
> + size_t len)
> +{
> + struct iommu_resv_region *region = NULL;
> + unsigned long prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> +
> + u64 start = le64_to_cpu(mem->start);
> + u64 end = le64_to_cpu(mem->end);
> + size_t size = end - start + 1;
> +
> + if (len < sizeof(*mem))
> + return -EINVAL;
> +
> + switch (mem->subtype) {
> + default:
> + dev_warn(vdev->dev, "unknown resv mem subtype 0x%x\n",
> + mem->subtype);
> + /* Fall-through */
> + case VIRTIO_IOMMU_RESV_MEM_T_RESERVED:
> + region = iommu_alloc_resv_region(start, size, 0,
> + IOMMU_RESV_RESERVED);
> + break;
> + case VIRTIO_IOMMU_RESV_MEM_T_MSI:
> + region = iommu_alloc_resv_region(start, size, prot,
> + IOMMU_RESV_MSI);
> + break;
> + }
> +
> + list_add(&vdev->resv_regions, ®ion->list);
> + return 0;
> +}
> +
> +static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev)
> +{
> + int ret;
> + u16 type, len;
> + size_t cur = 0;
> + size_t probe_len;
> + struct virtio_iommu_req_probe *probe;
> + struct virtio_iommu_probe_property *prop;
> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> + struct viommu_endpoint *vdev = fwspec->iommu_priv;
> +
> + if (!fwspec->num_ids)
> + return -EINVAL;
> +
> + probe_len = sizeof(*probe) + viommu->probe_size +
> + sizeof(struct virtio_iommu_req_tail);
> + probe = kzalloc(probe_len, GFP_KERNEL);
> + if (!probe)
> + return -ENOMEM;
> +
> + probe->head.type = VIRTIO_IOMMU_T_PROBE;
> + /*
> + * For now, assume that properties of an endpoint that outputs multiple
> + * IDs are consistent. Only probe the first one.
> + */
> + probe->endpoint = cpu_to_le32(fwspec->ids[0]);
> +
> + ret = viommu_send_req_sync(viommu, probe, probe_len);
> + if (ret)
> + goto out_free;
> +
> + prop = (void *)probe->properties;
> + type = le16_to_cpu(prop->type) & VIRTIO_IOMMU_PROBE_T_MASK;
> +
> + while (type != VIRTIO_IOMMU_PROBE_T_NONE &&
> + cur < viommu->probe_size) {
> + len = le16_to_cpu(prop->length) + sizeof(*prop);
> +
> + switch (type) {
> + case VIRTIO_IOMMU_PROBE_T_RESV_MEM:
> + ret = viommu_add_resv_mem(vdev, (void *)prop, len);
> + break;
> + default:
> + dev_err(dev, "unknown viommu prop 0x%x\n", type);
> + }
> +
> + if (ret)
> + dev_err(dev, "failed to parse viommu prop 0x%x\n", type);
> +
> + cur += len;
> + if (cur >= viommu->probe_size)
> + break;
> +
> + prop = (void *)probe->properties + cur;
> + type = le16_to_cpu(prop->type) & VIRTIO_IOMMU_PROBE_T_MASK;
> + }
> +
> +out_free:
> + kfree(probe);
> + return ret;
> +}
> +
> /* IOMMU API */
>
> static struct iommu_domain *viommu_domain_alloc(unsigned type)
> @@ -636,15 +737,33 @@ static void viommu_iotlb_sync(struct iommu_domain *domain)
>
> static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
> {
> - struct iommu_resv_region *region;
> + struct iommu_resv_region *entry, *new_entry, *msi = NULL;
> + struct viommu_endpoint *vdev = dev->iommu_fwspec->iommu_priv;
> int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>
> - region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH, prot,
> - IOMMU_RESV_SW_MSI);
> - if (!region)
> - return;
> + list_for_each_entry(entry, &vdev->resv_regions, list) {
> + /*
> + * If the device registered a bypass MSI windows, use it.
> + * Otherwise add a software-mapped region
> + */
> + if (entry->type == IOMMU_RESV_MSI)
> + msi = entry;
> +
> + new_entry = kmemdup(entry, sizeof(*entry), GFP_KERNEL);
> + if (!new_entry)
> + return;
> + list_add_tail(&new_entry->list, head);
> + }
> +
> + if (!msi) {
> + msi = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
> + prot, IOMMU_RESV_SW_MSI);
> + if (!msi)
> + return;
> +
> + list_add_tail(&msi->list, head);
> + }
>
> - list_add_tail(®ion->list, head);
> iommu_dma_get_resv_regions(dev, head);
> }
>
> @@ -692,9 +811,18 @@ static int viommu_add_device(struct device *dev)
> if (!vdev)
> return -ENOMEM;
>
> + vdev->dev = dev;
> vdev->viommu = viommu;
> + INIT_LIST_HEAD(&vdev->resv_regions);
> fwspec->iommu_priv = vdev;
>
> + if (viommu->probe_size) {
> + /* Get additional information for this endpoint */
> + ret = viommu_probe_endpoint(viommu, dev);
> + if (ret)
> + return ret;
> + }
> +
> ret = iommu_device_link(&viommu->iommu, dev);
> if (ret)
> goto err_free_dev;
> @@ -717,6 +845,7 @@ static int viommu_add_device(struct device *dev)
> iommu_device_unlink(&viommu->iommu, dev);
>
> err_free_dev:
> + viommu_put_resv_regions(dev, &vdev->resv_regions);
> kfree(vdev);
>
> return ret;
> @@ -734,6 +863,7 @@ static void viommu_remove_device(struct device *dev)
>
> iommu_group_remove_device(dev);
> iommu_device_unlink(&vdev->viommu->iommu, dev);
> + viommu_put_resv_regions(dev, &vdev->resv_regions);
> kfree(vdev);
> }
>
> @@ -832,6 +962,10 @@ static int viommu_probe(struct virtio_device *vdev)
> struct virtio_iommu_config, domain_bits,
> &viommu->domain_bits);
>
> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_PROBE,
> + struct virtio_iommu_config, probe_size,
> + &viommu->probe_size);
> +
> viommu->geometry = (struct iommu_domain_geometry) {
> .aperture_start = input_start,
> .aperture_end = input_end,
> @@ -913,6 +1047,7 @@ static unsigned int features[] = {
> VIRTIO_IOMMU_F_MAP_UNMAP,
> VIRTIO_IOMMU_F_DOMAIN_BITS,
> VIRTIO_IOMMU_F_INPUT_RANGE,
> + VIRTIO_IOMMU_F_PROBE,
> };
>
> static struct virtio_device_id id_table[] = {
> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
> index e808fc7fbe82..feed74586bb0 100644
> --- a/include/uapi/linux/virtio_iommu.h
> +++ b/include/uapi/linux/virtio_iommu.h
> @@ -14,6 +14,7 @@
> #define VIRTIO_IOMMU_F_DOMAIN_BITS 1
> #define VIRTIO_IOMMU_F_MAP_UNMAP 2
> #define VIRTIO_IOMMU_F_BYPASS 3
> +#define VIRTIO_IOMMU_F_PROBE 4
>
> struct virtio_iommu_config {
> /* Supported page sizes */
> @@ -25,6 +26,9 @@ struct virtio_iommu_config {
> } input_range;
> /* Max domain ID size */
> __u8 domain_bits;
> + __u8 padding[3];
> + /* Probe buffer size */
> + __u32 probe_size;
> };
>
> /* Request types */
> @@ -32,6 +36,7 @@ struct virtio_iommu_config {
> #define VIRTIO_IOMMU_T_DETACH 0x02
> #define VIRTIO_IOMMU_T_MAP 0x03
> #define VIRTIO_IOMMU_T_UNMAP 0x04
> +#define VIRTIO_IOMMU_T_PROBE 0x05
>
> /* Status types */
> #define VIRTIO_IOMMU_S_OK 0x00
> @@ -98,4 +103,38 @@ struct virtio_iommu_req_unmap {
> struct virtio_iommu_req_tail tail;
> };
>
> +#define VIRTIO_IOMMU_PROBE_T_NONE 0
> +#define VIRTIO_IOMMU_PROBE_T_RESV_MEM 1
> +
> +#define VIRTIO_IOMMU_PROBE_T_MASK 0xfff
> +
> +struct virtio_iommu_probe_property {
> + __le16 type;
> + __le16 length;
the value[] field has disappeared but still is documented in the v0.8 spec.
Thanks
Eric
> +};
> +
> +#define VIRTIO_IOMMU_RESV_MEM_T_RESERVED 0
> +#define VIRTIO_IOMMU_RESV_MEM_T_MSI 1
> +
> +struct virtio_iommu_probe_resv_mem {
> + struct virtio_iommu_probe_property head;
> + __u8 subtype;
> + __u8 reserved[3];
> + __le64 start;
> + __le64 end;
> +};
> +
> +struct virtio_iommu_req_probe {
> + struct virtio_iommu_req_head head;
> + __le32 endpoint;
> + __u8 reserved[64];
> +
> + __u8 properties[];
> +
> + /*
> + * Tail follows the variable-length properties array. No padding,
> + * property lengths are all aligned on 8 bytes.
> + */
> +};
> +
> #endif
>
^ permalink raw reply
* Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support
From: Michael S. Tsirkin @ 2018-11-08 14:14 UTC (permalink / raw)
To: Jason Wang; +Cc: virtio-dev, netdev, linux-kernel, virtualization, wexu
In-Reply-To: <2d46a41e-bc00-276a-e19a-105c9dffc75a@redhat.com>
On Thu, Nov 08, 2018 at 04:18:25PM +0800, Jason Wang wrote:
>
> On 2018/11/8 上午9:38, Tiwei Bie wrote:
> > > > +
> > > > + if (vq->vq.num_free < descs_used) {
> > > > + pr_debug("Can't add buf len %i - avail = %i\n",
> > > > + descs_used, vq->vq.num_free);
> > > > + /* FIXME: for historical reasons, we force a notify here if
> > > > + * there are outgoing parts to the buffer. Presumably the
> > > > + * host should service the ring ASAP. */
> > > I don't think we have a reason to do this for packed ring.
> > > No historical baggage there, right?
> > Based on the original commit log, it seems that the notify here
> > is just an "optimization". But I don't quite understand what does
> > the "the heuristics which KVM uses" refer to. If it's safe to drop
> > this in packed ring, I'd like to do it.
>
>
> According to the commit log, it seems like a workaround of lguest networking
> backend. I agree to drop it, we should not have such burden.
>
> But we should notice that, with this removed, the compare between packed vs
> split is kind of unfair.
I don't think this ever triggers to be frank. When would it?
> Consider the removal of lguest support recently,
> maybe we can drop this for split ring as well?
>
> Thanks
If it's helpful, then for sure we can drop it for virtio 1.
Can you see any perf differences at all? With which device?
>
> >
> > commit 44653eae1407f79dff6f52fcf594ae84cb165ec4
> > Author: Rusty Russell<rusty@rustcorp.com.au>
> > Date: Fri Jul 25 12:06:04 2008 -0500
> >
> > virtio: don't always force a notification when ring is full
> > We force notification when the ring is full, even if the host has
> > indicated it doesn't want to know. This seemed like a good idea at
> > the time: if we fill the transmit ring, we should tell the host
> > immediately.
> > Unfortunately this logic also applies to the receiving ring, which is
> > refilled constantly. We should introduce real notification thesholds
> > to replace this logic. Meanwhile, removing the logic altogether breaks
> > the heuristics which KVM uses, so we use a hack: only notify if there are
> > outgoing parts of the new buffer.
> > Here are the number of exits with lguest's crappy network implementation:
> > Before:
> > network xmit 7859051 recv 236420
> > After:
> > network xmit 7858610 recv 118136
> > Signed-off-by: Rusty Russell<rusty@rustcorp.com.au>
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 72bf8bc09014..21d9a62767af 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueue *_vq,
> > if (vq->num_free < out + in) {
> > pr_debug("Can't add buf len %i - avail = %i\n",
> > out + in, vq->num_free);
> > - /* We notify*even if* VRING_USED_F_NO_NOTIFY is set here. */
> > - vq->notify(&vq->vq);
> > + /* FIXME: for historical reasons, we force a notify here if
> > + * there are outgoing parts to the buffer. Presumably the
> > + * host should service the ring ASAP. */
> > + if (out)
> > + vq->notify(&vq->vq);
> > END_USE(vq);
> > return -ENOSPC;
> > }
> >
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support
From: Tiwei Bie @ 2018-11-08 11:51 UTC (permalink / raw)
To: Jason Wang
Cc: virtio-dev, Michael S. Tsirkin, netdev, linux-kernel,
virtualization, wexu
In-Reply-To: <2d46a41e-bc00-276a-e19a-105c9dffc75a@redhat.com>
On Thu, Nov 08, 2018 at 04:18:25PM +0800, Jason Wang wrote:
>
> On 2018/11/8 上午9:38, Tiwei Bie wrote:
> > > > +
> > > > + if (vq->vq.num_free < descs_used) {
> > > > + pr_debug("Can't add buf len %i - avail = %i\n",
> > > > + descs_used, vq->vq.num_free);
> > > > + /* FIXME: for historical reasons, we force a notify here if
> > > > + * there are outgoing parts to the buffer. Presumably the
> > > > + * host should service the ring ASAP. */
> > > I don't think we have a reason to do this for packed ring.
> > > No historical baggage there, right?
> > Based on the original commit log, it seems that the notify here
> > is just an "optimization". But I don't quite understand what does
> > the "the heuristics which KVM uses" refer to. If it's safe to drop
> > this in packed ring, I'd like to do it.
>
>
> According to the commit log, it seems like a workaround of lguest networking
> backend.
Do you know why removing this notify in Tx will break "the
heuristics which KVM uses"? Or what does "the heuristics
which KVM uses" refer to?
> I agree to drop it, we should not have such burden.
>
> But we should notice that, with this removed, the compare between packed vs
> split is kind of unfair. Consider the removal of lguest support recently,
> maybe we can drop this for split ring as well?
>
> Thanks
>
>
> >
> > commit 44653eae1407f79dff6f52fcf594ae84cb165ec4
> > Author: Rusty Russell<rusty@rustcorp.com.au>
> > Date: Fri Jul 25 12:06:04 2008 -0500
> >
> > virtio: don't always force a notification when ring is full
> > We force notification when the ring is full, even if the host has
> > indicated it doesn't want to know. This seemed like a good idea at
> > the time: if we fill the transmit ring, we should tell the host
> > immediately.
> > Unfortunately this logic also applies to the receiving ring, which is
> > refilled constantly. We should introduce real notification thesholds
> > to replace this logic. Meanwhile, removing the logic altogether breaks
> > the heuristics which KVM uses, so we use a hack: only notify if there are
> > outgoing parts of the new buffer.
> > Here are the number of exits with lguest's crappy network implementation:
> > Before:
> > network xmit 7859051 recv 236420
> > After:
> > network xmit 7858610 recv 118136
> > Signed-off-by: Rusty Russell<rusty@rustcorp.com.au>
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 72bf8bc09014..21d9a62767af 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueue *_vq,
> > if (vq->num_free < out + in) {
> > pr_debug("Can't add buf len %i - avail = %i\n",
> > out + in, vq->num_free);
> > - /* We notify*even if* VRING_USED_F_NO_NOTIFY is set here. */
> > - vq->notify(&vq->vq);
> > + /* FIXME: for historical reasons, we force a notify here if
> > + * there are outgoing parts to the buffer. Presumably the
> > + * host should service the ring ASAP. */
> > + if (out)
> > + vq->notify(&vq->vq);
> > END_USE(vq);
> > return -ENOSPC;
> > }
> >
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 2/5] VSOCK: support fill data to mergeable rx buffer in host
From: Jason Wang @ 2018-11-08 8:20 UTC (permalink / raw)
To: jiangyiwen, stefanha, stefanha
Cc: netdev, Michael S. Tsirkin, kvm, virtualization
In-Reply-To: <5BE397CA.5060000@huawei.com>
On 2018/11/8 上午9:56, jiangyiwen wrote:
> On 2018/11/7 21:32, Jason Wang wrote:
>> On 2018/11/7 下午3:11, jiangyiwen wrote:
>>> On 2018/11/7 14:18, Jason Wang wrote:
>>>> On 2018/11/6 下午2:30, jiangyiwen wrote:
>>>>>> Seems duplicated with the one used by vhost-net.
>>>>>>
>>>>>> In packed virtqueue implementation, I plan to move this to vhost.c.
>>>>>>
>>>>> Yes, this code is full copied from vhost-net, if it can be packed into
>>>>> vhost.c, it would be great.
>>>>>
>>>> If you try to reuse vhost-net, you don't even need to care about this:)
>>>>
>>>> Thanks
>>>>
>>>>
>>>> .
>>>>
>>> Hi Jason,
>>>
>>> Thank your advice, I will consider your idea. But I don't know
>>> what's stefan's suggestion? It seems that he doesn't care much
>>> about this community.:(
>> I think not. He is probably busy these days.
>>
>>
>>> I still hope this community can have some vitality.
>>>
>> Let's wait for few more days for the possible comments from Stefan or Michael. But I do prefer to unify the virtio networking datapath which will be easier to be extended and maintained.
>>
>> Thanks
>>
>>
>> .
>>
> Hi Jason,
>
> Actually vsock use virtio-net as transport path should be a better idea,
> I will try to consider the new idea.
>
> Thanks,
> Yiwen.
>
Good to know this and thanks!
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support
From: Jason Wang @ 2018-11-08 8:18 UTC (permalink / raw)
To: Tiwei Bie, Michael S. Tsirkin
Cc: virtio-dev, netdev, linux-kernel, virtualization, wexu
In-Reply-To: <20181108013759.GA20591@debian>
On 2018/11/8 上午9:38, Tiwei Bie wrote:
>>> +
>>> + if (vq->vq.num_free < descs_used) {
>>> + pr_debug("Can't add buf len %i - avail = %i\n",
>>> + descs_used, vq->vq.num_free);
>>> + /* FIXME: for historical reasons, we force a notify here if
>>> + * there are outgoing parts to the buffer. Presumably the
>>> + * host should service the ring ASAP. */
>> I don't think we have a reason to do this for packed ring.
>> No historical baggage there, right?
> Based on the original commit log, it seems that the notify here
> is just an "optimization". But I don't quite understand what does
> the "the heuristics which KVM uses" refer to. If it's safe to drop
> this in packed ring, I'd like to do it.
According to the commit log, it seems like a workaround of lguest
networking backend. I agree to drop it, we should not have such burden.
But we should notice that, with this removed, the compare between packed
vs split is kind of unfair. Consider the removal of lguest support
recently, maybe we can drop this for split ring as well?
Thanks
>
> commit 44653eae1407f79dff6f52fcf594ae84cb165ec4
> Author: Rusty Russell<rusty@rustcorp.com.au>
> Date: Fri Jul 25 12:06:04 2008 -0500
>
> virtio: don't always force a notification when ring is full
>
> We force notification when the ring is full, even if the host has
> indicated it doesn't want to know. This seemed like a good idea at
> the time: if we fill the transmit ring, we should tell the host
> immediately.
>
> Unfortunately this logic also applies to the receiving ring, which is
> refilled constantly. We should introduce real notification thesholds
> to replace this logic. Meanwhile, removing the logic altogether breaks
> the heuristics which KVM uses, so we use a hack: only notify if there are
> outgoing parts of the new buffer.
>
> Here are the number of exits with lguest's crappy network implementation:
> Before:
> network xmit 7859051 recv 236420
> After:
> network xmit 7858610 recv 118136
>
> Signed-off-by: Rusty Russell<rusty@rustcorp.com.au>
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 72bf8bc09014..21d9a62767af 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueue *_vq,
> if (vq->num_free < out + in) {
> pr_debug("Can't add buf len %i - avail = %i\n",
> out + in, vq->num_free);
> - /* We notify*even if* VRING_USED_F_NO_NOTIFY is set here. */
> - vq->notify(&vq->vq);
> + /* FIXME: for historical reasons, we force a notify here if
> + * there are outgoing parts to the buffer. Presumably the
> + * host should service the ring ASAP. */
> + if (out)
> + vq->notify(&vq->vq);
> END_USE(vq);
> return -ENOSPC;
> }
>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 2/5] VSOCK: support fill data to mergeable rx buffer in host
From: jiangyiwen @ 2018-11-08 1:56 UTC (permalink / raw)
To: Jason Wang, stefanha, stefanha; +Cc: netdev, kvm, virtualization
In-Reply-To: <76732898-8e4f-910a-aac5-ea4b635a1c15@redhat.com>
On 2018/11/7 21:32, Jason Wang wrote:
>
> On 2018/11/7 下午3:11, jiangyiwen wrote:
>> On 2018/11/7 14:18, Jason Wang wrote:
>>> On 2018/11/6 下午2:30, jiangyiwen wrote:
>>>>> Seems duplicated with the one used by vhost-net.
>>>>>
>>>>> In packed virtqueue implementation, I plan to move this to vhost.c.
>>>>>
>>>> Yes, this code is full copied from vhost-net, if it can be packed into
>>>> vhost.c, it would be great.
>>>>
>>> If you try to reuse vhost-net, you don't even need to care about this:)
>>>
>>> Thanks
>>>
>>>
>>> .
>>>
>> Hi Jason,
>>
>> Thank your advice, I will consider your idea. But I don't know
>> what's stefan's suggestion? It seems that he doesn't care much
>> about this community.:(
>
>
> I think not. He is probably busy these days.
>
>
>>
>> I still hope this community can have some vitality.
>>
>
> Let's wait for few more days for the possible comments from Stefan or Michael. But I do prefer to unify the virtio networking datapath which will be easier to be extended and maintained.
>
> Thanks
>
>
> .
>
Hi Jason,
Actually vsock use virtio-net as transport path should be a better idea,
I will try to consider the new idea.
Thanks,
Yiwen.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support
From: Tiwei Bie @ 2018-11-08 1:38 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtio-dev, netdev, linux-kernel, virtualization, wexu
In-Reply-To: <20181107123933-mutt-send-email-mst@kernel.org>
On Wed, Nov 07, 2018 at 12:48:46PM -0500, Michael S. Tsirkin wrote:
> On Wed, Jul 11, 2018 at 10:27:09AM +0800, Tiwei Bie wrote:
> > This commit introduces the support (without EVENT_IDX) for
> > packed ring.
> >
> > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > ---
> > drivers/virtio/virtio_ring.c | 495 ++++++++++++++++++++++++++++++++++-
> > 1 file changed, 487 insertions(+), 8 deletions(-)
[...]
> >
> > +static void vring_unmap_state_packed(const struct vring_virtqueue *vq,
> > + struct vring_desc_state_packed *state)
> > +{
> > + u16 flags;
> > +
> > + if (!vring_use_dma_api(vq->vq.vdev))
> > + return;
> > +
> > + flags = state->flags;
> > +
> > + if (flags & VRING_DESC_F_INDIRECT) {
> > + dma_unmap_single(vring_dma_dev(vq),
> > + state->addr, state->len,
> > + (flags & VRING_DESC_F_WRITE) ?
> > + DMA_FROM_DEVICE : DMA_TO_DEVICE);
> > + } else {
> > + dma_unmap_page(vring_dma_dev(vq),
> > + state->addr, state->len,
> > + (flags & VRING_DESC_F_WRITE) ?
> > + DMA_FROM_DEVICE : DMA_TO_DEVICE);
> > + }
> > +}
> > +
> > +static void vring_unmap_desc_packed(const struct vring_virtqueue *vq,
> > + struct vring_packed_desc *desc)
> > +{
> > + u16 flags;
> > +
> > + if (!vring_use_dma_api(vq->vq.vdev))
> > + return;
> > +
> > + flags = virtio16_to_cpu(vq->vq.vdev, desc->flags);
>
> BTW this stuff is only used on error etc. Is there a way to
> reuse vring_unmap_state_packed?
It's also used by the INDIRECT path. We don't allocate desc
state for INDIRECT descriptors to save DMA addr/len etc.
>
> > +
> > + if (flags & VRING_DESC_F_INDIRECT) {
> > + dma_unmap_single(vring_dma_dev(vq),
> > + virtio64_to_cpu(vq->vq.vdev, desc->addr),
> > + virtio32_to_cpu(vq->vq.vdev, desc->len),
> > + (flags & VRING_DESC_F_WRITE) ?
> > + DMA_FROM_DEVICE : DMA_TO_DEVICE);
> > + } else {
> > + dma_unmap_page(vring_dma_dev(vq),
> > + virtio64_to_cpu(vq->vq.vdev, desc->addr),
> > + virtio32_to_cpu(vq->vq.vdev, desc->len),
> > + (flags & VRING_DESC_F_WRITE) ?
> > + DMA_FROM_DEVICE : DMA_TO_DEVICE);
> > + }
> > +}
[...]
> > @@ -766,47 +840,449 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > void *ctx,
> > gfp_t gfp)
> > {
> > + struct vring_virtqueue *vq = to_vvq(_vq);
> > + struct vring_packed_desc *desc;
> > + struct scatterlist *sg;
> > + unsigned int i, n, descs_used, uninitialized_var(prev), err_idx;
> > + __virtio16 uninitialized_var(head_flags), flags;
> > + u16 head, avail_wrap_counter, id, curr;
> > + bool indirect;
> > +
> > + START_USE(vq);
> > +
> > + BUG_ON(data == NULL);
> > + BUG_ON(ctx && vq->indirect);
> > +
> > + if (unlikely(vq->broken)) {
> > + END_USE(vq);
> > + return -EIO;
> > + }
> > +
> > +#ifdef DEBUG
> > + {
> > + ktime_t now = ktime_get();
> > +
> > + /* No kick or get, with .1 second between? Warn. */
> > + if (vq->last_add_time_valid)
> > + WARN_ON(ktime_to_ms(ktime_sub(now, vq->last_add_time))
> > + > 100);
> > + vq->last_add_time = now;
> > + vq->last_add_time_valid = true;
> > + }
> > +#endif
> > +
> > + BUG_ON(total_sg == 0);
> > +
> > + head = vq->next_avail_idx;
> > + avail_wrap_counter = vq->avail_wrap_counter;
> > +
> > + if (virtqueue_use_indirect(_vq, total_sg))
> > + desc = alloc_indirect_packed(_vq, total_sg, gfp);
> > + else {
> > + desc = NULL;
> > + WARN_ON_ONCE(total_sg > vq->vring_packed.num && !vq->indirect);
> > + }
> > +
> > + if (desc) {
> > + /* Use a single buffer which doesn't continue */
> > + indirect = true;
> > + /* Set up rest to use this indirect table. */
> > + i = 0;
> > + descs_used = 1;
> > + } else {
> > + indirect = false;
> > + desc = vq->vring_packed.desc;
> > + i = head;
> > + descs_used = total_sg;
> > + }
> > +
> > + if (vq->vq.num_free < descs_used) {
> > + pr_debug("Can't add buf len %i - avail = %i\n",
> > + descs_used, vq->vq.num_free);
> > + /* FIXME: for historical reasons, we force a notify here if
> > + * there are outgoing parts to the buffer. Presumably the
> > + * host should service the ring ASAP. */
>
> I don't think we have a reason to do this for packed ring.
> No historical baggage there, right?
Based on the original commit log, it seems that the notify here
is just an "optimization". But I don't quite understand what does
the "the heuristics which KVM uses" refer to. If it's safe to drop
this in packed ring, I'd like to do it.
commit 44653eae1407f79dff6f52fcf594ae84cb165ec4
Author: Rusty Russell <rusty@rustcorp.com.au>
Date: Fri Jul 25 12:06:04 2008 -0500
virtio: don't always force a notification when ring is full
We force notification when the ring is full, even if the host has
indicated it doesn't want to know. This seemed like a good idea at
the time: if we fill the transmit ring, we should tell the host
immediately.
Unfortunately this logic also applies to the receiving ring, which is
refilled constantly. We should introduce real notification thesholds
to replace this logic. Meanwhile, removing the logic altogether breaks
the heuristics which KVM uses, so we use a hack: only notify if there are
outgoing parts of the new buffer.
Here are the number of exits with lguest's crappy network implementation:
Before:
network xmit 7859051 recv 236420
After:
network xmit 7858610 recv 118136
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 72bf8bc09014..21d9a62767af 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueue *_vq,
if (vq->num_free < out + in) {
pr_debug("Can't add buf len %i - avail = %i\n",
out + in, vq->num_free);
- /* We notify *even if* VRING_USED_F_NO_NOTIFY is set here. */
- vq->notify(&vq->vq);
+ /* FIXME: for historical reasons, we force a notify here if
+ * there are outgoing parts to the buffer. Presumably the
+ * host should service the ring ASAP. */
+ if (out)
+ vq->notify(&vq->vq);
END_USE(vq);
return -ENOSPC;
}
>
> > + if (out_sgs)
> > + vq->notify(&vq->vq);
> > + if (indirect)
> > + kfree(desc);
> > + END_USE(vq);
> > + return -ENOSPC;
> > + }
> > +
[...]
^ permalink raw reply related
* [PATCH][drm-next] drm/virtio: fix memory leak of vfpriv on error return path
From: Colin King @ 2018-11-07 20:31 UTC (permalink / raw)
To: David Airlie, Gerd Hoffmann, dri-devel, virtualization
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The allocation for vfpriv is being leaked on an error return path,
fix this by kfree'ing it before returning.
Detected by CoverityScan, CID#1475380 ("Resource Leak")
Fixes: 6a37c49a94a9 ("drm/virtio: Handle context ID allocation errors")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/gpu/drm/virtio/virtgpu_kms.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index bf609dcae224..f4f995639ed1 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -266,8 +266,10 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
get_task_comm(dbgname, current);
id = virtio_gpu_context_create(vgdev, strlen(dbgname), dbgname);
- if (id < 0)
+ if (id < 0) {
+ kfree(vfpriv);
return id;
+ }
vfpriv->ctx_id = id;
file->driver_priv = vfpriv;
--
2.19.1
^ permalink raw reply related
* Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support
From: Michael S. Tsirkin @ 2018-11-07 17:48 UTC (permalink / raw)
To: Tiwei Bie; +Cc: virtio-dev, netdev, linux-kernel, virtualization, wexu
In-Reply-To: <20180711022711.7090-4-tiwei.bie@intel.com>
On Wed, Jul 11, 2018 at 10:27:09AM +0800, Tiwei Bie wrote:
> This commit introduces the support (without EVENT_IDX) for
> packed ring.
>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
> drivers/virtio/virtio_ring.c | 495 ++++++++++++++++++++++++++++++++++-
> 1 file changed, 487 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index c4f8abc7445a..f317b485ba54 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -55,12 +55,21 @@
> #define END_USE(vq)
> #endif
>
> +#define _VRING_DESC_F_AVAIL(b) ((__u16)(b) << 7)
> +#define _VRING_DESC_F_USED(b) ((__u16)(b) << 15)
> +
> struct vring_desc_state {
> void *data; /* Data for callback. */
> struct vring_desc *indir_desc; /* Indirect descriptor, if any. */
> };
>
> struct vring_desc_state_packed {
> + void *data; /* Data for callback. */
> + struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */
> + int num; /* Descriptor list length. */
> + dma_addr_t addr; /* Buffer DMA addr. */
> + u32 len; /* Buffer length. */
> + u16 flags; /* Descriptor flags. */
> int next; /* The next desc state. */
> };
>
> @@ -660,7 +669,6 @@ static bool virtqueue_poll_split(struct virtqueue *_vq, unsigned last_used_idx)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
>
> - virtio_mb(vq->weak_barriers);
> return (u16)last_used_idx != virtio16_to_cpu(_vq->vdev, vq->vring.used->idx);
> }
>
> @@ -757,6 +765,72 @@ static inline unsigned vring_size_packed(unsigned int num, unsigned long align)
> & ~(align - 1)) + sizeof(struct vring_packed_desc_event) * 2;
> }
>
> +static void vring_unmap_state_packed(const struct vring_virtqueue *vq,
> + struct vring_desc_state_packed *state)
> +{
> + u16 flags;
> +
> + if (!vring_use_dma_api(vq->vq.vdev))
> + return;
> +
> + flags = state->flags;
> +
> + if (flags & VRING_DESC_F_INDIRECT) {
> + dma_unmap_single(vring_dma_dev(vq),
> + state->addr, state->len,
> + (flags & VRING_DESC_F_WRITE) ?
> + DMA_FROM_DEVICE : DMA_TO_DEVICE);
> + } else {
> + dma_unmap_page(vring_dma_dev(vq),
> + state->addr, state->len,
> + (flags & VRING_DESC_F_WRITE) ?
> + DMA_FROM_DEVICE : DMA_TO_DEVICE);
> + }
> +}
> +
> +static void vring_unmap_desc_packed(const struct vring_virtqueue *vq,
> + struct vring_packed_desc *desc)
> +{
> + u16 flags;
> +
> + if (!vring_use_dma_api(vq->vq.vdev))
> + return;
> +
> + flags = virtio16_to_cpu(vq->vq.vdev, desc->flags);
BTW this stuff is only used on error etc. Is there a way to
reuse vring_unmap_state_packed?
> +
> + if (flags & VRING_DESC_F_INDIRECT) {
> + dma_unmap_single(vring_dma_dev(vq),
> + virtio64_to_cpu(vq->vq.vdev, desc->addr),
> + virtio32_to_cpu(vq->vq.vdev, desc->len),
> + (flags & VRING_DESC_F_WRITE) ?
> + DMA_FROM_DEVICE : DMA_TO_DEVICE);
> + } else {
> + dma_unmap_page(vring_dma_dev(vq),
> + virtio64_to_cpu(vq->vq.vdev, desc->addr),
> + virtio32_to_cpu(vq->vq.vdev, desc->len),
> + (flags & VRING_DESC_F_WRITE) ?
> + DMA_FROM_DEVICE : DMA_TO_DEVICE);
> + }
> +}
> +
> +static struct vring_packed_desc *alloc_indirect_packed(struct virtqueue *_vq,
> + unsigned int total_sg,
> + gfp_t gfp)
> +{
> + struct vring_packed_desc *desc;
> +
> + /*
> + * We require lowmem mappings for the descriptors because
> + * otherwise virt_to_phys will give us bogus addresses in the
> + * virtqueue.
> + */
> + gfp &= ~__GFP_HIGHMEM;
> +
> + desc = kmalloc(total_sg * sizeof(struct vring_packed_desc), gfp);
> +
> + return desc;
> +}
> +
> static inline int virtqueue_add_packed(struct virtqueue *_vq,
> struct scatterlist *sgs[],
> unsigned int total_sg,
> @@ -766,47 +840,449 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> void *ctx,
> gfp_t gfp)
> {
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + struct vring_packed_desc *desc;
> + struct scatterlist *sg;
> + unsigned int i, n, descs_used, uninitialized_var(prev), err_idx;
> + __virtio16 uninitialized_var(head_flags), flags;
> + u16 head, avail_wrap_counter, id, curr;
> + bool indirect;
> +
> + START_USE(vq);
> +
> + BUG_ON(data == NULL);
> + BUG_ON(ctx && vq->indirect);
> +
> + if (unlikely(vq->broken)) {
> + END_USE(vq);
> + return -EIO;
> + }
> +
> +#ifdef DEBUG
> + {
> + ktime_t now = ktime_get();
> +
> + /* No kick or get, with .1 second between? Warn. */
> + if (vq->last_add_time_valid)
> + WARN_ON(ktime_to_ms(ktime_sub(now, vq->last_add_time))
> + > 100);
> + vq->last_add_time = now;
> + vq->last_add_time_valid = true;
> + }
> +#endif
> +
> + BUG_ON(total_sg == 0);
> +
> + head = vq->next_avail_idx;
> + avail_wrap_counter = vq->avail_wrap_counter;
> +
> + if (virtqueue_use_indirect(_vq, total_sg))
> + desc = alloc_indirect_packed(_vq, total_sg, gfp);
> + else {
> + desc = NULL;
> + WARN_ON_ONCE(total_sg > vq->vring_packed.num && !vq->indirect);
> + }
> +
> + if (desc) {
> + /* Use a single buffer which doesn't continue */
> + indirect = true;
> + /* Set up rest to use this indirect table. */
> + i = 0;
> + descs_used = 1;
> + } else {
> + indirect = false;
> + desc = vq->vring_packed.desc;
> + i = head;
> + descs_used = total_sg;
> + }
> +
> + if (vq->vq.num_free < descs_used) {
> + pr_debug("Can't add buf len %i - avail = %i\n",
> + descs_used, vq->vq.num_free);
> + /* FIXME: for historical reasons, we force a notify here if
> + * there are outgoing parts to the buffer. Presumably the
> + * host should service the ring ASAP. */
I don't think we have a reason to do this for packed ring.
No historical baggage there, right?
> + if (out_sgs)
> + vq->notify(&vq->vq);
> + if (indirect)
> + kfree(desc);
> + END_USE(vq);
> + return -ENOSPC;
> + }
> +
> + id = vq->free_head;
> + BUG_ON(id == vq->vring_packed.num);
> +
> + curr = id;
> + for (n = 0; n < out_sgs + in_sgs; n++) {
> + for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> + dma_addr_t addr = vring_map_one_sg(vq, sg, n < out_sgs ?
> + DMA_TO_DEVICE : DMA_FROM_DEVICE);
> + if (vring_mapping_error(vq, addr))
> + goto unmap_release;
> +
> + flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT |
> + (n < out_sgs ? 0 : VRING_DESC_F_WRITE) |
> + _VRING_DESC_F_AVAIL(vq->avail_wrap_counter) |
> + _VRING_DESC_F_USED(!vq->avail_wrap_counter));
> + if (!indirect && i == head)
> + head_flags = flags;
> + else
> + desc[i].flags = flags;
> +
> + desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
> + desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length);
> + i++;
> + if (!indirect) {
> + if (vring_use_dma_api(_vq->vdev)) {
> + vq->desc_state_packed[curr].addr = addr;
> + vq->desc_state_packed[curr].len =
> + sg->length;
> + vq->desc_state_packed[curr].flags =
> + virtio16_to_cpu(_vq->vdev,
> + flags);
> + }
> + curr = vq->desc_state_packed[curr].next;
> +
> + if (i >= vq->vring_packed.num) {
> + i = 0;
> + vq->avail_wrap_counter ^= 1;
> + }
> + }
> + }
> + }
> +
> + prev = (i > 0 ? i : vq->vring_packed.num) - 1;
> + desc[prev].id = cpu_to_virtio16(_vq->vdev, id);
> +
> + /* Last one doesn't continue. */
> + if (total_sg == 1)
> + head_flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
> + else
> + desc[prev].flags &= cpu_to_virtio16(_vq->vdev,
> + ~VRING_DESC_F_NEXT);
> +
> + if (indirect) {
> + /* Now that the indirect table is filled in, map it. */
> + dma_addr_t addr = vring_map_single(
> + vq, desc, total_sg * sizeof(struct vring_packed_desc),
> + DMA_TO_DEVICE);
> + if (vring_mapping_error(vq, addr))
> + goto unmap_release;
> +
> + head_flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_INDIRECT |
> + _VRING_DESC_F_AVAIL(avail_wrap_counter) |
> + _VRING_DESC_F_USED(!avail_wrap_counter));
> + vq->vring_packed.desc[head].addr = cpu_to_virtio64(_vq->vdev,
> + addr);
> + vq->vring_packed.desc[head].len = cpu_to_virtio32(_vq->vdev,
> + total_sg * sizeof(struct vring_packed_desc));
> + vq->vring_packed.desc[head].id = cpu_to_virtio16(_vq->vdev, id);
> +
> + if (vring_use_dma_api(_vq->vdev)) {
> + vq->desc_state_packed[id].addr = addr;
> + vq->desc_state_packed[id].len = total_sg *
> + sizeof(struct vring_packed_desc);
> + vq->desc_state_packed[id].flags =
> + virtio16_to_cpu(_vq->vdev, head_flags);
> + }
> + }
> +
> + /* We're using some buffers from the free list. */
> + vq->vq.num_free -= descs_used;
> +
> + /* Update free pointer */
> + if (indirect) {
> + n = head + 1;
> + if (n >= vq->vring_packed.num) {
> + n = 0;
> + vq->avail_wrap_counter ^= 1;
> + }
> + vq->next_avail_idx = n;
> + vq->free_head = vq->desc_state_packed[id].next;
> + } else {
> + vq->next_avail_idx = i;
> + vq->free_head = curr;
> + }
> +
> + /* Store token and indirect buffer state. */
> + vq->desc_state_packed[id].num = descs_used;
> + vq->desc_state_packed[id].data = data;
> + if (indirect)
> + vq->desc_state_packed[id].indir_desc = desc;
> + else
> + vq->desc_state_packed[id].indir_desc = ctx;
> +
> + /* A driver MUST NOT make the first descriptor in the list
> + * available before all subsequent descriptors comprising
> + * the list are made available. */
> + virtio_wmb(vq->weak_barriers);
> + vq->vring_packed.desc[head].flags = head_flags;
> + vq->num_added += descs_used;
> +
> + pr_debug("Added buffer head %i to %p\n", head, vq);
> + END_USE(vq);
> +
> + return 0;
> +
> +unmap_release:
> + err_idx = i;
> + i = head;
> +
> + for (n = 0; n < total_sg; n++) {
> + if (i == err_idx)
> + break;
> + vring_unmap_desc_packed(vq, &desc[i]);
> + i++;
> + if (!indirect && i >= vq->vring_packed.num)
> + i = 0;
> + }
> +
> + vq->avail_wrap_counter = avail_wrap_counter;
> +
> + if (indirect)
> + kfree(desc);
> +
> + END_USE(vq);
> return -EIO;
> }
>
> static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq)
> {
> - return false;
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + u16 flags;
> + bool needs_kick;
> + u32 snapshot;
> +
> + START_USE(vq);
> + /* We need to expose the new flags value before checking notification
> + * suppressions. */
> + virtio_mb(vq->weak_barriers);
> +
> + snapshot = READ_ONCE(*(u32 *)vq->vring_packed.device);
> + flags = virtio16_to_cpu(_vq->vdev, (__virtio16)(snapshot >> 16)) & 0x3;
> +
> +#ifdef DEBUG
> + if (vq->last_add_time_valid) {
> + WARN_ON(ktime_to_ms(ktime_sub(ktime_get(),
> + vq->last_add_time)) > 100);
> + }
> + vq->last_add_time_valid = false;
> +#endif
> +
> + needs_kick = (flags != VRING_EVENT_F_DISABLE);
> + END_USE(vq);
> + return needs_kick;
> +}
> +
> +static void detach_buf_packed(struct vring_virtqueue *vq,
> + unsigned int id, void **ctx)
> +{
> + struct vring_desc_state_packed *state = NULL;
> + struct vring_packed_desc *desc;
> + unsigned int curr, i;
> +
> + /* Clear data ptr. */
> + vq->desc_state_packed[id].data = NULL;
> +
> + curr = id;
> + for (i = 0; i < vq->desc_state_packed[id].num; i++) {
> + state = &vq->desc_state_packed[curr];
> + vring_unmap_state_packed(vq, state);
> + curr = state->next;
> + }
> +
> + BUG_ON(state == NULL);
> + vq->vq.num_free += vq->desc_state_packed[id].num;
> + state->next = vq->free_head;
> + vq->free_head = id;
> +
> + if (vq->indirect) {
> + u32 len;
> +
> + /* Free the indirect table, if any, now that it's unmapped. */
> + desc = vq->desc_state_packed[id].indir_desc;
> + if (!desc)
> + return;
> +
> + if (vring_use_dma_api(vq->vq.vdev)) {
> + len = vq->desc_state_packed[id].len;
> + for (i = 0; i < len / sizeof(struct vring_packed_desc);
> + i++)
> + vring_unmap_desc_packed(vq, &desc[i]);
> + }
> + kfree(desc);
> + vq->desc_state_packed[id].indir_desc = NULL;
> + } else if (ctx) {
> + *ctx = vq->desc_state_packed[id].indir_desc;
> + }
> +}
> +
> +static inline bool is_used_desc_packed(const struct vring_virtqueue *vq,
> + u16 idx, bool used_wrap_counter)
> +{
> + u16 flags;
> + bool avail, used;
> +
> + flags = virtio16_to_cpu(vq->vq.vdev,
> + vq->vring_packed.desc[idx].flags);
> + avail = !!(flags & VRING_DESC_F_AVAIL);
> + used = !!(flags & VRING_DESC_F_USED);
> +
> + return avail == used && used == used_wrap_counter;
> }
>
> static inline bool more_used_packed(const struct vring_virtqueue *vq)
> {
> - return false;
> + return is_used_desc_packed(vq, vq->last_used_idx,
> + vq->used_wrap_counter);
> }
>
> static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
> unsigned int *len,
> void **ctx)
> {
> - return NULL;
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + u16 last_used, id;
> + void *ret;
> +
> + START_USE(vq);
> +
> + if (unlikely(vq->broken)) {
> + END_USE(vq);
> + return NULL;
> + }
> +
> + if (!more_used_packed(vq)) {
> + pr_debug("No more buffers in queue\n");
> + END_USE(vq);
> + return NULL;
> + }
> +
> + /* Only get used elements after they have been exposed by host. */
> + virtio_rmb(vq->weak_barriers);
> +
> + last_used = vq->last_used_idx;
> + id = virtio16_to_cpu(_vq->vdev, vq->vring_packed.desc[last_used].id);
> + *len = virtio32_to_cpu(_vq->vdev, vq->vring_packed.desc[last_used].len);
> +
> + if (unlikely(id >= vq->vring_packed.num)) {
> + BAD_RING(vq, "id %u out of range\n", id);
> + return NULL;
> + }
> + if (unlikely(!vq->desc_state_packed[id].data)) {
> + BAD_RING(vq, "id %u is not a head!\n", id);
> + return NULL;
> + }
> +
> + vq->last_used_idx += vq->desc_state_packed[id].num;
> + if (vq->last_used_idx >= vq->vring_packed.num) {
> + vq->last_used_idx -= vq->vring_packed.num;
> + vq->used_wrap_counter ^= 1;
> + }
> +
> + /* detach_buf_packed clears data, so grab it now. */
> + ret = vq->desc_state_packed[id].data;
> + detach_buf_packed(vq, id, ctx);
> +
> +#ifdef DEBUG
> + vq->last_add_time_valid = false;
> +#endif
> +
> + END_USE(vq);
> + return ret;
> }
>
> static void virtqueue_disable_cb_packed(struct virtqueue *_vq)
> {
> + struct vring_virtqueue *vq = to_vvq(_vq);
> +
> + if (vq->event_flags_shadow != VRING_EVENT_F_DISABLE) {
> + vq->event_flags_shadow = VRING_EVENT_F_DISABLE;
> + vq->vring_packed.driver->flags = cpu_to_virtio16(_vq->vdev,
> + vq->event_flags_shadow);
> + }
> }
>
> static unsigned virtqueue_enable_cb_prepare_packed(struct virtqueue *_vq)
> {
> - return 0;
> + struct vring_virtqueue *vq = to_vvq(_vq);
> +
> + START_USE(vq);
> +
> + /* We optimistically turn back on interrupts, then check if there was
> + * more to do. */
> +
> + if (vq->event_flags_shadow == VRING_EVENT_F_DISABLE) {
> + vq->event_flags_shadow = VRING_EVENT_F_ENABLE;
> + vq->vring_packed.driver->flags = cpu_to_virtio16(_vq->vdev,
> + vq->event_flags_shadow);
> + }
> +
> + END_USE(vq);
> + return vq->last_used_idx | ((u16)vq->used_wrap_counter << 15);
> }
>
> -static bool virtqueue_poll_packed(struct virtqueue *_vq, unsigned last_used_idx)
> +static bool virtqueue_poll_packed(struct virtqueue *_vq, unsigned off_wrap)
> {
> - return false;
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + bool wrap_counter;
> + u16 used_idx;
> +
> + wrap_counter = off_wrap >> 15;
> + used_idx = off_wrap & ~(1 << 15);
> +
> + return is_used_desc_packed(vq, used_idx, wrap_counter);
> }
>
> static bool virtqueue_enable_cb_delayed_packed(struct virtqueue *_vq)
> {
> - return false;
> + struct vring_virtqueue *vq = to_vvq(_vq);
> +
> + START_USE(vq);
> +
> + /* We optimistically turn back on interrupts, then check if there was
> + * more to do. */
> +
> + if (vq->event_flags_shadow == VRING_EVENT_F_DISABLE) {
> + vq->event_flags_shadow = VRING_EVENT_F_ENABLE;
> + vq->vring_packed.driver->flags = cpu_to_virtio16(_vq->vdev,
> + vq->event_flags_shadow);
> + /* We need to enable interrupts first before re-checking
> + * for more used buffers. */
> + virtio_mb(vq->weak_barriers);
> + }
> +
> + if (more_used_packed(vq)) {
> + END_USE(vq);
> + return false;
> + }
> +
> + END_USE(vq);
> + return true;
> }
>
> static void *virtqueue_detach_unused_buf_packed(struct virtqueue *_vq)
> {
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + unsigned int i;
> + void *buf;
> +
> + START_USE(vq);
> +
> + for (i = 0; i < vq->vring_packed.num; i++) {
> + if (!vq->desc_state_packed[i].data)
> + continue;
> + /* detach_buf clears data, so grab it now. */
> + buf = vq->desc_state_packed[i].data;
> + detach_buf_packed(vq, i, NULL);
> + END_USE(vq);
> + return buf;
> + }
> + /* That should have freed everything. */
> + BUG_ON(vq->vq.num_free != vq->vring_packed.num);
> +
> + END_USE(vq);
> return NULL;
> }
>
> @@ -1083,6 +1559,9 @@ bool virtqueue_poll(struct virtqueue *_vq, unsigned last_used_idx)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
>
> + /* We need to enable interrupts first before re-checking
> + * for more used buffers. */
> + virtio_mb(vq->weak_barriers);
> return vq->packed ? virtqueue_poll_packed(_vq, last_used_idx) :
> virtqueue_poll_split(_vq, last_used_idx);
> }
> --
> 2.18.0
^ permalink raw reply
* Re: [PATCH 2/5] VSOCK: support fill data to mergeable rx buffer in host
From: Jason Wang @ 2018-11-07 13:32 UTC (permalink / raw)
To: jiangyiwen, stefanha, stefanha; +Cc: netdev, kvm, virtualization
In-Reply-To: <5BE2903C.50608@huawei.com>
On 2018/11/7 下午3:11, jiangyiwen wrote:
> On 2018/11/7 14:18, Jason Wang wrote:
>> On 2018/11/6 下午2:30, jiangyiwen wrote:
>>>> Seems duplicated with the one used by vhost-net.
>>>>
>>>> In packed virtqueue implementation, I plan to move this to vhost.c.
>>>>
>>> Yes, this code is full copied from vhost-net, if it can be packed into
>>> vhost.c, it would be great.
>>>
>> If you try to reuse vhost-net, you don't even need to care about this:)
>>
>> Thanks
>>
>>
>> .
>>
> Hi Jason,
>
> Thank your advice, I will consider your idea. But I don't know
> what's stefan's suggestion? It seems that he doesn't care much
> about this community.:(
I think not. He is probably busy these days.
>
> I still hope this community can have some vitality.
>
Let's wait for few more days for the possible comments from Stefan or
Michael. But I do prefer to unify the virtio networking datapath which
will be easier to be extended and maintained.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 3/5] VSOCK: support receive mergeable rx buffer in guest
From: Jason Wang @ 2018-11-07 13:29 UTC (permalink / raw)
To: jiangyiwen, stefanha; +Cc: netdev, kvm, virtualization
In-Reply-To: <5BE28F37.1030208@huawei.com>
On 2018/11/7 下午3:07, jiangyiwen wrote:
> On 2018/11/7 14:20, Jason Wang wrote:
>> On 2018/11/6 下午2:41, jiangyiwen wrote:
>>> On 2018/11/6 12:00, Jason Wang wrote:
>>>> On 2018/11/5 下午3:47, jiangyiwen wrote:
>>>>> Guest receive mergeable rx buffer, it can merge
>>>>> scatter rx buffer into a big buffer and then copy
>>>>> to user space.
>>>>>
>>>>> Signed-off-by: Yiwen Jiang<jiangyiwen@huawei.com>
>>>>> ---
>>>>> include/linux/virtio_vsock.h | 9 ++++
>>>>> net/vmw_vsock/virtio_transport.c | 75 +++++++++++++++++++++++++++++----
>>>>> net/vmw_vsock/virtio_transport_common.c | 59 ++++++++++++++++++++++----
>>>>> 3 files changed, 127 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>>>>> index da9e1fe..6be3cd7 100644
>>>>> --- a/include/linux/virtio_vsock.h
>>>>> +++ b/include/linux/virtio_vsock.h
>>>>> @@ -13,6 +13,8 @@
>>>>> #define VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE (1024 * 4)
>>>>> #define VIRTIO_VSOCK_MAX_BUF_SIZE 0xFFFFFFFFUL
>>>>> #define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (1024 * 64)
>>>>> +/* virtio_vsock_pkt + max_pkt_len(default MAX_PKT_BUF_SIZE) */
>>>>> +#define VIRTIO_VSOCK_MAX_MRG_BUF_NUM ((VIRTIO_VSOCK_MAX_PKT_BUF_SIZE / PAGE_SIZE) + 1)
>>>>>
>>>>> /* Virtio-vsock feature */
>>>>> #define VIRTIO_VSOCK_F_MRG_RXBUF 0 /* Host can merge receive buffers. */
>>>>> @@ -48,6 +50,11 @@ struct virtio_vsock_sock {
>>>>> struct list_head rx_queue;
>>>>> };
>>>>>
>>>>> +struct virtio_vsock_mrg_rxbuf {
>>>>> + void *buf;
>>>>> + u32 len;
>>>>> +};
>>>>> +
>>>>> struct virtio_vsock_pkt {
>>>>> struct virtio_vsock_hdr hdr;
>>>>> struct virtio_vsock_mrg_rxbuf_hdr mrg_rxbuf_hdr;
>>>>> @@ -59,6 +66,8 @@ struct virtio_vsock_pkt {
>>>>> u32 len;
>>>>> u32 off;
>>>>> bool reply;
>>>>> + bool mergeable;
>>>>> + struct virtio_vsock_mrg_rxbuf mrg_rxbuf[VIRTIO_VSOCK_MAX_MRG_BUF_NUM];
>>>>> };
>>>> It's better to use iov here I think, and drop buf completely.
>>>>
>>>> And this is better to be done in an independent patch.
>>>>
>>> You're right, I can use kvec instead of customized structure,
>>> in addition, I don't understand about drop buf completely and
>>> an independent patch.
>> I mean there a void *buf in struct virtio_vsock_pkt. You can drop it and switch to use iov(iter) or other data structure that supports sg.
>>
>> Thanks
>>
> Yes, I understand your idea, I don't want to modify tx process method, so I
> keep the buf.
>
I'm afraid this will end of codes that is hard to be maintained. Let's
try to unify them.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 2/5] VSOCK: support fill data to mergeable rx buffer in host
From: jiangyiwen @ 2018-11-07 7:11 UTC (permalink / raw)
To: Jason Wang, stefanha, stefanha; +Cc: netdev, kvm, virtualization
In-Reply-To: <8963dba5-c2fb-69d0-4adb-72d751a9dedf@redhat.com>
On 2018/11/7 14:18, Jason Wang wrote:
>
> On 2018/11/6 下午2:30, jiangyiwen wrote:
>>> Seems duplicated with the one used by vhost-net.
>>>
>>> In packed virtqueue implementation, I plan to move this to vhost.c.
>>>
>> Yes, this code is full copied from vhost-net, if it can be packed into
>> vhost.c, it would be great.
>>
>
> If you try to reuse vhost-net, you don't even need to care about this :)
>
> Thanks
>
>
> .
>
Hi Jason,
Thank your advice, I will consider your idea. But I don't know
what's stefan's suggestion? It seems that he doesn't care much
about this community. :(
I still hope this community can have some vitality.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 3/5] VSOCK: support receive mergeable rx buffer in guest
From: jiangyiwen @ 2018-11-07 7:07 UTC (permalink / raw)
To: Jason Wang, stefanha; +Cc: netdev, kvm, virtualization
In-Reply-To: <1b67b8ef-5dcc-0383-1b32-d80f294533d3@redhat.com>
On 2018/11/7 14:20, Jason Wang wrote:
>
> On 2018/11/6 下午2:41, jiangyiwen wrote:
>> On 2018/11/6 12:00, Jason Wang wrote:
>>> On 2018/11/5 下午3:47, jiangyiwen wrote:
>>>> Guest receive mergeable rx buffer, it can merge
>>>> scatter rx buffer into a big buffer and then copy
>>>> to user space.
>>>>
>>>> Signed-off-by: Yiwen Jiang<jiangyiwen@huawei.com>
>>>> ---
>>>> include/linux/virtio_vsock.h | 9 ++++
>>>> net/vmw_vsock/virtio_transport.c | 75 +++++++++++++++++++++++++++++----
>>>> net/vmw_vsock/virtio_transport_common.c | 59 ++++++++++++++++++++++----
>>>> 3 files changed, 127 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>>>> index da9e1fe..6be3cd7 100644
>>>> --- a/include/linux/virtio_vsock.h
>>>> +++ b/include/linux/virtio_vsock.h
>>>> @@ -13,6 +13,8 @@
>>>> #define VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE (1024 * 4)
>>>> #define VIRTIO_VSOCK_MAX_BUF_SIZE 0xFFFFFFFFUL
>>>> #define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (1024 * 64)
>>>> +/* virtio_vsock_pkt + max_pkt_len(default MAX_PKT_BUF_SIZE) */
>>>> +#define VIRTIO_VSOCK_MAX_MRG_BUF_NUM ((VIRTIO_VSOCK_MAX_PKT_BUF_SIZE / PAGE_SIZE) + 1)
>>>>
>>>> /* Virtio-vsock feature */
>>>> #define VIRTIO_VSOCK_F_MRG_RXBUF 0 /* Host can merge receive buffers. */
>>>> @@ -48,6 +50,11 @@ struct virtio_vsock_sock {
>>>> struct list_head rx_queue;
>>>> };
>>>>
>>>> +struct virtio_vsock_mrg_rxbuf {
>>>> + void *buf;
>>>> + u32 len;
>>>> +};
>>>> +
>>>> struct virtio_vsock_pkt {
>>>> struct virtio_vsock_hdr hdr;
>>>> struct virtio_vsock_mrg_rxbuf_hdr mrg_rxbuf_hdr;
>>>> @@ -59,6 +66,8 @@ struct virtio_vsock_pkt {
>>>> u32 len;
>>>> u32 off;
>>>> bool reply;
>>>> + bool mergeable;
>>>> + struct virtio_vsock_mrg_rxbuf mrg_rxbuf[VIRTIO_VSOCK_MAX_MRG_BUF_NUM];
>>>> };
>>> It's better to use iov here I think, and drop buf completely.
>>>
>>> And this is better to be done in an independent patch.
>>>
>> You're right, I can use kvec instead of customized structure,
>> in addition, I don't understand about drop buf completely and
>> an independent patch.
>
>
> I mean there a void *buf in struct virtio_vsock_pkt. You can drop it and switch to use iov(iter) or other data structure that supports sg.
>
> Thanks
>
Yes, I understand your idea, I don't want to modify tx process method, so I
keep the buf.
>
>>
>> Thanks.
>>
>
> .
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH] drm/qxl: use ttm_tt
From: Gerd Hoffmann @ 2018-11-07 7:05 UTC (permalink / raw)
To: dri-devel
Cc: David Airlie, Dave Airlie, open list,
open list:DRM DRIVER FOR QXL VIRTUAL GPU
qxl device will not dma, so we don't need ttm_dma_tt. Go use ttm_tt
instead, to avoid wasting resources (swiotlb bounce buffers for
example).
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/qxl/qxl_ttm.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
index 559a101138..7a778a46a2 100644
--- a/drivers/gpu/drm/qxl/qxl_ttm.c
+++ b/drivers/gpu/drm/qxl/qxl_ttm.c
@@ -252,7 +252,7 @@ static void qxl_ttm_io_mem_free(struct ttm_bo_device *bdev,
* TTM backend functions.
*/
struct qxl_ttm_tt {
- struct ttm_dma_tt ttm;
+ struct ttm_tt ttm;
struct qxl_device *qdev;
u64 offset;
};
@@ -281,7 +281,7 @@ static void qxl_ttm_backend_destroy(struct ttm_tt *ttm)
{
struct qxl_ttm_tt *gtt = (void *)ttm;
- ttm_dma_tt_fini(>t->ttm);
+ ttm_tt_fini(>t->ttm);
kfree(gtt);
}
@@ -301,13 +301,13 @@ static struct ttm_tt *qxl_ttm_tt_create(struct ttm_buffer_object *bo,
gtt = kzalloc(sizeof(struct qxl_ttm_tt), GFP_KERNEL);
if (gtt == NULL)
return NULL;
- gtt->ttm.ttm.func = &qxl_backend_func;
+ gtt->ttm.func = &qxl_backend_func;
gtt->qdev = qdev;
- if (ttm_dma_tt_init(>t->ttm, bo, page_flags)) {
+ if (ttm_tt_init(>t->ttm, bo, page_flags)) {
kfree(gtt);
return NULL;
}
- return >t->ttm.ttm;
+ return >t->ttm;
}
static void qxl_move_null(struct ttm_buffer_object *bo,
--
2.9.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox