Message-ID: <2422273d-5ac2-63ef-8a31-230ebfbe3b23@igel.co.jp>
Date:   Wed, 31 May 2023 19:51:38 +0900
From:   Shunsuke Mie <mie@...l.co.jp>
To:     Jason Wang <jasowang@...hat.com>
Cc:     Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Manivannan Sadhasivam <mani@...nel.org>,
        Kishon Vijay Abraham I <kishon@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        "Michael S. Tsirkin" <mst@...hat.com>, Frank Li <Frank.Li@....com>,
        Jon Mason <jdmason@...zu.us>,
        Randy Dunlap <rdunlap@...radead.org>,
        Ren Zhijie <renzhijie2@...wei.com>,
        linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        virtualization@...ts.linux-foundation.org
Subject: Re: [RFC PATCH v2 3/3] PCI: endpoint: Add EP function driver to
 provide virtio-console functionality

I'm sorry for late response.

On 2023/05/19 11:01, Jason Wang wrote:
> On Thu, May 18, 2023 at 5:54 PM Shunsuke Mie <mie@...l.co.jp> wrote:
>> Gentle ping ...
>>
>>
>> Thanks,
>>
>> Shunsuke.
>>
>> On 2023/05/10 12:17, Shunsuke Mie wrote:
>>> Hi Json,
>>> 2023年5月8日(月) 13:03 Jason Wang <jasowang@...hat.com>:
>>>> On Thu, Apr 27, 2023 at 6:44 PM Shunsuke Mie <mie@...l.co.jp> wrote:
>>>>> Add a new PCIe endpoint function driver that works as a pci virtio-console
>>>>> device. The console connect to endpoint side console. It enables to
>>>>> communicate PCIe host and endpoint.
>>>>>
>>>>> Architecture is following:
>>>>>
>>>>>    ┌────────────┐         ┌──────────────────────┬────────────┐
>>>>>    │virtioe     │         │                      │virtio      │
>>>>>    │console drv │         ├───────────────┐      │console drv │
>>>>>    ├────────────┤         │(virtio console│      ├────────────┤
>>>>>    │ virtio bus │         │ device)       │◄────►│ virtio bus │
>>>>>    ├────────────┤         ├---------------┤      └────────────┤
>>>>>    │            │         │ pci ep virtio │                   │
>>>>>    │  pci bus   │         │  console drv  │                   │
>>>>>    │            │  pcie   ├───────────────┤                   │
>>>>>    │            │ ◄─────► │  pci ep Bus   │                   │
>>>>>    └────────────┘         └───────────────┴───────────────────┘
>>>>>      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?
> I meant, for example we know in the case of virtio-net, TX can talk
> with RX belonging to another device directly.
>
> But this is not the case for other devices like virito-blk.
Thank you. I comprehended it.
>>>> 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?
> I meant, e.g in the case of networking, you can have a dedicated
> driver with two vringh in the endpoint side.
>
> The benefit is the performance, no need for the (datapath) mediation.
>
> But if we don't care about the performance, this proposal seems to be fine.
>
> Thanks
I agree with you.  The design you suggested is better in terms of 
performance.
However, the proposed design is not bad for the following the reasons I 
think.

The proposed design has one more operation in control plane because the data
steps over the virtio-net driver, but the number of copies at the data plane
remains the same. I think the operation added in control plane has small 
effects
for performance.

Moreover, there are some advantages when the data step over the virtio-net
driver. We can make use of the optimizations and some functions without
modifications and implementations. e.g. ethtool and XDP(BPF) supports.

Any comments would be appreciated.

>>>>> This driver has two roles. The first is as a PCIe endpoint virtio console
>>>>> function, which is implemented using the PCIe endpoint framework and PCIe
>>>>> 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 to
>>>>> login to another host.
>>>>>
>>>>> Signed-off-by: Shunsuke Mie <mie@...l.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/endpoint/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 module
>>>>> +         enables to show the virtio-console as pci device to PCIe host side, and
>>>>> +         another virtual virtio-console device registers to endpoint system.
>>>>> +         Those devices are connected virtually and can communicate each other.
>>>>> +
>>>>> diff --git a/drivers/pci/endpoint/functions/Makefile b/drivers/pci/endpoint/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)              += pci-epf-test.o
>>>>>    obj-$(CONFIG_PCI_EPF_NTB)              += pci-epf-ntb.o
>>>>>    obj-$(CONFIG_PCI_EPF_VNTB)             += pci-epf-vntb.o
>>>>>    obj-$(CONFIG_PCI_EPF_VIRTIO)           += pci-epf-virtio.o
>>>>> +obj-$(CONFIG_PCI_EPF_VCON)             += pci-epf-vcon.o
>>>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-vcon.c b/drivers/pci/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 = 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 remote. */
>>>>> +       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

Best regards,

Shunsuke

Powered by blists - more mailing lists