All of lore.kernel.org
 help / color / mirror / Atom feed
* [virtio-dev] [PATCH v1 0/2] transport-pci: Introduce legacy registers access using AQ
@ 2023-05-03  3:26 ` Parav Pandit
  0 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03  3:26 UTC (permalink / raw)
  To: mst, virtio-dev, cohuck, david.edmondson
  Cc: sburla, jasowang, virtio-comment, shahafs, Parav Pandit

This patch introduces legacy registers access commands for the owner
group member PCI PF to access the legacy registers of the member VFs.

If in future any SIOV devices to support legacy registers, they
can be easily supported using same commands by using the group
member identifiers of the future SIOV devices.

More details as overview, motivation, use case are further described
below.

Patch summary:
--------------
patch-1 adds administrative virtuqueue commands
patch-2 adds its conformance section

This short series is on top of [1].
It uses the newly introduced administrative virtqueue facility with 3 new
opcodes and uses the existing virtio_admin_cmd.

It is expected to go another rebase once v13 for [1] is rolled out and merged.

[1] https://lore.kernel.org/virtio-comment/cover.1682354275.git.mst@redhat.com/T/#t

Usecase:
--------
1. A hypervisor/system needs to provide transitional
   virtio devices to the guest VM at scale of thousands,
   typically, one to eight devices per VM.

2. A hypervisor/system needs to provide such devices using a
   vendor agnostic driver in the hypervisor system.

3. A hypervisor system prefers to have single stack regardless of
   virtio device type (net/blk) and be future compatible with a
   single vfio stack using SR-IOV or other scalable device
   virtualization technology to map PCI devices to the guest VM.
   (as transitional or otherwise)

Motivation/Background:
----------------------
The existing virtio transitional PCI device is missing support for
PCI SR-IOV based devices. Currently it does not work beyond
PCI PF, or as software emulated device in reality. Currently it
has below cited system level limitations:

[a] PCIe spec citation:
VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.

[b] cpu arch citiation:
Intel 64 and IA-32 Architectures Software Developer’s Manual:
The processor’s I/O address space is separate and distinct from
the physical-memory address space. The I/O address space consists
of 64K individually addressable 8-bit I/O ports, numbered 0 through FFFFH.

[c] PCIe spec citation:
If a bridge implements an I/O address range,...I/O address range will be
aligned to a 4 KB boundary.

Above usecase requirements can be solved by PCI PF group owner enabling
the access to its group member PCI VFs legacy registers using an admin
virtqueue of the group owner PCI PF.

Software usage example:
-----------------------
The most common way to use and map to the guest VM is by
using vfio driver framework in Linux kernel.

                +----------------------+
                |pci_dev_id = 0x100X   |
+---------------|pci_rev_id = 0x0      |-----+
|vfio device    |BAR0 = I/O region     |     |
|               |Other attributes      |     |
|               +----------------------+     |
|                                            |
+   +--------------+     +-----------------+ |
|   |I/O BAR to AQ |     | Other vfio      | |
|   |rd/wr mapper  |     | functionalities | |
|   +--------------+     +-----------------+ |
|                                            |
+------+-------------------------+-----------+
       |                         |
  +----+------------+       +----+------------+
  | +-----+         |       | PCI VF device A |
  | | AQ  |-------------+---->+-------------+ |
  | +-----+         |   |   | | legacy regs | |
  | PCI PF device   |   |   | +-------------+ |
  +-----------------+   |   +-----------------+
                        |
                        |   +----+------------+
                        |   | PCI VF device N |
                        +---->+-------------+ |
                            | | legacy regs | |
                            | +-------------+ |
                            +-----------------+

2. Virtio pci driver to bind to the listed device id and
   use it as native device in the host.

3. Use it in a light weight hypervisor to run bare-metal OS.

Please review.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v0->v1:
- addressed comments, suggesetions and ideas from Michael Tsirkin and Jason Wang
- far more simpler design than MMR access
- removed complexities of MMR device ids
- removed complexities of MMR registers and extended capabilities
- dropped adding new extended capabilities because if if they are
  added, a pci device still needs to have existing capabilities
  in the legacy configuration space and hypervisor driver do not
  need to access them


Parav Pandit (2):
  transport-pci: Introduce legacy registers access commands
  transport-pci: Add legacy register access conformance section

 admin.tex                 |   5 +-
 conformance.tex           |   2 +
 transport-pci-vf-regs.tex | 115 ++++++++++++++++++++++++++++++++++++++
 transport-pci.tex         |   2 +
 4 files changed, 123 insertions(+), 1 deletion(-)
 create mode 100644 transport-pci-vf-regs.tex

-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] [PATCH v1 0/2] transport-pci: Introduce legacy registers access using AQ
@ 2023-05-03  3:26 ` Parav Pandit
  0 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03  3:26 UTC (permalink / raw)
  To: mst, virtio-dev, cohuck, david.edmondson
  Cc: sburla, jasowang, virtio-comment, shahafs, Parav Pandit

This patch introduces legacy registers access commands for the owner
group member PCI PF to access the legacy registers of the member VFs.

If in future any SIOV devices to support legacy registers, they
can be easily supported using same commands by using the group
member identifiers of the future SIOV devices.

More details as overview, motivation, use case are further described
below.

Patch summary:
--------------
patch-1 adds administrative virtuqueue commands
patch-2 adds its conformance section

This short series is on top of [1].
It uses the newly introduced administrative virtqueue facility with 3 new
opcodes and uses the existing virtio_admin_cmd.

It is expected to go another rebase once v13 for [1] is rolled out and merged.

[1] https://lore.kernel.org/virtio-comment/cover.1682354275.git.mst@redhat.com/T/#t

Usecase:
--------
1. A hypervisor/system needs to provide transitional
   virtio devices to the guest VM at scale of thousands,
   typically, one to eight devices per VM.

2. A hypervisor/system needs to provide such devices using a
   vendor agnostic driver in the hypervisor system.

3. A hypervisor system prefers to have single stack regardless of
   virtio device type (net/blk) and be future compatible with a
   single vfio stack using SR-IOV or other scalable device
   virtualization technology to map PCI devices to the guest VM.
   (as transitional or otherwise)

Motivation/Background:
----------------------
The existing virtio transitional PCI device is missing support for
PCI SR-IOV based devices. Currently it does not work beyond
PCI PF, or as software emulated device in reality. Currently it
has below cited system level limitations:

[a] PCIe spec citation:
VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.

[b] cpu arch citiation:
Intel 64 and IA-32 Architectures Software Developer’s Manual:
The processor’s I/O address space is separate and distinct from
the physical-memory address space. The I/O address space consists
of 64K individually addressable 8-bit I/O ports, numbered 0 through FFFFH.

[c] PCIe spec citation:
If a bridge implements an I/O address range,...I/O address range will be
aligned to a 4 KB boundary.

Above usecase requirements can be solved by PCI PF group owner enabling
the access to its group member PCI VFs legacy registers using an admin
virtqueue of the group owner PCI PF.

Software usage example:
-----------------------
The most common way to use and map to the guest VM is by
using vfio driver framework in Linux kernel.

                +----------------------+
                |pci_dev_id = 0x100X   |
+---------------|pci_rev_id = 0x0      |-----+
|vfio device    |BAR0 = I/O region     |     |
|               |Other attributes      |     |
|               +----------------------+     |
|                                            |
+   +--------------+     +-----------------+ |
|   |I/O BAR to AQ |     | Other vfio      | |
|   |rd/wr mapper  |     | functionalities | |
|   +--------------+     +-----------------+ |
|                                            |
+------+-------------------------+-----------+
       |                         |
  +----+------------+       +----+------------+
  | +-----+         |       | PCI VF device A |
  | | AQ  |-------------+---->+-------------+ |
  | +-----+         |   |   | | legacy regs | |
  | PCI PF device   |   |   | +-------------+ |
  +-----------------+   |   +-----------------+
                        |
                        |   +----+------------+
                        |   | PCI VF device N |
                        +---->+-------------+ |
                            | | legacy regs | |
                            | +-------------+ |
                            +-----------------+

2. Virtio pci driver to bind to the listed device id and
   use it as native device in the host.

3. Use it in a light weight hypervisor to run bare-metal OS.

Please review.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v0->v1:
- addressed comments, suggesetions and ideas from Michael Tsirkin and Jason Wang
- far more simpler design than MMR access
- removed complexities of MMR device ids
- removed complexities of MMR registers and extended capabilities
- dropped adding new extended capabilities because if if they are
  added, a pci device still needs to have existing capabilities
  in the legacy configuration space and hypervisor driver do not
  need to access them


Parav Pandit (2):
  transport-pci: Introduce legacy registers access commands
  transport-pci: Add legacy register access conformance section

 admin.tex                 |   5 +-
 conformance.tex           |   2 +
 transport-pci-vf-regs.tex | 115 ++++++++++++++++++++++++++++++++++++++
 transport-pci.tex         |   2 +
 4 files changed, 123 insertions(+), 1 deletion(-)
 create mode 100644 transport-pci-vf-regs.tex

-- 
2.26.2


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-03  3:26 ` [virtio-comment] " Parav Pandit
@ 2023-05-03  3:26   ` Parav Pandit
  -1 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03  3:26 UTC (permalink / raw)
  To: mst, virtio-dev, cohuck, david.edmondson
  Cc: sburla, jasowang, virtio-comment, shahafs, Parav Pandit

This patch introduces legacy registers access commands for the owner
group member PCI PF to access the legacy registers of the member VFs.

If in future any SIOV devices to support legacy registers, they
can be easily supported using same commands by using the group
member identifiers of the future SIOV devices.

More details as overview, motivation, use case are further described
below.

Usecase:
--------
1. A hypervisor/system needs to provide transitional
   virtio devices to the guest VM at scale of thousands,
   typically, one to eight devices per VM.

2. A hypervisor/system needs to provide such devices using a
   vendor agnostic driver in the hypervisor system.

3. A hypervisor system prefers to have single stack regardless of
   virtio device type (net/blk) and be future compatible with a
   single vfio stack using SR-IOV or other scalable device
   virtualization technology to map PCI devices to the guest VM.
   (as transitional or otherwise)

Motivation/Background:
----------------------
The existing virtio transitional PCI device is missing support for
PCI SR-IOV based devices. Currently it does not work beyond
PCI PF, or as software emulated device in reality. Currently it
has below cited system level limitations:

[a] PCIe spec citation:
VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.

[b] cpu arch citiation:
Intel 64 and IA-32 Architectures Software Developer’s Manual:
The processor’s I/O address space is separate and distinct from
the physical-memory address space. The I/O address space consists
of 64K individually addressable 8-bit I/O ports, numbered 0 through FFFFH.

[c] PCIe spec citation:
If a bridge implements an I/O address range,...I/O address range will be
aligned to a 4 KB boundary.

Above usecase requirements can be solved by PCI PF group owner enabling
the access to its group member PCI VFs legacy registers using an admin
virtqueue of the group owner PCI PF.

Software usage example:
-----------------------
The most common way to use and map to the guest VM is by
using vfio driver framework in Linux kernel.

                +----------------------+
                |pci_dev_id = 0x100X   |
+---------------|pci_rev_id = 0x0      |-----+
|vfio device    |BAR0 = I/O region     |     |
|               |Other attributes      |     |
|               +----------------------+     |
|                                            |
+   +--------------+     +-----------------+ |
|   |I/O BAR to AQ |     | Other vfio      | |
|   |rd/wr mapper  |     | functionalities | |
|   +--------------+     +-----------------+ |
|                                            |
+------+-------------------------+-----------+
       |                         |
  +----+------------+       +----+------------+
  | +-----+         |       | PCI VF device A |
  | | AQ  |-------------+---->+-------------+ |
  | +-----+         |   |   | | legacy regs | |
  | PCI PF device   |   |   | +-------------+ |
  +-----------------+   |   +-----------------+
                        |
                        |   +----+------------+
                        |   | PCI VF device N |
                        +---->+-------------+ |
                            | | legacy regs | |
                            | +-------------+ |
                            +-----------------+

2. Virtio pci driver to bind to the listed device id and
   use it as native device in the host.

3. Use it in a light weight hypervisor to run bare-metal OS.

Please review.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v0->v1:
- addressed comments, suggesetions and ideas from Michael Tsirkin and Jason Wang
- far more simpler design than MMR access
- removed complexities of MMR device ids
- removed complexities of MMR registers and extended capabilities
- dropped adding new extended capabilities because if if they are
  added, a pci device still needs to have existing capabilities
  in the legacy configuration space and hypervisor driver do not
  need to access them
---
 admin.tex                 |  5 ++-
 transport-pci-vf-regs.tex | 84 +++++++++++++++++++++++++++++++++++++++
 transport-pci.tex         |  2 +
 3 files changed, 90 insertions(+), 1 deletion(-)
 create mode 100644 transport-pci-vf-regs.tex

diff --git a/admin.tex b/admin.tex
index 648253c..852ee04 100644
--- a/admin.tex
+++ b/admin.tex
@@ -115,7 +115,10 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 \hline \hline
 0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
 0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
-0x0002 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
+0x0002 & VIRTIO_ADMIN_CMD_LREG_WRITE & Write legacy registers of a member device    \\
+0x0003 & VIRTIO_ADMIN_CMD_LREG_READ & Read legacy registers of a member device    \\
+0x0004 & VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY & Read the queue notification offset for legacy interface \\
+0x0005 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
 \hline
 0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
 \hline
diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
new file mode 100644
index 0000000..16ced32
--- /dev/null
+++ b/transport-pci-vf-regs.tex
@@ -0,0 +1,84 @@
+\subsection{SR-IOV VFs Legacy Registers Access}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
+
+As described in PCIe base specification \hyperref[intro:PCIe]{[PCIe]} PCI VFs
+do not support IOBAR. A PCI PF device can optionally enable driver to access
+its member PCI VFs devices legacy common configuration and device configuration
+registers using an administration virtqueue. A PCI PF group owner device that
+supports its member VFs legacy registers access via the administration
+virtqueue should supports following commands.
+
+\begin{enumerate}
+\item Legacy Registers Write
+\item Legacy Registers Read
+\item Legacy Queue Notify Offset Query
+\end{enumerate}
+
+\subsubsection{Legacy Registers Write}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Registers Write}
+
+Legacy registers write admin command follows \field{struct virtio_admin_cmd}.
+This command writes legacy registers of a member VF device. Driver should write
+appropriate register \field{size} depending on the width of the legacy
+common registers or device specific registers.
+Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_WRITE.
+Driver sets \field{group_type} to 1 for VFs.
+Driver sets \field{group_member_id} to a valid VF number.
+
+The \field{command_specific_data} has following listed structure format:
+
+\begin{lstlisting}
+struct virtio_admin_cmd_lreg_wr_data {
+	u8 offset; /* Starting byte offset of the register(s) to write */
+	u8 size; /* Number of bytes to write into the register. */
+	u8 register[];
+};
+\end{lstlisting}
+
+This command does not have any command specific result.
+
+\subsubsection{Legacy Registers Read}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Registers Read}
+
+Legacy registers read admin command follows \field{struct virtio_admin_cmd}.
+This command reads legacy registers of a member VF device. Driver should write
+appropriate register \field{size} depending on the width of the legacy
+common configuration registers or device specific registers.
+Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_READ.
+Driver sets \field{group_type} to 1 for VFs.
+Driver sets \field{group_member_id} to a valid VF number.
+
+The \field{command_specific_data} has following listed structure format:
+
+\begin{lstlisting}
+struct virtio_admin_cmd_lreg_rd_data {
+	u8 offset; /* Starting byte offset of the register to read */
+	u8 size; /* Number of bytes to read from the registers */
+};
+\end{lstlisting}
+
+When command completes successfully, command result contains following
+listed content:
+
+\begin{lstlisting}
+struct virtio_admin_cmd_lreg_rd_result {
+	u8 registers[];
+};
+\end{lstlisting}
+
+\subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Queue Notify Offset Query}
+
+This command returns the notify offset of the member VF for queue
+notifications. This command follows \field{struct virtio_admin_cmd}.
+Driver sets command opcode \field{opcode} to VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY.
+There is no command specific data for this command.
+Driver sets \field{group_type} to 1.
+Driver sets \field{group_member_id} to a valid VF number.
+
+When command completes successfully, command result contains the queue
+notification address in the listed format:
+
+\begin{lstlisting}
+struct virtio_admin_cmd_lq_notify_query_result {
+	u8 bar; /* PCI BAR number of the member VF */
+	u8 reserved[7];
+	le64 offset; /* Byte offset within the BAR */
+};
+\end{lstlisting}
diff --git a/transport-pci.tex b/transport-pci.tex
index ff889d3..b187576 100644
--- a/transport-pci.tex
+++ b/transport-pci.tex
@@ -1179,3 +1179,5 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
         re-examine the configuration space to see what changed.
     \end{itemize}
 \end{itemize}
+
+\input{transport-pci-vf-regs.tex}
-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply related	[flat|nested] 54+ messages in thread

* [virtio-comment] [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-03  3:26   ` Parav Pandit
  0 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03  3:26 UTC (permalink / raw)
  To: mst, virtio-dev, cohuck, david.edmondson
  Cc: sburla, jasowang, virtio-comment, shahafs, Parav Pandit

This patch introduces legacy registers access commands for the owner
group member PCI PF to access the legacy registers of the member VFs.

If in future any SIOV devices to support legacy registers, they
can be easily supported using same commands by using the group
member identifiers of the future SIOV devices.

More details as overview, motivation, use case are further described
below.

Usecase:
--------
1. A hypervisor/system needs to provide transitional
   virtio devices to the guest VM at scale of thousands,
   typically, one to eight devices per VM.

2. A hypervisor/system needs to provide such devices using a
   vendor agnostic driver in the hypervisor system.

3. A hypervisor system prefers to have single stack regardless of
   virtio device type (net/blk) and be future compatible with a
   single vfio stack using SR-IOV or other scalable device
   virtualization technology to map PCI devices to the guest VM.
   (as transitional or otherwise)

Motivation/Background:
----------------------
The existing virtio transitional PCI device is missing support for
PCI SR-IOV based devices. Currently it does not work beyond
PCI PF, or as software emulated device in reality. Currently it
has below cited system level limitations:

[a] PCIe spec citation:
VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.

[b] cpu arch citiation:
Intel 64 and IA-32 Architectures Software Developer’s Manual:
The processor’s I/O address space is separate and distinct from
the physical-memory address space. The I/O address space consists
of 64K individually addressable 8-bit I/O ports, numbered 0 through FFFFH.

[c] PCIe spec citation:
If a bridge implements an I/O address range,...I/O address range will be
aligned to a 4 KB boundary.

Above usecase requirements can be solved by PCI PF group owner enabling
the access to its group member PCI VFs legacy registers using an admin
virtqueue of the group owner PCI PF.

Software usage example:
-----------------------
The most common way to use and map to the guest VM is by
using vfio driver framework in Linux kernel.

                +----------------------+
                |pci_dev_id = 0x100X   |
+---------------|pci_rev_id = 0x0      |-----+
|vfio device    |BAR0 = I/O region     |     |
|               |Other attributes      |     |
|               +----------------------+     |
|                                            |
+   +--------------+     +-----------------+ |
|   |I/O BAR to AQ |     | Other vfio      | |
|   |rd/wr mapper  |     | functionalities | |
|   +--------------+     +-----------------+ |
|                                            |
+------+-------------------------+-----------+
       |                         |
  +----+------------+       +----+------------+
  | +-----+         |       | PCI VF device A |
  | | AQ  |-------------+---->+-------------+ |
  | +-----+         |   |   | | legacy regs | |
  | PCI PF device   |   |   | +-------------+ |
  +-----------------+   |   +-----------------+
                        |
                        |   +----+------------+
                        |   | PCI VF device N |
                        +---->+-------------+ |
                            | | legacy regs | |
                            | +-------------+ |
                            +-----------------+

2. Virtio pci driver to bind to the listed device id and
   use it as native device in the host.

3. Use it in a light weight hypervisor to run bare-metal OS.

Please review.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v0->v1:
- addressed comments, suggesetions and ideas from Michael Tsirkin and Jason Wang
- far more simpler design than MMR access
- removed complexities of MMR device ids
- removed complexities of MMR registers and extended capabilities
- dropped adding new extended capabilities because if if they are
  added, a pci device still needs to have existing capabilities
  in the legacy configuration space and hypervisor driver do not
  need to access them
---
 admin.tex                 |  5 ++-
 transport-pci-vf-regs.tex | 84 +++++++++++++++++++++++++++++++++++++++
 transport-pci.tex         |  2 +
 3 files changed, 90 insertions(+), 1 deletion(-)
 create mode 100644 transport-pci-vf-regs.tex

diff --git a/admin.tex b/admin.tex
index 648253c..852ee04 100644
--- a/admin.tex
+++ b/admin.tex
@@ -115,7 +115,10 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 \hline \hline
 0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
 0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
-0x0002 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
+0x0002 & VIRTIO_ADMIN_CMD_LREG_WRITE & Write legacy registers of a member device    \\
+0x0003 & VIRTIO_ADMIN_CMD_LREG_READ & Read legacy registers of a member device    \\
+0x0004 & VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY & Read the queue notification offset for legacy interface \\
+0x0005 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
 \hline
 0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
 \hline
diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
new file mode 100644
index 0000000..16ced32
--- /dev/null
+++ b/transport-pci-vf-regs.tex
@@ -0,0 +1,84 @@
+\subsection{SR-IOV VFs Legacy Registers Access}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
+
+As described in PCIe base specification \hyperref[intro:PCIe]{[PCIe]} PCI VFs
+do not support IOBAR. A PCI PF device can optionally enable driver to access
+its member PCI VFs devices legacy common configuration and device configuration
+registers using an administration virtqueue. A PCI PF group owner device that
+supports its member VFs legacy registers access via the administration
+virtqueue should supports following commands.
+
+\begin{enumerate}
+\item Legacy Registers Write
+\item Legacy Registers Read
+\item Legacy Queue Notify Offset Query
+\end{enumerate}
+
+\subsubsection{Legacy Registers Write}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Registers Write}
+
+Legacy registers write admin command follows \field{struct virtio_admin_cmd}.
+This command writes legacy registers of a member VF device. Driver should write
+appropriate register \field{size} depending on the width of the legacy
+common registers or device specific registers.
+Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_WRITE.
+Driver sets \field{group_type} to 1 for VFs.
+Driver sets \field{group_member_id} to a valid VF number.
+
+The \field{command_specific_data} has following listed structure format:
+
+\begin{lstlisting}
+struct virtio_admin_cmd_lreg_wr_data {
+	u8 offset; /* Starting byte offset of the register(s) to write */
+	u8 size; /* Number of bytes to write into the register. */
+	u8 register[];
+};
+\end{lstlisting}
+
+This command does not have any command specific result.
+
+\subsubsection{Legacy Registers Read}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Registers Read}
+
+Legacy registers read admin command follows \field{struct virtio_admin_cmd}.
+This command reads legacy registers of a member VF device. Driver should write
+appropriate register \field{size} depending on the width of the legacy
+common configuration registers or device specific registers.
+Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_READ.
+Driver sets \field{group_type} to 1 for VFs.
+Driver sets \field{group_member_id} to a valid VF number.
+
+The \field{command_specific_data} has following listed structure format:
+
+\begin{lstlisting}
+struct virtio_admin_cmd_lreg_rd_data {
+	u8 offset; /* Starting byte offset of the register to read */
+	u8 size; /* Number of bytes to read from the registers */
+};
+\end{lstlisting}
+
+When command completes successfully, command result contains following
+listed content:
+
+\begin{lstlisting}
+struct virtio_admin_cmd_lreg_rd_result {
+	u8 registers[];
+};
+\end{lstlisting}
+
+\subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Queue Notify Offset Query}
+
+This command returns the notify offset of the member VF for queue
+notifications. This command follows \field{struct virtio_admin_cmd}.
+Driver sets command opcode \field{opcode} to VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY.
+There is no command specific data for this command.
+Driver sets \field{group_type} to 1.
+Driver sets \field{group_member_id} to a valid VF number.
+
+When command completes successfully, command result contains the queue
+notification address in the listed format:
+
+\begin{lstlisting}
+struct virtio_admin_cmd_lq_notify_query_result {
+	u8 bar; /* PCI BAR number of the member VF */
+	u8 reserved[7];
+	le64 offset; /* Byte offset within the BAR */
+};
+\end{lstlisting}
diff --git a/transport-pci.tex b/transport-pci.tex
index ff889d3..b187576 100644
--- a/transport-pci.tex
+++ b/transport-pci.tex
@@ -1179,3 +1179,5 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
         re-examine the configuration space to see what changed.
     \end{itemize}
 \end{itemize}
+
+\input{transport-pci-vf-regs.tex}
-- 
2.26.2


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply related	[flat|nested] 54+ messages in thread

* [virtio-dev] [PATCH v1 2/2] transport-pci: Add legacy register access conformance section
  2023-05-03  3:26 ` [virtio-comment] " Parav Pandit
@ 2023-05-03  3:26   ` Parav Pandit
  -1 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03  3:26 UTC (permalink / raw)
  To: mst, virtio-dev, cohuck, david.edmondson
  Cc: sburla, jasowang, virtio-comment, shahafs, Parav Pandit

Add device and driver conformanace section for legacy registers access
commands interface.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
 conformance.tex           |  2 ++
 transport-pci-vf-regs.tex | 31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/conformance.tex b/conformance.tex
index 01ccd69..dbd8cd6 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -109,6 +109,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
 \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
 \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
+\item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
 \end{itemize}
 
 \conformance{\subsection}{MMIO Driver Conformance}\label{sec:Conformance / Driver Conformance / MMIO Driver Conformance}
@@ -194,6 +195,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
 \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
 \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
+\item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
 \end{itemize}
 
 \conformance{\subsection}{MMIO Device Conformance}\label{sec:Conformance / Device Conformance / MMIO Device Conformance}
diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
index 16ced32..7d0574b 100644
--- a/transport-pci-vf-regs.tex
+++ b/transport-pci-vf-regs.tex
@@ -82,3 +82,34 @@ \subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Opti
 	le64 offset; /* Byte offset within the BAR */
 };
 \end{lstlisting}
+
+\devicenormative{\paragraph}{SR-IOV VFs Legacy Registers Access}{Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
+
+If the PCI PF device supports legacy registers access, it SHOULD set
+corresponding bits for commands VIRTIO_ADMIN_CMD_LREG_WRITE,
+VIRTIO_ADMIN_CMD_LREG_READ and VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY in 
+command result of VIRTIO_ADMIN_CMD_LIST_QUERY in
+\field{device_admin_cmd_opcodes}.
+
+The device MUST support legacy configuration registers to its defined width.
+
+The device MAY fail legacy configuration registers access when either the 
+access is for an incorrct register width or if the register offset is incorrect.
+
+The device MUST allow access of one or multiple bytes of the registers when
+such register is defined as byte array, for example \field{mac} of \field{struct
+virtio_net_config} of the Network Device.
+
+\drivernormative{\paragraph}{SR-IOV VFs Legacy Registers Access}{Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
+
+The driver MUST access legacy configuration registers to its defined width.
+
+The driver MUST access device specific registers to its defined width
+
+The driver MAY access one or multiple bytes of the device specific registers
+which are defined as byte array, for example \field{mac} of \field{struct
+virtio_net_config} of the Network Device.
+
+The driver SHOULD access all the bytes of a device specific registers in a
+single access when accessing a byte array, for example \field{mac} of
+\field{struct virtio_net_config} of the Network Device.
-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply related	[flat|nested] 54+ messages in thread

* [virtio-comment] [PATCH v1 2/2] transport-pci: Add legacy register access conformance section
@ 2023-05-03  3:26   ` Parav Pandit
  0 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03  3:26 UTC (permalink / raw)
  To: mst, virtio-dev, cohuck, david.edmondson
  Cc: sburla, jasowang, virtio-comment, shahafs, Parav Pandit

Add device and driver conformanace section for legacy registers access
commands interface.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
 conformance.tex           |  2 ++
 transport-pci-vf-regs.tex | 31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/conformance.tex b/conformance.tex
index 01ccd69..dbd8cd6 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -109,6 +109,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
 \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
 \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
+\item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
 \end{itemize}
 
 \conformance{\subsection}{MMIO Driver Conformance}\label{sec:Conformance / Driver Conformance / MMIO Driver Conformance}
@@ -194,6 +195,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
 \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
 \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
+\item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
 \end{itemize}
 
 \conformance{\subsection}{MMIO Device Conformance}\label{sec:Conformance / Device Conformance / MMIO Device Conformance}
diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
index 16ced32..7d0574b 100644
--- a/transport-pci-vf-regs.tex
+++ b/transport-pci-vf-regs.tex
@@ -82,3 +82,34 @@ \subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Opti
 	le64 offset; /* Byte offset within the BAR */
 };
 \end{lstlisting}
+
+\devicenormative{\paragraph}{SR-IOV VFs Legacy Registers Access}{Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
+
+If the PCI PF device supports legacy registers access, it SHOULD set
+corresponding bits for commands VIRTIO_ADMIN_CMD_LREG_WRITE,
+VIRTIO_ADMIN_CMD_LREG_READ and VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY in 
+command result of VIRTIO_ADMIN_CMD_LIST_QUERY in
+\field{device_admin_cmd_opcodes}.
+
+The device MUST support legacy configuration registers to its defined width.
+
+The device MAY fail legacy configuration registers access when either the 
+access is for an incorrct register width or if the register offset is incorrect.
+
+The device MUST allow access of one or multiple bytes of the registers when
+such register is defined as byte array, for example \field{mac} of \field{struct
+virtio_net_config} of the Network Device.
+
+\drivernormative{\paragraph}{SR-IOV VFs Legacy Registers Access}{Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
+
+The driver MUST access legacy configuration registers to its defined width.
+
+The driver MUST access device specific registers to its defined width
+
+The driver MAY access one or multiple bytes of the device specific registers
+which are defined as byte array, for example \field{mac} of \field{struct
+virtio_net_config} of the Network Device.
+
+The driver SHOULD access all the bytes of a device specific registers in a
+single access when accessing a byte array, for example \field{mac} of
+\field{struct virtio_net_config} of the Network Device.
-- 
2.26.2


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply related	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-03  3:26   ` [virtio-comment] " Parav Pandit
@ 2023-05-03  5:42     ` Michael S. Tsirkin
  -1 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03  5:42 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs

On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
> This patch introduces legacy registers access commands for the owner
> group member PCI PF to access the legacy registers of the member VFs.
> 
> If in future any SIOV devices to support legacy registers, they
> can be easily supported using same commands by using the group
> member identifiers of the future SIOV devices.
> 
> More details as overview, motivation, use case are further described
> below.
> 
> Usecase:
> --------
> 1. A hypervisor/system needs to provide transitional
>    virtio devices to the guest VM at scale of thousands,
>    typically, one to eight devices per VM.
> 
> 2. A hypervisor/system needs to provide such devices using a
>    vendor agnostic driver in the hypervisor system.
> 
> 3. A hypervisor system prefers to have single stack regardless of
>    virtio device type (net/blk) and be future compatible with a
>    single vfio stack using SR-IOV or other scalable device
>    virtualization technology to map PCI devices to the guest VM.
>    (as transitional or otherwise)
> 
> Motivation/Background:
> ----------------------
> The existing virtio transitional PCI device is missing support for
> PCI SR-IOV based devices. Currently it does not work beyond
> PCI PF, or as software emulated device in reality. Currently it
> has below cited system level limitations:
> 
> [a] PCIe spec citation:
> VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.
> 
> [b] cpu arch citiation:
> Intel 64 and IA-32 Architectures Software Developer’s Manual:
> The processor’s I/O address space is separate and distinct from
> the physical-memory address space. The I/O address space consists
> of 64K individually addressable 8-bit I/O ports, numbered 0 through FFFFH.
> 
> [c] PCIe spec citation:
> If a bridge implements an I/O address range,...I/O address range will be
> aligned to a 4 KB boundary.
> 
> Above usecase requirements can be solved by PCI PF group owner enabling
> the access to its group member PCI VFs legacy registers using an admin
> virtqueue of the group owner PCI PF.
> 
> Software usage example:
> -----------------------
> The most common way to use and map to the guest VM is by
> using vfio driver framework in Linux kernel.
> 
>                 +----------------------+
>                 |pci_dev_id = 0x100X   |
> +---------------|pci_rev_id = 0x0      |-----+
> |vfio device    |BAR0 = I/O region     |     |
> |               |Other attributes      |     |
> |               +----------------------+     |
> |                                            |
> +   +--------------+     +-----------------+ |
> |   |I/O BAR to AQ |     | Other vfio      | |
> |   |rd/wr mapper  |     | functionalities | |
> |   +--------------+     +-----------------+ |
> |                                            |
> +------+-------------------------+-----------+
>        |                         |
>   +----+------------+       +----+------------+
>   | +-----+         |       | PCI VF device A |
>   | | AQ  |-------------+---->+-------------+ |
>   | +-----+         |   |   | | legacy regs | |
>   | PCI PF device   |   |   | +-------------+ |
>   +-----------------+   |   +-----------------+
>                         |
>                         |   +----+------------+
>                         |   | PCI VF device N |
>                         +---->+-------------+ |
>                             | | legacy regs | |
>                             | +-------------+ |
>                             +-----------------+
> 
> 2. Virtio pci driver to bind to the listed device id and
>    use it as native device in the host.
> 
> 3. Use it in a light weight hypervisor to run bare-metal OS.
> 
> Please review.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> Signed-off-by: Parav Pandit <parav@nvidia.com>


A bunch of grammar mistakes below. We have actual interface to figure
out so I didn't bother correcting but pls try to run this through some
checker. The one in microsoft word is actually not bad :)

> ---
> changelog:
> v0->v1:
> - addressed comments, suggesetions and ideas from Michael Tsirkin and Jason Wang
> - far more simpler design than MMR access
> - removed complexities of MMR device ids
> - removed complexities of MMR registers and extended capabilities
> - dropped adding new extended capabilities because if if they are
>   added, a pci device still needs to have existing capabilities
>   in the legacy configuration space and hypervisor driver do not
>   need to access them
> ---
>  admin.tex                 |  5 ++-
>  transport-pci-vf-regs.tex | 84 +++++++++++++++++++++++++++++++++++++++
>  transport-pci.tex         |  2 +
>  3 files changed, 90 insertions(+), 1 deletion(-)
>  create mode 100644 transport-pci-vf-regs.tex
> 
> diff --git a/admin.tex b/admin.tex
> index 648253c..852ee04 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -115,7 +115,10 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>  \hline \hline
>  0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
>  0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
> -0x0002 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
> +0x0002 & VIRTIO_ADMIN_CMD_LREG_WRITE & Write legacy registers of a member device    \\
> +0x0003 & VIRTIO_ADMIN_CMD_LREG_READ & Read legacy registers of a member device    \\
> +0x0004 & VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY & Read the queue notification offset for legacy interface \\
> +0x0005 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
>  \hline
>  0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
>  \hline
> diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
> new file mode 100644
> index 0000000..16ced32
> --- /dev/null
> +++ b/transport-pci-vf-regs.tex

I'd like the name to reflect "legacy". Also I don't think this has
to be SRIOV generally. It's just legacy PCI over admin commands.
Except for virtio_admin_cmd_lq_notify_query_result
which refers to PCI? But that
one I can't say for sure what it does.


> @@ -0,0 +1,84 @@
> +\subsection{SR-IOV VFs Legacy Registers Access}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> +
> +As described in PCIe base specification \hyperref[intro:PCIe]{[PCIe]} PCI VFs
> +do not support IOBAR. A PCI PF device can optionally enable driver to access
> +its member PCI VFs devices legacy common configuration and device configuration
> +registers using an administration virtqueue. A PCI PF group owner device that
> +supports its member VFs legacy registers access via the administration
> +virtqueue should supports following commands.

As above. It actually can work for any group if we want to.


> +
> +\begin{enumerate}
> +\item Legacy Registers Write
> +\item Legacy Registers Read
> +\item Legacy Queue Notify Offset Query
> +\end{enumerate}
> +

Pls add some theory of operation. How can all this be used?

> +\subsubsection{Legacy Registers Write}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Registers Write}
> +
> +Legacy registers write admin command follows \field{struct virtio_admin_cmd}.
> +This command writes legacy registers of a member VF device. Driver should write
> +appropriate register \field{size} depending on the width of the legacy
> +common registers or device specific registers.
> +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_WRITE.
> +Driver sets \field{group_type} to 1 for VFs.
> +Driver sets \field{group_member_id} to a valid VF number.
> +
> +The \field{command_specific_data} has following listed structure format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_wr_data {
> +	u8 offset; /* Starting byte offset of the register(s) to write */
> +	u8 size; /* Number of bytes to write into the register. */
> +	u8 register[];
> +};
> +\end{lstlisting}
> +
> +This command does not have any command specific result.
> +
> +\subsubsection{Legacy Registers Read}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Registers Read}
> +
> +Legacy registers read admin command follows \field{struct virtio_admin_cmd}.
> +This command reads legacy registers of a member VF device. Driver should write
> +appropriate register \field{size} depending on the width of the legacy
> +common configuration registers or device specific registers.
> +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_READ.
> +Driver sets \field{group_type} to 1 for VFs.
> +Driver sets \field{group_member_id} to a valid VF number.
> +
> +The \field{command_specific_data} has following listed structure format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_rd_data {
> +	u8 offset; /* Starting byte offset of the register to read */
> +	u8 size; /* Number of bytes to read from the registers */
> +};
> +\end{lstlisting}
> +
> +When command completes successfully, command result contains following
> +listed content:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_rd_result {
> +	u8 registers[];
> +};
> +\end{lstlisting}
> +
> +\subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Queue Notify Offset Query}
> +
> +This command returns the notify offset of the member VF for queue
> +notifications.

What is this notify offset? It's never explained.

> This command follows \field{struct virtio_admin_cmd}.
> +Driver sets command opcode \field{opcode} to VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY.
> +There is no command specific data for this command.
> +Driver sets \field{group_type} to 1.
> +Driver sets \field{group_member_id} to a valid VF number.

I think ATM the limitation for this is that the member must be a pci
device, otherwise BAR is not well defined. We will have to
find a way to extend this for SIOV. But that is all, please do
not repeat documentation about virtio_admin_cmd header, we have that in
a central place.

> +
> +When command completes successfully, command result contains the queue
> +notification address in the listed format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lq_notify_query_result {
> +	u8 bar; /* PCI BAR number of the member VF */
> +	u8 reserved[7];
> +	le64 offset; /* Byte offset within the BAR */
> +};
> +\end{lstlisting}
> diff --git a/transport-pci.tex b/transport-pci.tex
> index ff889d3..b187576 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -1179,3 +1179,5 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
>          re-examine the configuration space to see what changed.
>      \end{itemize}
>  \end{itemize}
> +
> +\input{transport-pci-vf-regs.tex}

As simple as it is, I feel this falls far short of describing how
a device should operate.
Some issues:
	- legacy device config offset changes as msi is enabled/disabled
	  suggest separate commands for device/common config
	- legacy device endian-ness changes with guest
	  suggest commands to enable LE and BE mode
	- legacy guests often assume INT#x support
	  suggest a way to tunnel that too;
	  though supporting ISR is going to be a challenge :(
	- I presume admin command is not the way to do kicks? Or is it ok?
	- there's some kind of notify thing here?

I expected to see more statements along the lines of
        command ABC has the same effect as access
        to register DEF of the member through the legacy pci interface




> -- 
> 2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-03  5:42     ` Michael S. Tsirkin
  0 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03  5:42 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs

On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
> This patch introduces legacy registers access commands for the owner
> group member PCI PF to access the legacy registers of the member VFs.
> 
> If in future any SIOV devices to support legacy registers, they
> can be easily supported using same commands by using the group
> member identifiers of the future SIOV devices.
> 
> More details as overview, motivation, use case are further described
> below.
> 
> Usecase:
> --------
> 1. A hypervisor/system needs to provide transitional
>    virtio devices to the guest VM at scale of thousands,
>    typically, one to eight devices per VM.
> 
> 2. A hypervisor/system needs to provide such devices using a
>    vendor agnostic driver in the hypervisor system.
> 
> 3. A hypervisor system prefers to have single stack regardless of
>    virtio device type (net/blk) and be future compatible with a
>    single vfio stack using SR-IOV or other scalable device
>    virtualization technology to map PCI devices to the guest VM.
>    (as transitional or otherwise)
> 
> Motivation/Background:
> ----------------------
> The existing virtio transitional PCI device is missing support for
> PCI SR-IOV based devices. Currently it does not work beyond
> PCI PF, or as software emulated device in reality. Currently it
> has below cited system level limitations:
> 
> [a] PCIe spec citation:
> VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.
> 
> [b] cpu arch citiation:
> Intel 64 and IA-32 Architectures Software Developer’s Manual:
> The processor’s I/O address space is separate and distinct from
> the physical-memory address space. The I/O address space consists
> of 64K individually addressable 8-bit I/O ports, numbered 0 through FFFFH.
> 
> [c] PCIe spec citation:
> If a bridge implements an I/O address range,...I/O address range will be
> aligned to a 4 KB boundary.
> 
> Above usecase requirements can be solved by PCI PF group owner enabling
> the access to its group member PCI VFs legacy registers using an admin
> virtqueue of the group owner PCI PF.
> 
> Software usage example:
> -----------------------
> The most common way to use and map to the guest VM is by
> using vfio driver framework in Linux kernel.
> 
>                 +----------------------+
>                 |pci_dev_id = 0x100X   |
> +---------------|pci_rev_id = 0x0      |-----+
> |vfio device    |BAR0 = I/O region     |     |
> |               |Other attributes      |     |
> |               +----------------------+     |
> |                                            |
> +   +--------------+     +-----------------+ |
> |   |I/O BAR to AQ |     | Other vfio      | |
> |   |rd/wr mapper  |     | functionalities | |
> |   +--------------+     +-----------------+ |
> |                                            |
> +------+-------------------------+-----------+
>        |                         |
>   +----+------------+       +----+------------+
>   | +-----+         |       | PCI VF device A |
>   | | AQ  |-------------+---->+-------------+ |
>   | +-----+         |   |   | | legacy regs | |
>   | PCI PF device   |   |   | +-------------+ |
>   +-----------------+   |   +-----------------+
>                         |
>                         |   +----+------------+
>                         |   | PCI VF device N |
>                         +---->+-------------+ |
>                             | | legacy regs | |
>                             | +-------------+ |
>                             +-----------------+
> 
> 2. Virtio pci driver to bind to the listed device id and
>    use it as native device in the host.
> 
> 3. Use it in a light weight hypervisor to run bare-metal OS.
> 
> Please review.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> Signed-off-by: Parav Pandit <parav@nvidia.com>


A bunch of grammar mistakes below. We have actual interface to figure
out so I didn't bother correcting but pls try to run this through some
checker. The one in microsoft word is actually not bad :)

> ---
> changelog:
> v0->v1:
> - addressed comments, suggesetions and ideas from Michael Tsirkin and Jason Wang
> - far more simpler design than MMR access
> - removed complexities of MMR device ids
> - removed complexities of MMR registers and extended capabilities
> - dropped adding new extended capabilities because if if they are
>   added, a pci device still needs to have existing capabilities
>   in the legacy configuration space and hypervisor driver do not
>   need to access them
> ---
>  admin.tex                 |  5 ++-
>  transport-pci-vf-regs.tex | 84 +++++++++++++++++++++++++++++++++++++++
>  transport-pci.tex         |  2 +
>  3 files changed, 90 insertions(+), 1 deletion(-)
>  create mode 100644 transport-pci-vf-regs.tex
> 
> diff --git a/admin.tex b/admin.tex
> index 648253c..852ee04 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -115,7 +115,10 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>  \hline \hline
>  0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
>  0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
> -0x0002 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
> +0x0002 & VIRTIO_ADMIN_CMD_LREG_WRITE & Write legacy registers of a member device    \\
> +0x0003 & VIRTIO_ADMIN_CMD_LREG_READ & Read legacy registers of a member device    \\
> +0x0004 & VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY & Read the queue notification offset for legacy interface \\
> +0x0005 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
>  \hline
>  0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
>  \hline
> diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
> new file mode 100644
> index 0000000..16ced32
> --- /dev/null
> +++ b/transport-pci-vf-regs.tex

I'd like the name to reflect "legacy". Also I don't think this has
to be SRIOV generally. It's just legacy PCI over admin commands.
Except for virtio_admin_cmd_lq_notify_query_result
which refers to PCI? But that
one I can't say for sure what it does.


> @@ -0,0 +1,84 @@
> +\subsection{SR-IOV VFs Legacy Registers Access}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> +
> +As described in PCIe base specification \hyperref[intro:PCIe]{[PCIe]} PCI VFs
> +do not support IOBAR. A PCI PF device can optionally enable driver to access
> +its member PCI VFs devices legacy common configuration and device configuration
> +registers using an administration virtqueue. A PCI PF group owner device that
> +supports its member VFs legacy registers access via the administration
> +virtqueue should supports following commands.

As above. It actually can work for any group if we want to.


> +
> +\begin{enumerate}
> +\item Legacy Registers Write
> +\item Legacy Registers Read
> +\item Legacy Queue Notify Offset Query
> +\end{enumerate}
> +

Pls add some theory of operation. How can all this be used?

> +\subsubsection{Legacy Registers Write}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Registers Write}
> +
> +Legacy registers write admin command follows \field{struct virtio_admin_cmd}.
> +This command writes legacy registers of a member VF device. Driver should write
> +appropriate register \field{size} depending on the width of the legacy
> +common registers or device specific registers.
> +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_WRITE.
> +Driver sets \field{group_type} to 1 for VFs.
> +Driver sets \field{group_member_id} to a valid VF number.
> +
> +The \field{command_specific_data} has following listed structure format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_wr_data {
> +	u8 offset; /* Starting byte offset of the register(s) to write */
> +	u8 size; /* Number of bytes to write into the register. */
> +	u8 register[];
> +};
> +\end{lstlisting}
> +
> +This command does not have any command specific result.
> +
> +\subsubsection{Legacy Registers Read}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Registers Read}
> +
> +Legacy registers read admin command follows \field{struct virtio_admin_cmd}.
> +This command reads legacy registers of a member VF device. Driver should write
> +appropriate register \field{size} depending on the width of the legacy
> +common configuration registers or device specific registers.
> +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_READ.
> +Driver sets \field{group_type} to 1 for VFs.
> +Driver sets \field{group_member_id} to a valid VF number.
> +
> +The \field{command_specific_data} has following listed structure format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_rd_data {
> +	u8 offset; /* Starting byte offset of the register to read */
> +	u8 size; /* Number of bytes to read from the registers */
> +};
> +\end{lstlisting}
> +
> +When command completes successfully, command result contains following
> +listed content:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_rd_result {
> +	u8 registers[];
> +};
> +\end{lstlisting}
> +
> +\subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Queue Notify Offset Query}
> +
> +This command returns the notify offset of the member VF for queue
> +notifications.

What is this notify offset? It's never explained.

> This command follows \field{struct virtio_admin_cmd}.
> +Driver sets command opcode \field{opcode} to VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY.
> +There is no command specific data for this command.
> +Driver sets \field{group_type} to 1.
> +Driver sets \field{group_member_id} to a valid VF number.

I think ATM the limitation for this is that the member must be a pci
device, otherwise BAR is not well defined. We will have to
find a way to extend this for SIOV. But that is all, please do
not repeat documentation about virtio_admin_cmd header, we have that in
a central place.

> +
> +When command completes successfully, command result contains the queue
> +notification address in the listed format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lq_notify_query_result {
> +	u8 bar; /* PCI BAR number of the member VF */
> +	u8 reserved[7];
> +	le64 offset; /* Byte offset within the BAR */
> +};
> +\end{lstlisting}
> diff --git a/transport-pci.tex b/transport-pci.tex
> index ff889d3..b187576 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -1179,3 +1179,5 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
>          re-examine the configuration space to see what changed.
>      \end{itemize}
>  \end{itemize}
> +
> +\input{transport-pci-vf-regs.tex}

As simple as it is, I feel this falls far short of describing how
a device should operate.
Some issues:
	- legacy device config offset changes as msi is enabled/disabled
	  suggest separate commands for device/common config
	- legacy device endian-ness changes with guest
	  suggest commands to enable LE and BE mode
	- legacy guests often assume INT#x support
	  suggest a way to tunnel that too;
	  though supporting ISR is going to be a challenge :(
	- I presume admin command is not the way to do kicks? Or is it ok?
	- there's some kind of notify thing here?

I expected to see more statements along the lines of
        command ABC has the same effect as access
        to register DEF of the member through the legacy pci interface




> -- 
> 2.26.2


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 2/2] transport-pci: Add legacy register access conformance section
  2023-05-03  3:26   ` [virtio-comment] " Parav Pandit
@ 2023-05-03  5:48     ` Michael S. Tsirkin
  -1 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03  5:48 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs

On Wed, May 03, 2023 at 06:26:59AM +0300, Parav Pandit wrote:
> Add device and driver conformanace section for legacy registers access
> commands interface.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> ---
>  conformance.tex           |  2 ++
>  transport-pci-vf-regs.tex | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/conformance.tex b/conformance.tex
> index 01ccd69..dbd8cd6 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -109,6 +109,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
>  \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
>  \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> +\item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
>  \end{itemize}
>  
>  \conformance{\subsection}{MMIO Driver Conformance}\label{sec:Conformance / Driver Conformance / MMIO Driver Conformance}
> @@ -194,6 +195,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
>  \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
>  \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> +\item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
>  \end{itemize}
>  
>  \conformance{\subsection}{MMIO Device Conformance}\label{sec:Conformance / Device Conformance / MMIO Device Conformance}
> diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
> index 16ced32..7d0574b 100644
> --- a/transport-pci-vf-regs.tex
> +++ b/transport-pci-vf-regs.tex
> @@ -82,3 +82,34 @@ \subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Opti
>  	le64 offset; /* Byte offset within the BAR */
>  };
>  \end{lstlisting}
> +
> +\devicenormative{\paragraph}{SR-IOV VFs Legacy Registers Access}{Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> +
> +If the PCI PF device supports legacy registers access, it SHOULD set
> +corresponding bits for commands VIRTIO_ADMIN_CMD_LREG_WRITE,
> +VIRTIO_ADMIN_CMD_LREG_READ and VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY in 
> +command result of VIRTIO_ADMIN_CMD_LIST_QUERY in
> +\field{device_admin_cmd_opcodes}.

Don't repeat documentation of VIRTIO_ADMIN_CMD_LIST_QUERY please.
just say all these must be supported.
In fact what are you saying here? That all 3 are supported
or none at all? What about just
VIRTIO_ADMIN_CMD_LREG_READ/VIRTIO_ADMIN_CMD_LREG_WRITE?
Looks like a slower but working way to do notifications
is through a common config write, no?

> +
> +The device MUST support legacy configuration registers to its defined width.

what is this?

> +
> +The device MAY fail legacy configuration registers access when either the 
> +access is for an incorrct register width or if the register offset is incorrect.

with which error code?

> +
> +The device MUST allow access of one or multiple bytes of the registers when
> +such register is defined as byte array, for example \field{mac} of \field{struct
> +virtio_net_config} of the Network Device.

so which accesses need to be supported then?

> +
> +\drivernormative{\paragraph}{SR-IOV VFs Legacy Registers Access}{Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> +
> +The driver MUST access legacy configuration registers to its defined width.
> +
> +The driver MUST access device specific registers to its defined width
> +
> +The driver MAY access one or multiple bytes of the device specific registers
> +which are defined as byte array, for example \field{mac} of \field{struct
> +virtio_net_config} of the Network Device.
> +
> +The driver SHOULD access all the bytes of a device specific registers in a
> +single access when accessing a byte array, for example \field{mac} of
> +\field{struct virtio_net_config} of the Network Device.
> -- 
> 2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 2/2] transport-pci: Add legacy register access conformance section
@ 2023-05-03  5:48     ` Michael S. Tsirkin
  0 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03  5:48 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs

On Wed, May 03, 2023 at 06:26:59AM +0300, Parav Pandit wrote:
> Add device and driver conformanace section for legacy registers access
> commands interface.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> ---
>  conformance.tex           |  2 ++
>  transport-pci-vf-regs.tex | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/conformance.tex b/conformance.tex
> index 01ccd69..dbd8cd6 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -109,6 +109,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
>  \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
>  \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> +\item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
>  \end{itemize}
>  
>  \conformance{\subsection}{MMIO Driver Conformance}\label{sec:Conformance / Driver Conformance / MMIO Driver Conformance}
> @@ -194,6 +195,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
>  \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
>  \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> +\item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
>  \end{itemize}
>  
>  \conformance{\subsection}{MMIO Device Conformance}\label{sec:Conformance / Device Conformance / MMIO Device Conformance}
> diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
> index 16ced32..7d0574b 100644
> --- a/transport-pci-vf-regs.tex
> +++ b/transport-pci-vf-regs.tex
> @@ -82,3 +82,34 @@ \subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Opti
>  	le64 offset; /* Byte offset within the BAR */
>  };
>  \end{lstlisting}
> +
> +\devicenormative{\paragraph}{SR-IOV VFs Legacy Registers Access}{Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> +
> +If the PCI PF device supports legacy registers access, it SHOULD set
> +corresponding bits for commands VIRTIO_ADMIN_CMD_LREG_WRITE,
> +VIRTIO_ADMIN_CMD_LREG_READ and VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY in 
> +command result of VIRTIO_ADMIN_CMD_LIST_QUERY in
> +\field{device_admin_cmd_opcodes}.

Don't repeat documentation of VIRTIO_ADMIN_CMD_LIST_QUERY please.
just say all these must be supported.
In fact what are you saying here? That all 3 are supported
or none at all? What about just
VIRTIO_ADMIN_CMD_LREG_READ/VIRTIO_ADMIN_CMD_LREG_WRITE?
Looks like a slower but working way to do notifications
is through a common config write, no?

> +
> +The device MUST support legacy configuration registers to its defined width.

what is this?

> +
> +The device MAY fail legacy configuration registers access when either the 
> +access is for an incorrct register width or if the register offset is incorrect.

with which error code?

> +
> +The device MUST allow access of one or multiple bytes of the registers when
> +such register is defined as byte array, for example \field{mac} of \field{struct
> +virtio_net_config} of the Network Device.

so which accesses need to be supported then?

> +
> +\drivernormative{\paragraph}{SR-IOV VFs Legacy Registers Access}{Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> +
> +The driver MUST access legacy configuration registers to its defined width.
> +
> +The driver MUST access device specific registers to its defined width
> +
> +The driver MAY access one or multiple bytes of the device specific registers
> +which are defined as byte array, for example \field{mac} of \field{struct
> +virtio_net_config} of the Network Device.
> +
> +The driver SHOULD access all the bytes of a device specific registers in a
> +single access when accessing a byte array, for example \field{mac} of
> +\field{struct virtio_net_config} of the Network Device.
> -- 
> 2.26.2


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-03  3:26   ` [virtio-comment] " Parav Pandit
@ 2023-05-03  5:50     ` Michael S. Tsirkin
  -1 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03  5:50 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs

On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
> This patch introduces legacy registers access commands for the owner
> group member PCI PF to access the legacy registers of the member VFs.
> 
> If in future any SIOV devices to support legacy registers, they
> can be easily supported using same commands by using the group
> member identifiers of the future SIOV devices.
> 
> More details as overview, motivation, use case are further described
> below.
> 
> Usecase:
> --------
> 1. A hypervisor/system needs to provide transitional
>    virtio devices to the guest VM at scale of thousands,
>    typically, one to eight devices per VM.
> 
> 2. A hypervisor/system needs to provide such devices using a
>    vendor agnostic driver in the hypervisor system.
> 
> 3. A hypervisor system prefers to have single stack regardless of
>    virtio device type (net/blk) and be future compatible with a
>    single vfio stack using SR-IOV or other scalable device
>    virtualization technology to map PCI devices to the guest VM.
>    (as transitional or otherwise)
> 
> Motivation/Background:
> ----------------------
> The existing virtio transitional PCI device is missing support for
> PCI SR-IOV based devices. Currently it does not work beyond
> PCI PF, or as software emulated device in reality. Currently it
> has below cited system level limitations:
> 
> [a] PCIe spec citation:
> VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.
> 
> [b] cpu arch citiation:
> Intel 64 and IA-32 Architectures Software Developer’s Manual:
> The processor’s I/O address space is separate and distinct from
> the physical-memory address space. The I/O address space consists
> of 64K individually addressable 8-bit I/O ports, numbered 0 through FFFFH.
> 
> [c] PCIe spec citation:
> If a bridge implements an I/O address range,...I/O address range will be
> aligned to a 4 KB boundary.
> 
> Above usecase requirements can be solved by PCI PF group owner enabling
> the access to its group member PCI VFs legacy registers using an admin
> virtqueue of the group owner PCI PF.
> 
> Software usage example:
> -----------------------
> The most common way to use and map to the guest VM is by
> using vfio driver framework in Linux kernel.
> 
>                 +----------------------+
>                 |pci_dev_id = 0x100X   |
> +---------------|pci_rev_id = 0x0      |-----+
> |vfio device    |BAR0 = I/O region     |     |
> |               |Other attributes      |     |
> |               +----------------------+     |
> |                                            |
> +   +--------------+     +-----------------+ |
> |   |I/O BAR to AQ |     | Other vfio      | |
> |   |rd/wr mapper  |     | functionalities | |
> |   +--------------+     +-----------------+ |
> |                                            |
> +------+-------------------------+-----------+
>        |                         |
>   +----+------------+       +----+------------+
>   | +-----+         |       | PCI VF device A |
>   | | AQ  |-------------+---->+-------------+ |
>   | +-----+         |   |   | | legacy regs | |
>   | PCI PF device   |   |   | +-------------+ |
>   +-----------------+   |   +-----------------+
>                         |
>                         |   +----+------------+
>                         |   | PCI VF device N |
>                         +---->+-------------+ |
>                             | | legacy regs | |
>                             | +-------------+ |
>                             +-----------------+
> 
> 2. Virtio pci driver to bind to the listed device id and
>    use it as native device in the host.
> 
> 3. Use it in a light weight hypervisor to run bare-metal OS.
> 
> Please review.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> 
> ---
> changelog:
> v0->v1:
> - addressed comments, suggesetions and ideas from Michael Tsirkin and Jason Wang
> - far more simpler design than MMR access
> - removed complexities of MMR device ids
> - removed complexities of MMR registers and extended capabilities
> - dropped adding new extended capabilities because if if they are
>   added, a pci device still needs to have existing capabilities
>   in the legacy configuration space and hypervisor driver do not
>   need to access them
> ---
>  admin.tex                 |  5 ++-
>  transport-pci-vf-regs.tex | 84 +++++++++++++++++++++++++++++++++++++++
>  transport-pci.tex         |  2 +
>  3 files changed, 90 insertions(+), 1 deletion(-)
>  create mode 100644 transport-pci-vf-regs.tex
> 
> diff --git a/admin.tex b/admin.tex
> index 648253c..852ee04 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -115,7 +115,10 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>  \hline \hline
>  0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
>  0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
> -0x0002 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
> +0x0002 & VIRTIO_ADMIN_CMD_LREG_WRITE & Write legacy registers of a member device    \\
> +0x0003 & VIRTIO_ADMIN_CMD_LREG_READ & Read legacy registers of a member device    \\
> +0x0004 & VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY & Read the queue notification offset for legacy interface \\
> +0x0005 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
>  \hline
>  0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
>  \hline
> diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
> new file mode 100644
> index 0000000..16ced32
> --- /dev/null
> +++ b/transport-pci-vf-regs.tex
> @@ -0,0 +1,84 @@
> +\subsection{SR-IOV VFs Legacy Registers Access}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> +
> +As described in PCIe base specification \hyperref[intro:PCIe]{[PCIe]} PCI VFs
> +do not support IOBAR. A PCI PF device can optionally enable driver to access
> +its member PCI VFs devices legacy common configuration and device configuration
> +registers using an administration virtqueue. A PCI PF group owner device that
> +supports its member VFs legacy registers access via the administration
> +virtqueue should supports following commands.
> +
> +\begin{enumerate}
> +\item Legacy Registers Write
> +\item Legacy Registers Read
> +\item Legacy Queue Notify Offset Query
> +\end{enumerate}
> +
> +\subsubsection{Legacy Registers Write}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Registers Write}
> +
> +Legacy registers write admin command follows \field{struct virtio_admin_cmd}.
> +This command writes legacy registers of a member VF device. Driver should write
> +appropriate register \field{size} depending on the width of the legacy
> +common registers or device specific registers.
> +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_WRITE.
> +Driver sets \field{group_type} to 1 for VFs.
> +Driver sets \field{group_member_id} to a valid VF number.
> +
> +The \field{command_specific_data} has following listed structure format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_wr_data {
> +	u8 offset; /* Starting byte offset of the register(s) to write */
> +	u8 size; /* Number of bytes to write into the register. */
> +	u8 register[];

And maybe add
	u8 reserved[]; /* structure padding to multiple of 8 bytes */

> +};
> +\end{lstlisting}
> +
> +This command does not have any command specific result.
> +
> +\subsubsection{Legacy Registers Read}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Registers Read}
> +
> +Legacy registers read admin command follows \field{struct virtio_admin_cmd}.
> +This command reads legacy registers of a member VF device. Driver should write
> +appropriate register \field{size} depending on the width of the legacy
> +common configuration registers or device specific registers.
> +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_READ.
> +Driver sets \field{group_type} to 1 for VFs.
> +Driver sets \field{group_member_id} to a valid VF number.
> +
> +The \field{command_specific_data} has following listed structure format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_rd_data {
> +	u8 offset; /* Starting byte offset of the register to read */
> +	u8 size; /* Number of bytes to read from the registers */
> +};
> +\end{lstlisting}
> +
> +When command completes successfully, command result contains following
> +listed content:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_rd_result {
> +	u8 registers[];
> +};
> +\end{lstlisting}
> +
> +\subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Queue Notify Offset Query}
> +
> +This command returns the notify offset of the member VF for queue
> +notifications. This command follows \field{struct virtio_admin_cmd}.
> +Driver sets command opcode \field{opcode} to VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY.
> +There is no command specific data for this command.
> +Driver sets \field{group_type} to 1.
> +Driver sets \field{group_member_id} to a valid VF number.
> +
> +When command completes successfully, command result contains the queue
> +notification address in the listed format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lq_notify_query_result {
> +	u8 bar; /* PCI BAR number of the member VF */
> +	u8 reserved[7];
> +	le64 offset; /* Byte offset within the BAR */
> +};
> +\end{lstlisting}
> diff --git a/transport-pci.tex b/transport-pci.tex
> index ff889d3..b187576 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -1179,3 +1179,5 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
>          re-examine the configuration space to see what changed.
>      \end{itemize}
>  \end{itemize}
> +
> +\input{transport-pci-vf-regs.tex}
> -- 
> 2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-03  5:50     ` Michael S. Tsirkin
  0 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03  5:50 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs

On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
> This patch introduces legacy registers access commands for the owner
> group member PCI PF to access the legacy registers of the member VFs.
> 
> If in future any SIOV devices to support legacy registers, they
> can be easily supported using same commands by using the group
> member identifiers of the future SIOV devices.
> 
> More details as overview, motivation, use case are further described
> below.
> 
> Usecase:
> --------
> 1. A hypervisor/system needs to provide transitional
>    virtio devices to the guest VM at scale of thousands,
>    typically, one to eight devices per VM.
> 
> 2. A hypervisor/system needs to provide such devices using a
>    vendor agnostic driver in the hypervisor system.
> 
> 3. A hypervisor system prefers to have single stack regardless of
>    virtio device type (net/blk) and be future compatible with a
>    single vfio stack using SR-IOV or other scalable device
>    virtualization technology to map PCI devices to the guest VM.
>    (as transitional or otherwise)
> 
> Motivation/Background:
> ----------------------
> The existing virtio transitional PCI device is missing support for
> PCI SR-IOV based devices. Currently it does not work beyond
> PCI PF, or as software emulated device in reality. Currently it
> has below cited system level limitations:
> 
> [a] PCIe spec citation:
> VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.
> 
> [b] cpu arch citiation:
> Intel 64 and IA-32 Architectures Software Developer’s Manual:
> The processor’s I/O address space is separate and distinct from
> the physical-memory address space. The I/O address space consists
> of 64K individually addressable 8-bit I/O ports, numbered 0 through FFFFH.
> 
> [c] PCIe spec citation:
> If a bridge implements an I/O address range,...I/O address range will be
> aligned to a 4 KB boundary.
> 
> Above usecase requirements can be solved by PCI PF group owner enabling
> the access to its group member PCI VFs legacy registers using an admin
> virtqueue of the group owner PCI PF.
> 
> Software usage example:
> -----------------------
> The most common way to use and map to the guest VM is by
> using vfio driver framework in Linux kernel.
> 
>                 +----------------------+
>                 |pci_dev_id = 0x100X   |
> +---------------|pci_rev_id = 0x0      |-----+
> |vfio device    |BAR0 = I/O region     |     |
> |               |Other attributes      |     |
> |               +----------------------+     |
> |                                            |
> +   +--------------+     +-----------------+ |
> |   |I/O BAR to AQ |     | Other vfio      | |
> |   |rd/wr mapper  |     | functionalities | |
> |   +--------------+     +-----------------+ |
> |                                            |
> +------+-------------------------+-----------+
>        |                         |
>   +----+------------+       +----+------------+
>   | +-----+         |       | PCI VF device A |
>   | | AQ  |-------------+---->+-------------+ |
>   | +-----+         |   |   | | legacy regs | |
>   | PCI PF device   |   |   | +-------------+ |
>   +-----------------+   |   +-----------------+
>                         |
>                         |   +----+------------+
>                         |   | PCI VF device N |
>                         +---->+-------------+ |
>                             | | legacy regs | |
>                             | +-------------+ |
>                             +-----------------+
> 
> 2. Virtio pci driver to bind to the listed device id and
>    use it as native device in the host.
> 
> 3. Use it in a light weight hypervisor to run bare-metal OS.
> 
> Please review.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> 
> ---
> changelog:
> v0->v1:
> - addressed comments, suggesetions and ideas from Michael Tsirkin and Jason Wang
> - far more simpler design than MMR access
> - removed complexities of MMR device ids
> - removed complexities of MMR registers and extended capabilities
> - dropped adding new extended capabilities because if if they are
>   added, a pci device still needs to have existing capabilities
>   in the legacy configuration space and hypervisor driver do not
>   need to access them
> ---
>  admin.tex                 |  5 ++-
>  transport-pci-vf-regs.tex | 84 +++++++++++++++++++++++++++++++++++++++
>  transport-pci.tex         |  2 +
>  3 files changed, 90 insertions(+), 1 deletion(-)
>  create mode 100644 transport-pci-vf-regs.tex
> 
> diff --git a/admin.tex b/admin.tex
> index 648253c..852ee04 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -115,7 +115,10 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>  \hline \hline
>  0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
>  0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
> -0x0002 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
> +0x0002 & VIRTIO_ADMIN_CMD_LREG_WRITE & Write legacy registers of a member device    \\
> +0x0003 & VIRTIO_ADMIN_CMD_LREG_READ & Read legacy registers of a member device    \\
> +0x0004 & VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY & Read the queue notification offset for legacy interface \\
> +0x0005 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
>  \hline
>  0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
>  \hline
> diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
> new file mode 100644
> index 0000000..16ced32
> --- /dev/null
> +++ b/transport-pci-vf-regs.tex
> @@ -0,0 +1,84 @@
> +\subsection{SR-IOV VFs Legacy Registers Access}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> +
> +As described in PCIe base specification \hyperref[intro:PCIe]{[PCIe]} PCI VFs
> +do not support IOBAR. A PCI PF device can optionally enable driver to access
> +its member PCI VFs devices legacy common configuration and device configuration
> +registers using an administration virtqueue. A PCI PF group owner device that
> +supports its member VFs legacy registers access via the administration
> +virtqueue should supports following commands.
> +
> +\begin{enumerate}
> +\item Legacy Registers Write
> +\item Legacy Registers Read
> +\item Legacy Queue Notify Offset Query
> +\end{enumerate}
> +
> +\subsubsection{Legacy Registers Write}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Registers Write}
> +
> +Legacy registers write admin command follows \field{struct virtio_admin_cmd}.
> +This command writes legacy registers of a member VF device. Driver should write
> +appropriate register \field{size} depending on the width of the legacy
> +common registers or device specific registers.
> +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_WRITE.
> +Driver sets \field{group_type} to 1 for VFs.
> +Driver sets \field{group_member_id} to a valid VF number.
> +
> +The \field{command_specific_data} has following listed structure format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_wr_data {
> +	u8 offset; /* Starting byte offset of the register(s) to write */
> +	u8 size; /* Number of bytes to write into the register. */
> +	u8 register[];

And maybe add
	u8 reserved[]; /* structure padding to multiple of 8 bytes */

> +};
> +\end{lstlisting}
> +
> +This command does not have any command specific result.
> +
> +\subsubsection{Legacy Registers Read}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Registers Read}
> +
> +Legacy registers read admin command follows \field{struct virtio_admin_cmd}.
> +This command reads legacy registers of a member VF device. Driver should write
> +appropriate register \field{size} depending on the width of the legacy
> +common configuration registers or device specific registers.
> +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_READ.
> +Driver sets \field{group_type} to 1 for VFs.
> +Driver sets \field{group_member_id} to a valid VF number.
> +
> +The \field{command_specific_data} has following listed structure format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_rd_data {
> +	u8 offset; /* Starting byte offset of the register to read */
> +	u8 size; /* Number of bytes to read from the registers */
> +};
> +\end{lstlisting}
> +
> +When command completes successfully, command result contains following
> +listed content:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_rd_result {
> +	u8 registers[];
> +};
> +\end{lstlisting}
> +
> +\subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Queue Notify Offset Query}
> +
> +This command returns the notify offset of the member VF for queue
> +notifications. This command follows \field{struct virtio_admin_cmd}.
> +Driver sets command opcode \field{opcode} to VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY.
> +There is no command specific data for this command.
> +Driver sets \field{group_type} to 1.
> +Driver sets \field{group_member_id} to a valid VF number.
> +
> +When command completes successfully, command result contains the queue
> +notification address in the listed format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lq_notify_query_result {
> +	u8 bar; /* PCI BAR number of the member VF */
> +	u8 reserved[7];
> +	le64 offset; /* Byte offset within the BAR */
> +};
> +\end{lstlisting}
> diff --git a/transport-pci.tex b/transport-pci.tex
> index ff889d3..b187576 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -1179,3 +1179,5 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
>          re-examine the configuration space to see what changed.
>      \end{itemize}
>  \end{itemize}
> +
> +\input{transport-pci-vf-regs.tex}
> -- 
> 2.26.2


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 0/2] transport-pci: Introduce legacy registers access using AQ
  2023-05-03  3:26 ` [virtio-comment] " Parav Pandit
@ 2023-05-03 10:16   ` Michael S. Tsirkin
  -1 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03 10:16 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs

On Wed, May 03, 2023 at 06:26:57AM +0300, Parav Pandit wrote:
> This patch introduces legacy registers access commands for the owner
> group member PCI PF to access the legacy registers of the member VFs.
> 
> If in future any SIOV devices to support legacy registers, they
> can be easily supported using same commands by using the group
> member identifiers of the future SIOV devices.

Absolutely, but maybe we should not create work for this
case by repeating PF/VF terminology everywhere?

> More details as overview, motivation, use case are further described
> below.
> 
> Patch summary:
> --------------
> patch-1 adds administrative virtuqueue commands
> patch-2 adds its conformance section
> 
> This short series is on top of [1].
> It uses the newly introduced administrative virtqueue facility with 3 new
> opcodes and uses the existing virtio_admin_cmd.
> 
> It is expected to go another rebase once v13 for [1] is rolled out and merged.
> 
> [1] https://lore.kernel.org/virtio-comment/cover.1682354275.git.mst@redhat.com/T/#t
> 
> Usecase:
> --------
> 1. A hypervisor/system needs to provide transitional
>    virtio devices to the guest VM at scale of thousands,
>    typically, one to eight devices per VM.
> 
> 2. A hypervisor/system needs to provide such devices using a
>    vendor agnostic driver in the hypervisor system.
> 
> 3. A hypervisor system prefers to have single stack regardless of
>    virtio device type (net/blk) and be future compatible with a
>    single vfio stack using SR-IOV or other scalable device
>    virtualization technology to map PCI devices to the guest VM.
>    (as transitional or otherwise)
> 
> Motivation/Background:
> ----------------------
> The existing virtio transitional PCI device is missing support for
> PCI SR-IOV based devices. Currently it does not work beyond
> PCI PF, or as software emulated device in reality. Currently it
> has below cited system level limitations:
> 
> [a] PCIe spec citation:
> VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.
> 
> [b] cpu arch citiation:
> Intel 64 and IA-32 Architectures Software Developer’s Manual:
> The processor’s I/O address space is separate and distinct from
> the physical-memory address space. The I/O address space consists
> of 64K individually addressable 8-bit I/O ports, numbered 0 through FFFFH.
> 
> [c] PCIe spec citation:
> If a bridge implements an I/O address range,...I/O address range will be
> aligned to a 4 KB boundary.
> 
> Above usecase requirements can be solved by PCI PF group owner enabling
> the access to its group member PCI VFs legacy registers using an admin
> virtqueue of the group owner PCI PF.
> 
> Software usage example:
> -----------------------
> The most common way to use and map to the guest VM is by
> using vfio driver framework in Linux kernel.
> 
>                 +----------------------+
>                 |pci_dev_id = 0x100X   |
> +---------------|pci_rev_id = 0x0      |-----+
> |vfio device    |BAR0 = I/O region     |     |
> |               |Other attributes      |     |
> |               +----------------------+     |
> |                                            |
> +   +--------------+     +-----------------+ |
> |   |I/O BAR to AQ |     | Other vfio      | |
> |   |rd/wr mapper  |     | functionalities | |
> |   +--------------+     +-----------------+ |
> |                                            |
> +------+-------------------------+-----------+
>        |                         |
>   +----+------------+       +----+------------+
>   | +-----+         |       | PCI VF device A |
>   | | AQ  |-------------+---->+-------------+ |
>   | +-----+         |   |   | | legacy regs | |
>   | PCI PF device   |   |   | +-------------+ |
>   +-----------------+   |   +-----------------+
>                         |
>                         |   +----+------------+
>                         |   | PCI VF device N |
>                         +---->+-------------+ |
>                             | | legacy regs | |
>                             | +-------------+ |
>                             +-----------------+
> 
> 2. Virtio pci driver to bind to the listed device id and
>    use it as native device in the host.
> 
> 3. Use it in a light weight hypervisor to run bare-metal OS.
> 
> Please review.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> Signed-off-by: Parav Pandit <parav@nvidia.com>

I don't see an overview here though.


> ---
> changelog:
> v0->v1:
> - addressed comments, suggesetions and ideas from Michael Tsirkin and Jason Wang
> - far more simpler design than MMR access
> - removed complexities of MMR device ids
> - removed complexities of MMR registers and extended capabilities
> - dropped adding new extended capabilities because if if they are
>   added, a pci device still needs to have existing capabilities
>   in the legacy configuration space and hypervisor driver do not
>   need to access them
> 
> 
> Parav Pandit (2):
>   transport-pci: Introduce legacy registers access commands
>   transport-pci: Add legacy register access conformance section
> 
>  admin.tex                 |   5 +-
>  conformance.tex           |   2 +
>  transport-pci-vf-regs.tex | 115 ++++++++++++++++++++++++++++++++++++++
>  transport-pci.tex         |   2 +
>  4 files changed, 123 insertions(+), 1 deletion(-)
>  create mode 100644 transport-pci-vf-regs.tex
> 
> -- 
> 2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 0/2] transport-pci: Introduce legacy registers access using AQ
@ 2023-05-03 10:16   ` Michael S. Tsirkin
  0 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03 10:16 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs

On Wed, May 03, 2023 at 06:26:57AM +0300, Parav Pandit wrote:
> This patch introduces legacy registers access commands for the owner
> group member PCI PF to access the legacy registers of the member VFs.
> 
> If in future any SIOV devices to support legacy registers, they
> can be easily supported using same commands by using the group
> member identifiers of the future SIOV devices.

Absolutely, but maybe we should not create work for this
case by repeating PF/VF terminology everywhere?

> More details as overview, motivation, use case are further described
> below.
> 
> Patch summary:
> --------------
> patch-1 adds administrative virtuqueue commands
> patch-2 adds its conformance section
> 
> This short series is on top of [1].
> It uses the newly introduced administrative virtqueue facility with 3 new
> opcodes and uses the existing virtio_admin_cmd.
> 
> It is expected to go another rebase once v13 for [1] is rolled out and merged.
> 
> [1] https://lore.kernel.org/virtio-comment/cover.1682354275.git.mst@redhat.com/T/#t
> 
> Usecase:
> --------
> 1. A hypervisor/system needs to provide transitional
>    virtio devices to the guest VM at scale of thousands,
>    typically, one to eight devices per VM.
> 
> 2. A hypervisor/system needs to provide such devices using a
>    vendor agnostic driver in the hypervisor system.
> 
> 3. A hypervisor system prefers to have single stack regardless of
>    virtio device type (net/blk) and be future compatible with a
>    single vfio stack using SR-IOV or other scalable device
>    virtualization technology to map PCI devices to the guest VM.
>    (as transitional or otherwise)
> 
> Motivation/Background:
> ----------------------
> The existing virtio transitional PCI device is missing support for
> PCI SR-IOV based devices. Currently it does not work beyond
> PCI PF, or as software emulated device in reality. Currently it
> has below cited system level limitations:
> 
> [a] PCIe spec citation:
> VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.
> 
> [b] cpu arch citiation:
> Intel 64 and IA-32 Architectures Software Developer’s Manual:
> The processor’s I/O address space is separate and distinct from
> the physical-memory address space. The I/O address space consists
> of 64K individually addressable 8-bit I/O ports, numbered 0 through FFFFH.
> 
> [c] PCIe spec citation:
> If a bridge implements an I/O address range,...I/O address range will be
> aligned to a 4 KB boundary.
> 
> Above usecase requirements can be solved by PCI PF group owner enabling
> the access to its group member PCI VFs legacy registers using an admin
> virtqueue of the group owner PCI PF.
> 
> Software usage example:
> -----------------------
> The most common way to use and map to the guest VM is by
> using vfio driver framework in Linux kernel.
> 
>                 +----------------------+
>                 |pci_dev_id = 0x100X   |
> +---------------|pci_rev_id = 0x0      |-----+
> |vfio device    |BAR0 = I/O region     |     |
> |               |Other attributes      |     |
> |               +----------------------+     |
> |                                            |
> +   +--------------+     +-----------------+ |
> |   |I/O BAR to AQ |     | Other vfio      | |
> |   |rd/wr mapper  |     | functionalities | |
> |   +--------------+     +-----------------+ |
> |                                            |
> +------+-------------------------+-----------+
>        |                         |
>   +----+------------+       +----+------------+
>   | +-----+         |       | PCI VF device A |
>   | | AQ  |-------------+---->+-------------+ |
>   | +-----+         |   |   | | legacy regs | |
>   | PCI PF device   |   |   | +-------------+ |
>   +-----------------+   |   +-----------------+
>                         |
>                         |   +----+------------+
>                         |   | PCI VF device N |
>                         +---->+-------------+ |
>                             | | legacy regs | |
>                             | +-------------+ |
>                             +-----------------+
> 
> 2. Virtio pci driver to bind to the listed device id and
>    use it as native device in the host.
> 
> 3. Use it in a light weight hypervisor to run bare-metal OS.
> 
> Please review.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> Signed-off-by: Parav Pandit <parav@nvidia.com>

I don't see an overview here though.


> ---
> changelog:
> v0->v1:
> - addressed comments, suggesetions and ideas from Michael Tsirkin and Jason Wang
> - far more simpler design than MMR access
> - removed complexities of MMR device ids
> - removed complexities of MMR registers and extended capabilities
> - dropped adding new extended capabilities because if if they are
>   added, a pci device still needs to have existing capabilities
>   in the legacy configuration space and hypervisor driver do not
>   need to access them
> 
> 
> Parav Pandit (2):
>   transport-pci: Introduce legacy registers access commands
>   transport-pci: Add legacy register access conformance section
> 
>  admin.tex                 |   5 +-
>  conformance.tex           |   2 +
>  transport-pci-vf-regs.tex | 115 ++++++++++++++++++++++++++++++++++++++
>  transport-pci.tex         |   2 +
>  4 files changed, 123 insertions(+), 1 deletion(-)
>  create mode 100644 transport-pci-vf-regs.tex
> 
> -- 
> 2.26.2


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-03  5:42     ` [virtio-comment] " Michael S. Tsirkin
@ 2023-05-03 14:47       ` Parav Pandit
  -1 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 14:47 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs



On 5/3/2023 1:42 AM, Michael S. Tsirkin wrote:
> On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:

>> diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
>> new file mode 100644
>> index 0000000..16ced32
>> --- /dev/null
>> +++ b/transport-pci-vf-regs.tex
> 
> I'd like the name to reflect "legacy". Also I don't think this has
> to be SRIOV generally. It's just legacy PCI over admin commands.
> Except for virtio_admin_cmd_lq_notify_query_result
> which refers to PCI? But that
> one I can't say for sure what it does.
> 
It is for legacy now, in future it can be renamed if this is supported.
We already discussed in v0 that non legacy should not involve hypervisor 
mediation. May be you still it should be. In such case lets park that 
discussion for future. This proposal is not limiting it.

> 
>> @@ -0,0 +1,84 @@
>> +\subsection{SR-IOV VFs Legacy Registers Access}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
>> +
>> +As described in PCIe base specification \hyperref[intro:PCIe]{[PCIe]} PCI VFs
>> +do not support IOBAR. A PCI PF device can optionally enable driver to access
>> +its member PCI VFs devices legacy common configuration and device configuration
>> +registers using an administration virtqueue. A PCI PF group owner device that
>> +supports its member VFs legacy registers access via the administration
>> +virtqueue should supports following commands.
> 
> As above. It actually can work for any group if we want to.
True. As defined by the PCIe spec, for virtualized VFs and SFs devices, 
VI is not necessary, and many devices in PCI space are avoiding 
hypervisor mediation, so whether to tunnel or not is really a question 
for future for non legacy registers.

> 
> 
>> +
>> +\begin{enumerate}
>> +\item Legacy Registers Write
>> +\item Legacy Registers Read
>> +\item Legacy Queue Notify Offset Query
>> +\end{enumerate}
>> +
> 
> Pls add some theory of operation. How can all this be used?
> 
ok. I will add in this section.

>> +\begin{lstlisting}
>> +struct virtio_admin_cmd_lreg_rd_result {
>> +	u8 registers[];
>> +};
>> +\end{lstlisting}
>> +
>> +\subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Queue Notify Offset Query}
>> +
>> +This command returns the notify offset of the member VF for queue
>> +notifications.
> 
> What is this notify offset? It's never explained.
> 
ok. will add it.
It is the notification offset where a hypervisor driver can perform 
driver notifications.

>> This command follows \field{struct virtio_admin_cmd}.
>> +Driver sets command opcode \field{opcode} to VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY.
>> +There is no command specific data for this command.
>> +Driver sets \field{group_type} to 1.
>> +Driver sets \field{group_member_id} to a valid VF number.
> 
> I think ATM the limitation for this is that the member must be a pci
> device, otherwise BAR is not well defined. We will have to
> find a way to extend this for SIOV. 

SIOV device will also have the BAR and offset of the parent PF.
The limitation of current AQ is currently is it indicates the BAR of the 
member device (and does not allow group owner for SIOV), but we can 
craft it via SIOV device BAR and it will be fine. SIOV spec is not yet 
released for this details at all. So we can wait.

> But that is all, please do
> not repeat documentation about virtio_admin_cmd header, we have that in
> a central place.
> 
Make sense. I will omit it here.

>> +
>> +When command completes successfully, command result contains the queue
>> +notification address in the listed format:
>> +
>> +\begin{lstlisting}
>> +struct virtio_admin_cmd_lq_notify_query_result {
>> +	u8 bar; /* PCI BAR number of the member VF */
>> +	u8 reserved[7];
>> +	le64 offset; /* Byte offset within the BAR */
>> +};
>> +\end{lstlisting}
>> diff --git a/transport-pci.tex b/transport-pci.tex
>> index ff889d3..b187576 100644
>> --- a/transport-pci.tex
>> +++ b/transport-pci.tex
>> @@ -1179,3 +1179,5 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
>>           re-examine the configuration space to see what changed.
>>       \end{itemize}
>>   \end{itemize}
>> +
>> +\input{transport-pci-vf-regs.tex}
> 
> As simple as it is, I feel this falls far short of describing how
> a device should operate.
> Some issues:
> 	- legacy device config offset changes as msi is enabled/disabled
> 	  suggest separate commands for device/common config
This is fine and covered here. The one who is making msix enable/disable 
knows which registers to access before/after disable/enable and device 
also knows it as it is supplying the register.
So they just follow the standard legacy register access behavior.

> 	- legacy device endian-ness changes with guest
> 	  suggest commands to enable LE and BE mode
guest endianeness is not known to the device. Currently it is only for 
the LE guests who had some legacy requirement.
and PCIe is LE anyway.

> 	- legacy guests often assume INT#x support
> 	  suggest a way to tunnel that too;
INT#x is present on the PCI device itself. So no need to tunnel it.
Also INT#x is very narrow case. When MSI-X is not supported, a 
hypervisor can choose not to even connect such device to the guest VM.

> 	  though supporting ISR is going to be a challenge :(
> 	- I presume admin command is not the way to do kicks? Or is it ok?
> 	- there's some kind of notify thing here?
> 
Right.
We already discussed this in v0.
Summary of it is: admin command can, but it wont work any performant 
way. The device and driver uses the notification region already present 
on the PCI VF device, which is queried using NOTIFY_QUERY command.

> I expected to see more statements along the lines of
>          command ABC has the same effect as access
>          to register DEF of the member through the legacy pci interface
> 
Yes, good point. I will add it in the theory of operation section for 
this mapping detail.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-03 14:47       ` Parav Pandit
  0 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 14:47 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs



On 5/3/2023 1:42 AM, Michael S. Tsirkin wrote:
> On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:

>> diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
>> new file mode 100644
>> index 0000000..16ced32
>> --- /dev/null
>> +++ b/transport-pci-vf-regs.tex
> 
> I'd like the name to reflect "legacy". Also I don't think this has
> to be SRIOV generally. It's just legacy PCI over admin commands.
> Except for virtio_admin_cmd_lq_notify_query_result
> which refers to PCI? But that
> one I can't say for sure what it does.
> 
It is for legacy now, in future it can be renamed if this is supported.
We already discussed in v0 that non legacy should not involve hypervisor 
mediation. May be you still it should be. In such case lets park that 
discussion for future. This proposal is not limiting it.

> 
>> @@ -0,0 +1,84 @@
>> +\subsection{SR-IOV VFs Legacy Registers Access}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
>> +
>> +As described in PCIe base specification \hyperref[intro:PCIe]{[PCIe]} PCI VFs
>> +do not support IOBAR. A PCI PF device can optionally enable driver to access
>> +its member PCI VFs devices legacy common configuration and device configuration
>> +registers using an administration virtqueue. A PCI PF group owner device that
>> +supports its member VFs legacy registers access via the administration
>> +virtqueue should supports following commands.
> 
> As above. It actually can work for any group if we want to.
True. As defined by the PCIe spec, for virtualized VFs and SFs devices, 
VI is not necessary, and many devices in PCI space are avoiding 
hypervisor mediation, so whether to tunnel or not is really a question 
for future for non legacy registers.

> 
> 
>> +
>> +\begin{enumerate}
>> +\item Legacy Registers Write
>> +\item Legacy Registers Read
>> +\item Legacy Queue Notify Offset Query
>> +\end{enumerate}
>> +
> 
> Pls add some theory of operation. How can all this be used?
> 
ok. I will add in this section.

>> +\begin{lstlisting}
>> +struct virtio_admin_cmd_lreg_rd_result {
>> +	u8 registers[];
>> +};
>> +\end{lstlisting}
>> +
>> +\subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Queue Notify Offset Query}
>> +
>> +This command returns the notify offset of the member VF for queue
>> +notifications.
> 
> What is this notify offset? It's never explained.
> 
ok. will add it.
It is the notification offset where a hypervisor driver can perform 
driver notifications.

>> This command follows \field{struct virtio_admin_cmd}.
>> +Driver sets command opcode \field{opcode} to VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY.
>> +There is no command specific data for this command.
>> +Driver sets \field{group_type} to 1.
>> +Driver sets \field{group_member_id} to a valid VF number.
> 
> I think ATM the limitation for this is that the member must be a pci
> device, otherwise BAR is not well defined. We will have to
> find a way to extend this for SIOV. 

SIOV device will also have the BAR and offset of the parent PF.
The limitation of current AQ is currently is it indicates the BAR of the 
member device (and does not allow group owner for SIOV), but we can 
craft it via SIOV device BAR and it will be fine. SIOV spec is not yet 
released for this details at all. So we can wait.

> But that is all, please do
> not repeat documentation about virtio_admin_cmd header, we have that in
> a central place.
> 
Make sense. I will omit it here.

>> +
>> +When command completes successfully, command result contains the queue
>> +notification address in the listed format:
>> +
>> +\begin{lstlisting}
>> +struct virtio_admin_cmd_lq_notify_query_result {
>> +	u8 bar; /* PCI BAR number of the member VF */
>> +	u8 reserved[7];
>> +	le64 offset; /* Byte offset within the BAR */
>> +};
>> +\end{lstlisting}
>> diff --git a/transport-pci.tex b/transport-pci.tex
>> index ff889d3..b187576 100644
>> --- a/transport-pci.tex
>> +++ b/transport-pci.tex
>> @@ -1179,3 +1179,5 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
>>           re-examine the configuration space to see what changed.
>>       \end{itemize}
>>   \end{itemize}
>> +
>> +\input{transport-pci-vf-regs.tex}
> 
> As simple as it is, I feel this falls far short of describing how
> a device should operate.
> Some issues:
> 	- legacy device config offset changes as msi is enabled/disabled
> 	  suggest separate commands for device/common config
This is fine and covered here. The one who is making msix enable/disable 
knows which registers to access before/after disable/enable and device 
also knows it as it is supplying the register.
So they just follow the standard legacy register access behavior.

> 	- legacy device endian-ness changes with guest
> 	  suggest commands to enable LE and BE mode
guest endianeness is not known to the device. Currently it is only for 
the LE guests who had some legacy requirement.
and PCIe is LE anyway.

> 	- legacy guests often assume INT#x support
> 	  suggest a way to tunnel that too;
INT#x is present on the PCI device itself. So no need to tunnel it.
Also INT#x is very narrow case. When MSI-X is not supported, a 
hypervisor can choose not to even connect such device to the guest VM.

> 	  though supporting ISR is going to be a challenge :(
> 	- I presume admin command is not the way to do kicks? Or is it ok?
> 	- there's some kind of notify thing here?
> 
Right.
We already discussed this in v0.
Summary of it is: admin command can, but it wont work any performant 
way. The device and driver uses the notification region already present 
on the PCI VF device, which is queried using NOTIFY_QUERY command.

> I expected to see more statements along the lines of
>          command ABC has the same effect as access
>          to register DEF of the member through the legacy pci interface
> 
Yes, good point. I will add it in the theory of operation section for 
this mapping detail.

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-03  5:50     ` [virtio-comment] " Michael S. Tsirkin
@ 2023-05-03 14:49       ` Parav Pandit
  -1 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 14:49 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs



On 5/3/2023 1:50 AM, Michael S. Tsirkin wrote:
> On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:

>> +Legacy registers write admin command follows \field{struct virtio_admin_cmd}.
>> +This command writes legacy registers of a member VF device. Driver should write
>> +appropriate register \field{size} depending on the width of the legacy
>> +common registers or device specific registers.
>> +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_WRITE.
>> +Driver sets \field{group_type} to 1 for VFs.
>> +Driver sets \field{group_member_id} to a valid VF number.
>> +
>> +The \field{command_specific_data} has following listed structure format:
>> +
>> +\begin{lstlisting}
>> +struct virtio_admin_cmd_lreg_wr_data {
>> +	u8 offset; /* Starting byte offset of the register(s) to write */
>> +	u8 size; /* Number of bytes to write into the register. */
>> +	u8 register[];
> 
> And maybe add
> 	u8 reserved[]; /* structure padding to multiple of 8 bytes */
> 
>> +};
Thinking, what do we miss without the padding here?

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-03 14:49       ` Parav Pandit
  0 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 14:49 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs



On 5/3/2023 1:50 AM, Michael S. Tsirkin wrote:
> On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:

>> +Legacy registers write admin command follows \field{struct virtio_admin_cmd}.
>> +This command writes legacy registers of a member VF device. Driver should write
>> +appropriate register \field{size} depending on the width of the legacy
>> +common registers or device specific registers.
>> +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_WRITE.
>> +Driver sets \field{group_type} to 1 for VFs.
>> +Driver sets \field{group_member_id} to a valid VF number.
>> +
>> +The \field{command_specific_data} has following listed structure format:
>> +
>> +\begin{lstlisting}
>> +struct virtio_admin_cmd_lreg_wr_data {
>> +	u8 offset; /* Starting byte offset of the register(s) to write */
>> +	u8 size; /* Number of bytes to write into the register. */
>> +	u8 register[];
> 
> And maybe add
> 	u8 reserved[]; /* structure padding to multiple of 8 bytes */
> 
>> +};
Thinking, what do we miss without the padding here?

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [virtio-comment] Re: [PATCH v1 2/2] transport-pci: Add legacy register access conformance section
  2023-05-03  5:48     ` [virtio-comment] " Michael S. Tsirkin
@ 2023-05-03 14:53       ` Parav Pandit
  -1 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 14:53 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs



On 5/3/2023 1:48 AM, Michael S. Tsirkin wrote:
> On Wed, May 03, 2023 at 06:26:59AM +0300, Parav Pandit wrote:
>> Add device and driver conformanace section for legacy registers access
>> commands interface.
>>
>> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
>> Signed-off-by: Parav Pandit <parav@nvidia.com>
>> ---
>>   conformance.tex           |  2 ++
>>   transport-pci-vf-regs.tex | 31 +++++++++++++++++++++++++++++++
>>   2 files changed, 33 insertions(+)
>>
>> diff --git a/conformance.tex b/conformance.tex
>> index 01ccd69..dbd8cd6 100644
>> --- a/conformance.tex
>> +++ b/conformance.tex
>> @@ -109,6 +109,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>>   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
>>   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
>>   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
>> +\item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
>>   \end{itemize}
>>   
>>   \conformance{\subsection}{MMIO Driver Conformance}\label{sec:Conformance / Driver Conformance / MMIO Driver Conformance}
>> @@ -194,6 +195,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>>   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
>>   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
>>   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
>> +\item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
>>   \end{itemize}
>>   
>>   \conformance{\subsection}{MMIO Device Conformance}\label{sec:Conformance / Device Conformance / MMIO Device Conformance}
>> diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
>> index 16ced32..7d0574b 100644
>> --- a/transport-pci-vf-regs.tex
>> +++ b/transport-pci-vf-regs.tex
>> @@ -82,3 +82,34 @@ \subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Opti
>>   	le64 offset; /* Byte offset within the BAR */
>>   };
>>   \end{lstlisting}
>> +
>> +\devicenormative{\paragraph}{SR-IOV VFs Legacy Registers Access}{Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
>> +
>> +If the PCI PF device supports legacy registers access, it SHOULD set
>> +corresponding bits for commands VIRTIO_ADMIN_CMD_LREG_WRITE,
>> +VIRTIO_ADMIN_CMD_LREG_READ and VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY in
>> +command result of VIRTIO_ADMIN_CMD_LIST_QUERY in
>> +\field{device_admin_cmd_opcodes}.
> 
> Don't repeat documentation of VIRTIO_ADMIN_CMD_LIST_QUERY please.
yeah, I had dual thoughts, I am fine if this looks duplicate.
Will remove.

> just say all these must be supported.
> In fact what are you saying here? That all 3 are supported
> or none at all? What about just
> VIRTIO_ADMIN_CMD_LREG_READ/VIRTIO_ADMIN_CMD_LREG_WRITE?
> Looks like a slower but working way to do notifications
> is through a common config write, no?
Notifications to be done using the notification region exposed and 
queried using the 3rd QUERY command.
> 
>> +
>> +The device MUST support legacy configuration registers to its defined width.
> 
> what is this?
> 
Each register has its defined width as 8-bit, 16-bit, 32-bit etc.
So device support the access to its defined width.
May be to rewrite it differently?

>> +
>> +The device MAY fail legacy configuration registers access when either the
>> +access is for an incorrct register width or if the register offset is incorrect.
> 
Spell checkers didnt capture the error of incorrct. Need to find better 
tool.

> with which error code?
EINVAL
but should we repeat the general section here?

> 
>> +
>> +The device MUST allow access of one or multiple bytes of the registers when
>> +such register is defined as byte array, for example \field{mac} of \field{struct
>> +virtio_net_config} of the Network Device.
> 
> so which accesses need to be supported then?
> 
Not sure I follow the question.
Can you please explain?

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [virtio-comment] Re: [PATCH v1 2/2] transport-pci: Add legacy register access conformance section
@ 2023-05-03 14:53       ` Parav Pandit
  0 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 14:53 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs



On 5/3/2023 1:48 AM, Michael S. Tsirkin wrote:
> On Wed, May 03, 2023 at 06:26:59AM +0300, Parav Pandit wrote:
>> Add device and driver conformanace section for legacy registers access
>> commands interface.
>>
>> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
>> Signed-off-by: Parav Pandit <parav@nvidia.com>
>> ---
>>   conformance.tex           |  2 ++
>>   transport-pci-vf-regs.tex | 31 +++++++++++++++++++++++++++++++
>>   2 files changed, 33 insertions(+)
>>
>> diff --git a/conformance.tex b/conformance.tex
>> index 01ccd69..dbd8cd6 100644
>> --- a/conformance.tex
>> +++ b/conformance.tex
>> @@ -109,6 +109,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>>   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
>>   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
>>   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
>> +\item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
>>   \end{itemize}
>>   
>>   \conformance{\subsection}{MMIO Driver Conformance}\label{sec:Conformance / Driver Conformance / MMIO Driver Conformance}
>> @@ -194,6 +195,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>>   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
>>   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
>>   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
>> +\item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
>>   \end{itemize}
>>   
>>   \conformance{\subsection}{MMIO Device Conformance}\label{sec:Conformance / Device Conformance / MMIO Device Conformance}
>> diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
>> index 16ced32..7d0574b 100644
>> --- a/transport-pci-vf-regs.tex
>> +++ b/transport-pci-vf-regs.tex
>> @@ -82,3 +82,34 @@ \subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Opti
>>   	le64 offset; /* Byte offset within the BAR */
>>   };
>>   \end{lstlisting}
>> +
>> +\devicenormative{\paragraph}{SR-IOV VFs Legacy Registers Access}{Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
>> +
>> +If the PCI PF device supports legacy registers access, it SHOULD set
>> +corresponding bits for commands VIRTIO_ADMIN_CMD_LREG_WRITE,
>> +VIRTIO_ADMIN_CMD_LREG_READ and VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY in
>> +command result of VIRTIO_ADMIN_CMD_LIST_QUERY in
>> +\field{device_admin_cmd_opcodes}.
> 
> Don't repeat documentation of VIRTIO_ADMIN_CMD_LIST_QUERY please.
yeah, I had dual thoughts, I am fine if this looks duplicate.
Will remove.

> just say all these must be supported.
> In fact what are you saying here? That all 3 are supported
> or none at all? What about just
> VIRTIO_ADMIN_CMD_LREG_READ/VIRTIO_ADMIN_CMD_LREG_WRITE?
> Looks like a slower but working way to do notifications
> is through a common config write, no?
Notifications to be done using the notification region exposed and 
queried using the 3rd QUERY command.
> 
>> +
>> +The device MUST support legacy configuration registers to its defined width.
> 
> what is this?
> 
Each register has its defined width as 8-bit, 16-bit, 32-bit etc.
So device support the access to its defined width.
May be to rewrite it differently?

>> +
>> +The device MAY fail legacy configuration registers access when either the
>> +access is for an incorrct register width or if the register offset is incorrect.
> 
Spell checkers didnt capture the error of incorrct. Need to find better 
tool.

> with which error code?
EINVAL
but should we repeat the general section here?

> 
>> +
>> +The device MUST allow access of one or multiple bytes of the registers when
>> +such register is defined as byte array, for example \field{mac} of \field{struct
>> +virtio_net_config} of the Network Device.
> 
> so which accesses need to be supported then?
> 
Not sure I follow the question.
Can you please explain?

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 0/2] transport-pci: Introduce legacy registers access using AQ
  2023-05-03 10:16   ` [virtio-comment] " Michael S. Tsirkin
@ 2023-05-03 14:57     ` Parav Pandit
  -1 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 14:57 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs, Yishai Hadas, Maor Gottlieb



On 5/3/2023 6:16 AM, Michael S. Tsirkin wrote:
> On Wed, May 03, 2023 at 06:26:57AM +0300, Parav Pandit wrote:
>> This patch introduces legacy registers access commands for the owner
>> group member PCI PF to access the legacy registers of the member VFs.
>>
>> If in future any SIOV devices to support legacy registers, they
>> can be easily supported using same commands by using the group
>> member identifiers of the future SIOV devices.
> 
> Absolutely, but maybe we should not create work for this
> case by repeating PF/VF terminology everywhere?
> 
If we omit the PF, VF it is kind of hard to explain things without any 
tangible objects.
For example in your series lot of documentation of AQ is around SR-IOV 
and VFs. If you take out that notion of VFs for inclusion of undefined 
SIOV objects its hard as well.

So I think we can continue to talk about PF and VFs as is. When SIOV 
enters, it will be easier to talk about it as VF or SIOV VDEV or 
something similar.

>> More details as overview, motivation, use case are further described
>> below.
>>
>> Patch summary:
>> --------------
>> patch-1 adds administrative virtuqueue commands
>> patch-2 adds its conformance section
>>
>> This short series is on top of [1].
>> It uses the newly introduced administrative virtqueue facility with 3 new
>> opcodes and uses the existing virtio_admin_cmd.
>>
>> It is expected to go another rebase once v13 for [1] is rolled out and merged.
>>
>> [1] https://lore.kernel.org/virtio-comment/cover.1682354275.git.mst@redhat.com/T/#t
>>
>> Usecase:
>> --------
>> 1. A hypervisor/system needs to provide transitional
>>     virtio devices to the guest VM at scale of thousands,
>>     typically, one to eight devices per VM.
>>
>> 2. A hypervisor/system needs to provide such devices using a
>>     vendor agnostic driver in the hypervisor system.
>>
>> 3. A hypervisor system prefers to have single stack regardless of
>>     virtio device type (net/blk) and be future compatible with a
>>     single vfio stack using SR-IOV or other scalable device
>>     virtualization technology to map PCI devices to the guest VM.
>>     (as transitional or otherwise)
>>
>> Motivation/Background:
>> ----------------------
>> The existing virtio transitional PCI device is missing support for
>> PCI SR-IOV based devices. Currently it does not work beyond
>> PCI PF, or as software emulated device in reality. Currently it
>> has below cited system level limitations:
>>
>> [a] PCIe spec citation:
>> VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.
>>
>> [b] cpu arch citiation:
>> Intel 64 and IA-32 Architectures Software Developer’s Manual:
>> The processor’s I/O address space is separate and distinct from
>> the physical-memory address space. The I/O address space consists
>> of 64K individually addressable 8-bit I/O ports, numbered 0 through FFFFH.
>>
>> [c] PCIe spec citation:
>> If a bridge implements an I/O address range,...I/O address range will be
>> aligned to a 4 KB boundary.
>>
>> Above usecase requirements can be solved by PCI PF group owner enabling
>> the access to its group member PCI VFs legacy registers using an admin
>> virtqueue of the group owner PCI PF.
>>
>> Software usage example:
>> -----------------------
>> The most common way to use and map to the guest VM is by
>> using vfio driver framework in Linux kernel.
>>
>>                  +----------------------+
>>                  |pci_dev_id = 0x100X   |
>> +---------------|pci_rev_id = 0x0      |-----+
>> |vfio device    |BAR0 = I/O region     |     |
>> |               |Other attributes      |     |
>> |               +----------------------+     |
>> |                                            |
>> +   +--------------+     +-----------------+ |
>> |   |I/O BAR to AQ |     | Other vfio      | |
>> |   |rd/wr mapper  |     | functionalities | |
>> |   +--------------+     +-----------------+ |
>> |                                            |
>> +------+-------------------------+-----------+
>>         |                         |
>>    +----+------------+       +----+------------+
>>    | +-----+         |       | PCI VF device A |
>>    | | AQ  |-------------+---->+-------------+ |
>>    | +-----+         |   |   | | legacy regs | |
>>    | PCI PF device   |   |   | +-------------+ |
>>    +-----------------+   |   +-----------------+
>>                          |
>>                          |   +----+------------+
>>                          |   | PCI VF device N |
>>                          +---->+-------------+ |
>>                              | | legacy regs | |
>>                              | +-------------+ |
>>                              +-----------------+
>>
>> 2. Virtio pci driver to bind to the listed device id and
>>     use it as native device in the host.
>>
>> 3. Use it in a light weight hypervisor to run bare-metal OS.
>>
>> Please review.
>>
>> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
>> Signed-off-by: Parav Pandit <parav@nvidia.com>
> 
> I don't see an overview here though.
> 
I will add it in v2.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 0/2] transport-pci: Introduce legacy registers access using AQ
@ 2023-05-03 14:57     ` Parav Pandit
  0 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 14:57 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs, Yishai Hadas, Maor Gottlieb



On 5/3/2023 6:16 AM, Michael S. Tsirkin wrote:
> On Wed, May 03, 2023 at 06:26:57AM +0300, Parav Pandit wrote:
>> This patch introduces legacy registers access commands for the owner
>> group member PCI PF to access the legacy registers of the member VFs.
>>
>> If in future any SIOV devices to support legacy registers, they
>> can be easily supported using same commands by using the group
>> member identifiers of the future SIOV devices.
> 
> Absolutely, but maybe we should not create work for this
> case by repeating PF/VF terminology everywhere?
> 
If we omit the PF, VF it is kind of hard to explain things without any 
tangible objects.
For example in your series lot of documentation of AQ is around SR-IOV 
and VFs. If you take out that notion of VFs for inclusion of undefined 
SIOV objects its hard as well.

So I think we can continue to talk about PF and VFs as is. When SIOV 
enters, it will be easier to talk about it as VF or SIOV VDEV or 
something similar.

>> More details as overview, motivation, use case are further described
>> below.
>>
>> Patch summary:
>> --------------
>> patch-1 adds administrative virtuqueue commands
>> patch-2 adds its conformance section
>>
>> This short series is on top of [1].
>> It uses the newly introduced administrative virtqueue facility with 3 new
>> opcodes and uses the existing virtio_admin_cmd.
>>
>> It is expected to go another rebase once v13 for [1] is rolled out and merged.
>>
>> [1] https://lore.kernel.org/virtio-comment/cover.1682354275.git.mst@redhat.com/T/#t
>>
>> Usecase:
>> --------
>> 1. A hypervisor/system needs to provide transitional
>>     virtio devices to the guest VM at scale of thousands,
>>     typically, one to eight devices per VM.
>>
>> 2. A hypervisor/system needs to provide such devices using a
>>     vendor agnostic driver in the hypervisor system.
>>
>> 3. A hypervisor system prefers to have single stack regardless of
>>     virtio device type (net/blk) and be future compatible with a
>>     single vfio stack using SR-IOV or other scalable device
>>     virtualization technology to map PCI devices to the guest VM.
>>     (as transitional or otherwise)
>>
>> Motivation/Background:
>> ----------------------
>> The existing virtio transitional PCI device is missing support for
>> PCI SR-IOV based devices. Currently it does not work beyond
>> PCI PF, or as software emulated device in reality. Currently it
>> has below cited system level limitations:
>>
>> [a] PCIe spec citation:
>> VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.
>>
>> [b] cpu arch citiation:
>> Intel 64 and IA-32 Architectures Software Developer’s Manual:
>> The processor’s I/O address space is separate and distinct from
>> the physical-memory address space. The I/O address space consists
>> of 64K individually addressable 8-bit I/O ports, numbered 0 through FFFFH.
>>
>> [c] PCIe spec citation:
>> If a bridge implements an I/O address range,...I/O address range will be
>> aligned to a 4 KB boundary.
>>
>> Above usecase requirements can be solved by PCI PF group owner enabling
>> the access to its group member PCI VFs legacy registers using an admin
>> virtqueue of the group owner PCI PF.
>>
>> Software usage example:
>> -----------------------
>> The most common way to use and map to the guest VM is by
>> using vfio driver framework in Linux kernel.
>>
>>                  +----------------------+
>>                  |pci_dev_id = 0x100X   |
>> +---------------|pci_rev_id = 0x0      |-----+
>> |vfio device    |BAR0 = I/O region     |     |
>> |               |Other attributes      |     |
>> |               +----------------------+     |
>> |                                            |
>> +   +--------------+     +-----------------+ |
>> |   |I/O BAR to AQ |     | Other vfio      | |
>> |   |rd/wr mapper  |     | functionalities | |
>> |   +--------------+     +-----------------+ |
>> |                                            |
>> +------+-------------------------+-----------+
>>         |                         |
>>    +----+------------+       +----+------------+
>>    | +-----+         |       | PCI VF device A |
>>    | | AQ  |-------------+---->+-------------+ |
>>    | +-----+         |   |   | | legacy regs | |
>>    | PCI PF device   |   |   | +-------------+ |
>>    +-----------------+   |   +-----------------+
>>                          |
>>                          |   +----+------------+
>>                          |   | PCI VF device N |
>>                          +---->+-------------+ |
>>                              | | legacy regs | |
>>                              | +-------------+ |
>>                              +-----------------+
>>
>> 2. Virtio pci driver to bind to the listed device id and
>>     use it as native device in the host.
>>
>> 3. Use it in a light weight hypervisor to run bare-metal OS.
>>
>> Please review.
>>
>> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
>> Signed-off-by: Parav Pandit <parav@nvidia.com>
> 
> I don't see an overview here though.
> 
I will add it in v2.

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-03 14:47       ` [virtio-comment] " Parav Pandit
@ 2023-05-03 16:48         ` Michael S. Tsirkin
  -1 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03 16:48 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs

On Wed, May 03, 2023 at 10:47:26AM -0400, Parav Pandit wrote:
> 
> 
> On 5/3/2023 1:42 AM, Michael S. Tsirkin wrote:
> > On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
> 
> > > diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
> > > new file mode 100644
> > > index 0000000..16ced32
> > > --- /dev/null
> > > +++ b/transport-pci-vf-regs.tex
> > 
> > I'd like the name to reflect "legacy". Also I don't think this has
> > to be SRIOV generally. It's just legacy PCI over admin commands.
> > Except for virtio_admin_cmd_lq_notify_query_result
> > which refers to PCI? But that
> > one I can't say for sure what it does.
> > 
> It is for legacy now, in future it can be renamed if this is supported.
> We already discussed in v0 that non legacy should not involve hypervisor
> mediation. May be you still it should be. In such case lets park that
> discussion for future. This proposal is not limiting it.
> 
> > 
> > > @@ -0,0 +1,84 @@
> > > +\subsection{SR-IOV VFs Legacy Registers Access}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> > > +
> > > +As described in PCIe base specification \hyperref[intro:PCIe]{[PCIe]} PCI VFs
> > > +do not support IOBAR. A PCI PF device can optionally enable driver to access
> > > +its member PCI VFs devices legacy common configuration and device configuration
> > > +registers using an administration virtqueue. A PCI PF group owner device that
> > > +supports its member VFs legacy registers access via the administration
> > > +virtqueue should supports following commands.
> > 
> > As above. It actually can work for any group if we want to.
> True. As defined by the PCIe spec, for virtualized VFs and SFs devices, VI
> is not necessary, and many devices in PCI space are avoiding hypervisor
> mediation, so whether to tunnel or not is really a question for future for
> non legacy registers.
> 
> > 
> > 
> > > +
> > > +\begin{enumerate}
> > > +\item Legacy Registers Write
> > > +\item Legacy Registers Read
> > > +\item Legacy Queue Notify Offset Query
> > > +\end{enumerate}
> > > +
> > 
> > Pls add some theory of operation. How can all this be used?
> > 
> ok. I will add in this section.
> 
> > > +\begin{lstlisting}
> > > +struct virtio_admin_cmd_lreg_rd_result {
> > > +	u8 registers[];
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Queue Notify Offset Query}
> > > +
> > > +This command returns the notify offset of the member VF for queue
> > > +notifications.
> > 
> > What is this notify offset? It's never explained.
> > 
> ok. will add it.
> It is the notification offset where a hypervisor driver can perform driver
> notifications.
> 
> > > This command follows \field{struct virtio_admin_cmd}.
> > > +Driver sets command opcode \field{opcode} to VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY.
> > > +There is no command specific data for this command.
> > > +Driver sets \field{group_type} to 1.
> > > +Driver sets \field{group_member_id} to a valid VF number.
> > 
> > I think ATM the limitation for this is that the member must be a pci
> > device, otherwise BAR is not well defined. We will have to
> > find a way to extend this for SIOV.
> 
> SIOV device will also have the BAR and offset of the parent PF.
> The limitation of current AQ is currently is it indicates the BAR of the
> member device (and does not allow group owner for SIOV), but we can craft it
> via SIOV device BAR and it will be fine. SIOV spec is not yet released for
> this details at all. So we can wait.
> 
> > But that is all, please do
> > not repeat documentation about virtio_admin_cmd header, we have that in
> > a central place.
> > 
> Make sense. I will omit it here.
> 
> > > +
> > > +When command completes successfully, command result contains the queue
> > > +notification address in the listed format:
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_admin_cmd_lq_notify_query_result {
> > > +	u8 bar; /* PCI BAR number of the member VF */
> > > +	u8 reserved[7];
> > > +	le64 offset; /* Byte offset within the BAR */
> > > +};
> > > +\end{lstlisting}
> > > diff --git a/transport-pci.tex b/transport-pci.tex
> > > index ff889d3..b187576 100644
> > > --- a/transport-pci.tex
> > > +++ b/transport-pci.tex
> > > @@ -1179,3 +1179,5 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
> > >           re-examine the configuration space to see what changed.
> > >       \end{itemize}
> > >   \end{itemize}
> > > +
> > > +\input{transport-pci-vf-regs.tex}
> > 
> > As simple as it is, I feel this falls far short of describing how
> > a device should operate.
> > Some issues:
> > 	- legacy device config offset changes as msi is enabled/disabled
> > 	  suggest separate commands for device/common config
> This is fine and covered here. The one who is making msix enable/disable
> knows which registers to access before/after disable/enable and device also
> knows it as it is supplying the register.
> So they just follow the standard legacy register access behavior.

But do VFs support INT#x? I will have to re-read the spec.

> > 	- legacy device endian-ness changes with guest
> > 	  suggest commands to enable LE and BE mode
> guest endianeness is not known to the device.

But is known to hypervisor.

> Currently it is only for the
> LE guests who had some legacy requirement.

I don't like tying this to LE implicitly some devices might be BE only.
With my idea device can support command to set LE, command to set BE or
both.

> and PCIe is LE anyway.

PCIE config endian-ness does not matter heere.

> > 	- legacy guests often assume INT#x support
> > 	  suggest a way to tunnel that too;
> INT#x is present on the PCI device itself. So no need to tunnel it.
> Also INT#x is very narrow case. When MSI-X is not supported, a hypervisor
> can choose not to even connect such device to the guest VM.

devices will support MSI. But *guest* might not support MSIX you only
find out late when it is driving the device.

> > 	  though supporting ISR is going to be a challenge :(
> > 	- I presume admin command is not the way to do kicks? Or is it ok?
> > 	- there's some kind of notify thing here?
> > 
> Right.
> We already discussed this in v0.
> Summary of it is: admin command can, but it wont work any performant way.
> The device and driver uses the notification region already present on the
> PCI VF device, which is queried using NOTIFY_QUERY command.
> 
> > I expected to see more statements along the lines of
> >          command ABC has the same effect as access
> >          to register DEF of the member through the legacy pci interface
> > 
> Yes, good point. I will add it in the theory of operation section for this
> mapping detail.

OK, and overall if you see an existing statement about legacy do not
copy it, just explain how it is mapped.

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-03 16:48         ` Michael S. Tsirkin
  0 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03 16:48 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs

On Wed, May 03, 2023 at 10:47:26AM -0400, Parav Pandit wrote:
> 
> 
> On 5/3/2023 1:42 AM, Michael S. Tsirkin wrote:
> > On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
> 
> > > diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
> > > new file mode 100644
> > > index 0000000..16ced32
> > > --- /dev/null
> > > +++ b/transport-pci-vf-regs.tex
> > 
> > I'd like the name to reflect "legacy". Also I don't think this has
> > to be SRIOV generally. It's just legacy PCI over admin commands.
> > Except for virtio_admin_cmd_lq_notify_query_result
> > which refers to PCI? But that
> > one I can't say for sure what it does.
> > 
> It is for legacy now, in future it can be renamed if this is supported.
> We already discussed in v0 that non legacy should not involve hypervisor
> mediation. May be you still it should be. In such case lets park that
> discussion for future. This proposal is not limiting it.
> 
> > 
> > > @@ -0,0 +1,84 @@
> > > +\subsection{SR-IOV VFs Legacy Registers Access}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> > > +
> > > +As described in PCIe base specification \hyperref[intro:PCIe]{[PCIe]} PCI VFs
> > > +do not support IOBAR. A PCI PF device can optionally enable driver to access
> > > +its member PCI VFs devices legacy common configuration and device configuration
> > > +registers using an administration virtqueue. A PCI PF group owner device that
> > > +supports its member VFs legacy registers access via the administration
> > > +virtqueue should supports following commands.
> > 
> > As above. It actually can work for any group if we want to.
> True. As defined by the PCIe spec, for virtualized VFs and SFs devices, VI
> is not necessary, and many devices in PCI space are avoiding hypervisor
> mediation, so whether to tunnel or not is really a question for future for
> non legacy registers.
> 
> > 
> > 
> > > +
> > > +\begin{enumerate}
> > > +\item Legacy Registers Write
> > > +\item Legacy Registers Read
> > > +\item Legacy Queue Notify Offset Query
> > > +\end{enumerate}
> > > +
> > 
> > Pls add some theory of operation. How can all this be used?
> > 
> ok. I will add in this section.
> 
> > > +\begin{lstlisting}
> > > +struct virtio_admin_cmd_lreg_rd_result {
> > > +	u8 registers[];
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Queue Notify Offset Query}
> > > +
> > > +This command returns the notify offset of the member VF for queue
> > > +notifications.
> > 
> > What is this notify offset? It's never explained.
> > 
> ok. will add it.
> It is the notification offset where a hypervisor driver can perform driver
> notifications.
> 
> > > This command follows \field{struct virtio_admin_cmd}.
> > > +Driver sets command opcode \field{opcode} to VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY.
> > > +There is no command specific data for this command.
> > > +Driver sets \field{group_type} to 1.
> > > +Driver sets \field{group_member_id} to a valid VF number.
> > 
> > I think ATM the limitation for this is that the member must be a pci
> > device, otherwise BAR is not well defined. We will have to
> > find a way to extend this for SIOV.
> 
> SIOV device will also have the BAR and offset of the parent PF.
> The limitation of current AQ is currently is it indicates the BAR of the
> member device (and does not allow group owner for SIOV), but we can craft it
> via SIOV device BAR and it will be fine. SIOV spec is not yet released for
> this details at all. So we can wait.
> 
> > But that is all, please do
> > not repeat documentation about virtio_admin_cmd header, we have that in
> > a central place.
> > 
> Make sense. I will omit it here.
> 
> > > +
> > > +When command completes successfully, command result contains the queue
> > > +notification address in the listed format:
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_admin_cmd_lq_notify_query_result {
> > > +	u8 bar; /* PCI BAR number of the member VF */
> > > +	u8 reserved[7];
> > > +	le64 offset; /* Byte offset within the BAR */
> > > +};
> > > +\end{lstlisting}
> > > diff --git a/transport-pci.tex b/transport-pci.tex
> > > index ff889d3..b187576 100644
> > > --- a/transport-pci.tex
> > > +++ b/transport-pci.tex
> > > @@ -1179,3 +1179,5 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
> > >           re-examine the configuration space to see what changed.
> > >       \end{itemize}
> > >   \end{itemize}
> > > +
> > > +\input{transport-pci-vf-regs.tex}
> > 
> > As simple as it is, I feel this falls far short of describing how
> > a device should operate.
> > Some issues:
> > 	- legacy device config offset changes as msi is enabled/disabled
> > 	  suggest separate commands for device/common config
> This is fine and covered here. The one who is making msix enable/disable
> knows which registers to access before/after disable/enable and device also
> knows it as it is supplying the register.
> So they just follow the standard legacy register access behavior.

But do VFs support INT#x? I will have to re-read the spec.

> > 	- legacy device endian-ness changes with guest
> > 	  suggest commands to enable LE and BE mode
> guest endianeness is not known to the device.

But is known to hypervisor.

> Currently it is only for the
> LE guests who had some legacy requirement.

I don't like tying this to LE implicitly some devices might be BE only.
With my idea device can support command to set LE, command to set BE or
both.

> and PCIe is LE anyway.

PCIE config endian-ness does not matter heere.

> > 	- legacy guests often assume INT#x support
> > 	  suggest a way to tunnel that too;
> INT#x is present on the PCI device itself. So no need to tunnel it.
> Also INT#x is very narrow case. When MSI-X is not supported, a hypervisor
> can choose not to even connect such device to the guest VM.

devices will support MSI. But *guest* might not support MSIX you only
find out late when it is driving the device.

> > 	  though supporting ISR is going to be a challenge :(
> > 	- I presume admin command is not the way to do kicks? Or is it ok?
> > 	- there's some kind of notify thing here?
> > 
> Right.
> We already discussed this in v0.
> Summary of it is: admin command can, but it wont work any performant way.
> The device and driver uses the notification region already present on the
> PCI VF device, which is queried using NOTIFY_QUERY command.
> 
> > I expected to see more statements along the lines of
> >          command ABC has the same effect as access
> >          to register DEF of the member through the legacy pci interface
> > 
> Yes, good point. I will add it in the theory of operation section for this
> mapping detail.

OK, and overall if you see an existing statement about legacy do not
copy it, just explain how it is mapped.

-- 
MST


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-03 14:49       ` [virtio-comment] " Parav Pandit
@ 2023-05-03 16:49         ` Michael S. Tsirkin
  -1 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03 16:49 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs

On Wed, May 03, 2023 at 10:49:14AM -0400, Parav Pandit wrote:
> 
> 
> On 5/3/2023 1:50 AM, Michael S. Tsirkin wrote:
> > On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
> 
> > > +Legacy registers write admin command follows \field{struct virtio_admin_cmd}.
> > > +This command writes legacy registers of a member VF device. Driver should write
> > > +appropriate register \field{size} depending on the width of the legacy
> > > +common registers or device specific registers.
> > > +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_WRITE.
> > > +Driver sets \field{group_type} to 1 for VFs.
> > > +Driver sets \field{group_member_id} to a valid VF number.
> > > +
> > > +The \field{command_specific_data} has following listed structure format:
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_admin_cmd_lreg_wr_data {
> > > +	u8 offset; /* Starting byte offset of the register(s) to write */
> > > +	u8 size; /* Number of bytes to write into the register. */
> > > +	u8 register[];
> > 
> > And maybe add
> > 	u8 reserved[]; /* structure padding to multiple of 8 bytes */
> > 
> > > +};
> Thinking, what do we miss without the padding here?

it's not 100% clear where the padding is.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-03 16:49         ` Michael S. Tsirkin
  0 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03 16:49 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs

On Wed, May 03, 2023 at 10:49:14AM -0400, Parav Pandit wrote:
> 
> 
> On 5/3/2023 1:50 AM, Michael S. Tsirkin wrote:
> > On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
> 
> > > +Legacy registers write admin command follows \field{struct virtio_admin_cmd}.
> > > +This command writes legacy registers of a member VF device. Driver should write
> > > +appropriate register \field{size} depending on the width of the legacy
> > > +common registers or device specific registers.
> > > +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_WRITE.
> > > +Driver sets \field{group_type} to 1 for VFs.
> > > +Driver sets \field{group_member_id} to a valid VF number.
> > > +
> > > +The \field{command_specific_data} has following listed structure format:
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_admin_cmd_lreg_wr_data {
> > > +	u8 offset; /* Starting byte offset of the register(s) to write */
> > > +	u8 size; /* Number of bytes to write into the register. */
> > > +	u8 register[];
> > 
> > And maybe add
> > 	u8 reserved[]; /* structure padding to multiple of 8 bytes */
> > 
> > > +};
> Thinking, what do we miss without the padding here?

it's not 100% clear where the padding is.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-03 16:48         ` [virtio-comment] " Michael S. Tsirkin
@ 2023-05-03 17:21           ` Parav Pandit
  -1 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 17:21 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs, Yishai Hadas, Maor Gottlieb



On 5/3/2023 12:48 PM, Michael S. Tsirkin wrote:

>>> As simple as it is, I feel this falls far short of describing how
>>> a device should operate.
>>> Some issues:
>>> 	- legacy device config offset changes as msi is enabled/disabled
>>> 	  suggest separate commands for device/common config
>> This is fine and covered here. The one who is making msix enable/disable
>> knows which registers to access before/after disable/enable and device also
>> knows it as it is supplying the register.
>> So they just follow the standard legacy register access behavior.
> 
> But do VFs support INT#x? I will have to re-read the spec.
> 
VFs do not support INTx.
When hypervisor knows that it cannot support msix for the guest, it can 
avoid using the VF for the guest.

>>> 	- legacy device endian-ness changes with guest
>>> 	  suggest commands to enable LE and BE mode
>> guest endianeness is not known to the device.
> 
> But is known to hypervisor.
> 
It can be an extension command in future as part of the VF 
administration command to set it by the hypervisor PF.

>> Currently it is only for the
>> LE guests who had some legacy requirement.
> 
> I don't like tying this to LE implicitly some devices might be BE only.
> With my idea device can support command to set LE, command to set BE or
> both.
> 
It can be an addition in future if needed.

>> and PCIe is LE anyway.
> 
> PCIE config endian-ness does not matter heere.
> 
>>> 	- legacy guests often assume INT#x support
>>> 	  suggest a way to tunnel that too;
>> INT#x is present on the PCI device itself. So no need to tunnel it.
>> Also INT#x is very narrow case. When MSI-X is not supported, a hypervisor
>> can choose not to even connect such device to the guest VM.
> 
> devices will support MSI. But *guest* might not support MSIX you only
> find out late when it is driving the device.
> 
It is a pathological case that may not exist because legacy drivers and 
linux kernels all the way upto 2.6.32 have msix support.

And in case if hypervisor sw wants to support for unknown scenario, it 
can use the one msix based interrupt to emulate intx.

>>> I expected to see more statements along the lines of
>>>           command ABC has the same effect as access
>>>           to register DEF of the member through the legacy pci interface
>>>
>> Yes, good point. I will add it in the theory of operation section for this
>> mapping detail.
> 
> OK, and overall if you see an existing statement about legacy do not
> copy it, just explain how it is mapped.
> 
Yes, will not copy, only the mapping part to add.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-03 17:21           ` Parav Pandit
  0 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 17:21 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs, Yishai Hadas, Maor Gottlieb



On 5/3/2023 12:48 PM, Michael S. Tsirkin wrote:

>>> As simple as it is, I feel this falls far short of describing how
>>> a device should operate.
>>> Some issues:
>>> 	- legacy device config offset changes as msi is enabled/disabled
>>> 	  suggest separate commands for device/common config
>> This is fine and covered here. The one who is making msix enable/disable
>> knows which registers to access before/after disable/enable and device also
>> knows it as it is supplying the register.
>> So they just follow the standard legacy register access behavior.
> 
> But do VFs support INT#x? I will have to re-read the spec.
> 
VFs do not support INTx.
When hypervisor knows that it cannot support msix for the guest, it can 
avoid using the VF for the guest.

>>> 	- legacy device endian-ness changes with guest
>>> 	  suggest commands to enable LE and BE mode
>> guest endianeness is not known to the device.
> 
> But is known to hypervisor.
> 
It can be an extension command in future as part of the VF 
administration command to set it by the hypervisor PF.

>> Currently it is only for the
>> LE guests who had some legacy requirement.
> 
> I don't like tying this to LE implicitly some devices might be BE only.
> With my idea device can support command to set LE, command to set BE or
> both.
> 
It can be an addition in future if needed.

>> and PCIe is LE anyway.
> 
> PCIE config endian-ness does not matter heere.
> 
>>> 	- legacy guests often assume INT#x support
>>> 	  suggest a way to tunnel that too;
>> INT#x is present on the PCI device itself. So no need to tunnel it.
>> Also INT#x is very narrow case. When MSI-X is not supported, a hypervisor
>> can choose not to even connect such device to the guest VM.
> 
> devices will support MSI. But *guest* might not support MSIX you only
> find out late when it is driving the device.
> 
It is a pathological case that may not exist because legacy drivers and 
linux kernels all the way upto 2.6.32 have msix support.

And in case if hypervisor sw wants to support for unknown scenario, it 
can use the one msix based interrupt to emulate intx.

>>> I expected to see more statements along the lines of
>>>           command ABC has the same effect as access
>>>           to register DEF of the member through the legacy pci interface
>>>
>> Yes, good point. I will add it in the theory of operation section for this
>> mapping detail.
> 
> OK, and overall if you see an existing statement about legacy do not
> copy it, just explain how it is mapped.
> 
Yes, will not copy, only the mapping part to add.

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-03 16:49         ` [virtio-comment] " Michael S. Tsirkin
@ 2023-05-03 17:23           ` Parav Pandit
  -1 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 17:23 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs, Yishai Hadas, Maor Gottlieb



On 5/3/2023 12:49 PM, Michael S. Tsirkin wrote:
> On Wed, May 03, 2023 at 10:49:14AM -0400, Parav Pandit wrote:
>>
>>
>> On 5/3/2023 1:50 AM, Michael S. Tsirkin wrote:
>>> On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
>>
>>>> +Legacy registers write admin command follows \field{struct virtio_admin_cmd}.
>>>> +This command writes legacy registers of a member VF device. Driver should write
>>>> +appropriate register \field{size} depending on the width of the legacy
>>>> +common registers or device specific registers.
>>>> +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_WRITE.
>>>> +Driver sets \field{group_type} to 1 for VFs.
>>>> +Driver sets \field{group_member_id} to a valid VF number.
>>>> +
>>>> +The \field{command_specific_data} has following listed structure format:
>>>> +
>>>> +\begin{lstlisting}
>>>> +struct virtio_admin_cmd_lreg_wr_data {
>>>> +	u8 offset; /* Starting byte offset of the register(s) to write */
>>>> +	u8 size; /* Number of bytes to write into the register. */
>>>> +	u8 register[];
>>>
>>> And maybe add
>>> 	u8 reserved[]; /* structure padding to multiple of 8 bytes */
>>>
>>>> +};
>> Thinking, what do we miss without the padding here?
> 
> it's not 100% clear where the padding is.
> 
It is same as rest of the other virtio structures as described in 
"Structure Specifications" section.

"Many device and driver in-memory structure layouts are documented using 
the C struct syntax. All structures
are assumed to be without additional padding."

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-03 17:23           ` Parav Pandit
  0 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 17:23 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs, Yishai Hadas, Maor Gottlieb



On 5/3/2023 12:49 PM, Michael S. Tsirkin wrote:
> On Wed, May 03, 2023 at 10:49:14AM -0400, Parav Pandit wrote:
>>
>>
>> On 5/3/2023 1:50 AM, Michael S. Tsirkin wrote:
>>> On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
>>
>>>> +Legacy registers write admin command follows \field{struct virtio_admin_cmd}.
>>>> +This command writes legacy registers of a member VF device. Driver should write
>>>> +appropriate register \field{size} depending on the width of the legacy
>>>> +common registers or device specific registers.
>>>> +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_WRITE.
>>>> +Driver sets \field{group_type} to 1 for VFs.
>>>> +Driver sets \field{group_member_id} to a valid VF number.
>>>> +
>>>> +The \field{command_specific_data} has following listed structure format:
>>>> +
>>>> +\begin{lstlisting}
>>>> +struct virtio_admin_cmd_lreg_wr_data {
>>>> +	u8 offset; /* Starting byte offset of the register(s) to write */
>>>> +	u8 size; /* Number of bytes to write into the register. */
>>>> +	u8 register[];
>>>
>>> And maybe add
>>> 	u8 reserved[]; /* structure padding to multiple of 8 bytes */
>>>
>>>> +};
>> Thinking, what do we miss without the padding here?
> 
> it's not 100% clear where the padding is.
> 
It is same as rest of the other virtio structures as described in 
"Structure Specifications" section.

"Many device and driver in-memory structure layouts are documented using 
the C struct syntax. All structures
are assumed to be without additional padding."

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [virtio-comment] Re: [PATCH v1 2/2] transport-pci: Add legacy register access conformance section
  2023-05-03 14:53       ` Parav Pandit
@ 2023-05-03 19:18         ` Michael S. Tsirkin
  -1 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03 19:18 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs

On Wed, May 03, 2023 at 10:53:56AM -0400, Parav Pandit wrote:
> 
> 
> On 5/3/2023 1:48 AM, Michael S. Tsirkin wrote:
> > On Wed, May 03, 2023 at 06:26:59AM +0300, Parav Pandit wrote:
> > > Add device and driver conformanace section for legacy registers access
> > > commands interface.
> > > 
> > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > > ---
> > >   conformance.tex           |  2 ++
> > >   transport-pci-vf-regs.tex | 31 +++++++++++++++++++++++++++++++
> > >   2 files changed, 33 insertions(+)
> > > 
> > > diff --git a/conformance.tex b/conformance.tex
> > > index 01ccd69..dbd8cd6 100644
> > > --- a/conformance.tex
> > > +++ b/conformance.tex
> > > @@ -109,6 +109,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> > >   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
> > >   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
> > >   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> > > +\item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> > >   \end{itemize}
> > >   \conformance{\subsection}{MMIO Driver Conformance}\label{sec:Conformance / Driver Conformance / MMIO Driver Conformance}
> > > @@ -194,6 +195,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> > >   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
> > >   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
> > >   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> > > +\item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> > >   \end{itemize}
> > >   \conformance{\subsection}{MMIO Device Conformance}\label{sec:Conformance / Device Conformance / MMIO Device Conformance}
> > > diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
> > > index 16ced32..7d0574b 100644
> > > --- a/transport-pci-vf-regs.tex
> > > +++ b/transport-pci-vf-regs.tex
> > > @@ -82,3 +82,34 @@ \subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Opti
> > >   	le64 offset; /* Byte offset within the BAR */
> > >   };
> > >   \end{lstlisting}
> > > +
> > > +\devicenormative{\paragraph}{SR-IOV VFs Legacy Registers Access}{Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> > > +
> > > +If the PCI PF device supports legacy registers access, it SHOULD set
> > > +corresponding bits for commands VIRTIO_ADMIN_CMD_LREG_WRITE,
> > > +VIRTIO_ADMIN_CMD_LREG_READ and VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY in
> > > +command result of VIRTIO_ADMIN_CMD_LIST_QUERY in
> > > +\field{device_admin_cmd_opcodes}.
> > 
> > Don't repeat documentation of VIRTIO_ADMIN_CMD_LIST_QUERY please.
> yeah, I had dual thoughts, I am fine if this looks duplicate.
> Will remove.
> 
> > just say all these must be supported.
> > In fact what are you saying here? That all 3 are supported
> > or none at all? What about just
> > VIRTIO_ADMIN_CMD_LREG_READ/VIRTIO_ADMIN_CMD_LREG_WRITE?
> > Looks like a slower but working way to do notifications
> > is through a common config write, no?
> Notifications to be done using the notification region exposed and queried
> using the 3rd QUERY command.
> > 
> > > +
> > > +The device MUST support legacy configuration registers to its defined width.
> > 
> > what is this?
> > 
> Each register has its defined width as 8-bit, 16-bit, 32-bit etc.
> So device support the access to its defined width.
> May be to rewrite it differently?

Again you are duplicating the existing text, no?
And in this case, contradicting it?

	When using the legacy interface the driver MAY access
	the device-specific configuration region using any width accesses, and
	a transitional device MUST present driver with the same results as
	when accessed using the ``natural'' access method (i.e.
	32-bit accesses for 32-bit fields, etc).


> > > +
> > > +The device MAY fail legacy configuration registers access when either the
> > > +access is for an incorrct register width or if the register offset is incorrect.
> > 
> Spell checkers didnt capture the error of incorrct. Need to find better
> tool.
> 
> > with which error code?
> EINVAL
> but should we repeat the general section here?

this is a command specific case, isn't it?


> > 
> > > +
> > > +The device MUST allow access of one or multiple bytes of the registers when
> > > +such register is defined as byte array, for example \field{mac} of \field{struct
> > > +virtio_net_config} of the Network Device.
> > 
> > so which accesses need to be supported then?
> > 
> Not sure I follow the question.
> Can you please explain?

Consider mac, do you allow access to any length at all, from 1 to 6
bytes?


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [virtio-comment] Re: [PATCH v1 2/2] transport-pci: Add legacy register access conformance section
@ 2023-05-03 19:18         ` Michael S. Tsirkin
  0 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03 19:18 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs

On Wed, May 03, 2023 at 10:53:56AM -0400, Parav Pandit wrote:
> 
> 
> On 5/3/2023 1:48 AM, Michael S. Tsirkin wrote:
> > On Wed, May 03, 2023 at 06:26:59AM +0300, Parav Pandit wrote:
> > > Add device and driver conformanace section for legacy registers access
> > > commands interface.
> > > 
> > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > > ---
> > >   conformance.tex           |  2 ++
> > >   transport-pci-vf-regs.tex | 31 +++++++++++++++++++++++++++++++
> > >   2 files changed, 33 insertions(+)
> > > 
> > > diff --git a/conformance.tex b/conformance.tex
> > > index 01ccd69..dbd8cd6 100644
> > > --- a/conformance.tex
> > > +++ b/conformance.tex
> > > @@ -109,6 +109,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> > >   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
> > >   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
> > >   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> > > +\item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> > >   \end{itemize}
> > >   \conformance{\subsection}{MMIO Driver Conformance}\label{sec:Conformance / Driver Conformance / MMIO Driver Conformance}
> > > @@ -194,6 +195,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> > >   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
> > >   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
> > >   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> > > +\item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> > >   \end{itemize}
> > >   \conformance{\subsection}{MMIO Device Conformance}\label{sec:Conformance / Device Conformance / MMIO Device Conformance}
> > > diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
> > > index 16ced32..7d0574b 100644
> > > --- a/transport-pci-vf-regs.tex
> > > +++ b/transport-pci-vf-regs.tex
> > > @@ -82,3 +82,34 @@ \subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Opti
> > >   	le64 offset; /* Byte offset within the BAR */
> > >   };
> > >   \end{lstlisting}
> > > +
> > > +\devicenormative{\paragraph}{SR-IOV VFs Legacy Registers Access}{Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> > > +
> > > +If the PCI PF device supports legacy registers access, it SHOULD set
> > > +corresponding bits for commands VIRTIO_ADMIN_CMD_LREG_WRITE,
> > > +VIRTIO_ADMIN_CMD_LREG_READ and VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY in
> > > +command result of VIRTIO_ADMIN_CMD_LIST_QUERY in
> > > +\field{device_admin_cmd_opcodes}.
> > 
> > Don't repeat documentation of VIRTIO_ADMIN_CMD_LIST_QUERY please.
> yeah, I had dual thoughts, I am fine if this looks duplicate.
> Will remove.
> 
> > just say all these must be supported.
> > In fact what are you saying here? That all 3 are supported
> > or none at all? What about just
> > VIRTIO_ADMIN_CMD_LREG_READ/VIRTIO_ADMIN_CMD_LREG_WRITE?
> > Looks like a slower but working way to do notifications
> > is through a common config write, no?
> Notifications to be done using the notification region exposed and queried
> using the 3rd QUERY command.
> > 
> > > +
> > > +The device MUST support legacy configuration registers to its defined width.
> > 
> > what is this?
> > 
> Each register has its defined width as 8-bit, 16-bit, 32-bit etc.
> So device support the access to its defined width.
> May be to rewrite it differently?

Again you are duplicating the existing text, no?
And in this case, contradicting it?

	When using the legacy interface the driver MAY access
	the device-specific configuration region using any width accesses, and
	a transitional device MUST present driver with the same results as
	when accessed using the ``natural'' access method (i.e.
	32-bit accesses for 32-bit fields, etc).


> > > +
> > > +The device MAY fail legacy configuration registers access when either the
> > > +access is for an incorrct register width or if the register offset is incorrect.
> > 
> Spell checkers didnt capture the error of incorrct. Need to find better
> tool.
> 
> > with which error code?
> EINVAL
> but should we repeat the general section here?

this is a command specific case, isn't it?


> > 
> > > +
> > > +The device MUST allow access of one or multiple bytes of the registers when
> > > +such register is defined as byte array, for example \field{mac} of \field{struct
> > > +virtio_net_config} of the Network Device.
> > 
> > so which accesses need to be supported then?
> > 
> Not sure I follow the question.
> Can you please explain?

Consider mac, do you allow access to any length at all, from 1 to 6
bytes?


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-03  3:26   ` [virtio-comment] " Parav Pandit
@ 2023-05-03 19:21     ` Michael S. Tsirkin
  -1 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03 19:21 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs

On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_wr_data {
> +	u8 offset; /* Starting byte offset of the register(s) to write */
> +	u8 size; /* Number of bytes to write into the register. */
> +	u8 register[];
> +};

BTW if we don't have padding we could reuse buffer size and won't need
size here.  Do we want to tweak admin command structure generally to use
u8 and not le64 for command data then? WDYT?

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-03 19:21     ` Michael S. Tsirkin
  0 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03 19:21 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs

On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_wr_data {
> +	u8 offset; /* Starting byte offset of the register(s) to write */
> +	u8 size; /* Number of bytes to write into the register. */
> +	u8 register[];
> +};

BTW if we don't have padding we could reuse buffer size and won't need
size here.  Do we want to tweak admin command structure generally to use
u8 and not le64 for command data then? WDYT?

-- 
MST


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-03 19:21     ` [virtio-comment] " Michael S. Tsirkin
@ 2023-05-03 19:38       ` Parav Pandit
  -1 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 19:38 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, Shahaf Shuler



On 5/3/2023 3:21 PM, Michael S. Tsirkin wrote:
> On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
>> +\begin{lstlisting}
>> +struct virtio_admin_cmd_lreg_wr_data {
>> +	u8 offset; /* Starting byte offset of the register(s) to write */
>> +	u8 size; /* Number of bytes to write into the register. */
>> +	u8 register[];
>> +};
> 
> BTW if we don't have padding we could reuse buffer size and won't need
> size here.  
Yes. sounds good.

> Do we want to tweak admin command structure generally to use
> u8 and not le64 for command data then? WDYT?
> 
yeah, cmd and result be u8 is fine.
If padding is needed for perf, may be that command can define the padding.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-03 19:38       ` Parav Pandit
  0 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 19:38 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, Shahaf Shuler



On 5/3/2023 3:21 PM, Michael S. Tsirkin wrote:
> On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
>> +\begin{lstlisting}
>> +struct virtio_admin_cmd_lreg_wr_data {
>> +	u8 offset; /* Starting byte offset of the register(s) to write */
>> +	u8 size; /* Number of bytes to write into the register. */
>> +	u8 register[];
>> +};
> 
> BTW if we don't have padding we could reuse buffer size and won't need
> size here.  
Yes. sounds good.

> Do we want to tweak admin command structure generally to use
> u8 and not le64 for command data then? WDYT?
> 
yeah, cmd and result be u8 is fine.
If padding is needed for perf, may be that command can define the padding.

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] RE: [virtio-comment] Re: [PATCH v1 2/2] transport-pci: Add legacy register access conformance section
  2023-05-03 19:18         ` Michael S. Tsirkin
@ 2023-05-03 19:56           ` Parav Pandit
  -1 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 19:56 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, Shahaf Shuler


> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, May 3, 2023 3:19 PM

[..]
> > > just say all these must be supported.
> > > In fact what are you saying here? That all 3 are supported or none
> > > at all? What about just
> > > VIRTIO_ADMIN_CMD_LREG_READ/VIRTIO_ADMIN_CMD_LREG_WRITE?
> > > Looks like a slower but working way to do notifications is through a
> > > common config write, no?
> > Notifications to be done using the notification region exposed and
> > queried using the 3rd QUERY command.
> > >
> > > > +
> > > > +The device MUST support legacy configuration registers to its defined
> width.
> > >
> > > what is this?
> > >
> > Each register has its defined width as 8-bit, 16-bit, 32-bit etc.
> > So device support the access to its defined width.
> > May be to rewrite it differently?
> 
> Again you are duplicating the existing text, no?
> And in this case, contradicting it?
> 
Yes, to both the questions.
I started with slightly sane version that contradicts below, but since below is written it what is written so it is the current version.
I will drop above normative.

> 	When using the legacy interface the driver MAY access
> 	the device-specific configuration region using any width accesses, and
> 	a transitional device MUST present driver with the same results as
> 	when accessed using the ``natural'' access method (i.e.
> 	32-bit accesses for 32-bit fields, etc).
> 
> 
> > > > +
> > > > +The device MAY fail legacy configuration registers access when
> > > > +either the access is for an incorrct register width or if the register offset
> is incorrect.
> > >
> > Spell checkers didnt capture the error of incorrct. Need to find
> > better tool.
> >
> > > with which error code?
> > EINVAL
> > but should we repeat the general section here?
> 
> this is a command specific case, isn't it?
> 
In this case the generic define error codes are covering it for command.
In this specific example, if the register offset provided is outside the register range, 
it returns VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD.
This is already defined in general section.

> 
> > >
> > > > +
> > > > +The device MUST allow access of one or multiple bytes of the
> > > > +registers when such register is defined as byte array, for
> > > > +example \field{mac} of \field{struct virtio_net_config} of the Network
> Device.
> > >
> > > so which accesses need to be supported then?
> > >
> > Not sure I follow the question.
> > Can you please explain?
> 
> Consider mac, do you allow access to any length at all, from 1 to 6 bytes?

Yes, maybe you missed the text.
I wrote it as " The device MUST allow access of one or multiple bytes of the registers when its byte array".

The other paragraph you wrote above about " When using the legacy interface, the driver", it eliminates most of the normative here.

I tried to keep it little sane, but its fine to keep it relaxed too. Sw will be able to make it strict as it finds it suitable.

So I will drop these normatives.






---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* RE: [virtio-comment] Re: [PATCH v1 2/2] transport-pci: Add legacy register access conformance section
@ 2023-05-03 19:56           ` Parav Pandit
  0 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 19:56 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, Shahaf Shuler


> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, May 3, 2023 3:19 PM

[..]
> > > just say all these must be supported.
> > > In fact what are you saying here? That all 3 are supported or none
> > > at all? What about just
> > > VIRTIO_ADMIN_CMD_LREG_READ/VIRTIO_ADMIN_CMD_LREG_WRITE?
> > > Looks like a slower but working way to do notifications is through a
> > > common config write, no?
> > Notifications to be done using the notification region exposed and
> > queried using the 3rd QUERY command.
> > >
> > > > +
> > > > +The device MUST support legacy configuration registers to its defined
> width.
> > >
> > > what is this?
> > >
> > Each register has its defined width as 8-bit, 16-bit, 32-bit etc.
> > So device support the access to its defined width.
> > May be to rewrite it differently?
> 
> Again you are duplicating the existing text, no?
> And in this case, contradicting it?
> 
Yes, to both the questions.
I started with slightly sane version that contradicts below, but since below is written it what is written so it is the current version.
I will drop above normative.

> 	When using the legacy interface the driver MAY access
> 	the device-specific configuration region using any width accesses, and
> 	a transitional device MUST present driver with the same results as
> 	when accessed using the ``natural'' access method (i.e.
> 	32-bit accesses for 32-bit fields, etc).
> 
> 
> > > > +
> > > > +The device MAY fail legacy configuration registers access when
> > > > +either the access is for an incorrct register width or if the register offset
> is incorrect.
> > >
> > Spell checkers didnt capture the error of incorrct. Need to find
> > better tool.
> >
> > > with which error code?
> > EINVAL
> > but should we repeat the general section here?
> 
> this is a command specific case, isn't it?
> 
In this case the generic define error codes are covering it for command.
In this specific example, if the register offset provided is outside the register range, 
it returns VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD.
This is already defined in general section.

> 
> > >
> > > > +
> > > > +The device MUST allow access of one or multiple bytes of the
> > > > +registers when such register is defined as byte array, for
> > > > +example \field{mac} of \field{struct virtio_net_config} of the Network
> Device.
> > >
> > > so which accesses need to be supported then?
> > >
> > Not sure I follow the question.
> > Can you please explain?
> 
> Consider mac, do you allow access to any length at all, from 1 to 6 bytes?

Yes, maybe you missed the text.
I wrote it as " The device MUST allow access of one or multiple bytes of the registers when its byte array".

The other paragraph you wrote above about " When using the legacy interface, the driver", it eliminates most of the normative here.

I tried to keep it little sane, but its fine to keep it relaxed too. Sw will be able to make it strict as it finds it suitable.

So I will drop these normatives.






This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* Re: [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-03 19:38       ` [virtio-comment] " Parav Pandit
@ 2023-05-03 20:08         ` Michael S. Tsirkin
  -1 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03 20:08 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, Shahaf Shuler

On Wed, May 03, 2023 at 03:38:31PM -0400, Parav Pandit wrote:
> 
> 
> On 5/3/2023 3:21 PM, Michael S. Tsirkin wrote:
> > On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
> > > +\begin{lstlisting}
> > > +struct virtio_admin_cmd_lreg_wr_data {
> > > +	u8 offset; /* Starting byte offset of the register(s) to write */
> > > +	u8 size; /* Number of bytes to write into the register. */
> > > +	u8 register[];
> > > +};
> > 
> > BTW if we don't have padding we could reuse buffer size and won't need
> > size here.
> Yes. sounds good.
> 
> > Do we want to tweak admin command structure generally to use
> > u8 and not le64 for command data then? WDYT?
> > 
> yeah, cmd and result be u8 is fine.
> If padding is needed for perf, may be that command can define the padding.

The commands that I defined do need u64 (much easier to
work on bitmaps in fixed size chunks), so they will use that.

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-03 20:08         ` Michael S. Tsirkin
  0 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-03 20:08 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, Shahaf Shuler

On Wed, May 03, 2023 at 03:38:31PM -0400, Parav Pandit wrote:
> 
> 
> On 5/3/2023 3:21 PM, Michael S. Tsirkin wrote:
> > On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
> > > +\begin{lstlisting}
> > > +struct virtio_admin_cmd_lreg_wr_data {
> > > +	u8 offset; /* Starting byte offset of the register(s) to write */
> > > +	u8 size; /* Number of bytes to write into the register. */
> > > +	u8 register[];
> > > +};
> > 
> > BTW if we don't have padding we could reuse buffer size and won't need
> > size here.
> Yes. sounds good.
> 
> > Do we want to tweak admin command structure generally to use
> > u8 and not le64 for command data then? WDYT?
> > 
> yeah, cmd and result be u8 is fine.
> If padding is needed for perf, may be that command can define the padding.

The commands that I defined do need u64 (much easier to
work on bitmaps in fixed size chunks), so they will use that.

-- 
MST


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* RE: [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-03 20:08         ` [virtio-comment] " Michael S. Tsirkin
@ 2023-05-03 20:13           ` Parav Pandit
  -1 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 20:13 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, Shahaf Shuler, Yishai Hadas, Maor Gottlieb

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, May 3, 2023 4:09 PM
> 
> On Wed, May 03, 2023 at 03:38:31PM -0400, Parav Pandit wrote:
> >
> >
> > On 5/3/2023 3:21 PM, Michael S. Tsirkin wrote:
> > > On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
> > > > +\begin{lstlisting}
> > > > +struct virtio_admin_cmd_lreg_wr_data {
> > > > +	u8 offset; /* Starting byte offset of the register(s) to write */
> > > > +	u8 size; /* Number of bytes to write into the register. */
> > > > +	u8 register[];
> > > > +};
> > >
> > > BTW if we don't have padding we could reuse buffer size and won't
> > > need size here.
> > Yes. sounds good.
> >
> > > Do we want to tweak admin command structure generally to use
> > > u8 and not le64 for command data then? WDYT?
> > >
> > yeah, cmd and result be u8 is fine.
> > If padding is needed for perf, may be that command can define the padding.
> 
> The commands that I defined do need u64 (much easier to work on bitmaps in
> fixed size chunks), so they will use that.

You have already defined it as le64 field. See it below.
So it is not padding, the field itself is le64.
So no need to typecast it differently based on the descriptor size because it is always array of le64.
It behaves as padding when number of bits are < 64, but it is really not a (dynamic) padding.

le64 device_admin_cmd_opcodes[];

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] RE: [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-03 20:13           ` Parav Pandit
  0 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-03 20:13 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, Shahaf Shuler, Yishai Hadas, Maor Gottlieb

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, May 3, 2023 4:09 PM
> 
> On Wed, May 03, 2023 at 03:38:31PM -0400, Parav Pandit wrote:
> >
> >
> > On 5/3/2023 3:21 PM, Michael S. Tsirkin wrote:
> > > On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
> > > > +\begin{lstlisting}
> > > > +struct virtio_admin_cmd_lreg_wr_data {
> > > > +	u8 offset; /* Starting byte offset of the register(s) to write */
> > > > +	u8 size; /* Number of bytes to write into the register. */
> > > > +	u8 register[];
> > > > +};
> > >
> > > BTW if we don't have padding we could reuse buffer size and won't
> > > need size here.
> > Yes. sounds good.
> >
> > > Do we want to tweak admin command structure generally to use
> > > u8 and not le64 for command data then? WDYT?
> > >
> > yeah, cmd and result be u8 is fine.
> > If padding is needed for perf, may be that command can define the padding.
> 
> The commands that I defined do need u64 (much easier to work on bitmaps in
> fixed size chunks), so they will use that.

You have already defined it as le64 field. See it below.
So it is not padding, the field itself is le64.
So no need to typecast it differently based on the descriptor size because it is always array of le64.
It behaves as padding when number of bits are < 64, but it is really not a (dynamic) padding.

le64 device_admin_cmd_opcodes[];

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-03 17:23           ` [virtio-comment] " Parav Pandit
@ 2023-05-04  6:30             ` Michael S. Tsirkin
  -1 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-04  6:30 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs, Yishai Hadas, Maor Gottlieb

On Wed, May 03, 2023 at 01:23:55PM -0400, Parav Pandit wrote:
> 
> 
> On 5/3/2023 12:49 PM, Michael S. Tsirkin wrote:
> > On Wed, May 03, 2023 at 10:49:14AM -0400, Parav Pandit wrote:
> > > 
> > > 
> > > On 5/3/2023 1:50 AM, Michael S. Tsirkin wrote:
> > > > On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
> > > 
> > > > > +Legacy registers write admin command follows \field{struct virtio_admin_cmd}.
> > > > > +This command writes legacy registers of a member VF device. Driver should write
> > > > > +appropriate register \field{size} depending on the width of the legacy
> > > > > +common registers or device specific registers.
> > > > > +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_WRITE.
> > > > > +Driver sets \field{group_type} to 1 for VFs.
> > > > > +Driver sets \field{group_member_id} to a valid VF number.
> > > > > +
> > > > > +The \field{command_specific_data} has following listed structure format:
> > > > > +
> > > > > +\begin{lstlisting}
> > > > > +struct virtio_admin_cmd_lreg_wr_data {
> > > > > +	u8 offset; /* Starting byte offset of the register(s) to write */
> > > > > +	u8 size; /* Number of bytes to write into the register. */
> > > > > +	u8 register[];
> > > > 
> > > > And maybe add
> > > > 	u8 reserved[]; /* structure padding to multiple of 8 bytes */
> > > > 
> > > > > +};
> > > Thinking, what do we miss without the padding here?
> > 
> > it's not 100% clear where the padding is.
> > 
> It is same as rest of the other virtio structures as described in "Structure
> Specifications" section.
> 
> "Many device and driver in-memory structure layouts are documented using the
> C struct syntax. All structures
> are assumed to be without additional padding."

exactly, so please include the padding explicitly.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-04  6:30             ` Michael S. Tsirkin
  0 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-04  6:30 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs, Yishai Hadas, Maor Gottlieb

On Wed, May 03, 2023 at 01:23:55PM -0400, Parav Pandit wrote:
> 
> 
> On 5/3/2023 12:49 PM, Michael S. Tsirkin wrote:
> > On Wed, May 03, 2023 at 10:49:14AM -0400, Parav Pandit wrote:
> > > 
> > > 
> > > On 5/3/2023 1:50 AM, Michael S. Tsirkin wrote:
> > > > On Wed, May 03, 2023 at 06:26:58AM +0300, Parav Pandit wrote:
> > > 
> > > > > +Legacy registers write admin command follows \field{struct virtio_admin_cmd}.
> > > > > +This command writes legacy registers of a member VF device. Driver should write
> > > > > +appropriate register \field{size} depending on the width of the legacy
> > > > > +common registers or device specific registers.
> > > > > +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_WRITE.
> > > > > +Driver sets \field{group_type} to 1 for VFs.
> > > > > +Driver sets \field{group_member_id} to a valid VF number.
> > > > > +
> > > > > +The \field{command_specific_data} has following listed structure format:
> > > > > +
> > > > > +\begin{lstlisting}
> > > > > +struct virtio_admin_cmd_lreg_wr_data {
> > > > > +	u8 offset; /* Starting byte offset of the register(s) to write */
> > > > > +	u8 size; /* Number of bytes to write into the register. */
> > > > > +	u8 register[];
> > > > 
> > > > And maybe add
> > > > 	u8 reserved[]; /* structure padding to multiple of 8 bytes */
> > > > 
> > > > > +};
> > > Thinking, what do we miss without the padding here?
> > 
> > it's not 100% clear where the padding is.
> > 
> It is same as rest of the other virtio structures as described in "Structure
> Specifications" section.
> 
> "Many device and driver in-memory structure layouts are documented using the
> C struct syntax. All structures
> are assumed to be without additional padding."

exactly, so please include the padding explicitly.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-03 17:21           ` [virtio-comment] " Parav Pandit
@ 2023-05-04  6:30             ` Michael S. Tsirkin
  -1 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-04  6:30 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs, Yishai Hadas, Maor Gottlieb

On Wed, May 03, 2023 at 01:21:39PM -0400, Parav Pandit wrote:
> 
> 
> On 5/3/2023 12:48 PM, Michael S. Tsirkin wrote:
> 
> > > > As simple as it is, I feel this falls far short of describing how
> > > > a device should operate.
> > > > Some issues:
> > > > 	- legacy device config offset changes as msi is enabled/disabled
> > > > 	  suggest separate commands for device/common config
> > > This is fine and covered here. The one who is making msix enable/disable
> > > knows which registers to access before/after disable/enable and device also
> > > knows it as it is supplying the register.
> > > So they just follow the standard legacy register access behavior.
> > 
> > But do VFs support INT#x? I will have to re-read the spec.
> > 
> VFs do not support INTx.
> When hypervisor knows that it cannot support msix for the guest, it can
> avoid using the VF for the guest.

it's the guest that does not support msix. Not host.

> > > > 	- legacy device endian-ness changes with guest
> > > > 	  suggest commands to enable LE and BE mode
> > > guest endianeness is not known to the device.
> > 
> > But is known to hypervisor.
> > 
> It can be an extension command in future as part of the VF administration
> command to set it by the hypervisor PF.
> 
> > > Currently it is only for the
> > > LE guests who had some legacy requirement.
> > 
> > I don't like tying this to LE implicitly some devices might be BE only.
> > With my idea device can support command to set LE, command to set BE or
> > both.
> > 
> It can be an addition in future if needed.
> 
> > > and PCIe is LE anyway.
> > 
> > PCIE config endian-ness does not matter heere.
> > 
> > > > 	- legacy guests often assume INT#x support
> > > > 	  suggest a way to tunnel that too;
> > > INT#x is present on the PCI device itself. So no need to tunnel it.
> > > Also INT#x is very narrow case. When MSI-X is not supported, a hypervisor
> > > can choose not to even connect such device to the guest VM.
> > 
> > devices will support MSI. But *guest* might not support MSIX you only
> > find out late when it is driving the device.
> > 
> It is a pathological case that may not exist because legacy drivers and
> linux kernels all the way upto 2.6.32 have msix support.
> 
> And in case if hypervisor sw wants to support for unknown scenario, it can
> use the one msix based interrupt to emulate intx.
> 
> > > > I expected to see more statements along the lines of
> > > >           command ABC has the same effect as access
> > > >           to register DEF of the member through the legacy pci interface
> > > > 
> > > Yes, good point. I will add it in the theory of operation section for this
> > > mapping detail.
> > 
> > OK, and overall if you see an existing statement about legacy do not
> > copy it, just explain how it is mapped.
> > 
> Yes, will not copy, only the mapping part to add.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-04  6:30             ` Michael S. Tsirkin
  0 siblings, 0 replies; 54+ messages in thread
From: Michael S. Tsirkin @ 2023-05-04  6:30 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-dev, cohuck, david.edmondson, sburla, jasowang,
	virtio-comment, shahafs, Yishai Hadas, Maor Gottlieb

On Wed, May 03, 2023 at 01:21:39PM -0400, Parav Pandit wrote:
> 
> 
> On 5/3/2023 12:48 PM, Michael S. Tsirkin wrote:
> 
> > > > As simple as it is, I feel this falls far short of describing how
> > > > a device should operate.
> > > > Some issues:
> > > > 	- legacy device config offset changes as msi is enabled/disabled
> > > > 	  suggest separate commands for device/common config
> > > This is fine and covered here. The one who is making msix enable/disable
> > > knows which registers to access before/after disable/enable and device also
> > > knows it as it is supplying the register.
> > > So they just follow the standard legacy register access behavior.
> > 
> > But do VFs support INT#x? I will have to re-read the spec.
> > 
> VFs do not support INTx.
> When hypervisor knows that it cannot support msix for the guest, it can
> avoid using the VF for the guest.

it's the guest that does not support msix. Not host.

> > > > 	- legacy device endian-ness changes with guest
> > > > 	  suggest commands to enable LE and BE mode
> > > guest endianeness is not known to the device.
> > 
> > But is known to hypervisor.
> > 
> It can be an extension command in future as part of the VF administration
> command to set it by the hypervisor PF.
> 
> > > Currently it is only for the
> > > LE guests who had some legacy requirement.
> > 
> > I don't like tying this to LE implicitly some devices might be BE only.
> > With my idea device can support command to set LE, command to set BE or
> > both.
> > 
> It can be an addition in future if needed.
> 
> > > and PCIe is LE anyway.
> > 
> > PCIE config endian-ness does not matter heere.
> > 
> > > > 	- legacy guests often assume INT#x support
> > > > 	  suggest a way to tunnel that too;
> > > INT#x is present on the PCI device itself. So no need to tunnel it.
> > > Also INT#x is very narrow case. When MSI-X is not supported, a hypervisor
> > > can choose not to even connect such device to the guest VM.
> > 
> > devices will support MSI. But *guest* might not support MSIX you only
> > find out late when it is driving the device.
> > 
> It is a pathological case that may not exist because legacy drivers and
> linux kernels all the way upto 2.6.32 have msix support.
> 
> And in case if hypervisor sw wants to support for unknown scenario, it can
> use the one msix based interrupt to emulate intx.
> 
> > > > I expected to see more statements along the lines of
> > > >           command ABC has the same effect as access
> > > >           to register DEF of the member through the legacy pci interface
> > > > 
> > > Yes, good point. I will add it in the theory of operation section for this
> > > mapping detail.
> > 
> > OK, and overall if you see an existing statement about legacy do not
> > copy it, just explain how it is mapped.
> > 
> Yes, will not copy, only the mapping part to add.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-03  3:26   ` [virtio-comment] " Parav Pandit
@ 2023-05-05  3:26     ` Jason Wang
  -1 siblings, 0 replies; 54+ messages in thread
From: Jason Wang @ 2023-05-05  3:26 UTC (permalink / raw)
  To: Parav Pandit, mst, virtio-dev, cohuck, david.edmondson
  Cc: sburla, virtio-comment, shahafs, Zhu, Lingshan


在 2023/5/3 11:26, Parav Pandit 写道:
> This patch introduces legacy registers access commands for the owner
> group member PCI PF to access the legacy registers of the member VFs.
>
> If in future any SIOV devices to support legacy registers, they
> can be easily supported using same commands by using the group
> member identifiers of the future SIOV devices.
>
> More details as overview, motivation, use case are further described
> below.
>
> Usecase:
> --------
> 1. A hypervisor/system needs to provide transitional
>     virtio devices to the guest VM at scale of thousands,
>     typically, one to eight devices per VM.
>
> 2. A hypervisor/system needs to provide such devices using a
>     vendor agnostic driver in the hypervisor system.
>
> 3. A hypervisor system prefers to have single stack regardless of


So the "single stack" is kind of misleading, you need a dedicated virtio 
mediation layer which has different code path than a simpler vfio-pci 
which is completely duplicated with vDPA subsystem. And you lose all the 
advantages of vDPA in this way. The device should not be designed for a 
single type of software stack , it need to leave the decision to the 
hypervisor/cloud vendors.


>     virtio device type (net/blk) and be future compatible with a
>     single vfio stack using SR-IOV or other scalable device
>     virtualization technology to map PCI devices to the guest VM.
>     (as transitional or otherwise)
>
> Motivation/Background:
> ----------------------
> The existing virtio transitional PCI device is missing support for
> PCI SR-IOV based devices. Currently it does not work beyond
> PCI PF, or as software emulated device in reality. Currently it
> has below cited system level limitations:
>
> [a] PCIe spec citation:
> VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.
>
> [b] cpu arch citiation:
> Intel 64 and IA-32 Architectures Software Developer’s Manual:
> The processor’s I/O address space is separate and distinct from
> the physical-memory address space. The I/O address space consists
> of 64K individually addressable 8-bit I/O ports, numbered 0 through FFFFH.
>
> [c] PCIe spec citation:
> If a bridge implements an I/O address range,...I/O address range will be
> aligned to a 4 KB boundary.
>
> Above usecase requirements can be solved by PCI PF group owner enabling
> the access to its group member PCI VFs legacy registers using an admin
> virtqueue of the group owner PCI PF.
>
> Software usage example:
> -----------------------
> The most common way to use and map to the guest VM is by
> using vfio driver framework in Linux kernel.
>
>                  +----------------------+
>                  |pci_dev_id = 0x100X   |
> +---------------|pci_rev_id = 0x0      |-----+
> |vfio device    |BAR0 = I/O region     |     |
> |               |Other attributes      |     |
> |               +----------------------+     |
> |                                            |
> +   +--------------+     +-----------------+ |
> |   |I/O BAR to AQ |     | Other vfio      | |
> |   |rd/wr mapper  |     | functionalities | |
> |   +--------------+     +-----------------+ |
> |                                            |
> +------+-------------------------+-----------+
>         |                         |


So the mapper here is actually the control path mediation layer which 
duplicates with vDPA.


>    +----+------------+       +----+------------+
>    | +-----+         |       | PCI VF device A |
>    | | AQ  |-------------+---->+-------------+ |
>    | +-----+         |   |   | | legacy regs | |
>    | PCI PF device   |   |   | +-------------+ |
>    +-----------------+   |   +-----------------+
>                          |
>                          |   +----+------------+
>                          |   | PCI VF device N |
>                          +---->+-------------+ |
>                              | | legacy regs | |
>                              | +-------------+ |
>                              +-----------------+
>
> 2. Virtio pci driver to bind to the listed device id and
>     use it as native device in the host.


How this can be done now?


>
> 3. Use it in a light weight hypervisor to run bare-metal OS.
>
> Please review.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> Signed-off-by: Parav Pandit <parav@nvidia.com>
>
> ---
> changelog:
> v0->v1:
> - addressed comments, suggesetions and ideas from Michael Tsirkin and Jason Wang
> - far more simpler design than MMR access
> - removed complexities of MMR device ids
> - removed complexities of MMR registers and extended capabilities
> - dropped adding new extended capabilities because if if they are
>    added, a pci device still needs to have existing capabilities
>    in the legacy configuration space and hypervisor driver do not
>    need to access them
> ---
>   admin.tex                 |  5 ++-
>   transport-pci-vf-regs.tex | 84 +++++++++++++++++++++++++++++++++++++++
>   transport-pci.tex         |  2 +
>   3 files changed, 90 insertions(+), 1 deletion(-)
>   create mode 100644 transport-pci-vf-regs.tex
>
> diff --git a/admin.tex b/admin.tex
> index 648253c..852ee04 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -115,7 +115,10 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>   \hline \hline
>   0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
>   0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
> -0x0002 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
> +0x0002 & VIRTIO_ADMIN_CMD_LREG_WRITE & Write legacy registers of a member device    \\
> +0x0003 & VIRTIO_ADMIN_CMD_LREG_READ & Read legacy registers of a member device    \\
> +0x0004 & VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY & Read the queue notification offset for legacy interface \\
> +0x0005 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
>   \hline
>   0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
>   \hline
> diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
> new file mode 100644
> index 0000000..16ced32
> --- /dev/null
> +++ b/transport-pci-vf-regs.tex
> @@ -0,0 +1,84 @@
> +\subsection{SR-IOV VFs Legacy Registers Access}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> +
> +As described in PCIe base specification \hyperref[intro:PCIe]{[PCIe]} PCI VFs
> +do not support IOBAR. A PCI PF device can optionally enable driver to access
> +its member PCI VFs devices legacy common configuration and device configuration
> +registers using an administration virtqueue. A PCI PF group owner device that
> +supports its member VFs legacy registers access via the administration
> +virtqueue should supports following commands.
> +
> +\begin{enumerate}
> +\item Legacy Registers Write
> +\item Legacy Registers Read
> +\item Legacy Queue Notify Offset Query
> +\end{enumerate}
> +
> +\subsubsection{Legacy Registers Write}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Registers Write}
> +
> +Legacy registers write admin command follows \field{struct virtio_admin_cmd}.
> +This command writes legacy registers of a member VF device. Driver should write
> +appropriate register \field{size} depending on the width of the legacy
> +common registers or device specific registers.
> +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_WRITE.
> +Driver sets \field{group_type} to 1 for VFs.
> +Driver sets \field{group_member_id} to a valid VF number.
> +
> +The \field{command_specific_data} has following listed structure format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_wr_data {
> +	u8 offset; /* Starting byte offset of the register(s) to write */
> +	u8 size; /* Number of bytes to write into the register. */
> +	u8 register[];
> +};
> +\end{lstlisting}


So this actually implements a transport, I wonder if it would be better 
(and simpler) to do it on top of the transport vq proposal:

https://lists.oasis-open.org/archives/virtio-comment/202208/msg00003.html

Then it aligns with SIOV natively.

Or to make it more general like PCI over admin virtqueue.

Thanks


> +
> +This command does not have any command specific result.
> +
> +\subsubsection{Legacy Registers Read}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Registers Read}
> +
> +Legacy registers read admin command follows \field{struct virtio_admin_cmd}.
> +This command reads legacy registers of a member VF device. Driver should write
> +appropriate register \field{size} depending on the width of the legacy
> +common configuration registers or device specific registers.
> +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_READ.
> +Driver sets \field{group_type} to 1 for VFs.
> +Driver sets \field{group_member_id} to a valid VF number.
> +
> +The \field{command_specific_data} has following listed structure format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_rd_data {
> +	u8 offset; /* Starting byte offset of the register to read */
> +	u8 size; /* Number of bytes to read from the registers */
> +};
> +\end{lstlisting}
> +
> +When command completes successfully, command result contains following
> +listed content:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_rd_result {
> +	u8 registers[];
> +};
> +\end{lstlisting}
> +
> +\subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Queue Notify Offset Query}
> +
> +This command returns the notify offset of the member VF for queue
> +notifications. This command follows \field{struct virtio_admin_cmd}.
> +Driver sets command opcode \field{opcode} to VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY.
> +There is no command specific data for this command.
> +Driver sets \field{group_type} to 1.
> +Driver sets \field{group_member_id} to a valid VF number.
> +
> +When command completes successfully, command result contains the queue
> +notification address in the listed format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lq_notify_query_result {
> +	u8 bar; /* PCI BAR number of the member VF */
> +	u8 reserved[7];
> +	le64 offset; /* Byte offset within the BAR */
> +};
> +\end{lstlisting}
> diff --git a/transport-pci.tex b/transport-pci.tex
> index ff889d3..b187576 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -1179,3 +1179,5 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
>           re-examine the configuration space to see what changed.
>       \end{itemize}
>   \end{itemize}
> +
> +\input{transport-pci-vf-regs.tex}


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-05  3:26     ` Jason Wang
  0 siblings, 0 replies; 54+ messages in thread
From: Jason Wang @ 2023-05-05  3:26 UTC (permalink / raw)
  To: Parav Pandit, mst, virtio-dev, cohuck, david.edmondson
  Cc: sburla, virtio-comment, shahafs, Zhu, Lingshan


在 2023/5/3 11:26, Parav Pandit 写道:
> This patch introduces legacy registers access commands for the owner
> group member PCI PF to access the legacy registers of the member VFs.
>
> If in future any SIOV devices to support legacy registers, they
> can be easily supported using same commands by using the group
> member identifiers of the future SIOV devices.
>
> More details as overview, motivation, use case are further described
> below.
>
> Usecase:
> --------
> 1. A hypervisor/system needs to provide transitional
>     virtio devices to the guest VM at scale of thousands,
>     typically, one to eight devices per VM.
>
> 2. A hypervisor/system needs to provide such devices using a
>     vendor agnostic driver in the hypervisor system.
>
> 3. A hypervisor system prefers to have single stack regardless of


So the "single stack" is kind of misleading, you need a dedicated virtio 
mediation layer which has different code path than a simpler vfio-pci 
which is completely duplicated with vDPA subsystem. And you lose all the 
advantages of vDPA in this way. The device should not be designed for a 
single type of software stack , it need to leave the decision to the 
hypervisor/cloud vendors.


>     virtio device type (net/blk) and be future compatible with a
>     single vfio stack using SR-IOV or other scalable device
>     virtualization technology to map PCI devices to the guest VM.
>     (as transitional or otherwise)
>
> Motivation/Background:
> ----------------------
> The existing virtio transitional PCI device is missing support for
> PCI SR-IOV based devices. Currently it does not work beyond
> PCI PF, or as software emulated device in reality. Currently it
> has below cited system level limitations:
>
> [a] PCIe spec citation:
> VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.
>
> [b] cpu arch citiation:
> Intel 64 and IA-32 Architectures Software Developer’s Manual:
> The processor’s I/O address space is separate and distinct from
> the physical-memory address space. The I/O address space consists
> of 64K individually addressable 8-bit I/O ports, numbered 0 through FFFFH.
>
> [c] PCIe spec citation:
> If a bridge implements an I/O address range,...I/O address range will be
> aligned to a 4 KB boundary.
>
> Above usecase requirements can be solved by PCI PF group owner enabling
> the access to its group member PCI VFs legacy registers using an admin
> virtqueue of the group owner PCI PF.
>
> Software usage example:
> -----------------------
> The most common way to use and map to the guest VM is by
> using vfio driver framework in Linux kernel.
>
>                  +----------------------+
>                  |pci_dev_id = 0x100X   |
> +---------------|pci_rev_id = 0x0      |-----+
> |vfio device    |BAR0 = I/O region     |     |
> |               |Other attributes      |     |
> |               +----------------------+     |
> |                                            |
> +   +--------------+     +-----------------+ |
> |   |I/O BAR to AQ |     | Other vfio      | |
> |   |rd/wr mapper  |     | functionalities | |
> |   +--------------+     +-----------------+ |
> |                                            |
> +------+-------------------------+-----------+
>         |                         |


So the mapper here is actually the control path mediation layer which 
duplicates with vDPA.


>    +----+------------+       +----+------------+
>    | +-----+         |       | PCI VF device A |
>    | | AQ  |-------------+---->+-------------+ |
>    | +-----+         |   |   | | legacy regs | |
>    | PCI PF device   |   |   | +-------------+ |
>    +-----------------+   |   +-----------------+
>                          |
>                          |   +----+------------+
>                          |   | PCI VF device N |
>                          +---->+-------------+ |
>                              | | legacy regs | |
>                              | +-------------+ |
>                              +-----------------+
>
> 2. Virtio pci driver to bind to the listed device id and
>     use it as native device in the host.


How this can be done now?


>
> 3. Use it in a light weight hypervisor to run bare-metal OS.
>
> Please review.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> Signed-off-by: Parav Pandit <parav@nvidia.com>
>
> ---
> changelog:
> v0->v1:
> - addressed comments, suggesetions and ideas from Michael Tsirkin and Jason Wang
> - far more simpler design than MMR access
> - removed complexities of MMR device ids
> - removed complexities of MMR registers and extended capabilities
> - dropped adding new extended capabilities because if if they are
>    added, a pci device still needs to have existing capabilities
>    in the legacy configuration space and hypervisor driver do not
>    need to access them
> ---
>   admin.tex                 |  5 ++-
>   transport-pci-vf-regs.tex | 84 +++++++++++++++++++++++++++++++++++++++
>   transport-pci.tex         |  2 +
>   3 files changed, 90 insertions(+), 1 deletion(-)
>   create mode 100644 transport-pci-vf-regs.tex
>
> diff --git a/admin.tex b/admin.tex
> index 648253c..852ee04 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -115,7 +115,10 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>   \hline \hline
>   0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
>   0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
> -0x0002 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
> +0x0002 & VIRTIO_ADMIN_CMD_LREG_WRITE & Write legacy registers of a member device    \\
> +0x0003 & VIRTIO_ADMIN_CMD_LREG_READ & Read legacy registers of a member device    \\
> +0x0004 & VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY & Read the queue notification offset for legacy interface \\
> +0x0005 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
>   \hline
>   0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
>   \hline
> diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
> new file mode 100644
> index 0000000..16ced32
> --- /dev/null
> +++ b/transport-pci-vf-regs.tex
> @@ -0,0 +1,84 @@
> +\subsection{SR-IOV VFs Legacy Registers Access}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> +
> +As described in PCIe base specification \hyperref[intro:PCIe]{[PCIe]} PCI VFs
> +do not support IOBAR. A PCI PF device can optionally enable driver to access
> +its member PCI VFs devices legacy common configuration and device configuration
> +registers using an administration virtqueue. A PCI PF group owner device that
> +supports its member VFs legacy registers access via the administration
> +virtqueue should supports following commands.
> +
> +\begin{enumerate}
> +\item Legacy Registers Write
> +\item Legacy Registers Read
> +\item Legacy Queue Notify Offset Query
> +\end{enumerate}
> +
> +\subsubsection{Legacy Registers Write}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Registers Write}
> +
> +Legacy registers write admin command follows \field{struct virtio_admin_cmd}.
> +This command writes legacy registers of a member VF device. Driver should write
> +appropriate register \field{size} depending on the width of the legacy
> +common registers or device specific registers.
> +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_WRITE.
> +Driver sets \field{group_type} to 1 for VFs.
> +Driver sets \field{group_member_id} to a valid VF number.
> +
> +The \field{command_specific_data} has following listed structure format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_wr_data {
> +	u8 offset; /* Starting byte offset of the register(s) to write */
> +	u8 size; /* Number of bytes to write into the register. */
> +	u8 register[];
> +};
> +\end{lstlisting}


So this actually implements a transport, I wonder if it would be better 
(and simpler) to do it on top of the transport vq proposal:

https://lists.oasis-open.org/archives/virtio-comment/202208/msg00003.html

Then it aligns with SIOV natively.

Or to make it more general like PCI over admin virtqueue.

Thanks


> +
> +This command does not have any command specific result.
> +
> +\subsubsection{Legacy Registers Read}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Registers Read}
> +
> +Legacy registers read admin command follows \field{struct virtio_admin_cmd}.
> +This command reads legacy registers of a member VF device. Driver should write
> +appropriate register \field{size} depending on the width of the legacy
> +common configuration registers or device specific registers.
> +Driver sets command \field{opcode} to VIRTIO_ADMIN_CMD_LREG_READ.
> +Driver sets \field{group_type} to 1 for VFs.
> +Driver sets \field{group_member_id} to a valid VF number.
> +
> +The \field{command_specific_data} has following listed structure format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_rd_data {
> +	u8 offset; /* Starting byte offset of the register to read */
> +	u8 size; /* Number of bytes to read from the registers */
> +};
> +\end{lstlisting}
> +
> +When command completes successfully, command result contains following
> +listed content:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lreg_rd_result {
> +	u8 registers[];
> +};
> +\end{lstlisting}
> +
> +\subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access / Legacy Queue Notify Offset Query}
> +
> +This command returns the notify offset of the member VF for queue
> +notifications. This command follows \field{struct virtio_admin_cmd}.
> +Driver sets command opcode \field{opcode} to VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY.
> +There is no command specific data for this command.
> +Driver sets \field{group_type} to 1.
> +Driver sets \field{group_member_id} to a valid VF number.
> +
> +When command completes successfully, command result contains the queue
> +notification address in the listed format:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_lq_notify_query_result {
> +	u8 bar; /* PCI BAR number of the member VF */
> +	u8 reserved[7];
> +	le64 offset; /* Byte offset within the BAR */
> +};
> +\end{lstlisting}
> diff --git a/transport-pci.tex b/transport-pci.tex
> index ff889d3..b187576 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -1179,3 +1179,5 @@ \subsubsection{Driver Handling Interrupts}\label{sec:Virtio Transport Options /
>           re-examine the configuration space to see what changed.
>       \end{itemize}
>   \end{itemize}
> +
> +\input{transport-pci-vf-regs.tex}


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] RE: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-05  3:26     ` [virtio-comment] " Jason Wang
@ 2023-05-05 12:48       ` Parav Pandit
  -1 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-05 12:48 UTC (permalink / raw)
  To: Jason Wang, mst, virtio-dev, cohuck, david.edmondson
  Cc: sburla, virtio-comment, Shahaf Shuler, Zhu, Lingshan



> From: Jason Wang <jasowang@redhat.com>
> Sent: Thursday, May 4, 2023 11:27 PM
> 
> So the "single stack" is kind of misleading, you need a dedicated virtio
> mediation layer which has different code path than a simpler vfio-pci which is
> completely duplicated with vDPA subsystem. 
Huh. No. it is not duplicated. 
Vfio-pci provides the framework for extension than doing simple vfio-pci for extensions.
I am not debating here vdpa vs non vdpa yet again.

> And you lose all the advantages of
> vDPA in this way. The device should not be designed for a single type of
> software stack , it need to leave the decision to the hypervisor/cloud vendors.
>
It is left to the hypervisor/cloud user to decide to use vdpa or vfio or something else.
 
> 
> >     virtio device type (net/blk) and be future compatible with a
> >     single vfio stack using SR-IOV or other scalable device
> >     virtualization technology to map PCI devices to the guest VM.
> >     (as transitional or otherwise)
> >
> > Motivation/Background:
> > ----------------------
> > The existing virtio transitional PCI device is missing support for PCI
> > SR-IOV based devices. Currently it does not work beyond PCI PF, or as
> > software emulated device in reality. Currently it has below cited
> > system level limitations:
> >
> > [a] PCIe spec citation:
> > VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.
> >
> > [b] cpu arch citiation:
> > Intel 64 and IA-32 Architectures Software Developer’s Manual:
> > The processor’s I/O address space is separate and distinct from the
> > physical-memory address space. The I/O address space consists of 64K
> > individually addressable 8-bit I/O ports, numbered 0 through FFFFH.
> >
> > [c] PCIe spec citation:
> > If a bridge implements an I/O address range,...I/O address range will
> > be aligned to a 4 KB boundary.
> >
> > Above usecase requirements can be solved by PCI PF group owner
> > enabling the access to its group member PCI VFs legacy registers using
> > an admin virtqueue of the group owner PCI PF.
> >
> > Software usage example:
> > -----------------------
> > The most common way to use and map to the guest VM is by using vfio
> > driver framework in Linux kernel.
> >
> >                  +----------------------+
> >                  |pci_dev_id = 0x100X   |
> > +---------------|pci_rev_id = 0x0      |-----+
> > |vfio device    |BAR0 = I/O region     |     |
> > |               |Other attributes      |     |
> > |               +----------------------+     |
> > |                                            |
> > +   +--------------+     +-----------------+ |
> > |   |I/O BAR to AQ |     | Other vfio      | |
> > |   |rd/wr mapper  |     | functionalities | |
> > |   +--------------+     +-----------------+ |
> > |                                            |
> > +------+-------------------------+-----------+
> >         |                         |
> 
> 
> So the mapper here is actually the control path mediation layer which
> duplicates with vDPA.
> 
Yet again no. It implements PCI level abstraction.
It is not touching the whole QEMU layer and not at all getting involved in virtio device flow of understanding device reset, and device config space, cvq, features bits and more.
All of these were discussed in v0, lets not repeat.

> 
> >    +----+------------+       +----+------------+
> >    | +-----+         |       | PCI VF device A |
> >    | | AQ  |-------------+---->+-------------+ |
> >    | +-----+         |   |   | | legacy regs | |
> >    | PCI PF device   |   |   | +-------------+ |
> >    +-----------------+   |   +-----------------+
> >                          |
> >                          |   +----+------------+
> >                          |   | PCI VF device N |
> >                          +---->+-------------+ |
> >                              | | legacy regs | |
> >                              | +-------------+ |
> >                              +-----------------+
> >
> > 2. Virtio pci driver to bind to the listed device id and
> >     use it as native device in the host.
> 
> 
> How this can be done now?
> 
Currently a PCI VF binds to the virtio driver and without any vdpa layering, virtio net/blk etc devices are created on top of virtio PCI VF device.
Not sure I understood your question.

> > +\begin{lstlisting}
> > +struct virtio_admin_cmd_lreg_wr_data {
> > +	u8 offset; /* Starting byte offset of the register(s) to write */
> > +	u8 size; /* Number of bytes to write into the register. */
> > +	u8 register[];
> > +};
> > +\end{lstlisting}
> 
> 
> So this actually implements a transport, I wonder if it would be better
> (and simpler) to do it on top of the transport vq proposal:
> 
> https://lists.oasis-open.org/archives/virtio-comment/202208/msg00003.html
> 
I also wonder why TVQ cannot use AQ.

> Then it aligns with SIOV natively.
>
SIOV is not well defined spec, whenever it is defined, it can use AQ or TVQ.

We also discussed that mediation of hypervisor for control path in some use case is not desired, hence I will leave that discussion to the future when SIOV arrives.

^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] RE: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-05 12:48       ` Parav Pandit
  0 siblings, 0 replies; 54+ messages in thread
From: Parav Pandit @ 2023-05-05 12:48 UTC (permalink / raw)
  To: Jason Wang, mst, virtio-dev, cohuck, david.edmondson
  Cc: sburla, virtio-comment, Shahaf Shuler, Zhu, Lingshan



> From: Jason Wang <jasowang@redhat.com>
> Sent: Thursday, May 4, 2023 11:27 PM
> 
> So the "single stack" is kind of misleading, you need a dedicated virtio
> mediation layer which has different code path than a simpler vfio-pci which is
> completely duplicated with vDPA subsystem. 
Huh. No. it is not duplicated. 
Vfio-pci provides the framework for extension than doing simple vfio-pci for extensions.
I am not debating here vdpa vs non vdpa yet again.

> And you lose all the advantages of
> vDPA in this way. The device should not be designed for a single type of
> software stack , it need to leave the decision to the hypervisor/cloud vendors.
>
It is left to the hypervisor/cloud user to decide to use vdpa or vfio or something else.
 
> 
> >     virtio device type (net/blk) and be future compatible with a
> >     single vfio stack using SR-IOV or other scalable device
> >     virtualization technology to map PCI devices to the guest VM.
> >     (as transitional or otherwise)
> >
> > Motivation/Background:
> > ----------------------
> > The existing virtio transitional PCI device is missing support for PCI
> > SR-IOV based devices. Currently it does not work beyond PCI PF, or as
> > software emulated device in reality. Currently it has below cited
> > system level limitations:
> >
> > [a] PCIe spec citation:
> > VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.
> >
> > [b] cpu arch citiation:
> > Intel 64 and IA-32 Architectures Software Developer’s Manual:
> > The processor’s I/O address space is separate and distinct from the
> > physical-memory address space. The I/O address space consists of 64K
> > individually addressable 8-bit I/O ports, numbered 0 through FFFFH.
> >
> > [c] PCIe spec citation:
> > If a bridge implements an I/O address range,...I/O address range will
> > be aligned to a 4 KB boundary.
> >
> > Above usecase requirements can be solved by PCI PF group owner
> > enabling the access to its group member PCI VFs legacy registers using
> > an admin virtqueue of the group owner PCI PF.
> >
> > Software usage example:
> > -----------------------
> > The most common way to use and map to the guest VM is by using vfio
> > driver framework in Linux kernel.
> >
> >                  +----------------------+
> >                  |pci_dev_id = 0x100X   |
> > +---------------|pci_rev_id = 0x0      |-----+
> > |vfio device    |BAR0 = I/O region     |     |
> > |               |Other attributes      |     |
> > |               +----------------------+     |
> > |                                            |
> > +   +--------------+     +-----------------+ |
> > |   |I/O BAR to AQ |     | Other vfio      | |
> > |   |rd/wr mapper  |     | functionalities | |
> > |   +--------------+     +-----------------+ |
> > |                                            |
> > +------+-------------------------+-----------+
> >         |                         |
> 
> 
> So the mapper here is actually the control path mediation layer which
> duplicates with vDPA.
> 
Yet again no. It implements PCI level abstraction.
It is not touching the whole QEMU layer and not at all getting involved in virtio device flow of understanding device reset, and device config space, cvq, features bits and more.
All of these were discussed in v0, lets not repeat.

> 
> >    +----+------------+       +----+------------+
> >    | +-----+         |       | PCI VF device A |
> >    | | AQ  |-------------+---->+-------------+ |
> >    | +-----+         |   |   | | legacy regs | |
> >    | PCI PF device   |   |   | +-------------+ |
> >    +-----------------+   |   +-----------------+
> >                          |
> >                          |   +----+------------+
> >                          |   | PCI VF device N |
> >                          +---->+-------------+ |
> >                              | | legacy regs | |
> >                              | +-------------+ |
> >                              +-----------------+
> >
> > 2. Virtio pci driver to bind to the listed device id and
> >     use it as native device in the host.
> 
> 
> How this can be done now?
> 
Currently a PCI VF binds to the virtio driver and without any vdpa layering, virtio net/blk etc devices are created on top of virtio PCI VF device.
Not sure I understood your question.

> > +\begin{lstlisting}
> > +struct virtio_admin_cmd_lreg_wr_data {
> > +	u8 offset; /* Starting byte offset of the register(s) to write */
> > +	u8 size; /* Number of bytes to write into the register. */
> > +	u8 register[];
> > +};
> > +\end{lstlisting}
> 
> 
> So this actually implements a transport, I wonder if it would be better
> (and simpler) to do it on top of the transport vq proposal:
> 
> https://lists.oasis-open.org/archives/virtio-comment/202208/msg00003.html
> 
I also wonder why TVQ cannot use AQ.

> Then it aligns with SIOV natively.
>
SIOV is not well defined spec, whenever it is defined, it can use AQ or TVQ.

We also discussed that mediation of hypervisor for control path in some use case is not desired, hence I will leave that discussion to the future when SIOV arrives.

^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-05 12:48       ` [virtio-comment] " Parav Pandit
@ 2023-05-06  2:24         ` Jason Wang
  -1 siblings, 0 replies; 54+ messages in thread
From: Jason Wang @ 2023-05-06  2:24 UTC (permalink / raw)
  To: Parav Pandit
  Cc: mst, virtio-dev, cohuck, david.edmondson, sburla, virtio-comment,
	Shahaf Shuler, Zhu, Lingshan

On Fri, May 5, 2023 at 8:49 PM Parav Pandit <parav@nvidia.com> wrote:
>
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Thursday, May 4, 2023 11:27 PM
> >
> > So the "single stack" is kind of misleading, you need a dedicated virtio
> > mediation layer which has different code path than a simpler vfio-pci which is
> > completely duplicated with vDPA subsystem.
> Huh. No. it is not duplicated.
> Vfio-pci provides the framework for extension than doing simple vfio-pci for extensions.

I'm not sure how to define simple here, do you mean mdev?

> I am not debating here vdpa vs non vdpa yet again.
>
> > And you lose all the advantages of
> > vDPA in this way. The device should not be designed for a single type of
> > software stack , it need to leave the decision to the hypervisor/cloud vendors.
> >
> It is left to the hypervisor/cloud user to decide to use vdpa or vfio or something else.
>
> >
> > >     virtio device type (net/blk) and be future compatible with a
> > >     single vfio stack using SR-IOV or other scalable device
> > >     virtualization technology to map PCI devices to the guest VM.
> > >     (as transitional or otherwise)
> > >
> > > Motivation/Background:
> > > ----------------------
> > > The existing virtio transitional PCI device is missing support for PCI
> > > SR-IOV based devices. Currently it does not work beyond PCI PF, or as
> > > software emulated device in reality. Currently it has below cited
> > > system level limitations:
> > >
> > > [a] PCIe spec citation:
> > > VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.
> > >
> > > [b] cpu arch citiation:
> > > Intel 64 and IA-32 Architectures Software Developer’s Manual:
> > > The processor’s I/O address space is separate and distinct from the
> > > physical-memory address space. The I/O address space consists of 64K
> > > individually addressable 8-bit I/O ports, numbered 0 through FFFFH.
> > >
> > > [c] PCIe spec citation:
> > > If a bridge implements an I/O address range,...I/O address range will
> > > be aligned to a 4 KB boundary.
> > >
> > > Above usecase requirements can be solved by PCI PF group owner
> > > enabling the access to its group member PCI VFs legacy registers using
> > > an admin virtqueue of the group owner PCI PF.
> > >
> > > Software usage example:
> > > -----------------------
> > > The most common way to use and map to the guest VM is by using vfio
> > > driver framework in Linux kernel.
> > >
> > >                  +----------------------+
> > >                  |pci_dev_id = 0x100X   |
> > > +---------------|pci_rev_id = 0x0      |-----+
> > > |vfio device    |BAR0 = I/O region     |     |
> > > |               |Other attributes      |     |
> > > |               +----------------------+     |
> > > |                                            |
> > > +   +--------------+     +-----------------+ |
> > > |   |I/O BAR to AQ |     | Other vfio      | |
> > > |   |rd/wr mapper  |     | functionalities | |
> > > |   +--------------+     +-----------------+ |
> > > |                                            |
> > > +------+-------------------------+-----------+
> > >         |                         |
> >
> >
> > So the mapper here is actually the control path mediation layer which
> > duplicates with vDPA.
> >
> Yet again no. It implements PCI level abstraction.
> It is not touching the whole QEMU layer and not at all getting involved in virtio device flow of understanding device reset, and device config space, cvq, features bits and more.

I think you miss the fact that QEMU can choose to not understand all
you mentioned here with the help of the general vdpa device.
Vhost-vDPA provides a much simpler device abstraction than vfio-pci.
If a cloud vendor wants a tiny/thin hypervisor layer, it can be done
through vDPA for sure.

> All of these were discussed in v0, lets not repeat.
>
> >
> > >    +----+------------+       +----+------------+
> > >    | +-----+         |       | PCI VF device A |
> > >    | | AQ  |-------------+---->+-------------+ |
> > >    | +-----+         |   |   | | legacy regs | |
> > >    | PCI PF device   |   |   | +-------------+ |
> > >    +-----------------+   |   +-----------------+
> > >                          |
> > >                          |   +----+------------+
> > >                          |   | PCI VF device N |
> > >                          +---->+-------------+ |
> > >                              | | legacy regs | |
> > >                              | +-------------+ |
> > >                              +-----------------+
> > >
> > > 2. Virtio pci driver to bind to the listed device id and
> > >     use it as native device in the host.
> >
> >
> > How this can be done now?
> >
> Currently a PCI VF binds to the virtio driver and without any vdpa layering, virtio net/blk etc devices are created on top of virtio PCI VF device.
> Not sure I understood your question.

I meant the current virtio-pci driver can use what you propose here.

>
> > > +\begin{lstlisting}
> > > +struct virtio_admin_cmd_lreg_wr_data {
> > > +   u8 offset; /* Starting byte offset of the register(s) to write */
> > > +   u8 size; /* Number of bytes to write into the register. */
> > > +   u8 register[];
> > > +};
> > > +\end{lstlisting}
> >
> >
> > So this actually implements a transport, I wonder if it would be better
> > (and simpler) to do it on top of the transport vq proposal:
> >
> > https://lists.oasis-open.org/archives/virtio-comment/202208/msg00003.html
> >
> I also wonder why TVQ cannot use AQ.

It can for sure, but whether using a single virtqueue type for both
administration and transport is still questionable.

>
> > Then it aligns with SIOV natively.
> >
> SIOV is not well defined spec, whenever it is defined, it can use AQ or TVQ.
>
> We also discussed that mediation of hypervisor for control path in some use case is not desired, hence I will leave that discussion to the future when SIOV arrives.

We need to plan it ahead. We don't want to end up with redundant
design. For example, this proposal is actually a partial transport
implementation. Transport virtqueue can do much better in this case.

Thanks


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-06  2:24         ` Jason Wang
  0 siblings, 0 replies; 54+ messages in thread
From: Jason Wang @ 2023-05-06  2:24 UTC (permalink / raw)
  To: Parav Pandit
  Cc: mst, virtio-dev, cohuck, david.edmondson, sburla, virtio-comment,
	Shahaf Shuler, Zhu, Lingshan

On Fri, May 5, 2023 at 8:49 PM Parav Pandit <parav@nvidia.com> wrote:
>
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Thursday, May 4, 2023 11:27 PM
> >
> > So the "single stack" is kind of misleading, you need a dedicated virtio
> > mediation layer which has different code path than a simpler vfio-pci which is
> > completely duplicated with vDPA subsystem.
> Huh. No. it is not duplicated.
> Vfio-pci provides the framework for extension than doing simple vfio-pci for extensions.

I'm not sure how to define simple here, do you mean mdev?

> I am not debating here vdpa vs non vdpa yet again.
>
> > And you lose all the advantages of
> > vDPA in this way. The device should not be designed for a single type of
> > software stack , it need to leave the decision to the hypervisor/cloud vendors.
> >
> It is left to the hypervisor/cloud user to decide to use vdpa or vfio or something else.
>
> >
> > >     virtio device type (net/blk) and be future compatible with a
> > >     single vfio stack using SR-IOV or other scalable device
> > >     virtualization technology to map PCI devices to the guest VM.
> > >     (as transitional or otherwise)
> > >
> > > Motivation/Background:
> > > ----------------------
> > > The existing virtio transitional PCI device is missing support for PCI
> > > SR-IOV based devices. Currently it does not work beyond PCI PF, or as
> > > software emulated device in reality. Currently it has below cited
> > > system level limitations:
> > >
> > > [a] PCIe spec citation:
> > > VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.
> > >
> > > [b] cpu arch citiation:
> > > Intel 64 and IA-32 Architectures Software Developer’s Manual:
> > > The processor’s I/O address space is separate and distinct from the
> > > physical-memory address space. The I/O address space consists of 64K
> > > individually addressable 8-bit I/O ports, numbered 0 through FFFFH.
> > >
> > > [c] PCIe spec citation:
> > > If a bridge implements an I/O address range,...I/O address range will
> > > be aligned to a 4 KB boundary.
> > >
> > > Above usecase requirements can be solved by PCI PF group owner
> > > enabling the access to its group member PCI VFs legacy registers using
> > > an admin virtqueue of the group owner PCI PF.
> > >
> > > Software usage example:
> > > -----------------------
> > > The most common way to use and map to the guest VM is by using vfio
> > > driver framework in Linux kernel.
> > >
> > >                  +----------------------+
> > >                  |pci_dev_id = 0x100X   |
> > > +---------------|pci_rev_id = 0x0      |-----+
> > > |vfio device    |BAR0 = I/O region     |     |
> > > |               |Other attributes      |     |
> > > |               +----------------------+     |
> > > |                                            |
> > > +   +--------------+     +-----------------+ |
> > > |   |I/O BAR to AQ |     | Other vfio      | |
> > > |   |rd/wr mapper  |     | functionalities | |
> > > |   +--------------+     +-----------------+ |
> > > |                                            |
> > > +------+-------------------------+-----------+
> > >         |                         |
> >
> >
> > So the mapper here is actually the control path mediation layer which
> > duplicates with vDPA.
> >
> Yet again no. It implements PCI level abstraction.
> It is not touching the whole QEMU layer and not at all getting involved in virtio device flow of understanding device reset, and device config space, cvq, features bits and more.

I think you miss the fact that QEMU can choose to not understand all
you mentioned here with the help of the general vdpa device.
Vhost-vDPA provides a much simpler device abstraction than vfio-pci.
If a cloud vendor wants a tiny/thin hypervisor layer, it can be done
through vDPA for sure.

> All of these were discussed in v0, lets not repeat.
>
> >
> > >    +----+------------+       +----+------------+
> > >    | +-----+         |       | PCI VF device A |
> > >    | | AQ  |-------------+---->+-------------+ |
> > >    | +-----+         |   |   | | legacy regs | |
> > >    | PCI PF device   |   |   | +-------------+ |
> > >    +-----------------+   |   +-----------------+
> > >                          |
> > >                          |   +----+------------+
> > >                          |   | PCI VF device N |
> > >                          +---->+-------------+ |
> > >                              | | legacy regs | |
> > >                              | +-------------+ |
> > >                              +-----------------+
> > >
> > > 2. Virtio pci driver to bind to the listed device id and
> > >     use it as native device in the host.
> >
> >
> > How this can be done now?
> >
> Currently a PCI VF binds to the virtio driver and without any vdpa layering, virtio net/blk etc devices are created on top of virtio PCI VF device.
> Not sure I understood your question.

I meant the current virtio-pci driver can use what you propose here.

>
> > > +\begin{lstlisting}
> > > +struct virtio_admin_cmd_lreg_wr_data {
> > > +   u8 offset; /* Starting byte offset of the register(s) to write */
> > > +   u8 size; /* Number of bytes to write into the register. */
> > > +   u8 register[];
> > > +};
> > > +\end{lstlisting}
> >
> >
> > So this actually implements a transport, I wonder if it would be better
> > (and simpler) to do it on top of the transport vq proposal:
> >
> > https://lists.oasis-open.org/archives/virtio-comment/202208/msg00003.html
> >
> I also wonder why TVQ cannot use AQ.

It can for sure, but whether using a single virtqueue type for both
administration and transport is still questionable.

>
> > Then it aligns with SIOV natively.
> >
> SIOV is not well defined spec, whenever it is defined, it can use AQ or TVQ.
>
> We also discussed that mediation of hypervisor for control path in some use case is not desired, hence I will leave that discussion to the future when SIOV arrives.

We need to plan it ahead. We don't want to end up with redundant
design. For example, this proposal is actually a partial transport
implementation. Transport virtqueue can do much better in this case.

Thanks


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-dev] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
  2023-05-06  2:24         ` [virtio-comment] " Jason Wang
@ 2023-05-06  2:25           ` Jason Wang
  -1 siblings, 0 replies; 54+ messages in thread
From: Jason Wang @ 2023-05-06  2:25 UTC (permalink / raw)
  To: Parav Pandit
  Cc: mst, virtio-dev, cohuck, david.edmondson, sburla, virtio-comment,
	Shahaf Shuler, Zhu, Lingshan

On Sat, May 6, 2023 at 10:24 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, May 5, 2023 at 8:49 PM Parav Pandit <parav@nvidia.com> wrote:
> >
> >
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Thursday, May 4, 2023 11:27 PM
> > >
> > > So the "single stack" is kind of misleading, you need a dedicated virtio
> > > mediation layer which has different code path than a simpler vfio-pci which is
> > > completely duplicated with vDPA subsystem.
> > Huh. No. it is not duplicated.
> > Vfio-pci provides the framework for extension than doing simple vfio-pci for extensions.
>
> I'm not sure how to define simple here, do you mean mdev?
>
> > I am not debating here vdpa vs non vdpa yet again.
> >
> > > And you lose all the advantages of
> > > vDPA in this way. The device should not be designed for a single type of
> > > software stack , it need to leave the decision to the hypervisor/cloud vendors.
> > >
> > It is left to the hypervisor/cloud user to decide to use vdpa or vfio or something else.
> >
> > >
> > > >     virtio device type (net/blk) and be future compatible with a
> > > >     single vfio stack using SR-IOV or other scalable device
> > > >     virtualization technology to map PCI devices to the guest VM.
> > > >     (as transitional or otherwise)
> > > >
> > > > Motivation/Background:
> > > > ----------------------
> > > > The existing virtio transitional PCI device is missing support for PCI
> > > > SR-IOV based devices. Currently it does not work beyond PCI PF, or as
> > > > software emulated device in reality. Currently it has below cited
> > > > system level limitations:
> > > >
> > > > [a] PCIe spec citation:
> > > > VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.
> > > >
> > > > [b] cpu arch citiation:
> > > > Intel 64 and IA-32 Architectures Software Developer’s Manual:
> > > > The processor’s I/O address space is separate and distinct from the
> > > > physical-memory address space. The I/O address space consists of 64K
> > > > individually addressable 8-bit I/O ports, numbered 0 through FFFFH.
> > > >
> > > > [c] PCIe spec citation:
> > > > If a bridge implements an I/O address range,...I/O address range will
> > > > be aligned to a 4 KB boundary.
> > > >
> > > > Above usecase requirements can be solved by PCI PF group owner
> > > > enabling the access to its group member PCI VFs legacy registers using
> > > > an admin virtqueue of the group owner PCI PF.
> > > >
> > > > Software usage example:
> > > > -----------------------
> > > > The most common way to use and map to the guest VM is by using vfio
> > > > driver framework in Linux kernel.
> > > >
> > > >                  +----------------------+
> > > >                  |pci_dev_id = 0x100X   |
> > > > +---------------|pci_rev_id = 0x0      |-----+
> > > > |vfio device    |BAR0 = I/O region     |     |
> > > > |               |Other attributes      |     |
> > > > |               +----------------------+     |
> > > > |                                            |
> > > > +   +--------------+     +-----------------+ |
> > > > |   |I/O BAR to AQ |     | Other vfio      | |
> > > > |   |rd/wr mapper  |     | functionalities | |
> > > > |   +--------------+     +-----------------+ |
> > > > |                                            |
> > > > +------+-------------------------+-----------+
> > > >         |                         |
> > >
> > >
> > > So the mapper here is actually the control path mediation layer which
> > > duplicates with vDPA.
> > >
> > Yet again no. It implements PCI level abstraction.
> > It is not touching the whole QEMU layer and not at all getting involved in virtio device flow of understanding device reset, and device config space, cvq, features bits and more.
>
> I think you miss the fact that QEMU can choose to not understand all
> you mentioned here with the help of the general vdpa device.
> Vhost-vDPA provides a much simpler device abstraction than vfio-pci.
> If a cloud vendor wants a tiny/thin hypervisor layer, it can be done
> through vDPA for sure.
>
> > All of these were discussed in v0, lets not repeat.
> >
> > >
> > > >    +----+------------+       +----+------------+
> > > >    | +-----+         |       | PCI VF device A |
> > > >    | | AQ  |-------------+---->+-------------+ |
> > > >    | +-----+         |   |   | | legacy regs | |
> > > >    | PCI PF device   |   |   | +-------------+ |
> > > >    +-----------------+   |   +-----------------+
> > > >                          |
> > > >                          |   +----+------------+
> > > >                          |   | PCI VF device N |
> > > >                          +---->+-------------+ |
> > > >                              | | legacy regs | |
> > > >                              | +-------------+ |
> > > >                              +-----------------+
> > > >
> > > > 2. Virtio pci driver to bind to the listed device id and
> > > >     use it as native device in the host.
> > >
> > >
> > > How this can be done now?
> > >
> > Currently a PCI VF binds to the virtio driver and without any vdpa layering, virtio net/blk etc devices are created on top of virtio PCI VF device.
> > Not sure I understood your question.
>
> I meant the current virtio-pci driver can use what you propose here.

Actually, I meant "can't".

Thanks

>
> >
> > > > +\begin{lstlisting}
> > > > +struct virtio_admin_cmd_lreg_wr_data {
> > > > +   u8 offset; /* Starting byte offset of the register(s) to write */
> > > > +   u8 size; /* Number of bytes to write into the register. */
> > > > +   u8 register[];
> > > > +};
> > > > +\end{lstlisting}
> > >
> > >
> > > So this actually implements a transport, I wonder if it would be better
> > > (and simpler) to do it on top of the transport vq proposal:
> > >
> > > https://lists.oasis-open.org/archives/virtio-comment/202208/msg00003.html
> > >
> > I also wonder why TVQ cannot use AQ.
>
> It can for sure, but whether using a single virtqueue type for both
> administration and transport is still questionable.
>
> >
> > > Then it aligns with SIOV natively.
> > >
> > SIOV is not well defined spec, whenever it is defined, it can use AQ or TVQ.
> >
> > We also discussed that mediation of hypervisor for control path in some use case is not desired, hence I will leave that discussion to the future when SIOV arrives.
>
> We need to plan it ahead. We don't want to end up with redundant
> design. For example, this proposal is actually a partial transport
> implementation. Transport virtqueue can do much better in this case.
>
> Thanks


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 54+ messages in thread

* [virtio-comment] Re: [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands
@ 2023-05-06  2:25           ` Jason Wang
  0 siblings, 0 replies; 54+ messages in thread
From: Jason Wang @ 2023-05-06  2:25 UTC (permalink / raw)
  To: Parav Pandit
  Cc: mst, virtio-dev, cohuck, david.edmondson, sburla, virtio-comment,
	Shahaf Shuler, Zhu, Lingshan

On Sat, May 6, 2023 at 10:24 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, May 5, 2023 at 8:49 PM Parav Pandit <parav@nvidia.com> wrote:
> >
> >
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Thursday, May 4, 2023 11:27 PM
> > >
> > > So the "single stack" is kind of misleading, you need a dedicated virtio
> > > mediation layer which has different code path than a simpler vfio-pci which is
> > > completely duplicated with vDPA subsystem.
> > Huh. No. it is not duplicated.
> > Vfio-pci provides the framework for extension than doing simple vfio-pci for extensions.
>
> I'm not sure how to define simple here, do you mean mdev?
>
> > I am not debating here vdpa vs non vdpa yet again.
> >
> > > And you lose all the advantages of
> > > vDPA in this way. The device should not be designed for a single type of
> > > software stack , it need to leave the decision to the hypervisor/cloud vendors.
> > >
> > It is left to the hypervisor/cloud user to decide to use vdpa or vfio or something else.
> >
> > >
> > > >     virtio device type (net/blk) and be future compatible with a
> > > >     single vfio stack using SR-IOV or other scalable device
> > > >     virtualization technology to map PCI devices to the guest VM.
> > > >     (as transitional or otherwise)
> > > >
> > > > Motivation/Background:
> > > > ----------------------
> > > > The existing virtio transitional PCI device is missing support for PCI
> > > > SR-IOV based devices. Currently it does not work beyond PCI PF, or as
> > > > software emulated device in reality. Currently it has below cited
> > > > system level limitations:
> > > >
> > > > [a] PCIe spec citation:
> > > > VFs do not support I/O Space and thus VF BARs shall not indicate I/O Space.
> > > >
> > > > [b] cpu arch citiation:
> > > > Intel 64 and IA-32 Architectures Software Developer’s Manual:
> > > > The processor’s I/O address space is separate and distinct from the
> > > > physical-memory address space. The I/O address space consists of 64K
> > > > individually addressable 8-bit I/O ports, numbered 0 through FFFFH.
> > > >
> > > > [c] PCIe spec citation:
> > > > If a bridge implements an I/O address range,...I/O address range will
> > > > be aligned to a 4 KB boundary.
> > > >
> > > > Above usecase requirements can be solved by PCI PF group owner
> > > > enabling the access to its group member PCI VFs legacy registers using
> > > > an admin virtqueue of the group owner PCI PF.
> > > >
> > > > Software usage example:
> > > > -----------------------
> > > > The most common way to use and map to the guest VM is by using vfio
> > > > driver framework in Linux kernel.
> > > >
> > > >                  +----------------------+
> > > >                  |pci_dev_id = 0x100X   |
> > > > +---------------|pci_rev_id = 0x0      |-----+
> > > > |vfio device    |BAR0 = I/O region     |     |
> > > > |               |Other attributes      |     |
> > > > |               +----------------------+     |
> > > > |                                            |
> > > > +   +--------------+     +-----------------+ |
> > > > |   |I/O BAR to AQ |     | Other vfio      | |
> > > > |   |rd/wr mapper  |     | functionalities | |
> > > > |   +--------------+     +-----------------+ |
> > > > |                                            |
> > > > +------+-------------------------+-----------+
> > > >         |                         |
> > >
> > >
> > > So the mapper here is actually the control path mediation layer which
> > > duplicates with vDPA.
> > >
> > Yet again no. It implements PCI level abstraction.
> > It is not touching the whole QEMU layer and not at all getting involved in virtio device flow of understanding device reset, and device config space, cvq, features bits and more.
>
> I think you miss the fact that QEMU can choose to not understand all
> you mentioned here with the help of the general vdpa device.
> Vhost-vDPA provides a much simpler device abstraction than vfio-pci.
> If a cloud vendor wants a tiny/thin hypervisor layer, it can be done
> through vDPA for sure.
>
> > All of these were discussed in v0, lets not repeat.
> >
> > >
> > > >    +----+------------+       +----+------------+
> > > >    | +-----+         |       | PCI VF device A |
> > > >    | | AQ  |-------------+---->+-------------+ |
> > > >    | +-----+         |   |   | | legacy regs | |
> > > >    | PCI PF device   |   |   | +-------------+ |
> > > >    +-----------------+   |   +-----------------+
> > > >                          |
> > > >                          |   +----+------------+
> > > >                          |   | PCI VF device N |
> > > >                          +---->+-------------+ |
> > > >                              | | legacy regs | |
> > > >                              | +-------------+ |
> > > >                              +-----------------+
> > > >
> > > > 2. Virtio pci driver to bind to the listed device id and
> > > >     use it as native device in the host.
> > >
> > >
> > > How this can be done now?
> > >
> > Currently a PCI VF binds to the virtio driver and without any vdpa layering, virtio net/blk etc devices are created on top of virtio PCI VF device.
> > Not sure I understood your question.
>
> I meant the current virtio-pci driver can use what you propose here.

Actually, I meant "can't".

Thanks

>
> >
> > > > +\begin{lstlisting}
> > > > +struct virtio_admin_cmd_lreg_wr_data {
> > > > +   u8 offset; /* Starting byte offset of the register(s) to write */
> > > > +   u8 size; /* Number of bytes to write into the register. */
> > > > +   u8 register[];
> > > > +};
> > > > +\end{lstlisting}
> > >
> > >
> > > So this actually implements a transport, I wonder if it would be better
> > > (and simpler) to do it on top of the transport vq proposal:
> > >
> > > https://lists.oasis-open.org/archives/virtio-comment/202208/msg00003.html
> > >
> > I also wonder why TVQ cannot use AQ.
>
> It can for sure, but whether using a single virtqueue type for both
> administration and transport is still questionable.
>
> >
> > > Then it aligns with SIOV natively.
> > >
> > SIOV is not well defined spec, whenever it is defined, it can use AQ or TVQ.
> >
> > We also discussed that mediation of hypervisor for control path in some use case is not desired, hence I will leave that discussion to the future when SIOV arrives.
>
> We need to plan it ahead. We don't want to end up with redundant
> design. For example, this proposal is actually a partial transport
> implementation. Transport virtqueue can do much better in this case.
>
> Thanks


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/


^ permalink raw reply	[flat|nested] 54+ messages in thread

end of thread, other threads:[~2023-05-06  2:25 UTC | newest]

Thread overview: 54+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-03  3:26 [virtio-dev] [PATCH v1 0/2] transport-pci: Introduce legacy registers access using AQ Parav Pandit
2023-05-03  3:26 ` [virtio-comment] " Parav Pandit
2023-05-03  3:26 ` [virtio-dev] [PATCH v1 1/2] transport-pci: Introduce legacy registers access commands Parav Pandit
2023-05-03  3:26   ` [virtio-comment] " Parav Pandit
2023-05-03  5:42   ` [virtio-dev] " Michael S. Tsirkin
2023-05-03  5:42     ` [virtio-comment] " Michael S. Tsirkin
2023-05-03 14:47     ` [virtio-dev] " Parav Pandit
2023-05-03 14:47       ` [virtio-comment] " Parav Pandit
2023-05-03 16:48       ` [virtio-dev] " Michael S. Tsirkin
2023-05-03 16:48         ` [virtio-comment] " Michael S. Tsirkin
2023-05-03 17:21         ` [virtio-dev] " Parav Pandit
2023-05-03 17:21           ` [virtio-comment] " Parav Pandit
2023-05-04  6:30           ` [virtio-dev] " Michael S. Tsirkin
2023-05-04  6:30             ` [virtio-comment] " Michael S. Tsirkin
2023-05-03  5:50   ` [virtio-dev] " Michael S. Tsirkin
2023-05-03  5:50     ` [virtio-comment] " Michael S. Tsirkin
2023-05-03 14:49     ` [virtio-dev] " Parav Pandit
2023-05-03 14:49       ` [virtio-comment] " Parav Pandit
2023-05-03 16:49       ` [virtio-dev] " Michael S. Tsirkin
2023-05-03 16:49         ` [virtio-comment] " Michael S. Tsirkin
2023-05-03 17:23         ` [virtio-dev] " Parav Pandit
2023-05-03 17:23           ` [virtio-comment] " Parav Pandit
2023-05-04  6:30           ` [virtio-dev] " Michael S. Tsirkin
2023-05-04  6:30             ` [virtio-comment] " Michael S. Tsirkin
2023-05-03 19:21   ` [virtio-dev] " Michael S. Tsirkin
2023-05-03 19:21     ` [virtio-comment] " Michael S. Tsirkin
2023-05-03 19:38     ` [virtio-dev] " Parav Pandit
2023-05-03 19:38       ` [virtio-comment] " Parav Pandit
2023-05-03 20:08       ` Michael S. Tsirkin
2023-05-03 20:08         ` [virtio-comment] " Michael S. Tsirkin
2023-05-03 20:13         ` Parav Pandit
2023-05-03 20:13           ` [virtio-comment] " Parav Pandit
2023-05-05  3:26   ` Jason Wang
2023-05-05  3:26     ` [virtio-comment] " Jason Wang
2023-05-05 12:48     ` [virtio-dev] " Parav Pandit
2023-05-05 12:48       ` [virtio-comment] " Parav Pandit
2023-05-06  2:24       ` [virtio-dev] " Jason Wang
2023-05-06  2:24         ` [virtio-comment] " Jason Wang
2023-05-06  2:25         ` [virtio-dev] " Jason Wang
2023-05-06  2:25           ` [virtio-comment] " Jason Wang
2023-05-03  3:26 ` [virtio-dev] [PATCH v1 2/2] transport-pci: Add legacy register access conformance section Parav Pandit
2023-05-03  3:26   ` [virtio-comment] " Parav Pandit
2023-05-03  5:48   ` [virtio-dev] " Michael S. Tsirkin
2023-05-03  5:48     ` [virtio-comment] " Michael S. Tsirkin
2023-05-03 14:53     ` [virtio-dev] " Parav Pandit
2023-05-03 14:53       ` Parav Pandit
2023-05-03 19:18       ` [virtio-dev] " Michael S. Tsirkin
2023-05-03 19:18         ` Michael S. Tsirkin
2023-05-03 19:56         ` [virtio-dev] " Parav Pandit
2023-05-03 19:56           ` Parav Pandit
2023-05-03 10:16 ` [virtio-dev] Re: [PATCH v1 0/2] transport-pci: Introduce legacy registers access using AQ Michael S. Tsirkin
2023-05-03 10:16   ` [virtio-comment] " Michael S. Tsirkin
2023-05-03 14:57   ` [virtio-dev] " Parav Pandit
2023-05-03 14:57     ` [virtio-comment] " Parav Pandit

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.