Re: [dpdk-dev] [PATCH 6/6] virtio: add virtio v1.0 support

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Tue, Dec 29, 2015 at 11:39:47AM +0000, Tan, Jianfeng wrote:
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yuanhan Liu
> > Sent: Thursday, December 10, 2015 11:54 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH 6/6] virtio: add virtio v1.0 support
> > 
> ...
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > ---
> >  drivers/net/virtio/virtio_ethdev.c |  16 +-
> >  drivers/net/virtio/virtio_ethdev.h |   3 +-
> >  drivers/net/virtio/virtio_pci.c    | 313
> > ++++++++++++++++++++++++++++++++++++-
> >  drivers/net/virtio/virtio_pci.h    |  65 ++++++++
> >  drivers/net/virtio/virtqueue.h     |   2 +
> >  5 files changed, 395 insertions(+), 4 deletions(-)
> > 
> 
> As legacy VIRTIO_WRITE_REG_XXXs are only used in virtio_pci.c, move these definitions into virtio_pci.c?

Yes, and I planned to do it in another series, where I will do more virtio
cleanups. (hmm..., maybe I could stack them on top of this series).

> 
> > diff --git a/drivers/net/virtio/virtio_ethdev.c
> > b/drivers/net/virtio/virtio_ethdev.c
> > index 9847ed8..1f9de01 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -927,7 +927,7 @@ virtio_vlan_filter_set(struct rte_eth_dev *dev,
> > uint16_t vlan_id, int on)
> >  	return virtio_send_command(hw->cvq, &ctrl, &len, 1);
> >  }
> > 
> > -static void
> > +static int
> >  virtio_negotiate_features(struct virtio_hw *hw)
> >  {
> >  	uint64_t host_features;
> > @@ -949,6 +949,17 @@ virtio_negotiate_features(struct virtio_hw *hw)
> >  	hw->guest_features = vtpci_negotiate_features(hw, host_features);
> >  	PMD_INIT_LOG(DEBUG, "features after negotiate = %"PRIx64,
> >  		hw->guest_features);
> > +
> > +	if (hw->modern) {
> > +		if (!vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
> > +			PMD_INIT_LOG(ERR,
> > +				"VIRTIO_F_VERSION_1 features is not
> > enabled");
> > +			return -1;
> > +		}
> > +		vtpci_set_status(hw,
> > VIRTIO_CONFIG_STATUS_FEATURES_OK);
> 
> 
> As per Linux code drivers/virtio/virtio.c:virtio_finalize_features(), vtpci_set_status() may fail, and there's a double check.
> Is it necessary here?

Yes, it's necessary: see the spec (3.1.1 Driver Requirements: Device
Initialization):

    5. Set the FEATURES_OK status bit. The driver MUST NOT accept new
       feature bits after this step.

    6. Re-read device status to ensure the FEATURES_OK bit is still set:
       otherwise, the device does not support our subset of features and
       the device is unusable.
> 
> 
> > +	}
> > +
> > +	return 0;
> >  }
> > 
> >  /*
> > @@ -1032,7 +1043,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > 
> >  	/* Tell the host we've known how to drive the device. */
> >  	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
> > -	virtio_negotiate_features(hw);
> > +	if (virtio_negotiate_features(hw) < 0)
> > +		return -1;
> > 
> >  	/* If host does not support status then disable LSC */
> >  	if (!vtpci_with_feature(hw, VIRTIO_NET_F_STATUS))
> > diff --git a/drivers/net/virtio/virtio_ethdev.h
> > b/drivers/net/virtio/virtio_ethdev.h
> > index ae2d47d..fed9571 100644
> > --- a/drivers/net/virtio/virtio_ethdev.h
> > +++ b/drivers/net/virtio/virtio_ethdev.h
> > @@ -64,7 +64,8 @@
> >  	 1u << VIRTIO_NET_F_CTRL_VQ	  |	\
> >  	 1u << VIRTIO_NET_F_CTRL_RX	  |	\
> >  	 1u << VIRTIO_NET_F_CTRL_VLAN	  |	\
> > -	 1u << VIRTIO_NET_F_MRG_RXBUF)
> > +	 1u << VIRTIO_NET_F_MRG_RXBUF	  |	\
> > +	 1ULL << VIRTIO_F_VERSION_1)
> > 
> >  /*
> >   * CQ function prototype
> > diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
> > index 26b0a0c..7862d7f 100644
> > --- a/drivers/net/virtio/virtio_pci.c
> > +++ b/drivers/net/virtio/virtio_pci.c
> > @@ -31,6 +31,7 @@
> >   *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> > DAMAGE.
> >   */
> >  #include <stdint.h>
> > +#include <linux/pci_regs.h>
> 
> Put this "include" into macro RTE_EXEC_ENV_LINUXAPP followed?

We can't simply do that, as we need reference some macros from it
in common code path, virtio_read_caps(). But yes, you just remind
me that there should be no such header in non-linux platform. We
can't include it blindly here. Good catch, btw!

What's lucky here is that we just need reference quite few macros,
maybe we could just define them by ourself, to get rid of the
depency.

> 
> > 
> >  #ifdef RTE_EXEC_ENV_LINUXAPP
> >   #include <dirent.h>
> > @@ -448,6 +449,205 @@ static const struct virtio_pci_ops legacy_ops = {
> >  };
> > 
> > 
> ...
> > +
> > +static uint16_t
> > +modern_set_irq(struct virtio_hw *hw __rte_unused, uint16_t vec
> > __rte_unused)
> > +{
> > +	/* FIXME: xxx */
> > +	return 0;
> > +}
> 
> If not going to support LSC, please give clear indication at related doc. My concern is: this will affect
> some users, who are using LSC in legacy virtio. (If LSC works well on legacy virtio?).

TBH, I don't spent too much time on that, therefore, I marked it as
FIXME here, with the hope that I will revisit it with more cares in
future version.

> > +
> > +static uint16_t
> > +modern_get_queue_num(struct virtio_hw *hw, uint16_t queue_id)
> > +{
> > +	modern_write16(queue_id, &hw->common_cfg->queue_select);
> > +	return modern_read16(&hw->common_cfg->queue_size);
> > +}
> > +
> ...
> > 
> > +static inline void *
> > +get_cfg_addr(struct rte_pci_device *dev, struct virtio_pci_cap *cap)
> > +{
> > +	uint8_t  bar    = cap->bar;
> > +	uint32_t length = cap->length;
> > +	uint32_t offset = cap->offset;
> > +	uint8_t *base;
> > +
> 
> Use a macro to substitute hardcoded "5"?

The fact that 5 is the max bar number is so well known, that I don't
think it's necessary to add a macro here.

> > +	if (unlikely(bar > 5)) {
> > +		PMD_INIT_LOG(ERR, "invalid bar: %u", bar);
> > +		return NULL;
> > +	}
> > +
> ...
> >  int
> >  vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw)
> >  {
> > -	hw->vtpci_ops = &legacy_ops;
> > +	hw->dev = dev;
> > 
> > +	/*
> > +	 * Try if we can succeed reading virtio pci caps, which exists
> > +	 * only on modern pci device. If failed, we fallback to legacy
> > +	 * virtio hanlding.
> 
> hanlding -> handling

Oops, and thanks.

	--yliu