Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <cover.1631101392.git.wuzongyong@linux.alibaba.com>
In-Reply-To: <cover.1631101392.git.wuzongyong@linux.alibaba.com>
From:   Jason Wang <jasowang@redhat.com>
Date:   Thu, 9 Sep 2021 11:05:06 +0800
Message-ID: <CACGkMEuEwbDQUtYHz=0O4pQcb6ibY0MAT7hLDjN=Okw8c9CZGA@mail.gmail.com>
Subject: Re: [PATCH 0/6] vDPA driver for legacy virtio-pci device
To:     Wu Zongyong <wuzongyong@linux.alibaba.com>
Cc:     virtualization <virtualization@lists.linux-foundation.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        mst <mst@redhat.com>, wei.yang1@linux.alibaba.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Sep 8, 2021 at 8:22 PM Wu Zongyong <wuzongyong@linux.alibaba.com> wrote:
>
> This series implements the vDPA driver for legacy virtio-pci device.
> Currently we already have the vDPA driver for modern virtio-pci device
> only, but there are some legacy virtio-pci devices conforming to the
> virtio-pci specifications of 0.9.x or older versions. For example,
> ENI(Elastic Network Interface) of Alibaba ECS baremetal instance is a
> hardware virtio network device which follows the Virtio PCI Card 0.9.5
> Draft specification. Such legacy virtio-pci devices have some
> inconsistent behaviour with modern virtio-pci devices, so some common
> codes are split out and modern device specific codes are moved to a
> separated file.

What worries me a little bit are:

1) vDPA requires IOMMU_PLATFORM to be supported by the device to work,
if I understand ENI correctly, it's a legacy device so it can't
support ACCESS_PLATFORM. Or is it a legacy device that supports
ACCESS_PLATFORM implicitly.
2) vDPA tries to present a 1.0 device, in this case the behavior could
be ruled by the spec. If we tries to present an 1.0 device on top of
legacy device we may suffer a lot of issues:

- endian issue: 1.0 use le but legacy may use native endian
- queue_enable semantic which is missed in the legacy
- virtqueue size, as you mentioned below

I guess what the device(ENI) supports are:

1) semantic of ACCESS_PLATFORM without a feature
2) little endian
3) but a legacy device

So I think it might be better:

1) introduce the library for legacy as you did in this patch
2) having a dedicated ENI vDPA driver

3) live migration support, though it was not supported by the spec
yet, but we are working on the support, and we know legacy device can
support this.

Thanks

>
> For legacy devices, it is not supported to negotiate the virtqueue size
> by the specification. So a new callback get_vq_num_unchangeable is
> introduced to indicate user not to try change the virtqueue size of the
> legacy vdpa device. For example, QEMU should not allocate memory for
> virtqueue according to the properties tx_queue_size and rx_queue_size if
> we use legacy virtio-pci device as the vhost-vdpa backend. Instead, QEMU
> should use the new callback get_vq_num_unchangeable first to check if
> the vdpa device support to change virtqueue size. If not, QEMU should
> call the callback get_vq_num_max to get the static virtqueue size then
> allocate the same size memory for the virtqueue.
>
> This series have been tested with the ENI in Alibaba ECS baremetal
> instance.
>
> These patches may under consideration, welcome for comments.
>
>
> Wu Zongyong (6):
>   virtio-pci: introduce legacy device module
>   vdpa: fix typo
>   vp_vdpa: add vq irq offloading support
>   vp_vdpa: split out reusable and device specific codes to separate file
>   vdpa: add get_vq_num_unchangeable callback in vdpa_config_ops
>   vp_vdpa: introduce legacy virtio pci driver
>
>  drivers/vdpa/Kconfig                     |   7 +
>  drivers/vdpa/virtio_pci/Makefile         |   3 +
>  drivers/vdpa/virtio_pci/vp_vdpa.c        |   8 +
>  drivers/vdpa/virtio_pci/vp_vdpa_common.c | 220 ++++++++++++++
>  drivers/vdpa/virtio_pci/vp_vdpa_common.h |  67 +++++
>  drivers/vdpa/virtio_pci/vp_vdpa_legacy.c | 346 +++++++++++++++++++++++
>  drivers/vdpa/virtio_pci/vp_vdpa_modern.c | 327 +++++++++++++++++++++
>  drivers/vhost/vdpa.c                     |  19 ++
>  drivers/virtio/Kconfig                   |  10 +
>  drivers/virtio/Makefile                  |   1 +
>  drivers/virtio/virtio_pci_common.c       |  10 +-
>  drivers/virtio/virtio_pci_common.h       |   9 +-
>  drivers/virtio/virtio_pci_legacy.c       | 101 ++-----
>  drivers/virtio/virtio_pci_legacy_dev.c   | 220 ++++++++++++++
>  drivers/virtio/virtio_vdpa.c             |   5 +-
>  include/linux/vdpa.h                     |   6 +-
>  include/linux/virtio_pci_legacy.h        |  44 +++
>  include/uapi/linux/vhost.h               |   2 +
>  18 files changed, 1320 insertions(+), 85 deletions(-)
>  create mode 100644 drivers/vdpa/virtio_pci/vp_vdpa_common.c
>  create mode 100644 drivers/vdpa/virtio_pci/vp_vdpa_common.h
>  create mode 100644 drivers/vdpa/virtio_pci/vp_vdpa_legacy.c
>  create mode 100644 drivers/vdpa/virtio_pci/vp_vdpa_modern.c
>  create mode 100644 drivers/virtio/virtio_pci_legacy_dev.c
>  create mode 100644 include/linux/virtio_pci_legacy.h
>
> --
> 2.31.1
>