Re: [RFC PATCH v2 1/3] PCI: endpoint: introduce a helper to implement pci ep virtio function
I'll fix the typo and some styles you mentioned.
In this e-mail, I reply to the other comments.
2023年4月28日(金) 3:28 Bjorn Helgaas :
>
> Simple typos, don't repost until there's more substantive feedback.
>
> On Thu, Apr 27, 2023 at 07:44:26PM +0900, Shunsuke Mie wrote:
> > The Linux PCIe Endpoint framework supports to implement PCIe endpoint
> > functions using a PCIe controller operating in endpoint mode.
> > It is possble to realize the behavior of PCIe device, such as virtio PCI
> > device. This patch introduces a setof helper functions and data structures
> > to implement a PCIe endpoint function that behaves as a virtio device.
>
> s/possble/possible/
> s/setof/set of/
>
> > Those functions enable the implementation PCIe endpoint function that
> > comply with the virtio lecacy specification. Because modern virtio
> > specifications require devices to implement custom PCIe capabilities, which
> > are not currently supported by either PCIe controllers/drivers or the PCIe
> > endpoint framework.
>
> s/implementation PCIe endpoint function/implementation of PCIe endpoint
> functions/
> s/lecacy/legacy/ (What does "legacy" mean? Is there a spec for this?)
Yes, it is defined in spec.
> I guess "legacy virtio" devices need not implement custom PCIe
> capabilities, but "modern virtio" devices must implement them?
That I wanted to explain here.
> Capitalize "Endpoint framework" consistently; sometimes it's
> "Endpoint", other times it's "endpoint".
I'll take care to be consistent.
> > While this patch provides functions for negotiating with host drivers and
> > copying data, each PCIe function driver must impl ement operations that
> > depend on each specific device, such as network, block, etc.
>
> s/impl ement/implement/
>
> > +#include
> > +#include
> > +#include
>
> Typically the header includes would be alphabetized if possible.
I'll try to reorder them.
> > + vq_virt = pci_epc_map_addr(epf->epc, epf->func_no, epf->vfunc_no,
> > +vq_pci_addr, vq_phys, vq_size);
> > + if (IS_ERR(vq_virt)) {
> > + pr_err("Failed to map virtuqueue to local");
>
> s/virtuqueue/virtqueue/
>
> I know you probably don't have any way to use dev_err(), but this
> message really needs some context, like a driver name and instance or
> something.
I'll try to use the dev_* function appropriately If possible.
> > +#define VIRTIO_PCI_LEGACY_CFG_BAR 0
>
> What makes this a "legacy cfg BAR"? Is there some spec that covers
> virtio BAR usage?
Yes. Virtio Legacy interface defines the PCI BAR number to use.
https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-14500010
> > + * epf_virtio_init - initialize struct epf_virtio and setup BAR for virtio
> > + * @evio: struct epf_virtio to initialize.
> > + * @hdr: pci configuration space to show remote host.
> > + * @bar_size: pci BAR size it depends on the virtio device type.
>
> s/pci/PCI/ (there were also a few more instances above in messages or
> comments)
>
> > + * epf_virtio_final - finalize struct epf_virtio. it frees bar and memories
> > + * @evio: struct epf_virtio to finalize.
>
> s/bar/BAR/
>
> > +static void epf_virtio_monitor_qnotify(struct epf_virtio *evio)
> > +{
> > + const u16 qn_default = evio->nvq;
> > + u16 tmp;
> > +
> > + /* Since there is no way to synchronize between the host and EP
> > functions,
> > + * it is possible to miss multiple notifications.
>
> Multi-line comment style.
>
> > + err = epf_virtio_negotiate_vq(evio);
> > + if (err < 0) {
> > + pr_err("failed to negoticate configs with driver\n");
>
> s/negoticate/negotiate/
>
> > + * epf_virtio_reset - reset virtio status
>
> Some of the function descriptions end with a period (".") and others
> don't. Please figure out what the most common style is and use that
> consistently.
I agree. I'll fix to be consistent.
> > + dst = pci_epc_map_addr(epf->epc, epf->func_no,
> > +epf->vfunc_no, dbase, ,
> > +slen);
> > + if (IS_ERR(dst)) {
> > + pr_err("failed to map pci mmoery spact to
> > local\n");
>
> s/pci/PCI/
> s/mmoery/memory/
> s/spact/space/ ?
>
> Also below.
>
> IIRC some previous messages started with a capital letter. Please
> make them all consistent.
Sure.
> > + if (dir == DMA_MEM_TO_DEV) {
> > + pci_epc_unmap_addr(epf->epc, epf->func_no,
> > +epf->vfunc_no, phys, dst, slen);
> > + } else {
> > + pci_epc_unmap_addr(epf->epc, epf->func_no,
> > +epf->vfunc_no, phys, src, slen);
> > + }
> > + }
> > +
> > + return 1;
>
> I guess this function returns either a negative error code or the
> value 1? That seems sort of weird (I think "negative error code or
> *zero* is more
Re: [RFC PATCH v2 1/3] PCI: endpoint: introduce a helper to implement pci ep virtio function
On Thu, Apr 27, 2023 at 6:44 PM Shunsuke Mie wrote:
>
> The Linux PCIe Endpoint framework supports to implement PCIe endpoint
> functions using a PCIe controller operating in endpoint mode.
> It is possble to realize the behavior of PCIe device, such as virtio PCI
> device. This patch introduces a setof helper functions and data structures
> to implement a PCIe endpoint function that behaves as a virtio device.
>
> Those functions enable the implementation PCIe endpoint function that
> comply with the virtio lecacy specification. Because modern virtio
> specifications require devices to implement custom PCIe capabilities, which
> are not currently supported by either PCIe controllers/drivers or the PCIe
> endpoint framework.
>
> The helper functions work with the new struct epf_virtio, which is
> initialized and finalized using the following functions:
>
> - int epf_virtio_init();
> - void epf_virtio_final()
>
> Once initialized, the PCIe configuration space can be read and written
> using the following functions:
>
> - epf_virtio_cfg_{read,write}#size()
> - epf_virtio_cfg_{set,clear}#size()
> - epf_virtio_cfg_memcpy_toio()
>
> The size is supported 8, 16 and 32bits. The content of configuration space
> varies depending on the type of virtio device.
>
> After the setup, launch the kernel thread for negotiation with host virtio
> drivers and detection virtqueue notifications with the function:
>
> - epf_virtio_launch_bgtask()
>
> Also there are functions to shutdown and reset the kthread.
>
> - epf_virtio_terminate_bgtask()
> - epf_virtio_terminate_reset()
>
> Data transfer function is also provide.
>
> - epf_virtio_memcpy_kiov2kiov()
>
> While this patch provides functions for negotiating with host drivers and
> copying data, each PCIe function driver must impl ement operations that
> depend on each specific device, such as network, block, etc.
>
> Signed-off-by: Shunsuke Mie
> ---
>
> Changes from v2:
> - Improve the memcpy function between kiov and kiov on local ram and
> remote ram via pcie bus.
>
> drivers/pci/endpoint/functions/Kconfig| 7 +
> drivers/pci/endpoint/functions/Makefile | 1 +
> .../pci/endpoint/functions/pci-epf-virtio.c | 458 ++
> .../pci/endpoint/functions/pci-epf-virtio.h | 126 +
> 4 files changed, 592 insertions(+)
> create mode 100644 drivers/pci/endpoint/functions/pci-epf-virtio.c
> create mode 100644 drivers/pci/endpoint/functions/pci-epf-virtio.h
>
> diff --git a/drivers/pci/endpoint/functions/Kconfig
> b/drivers/pci/endpoint/functions/Kconfig
> index 9fd560886871..fa1a6a569a8f 100644
> --- a/drivers/pci/endpoint/functions/Kconfig
> +++ b/drivers/pci/endpoint/functions/Kconfig
> @@ -37,3 +37,10 @@ config PCI_EPF_VNTB
> between PCI Root Port and PCIe Endpoint.
>
> If in doubt, say "N" to disable Endpoint NTB driver.
> +
> +config PCI_EPF_VIRTIO
> + tristate
> + depends on PCI_ENDPOINT
> + select VHOST_RING_IOMEM
> + help
> + Helpers to implement PCI virtio Endpoint function
> diff --git a/drivers/pci/endpoint/functions/Makefile
> b/drivers/pci/endpoint/functions/Makefile
> index 5c13001deaba..a96f127ce900 100644
> --- a/drivers/pci/endpoint/functions/Makefile
> +++ b/drivers/pci/endpoint/functions/Makefile
> @@ -6,3 +6,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
> diff --git a/drivers/pci/endpoint/functions/pci-epf-virtio.c
> b/drivers/pci/endpoint/functions/pci-epf-virtio.c
> new file mode 100644
> index ..f67610dd247d
> --- /dev/null
> +++ b/drivers/pci/endpoint/functions/pci-epf-virtio.c
> @@ -0,0 +1,458 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Helpers to implement PCIe virtio EP function.
> + */
> +#include
> +#include
> +#include
> +
> +#include "pci-epf-virtio.h"
> +
> +static void epf_virtio_unmap_vq(struct pci_epf *epf, void __iomem *vq_virt,
> + phys_addr_t vq_phys, unsigned int num)
> +{
> + size_t vq_size = vring_size(num, VIRTIO_PCI_VRING_ALIGN);
> +
> + pci_epc_unmap_addr(epf->epc, epf->func_no, epf->vfunc_no, vq_phys,
> + vq_virt, vq_size);
> + pci_epc_mem_free_addr(epf->epc, vq_phys, vq_virt, vq_size);
> +}
> +
> +static void __iomem *epf_virtio_map_vq(struct pci_epf *epf,
> + phys_addr_t vq_pci_addr,
> + unsigned int num, phys_addr_t *vq_phys)
> +{
> + int err;
> + size_t vq_size;
> + void __iomem *vq_virt;
> +
> + vq_size = vring_size(num, VIRTIO_PCI_VRING_ALIGN);
> +
> + vq_virt = pci_epc_map_addr(epf->epc, epf->func_no, epf->vfunc_no,
> + vq_pci_addr, vq_phys, vq_size);
> + if (IS_ERR(vq_virt)) {
> +
Re: [RFC PATCH v2 1/3] PCI: endpoint: introduce a helper to implement pci ep virtio function
Simple typos, don't repost until there's more substantive feedback.
On Thu, Apr 27, 2023 at 07:44:26PM +0900, Shunsuke Mie wrote:
> The Linux PCIe Endpoint framework supports to implement PCIe endpoint
> functions using a PCIe controller operating in endpoint mode.
> It is possble to realize the behavior of PCIe device, such as virtio PCI
> device. This patch introduces a setof helper functions and data structures
> to implement a PCIe endpoint function that behaves as a virtio device.
s/possble/possible/
s/setof/set of/
> Those functions enable the implementation PCIe endpoint function that
> comply with the virtio lecacy specification. Because modern virtio
> specifications require devices to implement custom PCIe capabilities, which
> are not currently supported by either PCIe controllers/drivers or the PCIe
> endpoint framework.
s/implementation PCIe endpoint function/implementation of PCIe endpoint
functions/
s/lecacy/legacy/ (What does "legacy" mean? Is there a spec for this?)
I guess "legacy virtio" devices need not implement custom PCIe
capabilities, but "modern virtio" devices must implement them?
Capitalize "Endpoint framework" consistently; sometimes it's
"Endpoint", other times it's "endpoint".
> While this patch provides functions for negotiating with host drivers and
> copying data, each PCIe function driver must impl ement operations that
> depend on each specific device, such as network, block, etc.
s/impl ement/implement/
> +#include
> +#include
> +#include
Typically the header includes would be alphabetized if possible.
> + vq_virt = pci_epc_map_addr(epf->epc, epf->func_no, epf->vfunc_no,
> +vq_pci_addr, vq_phys, vq_size);
> + if (IS_ERR(vq_virt)) {
> + pr_err("Failed to map virtuqueue to local");
s/virtuqueue/virtqueue/
I know you probably don't have any way to use dev_err(), but this
message really needs some context, like a driver name and instance or
something.
> +#define VIRTIO_PCI_LEGACY_CFG_BAR 0
What makes this a "legacy cfg BAR"? Is there some spec that covers
virtio BAR usage?
> + * epf_virtio_init - initialize struct epf_virtio and setup BAR for virtio
> + * @evio: struct epf_virtio to initialize.
> + * @hdr: pci configuration space to show remote host.
> + * @bar_size: pci BAR size it depends on the virtio device type.
s/pci/PCI/ (there were also a few more instances above in messages or
comments)
> + * epf_virtio_final - finalize struct epf_virtio. it frees bar and memories
> + * @evio: struct epf_virtio to finalize.
s/bar/BAR/
> +static void epf_virtio_monitor_qnotify(struct epf_virtio *evio)
> +{
> + const u16 qn_default = evio->nvq;
> + u16 tmp;
> +
> + /* Since there is no way to synchronize between the host and EP
> functions,
> + * it is possible to miss multiple notifications.
Multi-line comment style.
> + err = epf_virtio_negotiate_vq(evio);
> + if (err < 0) {
> + pr_err("failed to negoticate configs with driver\n");
s/negoticate/negotiate/
> + * epf_virtio_reset - reset virtio status
Some of the function descriptions end with a period (".") and others
don't. Please figure out what the most common style is and use that
consistently.
> + dst = pci_epc_map_addr(epf->epc, epf->func_no,
> +epf->vfunc_no, dbase, ,
> +slen);
> + if (IS_ERR(dst)) {
> + pr_err("failed to map pci mmoery spact to
> local\n");
s/pci/PCI/
s/mmoery/memory/
s/spact/space/ ?
Also below.
IIRC some previous messages started with a capital letter. Please
make them all consistent.
> + if (dir == DMA_MEM_TO_DEV) {
> + pci_epc_unmap_addr(epf->epc, epf->func_no,
> +epf->vfunc_no, phys, dst, slen);
> + } else {
> + pci_epc_unmap_addr(epf->epc, epf->func_no,
> +epf->vfunc_no, phys, src, slen);
> + }
> + }
> +
> + return 1;
I guess this function returns either a negative error code or the
value 1? That seems sort of weird (I think "negative error code or
*zero* is more typical), but maybe you're following some convention?
> +#include
> +#include
> +#include
> +#include
Alpha order if possible
> + /* Virtual address of pci configuration space */
s/pci/PCI/
> + /* Callback function and parameter for queue notifcation
> + * Note: PCI EP function cannot detect qnotify accurately, therefore
> this
> + * callback function should check all of virtqueue's changes.
> + */
Multi-line comment style.
Bjorn
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH v2 1/3] PCI: endpoint: introduce a helper to implement pci ep virtio function
The Linux PCIe Endpoint framework supports to implement PCIe endpoint
functions using a PCIe controller operating in endpoint mode.
It is possble to realize the behavior of PCIe device, such as virtio PCI
device. This patch introduces a setof helper functions and data structures
to implement a PCIe endpoint function that behaves as a virtio device.
Those functions enable the implementation PCIe endpoint function that
comply with the virtio lecacy specification. Because modern virtio
specifications require devices to implement custom PCIe capabilities, which
are not currently supported by either PCIe controllers/drivers or the PCIe
endpoint framework.
The helper functions work with the new struct epf_virtio, which is
initialized and finalized using the following functions:
- int epf_virtio_init();
- void epf_virtio_final()
Once initialized, the PCIe configuration space can be read and written
using the following functions:
- epf_virtio_cfg_{read,write}#size()
- epf_virtio_cfg_{set,clear}#size()
- epf_virtio_cfg_memcpy_toio()
The size is supported 8, 16 and 32bits. The content of configuration space
varies depending on the type of virtio device.
After the setup, launch the kernel thread for negotiation with host virtio
drivers and detection virtqueue notifications with the function:
- epf_virtio_launch_bgtask()
Also there are functions to shutdown and reset the kthread.
- epf_virtio_terminate_bgtask()
- epf_virtio_terminate_reset()
Data transfer function is also provide.
- epf_virtio_memcpy_kiov2kiov()
While this patch provides functions for negotiating with host drivers and
copying data, each PCIe function driver must impl ement operations that
depend on each specific device, such as network, block, etc.
Signed-off-by: Shunsuke Mie
---
Changes from v2:
- Improve the memcpy function between kiov and kiov on local ram and
remote ram via pcie bus.
drivers/pci/endpoint/functions/Kconfig| 7 +
drivers/pci/endpoint/functions/Makefile | 1 +
.../pci/endpoint/functions/pci-epf-virtio.c | 458 ++
.../pci/endpoint/functions/pci-epf-virtio.h | 126 +
4 files changed, 592 insertions(+)
create mode 100644 drivers/pci/endpoint/functions/pci-epf-virtio.c
create mode 100644 drivers/pci/endpoint/functions/pci-epf-virtio.h
diff --git a/drivers/pci/endpoint/functions/Kconfig
b/drivers/pci/endpoint/functions/Kconfig
index 9fd560886871..fa1a6a569a8f 100644
--- a/drivers/pci/endpoint/functions/Kconfig
+++ b/drivers/pci/endpoint/functions/Kconfig
@@ -37,3 +37,10 @@ config PCI_EPF_VNTB
between PCI Root Port and PCIe Endpoint.
If in doubt, say "N" to disable Endpoint NTB driver.
+
+config PCI_EPF_VIRTIO
+ tristate
+ depends on PCI_ENDPOINT
+ select VHOST_RING_IOMEM
+ help
+ Helpers to implement PCI virtio Endpoint function
diff --git a/drivers/pci/endpoint/functions/Makefile
b/drivers/pci/endpoint/functions/Makefile
index 5c13001deaba..a96f127ce900 100644
--- a/drivers/pci/endpoint/functions/Makefile
+++ b/drivers/pci/endpoint/functions/Makefile
@@ -6,3 +6,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
diff --git a/drivers/pci/endpoint/functions/pci-epf-virtio.c
b/drivers/pci/endpoint/functions/pci-epf-virtio.c
new file mode 100644
index ..f67610dd247d
--- /dev/null
+++ b/drivers/pci/endpoint/functions/pci-epf-virtio.c
@@ -0,0 +1,458 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Helpers to implement PCIe virtio EP function.
+ */
+#include
+#include
+#include
+
+#include "pci-epf-virtio.h"
+
+static void epf_virtio_unmap_vq(struct pci_epf *epf, void __iomem *vq_virt,
+ phys_addr_t vq_phys, unsigned int num)
+{
+ size_t vq_size = vring_size(num, VIRTIO_PCI_VRING_ALIGN);
+
+ pci_epc_unmap_addr(epf->epc, epf->func_no, epf->vfunc_no, vq_phys,
+ vq_virt, vq_size);
+ pci_epc_mem_free_addr(epf->epc, vq_phys, vq_virt, vq_size);
+}
+
+static void __iomem *epf_virtio_map_vq(struct pci_epf *epf,
+ phys_addr_t vq_pci_addr,
+ unsigned int num, phys_addr_t *vq_phys)
+{
+ int err;
+ size_t vq_size;
+ void __iomem *vq_virt;
+
+ vq_size = vring_size(num, VIRTIO_PCI_VRING_ALIGN);
+
+ vq_virt = pci_epc_map_addr(epf->epc, epf->func_no, epf->vfunc_no,
+ vq_pci_addr, vq_phys, vq_size);
+ if (IS_ERR(vq_virt)) {
+ pr_err("Failed to map virtuqueue to local");
+ goto err_free;
+ }
+
+ return vq_virt;
+
+err_free:
+ pci_epc_mem_free_addr(epf->epc, *vq_phys, vq_virt, vq_size);
+
+ return ERR_PTR(err);
+}
+
+static void epf_virtio_free_vringh(struct pci_epf *epf, struct
