Re: [dpdk-dev] [PATCH v2 0/7] virtio 1.0 enabling for virtio pmd driver

[Tetsuya Mukawa <mukawa@igel.co.jp>]

On 2016/01/12 15:58, Yuanhan Liu wrote:
> v2: - fix a data corruption reported by Qian, due to hdr size mismatch.
>       check detailes at ptach 5.
>
>     - Add missing config_irq and isr reading support from v1.
>
>     - fix comments from v1.
>
> Almost all difference comes from virtio 1.0 are the PCI layout change:
> the major configuration structures are stored at bar space, and their
> location is stored at corresponding pci cap structure. Reading/parsing
> them is one of the major work of patch 7.
>
> To make handling virtio v1.0 and v0.95 co-exist well, this patch set
> introduces a virtio_pci_ops structure, to add another layer so that
> we could keep those vtpci_foo_bar "APIs". With that, we could do the
> minimum change to add virtio 1.0 support.
>
>
> ---
> Yuanhan Liu (7):
>   virtio: don't set vring address again at queue startup
>   virtio: introduce struct virtio_pci_ops
>   virtio: move left pci stuff to virtio_pci.c
>   viritio: switch to 64 bit features
>   virtio: retrieve hdr_size from hw->vtnet_hdr_size
>   eal: pci: export pci_map_device
>   virtio: add 1.0 support
>
>  doc/guides/rel_notes/release_2_3.rst            |   3 +
>  drivers/net/virtio/virtio_ethdev.c              | 301 +---------
>  drivers/net/virtio/virtio_ethdev.h              |   3 +-
>  drivers/net/virtio/virtio_pci.c                 | 768 +++++++++++++++++++++++-
>  drivers/net/virtio/virtio_pci.h                 | 102 +++-
>  drivers/net/virtio/virtio_rxtx.c                |  21 +-
>  drivers/net/virtio/virtqueue.h                  |   4 +-
>  lib/librte_eal/bsdapp/eal/eal_pci.c             |   2 +-
>  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |   6 +
>  lib/librte_eal/common/eal_common_pci.c          |   2 +-
>  lib/librte_eal/common/eal_private.h             |  11 -
>  lib/librte_eal/common/include/rte_pci.h         |  11 +
>  lib/librte_eal/linuxapp/eal/eal_pci.c           |   2 +-
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |   6 +
>  14 files changed, 899 insertions(+), 343 deletions(-)
>

Hi Yuanhan and Jianfeng,

Thanks for great patches.
I want to use VIRTIO-1.0 feature for my virtio container patch, because
it will solve 44 bit memory address limitation.
(So far, legacy virtio-net device only receives queue address under (1
<< (32 + 12)).)

I have a few comments to rebase virtio container patches on this patches.

1. VIRTIO_READ_REG_X

So far, VIRTIO_READ_REG_1/2/4 are defined in virtio_pci.h.
But these macros are only referred by virtio_pci.c.
How about moving the macros to virtio_pci.c?

2. Abstraction of read/write accesses.

It may be difficult to cleanly rebase my patches on this patches,
because virtio_read_caps() is not abstracted.
Let me describe it more.
So far, we need to handle below 3 virtio-net devices..
 - physical virtio-net device.
 - virtual virtio-net device in virtio-net PMD. (Jianfeng's patch)
 - virtual virtio-net device in QEMU. (my patch)

Almost all code of the virtio-net PMD can be shared between above
different cases.
Probably big difference is how to access to configuration space.

Yuanhan's patch introduces an abstraction layer to hide configuration
space layout and how to access it.
Is it possible to separate?
I guess "access method" will be nice to be abstracted separately from
"configuration space layout".
Probably access method will be defined by "eth_dev->dev_type" and the
PMD name like "eth_cvio".
And "configuration space layout" will be defined by capability list of
PCI configuration layout.

For example, if access method like below are abstracted separately and
current "virtio_pci.c" is implemented on this abstraction, we can easily
re-use virtio_read_caps().
 - how to read/write virtio configuration space.
 - how to mmap PCI configuration space.
 - how to read/(write) PCI configuration space.

Thanks,
Tetsuya