Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230427104428.862643-1-mie@igel.co.jp> <20230427104428.862643-4-mie@igel.co.jp>
 <CACGkMEsjH8fA2r=0CacK8WK_sUTAcTK7SQ_VwkJpa1rSgDP0dg@mail.gmail.com>
In-Reply-To: <CACGkMEsjH8fA2r=0CacK8WK_sUTAcTK7SQ_VwkJpa1rSgDP0dg@mail.gmail.com>
From:   Shunsuke Mie <mie@igel.co.jp>
Date:   Wed, 10 May 2023 12:17:46 +0900
Message-ID: <CANXvt5r7eha_xnExsdS_4yMW8xTJxVzYhMVrXyQkGQe-_ZURBg@mail.gmail.com>
Subject: Re: [RFC PATCH v2 3/3] PCI: endpoint: Add EP function driver to
 provide virtio-console functionality
To:     Jason Wang <jasowang@redhat.com>
Cc:     Lorenzo Pieralisi <lpieralisi@kernel.org>,
        =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= <kw@linux.com>,
        Manivannan Sadhasivam <mani@kernel.org>,
        Kishon Vijay Abraham I <kishon@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        "Michael S. Tsirkin" <mst@redhat.com>, Frank Li <Frank.Li@nxp.com>,
        Jon Mason <jdmason@kudzu.us>,
        Randy Dunlap <rdunlap@infradead.org>,
        Ren Zhijie <renzhijie2@huawei.com>,
        linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
        virtualization@lists.linux-foundation.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Json,
2023=E5=B9=B45=E6=9C=888=E6=97=A5(=E6=9C=88) 13:03 Jason Wang <jasowang@red=
hat.com>:
>
> On Thu, Apr 27, 2023 at 6:44=E2=80=AFPM Shunsuke Mie <mie@igel.co.jp> wro=
te:
> >
> > Add a new PCIe endpoint function driver that works as a pci virtio-cons=
ole
> > device. The console connect to endpoint side console. It enables to
> > communicate PCIe host and endpoint.
> >
> > Architecture is following:
> >
> >  =E2=94=8C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=
=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=90         =E2=94=8C=
=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=
=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=
=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=AC=E2=94=80=E2=94=80=
=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=
=94=80=E2=94=80=E2=94=90
> >  =E2=94=82virtioe     =E2=94=82         =E2=94=82                      =
=E2=94=82virtio      =E2=94=82
> >  =E2=94=82console drv =E2=94=82         =E2=94=9C=E2=94=80=E2=94=80=E2=
=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=
=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=90      =E2=94=82console drv =
=E2=94=82
> >  =E2=94=9C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=
=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=A4         =E2=94=82=
(virtio console=E2=94=82      =E2=94=9C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=
=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=
=94=A4
> >  =E2=94=82 virtio bus =E2=94=82         =E2=94=82 device)       =E2=94=
=82=E2=97=84=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=96=BA=E2=94=82 virtio b=
us =E2=94=82
> >  =E2=94=9C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=
=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=A4         =E2=94=9C=
---------------=E2=94=A4      =E2=94=94=E2=94=80=E2=94=80=E2=94=80=E2=94=80=
=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=
=94=A4
> >  =E2=94=82            =E2=94=82         =E2=94=82 pci ep virtio =E2=94=
=82                   =E2=94=82
> >  =E2=94=82  pci bus   =E2=94=82         =E2=94=82  console drv  =E2=94=
=82                   =E2=94=82
> >  =E2=94=82            =E2=94=82  pcie   =E2=94=9C=E2=94=80=E2=94=80=E2=
=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=
=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=A4                   =E2=94=
=82
> >  =E2=94=82            =E2=94=82 =E2=97=84=E2=94=80=E2=94=80=E2=94=80=E2=
=94=80=E2=94=80=E2=96=BA =E2=94=82  pci ep Bus   =E2=94=82                 =
  =E2=94=82
> >  =E2=94=94=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=
=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=98         =E2=94=94=
=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=
=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=B4=E2=94=
=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=
=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=
=94=80=E2=94=80=E2=94=98
> >    PCIe Root              PCIe Endpoint
> >
>
> I think it might only works for peer devices like:
>
> net, console or vsock.
Could you tell me what "peer devices" means?

> So there're many choices here, I'd like to know what's the reason for
> you to implement a mediation.
>
> An alternative is to implement a dedicated net, console and vsock
> driver for vringh (CAIF somehow works like this). This would have
> better performance.
Does it mean that the driver also functions as a network driver directly?
>
>
> > This driver has two roles. The first is as a PCIe endpoint virtio conso=
le
> > function, which is implemented using the PCIe endpoint framework and PC=
Ie
> > EP virtio helpers. The second is as a virtual virtio console device
> > connected to the virtio bus on PCIe endpoint Linux.
> >
> > Communication between the two is achieved by copying the virtqueue data
> > between PCIe root and endpoint, respectively.
> >
> > This is a simple implementation and does not include features of
> > virtio-console such as MULTIPORT, EMERG_WRITE, etc. As a result, each
> > virtio console driver only displays /dev/hvc0.
> >
> > As an example of usage, by setting getty to /dev/hvc0, it is possible t=
o
> > login to another host.
> >
> > Signed-off-by: Shunsuke Mie <mie@igel.co.jp>
> > ---
> > Changes from v2:
> > - Change to use copy functions between kiovs of pci-epf-virtio.
> >
> >  drivers/pci/endpoint/functions/Kconfig        |  12 +
> >  drivers/pci/endpoint/functions/Makefile       |   1 +
> >  drivers/pci/endpoint/functions/pci-epf-vcon.c | 596 ++++++++++++++++++
> >  3 files changed, 609 insertions(+)
> >  create mode 100644 drivers/pci/endpoint/functions/pci-epf-vcon.c
> >
> > diff --git a/drivers/pci/endpoint/functions/Kconfig b/drivers/pci/endpo=
int/functions/Kconfig
> > index fa1a6a569a8f..9ce2698b67e1 100644
> > --- a/drivers/pci/endpoint/functions/Kconfig
> > +++ b/drivers/pci/endpoint/functions/Kconfig
> > @@ -44,3 +44,15 @@ config PCI_EPF_VIRTIO
> >         select VHOST_RING_IOMEM
> >         help
> >           Helpers to implement PCI virtio Endpoint function
> > +
> > +config PCI_EPF_VCON
> > +       tristate "PCI Endpoint virito-console driver"
> > +       depends on PCI_ENDPOINT
> > +       select VHOST_RING
> > +       select PCI_EPF_VIRTIO
> > +       help
> > +         PCIe Endpoint virtio-console function implementatino. This mo=
dule
> > +         enables to show the virtio-console as pci device to PCIe host=
 side, and
> > +         another virtual virtio-console device registers to endpoint s=
ystem.
> > +         Those devices are connected virtually and can communicate eac=
h other.
> > +
> > diff --git a/drivers/pci/endpoint/functions/Makefile b/drivers/pci/endp=
oint/functions/Makefile
> > index a96f127ce900..b4056689ce33 100644
> > --- a/drivers/pci/endpoint/functions/Makefile
> > +++ b/drivers/pci/endpoint/functions/Makefile
> > @@ -7,3 +7,4 @@ obj-$(CONFIG_PCI_EPF_TEST)              +=3D pci-epf-te=
st.o
> >  obj-$(CONFIG_PCI_EPF_NTB)              +=3D pci-epf-ntb.o
> >  obj-$(CONFIG_PCI_EPF_VNTB)             +=3D pci-epf-vntb.o
> >  obj-$(CONFIG_PCI_EPF_VIRTIO)           +=3D pci-epf-virtio.o
> > +obj-$(CONFIG_PCI_EPF_VCON)             +=3D pci-epf-vcon.o
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-vcon.c b/drivers/pc=
i/endpoint/functions/pci-epf-vcon.c
> > new file mode 100644
> > index 000000000000..31f4247cd10f
> > --- /dev/null
> > +++ b/drivers/pci/endpoint/functions/pci-epf-vcon.c
> > @@ -0,0 +1,596 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * PCI Endpoint function driver to impliment virtio-console device
> > + * functionality.
> > + */
> > +#include <linux/pci-epf.h>
> > +#include <linux/virtio_ids.h>
> > +#include <linux/virtio_pci.h>
> > +#include <linux/virtio_console.h>
> > +#include <linux/virtio_ring.h>
> > +
> > +#include "pci-epf-virtio.h"
> > +
> > +static int virtio_queue_size =3D 0x100;
> > +module_param(virtio_queue_size, int, 0444);
> > +MODULE_PARM_DESC(virtio_queue_size, "A length of virtqueue");
> > +
> > +struct epf_vcon {
> > +       /* To access virtqueues on remote host */
> > +       struct epf_virtio evio;
> > +       struct vringh_kiov *rdev_iovs;
> > +
> > +       /* To register a local virtio bus */
> > +       struct virtio_device vdev;
> > +
> > +       /* To access virtqueus of local host driver */
> > +       struct vringh *vdev_vrhs;
> > +       struct vringh_kiov *vdev_iovs;
> > +       struct virtqueue **vdev_vqs;
> > +
> > +       /* For transportation and notification */
> > +       struct workqueue_struct *task_wq;
> > +       struct work_struct raise_irq_work, rx_work, tx_work;
> > +
> > +       /* To retain virtio features. It is commonly used local and rem=
ote. */
> > +       u64 features;
> > +
> > +       /* To show a status whether this driver is ready and the remote=
 is connected */
> > +       bool connected;
> > +};
> > +
> > +enum {
> > +       VCON_VIRTQUEUE_RX,
> > +       VCON_VIRTQUEUE_TX,
> > +       // Should be end of enum
> > +       VCON_VIRTQUEUE_NUM
> > +};
>
> It would be better if we can split the console specific thing out,
> then it allows us to do ethernet and vsock in the future.
I'm planning to implement each virtio device in a separate file.
https://lwn.net/Articles/922124/


> Thanks
>
Best regards,
Shunsuke