Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 6 Feb 2019 08:29:42 -0500
From:   Luiz Capitulino <lcapitulino@redhat.com>
To:     Nitesh Narayan Lal <nitesh@redhat.com>
Cc:     "Michael S. Tsirkin" <mst@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, pbonzini@redhat.com,
        pagupta@redhat.com, wei.w.wang@intel.com, yang.zhang.wz@gmail.com,
        riel@surriel.com, david@redhat.com, dodgen@google.com,
        konrad.wilk@oracle.com, dhildenb@redhat.com, aarcange@redhat.com
Subject: Re: [RFC][Patch v8 5/7] virtio: Enables to add a single descriptor
 to the host
Message-ID: <20190206082942.29f52fe9@doriath>
In-Reply-To: <1d478678-0a14-a144-2efc-10ff13cf1a5a@redhat.com>
References: <20190204201854.2328-1-nitesh@redhat.com>
        <20190204201854.2328-6-nitesh@redhat.com>
        <20190205154545-mutt-send-email-mst@kernel.org>
        <26a36489-2289-f970-3362-60547b268a76@redhat.com>
        <20190206081533.336110a0@doriath>
        <1d478678-0a14-a144-2efc-10ff13cf1a5a@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 6 Feb 2019 08:24:14 -0500
Nitesh Narayan Lal <nitesh@redhat.com> wrote:

> On 2/6/19 8:15 AM, Luiz Capitulino wrote:
> > On Wed, 6 Feb 2019 07:56:37 -0500
> > Nitesh Narayan Lal <nitesh@redhat.com> wrote:
> >  
> >> On 2/5/19 3:49 PM, Michael S. Tsirkin wrote:  
> >>> On Mon, Feb 04, 2019 at 03:18:52PM -0500, Nitesh Narayan Lal wrote:    
> >>>> This patch enables the caller to expose a single buffers to the
> >>>> other end using vring descriptor. It also allows the caller to
> >>>> perform this action in synchornous manner by using virtqueue_kick_sync.
> >>>>
> >>>> Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com>    
> >>> I am not sure why do we need this API. Polling in guest
> >>> until host runs isn't great either since these
> >>> might be running on the same host CPU.    
> >> True.
> >>
> >> However, my understanding is that the existing API such as
> >> virtqueue_add_outbuf() requires an allocation which will be problematic
> >> for my implementation.
> >> Although I am not blocking the allocation path during normal Linux
> >> kernel usage as even if one of the zone is locked the other zone could
> >> be used to get free pages.
> >> But during the initial boot time (device initialization), in certain
> >> situations the allocation can only come from a single zone, acquiring a
> >> lock on it may result in a deadlock situation.  
> > I might be wrong, but if I remember correctly, this was true for
> > your previous implementation where you'd report page hinting down
> > from arch_free_page() so you couldn't allocate memory. But this
> > is not the case anymore.  
> 
> With the earlier implementation, the allocation was blocked all the time
> when freeing was going on.
> With this implementation, the allocation is not blocked during normal
> Linux kernel usage (after Linux boots up). For example, on a 64 bit
> machine, if the Normal zone is locked and there is an allocation request
> then it can be served by DMA32 zone as well. (This is not the case
> during device initialization time)
> Feel free to correct me if I am wrong.

That's what I meant :) I have an impression that your virtio API
was necessary because of your earlier design. I guess it's not needed
anymore as Michael says.

> 
> >  
> >>>
> >>>    
> >>>> ---
> >>>>  drivers/virtio/virtio_ring.c | 72 ++++++++++++++++++++++++++++++++++++
> >>>>  include/linux/virtio.h       |  4 ++
> >>>>  2 files changed, 76 insertions(+)
> >>>>
> >>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> >>>> index cd7e755484e3..93c161ac6a28 100644
> >>>> --- a/drivers/virtio/virtio_ring.c
> >>>> +++ b/drivers/virtio/virtio_ring.c
> >>>> @@ -1695,6 +1695,52 @@ static inline int virtqueue_add(struct virtqueue *_vq,
> >>>>  					out_sgs, in_sgs, data, ctx, gfp);
> >>>>  }
> >>>>  
> >>>> +/**
> >>>> + * virtqueue_add_desc - add a buffer to a chain using a vring desc
> >>>> + * @vq: the struct virtqueue we're talking about.
> >>>> + * @addr: address of the buffer to add.
> >>>> + * @len: length of the buffer.
> >>>> + * @in: set if the buffer is for the device to write.
> >>>> + *
> >>>> + * Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
> >>>> + */
> >>>> +int virtqueue_add_desc(struct virtqueue *_vq, u64 addr, u32 len, int in)
> >>>> +{
> >>>> +	struct vring_virtqueue *vq = to_vvq(_vq);
> >>>> +	struct vring_desc *desc = vq->split.vring.desc;
> >>>> +	u16 flags = in ? VRING_DESC_F_WRITE : 0;
> >>>> +	unsigned int i;
> >>>> +	void *data = (void *)addr;
> >>>> +	int avail_idx;
> >>>> +
> >>>> +	/* Sanity check */
> >>>> +	if (!_vq)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	START_USE(vq);
> >>>> +	if (unlikely(vq->broken)) {
> >>>> +		END_USE(vq);
> >>>> +		return -EIO;
> >>>> +	}
> >>>> +
> >>>> +	i = vq->free_head;
> >>>> +	flags &= ~VRING_DESC_F_NEXT;
> >>>> +	desc[i].flags = cpu_to_virtio16(_vq->vdev, flags);
> >>>> +	desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
> >>>> +	desc[i].len = cpu_to_virtio32(_vq->vdev, len);
> >>>> +
> >>>> +	vq->vq.num_free--;
> >>>> +	vq->free_head = virtio16_to_cpu(_vq->vdev, desc[i].next);
> >>>> +	vq->split.desc_state[i].data = data;
> >>>> +	vq->split.avail_idx_shadow = 1;
> >>>> +	avail_idx = vq->split.avail_idx_shadow;
> >>>> +	vq->split.vring.avail->idx = cpu_to_virtio16(_vq->vdev, avail_idx);
> >>>> +	vq->num_added = 1;
> >>>> +	END_USE(vq);
> >>>> +	return 0;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(virtqueue_add_desc);
> >>>> +
> >>>>  /**
> >>>>   * virtqueue_add_sgs - expose buffers to other end
> >>>>   * @vq: the struct virtqueue we're talking about.
> >>>> @@ -1842,6 +1888,32 @@ bool virtqueue_notify(struct virtqueue *_vq)
> >>>>  }
> >>>>  EXPORT_SYMBOL_GPL(virtqueue_notify);
> >>>>  
> >>>> +/**
> >>>> + * virtqueue_kick_sync - update after add_buf and busy wait till update is done
> >>>> + * @vq: the struct virtqueue
> >>>> + *
> >>>> + * After one or more virtqueue_add_* calls, invoke this to kick
> >>>> + * the other side. Busy wait till the other side is done with the update.
> >>>> + *
> >>>> + * Caller must ensure we don't call this with other virtqueue
> >>>> + * operations at the same time (except where noted).
> >>>> + *
> >>>> + * Returns false if kick failed, otherwise true.
> >>>> + */
> >>>> +bool virtqueue_kick_sync(struct virtqueue *vq)
> >>>> +{
> >>>> +	u32 len;
> >>>> +
> >>>> +	if (likely(virtqueue_kick(vq))) {
> >>>> +		while (!virtqueue_get_buf(vq, &len) &&
> >>>> +		       !virtqueue_is_broken(vq))
> >>>> +			cpu_relax();
> >>>> +		return true;
> >>>> +	}
> >>>> +	return false;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(virtqueue_kick_sync);
> >>>> +
> >>>>  /**
> >>>>   * virtqueue_kick - update after add_buf
> >>>>   * @vq: the struct virtqueue
> >>>> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> >>>> index fa1b5da2804e..58943a3a0e8d 100644
> >>>> --- a/include/linux/virtio.h
> >>>> +++ b/include/linux/virtio.h
> >>>> @@ -57,6 +57,10 @@ int virtqueue_add_sgs(struct virtqueue *vq,
> >>>>  		      unsigned int in_sgs,
> >>>>  		      void *data,
> >>>>  		      gfp_t gfp);
> >>>> +/* A desc with this init id is treated as an invalid desc */
> >>>> +int virtqueue_add_desc(struct virtqueue *_vq, u64 addr, u32 len, int in);
> >>>> +
> >>>> +bool virtqueue_kick_sync(struct virtqueue *vq);
> >>>>  
> >>>>  bool virtqueue_kick(struct virtqueue *vq);
> >>>>  
> >>>> -- 
> >>>> 2.17.2